| Message ID | 20240827153455.1344529-8-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (ina2xx) Cleanup and convert to use with_info API | expand |
On Tue, Aug 27, 2024 at 08:34:51AM -0700, Guenter Roeck wrote: > @@ -440,7 +441,7 @@ static ssize_t ina226_alert_store(struct device *dev, > */ > mutex_lock(&data->config_lock); > ret = regmap_update_bits(regmap, INA226_MASK_ENABLE, > - INA226_ALERT_CONFIG_MASK, 0); > + INA226_ALERT_CONFIG_MASK | INA226_ALERT_LATCH_ENABLE, 0); > if (ret < 0) > goto abort; > > @@ -451,8 +452,8 @@ static ssize_t ina226_alert_store(struct device *dev, > > if (val != 0) { > ret = regmap_update_bits(regmap, INA226_MASK_ENABLE, > - INA226_ALERT_CONFIG_MASK, > - attr->index); > + INA226_ALERT_CONFIG_MASK | INA226_ALERT_LATCH_ENABLE, > + attr->index | INA226_ALERT_LATCH_ENABLE); Does it really need to clear and set every time? Could it set only once in ina2xx_probe() just like ina2xx_set_alert_polarity()?
On 8/29/24 07:54, Tzung-Bi Shih wrote: > On Tue, Aug 27, 2024 at 08:34:51AM -0700, Guenter Roeck wrote: >> @@ -440,7 +441,7 @@ static ssize_t ina226_alert_store(struct device *dev, >> */ >> mutex_lock(&data->config_lock); >> ret = regmap_update_bits(regmap, INA226_MASK_ENABLE, >> - INA226_ALERT_CONFIG_MASK, 0); >> + INA226_ALERT_CONFIG_MASK | INA226_ALERT_LATCH_ENABLE, 0); >> if (ret < 0) >> goto abort; >> >> @@ -451,8 +452,8 @@ static ssize_t ina226_alert_store(struct device *dev, >> >> if (val != 0) { >> ret = regmap_update_bits(regmap, INA226_MASK_ENABLE, >> - INA226_ALERT_CONFIG_MASK, >> - attr->index); >> + INA226_ALERT_CONFIG_MASK | INA226_ALERT_LATCH_ENABLE, >> + attr->index | INA226_ALERT_LATCH_ENABLE); > > Does it really need to clear and set every time? Could it set only once in > ina2xx_probe() just like ina2xx_set_alert_polarity()? > The idea was to clear any pending alerts when changing the monitored limit. As it turns out (I checked with real chips), this is not necessary; pending alerts are cleared if the mask is cleared/updated even if the latch bit is set. I made the change as you suggested. Thanks! Guenter
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c index f7d78588e579..9016c90f23c9 100644 --- a/drivers/hwmon/ina2xx.c +++ b/drivers/hwmon/ina2xx.c @@ -67,6 +67,7 @@ #define INA226_READ_AVG(reg) FIELD_GET(INA226_AVG_RD_MASK, reg) +#define INA226_ALERT_LATCH_ENABLE BIT(0) #define INA226_ALERT_POLARITY_MASK BIT(1) #define INA226_ALERT_POL_LOW 0 #define INA226_ALERT_POL_HIGH 1 @@ -440,7 +441,7 @@ static ssize_t ina226_alert_store(struct device *dev, */ mutex_lock(&data->config_lock); ret = regmap_update_bits(regmap, INA226_MASK_ENABLE, - INA226_ALERT_CONFIG_MASK, 0); + INA226_ALERT_CONFIG_MASK | INA226_ALERT_LATCH_ENABLE, 0); if (ret < 0) goto abort; @@ -451,8 +452,8 @@ static ssize_t ina226_alert_store(struct device *dev, if (val != 0) { ret = regmap_update_bits(regmap, INA226_MASK_ENABLE, - INA226_ALERT_CONFIG_MASK, - attr->index); + INA226_ALERT_CONFIG_MASK | INA226_ALERT_LATCH_ENABLE, + attr->index | INA226_ALERT_LATCH_ENABLE); if (ret < 0) goto abort; }
Alerts should only be cleared after reported, not immediately after the alert condition has been cleared. Set the latch enable bit to keep alerts latched until the alert register has been read from the chip. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/hwmon/ina2xx.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)