[PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Herve Codina]

Hi,

Note: Resent this series with Saravana added in Cc.

When a gpio used by the leds-gpio device is removed, the leds-gpio
device continues to use this gpio. Also, when the gpio is back, the
leds-gpio still uses the old removed gpio.

A consumer/supplier relationship is missing between the leds-gpio device
(consumer) and the gpio used (supplier).

This series adds an addionnal devlink between this two device.
With this link when the gpio is removed, the leds-gpio device is also
removed.

Best regards,
Hervé Codina

Herve Codina (2):
  gpiolib: Introduce gpiod_device_add_link()
  leds: gpio: Add devlinks between the gpio consumed and the gpio leds
    device

 drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
 drivers/leds/leds-gpio.c      | 15 +++++++++++++++
 include/linux/gpio/consumer.h |  5 +++++
 3 files changed, 52 insertions(+)

-- 
2.43.0

[PATCH RESEND 1/2] gpiolib: Introduce gpiod_device_add_link()

[Herve Codina]

With device-tree, some devlink related to gpios are automatically added
when a consumer device is added and the attached node has phandles
related to GPIOs.
In some cases, the real device used to get the gpio during a probe() can
be related to an of-node parent of the of-node used for the already done
automatically devlink creation.
For instance, a driver can be bound to a device and, during the
probe(), the driver can walk its of-node children to get the GPIO
described in these children nodes.
In that case, an additional devlink between the device attached to the
driver and the gpio consumer need to be created.
Indeed, if the GPIO is removed, the consumer/supplier dependency should
lead to remove first the consuming driver before removing the supplier.

In order to give the possibility to this kind of driver to add additional
devlinks, introduce gpiod_device_add_link().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h |  5 +++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8b3a0f45b574..416ab334b02d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4195,6 +4195,38 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 	return desc;
 }
 
+/**
+ * gpiod_device_add_link - Add a link between a GPIO consumer and a GPIO.
+ * @consumer: GPIO consumer.
+ * @desc: GPIO consumed.
+ * @flags: Link flags, see device_link_add().
+ *
+ * This function can be used for drivers that need to add an additional
+ * consumer/supplier device link to a GPIO.
+ *
+ * Returns:
+ * On successful, the link created.
+ * NULL if the link was not created due to a missing GPIO parent.
+ *
+ * In case of error an ERR_PTR() is returned.
+ */
+struct device_link *gpiod_device_add_link(struct device *consumer,
+					  struct gpio_desc *desc,
+					  u32 flags)
+{
+	struct device_link *link;
+
+	if (!desc->gdev->dev.parent)
+		return NULL;
+
+	link = device_link_add(consumer, desc->gdev->dev.parent, flags);
+	if (!link)
+		return ERR_PTR(-EINVAL);
+
+	return link;
+}
+EXPORT_SYMBOL_GPL(gpiod_device_add_link);
+
 /**
  * fwnode_gpiod_get_index - obtain a GPIO from firmware node
  * @fwnode:	handle of the firmware node
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index db2dfbae8edb..4feed4e166b0 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -7,6 +7,7 @@
 
 struct acpi_device;
 struct device;
+struct device_link;
 struct fwnode_handle;
 
 struct gpio_array;
@@ -106,6 +107,10 @@ void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
 void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc);
 void devm_gpiod_put_array(struct device *dev, struct gpio_descs *descs);
 
+struct device_link *gpiod_device_add_link(struct device *consumer,
+					  struct gpio_desc *desc,
+					  u32 flags);
+
 int gpiod_get_direction(struct gpio_desc *desc);
 int gpiod_direction_input(struct gpio_desc *desc);
 int gpiod_direction_output(struct gpio_desc *desc, int value);
-- 
2.43.0

[PATCH RESEND 2/2] leds: gpio: Add devlinks between the gpio consumed and the gpio leds device

[Herve Codina]

When a gpio is removed, the gpio leds consumer need to be removed first.
This dependency consumer/supplier can be described by devlink links.
In case of device-tree, even if some devlinks are created due to the
presence of gpio phandles in the gpio leds children, these links do not
help in removing the gpio leds device (i.e. the real consumer) before
the consumed gpio.
We can reach cases where the gpio are no more present and the gpio leds
driver continue to have leds using these gpio.
Further more, when the gpio come back, the gpio leds still use the old
removed one.

Indeed, the gpio are consumed by the parent of the consumer used in the
devlink creation due to phandles. A link is missing between the gpio and
the real gpio consumer, the gpio leds device itself.

Use the newly introduced gpiod_device_add_link() to create this
missing link between the gpio leds devices and the gpios.
With that done, if a gpio is removed, the gpio leds is removed and the
resources are correctly released.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/leds/leds-gpio.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 83fcd7b6afff..b3ec8ecbe5da 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -150,6 +150,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
 {
 	struct fwnode_handle *child;
 	struct gpio_leds_priv *priv;
+	struct device_link *link;
 	int count, ret;
 
 	count = device_get_child_node_count(dev);
@@ -197,6 +198,20 @@ static struct gpio_leds_priv *gpio_leds_create(struct device *dev)
 		/* Set gpiod label to match the corresponding LED name. */
 		gpiod_set_consumer_name(led_dat->gpiod,
 					led_dat->cdev.dev->kobj.name);
+
+		/*
+		 * Create a link between the GPIO and the gpio-leds device.
+		 * This allow to have a relationship between the gpio used and
+		 * the gpio-leds device in order to automatically remove the
+		 * gpio-leds device (consumer) when a GPIO (supplier) is removed.
+		 */
+		link = gpiod_device_add_link(dev, led_dat->gpiod,
+					     DL_FLAG_AUTOREMOVE_CONSUMER);
+		if (IS_ERR(link)) {
+			fwnode_handle_put(child);
+			return ERR_CAST(link);
+		}
+
 		priv->num_leds++;
 	}
 
-- 
2.43.0

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Bartosz Golaszewski]

On Tue, Feb 20, 2024 at 2:39 PM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi,
>
> Note: Resent this series with Saravana added in Cc.
>
> When a gpio used by the leds-gpio device is removed, the leds-gpio
> device continues to use this gpio. Also, when the gpio is back, the
> leds-gpio still uses the old removed gpio.
>
> A consumer/supplier relationship is missing between the leds-gpio device
> (consumer) and the gpio used (supplier).
>
> This series adds an addionnal devlink between this two device.
> With this link when the gpio is removed, the leds-gpio device is also
> removed.
>
> Best regards,
> Hervé Codina
>
> Herve Codina (2):
>   gpiolib: Introduce gpiod_device_add_link()
>   leds: gpio: Add devlinks between the gpio consumed and the gpio leds
>     device
>
>  drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
>  drivers/leds/leds-gpio.c      | 15 +++++++++++++++
>  include/linux/gpio/consumer.h |  5 +++++
>  3 files changed, 52 insertions(+)
>
> --
> 2.43.0
>

Can you add some more context here in the form of DT snippets that
lead to this being needed?

Bartosz

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Herve Codina]

On Tue, 20 Feb 2024 15:19:57 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Tue, Feb 20, 2024 at 2:39 PM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi,
> >
> > Note: Resent this series with Saravana added in Cc.
> >
> > When a gpio used by the leds-gpio device is removed, the leds-gpio
> > device continues to use this gpio. Also, when the gpio is back, the
> > leds-gpio still uses the old removed gpio.
> >
> > A consumer/supplier relationship is missing between the leds-gpio device
> > (consumer) and the gpio used (supplier).
> >
> > This series adds an addionnal devlink between this two device.
> > With this link when the gpio is removed, the leds-gpio device is also
> > removed.
> >
> > Best regards,
> > Hervé Codina
> >
> > Herve Codina (2):
> >   gpiolib: Introduce gpiod_device_add_link()
> >   leds: gpio: Add devlinks between the gpio consumed and the gpio leds
> >     device
> >
> >  drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
> >  drivers/leds/leds-gpio.c      | 15 +++++++++++++++
> >  include/linux/gpio/consumer.h |  5 +++++
> >  3 files changed, 52 insertions(+)
> >
> > --
> > 2.43.0
> >  
> 
> Can you add some more context here in the form of DT snippets that
> lead to this being needed?

/ {
	leds-dock {
		compatible = "gpio-leds";

		led-5 {
			label = "dock:alarm:red";
			gpios = <&tca6424_dock_2 12 GPIO_ACTIVE_HIGH>;
		};

		led-6 {
			label = "dock:alarm:yellow";
			gpios = <&tca6424_dock_2 13 GPIO_ACTIVE_HIGH>;
		};

		led-7 {
			label = "dock:alarm:blue";
			gpios = <&tca6424_dock_2 14 GPIO_ACTIVE_HIGH>;
		};
	};

	...
	i2c5 {
		...
		tca6424_dock_2: gpio@23 {
			compatible = "ti,tca6424";
			reg = <0x23>;
			gpio-controller;
			#gpio-cells = <2>;
			interrupt-parent = <&tca6424_dock_1>;
			interrupts = <23 IRQ_TYPE_EDGE_FALLING>;
			interrupt-controller;
			#interrupt-cells = <2>;
			vcc-supply = <&reg_dock_ctrl_3v3>;
		};
		tca6424_dock_1: gpio@22 {
			compatible = "ti,tca6424";
			reg = <0x22>;
			gpio-controller;
			#gpio-cells = <2>;
			interrupt-parent = <&gpio4>;
			interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
			interrupt-controller;
			#interrupt-cells = <2>;
			vcc-supply = <&reg_dock_ctrl_3v3>;
		};
	};
};

Also, had the exact same issue if I use a SoC gpio chip instead of an
i2c gpio expander.

Best regards,
Hervé

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Bartosz Golaszewski]

On Tue, Feb 20, 2024 at 3:53 PM Herve Codina <herve.codina@bootlin.com> wrote:
>
> On Tue, 20 Feb 2024 15:19:57 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > On Tue, Feb 20, 2024 at 2:39 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > Hi,
> > >
> > > Note: Resent this series with Saravana added in Cc.
> > >
> > > When a gpio used by the leds-gpio device is removed, the leds-gpio
> > > device continues to use this gpio. Also, when the gpio is back, the
> > > leds-gpio still uses the old removed gpio.
> > >
> > > A consumer/supplier relationship is missing between the leds-gpio device
> > > (consumer) and the gpio used (supplier).
> > >
> > > This series adds an addionnal devlink between this two device.
> > > With this link when the gpio is removed, the leds-gpio device is also
> > > removed.
> > >
> > > Best regards,
> > > Hervé Codina
> > >
> > > Herve Codina (2):
> > >   gpiolib: Introduce gpiod_device_add_link()
> > >   leds: gpio: Add devlinks between the gpio consumed and the gpio leds
> > >     device
> > >
> > >  drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
> > >  drivers/leds/leds-gpio.c      | 15 +++++++++++++++
> > >  include/linux/gpio/consumer.h |  5 +++++
> > >  3 files changed, 52 insertions(+)
> > >
> > > --
> > > 2.43.0
> > >
> >
> > Can you add some more context here in the form of DT snippets that
> > lead to this being needed?
>
> / {
>         leds-dock {
>                 compatible = "gpio-leds";
>
>                 led-5 {
>                         label = "dock:alarm:red";
>                         gpios = <&tca6424_dock_2 12 GPIO_ACTIVE_HIGH>;
>                 };

Do I understand correctly that the devlink is created between "led-5"
and "tca6424_dock_2" but actually should also be created between
"leds-dock" and "tca6424_dock_2"?

Bartosz

>
>                 led-6 {
>                         label = "dock:alarm:yellow";
>                         gpios = <&tca6424_dock_2 13 GPIO_ACTIVE_HIGH>;
>                 };
>
>                 led-7 {
>                         label = "dock:alarm:blue";
>                         gpios = <&tca6424_dock_2 14 GPIO_ACTIVE_HIGH>;
>                 };
>         };
>
>         ...
>         i2c5 {
>                 ...
>                 tca6424_dock_2: gpio@23 {
>                         compatible = "ti,tca6424";
>                         reg = <0x23>;
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-parent = <&tca6424_dock_1>;
>                         interrupts = <23 IRQ_TYPE_EDGE_FALLING>;
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>                         vcc-supply = <&reg_dock_ctrl_3v3>;
>                 };
>                 tca6424_dock_1: gpio@22 {
>                         compatible = "ti,tca6424";
>                         reg = <0x22>;
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-parent = <&gpio4>;
>                         interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>                         vcc-supply = <&reg_dock_ctrl_3v3>;
>                 };
>         };
> };
>
> Also, had the exact same issue if I use a SoC gpio chip instead of an
> i2c gpio expander.
>
> Best regards,
> Hervé

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Herve Codina]

Hi Bartosz,

On Tue, 20 Feb 2024 16:30:11 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Tue, Feb 20, 2024 at 3:53 PM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > On Tue, 20 Feb 2024 15:19:57 +0100
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >  
> > > On Tue, Feb 20, 2024 at 2:39 PM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > >
> > > > Hi,
> > > >
> > > > Note: Resent this series with Saravana added in Cc.
> > > >
> > > > When a gpio used by the leds-gpio device is removed, the leds-gpio
> > > > device continues to use this gpio. Also, when the gpio is back, the
> > > > leds-gpio still uses the old removed gpio.
> > > >
> > > > A consumer/supplier relationship is missing between the leds-gpio device
> > > > (consumer) and the gpio used (supplier).
> > > >
> > > > This series adds an addionnal devlink between this two device.
> > > > With this link when the gpio is removed, the leds-gpio device is also
> > > > removed.
> > > >
> > > > Best regards,
> > > > Hervé Codina
> > > >
> > > > Herve Codina (2):
> > > >   gpiolib: Introduce gpiod_device_add_link()
> > > >   leds: gpio: Add devlinks between the gpio consumed and the gpio leds
> > > >     device
> > > >
> > > >  drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
> > > >  drivers/leds/leds-gpio.c      | 15 +++++++++++++++
> > > >  include/linux/gpio/consumer.h |  5 +++++
> > > >  3 files changed, 52 insertions(+)
> > > >
> > > > --
> > > > 2.43.0
> > > >  
> > >
> > > Can you add some more context here in the form of DT snippets that
> > > lead to this being needed?  
> >
> > / {
> >         leds-dock {
> >                 compatible = "gpio-leds";
> >
> >                 led-5 {
> >                         label = "dock:alarm:red";
> >                         gpios = <&tca6424_dock_2 12 GPIO_ACTIVE_HIGH>;
> >                 };  
> 
> Do I understand correctly that the devlink is created between "led-5"
> and "tca6424_dock_2" but actually should also be created between
> "leds-dock" and "tca6424_dock_2"?
> 

Yes, that's my understanding too.

Best regards,
Hervé

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Saravana Kannan]

On Tue, Feb 20, 2024 at 7:47 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Bartosz,
>
> On Tue, 20 Feb 2024 16:30:11 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > On Tue, Feb 20, 2024 at 3:53 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > On Tue, 20 Feb 2024 15:19:57 +0100
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > On Tue, Feb 20, 2024 at 2:39 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Note: Resent this series with Saravana added in Cc.
> > > > >
> > > > > When a gpio used by the leds-gpio device is removed, the leds-gpio
> > > > > device continues to use this gpio. Also, when the gpio is back, the
> > > > > leds-gpio still uses the old removed gpio.
> > > > >
> > > > > A consumer/supplier relationship is missing between the leds-gpio device
> > > > > (consumer) and the gpio used (supplier).
> > > > >
> > > > > This series adds an addionnal devlink between this two device.
> > > > > With this link when the gpio is removed, the leds-gpio device is also
> > > > > removed.
> > > > >
> > > > > Best regards,
> > > > > Hervé Codina
> > > > >
> > > > > Herve Codina (2):
> > > > >   gpiolib: Introduce gpiod_device_add_link()
> > > > >   leds: gpio: Add devlinks between the gpio consumed and the gpio leds
> > > > >     device
> > > > >
> > > > >  drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
> > > > >  drivers/leds/leds-gpio.c      | 15 +++++++++++++++
> > > > >  include/linux/gpio/consumer.h |  5 +++++
> > > > >  3 files changed, 52 insertions(+)
> > > > >
> > > > > --
> > > > > 2.43.0
> > > > >
> > > >
> > > > Can you add some more context here in the form of DT snippets that
> > > > lead to this being needed?
> > >
> > > / {
> > >         leds-dock {
> > >                 compatible = "gpio-leds";
> > >
> > >                 led-5 {
> > >                         label = "dock:alarm:red";
> > >                         gpios = <&tca6424_dock_2 12 GPIO_ACTIVE_HIGH>;
> > >                 };
> >
> > Do I understand correctly that the devlink is created between "led-5"
> > and "tca6424_dock_2" but actually should also be created between
> > "leds-dock" and "tca6424_dock_2"?
> >
>
> Yes, that's my understanding too.

I'm replying here instead of the RESEND because here's where the
context and example are provided.

I quickly poked into the gpio-leds driver. Please correct me if I'm
misunderstanding anything.

It looks like led-5 will be added as a class device. But the
dev->fwnode is not set before it's added because it uses
device_create_with_groups(). So, fw_devlink doesn't create a link
between led-5 and tca6424_dock_2 unless tca6424_dock_2 is added after
led-5. Which coincidentally seems to be the case here. Might want to
explicitly create the device in gpio-leds driver.

The issue you are trying to fix is a generic issue that I'd like to
fix in a generic fashion. It's one of my TODOs which I've mentioned
before in conferences/emails to LKML: device links framework has a
bunch of gaps when it comes to class devices. I've been thinking about
it for a while, but it needs a lot more work and testing. I'll roll in
this case when I deal with it in a generic fashion. But here's the
general idea of things that need to be addressed:

1. "Managed" device links allow having a class device as a supplier,
but that'll mean the consumer will never probe.
2. What if a class device is a consumer and the supplier isn't ready.
What does it mean for the class device to be added? Is it available
for use? Probably not. Can we do something here that'll be useful for
the class implementation?
3. What if the supplier and consumer are class devices, when does the
consumer class device become "available" (do we check the suppliers of
the supplier?)?
4. What happens if the supplier of a class device gets removed? Do we
notify the class so it can do the right thing? Do we force unbind the
first ancestor that's on a bus? (your case).
5. What if a supplier class device is removed, should we unbind the
consumer (if it's a bus device)?

I'm currently working on a patch to break dependency cycles. Once
that's in, the next TODO item I work on is going to be this or clock
framework sync_state() support.

So, I'd recommend waiting this out if it's not urgent.

Heh, here's my commit on my local repo from a year ago when I touched
on this and realised the scope of the work.

commit 7dcaad52e569209104408f3e472fde4ef8cd5585 (class-devlinks-v1)
Author: Saravana Kannan <saravanak@google.com>
Date:   Mon Feb 13 13:40:43 2023 -0800

    add class support to device links


Thanks,
Saravana

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Herve Codina]

Hi Saravana,

On Tue, 20 Feb 2024 19:28:17 -0800
Saravana Kannan <saravanak@google.com> wrote:

> On Tue, Feb 20, 2024 at 7:47 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Bartosz,
> >
> > On Tue, 20 Feb 2024 16:30:11 +0100
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >  
> > > On Tue, Feb 20, 2024 at 3:53 PM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > >
> > > > On Tue, 20 Feb 2024 15:19:57 +0100
> > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >  
> > > > > On Tue, Feb 20, 2024 at 2:39 PM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Note: Resent this series with Saravana added in Cc.
> > > > > >
> > > > > > When a gpio used by the leds-gpio device is removed, the leds-gpio
> > > > > > device continues to use this gpio. Also, when the gpio is back, the
> > > > > > leds-gpio still uses the old removed gpio.
> > > > > >
> > > > > > A consumer/supplier relationship is missing between the leds-gpio device
> > > > > > (consumer) and the gpio used (supplier).
> > > > > >
> > > > > > This series adds an addionnal devlink between this two device.
> > > > > > With this link when the gpio is removed, the leds-gpio device is also
> > > > > > removed.
> > > > > >
> > > > > > Best regards,
> > > > > > Hervé Codina
> > > > > >
> > > > > > Herve Codina (2):
> > > > > >   gpiolib: Introduce gpiod_device_add_link()
> > > > > >   leds: gpio: Add devlinks between the gpio consumed and the gpio leds
> > > > > >     device
> > > > > >
> > > > > >  drivers/gpio/gpiolib.c        | 32 ++++++++++++++++++++++++++++++++
> > > > > >  drivers/leds/leds-gpio.c      | 15 +++++++++++++++
> > > > > >  include/linux/gpio/consumer.h |  5 +++++
> > > > > >  3 files changed, 52 insertions(+)
> > > > > >
> > > > > > --
> > > > > > 2.43.0
> > > > > >  
> > > > >
> > > > > Can you add some more context here in the form of DT snippets that
> > > > > lead to this being needed?  
> > > >
> > > > / {
> > > >         leds-dock {
> > > >                 compatible = "gpio-leds";
> > > >
> > > >                 led-5 {
> > > >                         label = "dock:alarm:red";
> > > >                         gpios = <&tca6424_dock_2 12 GPIO_ACTIVE_HIGH>;
> > > >                 };  
> > >
> > > Do I understand correctly that the devlink is created between "led-5"
> > > and "tca6424_dock_2" but actually should also be created between
> > > "leds-dock" and "tca6424_dock_2"?
> > >  
> >
> > Yes, that's my understanding too.  
> 
> I'm replying here instead of the RESEND because here's where the
> context and example are provided.
> 
> I quickly poked into the gpio-leds driver. Please correct me if I'm
> misunderstanding anything.
> 
> It looks like led-5 will be added as a class device. But the
> dev->fwnode is not set before it's added because it uses
> device_create_with_groups(). So, fw_devlink doesn't create a link
> between led-5 and tca6424_dock_2 unless tca6424_dock_2 is added after
> led-5. Which coincidentally seems to be the case here. Might want to
> explicitly create the device in gpio-leds driver.
> 
> The issue you are trying to fix is a generic issue that I'd like to
> fix in a generic fashion. It's one of my TODOs which I've mentioned
> before in conferences/emails to LKML: device links framework has a
> bunch of gaps when it comes to class devices. I've been thinking about
> it for a while, but it needs a lot more work and testing. I'll roll in
> this case when I deal with it in a generic fashion. But here's the
> general idea of things that need to be addressed:
> 
> 1. "Managed" device links allow having a class device as a supplier,
> but that'll mean the consumer will never probe.
> 2. What if a class device is a consumer and the supplier isn't ready.
> What does it mean for the class device to be added? Is it available
> for use? Probably not. Can we do something here that'll be useful for
> the class implementation?
> 3. What if the supplier and consumer are class devices, when does the
> consumer class device become "available" (do we check the suppliers of
> the supplier?)?
> 4. What happens if the supplier of a class device gets removed? Do we
> notify the class so it can do the right thing? Do we force unbind the
> first ancestor that's on a bus? (your case).
> 5. What if a supplier class device is removed, should we unbind the
> consumer (if it's a bus device)?
> 
> I'm currently working on a patch to break dependency cycles. Once
> that's in, the next TODO item I work on is going to be this or clock
> framework sync_state() support.
> 
> So, I'd recommend waiting this out if it's not urgent.
> 
> Heh, here's my commit on my local repo from a year ago when I touched
> on this and realised the scope of the work.
> 
> commit 7dcaad52e569209104408f3e472fde4ef8cd5585 (class-devlinks-v1)
> Author: Saravana Kannan <saravanak@google.com>
> Date:   Mon Feb 13 13:40:43 2023 -0800
> 
>     add class support to device links
> 

Well, on one side we have this current patch that fixes the current issue in
the gpio-led use case.

On the other side, I have a commit hash from your local repo which is one
year old and some more work is needed on it.

Maybe my patch is not fully correct but it fixes the issue.

Do you have any idea about when your work on that topic will be available?

Best regards,
Hervé

Re: [PATCH RESEND 0/2] leds: gpio: Add devlink between the leds-gpio device and the gpio used.

[Herve Codina]

Hi Saravana,

On Tue, 20 Feb 2024 19:28:17 -0800
Saravana Kannan <saravanak@google.com> wrote:

...
> > >
> > > Do I understand correctly that the devlink is created between "led-5"
> > > and "tca6424_dock_2" but actually should also be created between
> > > "leds-dock" and "tca6424_dock_2"?
> > >  
> >
> > Yes, that's my understanding too.  
> 
> I'm replying here instead of the RESEND because here's where the
> context and example are provided.
> 
> I quickly poked into the gpio-leds driver. Please correct me if I'm
> misunderstanding anything.
> 
> It looks like led-5 will be added as a class device. But the
> dev->fwnode is not set before it's added because it uses
> device_create_with_groups(). So, fw_devlink doesn't create a link
> between led-5 and tca6424_dock_2 unless tca6424_dock_2 is added after
> led-5. Which coincidentally seems to be the case here. Might want to
> explicitly create the device in gpio-leds driver.
> 
> The issue you are trying to fix is a generic issue that I'd like to
> fix in a generic fashion. It's one of my TODOs which I've mentioned
> before in conferences/emails to LKML: device links framework has a
> bunch of gaps when it comes to class devices. I've been thinking about
> it for a while, but it needs a lot more work and testing. I'll roll in
> this case when I deal with it in a generic fashion. But here's the
> general idea of things that need to be addressed:
> 
> 1. "Managed" device links allow having a class device as a supplier,
> but that'll mean the consumer will never probe.
> 2. What if a class device is a consumer and the supplier isn't ready.
> What does it mean for the class device to be added? Is it available
> for use? Probably not. Can we do something here that'll be useful for
> the class implementation?
> 3. What if the supplier and consumer are class devices, when does the
> consumer class device become "available" (do we check the suppliers of
> the supplier?)?
> 4. What happens if the supplier of a class device gets removed? Do we
> notify the class so it can do the right thing? Do we force unbind the
> first ancestor that's on a bus? (your case).
> 5. What if a supplier class device is removed, should we unbind the
> consumer (if it's a bus device)?
> 
> I'm currently working on a patch to break dependency cycles. Once
> that's in, the next TODO item I work on is going to be this or clock
> framework sync_state() support.
> 
> So, I'd recommend waiting this out if it's not urgent.
> 
> Heh, here's my commit on my local repo from a year ago when I touched
> on this and realised the scope of the work.
> 
> commit 7dcaad52e569209104408f3e472fde4ef8cd5585 (class-devlinks-v1)
> Author: Saravana Kannan <saravanak@google.com>
> Date:   Mon Feb 13 13:40:43 2023 -0800
> 
>     add class support to device links
> 
> 
> Thanks,
> Saravana

Did you move forward on this topic ?

Best regards,
Hervé