[PATCH 0/3] power: supply: ab8500: Improve code related to iio_read_channel_processed() and fix a bug

[PATCH 0/3] power: supply: ab8500: Improve code related to iio_read_channel_processed() and fix a bug

[Christophe JAILLET]

This series is inspired by a patch submitted at [1].

While looking if the same pattern was relevant elsewhere, I ended in
ab8500_charger.c.

Patch 1 fixes what looks to me as a regression introduced by
97ab78bac5d0.

Patch 2 is the initial goal of this series. That is to change some
iio_read_channel_processed() + multiplication by a single, cleaner,
iio_read_channel_processed_scale().

Patch 3 is a cosmetic change spotted while at it.


Honestly, I don't have a strong opinion if patch 2 helps in any way
(explanation or confirmation would be appreciated for my own
knowledge), but at least patch 1 deserves a look and seems value to me.

CJ

[1]: https://lore.kernel.org/all/20240620212005.821805-1-sean.anderson@linux.dev/

Christophe JAILLET (3):
  power: supply: ab8500: Fix error handling when calling
    iio_read_channel_processed()
  power: supply: ab8500: Use iio_read_channel_processed_scale()
  power: supply: ab8500: Clean some error messages

 drivers/power/supply/ab8500_charger.c | 52 ++++++++++++++++-----------
 1 file changed, 32 insertions(+), 20 deletions(-)

-- 
2.45.2

[PATCH 1/3] power: supply: ab8500: Fix error handling when calling iio_read_channel_processed()

[Christophe JAILLET]

The ab8500_charger_get_[ac|vbus]_[current|voltage]() functions should
return an error code on error.

Up to now, an un-initialized value is returned.
This makes the error handling of the callers un-reliable.

Return the error code instead, to fix the issue.

Fixes: 97ab78bac5d0 ("power: supply: ab8500_charger: Convert to IIO ADC")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only, but should be fine because it restores the behavior
before 97ab78bac5d0.
---
 drivers/power/supply/ab8500_charger.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/ab8500_charger.c b/drivers/power/supply/ab8500_charger.c
index 9b34d1a60f66..4b0ad1b4b4c9 100644
--- a/drivers/power/supply/ab8500_charger.c
+++ b/drivers/power/supply/ab8500_charger.c
@@ -488,8 +488,10 @@ static int ab8500_charger_get_ac_voltage(struct ab8500_charger *di)
 	/* Only measure voltage if the charger is connected */
 	if (di->ac.charger_connected) {
 		ret = iio_read_channel_processed(di->adc_main_charger_v, &vch);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			return ret;
+		}
 	} else {
 		vch = 0;
 	}
@@ -540,8 +542,10 @@ static int ab8500_charger_get_vbus_voltage(struct ab8500_charger *di)
 	/* Only measure voltage if the charger is connected */
 	if (di->usb.charger_connected) {
 		ret = iio_read_channel_processed(di->adc_vbus_v, &vch);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			return ret;
+		}
 	} else {
 		vch = 0;
 	}
@@ -563,8 +567,10 @@ static int ab8500_charger_get_usb_current(struct ab8500_charger *di)
 	/* Only measure current if the charger is online */
 	if (di->usb.charger_online) {
 		ret = iio_read_channel_processed(di->adc_usb_charger_c, &ich);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			return ret;
+		}
 	} else {
 		ich = 0;
 	}
@@ -586,8 +592,10 @@ static int ab8500_charger_get_ac_current(struct ab8500_charger *di)
 	/* Only measure current if the charger is online */
 	if (di->ac.charger_online) {
 		ret = iio_read_channel_processed(di->adc_main_charger_c, &ich);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			return ret;
+		}
 	} else {
 		ich = 0;
 	}
-- 
2.45.2

Re: [PATCH 1/3] power: supply: ab8500: Fix error handling when calling iio_read_channel_processed()

[Jonathan Cameron]

On Sat, 22 Jun 2024 09:04:24 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> The ab8500_charger_get_[ac|vbus]_[current|voltage]() functions should
> return an error code on error.
> 
> Up to now, an un-initialized value is returned.
> This makes the error handling of the callers un-reliable.
> 
> Return the error code instead, to fix the issue.
> 
> Fixes: 97ab78bac5d0 ("power: supply: ab8500_charger: Convert to IIO ADC")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Looks right to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 1/3] power: supply: ab8500: Fix error handling when calling iio_read_channel_processed()

[Linus Walleij]

On Sat, Jun 22, 2024 at 9:05 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:

> The ab8500_charger_get_[ac|vbus]_[current|voltage]() functions should
> return an error code on error.
>
> Up to now, an un-initialized value is returned.
> This makes the error handling of the callers un-reliable.
>
> Return the error code instead, to fix the issue.
>
> Fixes: 97ab78bac5d0 ("power: supply: ab8500_charger: Convert to IIO ADC")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/3] power: supply: ab8500: Use iio_read_channel_processed_scale()

[Christophe JAILLET]

Instead of rescaling current or voltage channels after the fact, use the
dedicated scaling API. This should reduce any inaccuracies resulting from
the scaling.

This is also slightly more efficient as it saves a function call and a
multiplication.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
It is inspired by:
  https://lore.kernel.org/all/20240620212005.821805-1-sean.anderson@linux.dev/
---
 drivers/power/supply/ab8500_charger.c | 28 +++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/ab8500_charger.c b/drivers/power/supply/ab8500_charger.c
index 4b0ad1b4b4c9..2f06b93682ac 100644
--- a/drivers/power/supply/ab8500_charger.c
+++ b/drivers/power/supply/ab8500_charger.c
@@ -487,7 +487,9 @@ static int ab8500_charger_get_ac_voltage(struct ab8500_charger *di)
 
 	/* Only measure voltage if the charger is connected */
 	if (di->ac.charger_connected) {
-		ret = iio_read_channel_processed(di->adc_main_charger_v, &vch);
+		/* Convert to microvolt, IIO returns millivolt */
+		ret = iio_read_channel_processed_scale(di->adc_main_charger_v,
+						       &vch, 1000);
 		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
 			return ret;
@@ -495,8 +497,7 @@ static int ab8500_charger_get_ac_voltage(struct ab8500_charger *di)
 	} else {
 		vch = 0;
 	}
-	/* Convert to microvolt, IIO returns millivolt */
-	return vch * 1000;
+	return vch;
 }
 
 /**
@@ -541,7 +542,9 @@ static int ab8500_charger_get_vbus_voltage(struct ab8500_charger *di)
 
 	/* Only measure voltage if the charger is connected */
 	if (di->usb.charger_connected) {
-		ret = iio_read_channel_processed(di->adc_vbus_v, &vch);
+		/* Convert to microvolt, IIO returns millivolt */
+		ret = iio_read_channel_processed_scale(di->adc_vbus_v,
+						       &vch, 1000);
 		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
 			return ret;
@@ -549,8 +552,7 @@ static int ab8500_charger_get_vbus_voltage(struct ab8500_charger *di)
 	} else {
 		vch = 0;
 	}
-	/* Convert to microvolt, IIO returns millivolt */
-	return vch * 1000;
+	return vch;
 }
 
 /**
@@ -566,7 +568,9 @@ static int ab8500_charger_get_usb_current(struct ab8500_charger *di)
 
 	/* Only measure current if the charger is online */
 	if (di->usb.charger_online) {
-		ret = iio_read_channel_processed(di->adc_usb_charger_c, &ich);
+		/* Return microamperes */
+		ret = iio_read_channel_processed_scale(di->adc_usb_charger_c,
+						       &ich, 1000);
 		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
 			return ret;
@@ -574,8 +578,7 @@ static int ab8500_charger_get_usb_current(struct ab8500_charger *di)
 	} else {
 		ich = 0;
 	}
-	/* Return microamperes */
-	return ich * 1000;
+	return ich;
 }
 
 /**
@@ -591,7 +594,9 @@ static int ab8500_charger_get_ac_current(struct ab8500_charger *di)
 
 	/* Only measure current if the charger is online */
 	if (di->ac.charger_online) {
-		ret = iio_read_channel_processed(di->adc_main_charger_c, &ich);
+		/* Return microamperes */
+		ret = iio_read_channel_processed_scale(di->adc_main_charger_c,
+						       &ich, 1000);
 		if (ret < 0) {
 			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
 			return ret;
@@ -599,8 +604,7 @@ static int ab8500_charger_get_ac_current(struct ab8500_charger *di)
 	} else {
 		ich = 0;
 	}
-	/* Return microamperes */
-	return ich * 1000;
+	return ich;
 }
 
 /**
-- 
2.45.2

Re: [PATCH 2/3] power: supply: ab8500: Use iio_read_channel_processed_scale()

[Jonathan Cameron]

On Sat, 22 Jun 2024 09:04:25 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Instead of rescaling current or voltage channels after the fact, use the
> dedicated scaling API. This should reduce any inaccuracies resulting from
> the scaling.
> 
> This is also slightly more efficient as it saves a function call and a
> multiplication.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 2/3] power: supply: ab8500: Use iio_read_channel_processed_scale()

[Linus Walleij]

On Sat, Jun 22, 2024 at 9:05 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:

> Instead of rescaling current or voltage channels after the fact, use the
> dedicated scaling API. This should reduce any inaccuracies resulting from
> the scaling.
>
> This is also slightly more efficient as it saves a function call and a
> multiplication.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Very nice!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 3/3] power: supply: ab8500: Clean some error messages

[Christophe JAILLET]

There is an useless extra comma at the end of some error messages, remove
them.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/power/supply/ab8500_charger.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/ab8500_charger.c b/drivers/power/supply/ab8500_charger.c
index 2f06b93682ac..93181ebfb324 100644
--- a/drivers/power/supply/ab8500_charger.c
+++ b/drivers/power/supply/ab8500_charger.c
@@ -491,7 +491,7 @@ static int ab8500_charger_get_ac_voltage(struct ab8500_charger *di)
 		ret = iio_read_channel_processed_scale(di->adc_main_charger_v,
 						       &vch, 1000);
 		if (ret < 0) {
-			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			dev_err(di->dev, "%s ADC conv failed\n", __func__);
 			return ret;
 		}
 	} else {
@@ -546,7 +546,7 @@ static int ab8500_charger_get_vbus_voltage(struct ab8500_charger *di)
 		ret = iio_read_channel_processed_scale(di->adc_vbus_v,
 						       &vch, 1000);
 		if (ret < 0) {
-			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			dev_err(di->dev, "%s ADC conv failed\n", __func__);
 			return ret;
 		}
 	} else {
@@ -572,7 +572,7 @@ static int ab8500_charger_get_usb_current(struct ab8500_charger *di)
 		ret = iio_read_channel_processed_scale(di->adc_usb_charger_c,
 						       &ich, 1000);
 		if (ret < 0) {
-			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			dev_err(di->dev, "%s ADC conv failed\n", __func__);
 			return ret;
 		}
 	} else {
@@ -598,7 +598,7 @@ static int ab8500_charger_get_ac_current(struct ab8500_charger *di)
 		ret = iio_read_channel_processed_scale(di->adc_main_charger_c,
 						       &ich, 1000);
 		if (ret < 0) {
-			dev_err(di->dev, "%s ADC conv failed,\n", __func__);
+			dev_err(di->dev, "%s ADC conv failed\n", __func__);
 			return ret;
 		}
 	} else {
-- 
2.45.2

Re: [PATCH 3/3] power: supply: ab8500: Clean some error messages

[Jonathan Cameron]

On Sat, 22 Jun 2024 09:04:26 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> There is an useless extra comma at the end of some error messages, remove
> them.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Odd indeed.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 3/3] power: supply: ab8500: Clean some error messages

[Linus Walleij]

On Sat, Jun 22, 2024 at 9:05 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:

> There is an useless extra comma at the end of some error messages, remove
> them.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 0/3] power: supply: ab8500: Improve code related to iio_read_channel_processed() and fix a bug

[Sebastian Reichel]

On Sat, 22 Jun 2024 09:04:23 +0200, Christophe JAILLET wrote:
> This series is inspired by a patch submitted at [1].
> 
> While looking if the same pattern was relevant elsewhere, I ended in
> ab8500_charger.c.
> 
> Patch 1 fixes what looks to me as a regression introduced by
> 97ab78bac5d0.
> 
> [...]

Applied, thanks!

[1/3] power: supply: ab8500: Fix error handling when calling iio_read_channel_processed()
      commit: 3288757087cbb93b91019ba6b7de53a1908c9d48
[2/3] power: supply: ab8500: Use iio_read_channel_processed_scale()
      commit: dc6ce568afd3452ac682261ea0db570d28f7d82d
[3/3] power: supply: ab8500: Clean some error messages
      commit: f62b267adcac33c64a26ec55973dad92bc8a8358

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>