Re: [PATCH v2] hwmon: (ads7871) Fix incorrect error code in voltage_show

[Guenter Roeck <linux@roeck-us.net>]

On Sat, Mar 07, 2026 at 05:22:26PM +0530, Tabrez Ahmed wrote:
> The voltage_show() function returns -1 when the A/D conversion
> fails to complete within the polling loop. -1 maps to -EPERM
> (operation not permitted), which does not describe the actual
> failure.
> 
> Replace this -1 error code with -ETIMEDOUT to better indicate
> the timeout condition to userspace.
> 
> Drop the else block after return.
> 
> Note: not runtime tested due to lack of hardware.
> 
> Signed-off-by: Tabrez Ahmed <tabreztalks@gmail.com>
> ---
> Changes in v2:
> - Dropped unnecessary 'else' block after return as suggested by Guenter Roeck.
> 
> Note: This patch applies on top of my previously submitted patch:
> "hwmon: (ads7871) Replace sprintf() with sysfs_emit()"
> 
>  drivers/hwmon/ads7871.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
> index b84426c940c5e..753bf77ce19b4 100644
> --- a/drivers/hwmon/ads7871.c
> +++ b/drivers/hwmon/ads7871.c
> @@ -125,9 +125,9 @@ static ssize_t voltage_show(struct device *dev, struct device_attribute *da,
>  		/*result in volts*10000 = (val/8192)*2.5*10000*/
>  		val = ((val >> 2) * 25000) / 8192;
>  		return sysfs_emit(buf, "%d\n", val);
> -	} else {
> -		return -1;
>  	}
> +
> +	return -ETIMEDOUT;

AI feedback below. Somewhat unrelated, but it turns out the driver has a number
of problems. I'll apply the two patches, but that won't fix the real problems
with the driver.

  Does this code mask actual SPI errors?
  If ads7871_read_reg8() fails, it returns a negative error code (e.g., -EIO).
  Since MUX_CNV_BM is 0x80, any standard negative error code will have the 7th bit
  set, meaning mux_cnv evaluates to 1:
	ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
	mux_cnv = ((ret & MUX_CNV_BM) >> MUX_CNV_BV);
  This causes the polling loop to continue and eventually return -ETIMEDOUT
  here. Should we check if ret < 0 and propagate the actual SPI error code
  instead of unconditionally returning -ETIMEDOUT?
  Also, since this attribute is registered via devm_hwmon_device_register_with_groups()
  rather than the _with_info() variant, the hwmon core does not serialize accesses.
  If userspace reads multiple channels concurrently, could they overwrite each
  other's REG_GAIN_MUX writes before the conversion completes? Should we add
  locking to serialize this function?
  Additionally, since we are improving error reporting, should we also check the
  return values of ads7871_write_reg8() and ads7871_read_reg16() to prevent
  operating on or returning bogus data when SPI transfers fail?

That driver would really benefit from a conversion to the with_info API.

Thanks,
Guenter