| Message ID | 20240529-apol-ina2xx-fix-v1-1-77b4b382190f@axis.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (ina2xx):Add Suppor for passing alert polarity from device tree to driver | expand |
On 29/05/2024 08:07, Amna Waseem wrote: > Add a property to the binding to configure the Alert Polarity. > Alert pin is asserted based on the value of Alert Polarity bit of > Mask/Enable register. It is by default 0 which means Alert pin is > configured to be active low. To configure it to active high, set > alert-polarity property value to 1. > > Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> > --- > Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml > index df86c2c92037..a3f0fd71fcc6 100644 > --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml > +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml > @@ -66,6 +66,14 @@ properties: > description: phandle to the regulator that provides the VS supply typically > in range from 2.7 V to 5.5 V. > > + alert-polarity: Missing vendor prefix. > + description: | Do not need '|' unless you need to preserve formatting. > + Alert polarity bit value of Mask/Enable register. Alert pin is asserted > + based on the value of Alert polarity Bit. Default value is active low. > + 0 selects active low, 1 selects active high. Just use string, easier to read. But for sure do not introduce different values than we already have - GPIO HIGH is 0, not 1. Best regards, Krzysztof
On 5/29/24 00:07, Krzysztof Kozlowski wrote: > On 29/05/2024 08:07, Amna Waseem wrote: >> Add a property to the binding to configure the Alert Polarity. >> Alert pin is asserted based on the value of Alert Polarity bit of >> Mask/Enable register. It is by default 0 which means Alert pin is >> configured to be active low. To configure it to active high, set >> alert-polarity property value to 1. >> >> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> >> --- >> Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >> index df86c2c92037..a3f0fd71fcc6 100644 >> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >> @@ -66,6 +66,14 @@ properties: >> description: phandle to the regulator that provides the VS supply typically >> in range from 2.7 V to 5.5 V. >> >> + alert-polarity: > > Missing vendor prefix. > Are you sure you want a vendor prefix here ? Reason for asking is that many hardware monitoring chips have configurable alert or interrupt polarity, only the name is different. Some examples are the JC42.4 standard ("event polarity"), adt7410/adt7420 "interrupt polarity", MAX31827 ("alarm polarity"), or DS1621 ("output polarity"). We even have a vendor property, "adi,alarm-pol", used for MAX31827. Secondary problem is that not all chips of the series support this configuration. INA209 has a configurable "warning polarity", but the warning pin and the smbus alert pin are two different pins. INA219 and INA220 do not have alert or interrupt output pins. Only INA226, INA230, INA231, INA238, and INA260 support configurable alert polarity. Thanks, Guenter >> + description: | > > Do not need '|' unless you need to preserve formatting. > >> + Alert polarity bit value of Mask/Enable register. Alert pin is asserted >> + based on the value of Alert polarity Bit. Default value is active low. >> + 0 selects active low, 1 selects active high. > > Just use string, easier to read. But for sure do not introduce different > values than we already have - GPIO HIGH is 0, not 1. > > > > Best regards, > Krzysztof >
On 5/29/24 16:01, Guenter Roeck wrote: > On 5/29/24 00:07, Krzysztof Kozlowski wrote: >> On 29/05/2024 08:07, Amna Waseem wrote: >>> Add a property to the binding to configure the Alert Polarity. >>> Alert pin is asserted based on the value of Alert Polarity bit of >>> Mask/Enable register. It is by default 0 which means Alert pin is >>> configured to be active low. To configure it to active high, set >>> alert-polarity property value to 1. >>> >>> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> >>> --- >>> Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> index df86c2c92037..a3f0fd71fcc6 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> @@ -66,6 +66,14 @@ properties: >>> description: phandle to the regulator that provides the VS >>> supply typically >>> in range from 2.7 V to 5.5 V. >>> + alert-polarity: >> >> Missing vendor prefix. >> > > Are you sure you want a vendor prefix here ? Reason for asking is that > many hardware monitoring chips have configurable alert or interrupt > polarity, > only the name is different. Some examples are the JC42.4 standard ("event > polarity"), adt7410/adt7420 "interrupt polarity", MAX31827 ("alarm > polarity"), > or DS1621 ("output polarity"). We even have a vendor property, > "adi,alarm-pol", > used for MAX31827. > > Secondary problem is that not all chips of the series support this > configuration. INA209 has a configurable "warning polarity", but the > warning pin and the smbus alert pin are two different pins. > INA219 and INA220 do not have alert or interrupt output pins. > Only INA226, INA230, INA231, INA238, and INA260 support configurable > alert polarity. > > Thanks, > Guenter I agree with not using vendor prefix with alert-polarity property. @Krzysztof Kozlowski what do you suggest? Amna >>> + description: | >> >> Do not need '|' unless you need to preserve formatting. >> >>> + Alert polarity bit value of Mask/Enable register. Alert pin >>> is asserted >>> + based on the value of Alert polarity Bit. Default value is >>> active low. >>> + 0 selects active low, 1 selects active high. >> >> Just use string, easier to read. But for sure do not introduce different >> values than we already have - GPIO HIGH is 0, not 1. >> >> >> >> Best regards, >> Krzysztof >> >
On 5/30/24 01:18, Amna Waseem wrote: > On 5/29/24 16:01, Guenter Roeck wrote: >> On 5/29/24 00:07, Krzysztof Kozlowski wrote: >>> On 29/05/2024 08:07, Amna Waseem wrote: >>>> Add a property to the binding to configure the Alert Polarity. >>>> Alert pin is asserted based on the value of Alert Polarity bit of >>>> Mask/Enable register. It is by default 0 which means Alert pin is >>>> configured to be active low. To configure it to active high, set >>>> alert-polarity property value to 1. >>>> >>>> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> >>>> --- >>>> Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>>> index df86c2c92037..a3f0fd71fcc6 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>>> @@ -66,6 +66,14 @@ properties: >>>> description: phandle to the regulator that provides the VS supply typically >>>> in range from 2.7 V to 5.5 V. >>>> + alert-polarity: >>> >>> Missing vendor prefix. >>> >> >> Are you sure you want a vendor prefix here ? Reason for asking is that >> many hardware monitoring chips have configurable alert or interrupt polarity, >> only the name is different. Some examples are the JC42.4 standard ("event >> polarity"), adt7410/adt7420 "interrupt polarity", MAX31827 ("alarm polarity"), >> or DS1621 ("output polarity"). We even have a vendor property, "adi,alarm-pol", >> used for MAX31827. >> >> Secondary problem is that not all chips of the series support this >> configuration. INA209 has a configurable "warning polarity", but the >> warning pin and the smbus alert pin are two different pins. >> INA219 and INA220 do not have alert or interrupt output pins. >> Only INA226, INA230, INA231, INA238, and INA260 support configurable >> alert polarity. >> >> Thanks, >> Guenter > > I agree with not using vendor prefix with alert-polarity property. @Krzysztof Kozlowski what do you suggest? > The version with vendor prefix was already accepted, so let's just go with it. It is not worth arguing. We can revisit if there is ever the need to support this for other chips. Thanks, Guenter
On 29/05/2024 16:01, Guenter Roeck wrote: > On 5/29/24 00:07, Krzysztof Kozlowski wrote: >> On 29/05/2024 08:07, Amna Waseem wrote: >>> Add a property to the binding to configure the Alert Polarity. >>> Alert pin is asserted based on the value of Alert Polarity bit of >>> Mask/Enable register. It is by default 0 which means Alert pin is >>> configured to be active low. To configure it to active high, set >>> alert-polarity property value to 1. >>> >>> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> >>> --- >>> Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> index df86c2c92037..a3f0fd71fcc6 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>> @@ -66,6 +66,14 @@ properties: >>> description: phandle to the regulator that provides the VS supply typically >>> in range from 2.7 V to 5.5 V. >>> >>> + alert-polarity: >> >> Missing vendor prefix. >> > > Are you sure you want a vendor prefix here ? Reason for asking is that > many hardware monitoring chips have configurable alert or interrupt polarity, > only the name is different. Some examples are the JC42.4 standard ("event > polarity"), adt7410/adt7420 "interrupt polarity", MAX31827 ("alarm polarity"), > or DS1621 ("output polarity"). We even have a vendor property, "adi,alarm-pol", > used for MAX31827. Hm, I just checked if this is already existing property, but indeed I did not check other variants. Indeed it could go to common properties - hwmon-common.yaml. But then how about using strings (as I asked before...). Best regards, Krzysztof
On 5/31/24 00:41, Krzysztof Kozlowski wrote: > On 29/05/2024 16:01, Guenter Roeck wrote: >> On 5/29/24 00:07, Krzysztof Kozlowski wrote: >>> On 29/05/2024 08:07, Amna Waseem wrote: >>>> Add a property to the binding to configure the Alert Polarity. >>>> Alert pin is asserted based on the value of Alert Polarity bit of >>>> Mask/Enable register. It is by default 0 which means Alert pin is >>>> configured to be active low. To configure it to active high, set >>>> alert-polarity property value to 1. >>>> >>>> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> >>>> --- >>>> Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>>> index df86c2c92037..a3f0fd71fcc6 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >>>> @@ -66,6 +66,14 @@ properties: >>>> description: phandle to the regulator that provides the VS supply typically >>>> in range from 2.7 V to 5.5 V. >>>> >>>> + alert-polarity: >>> >>> Missing vendor prefix. >>> >> >> Are you sure you want a vendor prefix here ? Reason for asking is that >> many hardware monitoring chips have configurable alert or interrupt polarity, >> only the name is different. Some examples are the JC42.4 standard ("event >> polarity"), adt7410/adt7420 "interrupt polarity", MAX31827 ("alarm polarity"), >> or DS1621 ("output polarity"). We even have a vendor property, "adi,alarm-pol", >> used for MAX31827. > > Hm, I just checked if this is already existing property, but indeed I > did not check other variants. > > Indeed it could go to common properties - hwmon-common.yaml. But then > how about using strings (as I asked before...). > I can't really comment on the strings; that is out of my knowledge zone. It might make sense to have hwmon-common.yaml, but I think that would require a some (read: a lot of) work. Guenter
On Wed, May 29, 2024 at 08:07:14AM +0200, Amna Waseem wrote: > Add a property to the binding to configure the Alert Polarity. > Alert pin is asserted based on the value of Alert Polarity bit of > Mask/Enable register. It is by default 0 which means Alert pin is > configured to be active low. To configure it to active high, set > alert-polarity property value to 1. > > Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> > --- > Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml > index df86c2c92037..a3f0fd71fcc6 100644 > --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml > +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml > @@ -66,6 +66,14 @@ properties: > description: phandle to the regulator that provides the VS supply typically > in range from 2.7 V to 5.5 V. > > + alert-polarity: > + description: | > + Alert polarity bit value of Mask/Enable register. Alert pin is asserted > + based on the value of Alert polarity Bit. Default value is active low. > + 0 selects active low, 1 selects active high. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] This is alert as in SMBus Alert? That's handled as an interrupt, but this binding has no interrupt property. And the interrupt binding provides a way already to specify active trigger state. Why do we need a second way to do this? Rob
On 6/3/24 08:47, Rob Herring wrote: > On Wed, May 29, 2024 at 08:07:14AM +0200, Amna Waseem wrote: >> Add a property to the binding to configure the Alert Polarity. >> Alert pin is asserted based on the value of Alert Polarity bit of >> Mask/Enable register. It is by default 0 which means Alert pin is >> configured to be active low. To configure it to active high, set >> alert-polarity property value to 1. >> >> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> >> --- >> Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >> index df86c2c92037..a3f0fd71fcc6 100644 >> --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml >> @@ -66,6 +66,14 @@ properties: >> description: phandle to the regulator that provides the VS supply typically >> in range from 2.7 V to 5.5 V. >> >> + alert-polarity: >> + description: | >> + Alert polarity bit value of Mask/Enable register. Alert pin is asserted >> + based on the value of Alert polarity Bit. Default value is active low. >> + 0 selects active low, 1 selects active high. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1] > > This is alert as in SMBus Alert? That's handled as an interrupt, but > this binding has no interrupt property. And the interrupt binding > provides a way already to specify active trigger state. Why do we need a > second way to do this? > SMBus alert is a single interrupt/alert line for all chips in a single I2C/SMBus. it is handled by drivers/i2c/i2c-smbus.c. It can not be handled by an individual driver since it affects all chips on a single bus. A driver supporting it is supposed to implement an alert callback in struct i2c_driver. The signal is supposed to be active-low open collector. Some chips, such as this series, make it configurable; in this case the alternative is active-high open collector. Presumably there is some wiring to attach it to the active-low open collector SMBus interrupt signal. Having said this, I don't really know what the use case is for this driver. It doesn't implement an alert callback, and it doesn't implement an interrupt handler either (after all, it might be conceivable that the alert signal _is_ connected to a dedicated interrupt line). So your question is fair - with neither SMBus alert nor interrupt support by the driver, the alert signal polarity should not really matter. Thanks, Guenter
diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml index df86c2c92037..a3f0fd71fcc6 100644 --- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml +++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml @@ -66,6 +66,14 @@ properties: description: phandle to the regulator that provides the VS supply typically in range from 2.7 V to 5.5 V. + alert-polarity: + description: | + Alert polarity bit value of Mask/Enable register. Alert pin is asserted + based on the value of Alert polarity Bit. Default value is active low. + 0 selects active low, 1 selects active high. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + required: - compatible - reg @@ -88,5 +96,6 @@ examples: label = "vdd_3v0"; shunt-resistor = <1000>; vs-supply = <&vdd_3v0>; + alert-polarity = <1>; }; };
Add a property to the binding to configure the Alert Polarity. Alert pin is asserted based on the value of Alert Polarity bit of Mask/Enable register. It is by default 0 which means Alert pin is configured to be active low. To configure it to active high, set alert-polarity property value to 1. Signed-off-by: Amna Waseem <Amna.Waseem@axis.com> --- Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 9 +++++++++ 1 file changed, 9 insertions(+)