Re: [PATCH v8 6/8] i2c: of-prober: Add GPIO support to simple helpers

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 12:06:16PM +0800, Chen-Yu Tsai wrote:
> On Thu, Oct 10, 2024 at 11:20 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Tue, Oct 08, 2024 at 03:34:25PM +0800, Chen-Yu Tsai wrote:
> > > Add GPIO support to the simple helpers for the I2C OF component prober.
> > > Components that the prober intends to probe likely require their
> > > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > > bring them out of reset before they will respond to probe attempts.
> > > Regulator supplies were handled in the previous patch.
> > >
> > > The assumption is that the same class of components to be probed are
> > > always connected in the same fashion with the same regulator supply
> > > and GPIO. The names may vary due to binding differences, but the
> > > physical layout does not change.
> > >
> > > This supports at most one GPIO pin. The user must specify the GPIO name,
> > > the polarity, and the amount of time to wait after the GPIO is toggled.
> > > Devices with more than one GPIO pin likely require specific power
> > > sequencing beyond what generic code can easily support.

...

> > > +     /* An empty string signals an unnamed GPIO */
> > > +     if (!ctx->opts->gpio_name[0])
> > > +             con_id = NULL;
> > > +     else
> > > +             con_id = ctx->opts->gpio_name;
> >
> > Can it use positive conditional?
> >
> >         if (ctx->opts->gpio_name[0])
> >                 con_id = ctx->opts->gpio_name;
> >         else
> >                 con_id = NULL;
> 
> You suggested writing it this way in your reply to v7. Please pick one.

Oh, whatever you will finish with then, sorry for the noise.

...

> > > +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> > > +{
> > > +     if (!ctx->gpiod)
> > > +             return;
> >
> > Do you need this check for the future patches?
> 
> Not sure I follow. The check is needed because this function is called
> in i2c_of_probe_simple_cleanup(), but the GPIO could have been released
> earlier in i2c_of_probe_simple_cleanup_early(), and that makes this
> function a no-op.

Do you have a known race condition then? This is bad. You shouldn't rely on
the sequence of events here, or the serialisation has to be added.

> The helpers for the release side are quite short, but the ones on the
> request side wrap some conditional and error handling. I think it's
> better to keep it symmetric?

Yes, but why do you need the above check, I didn't still get...
I.o.w. you think that there is a gap in time that (if no check) the GPIO
descriptor might be changed? But then how does it affect anyway the possibility
that it becomes not NULL even with the current code.

> > > +     /* Ignore error if GPIO is not in output direction */
> > > +     gpiod_set_value(ctx->gpiod, !ctx->opts->gpio_assert_to_enable);
> > > +}

-- 
With Best Regards,
Andy Shevchenko