| Message ID | 51e49c18574003db1e20c9299061a5ecd1661a3c.1719121781.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | power: supply: ingenic: Fix some error handling paths in ingenic_battery_get_property() | expand |
Hi Christophe, thanks for the patch, looking good. Acked-by: Artur Rojek <contact@artur-rojek.eu> Cheers, Artur On 2024-06-23 07:50, Christophe JAILLET wrote: > If iio_read_channel_processed() fails, 'val->intval' is not updated, > but it > is still *1000 just after. So, in case of error, the *1000 accumulate > and > 'val->intval' becomes erroneous. > > So instead of rescaling the value after the fact, use the dedicated > scaling > API. This way the result is updated only when needed. In case of error, > the > previous value is kept, unmodified. > > This should also reduce any inaccuracies resulting from the scaling. > > Finally, this is also slightly more efficient as it saves a function > call > and a multiplication. > > Fixes: fb24ccfbe1e0 ("power: supply: add Ingenic JZ47xx battery > driver.") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/power/supply/ingenic-battery.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/power/supply/ingenic-battery.c > b/drivers/power/supply/ingenic-battery.c > index 2e7fdfde47ec..0a40f425c277 100644 > --- a/drivers/power/supply/ingenic-battery.c > +++ b/drivers/power/supply/ingenic-battery.c > @@ -31,8 +31,9 @@ static int ingenic_battery_get_property(struct > power_supply *psy, > > switch (psp) { > case POWER_SUPPLY_PROP_HEALTH: > - ret = iio_read_channel_processed(bat->channel, &val->intval); > - val->intval *= 1000; > + ret = iio_read_channel_processed_scale(bat->channel, > + &val->intval, > + 1000); > if (val->intval < info->voltage_min_design_uv) > val->intval = POWER_SUPPLY_HEALTH_DEAD; > else if (val->intval > info->voltage_max_design_uv) > @@ -41,8 +42,9 @@ static int ingenic_battery_get_property(struct > power_supply *psy, > val->intval = POWER_SUPPLY_HEALTH_GOOD; > return ret; > case POWER_SUPPLY_PROP_VOLTAGE_NOW: > - ret = iio_read_channel_processed(bat->channel, &val->intval); > - val->intval *= 1000; > + ret = iio_read_channel_processed_scale(bat->channel, > + &val->intval, > + 1000); > return ret; > case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > val->intval = info->voltage_min_design_uv;
On Sun, 23 Jun 2024 07:50:32 +0200, Christophe JAILLET wrote: > If iio_read_channel_processed() fails, 'val->intval' is not updated, but it > is still *1000 just after. So, in case of error, the *1000 accumulate and > 'val->intval' becomes erroneous. > > So instead of rescaling the value after the fact, use the dedicated scaling > API. This way the result is updated only when needed. In case of error, the > previous value is kept, unmodified. > > [...] Applied, thanks! [1/1] power: supply: ingenic: Fix some error handling paths in ingenic_battery_get_property() commit: f8b6c1eb76f73ed721facd58d0cfb08513aad34c Best regards,
diff --git a/drivers/power/supply/ingenic-battery.c b/drivers/power/supply/ingenic-battery.c index 2e7fdfde47ec..0a40f425c277 100644 --- a/drivers/power/supply/ingenic-battery.c +++ b/drivers/power/supply/ingenic-battery.c @@ -31,8 +31,9 @@ static int ingenic_battery_get_property(struct power_supply *psy, switch (psp) { case POWER_SUPPLY_PROP_HEALTH: - ret = iio_read_channel_processed(bat->channel, &val->intval); - val->intval *= 1000; + ret = iio_read_channel_processed_scale(bat->channel, + &val->intval, + 1000); if (val->intval < info->voltage_min_design_uv) val->intval = POWER_SUPPLY_HEALTH_DEAD; else if (val->intval > info->voltage_max_design_uv) @@ -41,8 +42,9 @@ static int ingenic_battery_get_property(struct power_supply *psy, val->intval = POWER_SUPPLY_HEALTH_GOOD; return ret; case POWER_SUPPLY_PROP_VOLTAGE_NOW: - ret = iio_read_channel_processed(bat->channel, &val->intval); - val->intval *= 1000; + ret = iio_read_channel_processed_scale(bat->channel, + &val->intval, + 1000); return ret; case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: val->intval = info->voltage_min_design_uv;
If iio_read_channel_processed() fails, 'val->intval' is not updated, but it is still *1000 just after. So, in case of error, the *1000 accumulate and 'val->intval' becomes erroneous. So instead of rescaling the value after the fact, use the dedicated scaling API. This way the result is updated only when needed. In case of error, the previous value is kept, unmodified. This should also reduce any inaccuracies resulting from the scaling. Finally, this is also slightly more efficient as it saves a function call and a multiplication. Fixes: fb24ccfbe1e0 ("power: supply: add Ingenic JZ47xx battery driver.") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/power/supply/ingenic-battery.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)