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

[PATCH v3] 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. Return a relock error when both configuration
writes succeeded.

lmp91000_probe() returns either error before registering the IIO device,
so the caller does not need to distinguish their source. Preserve the
configuration error if restoring write protection also fails.

Fixes: 67e17300dc1d ("iio: potentiostat: add LMP91000 support")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
Changes in v3:
- Declare the loop counter separately from the return code (Andy Shevchenko).
- Explain how probe handles configuration and relock failures.

Changes in v2:
- Use an explicit error path to restore write protection and return the
  original configuration error, as suggested by Jonathan Cameron.
- Return the final lock write result directly on the successful path.

 drivers/iio/potentiostat/lmp91000.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 00cb3fea97ca..1e07917ff76a 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -205,7 +205,8 @@ static int lmp91000_read_config(struct lmp91000_data *data)
 {
 	struct device *dev = data->dev;
 	unsigned int reg, val;
-	int i, ret;
+	int ret;
+	int i;
 
 	ret = device_property_read_u32(dev, "ti,tia-gain-ohm", &val);
 	if (ret) {
@@ -250,13 +251,25 @@ 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);
+	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 relock;
+
+	ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
+			   LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
+	if (ret)
+		goto relock;
+
+	return regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
+
+relock:
 	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
 
-	return 0;
+	return ret;
 }
 
 static int lmp91000_buffer_cb(const void *val, void *private)
-- 
2.51.0

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

[Andy Shevchenko]

On Fri, Nov 13, 2026 at 10:25 AM 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. Return a relock error when both configuration
> writes succeeded.
>
> lmp91000_probe() returns either error before registering the IIO device,
> so the caller does not need to distinguish their source. Preserve the
> configuration error if restoring write protection also fails.

...

> Changes in v3:
> - Declare the loop counter separately from the return code (Andy Shevchenko).

Now this makes no sense as you haven't added any new variable.

> - Explain how probe handles configuration and relock failures.

> -       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);
> +       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 relock;
> +
> +       ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
> +                          LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
> +       if (ret)
> +               goto relock;
> +
> +       return regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
> +
> +relock:
>         regmap_write(data->regmap, LMP91000_REG_LOCK, 1);

What's the point of duplicating this?
Before label you can easily do

  ret = 0;

relock:
  ...

>
> -       return 0;
> +       return ret;
>  }

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Nov 13, 2026 at 10:41 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Nov 13, 2026 at 10:25 AM Zhu Jun <zhujun2@cmss.chinamobile.com> wrote:

...

> > Check the configuration writes and return the first error. After a
> > successful unlock, attempt to restore write protection even if a
> > configuration write fails. Return a relock error when both configuration
> > writes succeeded.
> >
> > lmp91000_probe() returns either error before registering the IIO device,
> > so the caller does not need to distinguish their source. Preserve the
> > configuration error if restoring write protection also fails.

Btw, this is still unclear. See below.

...

> > -       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);
> > +       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 relock;
> > +
> > +       ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
> > +                          LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
> > +       if (ret)
> > +               goto relock;
> > +
> > +       return regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
> > +
> > +relock:
> >         regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
>
> What's the point of duplicating this?
> Before label you can easily do
>
>   ret = 0;
>
> relock:
>   ...

Ah, okay, I see that the relock error is returned only on success of
the configuration update. Why is this inconsistency? Really, you need
to elaborate first on what the caller is suppose to do when
1) unlock failed;
2) unlock was successful, but configuration was failed;
3) unlock and configuration was successful, but relock failed.

In all of these cases the user gets an error, but it doesn't know
which stage was failed. What is the caller supposed to do?

No need to send a new patch, let's keep discussion here to understand
the code flow better.

> > -       return 0;
> > +       return ret;
> >  }


-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Fri, 13 Nov 2026 10:47:32 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Nov 13, 2026 at 10:41 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Nov 13, 2026 at 10:25 AM Zhu Jun <zhujun2@cmss.chinamobile.com> wrote:  
> 
> ...
> 
> > > Check the configuration writes and return the first error. After a
> > > successful unlock, attempt to restore write protection even if a
> > > configuration write fails. Return a relock error when both configuration
> > > writes succeeded.
> > >
> > > lmp91000_probe() returns either error before registering the IIO device,
> > > so the caller does not need to distinguish their source. Preserve the
> > > configuration error if restoring write protection also fails.  
> 
> Btw, this is still unclear. See below.
> 
> ...
> 
> > > -       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);
> > > +       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 relock;
> > > +
> > > +       ret = regmap_write(data->regmap, LMP91000_REG_REFCN,
> > > +                          LMP91000_REG_REFCN_EXT_REF | LMP91000_REG_REFCN_50_ZERO);
> > > +       if (ret)
> > > +               goto relock;
> > > +
> > > +       return regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
> > > +
> > > +relock:
> > >         regmap_write(data->regmap, LMP91000_REG_LOCK, 1);  
> >
> > What's the point of duplicating this?
> > Before label you can easily do
> >
> >   ret = 0;
> >
> > relock:
> >   ...  
> 
> Ah, okay, I see that the relock error is returned only on success of
> the configuration update. Why is this inconsistency? Really, you need
> to elaborate first on what the caller is suppose to do when
> 1) unlock failed;
> 2) unlock was successful, but configuration was failed;
> 3) unlock and configuration was successful, but relock failed.
> 
> In all of these cases the user gets an error, but it doesn't know
> which stage was failed. What is the caller supposed to do?
> 
> No need to send a new patch, let's keep discussion here to understand
> the code flow better.

I think I requested this flow.  I didn't like v1 doing some dance
with ret2 and only return that if ret wasn't set.  Argument of the patch is
that we must make sure to unlock even on error.


> 
> > > -       return 0;
> > > +       return ret;
> > >  }  
> 
>