| Message ID | 20191003082812.28491-3-jjhiblot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a generic driver for LED-based backlight | expand |
Hi, On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: > From: Tomi Valkeinen <tomi.valkeinen@ti.com> > > This patch adds basic support for a kernel driver to get a LED device. > This will be used by the led-backlight driver. > > Only OF version is implemented for now, and the behavior is similar to > PWM's of_pwm_get() and pwm_put(). > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > Acked-by: Pavel Machek <pavel@ucw.cz> > --- > drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++ > include/linux/leds.h | 4 ++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index c2167b66b61f..455545f5d663 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -19,6 +19,7 @@ > #include <linux/spinlock.h> > #include <linux/timer.h> > #include <uapi/linux/uleds.h> > +#include <linux/of.h> > #include "leds.h" > > static struct class *leds_class; > @@ -214,6 +215,49 @@ static int led_resume(struct device *dev) > > static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume); > > +/** > + * of_led_get() - request a LED device via the LED framework > + * @np: device node to get the LED device from > + * @index: the index of the LED > + * > + * Returns the LED device parsed from the phandle specified in the "leds" > + * property of a device tree node or a negative error-code on failure. > + */ > +struct led_classdev *of_led_get(struct device_node *np, int index) > +{ > + struct device *led_dev; > + struct led_classdev *led_cdev; > + struct device_node *led_node; > + > + led_node = of_parse_phandle(np, "leds", index); > + if (!led_node) > + return ERR_PTR(-ENOENT); > + > + led_dev = class_find_device_by_of_node(leds_class, led_node); If you convert led_node into a fwnode, you can use class_find_device_by_fwnode() instead. That way the first patch can just be dropped. -- Sebastian > + of_node_put(led_node); > + > + if (!led_dev) > + return ERR_PTR(-EPROBE_DEFER); > + > + led_cdev = dev_get_drvdata(led_dev); > + > + if (!try_module_get(led_cdev->dev->parent->driver->owner)) > + return ERR_PTR(-ENODEV); > + > + return led_cdev; > +} > +EXPORT_SYMBOL_GPL(of_led_get); > + > +/** > + * led_put() - release a LED device > + * @led_cdev: LED device > + */ > +void led_put(struct led_classdev *led_cdev) > +{ > + module_put(led_cdev->dev->parent->driver->owner); > +} > +EXPORT_SYMBOL_GPL(led_put); > + > static int led_classdev_next_name(const char *init_name, char *name, > size_t len) > { > diff --git a/include/linux/leds.h b/include/linux/leds.h > index b8df71193329..6f7371bc7757 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -20,6 +20,7 @@ > > struct device; > struct led_pattern; > +struct device_node; > /* > * LED Core > */ > @@ -196,6 +197,9 @@ extern void devm_led_classdev_unregister(struct device *parent, > extern void led_classdev_suspend(struct led_classdev *led_cdev); > extern void led_classdev_resume(struct led_classdev *led_cdev); > > +extern struct led_classdev *of_led_get(struct device_node *np, int index); > +extern void led_put(struct led_classdev *led_cdev); > + > /** > * led_blink_set - set blinking with software fallback > * @led_cdev: the LED to start blinking > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sebastian, On 03/10/2019 12:42, Sebastian Reichel wrote: > Hi, > > On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: >> From: Tomi Valkeinen <tomi.valkeinen@ti.com> >> >> This patch adds basic support for a kernel driver to get a LED device. >> This will be used by the led-backlight driver. >> >> Only OF version is implemented for now, and the behavior is similar to >> PWM's of_pwm_get() and pwm_put(). >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >> Acked-by: Pavel Machek <pavel@ucw.cz> >> --- >> drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/leds.h | 4 ++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >> index c2167b66b61f..455545f5d663 100644 >> --- a/drivers/leds/led-class.c >> +++ b/drivers/leds/led-class.c >> @@ -19,6 +19,7 @@ >> #include <linux/spinlock.h> >> #include <linux/timer.h> >> #include <uapi/linux/uleds.h> >> +#include <linux/of.h> >> #include "leds.h" >> >> static struct class *leds_class; >> @@ -214,6 +215,49 @@ static int led_resume(struct device *dev) >> >> static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume); >> >> +/** >> + * of_led_get() - request a LED device via the LED framework >> + * @np: device node to get the LED device from >> + * @index: the index of the LED >> + * >> + * Returns the LED device parsed from the phandle specified in the "leds" >> + * property of a device tree node or a negative error-code on failure. >> + */ >> +struct led_classdev *of_led_get(struct device_node *np, int index) >> +{ >> + struct device *led_dev; >> + struct led_classdev *led_cdev; >> + struct device_node *led_node; >> + >> + led_node = of_parse_phandle(np, "leds", index); >> + if (!led_node) >> + return ERR_PTR(-ENOENT); >> + >> + led_dev = class_find_device_by_of_node(leds_class, led_node); > If you convert led_node into a fwnode, you can use > class_find_device_by_fwnode() instead. That way the > first patch can just be dropped. Thanks for the reviews. The first patch could be dropped indeed, but it would break something else I'm working on: getting regulator support in the LED core. This has been discussed during the v7 iteration of this series. JJ > > -- Sebastian > >> + of_node_put(led_node); >> + >> + if (!led_dev) >> + return ERR_PTR(-EPROBE_DEFER); >> + >> + led_cdev = dev_get_drvdata(led_dev); >> + >> + if (!try_module_get(led_cdev->dev->parent->driver->owner)) >> + return ERR_PTR(-ENODEV); >> + >> + return led_cdev; >> +} >> +EXPORT_SYMBOL_GPL(of_led_get); >> + >> +/** >> + * led_put() - release a LED device >> + * @led_cdev: LED device >> + */ >> +void led_put(struct led_classdev *led_cdev) >> +{ >> + module_put(led_cdev->dev->parent->driver->owner); >> +} >> +EXPORT_SYMBOL_GPL(led_put); >> + >> static int led_classdev_next_name(const char *init_name, char *name, >> size_t len) >> { >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index b8df71193329..6f7371bc7757 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -20,6 +20,7 @@ >> >> struct device; >> struct led_pattern; >> +struct device_node; >> /* >> * LED Core >> */ >> @@ -196,6 +197,9 @@ extern void devm_led_classdev_unregister(struct device *parent, >> extern void led_classdev_suspend(struct led_classdev *led_cdev); >> extern void led_classdev_resume(struct led_classdev *led_cdev); >> >> +extern struct led_classdev *of_led_get(struct device_node *np, int index); >> +extern void led_put(struct led_classdev *led_cdev); >> + >> /** >> * led_blink_set - set blinking with software fallback >> * @led_cdev: the LED to start blinking >> -- >> 2.17.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Jean, On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote: > Hi Sebastian, > > On 03/10/2019 12:42, Sebastian Reichel wrote: >> Hi, >> >> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: >>> From: Tomi Valkeinen <tomi.valkeinen@ti.com> >>> >>> This patch adds basic support for a kernel driver to get a LED device. >>> This will be used by the led-backlight driver. >>> >>> Only OF version is implemented for now, and the behavior is similar to >>> PWM's of_pwm_get() and pwm_put(). >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> >>> Acked-by: Pavel Machek <pavel@ucw.cz> >>> --- >>> drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/leds.h | 4 ++++ >>> 2 files changed, 48 insertions(+) >>> >>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c >>> index c2167b66b61f..455545f5d663 100644 >>> --- a/drivers/leds/led-class.c >>> +++ b/drivers/leds/led-class.c >>> @@ -19,6 +19,7 @@ >>> #include <linux/spinlock.h> >>> #include <linux/timer.h> >>> #include <uapi/linux/uleds.h> >>> +#include <linux/of.h> >>> #include "leds.h" >>> static struct class *leds_class; >>> @@ -214,6 +215,49 @@ static int led_resume(struct device *dev) >>> static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, >>> led_resume); >>> +/** >>> + * of_led_get() - request a LED device via the LED framework >>> + * @np: device node to get the LED device from >>> + * @index: the index of the LED >>> + * >>> + * Returns the LED device parsed from the phandle specified in the >>> "leds" >>> + * property of a device tree node or a negative error-code on failure. >>> + */ >>> +struct led_classdev *of_led_get(struct device_node *np, int index) >>> +{ >>> + struct device *led_dev; >>> + struct led_classdev *led_cdev; >>> + struct device_node *led_node; >>> + >>> + led_node = of_parse_phandle(np, "leds", index); >>> + if (!led_node) >>> + return ERR_PTR(-ENOENT); >>> + >>> + led_dev = class_find_device_by_of_node(leds_class, led_node); >> If you convert led_node into a fwnode, you can use >> class_find_device_by_fwnode() instead. That way the >> first patch can just be dropped. > > Thanks for the reviews. > > The first patch could be dropped indeed, but it would break something > else I'm working on: getting regulator support in the LED core. > > This has been discussed during the v7 iteration of this series. I wonder if it wouldn't make sense to add support for fwnode parsing to regulator core. Or maybe it is either somehow supported or not supported on purpose? These are questions to regulator maintainers. Adding Liam and Mark.
On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote: > On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote: > > On 03/10/2019 12:42, Sebastian Reichel wrote: > >> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: This mail has nothing relevant in the subject line and pages of quotes before the question for me, it's kind of lucky I noticed it.... > I wonder if it wouldn't make sense to add support for fwnode > parsing to regulator core. Or maybe it is either somehow supported > or not supported on purpose? Anything attempting to use the regulator DT bindings in ACPI has very serious problems, ACPI has its own power model which isn't compatible with that used in DT.
Hi Mark, On 10/3/19 8:35 PM, Mark Brown wrote: > On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote: >> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote: >>> On 03/10/2019 12:42, Sebastian Reichel wrote: >>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: > > This mail has nothing relevant in the subject line and pages of quotes > before the question for me, it's kind of lucky I noticed it.... Isn't it all about creating proper filters? >> I wonder if it wouldn't make sense to add support for fwnode >> parsing to regulator core. Or maybe it is either somehow supported >> or not supported on purpose? > > Anything attempting to use the regulator DT bindings in ACPI has very > serious problems, ACPI has its own power model which isn't compatible > with that used in DT. We have a means for checking if fwnode refers to of_node: is_of_node(const struct fwnode_handle *fwnode) Couldn't it be employed for OF case?
On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote: > On 10/3/19 8:35 PM, Mark Brown wrote: > > On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote: > >> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote: > >>> On 03/10/2019 12:42, Sebastian Reichel wrote: > >>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: > > This mail has nothing relevant in the subject line and pages of quotes > > before the question for me, it's kind of lucky I noticed it.... > Isn't it all about creating proper filters? My point there is that there's nothing obvious in the mail that suggests it should get past filters - just being CCed on a mail isn't super reliable, people often get pulled in due to things like checkpatch or someone copying a CC list from an earlier patch series where there were things were relevant. > >> I wonder if it wouldn't make sense to add support for fwnode > >> parsing to regulator core. Or maybe it is either somehow supported > >> or not supported on purpose? > > Anything attempting to use the regulator DT bindings in ACPI has very > > serious problems, ACPI has its own power model which isn't compatible > > with that used in DT. > We have a means for checking if fwnode refers to of_node: > is_of_node(const struct fwnode_handle *fwnode) > Couldn't it be employed for OF case? Why would we want to do that? We'd continue to support only DT systems, just with code that's less obviously DT only and would need to put checks in. I'm not seeing an upside here.
On 10/3/19 9:41 PM, Mark Brown wrote: > On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote: >> On 10/3/19 8:35 PM, Mark Brown wrote: >>> On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote: >>>> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote: >>>>> On 03/10/2019 12:42, Sebastian Reichel wrote: >>>>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: > >>> This mail has nothing relevant in the subject line and pages of quotes >>> before the question for me, it's kind of lucky I noticed it.... > >> Isn't it all about creating proper filters? > > My point there is that there's nothing obvious in the mail that suggests > it should get past filters - just being CCed on a mail isn't super > reliable, people often get pulled in due to things like checkpatch or > someone copying a CC list from an earlier patch series where there were > things were relevant. OK, updated the subject. >>>> I wonder if it wouldn't make sense to add support for fwnode >>>> parsing to regulator core. Or maybe it is either somehow supported >>>> or not supported on purpose? > >>> Anything attempting to use the regulator DT bindings in ACPI has very >>> serious problems, ACPI has its own power model which isn't compatible >>> with that used in DT. > >> We have a means for checking if fwnode refers to of_node: > >> is_of_node(const struct fwnode_handle *fwnode) > >> Couldn't it be employed for OF case? > > Why would we want to do that? We'd continue to support only DT systems, > just with code that's less obviously DT only and would need to put > checks in. I'm not seeing an upside here. For instance few weeks ago we had a patch [0] in the LED core switching from using struct device's of_node property to fwnode for conveying device property data. And this transition to fwnode property API can be observed as a frequent pattern across subsystems. Recently there is an ongoing effort aiming to add generic support for handling regulators in the LED core [1], but it turns out to require bringing back initialization of of_node property for devm_regulator_get_optional() to work properly. Support for OF related fwnodes in regulator core could help reducing this noise. [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/leds/led-class.c?id=fd81d7e946c6bdb86dbf0bd88fee3e1a545e7979 [1] https://lore.kernel.org/linux-leds/20190923102059.17818-4-jjhiblot@ti.com/
On 03/10/2019 22:27, Jacek Anaszewski wrote: > On 10/3/19 9:41 PM, Mark Brown wrote: >> On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote: >>> On 10/3/19 8:35 PM, Mark Brown wrote: >>>> On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote: >>>>> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote: >>>>>> On 03/10/2019 12:42, Sebastian Reichel wrote: >>>>>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote: >>>> This mail has nothing relevant in the subject line and pages of quotes >>>> before the question for me, it's kind of lucky I noticed it.... >>> Isn't it all about creating proper filters? >> My point there is that there's nothing obvious in the mail that suggests >> it should get past filters - just being CCed on a mail isn't super >> reliable, people often get pulled in due to things like checkpatch or >> someone copying a CC list from an earlier patch series where there were >> things were relevant. > OK, updated the subject. > >>>>> I wonder if it wouldn't make sense to add support for fwnode >>>>> parsing to regulator core. Or maybe it is either somehow supported >>>>> or not supported on purpose? >>>> Anything attempting to use the regulator DT bindings in ACPI has very >>>> serious problems, ACPI has its own power model which isn't compatible >>>> with that used in DT. >>> We have a means for checking if fwnode refers to of_node: >>> is_of_node(const struct fwnode_handle *fwnode) >>> Couldn't it be employed for OF case? >> Why would we want to do that? We'd continue to support only DT systems, >> just with code that's less obviously DT only and would need to put >> checks in. I'm not seeing an upside here. > For instance few weeks ago we had a patch [0] in the LED core switching > from using struct device's of_node property to fwnode for conveying > device property data. And this transition to fwnode property API can be > observed as a frequent pattern across subsystems. > > Recently there is an ongoing effort aiming to add generic support for > handling regulators in the LED core [1], but it turns out to require > bringing back initialization of of_node property for > devm_regulator_get_optional() to work properly. > > Support for OF related fwnodes in regulator core could help reducing > this noise. We could have this done in dev_of_node(): static inline struct device_node *dev_of_node(struct device *dev) { if (!IS_ENABLED(CONFIG_OF) || !dev) return NULL; return dev->of_node ? dev->of_node : to_of_node(dev->fwnode); } Then it will only be a matter of using dev_of_node() instead of accessing directly dev->of_node > > [0] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/leds/led-class.c?id=fd81d7e946c6bdb86dbf0bd88fee3e1a545e7979 > [1] > https://lore.kernel.org/linux-leds/20190923102059.17818-4-jjhiblot@ti.com/ >
On Thu, Oct 03, 2019 at 10:27:26PM +0200, Jacek Anaszewski wrote: > On 10/3/19 9:41 PM, Mark Brown wrote: > > Why would we want to do that? We'd continue to support only DT systems, > > just with code that's less obviously DT only and would need to put > > checks in. I'm not seeing an upside here. > For instance few weeks ago we had a patch [0] in the LED core switching > from using struct device's of_node property to fwnode for conveying > device property data. And this transition to fwnode property API can be > observed as a frequent pattern across subsystems. For most subsystems the intent is to reuse DT bindings on embedded ACPI systems via _DSD. > Recently there is an ongoing effort aiming to add generic support for > handling regulators in the LED core [1], but it turns out to require > bringing back initialization of of_node property for > devm_regulator_get_optional() to work properly. Consumers should just be able to request a regulator without having to worry about how that's being provided - they should have no knowledge at all of firmware bindings or platform data for defining this. If they do that suggests there's an abstraction issue somewhere, what makes you think that doing something with of_node is required? Further, unless you have LEDs that work without power you probably shouldn't be using _get_optional() for their supply. That interface is intended only for supplies that may be physically absent.
On 04/10/2019 13:39, Mark Brown wrote: > On Thu, Oct 03, 2019 at 10:27:26PM +0200, Jacek Anaszewski wrote: >> On 10/3/19 9:41 PM, Mark Brown wrote: >>> Why would we want to do that? We'd continue to support only DT systems, >>> just with code that's less obviously DT only and would need to put >>> checks in. I'm not seeing an upside here. >> For instance few weeks ago we had a patch [0] in the LED core switching >> from using struct device's of_node property to fwnode for conveying >> device property data. And this transition to fwnode property API can be >> observed as a frequent pattern across subsystems. > For most subsystems the intent is to reuse DT bindings on embedded ACPI > systems via _DSD. > >> Recently there is an ongoing effort aiming to add generic support for >> handling regulators in the LED core [1], but it turns out to require >> bringing back initialization of of_node property for >> devm_regulator_get_optional() to work properly. > Consumers should just be able to request a regulator without having to > worry about how that's being provided - they should have no knowledge at > all of firmware bindings or platform data for defining this. If they > do that suggests there's an abstraction issue somewhere, what makes you > think that doing something with of_node is required? The regulator core accesses consumer->of_node to get a phandle to a regulator's node. The trouble arises from the fact that the LED core does not populate of_node anymore, instead it populates fwnode. This allows the LED core to be agnostic of ACPI or OF to get the properties of a LED. IMO it is better to populate both of_node and fwnode in the LED core at the moment. It has already been fixed this way for the platform driver [0], MTD [1] and PCI-OF [2]. > > Further, unless you have LEDs that work without power you probably > shouldn't be using _get_optional() for their supply. That interface is > intended only for supplies that may be physically absent. Not all LEDs have a regulator to provide the power. The power can be supplied by the LED controller for example. [0] f94277af03ead0d3bf2 of/platform: Initialise dev->fwnode appropriately [1] c176c6d7e932662668 mfd: core: Set fwnode for created devices [2] 9b099a6c75e4ddceea PCI: OF: Initialize dev->fwnode appropriately
On Fri, Oct 04, 2019 at 03:33:13PM +0200, Jean-Jacques Hiblot wrote: > On 04/10/2019 13:39, Mark Brown wrote: > > Consumers should just be able to request a regulator without having to > > worry about how that's being provided - they should have no knowledge at > > all of firmware bindings or platform data for defining this. If they > > do that suggests there's an abstraction issue somewhere, what makes you > > think that doing something with of_node is required? > The regulator core accesses consumer->of_node to get a phandle to a > regulator's node. The trouble arises from the fact that the LED core does > not populate of_node anymore, instead it populates fwnode. This allows the > LED core to be agnostic of ACPI or OF to get the properties of a LED. Why is the LED core populating anything? Is the LED core copying bits out of the struct device for the actual device into a synthetic device rather than passing the actual device in? That really doesn't seem like a good idea, it's likely to lead to things like this where you don't copy something that's required (or worse where something directly in the struct device that can't be copied is needed). > IMO it is better to populate both of_node and fwnode in the LED core at the > moment. It has already been fixed this way for the platform driver [0], MTD > [1] and PCI-OF [2]. Yeah, if you're going to be copying stuff out of the real device I'd copy the of_node as well. > > Further, unless you have LEDs that work without power you probably > > shouldn't be using _get_optional() for their supply. That interface is > > intended only for supplies that may be physically absent. > Not all LEDs have a regulator to provide the power. The power can be > supplied by the LED controller for example. This code probably shouldn't be being run at all for LEDs like that, I was assuming this was just for GPIO LEDs and similar rather than all LEDs.
On 04/10/2019 16:40, Mark Brown wrote: > On Fri, Oct 04, 2019 at 03:33:13PM +0200, Jean-Jacques Hiblot wrote: >> On 04/10/2019 13:39, Mark Brown wrote: >>> Consumers should just be able to request a regulator without having to >>> worry about how that's being provided - they should have no knowledge at >>> all of firmware bindings or platform data for defining this. If they >>> do that suggests there's an abstraction issue somewhere, what makes you >>> think that doing something with of_node is required? >> The regulator core accesses consumer->of_node to get a phandle to a >> regulator's node. The trouble arises from the fact that the LED core does >> not populate of_node anymore, instead it populates fwnode. This allows the >> LED core to be agnostic of ACPI or OF to get the properties of a LED. > Why is the LED core populating anything? Is the LED core copying bits > out of the struct device for the actual device into a synthetic device > rather than passing the actual device in? That really doesn't seem like > a good idea, it's likely to lead to things like this where you don't > copy something that's required (or worse where something directly in the > struct device that can't be copied is needed). This is not a copy of a device of parent's of_node or something like that. You can think of a LED controller as a bus. It 'enumerates' its children LED, create the children devices (one per LED) and provides the functions to interact with them. The device node we are talking about here is a per-LED thing, it is a child node of the node of the LED controller. here is an example: tlc59108: tlc59116@40 { /* this is the node for the LED controller */ status = "okay"; #address-cells = <1>; #size-cells = <0>; compatible = "ti,tlc59108"; reg = <0x40>; backlight_led: led@2 { /* this is the node of one LED attached to pin#2 of the LED controller */ power-supply = <&bkl_fixed>; reg = <0x2>; }; other_led: led@3 { /* this is the node another LED attached to pin #3 of the LED controller */ power-supply = <®_3v3>; reg = <0x3>; }; }; > >> IMO it is better to populate both of_node and fwnode in the LED core at the >> moment. It has already been fixed this way for the platform driver [0], MTD >> [1] and PCI-OF [2]. > Yeah, if you're going to be copying stuff out of the real device I'd > copy the of_node as well. > >>> Further, unless you have LEDs that work without power you probably >>> shouldn't be using _get_optional() for their supply. That interface is >>> intended only for supplies that may be physically absent. >> Not all LEDs have a regulator to provide the power. The power can be >> supplied by the LED controller for example. > This code probably shouldn't be being run at all for LEDs like that, I > was assuming this was just for GPIO LEDs and similar rather than all > LEDs. > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Oct 04, 2019 at 05:13:13PM +0200, Jean-Jacques Hiblot wrote: > On 04/10/2019 16:40, Mark Brown wrote: > > Why is the LED core populating anything? Is the LED core copying bits > > out of the struct device for the actual device into a synthetic device > > rather than passing the actual device in? That really doesn't seem like > > a good idea, it's likely to lead to things like this where you don't > > copy something that's required (or worse where something directly in the > > struct device that can't be copied is needed). > This is not a copy of a device of parent's of_node or something like that. > You can think of a LED controller as a bus. It 'enumerates' its children > LED, create the children devices (one per LED) and provides the functions to > interact with them. > The device node we are talking about here is a per-LED thing, it is a child > node of the node of the LED controller. > here is an example: > > tlc59108: tlc59116@40 { /* this is the node for the LED controller */ > status = "okay"; > #address-cells = <1>; > #size-cells = <0>; > compatible = "ti,tlc59108"; > reg = <0x40>; > > backlight_led: led@2 { /* this is the node of one LED attached to > pin#2 of the LED controller */ > power-supply = <&bkl_fixed>; > reg = <0x2>; > }; Regulator supplies are supposed to be defined at the chip level rather than subfunctions with names corresponding to the names on the chip. This ensures that all chips look similar when you're mapping the schematic into a DT, you shouldn't need to know about the binding for a given chip to write a DT for it and if multiple people (perhaps working on different OSs) write bindings for the same chip there should be a good chance that they come up with the same mapping. The supply_alias interface is there to allow mapping these through to subfunctions if needed, it looks like the LED framework should be using this. That said if you are doing the above and the LEDs are appearing as devices it's extremely surprising that their of_node might not be initialized.
On 04/10/2019 17:58, Mark Brown wrote: > On Fri, Oct 04, 2019 at 05:13:13PM +0200, Jean-Jacques Hiblot wrote: >> On 04/10/2019 16:40, Mark Brown wrote: >>> Why is the LED core populating anything? Is the LED core copying bits >>> out of the struct device for the actual device into a synthetic device >>> rather than passing the actual device in? That really doesn't seem like >>> a good idea, it's likely to lead to things like this where you don't >>> copy something that's required (or worse where something directly in the >>> struct device that can't be copied is needed). >> This is not a copy of a device of parent's of_node or something like that. >> You can think of a LED controller as a bus. It 'enumerates' its children >> LED, create the children devices (one per LED) and provides the functions to >> interact with them. >> The device node we are talking about here is a per-LED thing, it is a child >> node of the node of the LED controller. >> here is an example: >> >> tlc59108: tlc59116@40 { /* this is the node for the LED controller */ >> status = "okay"; >> #address-cells = <1>; >> #size-cells = <0>; >> compatible = "ti,tlc59108"; >> reg = <0x40>; >> >> backlight_led: led@2 { /* this is the node of one LED attached to >> pin#2 of the LED controller */ >> power-supply = <&bkl_fixed>; >> reg = <0x2>; >> }; > Regulator supplies are supposed to be defined at the chip level rather > than subfunctions with names corresponding to the names on the chip. > This ensures that all chips look similar when you're mapping the > schematic into a DT, you shouldn't need to know about the binding for a > given chip to write a DT for it and if multiple people (perhaps working > on different OSs) write bindings for the same chip there should be a > good chance that they come up with the same mapping. The supply_alias > interface is there to allow mapping these through to subfunctions if > needed, it looks like the LED framework should be using this. In case of current-sink LED drivers, each LED can be powered by a different regulator, because the driver is only a switch between the LED cathod and the ground. > > That said if you are doing the above and the LEDs are appearing as > devices it's extremely surprising that their of_node might not be > initialized. That is because this is usually done by the platform core which is not involved here. JJ
On Fri, Oct 04, 2019 at 06:12:52PM +0200, Jean-Jacques Hiblot wrote: > On 04/10/2019 17:58, Mark Brown wrote: > > Regulator supplies are supposed to be defined at the chip level rather > > than subfunctions with names corresponding to the names on the chip. ... > > good chance that they come up with the same mapping. The supply_alias > > interface is there to allow mapping these through to subfunctions if > > needed, it looks like the LED framework should be using this. > In case of current-sink LED drivers, each LED can be powered by a different > regulator, because the driver is only a switch between the LED cathod and > the ground. Sure, it's common for devices to have supplies that are only needed by one part of the chip which is why we have the supply_alias interface for mapping things through. > > That said if you are doing the above and the LEDs are appearing as > > devices it's extremely surprising that their of_node might not be > > initialized. > That is because this is usually done by the platform core which is not > involved here. The surprise is more that it got instantiated from the DT without keeping the node around than how it happened.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index c2167b66b61f..455545f5d663 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -19,6 +19,7 @@ #include <linux/spinlock.h> #include <linux/timer.h> #include <uapi/linux/uleds.h> +#include <linux/of.h> #include "leds.h" static struct class *leds_class; @@ -214,6 +215,49 @@ static int led_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume); +/** + * of_led_get() - request a LED device via the LED framework + * @np: device node to get the LED device from + * @index: the index of the LED + * + * Returns the LED device parsed from the phandle specified in the "leds" + * property of a device tree node or a negative error-code on failure. + */ +struct led_classdev *of_led_get(struct device_node *np, int index) +{ + struct device *led_dev; + struct led_classdev *led_cdev; + struct device_node *led_node; + + led_node = of_parse_phandle(np, "leds", index); + if (!led_node) + return ERR_PTR(-ENOENT); + + led_dev = class_find_device_by_of_node(leds_class, led_node); + of_node_put(led_node); + + if (!led_dev) + return ERR_PTR(-EPROBE_DEFER); + + led_cdev = dev_get_drvdata(led_dev); + + if (!try_module_get(led_cdev->dev->parent->driver->owner)) + return ERR_PTR(-ENODEV); + + return led_cdev; +} +EXPORT_SYMBOL_GPL(of_led_get); + +/** + * led_put() - release a LED device + * @led_cdev: LED device + */ +void led_put(struct led_classdev *led_cdev) +{ + module_put(led_cdev->dev->parent->driver->owner); +} +EXPORT_SYMBOL_GPL(led_put); + static int led_classdev_next_name(const char *init_name, char *name, size_t len) { diff --git a/include/linux/leds.h b/include/linux/leds.h index b8df71193329..6f7371bc7757 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -20,6 +20,7 @@ struct device; struct led_pattern; +struct device_node; /* * LED Core */ @@ -196,6 +197,9 @@ extern void devm_led_classdev_unregister(struct device *parent, extern void led_classdev_suspend(struct led_classdev *led_cdev); extern void led_classdev_resume(struct led_classdev *led_cdev); +extern struct led_classdev *of_led_get(struct device_node *np, int index); +extern void led_put(struct led_classdev *led_cdev); + /** * led_blink_set - set blinking with software fallback * @led_cdev: the LED to start blinking