| Message ID | 1440502442-19531-2-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 25, 2015 at 02:34:00PM +0300, Tomi Valkeinen wrote: > 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(). Hi Tomi Is this the correct way to do it? I would of expected an xlate function. I'm also wondering if this is the right place, in the class. Don't you just need the core? Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/08/15 15:18, Andrew Lunn wrote: > On Tue, Aug 25, 2015 at 02:34:00PM +0300, Tomi Valkeinen wrote: >> 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(). > > Hi Tomi > > Is this the correct way to do it? I would of expected an xlate > function. I don't know. To be honest I haven't worked much with this kind of code, and I didn't want to start writing full blown "of" support for leds. My solution cuts the corners a bit... Probably this is more of an RFC than a ready patch series. I need to look what the of_xlate functions are doing in other frameworks. > I'm also wondering if this is the right place, in the class. Don't you > just need the core? I'm using the "static struct class *leds_class;" to find the actual led_classdev, and that variable is local to led-class.c. Tomi
Hi Tomi, Thanks for the patch. Generally, I'd prefer to add files drivers/leds/of.c and include/linux/of_leds.h and put related functions there. Those functions' names should begin with "of_". Please provide also no-op versions of the functions to address configurations when CONFIG_OF isn't enabled. I have also few comments below. On 08/25/2015 01:34 PM, Tomi Valkeinen wrote: > 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> > --- > drivers/leds/led-class.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/leds.h | 4 +++ > 2 files changed, 79 insertions(+) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index beabfbc6f7cd..0cb4fd7d71d6 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -20,6 +20,7 @@ > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > +#include <linux/of.h> > #include "leds.h" > > static struct class *leds_class; > @@ -216,6 +217,80 @@ static int led_resume(struct device *dev) > > static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume); > > +/* find OF node for the given led_cdev */ > +static struct device_node *find_led_of_node(struct led_classdev *led_cdev) > +{ > + struct device *led_dev = led_cdev->dev; > + struct device_node *child; > + > + for_each_child_of_node(led_dev->parent->of_node, child) { > + if (of_property_match_string(child, "label", led_cdev->name) == 0) > + return child; > + } > + > + return NULL; > +} > + > +static int led_match_led_node(struct device *led_dev, const void *data) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(led_dev); > + const struct device_node *target_node = data; > + struct device_node *led_node; > + > + led_node = find_led_of_node(led_cdev); > + if (!led_node) > + return 0; > + > + of_node_put(led_node); > + > + return led_node == target_node ? 1 : 0; > +} > + > +/** > + * of_led_get() - request a LED device via the LED framework > + * @np: device node to get the LED device from > + * > + * 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) > +{ > + struct device *led_dev; > + struct led_classdev *led_cdev; > + struct device_node *led_node; > + > + led_node = of_parse_phandle(np, "leds", 0); > + if (!led_node) > + return ERR_PTR(-ENODEV); > + > + led_dev = class_find_device(leds_class, NULL, led_node, > + led_match_led_node); Single of_node_put(led_node) here will do. > + if (!led_dev) { > + of_node_put(led_node); > + return ERR_PTR(-EPROBE_DEFER); > + } > + > + of_node_put(led_node); > + 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); Please move it to include/linux/leds.h, make static inline and provide also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled. > static int match_name(struct device *dev, const void *data) > { > if (!dev_name(dev)) > diff --git a/include/linux/leds.h b/include/linux/leds.h > index b122eeafb5dc..efc9b28af564 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -21,6 +21,7 @@ > #include <linux/workqueue.h> > > struct device; > +struct device_node; > /* > * LED Core > */ > @@ -113,6 +114,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); We need also no-op version for (!CONFIG_OF || !CONFIG_LEDS_CLASS). > +extern void led_put(struct led_classdev *led_cdev); > + > /** > * led_blink_set - set blinking with software fallback > * @led_cdev: the LED to start blinking >
Hi, On 25/08/15 16:25, Jacek Anaszewski wrote: > Hi Tomi, > > Thanks for the patch. Generally, I'd prefer to add files > drivers/leds/of.c and include/linux/of_leds.h and put related functions > there. Those functions' names should begin with "of_". Please provide Ok, I'll do that. I do need to export something from led-class in that case, so that the of.c gets hold of 'leds_class' pointer, either directly or indirectly. > also no-op versions of the functions to address configurations when > CONFIG_OF isn't enabled. I have also few comments below. Yep. No-ops for the purpose of making the kernel image smaller? I do think the current code compiles and works fine with CONFIG_OF disabled (although I have to say I don't remember if I actually tested it). >> +struct led_classdev *of_led_get(struct device_node *np) >> +{ >> + struct device *led_dev; >> + struct led_classdev *led_cdev; >> + struct device_node *led_node; >> + >> + led_node = of_parse_phandle(np, "leds", 0); >> + if (!led_node) >> + return ERR_PTR(-ENODEV); >> + >> + led_dev = class_find_device(leds_class, NULL, led_node, >> + led_match_led_node); > > Single of_node_put(led_node) here will do. Right. >> + if (!led_dev) { >> + of_node_put(led_node); >> + return ERR_PTR(-EPROBE_DEFER); >> + } >> + >> + of_node_put(led_node); > >> + 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); > > Please move it to include/linux/leds.h, make static inline and provide > also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled. Ok. Why do you want it as static inline in the leds.h? I usually like to keep the matching functions (get and put here) in the same place. Tomi
Hi, On 25/08/15 16:25, Jacek Anaszewski wrote: > Hi Tomi, > > Thanks for the patch. Generally, I'd prefer to add files > drivers/leds/of.c and include/linux/of_leds.h and put related functions > there. Those functions' names should begin with "of_". Please provide So I presume leds/of.c should be linked together with led-class.c. Afaics, that means I need to either rename the resulting led-class.ko or led-class.c, so that I can do it in the Makefile. Any preferences? Alternatively I could create a led-of.ko, but that doesn't feel right. Tomi
On 09/07/2015 03:18 PM, Tomi Valkeinen wrote: > Hi, > > On 25/08/15 16:25, Jacek Anaszewski wrote: >> Hi Tomi, >> >> Thanks for the patch. Generally, I'd prefer to add files >> drivers/leds/of.c and include/linux/of_leds.h and put related functions >> there. Those functions' names should begin with "of_". Please provide > > So I presume leds/of.c should be linked together with led-class.c. > Afaics, that means I need to either rename the resulting led-class.ko or > led-class.c, so that I can do it in the Makefile. Any preferences? Let's rename led-class.ko to led-class-objs.ko. > Alternatively I could create a led-of.ko, but that doesn't feel right.
On 09/07/2015 02:35 PM, Tomi Valkeinen wrote: > Hi, > > On 25/08/15 16:25, Jacek Anaszewski wrote: >> Hi Tomi, >> >> Thanks for the patch. Generally, I'd prefer to add files >> drivers/leds/of.c and include/linux/of_leds.h and put related functions >> there. Those functions' names should begin with "of_". Please provide > > Ok, I'll do that. I do need to export something from led-class in that > case, so that the of.c gets hold of 'leds_class' pointer, either > directly or indirectly. What exactly do you need to export? > >> also no-op versions of the functions to address configurations when >> CONFIG_OF isn't enabled. I have also few comments below. > > Yep. No-ops for the purpose of making the kernel image smaller? No, for the case when support for LED subsystem is disabled in the kernel config. I'd rather backlight driver depended on LED subsystem than selected it. > I do > think the current code compiles and works fine with CONFIG_OF disabled > (although I have to say I don't remember if I actually tested it). > >>> +struct led_classdev *of_led_get(struct device_node *np) >>> +{ >>> + struct device *led_dev; >>> + struct led_classdev *led_cdev; >>> + struct device_node *led_node; >>> + >>> + led_node = of_parse_phandle(np, "leds", 0); >>> + if (!led_node) >>> + return ERR_PTR(-ENODEV); >>> + >>> + led_dev = class_find_device(leds_class, NULL, led_node, >>> + led_match_led_node); >> >> Single of_node_put(led_node) here will do. > > Right. > >>> + if (!led_dev) { >>> + of_node_put(led_node); >>> + return ERR_PTR(-EPROBE_DEFER); >>> + } >>> + >>> + of_node_put(led_node); >> >>> + led_cdev = dev_get_drvdata(led_dev); >>> +pinctrl/pinctrl.h" >>> + 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); >> >> Please move it to include/linux/leds.h, make static inline and provide >> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled. > > Ok. Why do you want it as static inline in the leds.h? This function contains only one instruction, why should we waste a function call for it? > I usually like to > keep the matching functions (get and put here) in the same place.
On 07/09/15 17:11, Jacek Anaszewski wrote: >>> Thanks for the patch. Generally, I'd prefer to add files >>> drivers/leds/of.c and include/linux/of_leds.h and put related functions >>> there. Those functions' names should begin with "of_". Please provide >> >> Ok, I'll do that. I do need to export something from led-class in that >> case, so that the of.c gets hold of 'leds_class' pointer, either >> directly or indirectly. > > What exactly do you need to export? The "static struct class *leds_class" from led-class.c, in one way or another. of_led_get() needs to go through the led devices from the class. For now I just removed the "static" from it, so that I can use it from of.c. >>> also no-op versions of the functions to address configurations when >>> CONFIG_OF isn't enabled. I have also few comments below. >> >> Yep. No-ops for the purpose of making the kernel image smaller? > > No, for the case when support for LED subsystem is disabled in the > kernel config. I'd rather backlight driver depended on LED subsystem > than selected it. Sorry, I didn't get that one. How does the backlight driver's depend/select affect this? >>> Please move it to include/linux/leds.h, make static inline and provide >>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled. >> >> Ok. Why do you want it as static inline in the leds.h? > > This function contains only one instruction, why should we waste > a function call for it? I like to keep code in .c files and leave .h files for declarations, and only in special cases have inline code in .h files. What do you mean with "waste"? In this case there's no need to get more performance by inlining (which anyway may not help and needs to be studied separately for every case), with inlining the resulting kernel image is larger, and inlining forces the users of led_put to include module.h to compile. Tomi
On 09/08/2015 09:10 AM, Tomi Valkeinen wrote: > > On 07/09/15 17:11, Jacek Anaszewski wrote: > >>>> Thanks for the patch. Generally, I'd prefer to add files >>>> drivers/leds/of.c and include/linux/of_leds.h and put related functions >>>> there. Those functions' names should begin with "of_". Please provide >>> >>> Ok, I'll do that. I do need to export something from led-class in that >>> case, so that the of.c gets hold of 'leds_class' pointer, either >>> directly or indirectly. >> >> What exactly do you need to export? > > The "static struct class *leds_class" from led-class.c, in one way or > another. of_led_get() needs to go through the led devices from the class. > > For now I just removed the "static" from it, so that I can use it from of.c. I think we can go in this direction. I've skimmed through existing class drivers and found similar examples (e.g. tty_class, rtc_class). >>>> also no-op versions of the functions to address configurations when >>>> CONFIG_OF isn't enabled. I have also few comments below. >>> >>> Yep. No-ops for the purpose of making the kernel image smaller? >> >> No, for the case when support for LED subsystem is disabled in the >> kernel config. I'd rather backlight driver depended on LED subsystem >> than selected it. > > Sorry, I didn't get that one. How does the backlight driver's > depend/select affect this? OK, I confused something here. Backlight driver should depend on LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig. It should also depend on OF. In case of this driver the no-ops would be only for the purpose of making the kernel image smaller, as the driver probe will fail without them anyway. Nevertheless, there might be added other drivers in the future, using of_led_get{put} API which would like to make some decisions basing on whether LEDS_CLASS or/and OF are turned on. No-ops would be of use then. >>>> Please move it to include/linux/leds.h, make static inline and provide >>>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled. >>> >>> Ok. Why do you want it as static inline in the leds.h? >> >> This function contains only one instruction, why should we waste >> a function call for it? > > I like to keep code in .c files and leave .h files for declarations, and > only in special cases have inline code in .h files. > > What do you mean with "waste"? I used 'waste' because we would be wasting time here for the call which can be avoided at no cost. Of course if compiler will decide to inline it. > In this case there's no need to get more performance by inlining Why so? > (which > anyway may not help and needs to be studied separately for every case), There is the tradeoff. Readability vs performance. > with inlining the resulting kernel image is larger, That's why we inline only small functions. > and inlining forces the users of led_put to include module.h to compile. We could include module.h from leds.h.
On 08/09/15 12:21, Jacek Anaszewski wrote: >> The "static struct class *leds_class" from led-class.c, in one way or >> another. of_led_get() needs to go through the led devices from the class. >> >> For now I just removed the "static" from it, so that I can use it from >> of.c. > > I think we can go in this direction. I've skimmed through existing > class drivers and found similar examples (e.g. tty_class, rtc_class). Yep. >> Sorry, I didn't get that one. How does the backlight driver's >> depend/select affect this? > > OK, I confused something here. Backlight driver should depend on > LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig. > It should also depend on OF. In case of this driver the no-ops would be > only for the purpose of making the kernel image smaller, as the driver > probe will fail without them anyway. Nevertheless, there might be added > other drivers in the future, using of_led_get{put} API which would like > to make some decisions basing on whether LEDS_CLASS or/and OF are > turned on. No-ops would be of use then. I agree. >> What do you mean with "waste"? > > I used 'waste' because we would be wasting time here for the call which > can be avoided at no cost. Of course if compiler will decide to inline > it. > >> In this case there's no need to get more performance by inlining > > Why so? Reserving and freeing resources are rarely hot paths. The functions in question are usually called in a driver's probe and remove. Saving a few CPU cycles there doesn't really matter, so I think readability is the important part here. >> and inlining forces the users of led_put to include module.h to compile. > > We could include module.h from leds.h. Yes we can. And I can do that in my patch. I don't agree with the solution but neither do I really have a problem with it =). Tomi
On 09/08/2015 12:41 PM, Tomi Valkeinen wrote: > > On 08/09/15 12:21, Jacek Anaszewski wrote: > >>> The "static struct class *leds_class" from led-class.c, in one way or >>> another. of_led_get() needs to go through the led devices from the class. >>> >>> For now I just removed the "static" from it, so that I can use it from >>> of.c. >> >> I think we can go in this direction. I've skimmed through existing >> class drivers and found similar examples (e.g. tty_class, rtc_class). > > Yep. > >>> Sorry, I didn't get that one. How does the backlight driver's >>> depend/select affect this? >> >> OK, I confused something here. Backlight driver should depend on >> LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig. >> It should also depend on OF. In case of this driver the no-ops would be >> only for the purpose of making the kernel image smaller, as the driver >> probe will fail without them anyway. Nevertheless, there might be added >> other drivers in the future, using of_led_get{put} API which would like >> to make some decisions basing on whether LEDS_CLASS or/and OF are >> turned on. No-ops would be of use then. > > I agree. > >>> What do you mean with "waste"? >> >> I used 'waste' because we would be wasting time here for the call which >> can be avoided at no cost. Of course if compiler will decide to inline >> it. >> >>> In this case there's no need to get more performance by inlining >> >> Why so? > > Reserving and freeing resources are rarely hot paths. The functions in > question are usually called in a driver's probe and remove. Saving a few > CPU cycles there doesn't really matter, so I think readability is the > important part here. OK, I agree. >>> and inlining forces the users of led_put to include module.h to compile. >> >> We could include module.h from leds.h. > > Yes we can. And I can do that in my patch. I don't agree with the > solution but neither do I really have a problem with it =). Please go ahead as you originally planned, i.e. without inlining.
Hi Andrew, On 25/08/15 15:18, Andrew Lunn wrote: > On Tue, Aug 25, 2015 at 02:34:00PM +0300, Tomi Valkeinen wrote: >> 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(). > > Hi Tomi > > Is this the correct way to do it? I would of expected an xlate > function. I just sent v2, but I don't use xlate there. If I understand the purpose of xlate (in pwm, for example) correctly, xlate is a function in the pwm chip to allow custom bindings for the pwm outputs from that pwm chip. The problem with LEDs is that there's no "LED chip". Each LED is modelled as individual device, without a well defined parent. Thus there's no place to add such an xlate function. Do you have any thoughts on what the xlate for LEDs should look like? Tomi
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index beabfbc6f7cd..0cb4fd7d71d6 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -20,6 +20,7 @@ #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/timer.h> +#include <linux/of.h> #include "leds.h" static struct class *leds_class; @@ -216,6 +217,80 @@ static int led_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume); +/* find OF node for the given led_cdev */ +static struct device_node *find_led_of_node(struct led_classdev *led_cdev) +{ + struct device *led_dev = led_cdev->dev; + struct device_node *child; + + for_each_child_of_node(led_dev->parent->of_node, child) { + if (of_property_match_string(child, "label", led_cdev->name) == 0) + return child; + } + + return NULL; +} + +static int led_match_led_node(struct device *led_dev, const void *data) +{ + struct led_classdev *led_cdev = dev_get_drvdata(led_dev); + const struct device_node *target_node = data; + struct device_node *led_node; + + led_node = find_led_of_node(led_cdev); + if (!led_node) + return 0; + + of_node_put(led_node); + + return led_node == target_node ? 1 : 0; +} + +/** + * of_led_get() - request a LED device via the LED framework + * @np: device node to get the LED device from + * + * 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) +{ + struct device *led_dev; + struct led_classdev *led_cdev; + struct device_node *led_node; + + led_node = of_parse_phandle(np, "leds", 0); + if (!led_node) + return ERR_PTR(-ENODEV); + + led_dev = class_find_device(leds_class, NULL, led_node, + led_match_led_node); + if (!led_dev) { + of_node_put(led_node); + return ERR_PTR(-EPROBE_DEFER); + } + + of_node_put(led_node); + + 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 match_name(struct device *dev, const void *data) { if (!dev_name(dev)) diff --git a/include/linux/leds.h b/include/linux/leds.h index b122eeafb5dc..efc9b28af564 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -21,6 +21,7 @@ #include <linux/workqueue.h> struct device; +struct device_node; /* * LED Core */ @@ -113,6 +114,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); +extern void led_put(struct led_classdev *led_cdev); + /** * led_blink_set - set blinking with software fallback * @led_cdev: the LED to start blinking
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> --- drivers/leds/led-class.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/leds.h | 4 +++ 2 files changed, 79 insertions(+)