[PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[Ivy Lopez]

wm8350_read_auxadc() logs an error via dev_err() when the AUX ADC
read times out, but then falls through to return the masked value
of `result`, which was left at its initial value of 0. Callers have
no way to distinguish a genuine 0V reading from a failed read.

Return -ETIMEDOUT on timeout instead, and update the wm8350 hwmon
and power_supply drivers, which call this function directly, to
check for a negative return and propagate the error rather than
using it as a voltage value.

Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/hwmon/wm8350-hwmon.c        |  9 ++++--
 drivers/mfd/wm8350-core.c           | 14 ++++++---
 drivers/power/supply/wm8350_power.c | 45 ++++++++++++++++++++++-------
 3 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/wm8350-hwmon.c b/drivers/hwmon/wm8350-hwmon.c
index 10e17bb79895..a455e1c5476c 100644
--- a/drivers/hwmon/wm8350-hwmon.c
+++ b/drivers/hwmon/wm8350-hwmon.c
@@ -23,14 +23,17 @@ static const char * const input_names[] = {
 };
 
 static ssize_t show_voltage(struct device *dev,
-			    struct device_attribute *attr, char *buf)
+			  struct device_attribute *attr, char *buf)
 {
 	struct wm8350 *wm8350 = dev_get_drvdata(dev);
 	int channel = to_sensor_dev_attr(attr)->index;
 	int val;
 
-	val = wm8350_read_auxadc(wm8350, channel, 0, 0) * WM8350_AUX_COEFF;
-	val = DIV_ROUND_CLOSEST(val, 1000);
+	val = wm8350_read_auxadc(wm8350, channel, 0, 0);
+	if (val < 0)
+		return val;
+
+	val = DIV_ROUND_CLOSEST(val * WM8350_AUX_COEFF, 1000);
 
 	return sprintf(buf, "%d\n", val);
 }
diff --git a/drivers/mfd/wm8350-core.c b/drivers/mfd/wm8350-core.c
index fbc77b218215..3120c6b8fa3f 100644
--- a/drivers/mfd/wm8350-core.c
+++ b/drivers/mfd/wm8350-core.c
@@ -188,6 +188,7 @@ EXPORT_SYMBOL_GPL(wm8350_reg_unlock);
 int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref)
 {
 	u16 reg, result = 0;
+	int ret = 0;
 
 	if (channel < WM8350_AUXADC_AUX1 || channel > WM8350_AUXADC_TEMP)
 		return -EINVAL;
@@ -221,19 +222,24 @@ int wm8350_read_auxadc(struct wm8350 *wm8350, int channel, int scale, int vref)
 	wait_for_completion_timeout(&wm8350->auxadc_done, msecs_to_jiffies(5));
 
 	reg = wm8350_reg_read(wm8350, WM8350_DIGITISER_CONTROL_1);
-	if (reg & WM8350_AUXADC_POLL)
+	if (reg & WM8350_AUXADC_POLL) {
 		dev_err(wm8350->dev, "adc chn %d read timeout\n", channel);
-	else
+		ret = -ETIMEDOUT;
+	} else {
 		result = wm8350_reg_read(wm8350,
-					 WM8350_AUX1_READBACK + channel);
+					WM8350_AUX1_READBACK + channel);
+	}
 
 	/* Turn off the ADC */
 	reg = wm8350_reg_read(wm8350, WM8350_POWER_MGMT_5);
 	wm8350_reg_write(wm8350, WM8350_POWER_MGMT_5,
-			 reg & ~WM8350_AUXADC_ENA);
+			reg & ~WM8350_AUXADC_ENA);
 
 	mutex_unlock(&wm8350->auxadc_mutex);
 
+	if (ret)
+		return ret;
+
 	return result & WM8350_AUXADC_DATA1_MASK;
 }
 EXPORT_SYMBOL_GPL(wm8350_read_auxadc);
diff --git a/drivers/power/supply/wm8350_power.c b/drivers/power/supply/wm8350_power.c
index b0eb6e0ce8bc..111373027905 100644
--- a/drivers/power/supply/wm8350_power.c
+++ b/drivers/power/supply/wm8350_power.c
@@ -19,20 +19,32 @@
 
 static int wm8350_read_battery_uvolts(struct wm8350 *wm8350)
 {
-	return wm8350_read_auxadc(wm8350, WM8350_AUXADC_BATT, 0, 0)
-		* WM8350_AUX_COEFF;
+	int val = wm8350_read_auxadc(wm8350, WM8350_AUXADC_BATT, 0, 0);
+
+	if (val < 0)
+		return val;
+
+	return val * WM8350_AUX_COEFF;
 }
 
 static int wm8350_read_line_uvolts(struct wm8350 *wm8350)
 {
-	return wm8350_read_auxadc(wm8350, WM8350_AUXADC_LINE, 0, 0)
-		* WM8350_AUX_COEFF;
+	int val = wm8350_read_auxadc(wm8350, WM8350_AUXADC_LINE, 0, 0);
+
+	if (val < 0)
+		return val;
+
+	return val * WM8350_AUX_COEFF;
 }
 
 static int wm8350_read_usb_uvolts(struct wm8350 *wm8350)
 {
-	return wm8350_read_auxadc(wm8350, WM8350_AUXADC_USB, 0, 0)
-		* WM8350_AUX_COEFF;
+	int val = wm8350_read_auxadc(wm8350, WM8350_AUXADC_USB, 0, 0);
+
+	if (val < 0)
+		return val;
+
+	return val * WM8350_AUX_COEFF;
 }
 
 #define WM8350_BATT_SUPPLY	1
@@ -257,7 +269,10 @@ static int wm8350_ac_get_prop(struct power_supply *psy,
 				 WM8350_LINE_SUPPLY);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = wm8350_read_line_uvolts(wm8350);
+		ret = wm8350_read_line_uvolts(wm8350);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	default:
 		ret = -EINVAL;
@@ -287,7 +302,10 @@ static int wm8350_usb_get_prop(struct power_supply *psy,
 				 WM8350_USB_SUPPLY);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = wm8350_read_usb_uvolts(wm8350);
+		ret = wm8350_read_usb_uvolts(wm8350);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	default:
 		ret = -EINVAL;
@@ -308,8 +326,12 @@ static enum power_supply_property wm8350_usb_props[] = {
 static int wm8350_bat_check_health(struct wm8350 *wm8350)
 {
 	u16 reg;
+	int uvolts = wm8350_read_battery_uvolts(wm8350);
+
+	if (uvolts < 0)
+		return POWER_SUPPLY_HEALTH_UNKNOWN;
 
-	if (wm8350_read_battery_uvolts(wm8350) < 2850000)
+	if (uvolts < 2850000)
 		return POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
 
 	reg = wm8350_reg_read(wm8350, WM8350_CHARGER_OVERRIDES);
@@ -356,7 +378,10 @@ static int wm8350_bat_get_property(struct power_supply *psy,
 				 WM8350_BATT_SUPPLY);
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = wm8350_read_battery_uvolts(wm8350);
+		ret = wm8350_read_battery_uvolts(wm8350);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
 		val->intval = wm8350_bat_check_health(wm8350);
-- 
2.55.0

Re: [PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[Guenter Roeck]

On 8/4/26 18:42, Ivy Lopez wrote:
> wm8350_read_auxadc() logs an error via dev_err() when the AUX ADC
> read times out, but then falls through to return the masked value
> of `result`, which was left at its initial value of 0. Callers have
> no way to distinguish a genuine 0V reading from a failed read.
> 
> Return -ETIMEDOUT on timeout instead, and update the wm8350 hwmon
> and power_supply drivers, which call this function directly, to
> check for a negative return and propagate the error rather than
> using it as a voltage value.
> 

I am not sure if this warrants fixing in a 17+ year old driver,
unless it is observed to be a real problem. It appears to me that the
error handling was likely not implemented on purpose.

On top of that, as is seen a lot lately, this fixes a rare issue
that is likely never going to be seen in the real world while not
addressing the many real problems in the driver (as reported by Sashiko).

Guenter

Re: [PATCH] mfd: wm8350: propagate ADC read timeout instead of returning 0

[Ivy Lopez]

On 8/5/26, Guenter Roeck wrote:
> I am not sure if this warrants fixing in a 17+ year old driver,
> unless it is observed to be a real problem. It appears to me that the
> error handling was likely not implemented on purpose.
>
> On top of that, as is seen a lot lately, this fixes a rare issue
> that is likely never going to be seen in the real world while not
> addressing the many real problems in the driver (as reported by Sashiko).

That's fair, sure. The automated review also caught a real regression
in this patch itself: reusing `ret` for the voltage value in the
power_supply get_property callbacks means a successful read returns
a nonzero value instead of 0, which breaks the normal
if (power_supply_get_property(...)) error-checking convention for
callers which is on my end.. I'll drop this patch rather than resubmit a v2 for an issue
you've said isn't worth fixing here.

Thanks,
ivy