diff mbox series

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

Message ID 20181107040010.27436-2-chris.packham@alliedtelesis.co.nz (mailing list archive)
State Superseded
Headers show
Series hwmon: (adt7475): inverted PWM output | expand

Commit Message

Chris Packham Nov. 7, 2018, 4 a.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>
---
 .../devicetree/bindings/hwmon/adt7475.txt     | 22 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.txt   |  4 ----
 2 files changed, 22 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.txt

Comments

Guenter Roeck Nov. 7, 2018, 4:24 a.m. UTC | #1 
On 11/6/18 8:00 PM, 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>
> ---
>   .../devicetree/bindings/hwmon/adt7475.txt     | 22 +++++++++++++++++++
>   .../devicetree/bindings/trivial-devices.txt   |  4 ----
>   2 files changed, 22 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..79255439e157
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
> @@ -0,0 +1,22 @@
> +*ADT7475 hwmon sensor.
> +
> +Required properties:
> +- compatible: One of
> +	"adi,adt7473"
> +	"adi,adt7475"
> +	"adi,adt7476"
> +	"adi,adt7490"
> +
> +- reg: I2C address
> +
> +optional properties:
> +
> +- invert-pwm: This configures the PWM to use logic low for 100% duty cycle.
> +
> +Example:
> +
> +hwmon@2e {
> +	compatible = ;
> +	reg = <0x2e>;
> +	invert-pwm;

If I understand correctly, the flag is set per pwm channel. A single global
property seems inappropriate.

Guenter

> +};
> 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
>
Chris Packham Nov. 7, 2018, 4:27 a.m. UTC | #2 
On 7/11/18 5:24 PM, Guenter Roeck wrote:
> On 11/6/18 8:00 PM, 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>
>> ---
>>    .../devicetree/bindings/hwmon/adt7475.txt     | 22 +++++++++++++++++++
>>    .../devicetree/bindings/trivial-devices.txt   |  4 ----
>>    2 files changed, 22 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..79255439e157
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
>> @@ -0,0 +1,22 @@
>> +*ADT7475 hwmon sensor.
>> +
>> +Required properties:
>> +- compatible: One of
>> +	"adi,adt7473"
>> +	"adi,adt7475"
>> +	"adi,adt7476"
>> +	"adi,adt7490"
>> +
>> +- reg: I2C address
>> +
>> +optional properties:
>> +
>> +- invert-pwm: This configures the PWM to use logic low for 100% duty cycle.
>> +
>> +Example:
>> +
>> +hwmon@2e {
>> +	compatible = ;
>> +	reg = <0x2e>;
>> +	invert-pwm;
> 
> If I understand correctly, the flag is set per pwm channel. A single global
> property seems inappropriate.

Yes it is per-channel. But I was having a hard time imagining a hardware 
design that would use both inverted and non-inverted at the same time.

Do you have a preference for how the properties should look?

   invert-pwm[123]?

   invert-pwm = <0x7>;?

> 
> Guenter
> 
>> +};
>> 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
>>
> 
>
Guenter Roeck Nov. 7, 2018, 5:07 a.m. UTC | #3 
On 11/6/18 8:27 PM, Chris Packham wrote:
> On 7/11/18 5:24 PM, Guenter Roeck wrote:
>> On 11/6/18 8:00 PM, 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>
>>> ---
>>>     .../devicetree/bindings/hwmon/adt7475.txt     | 22 +++++++++++++++++++
>>>     .../devicetree/bindings/trivial-devices.txt   |  4 ----
>>>     2 files changed, 22 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..79255439e157
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
>>> @@ -0,0 +1,22 @@
>>> +*ADT7475 hwmon sensor.
>>> +
>>> +Required properties:
>>> +- compatible: One of
>>> +	"adi,adt7473"
>>> +	"adi,adt7475"
>>> +	"adi,adt7476"
>>> +	"adi,adt7490"
>>> +
>>> +- reg: I2C address
>>> +
>>> +optional properties:
>>> +
>>> +- invert-pwm: This configures the PWM to use logic low for 100% duty cycle.
>>> +
>>> +Example:
>>> +
>>> +hwmon@2e {
>>> +	compatible = ;
>>> +	reg = <0x2e>;
>>> +	invert-pwm;
>>
>> If I understand correctly, the flag is set per pwm channel. A single global
>> property seems inappropriate.
> 
> Yes it is per-channel. But I was having a hard time imagining a hardware
> design that would use both inverted and non-inverted at the same time.
> 

People can be inventive. Sometimes too much so.

> Do you have a preference for how the properties should look?
> 
>     invert-pwm[123]?
> 
>     invert-pwm = <0x7>;?
>
Ultimately we'll have to find a generic way of defining devicetree properties for
hardware monitoring devices, not just for pwm but for all sensor types. A sub-node
per channel seems to be the most likely answer, but I think that is a long way off
(and will require lengthy discussions about acceptable properties).

I was looking into pwm DT properties, but they define a set of flags
(PWM_POLARITY_INVERTED, PWM_POLARITY_NORMAL).

The g762 driver uses "pwm_polarity". Underscore - hmm. Other drivers use module
parameters, sysfs attributes (pwmX_invert in asc7621 driver), or platform data
(g762, max6639).

Maybe pwm-polarity = <PWM_POLARITY_NORMAL | PWM_POLARITY_INVERTED> ?
No idea if that makes sense. Or just a boolean pwm-polarity-inverted.

Thinking about it per channel vs. per chip ... other drivers also seem to
use a single property / attribute for the entire chip, so that is fine here
as well. We can always extend it if needed.

Anyway, I am fine with whatever Rob accepts.

Guenter

>>
>> Guenter
>>
>>> +};
>>> 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
>>>
>>
>>
> 
>
Chris Packham Nov. 8, 2018, 7:55 p.m. UTC | #4 
On 7/11/18 6:08 PM, Guenter Roeck wrote:
> On 11/6/18 8:27 PM, Chris Packham wrote:
>> On 7/11/18 5:24 PM, Guenter Roeck wrote:
>>> On 11/6/18 8:00 PM, 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>
>>>> ---
>>>>      .../devicetree/bindings/hwmon/adt7475.txt     | 22 +++++++++++++++++++
>>>>      .../devicetree/bindings/trivial-devices.txt   |  4 ----
>>>>      2 files changed, 22 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..79255439e157
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
>>>> @@ -0,0 +1,22 @@
>>>> +*ADT7475 hwmon sensor.
>>>> +
>>>> +Required properties:
>>>> +- compatible: One of
>>>> +	"adi,adt7473"
>>>> +	"adi,adt7475"
>>>> +	"adi,adt7476"
>>>> +	"adi,adt7490"
>>>> +
>>>> +- reg: I2C address
>>>> +
>>>> +optional properties:
>>>> +
>>>> +- invert-pwm: This configures the PWM to use logic low for 100% duty cycle.
>>>> +
>>>> +Example:
>>>> +
>>>> +hwmon@2e {
>>>> +	compatible = ;
>>>> +	reg = <0x2e>;
>>>> +	invert-pwm;
>>>
>>> If I understand correctly, the flag is set per pwm channel. A single global
>>> property seems inappropriate.
>>
>> Yes it is per-channel. But I was having a hard time imagining a hardware
>> design that would use both inverted and non-inverted at the same time.
>>
> 
> People can be inventive. Sometimes too much so.
> 
>> Do you have a preference for how the properties should look?
>>
>>      invert-pwm[123]?
>>
>>      invert-pwm = <0x7>;?
>>
> Ultimately we'll have to find a generic way of defining devicetree properties for
> hardware monitoring devices, not just for pwm but for all sensor types. A sub-node
> per channel seems to be the most likely answer, but I think that is a long way off
> (and will require lengthy discussions about acceptable properties).
> 
> I was looking into pwm DT properties, but they define a set of flags
> (PWM_POLARITY_INVERTED, PWM_POLARITY_NORMAL).
> 
> The g762 driver uses "pwm_polarity". Underscore - hmm. Other drivers use module
> parameters, sysfs attributes (pwmX_invert in asc7621 driver), or platform data
> (g762, max6639).
> 
> Maybe pwm-polarity = <PWM_POLARITY_NORMAL | PWM_POLARITY_INVERTED> ?
> No idea if that makes sense. Or just a boolean pwm-polarity-inverted.

That would allow undoing an earlier invert which is something my initial 
implementation lacked.

In the future we could also make this a list of values to deal with 
multiple pwm outputs

e.g. pwm-polarity = <PWM_POLARITY_NORMAL>, <PWM_POLARITY_NORMAL> 
<PWM_POLARITY_INVERTED>;

I'll look at a v2 that does the first part for now.

> 
> Thinking about it per channel vs. per chip ... other drivers also seem to
> use a single property / attribute for the entire chip, so that is fine here
> as well. We can always extend it if needed.
> 
> Anyway, I am fine with whatever Rob accepts.
> 
> Guenter
> 
>>>
>>> Guenter
>>>
>>>> +};
>>>> 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
>>>>
>>>
>>>
>>
>>
> 
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.txt b/Documentation/devicetree/bindings/hwmon/adt7475.txt
new file mode 100644
index 000000000000..79255439e157
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.txt
@@ -0,0 +1,22 @@ 
+*ADT7475 hwmon sensor.
+
+Required properties:
+- compatible: One of
+	"adi,adt7473"
+	"adi,adt7475"
+	"adi,adt7476"
+	"adi,adt7490"
+
+- reg: I2C address
+
+optional properties:
+
+- invert-pwm: This configures the PWM to use logic low for 100% duty cycle.
+
+Example:
+
+hwmon@2e {
+	compatible = ;
+	reg = <0x2e>;
+	invert-pwm;
+};
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