Re: [PATCH v6 3/4] iio: adc: qcom-pm8xxx-xoadc: remove redundant error logs when reading values

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The timeout value passed to wait_for_completion_timeout() is in microseconds but the function expects jiffies, resulting in a massively inflated wait time.
- [High] The timeout error path in pm8xxx_read_channel_rsv() fails to disable the ADC arbiter, which can cause delayed interrupts to corrupt subsequent readings.
- [Medium] Removing the error log for -ETIMEDOUT hides legitimate hardware and driver failures from dmesg.
--

commit 24271541e5551b415dc4bb4616582d63863e0096
Author: Antony Kurniawan Soemardi <linux@smankusors.com>

iio: adc: qcom-pm8xxx-xoadc: remove redundant error logs when reading values

This commit drops dev_err() logging for -EINVAL and -ETIMEDOUT cases and relies
on return values to report errors, reducing unnecessary log noise.

> diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> index 31f88cf7f7f18..282a67b46a5eb 100644
> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> @@ -535,10 +535,7 @@ static int pm8xxx_read_channel_rsv(struct pm8xxx_xoadc *adc,
>  		goto unlock;
>  
>  	/* Next the interrupt occurs */
> -	ret = wait_for_completion_timeout(&adc->complete,
> -					  VADC_CONV_TIME_MAX_US);
> -	if (!ret) {
> -		dev_err(adc->dev, "conversion timed out\n");
> +	if (!wait_for_completion_timeout(&adc->complete, VADC_CONV_TIME_MAX_US)) {

This is a pre-existing issue, but wait_for_completion_timeout() expects a
timeout in jiffies. Does passing VADC_CONV_TIME_MAX_US directly result in a
massively inflated wait time instead of the intended microseconds?

Also, while removing the log for an invalid channel addresses user input
noise, does completely suppressing the -ETIMEDOUT error hide legitimate
hardware or interrupt failures from dmesg? Would dev_err_ratelimited() be a
better alternative here?

>  		ret = -ETIMEDOUT;
>  		goto unlock;

This is also a pre-existing issue, but if the timeout occurs and the code
jumps directly to the unlock label, it bypasses turning off the ADC arbiter
which is handled later in the function:

pm8xxx_read_channel_rsv() {
    ...
    /* Turn off the ADC by setting the arbiter to 0 twice */
    ret = regmap_write(adc->map, ADC_ARB_USRP_CNTRL, 0);
    if (ret)
        goto unlock;
    ...
}

Does jumping to unlock leave the hardware running with its interrupt armed,
potentially causing a delayed interrupt to fire and corrupt a subsequent
reading?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-pm8xxx-xoadc-label-v6-0-49700fd03005@smankusors.com?part=3