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

[PATCH v2 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     | 24 ++++++++++++++++
 .../devicetree/bindings/trivial-devices.txt   |  4 ---
 drivers/hwmon/adt7475.c                       | 28 +++++++++++++++++++
 3 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.txt

-- 
2.19.1

[PATCH v2 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>
---
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 = ;
+	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
-- 
2.19.1

[PATCH v2 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>
---
Changes in v2:
- use "pwm-polarity" and support setting both logic high and logic low

 drivers/hwmon/adt7475.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index f4c7516eb989..ce2c7d6493c0 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -23,6 +23,7 @@
 #include <linux/err.h>
 #include <linux/jiffies.h>
 #include <linux/util_macros.h>
+#include <linux/pwm.h>
 
 /* Indexes for the sysfs hooks */
 
@@ -1536,6 +1537,23 @@ static int adt7475_update_limits(struct i2c_client *client)
 	return 0;
 }
 
+static int adt7475_set_pwm_polarity(struct i2c_client *client, int index,
+				    enum pwm_polarity pol)
+{
+	int ret;
+
+	ret = adt7475_read(PWM_CONFIG_REG(index));
+	if (ret < 0)
+		return ret;
+
+	if (pol == PWM_POLARITY_INVERSED)
+		ret |= BIT(4);
+	else
+		ret &= ~BIT(4);
+
+	return i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(index), ret);
+}
+
 static int adt7475_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1638,6 +1656,16 @@ static int adt7475_probe(struct i2c_client *client,
 	for (i = 0; i < ADT7475_PWM_COUNT; i++)
 		adt7475_read_pwm(client, i);
 
+	if (client->dev.of_node) {
+		enum pwm_polarity polarity;
+
+		ret = of_property_read_u32(client->dev.of_node, "pwm-polarity",
+					   &polarity);
+		if (!ret)
+			for (i = 0; i < ADT7475_PWM_COUNT; i++)
+				adt7475_set_pwm_polarity(client, i, polarity);
+	}
+
 	/* Start monitoring */
 	switch (chip) {
 	case adt7475:
-- 
2.19.1

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

[Chris Packham]

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
> 

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

[Rob Herring]

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

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

[Guenter Roeck]

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

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

[Rob Herring]

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

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