[PATCH] iio: potentiostat: lmp91000: Handle configuration write errors

[PATCH] iio: potentiostat: lmp91000: Handle configuration write errors

[Zhu Jun]

lmp91000_read_config() ignores errors when unlocking and programming
the TIA and reference registers. Probe can then succeed even though
the hardware configuration does not match the parameters used to
interpret measurements.

Check the configuration writes and return the first error. After a
successful unlock, attempt to restore write protection even if a
configuration write fails, and report a relock failure as well.

Fixes: 67e17300dc1d ("iio: potentiostat: add LMP91000 support")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
 drivers/iio/potentiostat/lmp91000.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 00cb3fea97ca..4a29fe627af7 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -205,7 +205,7 @@ static int lmp91000_read_config(struct lmp91000_data *data)
 {
 	struct device *dev = data->dev;
 	unsigned int reg, val;
-	int i, ret;
+	int i, ret, lock_ret;
 
 	ret = device_property_read_u32(dev, "ti,tia-gain-ohm", &val);
 	if (ret) {
@@ -250,13 +250,19 @@ static int lmp91000_read_config(struct lmp91000_data *data)
 		return ret;
 	}
 
-	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
-	regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
-	regmap_write(data->regmap, LMP91000_REG_REFCN,
-		     LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
-	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
+	ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
+	if (ret)
+		return ret;
 
-	return 0;
+	ret = regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
+	if (!ret)
+		ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
+				   LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
+
+	/* Restore write protection even if programming the registers failed. */
+	lock_ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
+
+	return ret ? ret : lock_ret;
 }
 
 static int lmp91000_buffer_cb(const void *val, void *private)
-- 
2.51.0

Re: [PATCH] iio: potentiostat: lmp91000: Handle configuration write errors

[Andy Shevchenko]

On Thu, Nov 12, 2026 at 03:17:47PM +0800, Zhu Jun wrote:
> lmp91000_read_config() ignores errors when unlocking and programming
> the TIA and reference registers. Probe can then succeed even though
> the hardware configuration does not match the parameters used to
> interpret measurements.
> 
> Check the configuration writes and return the first error. After a
> successful unlock, attempt to restore write protection even if a
> configuration write fails, and report a relock failure as well.

...

> static int lmp91000_read_config(struct lmp91000_data *data)
>  {
>  	struct device *dev = data->dev;
>  	unsigned int reg, val;
> -	int i, ret;
> +	int i, ret, lock_ret;

While at it, it's better to split semantically different variables:

	int ret, lock_ret;
	int i;

> +	ret = regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
> +	if (!ret)
> +		ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
> +				   LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
> +
> +	/* Restore write protection even if programming the registers failed. */
> +	lock_ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
> +
> +	return ret ? ret : lock_ret;

It doesn't use any guard()() or so, hence just make a goto instead
(and use Elvis):

	ret = regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
	if (ret)
		goto restore_lock;

	ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
			   LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);

restore_lock:

	/* Restore write protection even if programming the registers failed. */
	lock_ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 1);

	return ret ?: lock_ret;

>  }

Alternatively, wrap the existing function (this will make code cleaner in some
maintainer's opinion).

...

In any case there is a question how may the caller distinguish the error from
register write and from the lock restoration? And what will it do in either of
the cases?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: potentiostat: lmp91000: Handle configuration write errors

[Jonathan Cameron]

On Thu, 12 Nov 2026 15:17:47 +0800
Zhu Jun <zhujun2@cmss.chinamobile.com> wrote:

> lmp91000_read_config() ignores errors when unlocking and programming
> the TIA and reference registers. Probe can then succeed even though
> the hardware configuration does not match the parameters used to
> interpret measurements.
> 
> Check the configuration writes and return the first error. After a
> successful unlock, attempt to restore write protection even if a
> configuration write fails, and report a relock failure as well.
> 
> Fixes: 67e17300dc1d ("iio: potentiostat: add LMP91000 support")
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>

Minor comment on the approach used inline.

Thanks,

Jonathan

> ---
>  drivers/iio/potentiostat/lmp91000.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
> index 00cb3fea97ca..4a29fe627af7 100644
> --- a/drivers/iio/potentiostat/lmp91000.c
> +++ b/drivers/iio/potentiostat/lmp91000.c
> @@ -205,7 +205,7 @@ static int lmp91000_read_config(struct lmp91000_data *data)
>  {
>  	struct device *dev = data->dev;
>  	unsigned int reg, val;
> -	int i, ret;
> +	int i, ret, lock_ret;
>  
>  	ret = device_property_read_u32(dev, "ti,tia-gain-ohm", &val);
>  	if (ret) {
> @@ -250,13 +250,19 @@ static int lmp91000_read_config(struct lmp91000_data *data)
>  		return ret;
>  	}
>  
> -	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
> -	regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
> -	regmap_write(data->regmap, LMP91000_REG_REFCN,
> -		     LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
> -	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
> +	ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
> +	if (ret)
> +		return ret;
>  
> -	return 0;
> +	ret = regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
> +	if (!ret)
> +		ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
> +				   LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
> +
> +	/* Restore write protection even if programming the registers failed. */
> +	lock_ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 1);

I'm never a fan of flows where we pick between ret values. tHey make for very messy
code.  It is easier to read with one line effectively duplicated.


	ret = regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
	if (ret)
		return ret;

	ret = regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
	if (ret)
		goto unlock;
	
	ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
			   LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
	if (ret)
		goto unlock;

	return regmap_write(data->regmap, LMP91000_REG_LOCK, 1);

unlock:
	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
	
	return ret;
	
> +
> +	return ret ? ret : lock_ret;
>  }
>  
>  static int lmp91000_buffer_cb(const void *val, void *private)