[PATCH 0/2] hwmon: (adt7475): inverted PWM output

[PATCH 0/2] hwmon: (adt7475): inverted PWM output

[Chris Packham]

Hi,

I have a hardware design that requires logic low PWM output. The adt7475
supports this but the current driver does not have any way of configuring this.
I've gone with a device-tree property as this is an artifact of the hardware
design and not something that might be configured via sysfs.

Chris Packham (2):
  dt-bindings: hwmon: add binding documentation for adt7475
  hwmon: (adt7475): add support for inverting the pwm output

 .../devicetree/bindings/hwmon/adt7475.txt     | 22 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.txt   |  4 ----
 drivers/hwmon/adt7475.c                       | 17 ++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.txt

-- 
2.19.1

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

[Chris Packham]

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;
+};
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
-- 
2.19.1

[PATCH 2/2] hwmon: (adt7475): add support for inverting the pwm output

[Chris Packham]

Add a "invert-pwm" device-tree property to allow hardware designs to use
inverted logic on the PWM output. We intentionally preserve the invert
PWM output bit if the property is not found to allow for
bootloaders/bios which may have configured this earlier.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/hwmon/adt7475.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index f4c7516eb989..85e4dc674449 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1536,9 +1536,22 @@ static int adt7475_update_limits(struct i2c_client *client)
 	return 0;
 }
 
+static int adt7475_invert_pwm(struct i2c_client *client, int index)
+{
+	int ret;
+
+	ret = adt7475_read(PWM_CONFIG_REG(index));
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index),
+					 ret | BIT(4));
+}
+
 static int adt7475_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
+	struct device_node *of_node = client->dev.of_node;
 	enum chips chip;
 	static const char * const names[] = {
 		[adt7473] = "ADT7473",
@@ -1638,6 +1651,10 @@ static int adt7475_probe(struct i2c_client *client,
 	for (i = 0; i < ADT7475_PWM_COUNT; i++)
 		adt7475_read_pwm(client, i);
 
+	if (of_node && of_property_read_bool(of_node, "invert-pwm"))
+		for (i = 0; i < ADT7475_PWM_COUNT; i++)
+			adt7475_invert_pwm(client, i);
+
 	/* Start monitoring */
 	switch (chip) {
 	case adt7475:
-- 
2.19.1

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

[Guenter Roeck]

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
> 

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

[Chris Packham]

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

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

[Guenter Roeck]

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

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

[Chris Packham]

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