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.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

---
Changes since v7:
- Dropped mention of time unit in struct i2c_of_probe_simple_opts
  kernel doc (Andy)
- Added check for non-zero delay before msleep() (Doug)
- Simplified GPIO name check and reverse conditional branches (Andy)
- Added description about the supported power sequence
- Switched GPIO usage to logical levels (Doug)
  - Changed some variable names and comments to fit
- Added description of power sequence to struct i2c_of_probe_simple_opts
  (Doug)
- Added comment saying i2c_of_probe_simple_put_gpiod() might be no-op
  (Doug)
- Combined callbacks (.get_resources with .enable; .cleanup with
  .free_resources_late); renamed i2c_of_probe_simple_free_res_early() to
  i2c_of_probe_simple_cleanup_early()

Changes since v6:
- Restructured into helpers for the I2C OF component prober
- Reduced to only handle one GPIO
- Set GPIO to input on (failure) cleanup
- Updated commit message

Changes since v5:
- Renamed "con" to "propname" in i2c_of_probe_get_gpiod()
- Copy string first and check return value of strscpy() for overflow in
  i2c_of_probe_get_gpiod()
- Add parenthesis around "enable" and "reset" GPIO names in comments
- Split resource count debug message into two separate lines
- Split out GPIO helper from i2c_of_probe_enable_res() to keep code
  cleaner following the previous patch
- Adopted options for customizing power sequencing delay following
  previous patch

Changes since v4:
- Split out from previous patch
- Moved GPIO property name check to common function in gpiolib.c in new
  patch
- Moved i2c_of_probe_free_gpios() into for_each_child_of_node_scoped()
- Rewrote in gpiod_*_array-esque fashion
---
 drivers/i2c/i2c-core-of-prober.c | 104 ++++++++++++++++++++++++++++++-
 include/linux/i2c-of-prober.h    |  20 ++++++
 2 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index d7f51ff872b8..b84e88624b63 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/dev_printk.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/i2c-of-prober.h>
 #include <linux/module.h>
@@ -30,7 +31,6 @@
  * address responds.
  *
  * TODO:
- * - Support handling common GPIOs.
  * - Support I2C muxes
  */
 
@@ -239,6 +239,66 @@ static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c
 	regulator_disable(ctx->supply);
 }
 
+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;
+
+	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_put_gpiod(struct i2c_of_probe_simple_ctx *ctx)
+{
+	gpiod_put(ctx->gpiod);
+	ctx->gpiod = NULL;
+}
+
+static int i2c_of_probe_simple_set_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
+{
+	int ret;
+
+	if (!ctx->gpiod)
+		return 0;
+
+	dev_dbg(dev, "Configuring GPIO\n");
+
+	ret = gpiod_direction_output(ctx->gpiod, ctx->opts->gpio_assert_to_enable);
+	if (ret)
+		return ret;
+
+	if (ctx->opts->post_gpio_config_delay_ms)
+		msleep(ctx->opts->post_gpio_config_delay_ms);
+
+	return 0;
+}
+
+static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
+{
+	if (!ctx->gpiod)
+		return;
+
+	/* Ignore error if GPIO is not in output direction */
+	gpiod_set_value(ctx->gpiod, !ctx->opts->gpio_assert_to_enable);
+}
+
 /**
  * i2c_of_probe_simple_enable - Simple helper for I2C OF prober to get and enable resources
  * @dev: Pointer to the &struct device of the caller, only used for dev_printk() messages
@@ -246,7 +306,11 @@ static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c
  * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context.
  *
  * If &i2c_of_probe_simple_opts->supply_name is given, request the named regulator supply.
+ * If &i2c_of_probe_simple_opts->gpio_name is given, request the named GPIO. Or if it is
+ * the empty string, request the unnamed GPIO.
  * If a regulator supply was found, enable that regulator.
+ * If a GPIO line was found, configure the GPIO line to output and set value
+ * according to given options.
  *
  * Return: %0 on success or no-op, or a negative error number on failure.
  */
@@ -275,12 +339,24 @@ int i2c_of_probe_simple_enable(struct device *dev, struct device_node *bus_node,
 	if (ret)
 		goto out_put_node;
 
-	ret = i2c_of_probe_simple_enable_regulator(dev, ctx);
+	ret = i2c_of_probe_simple_get_gpiod(dev, node, ctx);
 	if (ret)
 		goto out_put_supply;
 
+	ret = i2c_of_probe_simple_enable_regulator(dev, ctx);
+	if (ret)
+		goto out_put_gpiod;
+
+	ret = i2c_of_probe_simple_set_gpio(dev, ctx);
+	if (ret)
+		goto out_disable_regulator;
+
 	return 0;
 
+out_disable_regulator:
+	i2c_of_probe_simple_disable_regulator(dev, ctx);
+out_put_gpiod:
+	i2c_of_probe_simple_put_gpiod(ctx);
 out_put_supply:
 	i2c_of_probe_simple_put_supply(ctx);
 out_put_node:
@@ -289,17 +365,40 @@ int i2c_of_probe_simple_enable(struct device *dev, struct device_node *bus_node,
 }
 EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_enable, I2C_OF_PROBER);
 
+/**
+ * i2c_of_probe_simple_cleanup_early - \
+ *	Simple helper for I2C OF prober to release GPIOs before component is enabled
+ * @dev: Pointer to the &struct device of the caller; unused.
+ * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context.
+ *
+ * GPIO descriptors are exclusive and have to be released before the
+ * actual driver probes so that the latter can acquire them.
+ */
+void i2c_of_probe_simple_cleanup_early(struct device *dev, void *data)
+{
+	struct i2c_of_probe_simple_ctx *ctx = data;
+
+	i2c_of_probe_simple_put_gpiod(ctx);
+}
+EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_cleanup_early, I2C_OF_PROBER);
+
 /**
  * i2c_of_probe_simple_cleanup - Clean up and release resources for I2C OF prober simple helpers
  * @dev: Pointer to the &struct device of the caller, only used for dev_printk() messages
  * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context.
  *
+ * * If a GPIO line was found and not yet released, set its value to the opposite of that
+ *   set in i2c_of_probe_simple_enable() and release it.
  * * If a regulator supply was found, disable that regulator and release it.
  */
 void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
 {
 	struct i2c_of_probe_simple_ctx *ctx = data;
 
+	/* GPIO operations here are no-ops if a component was found and enabled. */
+	i2c_of_probe_simple_disable_gpio(dev, ctx);
+	i2c_of_probe_simple_put_gpiod(ctx);
+
 	i2c_of_probe_simple_disable_regulator(dev, ctx);
 	i2c_of_probe_simple_put_supply(ctx);
 }
@@ -307,6 +406,7 @@ EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_cleanup, I2C_OF_PROBER);
 
 struct i2c_of_probe_ops i2c_of_probe_simple_ops = {
 	.enable = i2c_of_probe_simple_enable,
+	.cleanup_early = i2c_of_probe_simple_cleanup_early,
 	.cleanup = i2c_of_probe_simple_cleanup,
 };
 EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_ops, I2C_OF_PROBER);
diff --git a/include/linux/i2c-of-prober.h b/include/linux/i2c-of-prober.h
index c4938a34b901..513ab37b72a5 100644
--- a/include/linux/i2c-of-prober.h
+++ b/include/linux/i2c-of-prober.h
@@ -80,6 +80,7 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf
  *
  * The following helpers are provided:
  * * i2c_of_probe_simple_enable()
+ * * i2c_of_probe_simple_cleanup_early()
  * * i2c_of_probe_simple_cleanup()
  */
 
@@ -87,24 +88,43 @@ int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cf
  * struct i2c_of_probe_simple_opts - Options for simple I2C component prober callbacks
  * @res_node_compatible: Compatible string of device node to retrieve resources from.
  * @supply_name: Name of regulator supply.
+ * @gpio_name: Name of GPIO. NULL if no GPIO line is used. Empty string ("") if GPIO
+ *	       line is unnamed.
  * @post_power_on_delay_ms: Delay after regulators are powered on. Passed to msleep().
+ * @post_gpio_config_delay_ms: Delay after GPIO is configured. Passed to msleep().
+ * @gpio_assert_to_enable: %true if GPIO should be asserted, i.e. set to logical high,
+ *			   to enable the component.
+ *
+ * This describes power sequences common for the class of components supported by the
+ * simple component prober:
+ * * @gpio_name is configured to the non-active setting according to @gpio_assert_to_enable.
+ * * @supply_name regulator supply is enabled.
+ * * Wait for @post_power_on_delay_ms to pass.
+ * * @gpio_name is configured to the active setting according to @gpio_assert_to_enable.
+ * * Wait for @post_gpio_config_delay_ms to pass.
  */
 struct i2c_of_probe_simple_opts {
 	const char *res_node_compatible;
 	const char *supply_name;
+	const char *gpio_name;
 	unsigned int post_power_on_delay_ms;
+	unsigned int post_gpio_config_delay_ms;
+	bool gpio_assert_to_enable;
 };
 
 struct regulator;
+struct gpio_desc;
 
 struct i2c_of_probe_simple_ctx {
 	/* public: provided by user before helpers are used. */
 	const struct i2c_of_probe_simple_opts *opts;
 	/* private: internal fields for helpers. */
 	struct regulator *supply;
+	struct gpio_desc *gpiod;
 };
 
 int i2c_of_probe_simple_enable(struct device *dev, struct device_node *bus_node, void *data);
+void i2c_of_probe_simple_cleanup_early(struct device *dev, void *data);
 void i2c_of_probe_simple_cleanup(struct device *dev, void *data);
 
 extern struct i2c_of_probe_ops i2c_of_probe_simple_ops;
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Hi,

On Tue, Oct 8, 2024 at 12:35 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> +{
> +       if (!ctx->gpiod)
> +               return;
> +
> +       /* Ignore error if GPIO is not in output direction */
> +       gpiod_set_value(ctx->gpiod, !ctx->opts->gpio_assert_to_enable);

I'm not sure I understand the comment. Does disable() ever get called
when set() wasn't called beforehand? How could it not be in output
direction?


>  void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
>  {
>         struct i2c_of_probe_simple_ctx *ctx = data;
>
> +       /* GPIO operations here are no-ops if a component was found and enabled. */

Instead of the above, maybe:

GPIO operations here are no-ops if i2c_of_probe_simple_cleanup_early()
was called.


Just commenting nits, so:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

On Wed, Oct 16, 2024 at 2:00 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Oct 8, 2024 at 12:35 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> > +{
> > +       if (!ctx->gpiod)
> > +               return;
> > +
> > +       /* Ignore error if GPIO is not in output direction */
> > +       gpiod_set_value(ctx->gpiod, !ctx->opts->gpio_assert_to_enable);
>
> I'm not sure I understand the comment. Does disable() ever get called
> when set() wasn't called beforehand? How could it not be in output
> direction?

You're right. I think it was carried over (in my mind) from when it was
still four callbacks.

> >  void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
> >  {
> >         struct i2c_of_probe_simple_ctx *ctx = data;
> >
> > +       /* GPIO operations here are no-ops if a component was found and enabled. */
>
> Instead of the above, maybe:
>
> GPIO operations here are no-ops if i2c_of_probe_simple_cleanup_early()
> was called.

Makes sense. Will change.

> Just commenting nits, so:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

I assume this stands even after Andy's suggestion to drop the gpiod check
is applied.

Thanks
ChenYu

Hi,

On Wed, Oct 16, 2024 at 12:49 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> > Just commenting nits, so:
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> I assume this stands even after Andy's suggestion to drop the gpiod check
> is applied.

Yeah, sounds fine.

-Doug

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;

> +	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?

> +	/* 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?

-- 
With Best Regards,
Andy Shevchenko

On Thu, Oct 10, 2024 at 11:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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

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@linux.intel.com> 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

On Mon, Oct 14, 2024 at 7:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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@linux.intel.com> 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.

The opposite actually. Either it is always NULL, or it was initially valid,
but the early cleanup function released it and thus it became NULL by the
time this function gets called.

ChenYu

> > > > +     /* 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
>
>

On Tue, Oct 15, 2024 at 01:34:52PM +0800, Chen-Yu Tsai wrote:
> On Mon, Oct 14, 2024 at 7:20 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 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@linux.intel.com> wrote:
> > > > On Tue, Oct 08, 2024 at 03:34:25PM +0800, Chen-Yu Tsai wrote:

...

> > > > > +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.
> 
> The opposite actually. Either it is always NULL, or it was initially valid,
> but the early cleanup function released it and thus it became NULL by the
> time this function gets called.

Then I don't see any points to have this check (details in the other reply).

> > > > > +     /* 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

On Mon, Oct 14, 2024 at 7:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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@linux.intel.com> 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.

Thank you.

> ...
>
> > > > +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.

No there isn't. Explanation below.

> > 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.

There are two codes paths, either

    a) successfully finding a device and enabling it, or
    b) exhausting all options and not finding a device, because it was
       optional or it is malfunctioning.

After either code path, this cleanup function is called.

In path (a), the GPIO descriptor is released prior to enabling the device,
because the descriptor is an exclusive resource, and as soon as the device
is enabled, its corresponding driver may probe and request the same GPIO,
and would fail if it was not released.

In path (b), nothing was enabled, and the GPIO descriptor was not released
early.

The cleanup function here accounts for both cases, hence the check.

A step-by-step description might be clearer:

1. i2c_of_probe_simple_enable()
   ...
   1a. i2c_of_probe_simple_get_supply()
   1b. i2c_of_probe_simple_get_gpiod()
   1c. i2c_of_probe_simple_enable_regulator()
   1d. i2c_of_probe_simple_set_gpio()

2. Loop through potential component options and probe; if one is found:
   2a. i2c_of_probe_simple_cleanup_early()
       2a-i. i2c_of_probe_simple_put_gpiod
   2b. Enable device and driver's probe() gets called

3. i2c_of_probe_simple_cleanup()
   3a. i2c_of_probe_simple_disable_gpio()
   3b. i2c_of_probe_simple_put_gpiod()
   3c. i2c_of_probe_simple_disable_regulator()
   3d. i2c_of_probe_simple_put_supply()


ChenYu

> > > > +     /* 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
>
>

On Tue, Oct 15, 2024 at 01:31:40PM +0800, Chen-Yu Tsai wrote:
> On Mon, Oct 14, 2024 at 7:20 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 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@linux.intel.com> wrote:
> > > > On Tue, Oct 08, 2024 at 03:34:25PM +0800, Chen-Yu Tsai wrote:

...

> > > > > +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.
> 
> No there isn't. Explanation below.
> 
> > > 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.
> 
> There are two codes paths, either
> 
>     a) successfully finding a device and enabling it, or
>     b) exhausting all options and not finding a device, because it was
>        optional or it is malfunctioning.
> 
> After either code path, this cleanup function is called.
> 
> In path (a), the GPIO descriptor is released prior to enabling the device,
> because the descriptor is an exclusive resource, and as soon as the device
> is enabled, its corresponding driver may probe and request the same GPIO,
> and would fail if it was not released.
> 
> In path (b), nothing was enabled, and the GPIO descriptor was not released
> early.
> 
> The cleanup function here accounts for both cases, hence the check.

Yes, but the very same check is inside gpiod_set_value(). I'm still puzzled
about the duplication. Maybe I'm missing something...

> A step-by-step description might be clearer:
> 
> 1. i2c_of_probe_simple_enable()
>    ...
>    1a. i2c_of_probe_simple_get_supply()
>    1b. i2c_of_probe_simple_get_gpiod()
>    1c. i2c_of_probe_simple_enable_regulator()
>    1d. i2c_of_probe_simple_set_gpio()
> 
> 2. Loop through potential component options and probe; if one is found:
>    2a. i2c_of_probe_simple_cleanup_early()
>        2a-i. i2c_of_probe_simple_put_gpiod
>    2b. Enable device and driver's probe() gets called
> 
> 3. i2c_of_probe_simple_cleanup()
>    3a. i2c_of_probe_simple_disable_gpio()
>    3b. i2c_of_probe_simple_put_gpiod()
>    3c. i2c_of_probe_simple_disable_regulator()
>    3d. i2c_of_probe_simple_put_supply()
> 
> > > > > +     /* 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

On Tue, Oct 15, 2024 at 7:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Oct 15, 2024 at 01:31:40PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Oct 14, 2024 at 7:20 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > 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@linux.intel.com> wrote:
> > > > > On Tue, Oct 08, 2024 at 03:34:25PM +0800, Chen-Yu Tsai wrote:
>
> ...
>
> > > > > > +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.
> >
> > No there isn't. Explanation below.
> >
> > > > 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.
> >
> > There are two codes paths, either
> >
> >     a) successfully finding a device and enabling it, or
> >     b) exhausting all options and not finding a device, because it was
> >        optional or it is malfunctioning.
> >
> > After either code path, this cleanup function is called.
> >
> > In path (a), the GPIO descriptor is released prior to enabling the device,
> > because the descriptor is an exclusive resource, and as soon as the device
> > is enabled, its corresponding driver may probe and request the same GPIO,
> > and would fail if it was not released.
> >
> > In path (b), nothing was enabled, and the GPIO descriptor was not released
> > early.
> >
> > The cleanup function here accounts for both cases, hence the check.
>
> Yes, but the very same check is inside gpiod_set_value(). I'm still puzzled
> about the duplication. Maybe I'm missing something...

My bad. I did not check if the GPIO descriptor API had these checks.
In that case I will drop the check in this patch.


Thanks
ChenYu

> > A step-by-step description might be clearer:
> >
> > 1. i2c_of_probe_simple_enable()
> >    ...
> >    1a. i2c_of_probe_simple_get_supply()
> >    1b. i2c_of_probe_simple_get_gpiod()
> >    1c. i2c_of_probe_simple_enable_regulator()
> >    1d. i2c_of_probe_simple_set_gpio()
> >
> > 2. Loop through potential component options and probe; if one is found:
> >    2a. i2c_of_probe_simple_cleanup_early()
> >        2a-i. i2c_of_probe_simple_put_gpiod
> >    2b. Enable device and driver's probe() gets called
> >
> > 3. i2c_of_probe_simple_cleanup()
> >    3a. i2c_of_probe_simple_disable_gpio()
> >    3b. i2c_of_probe_simple_put_gpiod()
> >    3c. i2c_of_probe_simple_disable_regulator()
> >    3d. i2c_of_probe_simple_put_supply()
> >
> > > > > > +     /* 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
>
>