diff mbox series

power: supply: ingenic: Fix some error handling paths in ingenic_battery_get_property()

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

Commit Message

Christophe JAILLET June 23, 2024, 5:50 a.m. UTC
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(-)

Comments

Artur Rojek June 23, 2024, 11:26 a.m. UTC | #1
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;
Sebastian Reichel June 26, 2024, 1:54 p.m. UTC | #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,
diff mbox series

Patch

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;