| Message ID | 20181108225607.7521-2-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (adt7475): inverted PWM output | expand |
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 >
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
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
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
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.
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
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