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.
>
> ...
>
> > +static int i2c_of_probe_simple_get_gpiod(struct device *dev, struct device_node *node,
> > + struct i2c_of_probe_simple_ctx *ctx)
> > +{
> > + struct fwnode_handle *fwnode = of_fwnode_handle(node);
> > + struct gpio_desc *gpiod;
> > + const char *con_id;
> > +
> > + /* NULL signals no GPIO needed */
> > + if (!ctx->opts->gpio_name)
> > + return 0;
> > +
> > + /* 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.
> > + gpiod = fwnode_gpiod_get_index(fwnode, con_id, 0, GPIOD_ASIS, "i2c-of-prober");
> > + if (IS_ERR(gpiod))
> > + return PTR_ERR(gpiod);
> > +
> > + ctx->gpiod = gpiod;
> > +
> > + return 0;
> > +}
>
> ...
>
> > +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.
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?
> > + /* Ignore error if GPIO is not in output direction */
> > + gpiod_set_value(ctx->gpiod, !ctx->opts->gpio_assert_to_enable);
> > +}
>
> ...
>
> > struct regulator;
> > +struct gpio_desc;
>
> Ordered?
Will fix.
Thanks
ChenYu
