| Message ID | 0ea50e87-2b63-4062-8c2a-17537495f481@stanley.mountain (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: max77705: Fix two static checker issues | expand |
On 21/03/2025 15:34, Dan Carpenter wrote: > Return -EINVAL if the health is bad. Don't return success. > > Fixes: a6a494c8e3ce ("power: supply: max77705: Add charger driver for Maxim 77705") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/power/supply/max77705_charger.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/power/supply/max77705_charger.c b/drivers/power/supply/max77705_charger.c > index 329b430d0e50..0e347353c41e 100644 > --- a/drivers/power/supply/max77705_charger.c > +++ b/drivers/power/supply/max77705_charger.c > @@ -285,7 +285,7 @@ static int max77705_get_health(struct max77705_charger_data *charger, int *val) > if (is_online) { > ret = max77705_get_vbus_state(regmap, val); > if (ret || (*val != POWER_SUPPLY_HEALTH_GOOD)) > - return ret; > + return -EINVAL; I don't think this is right. First, your commit msg should mention why returning -EINVAL in such case. Second, if get_vbus_state succeeded, but 'val' is not good (e.g. overvoltage), the callback is supposed to return 0 as success of retrieving the data, no? So the user-space can read 'val' and figure out whatever it needs to figure out (overvoltage). The EINVAL is when the data could not be read, thus user-space should ignore 'val'. Best regards, Krzysztof
On Wed, Mar 26, 2025 at 06:14:13PM +0100, Krzysztof Kozlowski wrote: > On 21/03/2025 15:34, Dan Carpenter wrote: > > Return -EINVAL if the health is bad. Don't return success. > > > > Fixes: a6a494c8e3ce ("power: supply: max77705: Add charger driver for Maxim 77705") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/power/supply/max77705_charger.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/power/supply/max77705_charger.c b/drivers/power/supply/max77705_charger.c > > index 329b430d0e50..0e347353c41e 100644 > > --- a/drivers/power/supply/max77705_charger.c > > +++ b/drivers/power/supply/max77705_charger.c > > @@ -285,7 +285,7 @@ static int max77705_get_health(struct max77705_charger_data *charger, int *val) > > if (is_online) { > > ret = max77705_get_vbus_state(regmap, val); > > if (ret || (*val != POWER_SUPPLY_HEALTH_GOOD)) > > - return ret; > > + return -EINVAL; > > > I don't think this is right. First, your commit msg should mention why > returning -EINVAL in such case. > > Second, if get_vbus_state succeeded, but 'val' is not good (e.g. > overvoltage), the callback is supposed to return 0 as success of > retrieving the data, no? So the user-space can read 'val' and figure out > whatever it needs to figure out (overvoltage). > Yeah. What you're saying makes sense, especially in context. I misread the code. regards, dan carpenter
diff --git a/drivers/power/supply/max77705_charger.c b/drivers/power/supply/max77705_charger.c index 329b430d0e50..0e347353c41e 100644 --- a/drivers/power/supply/max77705_charger.c +++ b/drivers/power/supply/max77705_charger.c @@ -285,7 +285,7 @@ static int max77705_get_health(struct max77705_charger_data *charger, int *val) if (is_online) { ret = max77705_get_vbus_state(regmap, val); if (ret || (*val != POWER_SUPPLY_HEALTH_GOOD)) - return ret; + return -EINVAL; } return max77705_get_battery_health(charger, val); }
Return -EINVAL if the health is bad. Don't return success. Fixes: a6a494c8e3ce ("power: supply: max77705: Add charger driver for Maxim 77705") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/power/supply/max77705_charger.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)