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

[PATCH v2 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>
---
Changes in v2:
- Add vendor specific prefix to alert polarity property in binding.
- Minor improvement in description of alert polarity binding property
- Remove usage of mutex while setting alert polarity in Mask/Enable
  register
- Correct indentation
- Link to v1: https://lore.kernel.org/r/20240529-apol-ina2xx-fix-v1-0-77b4b382190f@axis.com

---
Amna Waseem (2):
      dt-bindings: hwmon: ti,ina2xx: Add ti,alert-polarity property
      hwmon: (ina2xx) Add device tree support to pass alert polarity

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

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

[PATCH v2 1/2] dt-bindings: hwmon: ti,ina2xx: Add ti,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 open collector. Value of 1 maps to
Inverted (active high open collector).

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..5a16d2d94587 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.
 
+  ti,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 Normal
+      (0 which maps to active-low open collector). The other value is Inverted
+      (1 which maps to active-high open collector).
+    $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>;
+            ti,alert-polarity = <1>;
         };
     };

-- 
2.30.2

[PATCH v2 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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index d8415d1f21fc..9afaabdc367d 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,21 @@ 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;
+
+	ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
+				 INA226_ALERT_POLARITY_MASK,
+				 INA226_SHIFT_ALERT_POLARITY(val));
+
+	return ret;
+}
+
 /*
  * Calibration register is set to the best value, which eliminates
  * truncation errors on calculating current register in hardware.
@@ -659,6 +677,15 @@ 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, "ti,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 v2 2/2] hwmon: (ina2xx) Add device tree support to pass alert polarity

[Guenter Roeck]

On 5/29/24 02:47, 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>

Please address my earlier comments, and in the future please wait a few minutes
before sending another version to give people time to provide feedback
on the earlier version(s).

> ---
>   drivers/hwmon/ina2xx.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index d8415d1f21fc..9afaabdc367d 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,21 @@ 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;
> +
> +	ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
> +				 INA226_ALERT_POLARITY_MASK,
> +				 INA226_SHIFT_ALERT_POLARITY(val));
> +
> +	return ret;

ret is an unnecessary variable.
	return regmap_update_bits(...);


> +}
> +
>   /*
>    * Calibration register is set to the best value, which eliminates
>    * truncation errors on calculating current register in hardware.
> @@ -659,6 +677,15 @@ 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, "ti,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");

Line split is still as bad as before.

Guenter

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

On Wed, May 29, 2024 at 11:47:44AM +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 open collector. Value of 1 maps to
> Inverted (active high open collector).
> 
> 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..5a16d2d94587 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.
>  
> +  ti,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 Normal
> +      (0 which maps to active-low open collector). The other value is Inverted
> +      (1 which maps to active-high open collector).
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

There's no need for this to have a value, it's sufficient to be a flag
of "ti,alert-active-high". Present would mean active-high and absent
active-low. This has the added benefit the devicetree node being
understandable to a reader.

Thanks,
Conor.

> +
>  required:
>    - compatible
>    - reg
> @@ -88,5 +96,6 @@ examples:
>              label = "vdd_3v0";
>              shunt-resistor = <1000>;
>              vs-supply = <&vdd_3v0>;
> +            ti,alert-polarity = <1>;
>          };
>      };
> 
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Guenter Roeck]

On 5/29/24 09:17, Conor Dooley wrote:
> On Wed, May 29, 2024 at 11:47:44AM +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 open collector. Value of 1 maps to
>> Inverted (active high open collector).
>>
>> 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..5a16d2d94587 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.
>>   
>> +  ti,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 Normal
>> +      (0 which maps to active-low open collector). The other value is Inverted
>> +      (1 which maps to active-high open collector).
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
> 
> There's no need for this to have a value, it's sufficient to be a flag
> of "ti,alert-active-high". Present would mean active-high and absent
> active-low. This has the added benefit the devicetree node being
> understandable to a reader.
> 

Agreed, makes sense. Even better, at the same time simplifies the code.

Guenter

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

[Amna Waseem]

On 5/29/24 16:11, Guenter Roeck wrote:
> On 5/29/24 02:47, 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>
>
> Please address my earlier comments, and in the future please wait a 
> few minutes
> before sending another version to give people time to provide feedback
> on the earlier version(s).
Ok.
>> ---
>>   drivers/hwmon/ina2xx.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index d8415d1f21fc..9afaabdc367d 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,21 @@ 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;
>> +
>> +    ret = regmap_update_bits(data->regmap, INA226_MASK_ENABLE,
>> +                 INA226_ALERT_POLARITY_MASK,
>> +                 INA226_SHIFT_ALERT_POLARITY(val));
>> +
>> +    return ret;
>
> ret is an unnecessary variable.
>     return regmap_update_bits(...);
>
Agreed. Will do it in next patch
>
>> +}
>> +
>>   /*
>>    * Calibration register is set to the best value, which eliminates
>>    * truncation errors on calculating current register in hardware.
>> @@ -659,6 +677,15 @@ 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, "ti,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");
>
> Line split is still as bad as before.
>
> Guenter
>
I have tried to apply clang-format and it still shows the line split as 
follows:

     if (!of_property_read_u32(dev->of_node, "ti,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");
         }
     }

What format will you suggest for line split? Is there any reference you 
can provide for splitting up the lines in Linux kernel drivers' code?

Amna

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

[Amna Waseem]

On 5/29/24 18:20, Guenter Roeck wrote:
> On 5/29/24 09:17, Conor Dooley wrote:
>> On Wed, May 29, 2024 at 11:47:44AM +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 open collector. Value of 1 maps to
>>> Inverted (active high open collector).
>>>
>>> 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..5a16d2d94587 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.
>>>   +  ti,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 Normal
>>> +      (0 which maps to active-low open collector). The other value 
>>> is Inverted
>>> +      (1 which maps to active-high open collector).
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1]
>>
>> There's no need for this to have a value, it's sufficient to be a flag
>> of "ti,alert-active-high". Present would mean active-high and absent
>> active-low. This has the added benefit the devicetree node being
>> understandable to a reader.
>>
>
> Agreed, makes sense. Even better, at the same time simplifies the code.
>
> Guenter
>
>
Agreed. Will do it in next patch

Amna