[v2,1/2] dt-bindings: hwmon: add binding documentation for adt7475
diff mbox series

Message ID 20181108225607.7521-2-chris.packham@alliedtelesis.co.nz
State Under Review
Headers show
Series
  • hwmon: (adt7475): inverted PWM output
Related show

Commit Message

Chris Packham Nov. 8, 2018, 10:56 p.m. UTC
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@alliedtelesis.co.nz>
---
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

Comments

Chris Packham Nov. 12, 2018, 12:34 a.m. UTC | #1
On 9/11/18 11:56 AM, 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@alliedtelesis.co.nz>
> ---
> 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.
> +
> +Example:
> +
> +hwmon@2e {
> +	compatible = ;

Oops. I should include a working example. I've fixed this locally but 
I'll hold off on sending v3 to see if there is any additional feedback.

> +	reg = <0x2e>;
> +	pwm-polarity = <1>;
> +};
> diff --git a/Documentation/devicetree/bindings/trivial-devices.txt b/Documentation/devicetree/bindings/trivial-devices.txt
> index 69c934aec13b..4f29100d6bbf 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/trivial-devices.txt
> @@ -14,10 +14,6 @@ ad,ad7414		SMBus/I2C Digital Temperature Sensor in 6-Pin SOT with SMBus Alert an
>   ad,adm9240		ADM9240:  Complete System Hardware Monitor for uProcessor-Based Systems
>   adi,adt7461		+/-1C TDM Extended Temp Range I.C
>   adt7461			+/-1C TDM Extended Temp Range I.C
> -adi,adt7473		+/-1C TDM Extended Temp Range I.C
> -adi,adt7475		+/-1C TDM Extended Temp Range I.C
> -adi,adt7476		+/-1C TDM Extended Temp Range I.C
> -adi,adt7490		+/-1C TDM Extended Temp Range I.C
>   adi,adxl345		Three-Axis Digital Accelerometer
>   adi,adxl346		Three-Axis Digital Accelerometer (backward-compatibility value "adi,adxl345" must be listed too)
>   ams,iaq-core		AMS iAQ-Core VOC Sensor
>
Rob Herring Nov. 17, 2018, 3:29 p.m. UTC | #2
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@alliedtelesis.co.nz>
> ---
> 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?

It sounds like this is common for sensors, so this should be documented 
somewhere common.

Rob
Guenter Roeck Nov. 18, 2018, 10:56 p.m. UTC | #3
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@alliedtelesis.co.nz>
>> ---
>> 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.

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 ?

Thanks,
Guenter
Rob Herring Nov. 19, 2018, 9:40 p.m. UTC | #4
On Sun, Nov 18, 2018 at 4:56 PM Guenter Roeck <linux@roeck-us.net> 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@alliedtelesis.co.nz>
> >> ---
> >> 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.

> 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.

Rob
Chris Packham Nov. 19, 2018, 9:55 p.m. UTC | #5
On 20/11/18 10:40 AM, Rob Herring wrote:
> On Sun, Nov 18, 2018 at 4:56 PM Guenter Roeck <linux@roeck-us.net> 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@alliedtelesis.co.nz>
>>>> ---
>>>> 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.

Patch
diff mbox series

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.
+
+Example:
+
+hwmon@2e {
+	compatible = ;
+	reg = <0x2e>;
+	pwm-polarity = <1>;
+};
diff --git a/Documentation/devicetree/bindings/trivial-devices.txt b/Documentation/devicetree/bindings/trivial-devices.txt
index 69c934aec13b..4f29100d6bbf 100644
--- a/Documentation/devicetree/bindings/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/trivial-devices.txt
@@ -14,10 +14,6 @@  ad,ad7414		SMBus/I2C Digital Temperature Sensor in 6-Pin SOT with SMBus Alert an
 ad,adm9240		ADM9240:  Complete System Hardware Monitor for uProcessor-Based Systems
 adi,adt7461		+/-1C TDM Extended Temp Range I.C
 adt7461			+/-1C TDM Extended Temp Range I.C
-adi,adt7473		+/-1C TDM Extended Temp Range I.C
-adi,adt7475		+/-1C TDM Extended Temp Range I.C
-adi,adt7476		+/-1C TDM Extended Temp Range I.C
-adi,adt7490		+/-1C TDM Extended Temp Range I.C
 adi,adxl345		Three-Axis Digital Accelerometer
 adi,adxl346		Three-Axis Digital Accelerometer (backward-compatibility value "adi,adxl345" must be listed too)
 ams,iaq-core		AMS iAQ-Core VOC Sensor