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

[Jonathan Cameron <jic23@kernel.org>]

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)