1. Patchew
  2. linux
  3. View series

[PATCH] iio: adc: mt6360: Handle error in cleanup path correctly

Haotian Zhang posted 1 patch 4 months, 1 week ago
There is a newer version of this series
drivers/iio/adc/mt6360-adc.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH] iio: adc: mt6360: Handle error in cleanup path correctly
Posted by Haotian Zhang 4 months, 1 week ago 
The return value of a regmap_raw_write() in the cleanup path was
being ignored.

Fix this by checking the return value and propagating the error.

Fixes: 1f4877218f7e ("iio: adc: mt6360: Add ADC driver for MT6360")
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
 drivers/iio/adc/mt6360-adc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
index 69b3569c90e5..97c4af8a93fc 100644
--- a/drivers/iio/adc/mt6360-adc.c
+++ b/drivers/iio/adc/mt6360-adc.c
@@ -70,6 +70,7 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
 	ktime_t predict_end_t, timeout;
 	unsigned int pre_wait_time;
 	int ret;
+	int cleanup_ret;
 
 	mutex_lock(&mad->adc_lock);
 
@@ -130,11 +131,15 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
 out_adc_conv:
 	/* Only keep ADC enable */
 	adc_enable = cpu_to_be16(MT6360_ADCEN_MASK);
-	regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, &adc_enable, sizeof(adc_enable));
+	cleanup_ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG,
+				&adc_enable, sizeof(adc_enable));
+	if (ret >= 0)
+		ret = cleanup_ret;
 	mad->last_off_timestamps[channel] = ktime_get();
 	/* Config prefer channel to NO_PREFER */
 	regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
 			   MT6360_NO_PREFER << MT6360_PREFERCH_SHFT);
+
 out_adc_lock:
 	mutex_unlock(&mad->adc_lock);
 
-- 
2.50.1.windows.1
Re: [PATCH] iio: adc: mt6360: Handle error in cleanup path correctly
Posted by David Lechner 4 months, 1 week ago 
It looks like you missed the IIO mailing list in the CC, so this might
not show up in Jonathan's queue.

On Mon, Sep 29, 2025 at 4:54 AM Haotian Zhang <vulab@iscas.ac.cn> wrote:
>
> The return value of a regmap_raw_write() in the cleanup path was
> being ignored.
>
> Fix this by checking the return value and propagating the error.
>
> Fixes: 1f4877218f7e ("iio: adc: mt6360: Add ADC driver for MT6360")
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> ---
>  drivers/iio/adc/mt6360-adc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> index 69b3569c90e5..97c4af8a93fc 100644
> --- a/drivers/iio/adc/mt6360-adc.c
> +++ b/drivers/iio/adc/mt6360-adc.c
> @@ -70,6 +70,7 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
>         ktime_t predict_end_t, timeout;
>         unsigned int pre_wait_time;
>         int ret;
> +       int cleanup_ret;
>
>         mutex_lock(&mad->adc_lock);
>
> @@ -130,11 +131,15 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
>  out_adc_conv:
>         /* Only keep ADC enable */
>         adc_enable = cpu_to_be16(MT6360_ADCEN_MASK);
> -       regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, &adc_enable, sizeof(adc_enable));
> +       cleanup_ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG,
> +                               &adc_enable, sizeof(adc_enable));
> +       if (ret >= 0)
> +               ret = cleanup_ret;

This overwrites the original return error, which is IIO_VAL_INT on
success or an error code. In either case, changing the return value
will break things.

In cleanup paths, there really isn't anything we can do with return
values other than ignore them like we were already doing or log an
error message.

>         mad->last_off_timestamps[channel] = ktime_get();
>         /* Config prefer channel to NO_PREFER */
>         regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,

The return value here is also being ignored. If we decide that we
should add error messages, then we should add one here as well.

>                            MT6360_NO_PREFER << MT6360_PREFERCH_SHFT);
> +
>  out_adc_lock:
>         mutex_unlock(&mad->adc_lock);
>
> --
> 2.50.1.windows.1
>
[PATCH v2] iio: adc: mt6360: Handle error in cleanup path correctly
Posted by Haotian Zhang 4 months, 1 week ago 
The return value of a regmap_raw_write() and regmap_update_bits()
in the cleanup path was being ignored.

Fix this by checking the return value and warn on error.

Fixes: 1f4877218f7e ("iio: adc: mt6360: Add ADC driver for MT6360")
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>

---
Changes in v2:
 - Do not propagate cleanup path errors.
 - Log a warning on failure instead of overwriting the return value, as
   suggested by the maintainer.
 - Also check the return value of regmap_update_bits() for consistency.
---
 drivers/iio/adc/mt6360-adc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
index 69b3569c90e5..9ee7247aacbe 100644
--- a/drivers/iio/adc/mt6360-adc.c
+++ b/drivers/iio/adc/mt6360-adc.c
@@ -70,6 +70,7 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
 	ktime_t predict_end_t, timeout;
 	unsigned int pre_wait_time;
 	int ret;
+	int cleanup_ret;
 
 	mutex_lock(&mad->adc_lock);
 
@@ -130,11 +131,16 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
 out_adc_conv:
 	/* Only keep ADC enable */
 	adc_enable = cpu_to_be16(MT6360_ADCEN_MASK);
-	regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, &adc_enable, sizeof(adc_enable));
+	cleanup_ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG,
+				&adc_enable, sizeof(adc_enable));
+	if (cleanup_ret)
+		dev_warn(mad->dev, "Failed to reset ADC config: %d\n", cleanup_ret);
 	mad->last_off_timestamps[channel] = ktime_get();
 	/* Config prefer channel to NO_PREFER */
-	regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
+	cleanup_ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
 			   MT6360_NO_PREFER << MT6360_PREFERCH_SHFT);
+	if (cleanup_ret)
+		dev_warn(mad->dev, "Failed to reset prefer channel: %d\n", cleanup_ret);
 out_adc_lock:
 	mutex_unlock(&mad->adc_lock);
 
-- 
2.50.1.windows.1
Re: [PATCH v2] iio: adc: mt6360: Handle error in cleanup path correctly
Posted by Jonathan Cameron 4 months ago 
On Tue, 30 Sep 2025 00:24:53 +0800
Haotian Zhang <vulab@iscas.ac.cn> wrote:

> The return value of a regmap_raw_write() and regmap_update_bits()
> in the cleanup path was being ignored.
> 
> Fix this by checking the return value and warn on error.
> 
> Fixes: 1f4877218f7e ("iio: adc: mt6360: Add ADC driver for MT6360")
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> 
> ---
> Changes in v2:
>  - Do not propagate cleanup path errors.
>  - Log a warning on failure instead of overwriting the return value, as
>    suggested by the maintainer.

As below. I think dev_err() is appropriate.

>  - Also check the return value of regmap_update_bits() for consistency.
> ---
>  drivers/iio/adc/mt6360-adc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/mt6360-adc.c b/drivers/iio/adc/mt6360-adc.c
> index 69b3569c90e5..9ee7247aacbe 100644
> --- a/drivers/iio/adc/mt6360-adc.c
> +++ b/drivers/iio/adc/mt6360-adc.c
> @@ -70,6 +70,7 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
>  	ktime_t predict_end_t, timeout;
>  	unsigned int pre_wait_time;
>  	int ret;
> +	int cleanup_ret;
>  
>  	mutex_lock(&mad->adc_lock);
>  
> @@ -130,11 +131,16 @@ static int mt6360_adc_read_channel(struct mt6360_adc_data *mad, int channel, int
>  out_adc_conv:
>  	/* Only keep ADC enable */
>  	adc_enable = cpu_to_be16(MT6360_ADCEN_MASK);
> -	regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG, &adc_enable, sizeof(adc_enable));
> +	cleanup_ret = regmap_raw_write(mad->regmap, MT6360_REG_PMUADCCFG,
> +				&adc_enable, sizeof(adc_enable));
> +	if (cleanup_ret)
> +		dev_warn(mad->dev, "Failed to reset ADC config: %d\n", cleanup_ret);
Why warn? If this happens it's definite and error and may bite us later.

>  	mad->last_off_timestamps[channel] = ktime_get();
>  	/* Config prefer channel to NO_PREFER */
> -	regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> +	cleanup_ret = regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
>  			   MT6360_NO_PREFER << MT6360_PREFERCH_SHFT);
> +	if (cleanup_ret)
> +		dev_warn(mad->dev, "Failed to reset prefer channel: %d\n", cleanup_ret);
>  out_adc_lock:
>  	mutex_unlock(&mad->adc_lock);
>  

Maybe it is worth trying to surface the error if nothing else has already gone wrong. That is a little fiddly
to do but something like

	if (cleanup_ret) {
		dev_err()
		ret = ret ?: cleanup_ret;
	}

I'm not sure it is worth the complexity however, so perhaps see if others have comments on this
in next few days before spinning a v3.

Thanks,

Jonathan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.