[PATCH 0/2] hwmon: (ina2xx):Add Suppor for passing alert polarity from device tree to driver

[PATCH 0/2] hwmon: (ina2xx):Add Suppor for passing alert polarity from device tree to driver

[Amna Waseem]

The INA230 has alert polarity bit in Mask/Enable register which can be
configured to be active high or active low depending upon the requirements
of the hardware using this chip. The patches in this series adds the support
for passing alert polarity value from device tree to the driver. Alert polarity
property is added device tree bindings and the driver is modified to read
this property and set the Alert polarity (APOL) bit value in Mask/Enable register
of INA230.

Signed-off-by: Amna Waseem <Amna.Waseem@axis.com>
---
Amna Waseem (2):
      dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property
      hwmon: (ina2xx) Add device tree support to pass alert polarity

 .../devicetree/bindings/hwmon/ti,ina2xx.yaml       |  9 +++++++
 drivers/hwmon/ina2xx.c                             | 28 ++++++++++++++++++++++
 2 files changed, 37 insertions(+)
---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240524-apol-ina2xx-fix-34e76346cb26

Best regards,
-- 
Amna Waseem <Amna.Waseem@axis.com>

[PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Amna Waseem]

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]
+
 required:
   - compatible
   - reg
@@ -88,5 +96,6 @@ examples:
             label = "vdd_3v0";
             shunt-resistor = <1000>;
             vs-supply = <&vdd_3v0>;
+            alert-polarity = <1>;
         };
     };

-- 
2.30.2

[PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

[Amna Waseem]

The INA230 has an Alert pin which is asserted when the alert
function selected in the Mask/Enable register exceeds the
value programmed into the Alert Limit register. Assertion is based
on the Alert Polarity Bit (APOL, bit 1 of the Mask/Enable register).
It is default set to value 0 i.e Normal (active-low open collector).
However, hardware can be designed in such a way that expects Alert pin
to become active high if a user-defined threshold in Alert limit
register has been exceeded. This patch adds a way to pass alert polarity
value to the driver via device tree.

Signed-off-by: Amna Waseem <Amna.Waseem@axis.com>
---
 drivers/hwmon/ina2xx.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index d8415d1f21fc..b58e795bdc8f 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -73,6 +73,9 @@
 #define INA226_READ_AVG(reg)		(((reg) & INA226_AVG_RD_MASK) >> 9)
 #define INA226_SHIFT_AVG(val)		((val) << 9)
 
+#define INA226_ALERT_POLARITY_MASK		0x0002
+#define INA226_SHIFT_ALERT_POLARITY(val)	((val) << 1)
+
 /* bit number of alert functions in Mask/Enable Register */
 #define INA226_SHUNT_OVER_VOLTAGE_BIT	15
 #define INA226_SHUNT_UNDER_VOLTAGE_BIT	14
@@ -178,6 +181,23 @@ static u16 ina226_interval_to_reg(int interval)
 	return INA226_SHIFT_AVG(avg_bits);
 }
 
+static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
+				     unsigned long val)
+{
+	int ret;
+
+	if (val > INT_MAX || !(val == 0 || val == 1))
+		return -EINVAL;
+
+	mutex_lock(&data->config_lock);
+	ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
+				 INA226_ALERT_POLARITY_MASK,
+				 INA226_SHIFT_ALERT_POLARITY(val));
+
+	mutex_unlock(&data->config_lock);
+	return ret;
+}
+
 /*
  * Calibration register is set to the best value, which eliminates
  * truncation errors on calculating current register in hardware.
@@ -659,6 +679,14 @@ static int ina2xx_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
 
+	if (!of_property_read_u32(dev->of_node, "alert-polarity", &val)) {
+		ret = ina2xx_set_alert_polarity(data, val);
+		if (ret < 0)
+			return dev_err_probe(
+				dev, ret,
+				"failed to set APOL bit of Enable/Mask register\n");
+	}
+
 	ret = ina2xx_init(data);
 	if (ret < 0) {
 		dev_err(dev, "error configuring the device: %d\n", ret);

-- 
2.30.2

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Krzysztof Kozlowski]

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

Re: [PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

[Krzysztof Kozlowski]

On 29/05/2024 08:07, Amna Waseem wrote:
>  
> +static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
> +				     unsigned long val)
> +{
> +	int ret;
> +
> +	if (val > INT_MAX || !(val == 0 || val == 1))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->config_lock);

Aren't you calling it before registering sysfs interface? Why do you
need mutex?

> +	ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
> +				 INA226_ALERT_POLARITY_MASK,
> +				 INA226_SHIFT_ALERT_POLARITY(val));
> +
> +	mutex_unlock(&data->config_lock);
> +	return ret;
> +}
> +
>  /*
>   * Calibration register is set to the best value, which eliminates
>   * truncation errors on calculating current register in hardware.
> @@ -659,6 +679,14 @@ static int ina2xx_probe(struct i2c_client *client)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
>  
> +	if (!of_property_read_u32(dev->of_node, "alert-polarity", &val)) {
> +		ret = ina2xx_set_alert_polarity(data, val);
> +		if (ret < 0)
> +			return dev_err_probe(
> +				dev, ret,
> +				"failed to set APOL bit of Enable/Mask register\n");

That's odd wrapping. Please follow Linux coding style and align these.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Guenter Roeck]

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
> 

Re: [PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

[Guenter Roeck]

On 5/28/24 23:07, Amna Waseem wrote:
> The INA230 has an Alert pin which is asserted when the alert
> function selected in the Mask/Enable register exceeds the
> value programmed into the Alert Limit register. Assertion is based
> on the Alert Polarity Bit (APOL, bit 1 of the Mask/Enable register).
> It is default set to value 0 i.e Normal (active-low open collector).
> However, hardware can be designed in such a way that expects Alert pin
> to become active high if a user-defined threshold in Alert limit
> register has been exceeded. This patch adds a way to pass alert polarity
> value to the driver via device tree.
> 
> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com>
> ---
>   drivers/hwmon/ina2xx.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index d8415d1f21fc..b58e795bdc8f 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -73,6 +73,9 @@
>   #define INA226_READ_AVG(reg)		(((reg) & INA226_AVG_RD_MASK) >> 9)
>   #define INA226_SHIFT_AVG(val)		((val) << 9)
>   
> +#define INA226_ALERT_POLARITY_MASK		0x0002
> +#define INA226_SHIFT_ALERT_POLARITY(val)	((val) << 1)
> +
>   /* bit number of alert functions in Mask/Enable Register */
>   #define INA226_SHUNT_OVER_VOLTAGE_BIT	15
>   #define INA226_SHUNT_UNDER_VOLTAGE_BIT	14
> @@ -178,6 +181,23 @@ static u16 ina226_interval_to_reg(int interval)
>   	return INA226_SHIFT_AVG(avg_bits);
>   }
>   
> +static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
> +				     unsigned long val)
> +{
> +	int ret;
> +
> +	if (val > INT_MAX || !(val == 0 || val == 1))

	if (val != 0 && val !=1)

would be sufficient and much easier to understand.

> +		return -EINVAL;
> +
> +	mutex_lock(&data->config_lock);

Pointless lock.

> +	ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
> +				 INA226_ALERT_POLARITY_MASK,
> +				 INA226_SHIFT_ALERT_POLARITY(val));
> +
> +	mutex_unlock(&data->config_lock);
> +	return ret;
> +}
> +
>   /*
>    * Calibration register is set to the best value, which eliminates
>    * truncation errors on calculating current register in hardware.
> @@ -659,6 +679,14 @@ static int ina2xx_probe(struct i2c_client *client)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
>   
> +	if (!of_property_read_u32(dev->of_node, "alert-polarity", &val)) {
> +		ret = ina2xx_set_alert_polarity(data, val);
> +		if (ret < 0)
> +			return dev_err_probe(
> +				dev, ret,
> +				"failed to set APOL bit of Enable/Mask register\n");
> +	}

INA219 and INA220 do not support alert pin configuration (or, naturally,
the mask register in the first place). This will need to be validated.

Guenter

Re: [PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

[Amna Waseem]

On 5/29/24 16:07, Guenter Roeck wrote:
> On 5/28/24 23:07, Amna Waseem wrote:
>> The INA230 has an Alert pin which is asserted when the alert
>> function selected in the Mask/Enable register exceeds the
>> value programmed into the Alert Limit register. Assertion is based
>> on the Alert Polarity Bit (APOL, bit 1 of the Mask/Enable register).
>> It is default set to value 0 i.e Normal (active-low open collector).
>> However, hardware can be designed in such a way that expects Alert pin
>> to become active high if a user-defined threshold in Alert limit
>> register has been exceeded. This patch adds a way to pass alert polarity
>> value to the driver via device tree.
>>
>> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com>
>> ---
>>   drivers/hwmon/ina2xx.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index d8415d1f21fc..b58e795bdc8f 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>> @@ -73,6 +73,9 @@
>>   #define INA226_READ_AVG(reg)        (((reg) & INA226_AVG_RD_MASK) 
>> >> 9)
>>   #define INA226_SHIFT_AVG(val)        ((val) << 9)
>>   +#define INA226_ALERT_POLARITY_MASK        0x0002
>> +#define INA226_SHIFT_ALERT_POLARITY(val)    ((val) << 1)
>> +
>>   /* bit number of alert functions in Mask/Enable Register */
>>   #define INA226_SHUNT_OVER_VOLTAGE_BIT    15
>>   #define INA226_SHUNT_UNDER_VOLTAGE_BIT    14
>> @@ -178,6 +181,23 @@ static u16 ina226_interval_to_reg(int interval)
>>       return INA226_SHIFT_AVG(avg_bits);
>>   }
>>   +static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
>> +                     unsigned long val)
>> +{
>> +    int ret;
>> +
>> +    if (val > INT_MAX || !(val == 0 || val == 1))
>
>     if (val != 0 && val !=1)
>
> would be sufficient and much easier to understand.


Agreed.

>
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&data->config_lock);
>
> Pointless lock.
>
>> +    ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
>> +                 INA226_ALERT_POLARITY_MASK,
>> +                 INA226_SHIFT_ALERT_POLARITY(val));
>> +
>> +    mutex_unlock(&data->config_lock);
>> +    return ret;
>> +}
>> +
>>   /*
>>    * Calibration register is set to the best value, which eliminates
>>    * truncation errors on calculating current register in hardware.
>> @@ -659,6 +679,14 @@ static int ina2xx_probe(struct i2c_client *client)
>>       if (ret)
>>           return dev_err_probe(dev, ret, "failed to enable vs 
>> regulator\n");
>>   +    if (!of_property_read_u32(dev->of_node, "alert-polarity", 
>> &val)) {
>> +        ret = ina2xx_set_alert_polarity(data, val);
>> +        if (ret < 0)
>> +            return dev_err_probe(
>> +                dev, ret,
>> +                "failed to set APOL bit of Enable/Mask register\n");
>> +    }
>
> INA219 and INA220 do not support alert pin configuration (or, naturally,
> the mask register in the first place). This will need to be validated.
>
> Guenter
>
Would "of_property_read_bool" be sufficient to check whether the 
property exists or not for different chips? It means that if INA219 and 
INA220 are being used, they will not have a property "alert-polarity" 
defined in their devicetree so of_property_read_bool will return false 
and nothing will happen for these chips.

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Amna Waseem]

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

Re: [PATCH 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

[Guenter Roeck]

On 5/30/24 01:06, Amna Waseem wrote:
> On 5/29/24 16:07, Guenter Roeck wrote:
>> On 5/28/24 23:07, Amna Waseem wrote:
>>> The INA230 has an Alert pin which is asserted when the alert
>>> function selected in the Mask/Enable register exceeds the
>>> value programmed into the Alert Limit register. Assertion is based
>>> on the Alert Polarity Bit (APOL, bit 1 of the Mask/Enable register).
>>> It is default set to value 0 i.e Normal (active-low open collector).
>>> However, hardware can be designed in such a way that expects Alert pin
>>> to become active high if a user-defined threshold in Alert limit
>>> register has been exceeded. This patch adds a way to pass alert polarity
>>> value to the driver via device tree.
>>>
>>> Signed-off-by: Amna Waseem <Amna.Waseem@axis.com>
>>> ---
>>>   drivers/hwmon/ina2xx.c | 28 ++++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>>> index d8415d1f21fc..b58e795bdc8f 100644
>>> --- a/drivers/hwmon/ina2xx.c
>>> +++ b/drivers/hwmon/ina2xx.c
>>> @@ -73,6 +73,9 @@
>>>   #define INA226_READ_AVG(reg)        (((reg) & INA226_AVG_RD_MASK) >> 9)
>>>   #define INA226_SHIFT_AVG(val)        ((val) << 9)
>>>   +#define INA226_ALERT_POLARITY_MASK        0x0002
>>> +#define INA226_SHIFT_ALERT_POLARITY(val)    ((val) << 1)
>>> +
>>>   /* bit number of alert functions in Mask/Enable Register */
>>>   #define INA226_SHUNT_OVER_VOLTAGE_BIT    15
>>>   #define INA226_SHUNT_UNDER_VOLTAGE_BIT    14
>>> @@ -178,6 +181,23 @@ static u16 ina226_interval_to_reg(int interval)
>>>       return INA226_SHIFT_AVG(avg_bits);
>>>   }
>>>   +static int ina2xx_set_alert_polarity(struct ina2xx_data *data,
>>> +                     unsigned long val)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (val > INT_MAX || !(val == 0 || val == 1))
>>
>>     if (val != 0 && val !=1)
>>
>> would be sufficient and much easier to understand.
> 
> 
> Agreed.
> 
>>
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&data->config_lock);
>>
>> Pointless lock.
>>
>>> +    ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
>>> +                 INA226_ALERT_POLARITY_MASK,
>>> +                 INA226_SHIFT_ALERT_POLARITY(val));
>>> +
>>> +    mutex_unlock(&data->config_lock);
>>> +    return ret;
>>> +}
>>> +
>>>   /*
>>>    * Calibration register is set to the best value, which eliminates
>>>    * truncation errors on calculating current register in hardware.
>>> @@ -659,6 +679,14 @@ static int ina2xx_probe(struct i2c_client *client)
>>>       if (ret)
>>>           return dev_err_probe(dev, ret, "failed to enable vs regulator\n");
>>>   +    if (!of_property_read_u32(dev->of_node, "alert-polarity", &val)) {
>>> +        ret = ina2xx_set_alert_polarity(data, val);
>>> +        if (ret < 0)
>>> +            return dev_err_probe(
>>> +                dev, ret,
>>> +                "failed to set APOL bit of Enable/Mask register\n");
>>> +    }
>>
>> INA219 and INA220 do not support alert pin configuration (or, naturally,
>> the mask register in the first place). This will need to be validated.
>>
>> Guenter
>>
> Would "of_property_read_bool" be sufficient to check whether the property exists or not for different chips? It means that if INA219 and INA220 are being used, they will not have a property "alert-polarity" defined in their devicetree so of_property_read_bool will return false and nothing will happen for these chips.
> 

No, that would not be sufficient, because the non-existence of the property
also has a meaning and still should be used to set the polarity.

You have two options: Not evaluate the property in the first place if
the chip doesn't support it, or return an error if the property exists
but the chip does not support it. Personally I'd go the easy route;
something like

	if (chip supports it) {
		ret = ina2xx_set_alert_polarity(dev, regmap);
		if (ret)
			return dev_err_probe(...);
	}

and evaluate the property in ina2xx_set_alert_polarity().

Guenter

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Guenter Roeck]

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

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Krzysztof Kozlowski]

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

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Guenter Roeck]

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

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Rob Herring]

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

Re: [PATCH 1/2] dt-bindings: hwmon: ti,ina2xx: Add alert-polarity property

[Guenter Roeck]

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