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@xxxxxxxxx> wrote:

> On Fri, Nov 13, 2026 at 10:41 AM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Fri, Nov 13, 2026 at 10:25 AM Zhu Jun <zhujun2@xxxxxxxxxxxxxxxxxxxx> 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;
> > >  }  
> 
>