| Message ID | 20240529-apol-ina2xx-fix-v1-2-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: > > +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
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
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.
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
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);
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(+)