Re: [PATCH v2 1/2] dt-bindings: hwmon: add binding documentation for adt7475

[Chris Packham]

On 20/11/18 10:40 AM, Rob Herring wrote:
> On Sun, Nov 18, 2018 at 4:56 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>
>> On 11/17/18 7:29 AM, Rob Herring wrote:
>>> On Fri, Nov 09, 2018 at 11:56:06AM +1300, Chris Packham wrote:
>>>> With the addition of the invert-pwm property the adt7475 needs its own
>>>> binding documentation rather being captured under trivial-devices.txt.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - use pwm-polarity attiribute to indicate normal or inverted polarity.
>>>>
>>>>    .../devicetree/bindings/hwmon/adt7475.txt     | 24 +++++++++++++++++++
>>>>    .../devicetree/bindings/trivial-devices.txt   |  4 ----
>>>>    2 files changed, 24 insertions(+), 4 deletions(-)
>>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.txt b/Documentation/devicetree/bindings/hwmon/adt7475.txt
>>>> new file mode 100644
>>>> index 000000000000..d9212b5e9036
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
>>>> @@ -0,0 +1,24 @@
>>>> +*ADT7475 hwmon sensor.
>>>> +
>>>> +Required properties:
>>>> +- compatible: One of
>>>> +    "adi,adt7473"
>>>> +    "adi,adt7475"
>>>> +    "adi,adt7476"
>>>> +    "adi,adt7490"
>>>> +
>>>> +- reg: I2C address
>>>> +
>>>> +optional properties:
>>>> +
>>>> +- pwm-polarity: This configures the polarity of the PWM. 0
>>>> +            (PWM_POLARITY_NORMAL) uses logic high for 100% duty cycle. 1
>>>> +                (PWM_POLARITY_INVERSED) uses logic low for 100% duty cycle.
>>>
>>> So we're using part of the PWM binding, but none of the rest of it? What
>>> is the PWM connection here?
>>> The chip has a built-in PWM controller to control fan speeds. PWM frequency
>> as well as PWM polarity are configurable. I had suggested to use the terminology
>> from Documentation/devicetree/bindings/pwm/pwm.txt. Other than terminology there
>> is no connection to a standard pwm controller.
> 
> Ah, a fan controller. Please make that clear.
> 
> The fan related bindings need a bit of work IMO. We have the start of
> something somewhat common with aspeed and npcm bindings, but they
> could use a bit more work. We need the fan and controller bindings to
> be somewhat separate where a fan node for a given type of fan doesn't
> depend on the controller node.
> 
> AIUI, all fans are not the same, so does this controller only work
> with 1 type of fan and not need any more board level config? Isn't fan
> speed not linear with PWM duty cycle and need some sort of map.

That level of configuration is done in userland via the sysfs API (e.g 
lm-sensors). I could have added the pwm output inversion via sysfs but I 
erred on the side of the polarity being an artifact of the hardware 
design that is fairly static.

Things like what thresholds to configure and whether to use automatic 
pwm control tend to be much more fluid. Then theres pathological cases 
where other external factors need to be included in the fan control (my 
particular use case is a network switch acting as a PoE PSE so I not 
only need to take into account the ambient temperature but how much PoE 
power is being delivered to connected PDs),

>> Another option/possibility would be to use a boolean, such as pwm-polarity-inverted.
>> We are open to suggestions.
>>
>>> It sounds like this is common for sensors, so this should be documented
>>> somewhere common.
>>>
>> Sure, no problem. Any suggestion where ?
> 
> hwmon/fan.txt (that doesn't exist yet.)
> 
> Probably all of bindings/hwmon/ should be split up by class of device.

Most of the hwmon chips I've been dealing with lately tend to be multi 
function (e.g. temperature, voltage, fan tach and fan control). The 
single function devices tend to land in trivial-devices.txt because 
there's not much to document other than the compatible string.