| Message ID | 20240220133950.138452-1-herve.codina@bootlin.com |
|---|---|
| Headers | show |
| Series | leds: gpio: Add devlink between the leds-gpio device and the gpio used. | expand |
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é
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é
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(+)