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;
-- 
2.45.2

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,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

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;