| Message ID | 20221216113013.126881-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote: > Add a generic [devm_]led_get() method which can be used on both devicetree > and non devicetree platforms to get a LED classdev associated with > a specific function on a specific device, e.g. the privacy LED associated > with a specific camera sensor. > > Note unlike of_led_get() this takes a string describing the function > rather then an index. This is done because e.g. camera sensors might > have a privacy LED, or a flash LED, or both and using an index > approach leaves it unclear what the function of index 0 is if there is > only 1 LED. The existing of support is extended to also support > getting a LED by function-name using the standard devicetree pattern > of adding a -names string array to map names to the indexes. > > For non devicetree platforms a lookup-table mechanism is added to > allow the platform code to map specific LED class_dev-s to specific > device,function combinations this way. ... > + list_for_each_entry(lookup, &leds_lookup_list, list) { > + if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) && > + !strcmp(lookup->consumer_function, function)) { > + led_name = kstrdup(lookup->led_name, GFP_KERNEL); kstrdup_const() ? > + break; > + } > + } > + if (!led_name) > + return ERR_PTR(-ENOENT); > + > + led_dev = class_find_device_by_name(leds_class, led_name); > + kfree(led_name); kfree_const() ? > + return __led_get(led_dev); > +} ... > +EXPORT_SYMBOL_GPL(led_add_lookup); > +EXPORT_SYMBOL_GPL(led_remove_lookup); Wondering why we can't make at least those two to be namespaced from day 1,
On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote: > Add a generic [devm_]led_get() method which can be used on both devicetree > and non devicetree platforms to get a LED classdev associated with > a specific function on a specific device, e.g. the privacy LED associated > with a specific camera sensor. > > Note unlike of_led_get() this takes a string describing the function > rather then an index. This is done because e.g. camera sensors might > have a privacy LED, or a flash LED, or both and using an index > approach leaves it unclear what the function of index 0 is if there is > only 1 LED. The existing of support is extended to also support > getting a LED by function-name using the standard devicetree pattern > of adding a -names string array to map names to the indexes. > > For non devicetree platforms a lookup-table mechanism is added to > allow the platform code to map specific LED class_dev-s to specific > device,function combinations this way. ... > +void led_remove_lookup(struct led_lookup_data *led_lookup) > +{ > + mutex_lock(&leds_lookup_lock); > + list_del(&led_lookup->list); Perhaps list_del_init(). Some thoughts about this in the one of the following comments. > + mutex_unlock(&leds_lookup_lock); > +}
Hi, On 12/16/22 14:43, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote: >> Add a generic [devm_]led_get() method which can be used on both devicetree >> and non devicetree platforms to get a LED classdev associated with >> a specific function on a specific device, e.g. the privacy LED associated >> with a specific camera sensor. >> >> Note unlike of_led_get() this takes a string describing the function >> rather then an index. This is done because e.g. camera sensors might >> have a privacy LED, or a flash LED, or both and using an index >> approach leaves it unclear what the function of index 0 is if there is >> only 1 LED. The existing of support is extended to also support >> getting a LED by function-name using the standard devicetree pattern >> of adding a -names string array to map names to the indexes. >> >> For non devicetree platforms a lookup-table mechanism is added to >> allow the platform code to map specific LED class_dev-s to specific >> device,function combinations this way. > > ... > >> + list_for_each_entry(lookup, &leds_lookup_list, list) { >> + if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) && >> + !strcmp(lookup->consumer_function, function)) { >> + led_name = kstrdup(lookup->led_name, GFP_KERNEL); > > kstrdup_const() ? Ack. > >> + break; >> + } >> + } > >> + if (!led_name) >> + return ERR_PTR(-ENOENT); >> + >> + led_dev = class_find_device_by_name(leds_class, led_name); >> + kfree(led_name); > > kfree_const() ? Ack, will change for v4. > >> + return __led_get(led_dev); >> +} > > ... > >> +EXPORT_SYMBOL_GPL(led_add_lookup); > >> +EXPORT_SYMBOL_GPL(led_remove_lookup); > > Wondering why we can't make at least those two to be namespaced from day 1, switching the LED subsystem code to use module namespaces is a RFE which is independent of / orthogonal to this patchset. Regards, Hans
On Fri, Dec 16, 2022 at 04:54:59PM +0100, Hans de Goede wrote: > On 12/16/22 14:43, Andy Shevchenko wrote: > > On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote: ... > >> +EXPORT_SYMBOL_GPL(led_add_lookup); > > > >> +EXPORT_SYMBOL_GPL(led_remove_lookup); > > > > Wondering why we can't make at least those two to be namespaced from day 1, > > switching the LED subsystem code to use module namespaces is > a RFE which is independent of / orthogonal to this patchset. Adding new unnamespaced APIs only delays and increases a burden of the work. We can start already for the logically separated subset of API, no? That's why I asked about this.
Hi, On 12/16/22 17:07, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 04:54:59PM +0100, Hans de Goede wrote: >> On 12/16/22 14:43, Andy Shevchenko wrote: >>> On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote: > > ... > >>>> +EXPORT_SYMBOL_GPL(led_add_lookup); >>> >>>> +EXPORT_SYMBOL_GPL(led_remove_lookup); >>> >>> Wondering why we can't make at least those two to be namespaced from day 1, >> >> switching the LED subsystem code to use module namespaces is >> a RFE which is independent of / orthogonal to this patchset. > > Adding new unnamespaced APIs only delays and increases a burden of the work. > We can start already for the logically separated subset of API, no? That's why > I asked about this. Sorry, but I really see this as an independent changes and there is enough discussion around these changes as is without throwing this into the mix. Regards, Hans
On Fri, Dec 16, 2022 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote: > Add a generic [devm_]led_get() method which can be used on both devicetree > and non devicetree platforms to get a LED classdev associated with > a specific function on a specific device, e.g. the privacy LED associated > with a specific camera sensor. > > Note unlike of_led_get() this takes a string describing the function > rather then an index. This is done because e.g. camera sensors might > have a privacy LED, or a flash LED, or both and using an index > approach leaves it unclear what the function of index 0 is if there is > only 1 LED. The existing of support is extended to also support > getting a LED by function-name using the standard devicetree pattern > of adding a -names string array to map names to the indexes. > > For non devicetree platforms a lookup-table mechanism is added to > allow the platform code to map specific LED class_dev-s to specific > device,function combinations this way. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I see you need to iron out some details but the concept is clean cut and this is exactly what we want to do. I think it was Bjorn Andersson who pointed out to me years and years ago "why can't we do that, like any other resource?" and here it is. Yours, Linus Walleij
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 6e25c411ba35..76b3ab66eb88 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -23,6 +23,8 @@ #include "leds.h" static struct class *leds_class; +static DEFINE_MUTEX(leds_lookup_lock); +static LIST_HEAD(leds_lookup_list); static ssize_t brightness_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -329,6 +331,95 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev, } EXPORT_SYMBOL_GPL(devm_of_led_get); +/** + * led_get() - request a LED device via the LED framework + * @dev: device for which to get the LED device + * @function: string describing the function of the LED device + * + * @return a pointer to a LED device or ERR_PTR(errno) on failure. + */ +struct led_classdev *led_get(struct device *dev, char *function) +{ + struct led_lookup_data *lookup; + struct led_classdev *led_cdev; + const char *led_name = NULL; + struct device *led_dev; + + if (dev->of_node) { + led_cdev = __of_led_get(dev->of_node, -1, function); + if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT) + return led_cdev; + } + + mutex_lock(&leds_lookup_lock); + list_for_each_entry(lookup, &leds_lookup_list, list) { + if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) && + !strcmp(lookup->consumer_function, function)) { + led_name = kstrdup(lookup->led_name, GFP_KERNEL); + break; + } + } + mutex_unlock(&leds_lookup_lock); + + if (!led_name) + return ERR_PTR(-ENOENT); + + led_dev = class_find_device_by_name(leds_class, led_name); + kfree(led_name); + + return __led_get(led_dev); +} +EXPORT_SYMBOL_GPL(led_get); + +/** + * devm_led_get() - request a LED device via the LED framework + * @dev: device for which to get the LED device + * @function: string describing the function of the LED device + * + * The LED device returned from this function is automatically released + * on driver detach. + * + * @return a pointer to a LED device or ERR_PTR(errno) on failure. + */ +struct led_classdev *devm_led_get(struct device *dev, char *function) +{ + struct led_classdev *led; + + led = led_get(dev, function); + if (IS_ERR(led)) + return led; + + return __devm_led_get(dev, led); +} +EXPORT_SYMBOL_GPL(devm_led_get); + +/** + * led_add_lookup() - Add a LED lookup table entry + * @led_lookup: the lookup table entry to add + * + * Add a LED lookup table entry. On systems without devicetree the lookup table + * is used by led_get() to find LEDs. + */ +void led_add_lookup(struct led_lookup_data *led_lookup) +{ + mutex_lock(&leds_lookup_lock); + list_add_tail(&led_lookup->list, &leds_lookup_list); + mutex_unlock(&leds_lookup_lock); +} +EXPORT_SYMBOL_GPL(led_add_lookup); + +/** + * led_remove_lookup() - Remove a LED lookup table entry + * @led_lookup: the lookup table entry to remove + */ +void led_remove_lookup(struct led_lookup_data *led_lookup) +{ + mutex_lock(&leds_lookup_lock); + list_del(&led_lookup->list); + mutex_unlock(&leds_lookup_lock); +} +EXPORT_SYMBOL_GPL(led_remove_lookup); + 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 ba4861ec73d3..e44fc5ec7c9a 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -39,6 +39,18 @@ enum led_default_state { LEDS_DEFSTATE_KEEP = 2, }; +/* + * This is used to tell led_get() device which led_classdev to return for + * a specific consumer device-name, function pair on non devicetree platforms. + * Note all strings must be set. + */ +struct led_lookup_data { + struct list_head list; + const char *led_name; + const char *consumer_dev_name; + const char *consumer_function; +}; + struct led_init_data { /* device fwnode handle */ struct fwnode_handle *fwnode; @@ -211,6 +223,12 @@ void devm_led_classdev_unregister(struct device *parent, void led_classdev_suspend(struct led_classdev *led_cdev); void led_classdev_resume(struct led_classdev *led_cdev); +void led_add_lookup(struct led_lookup_data *led_lookup); +void led_remove_lookup(struct led_lookup_data *led_lookup); + +struct led_classdev *__must_check led_get(struct device *dev, char *function); +struct led_classdev *__must_check devm_led_get(struct device *dev, char *function); + extern struct led_classdev *of_led_get(struct device_node *np, int index); extern void led_put(struct led_classdev *led_cdev); struct led_classdev *__must_check devm_of_led_get(struct device *dev,
Add a generic [devm_]led_get() method which can be used on both devicetree and non devicetree platforms to get a LED classdev associated with a specific function on a specific device, e.g. the privacy LED associated with a specific camera sensor. Note unlike of_led_get() this takes a string describing the function rather then an index. This is done because e.g. camera sensors might have a privacy LED, or a flash LED, or both and using an index approach leaves it unclear what the function of index 0 is if there is only 1 LED. The existing of support is extended to also support getting a LED by function-name using the standard devicetree pattern of adding a -names string array to map names to the indexes. For non devicetree platforms a lookup-table mechanism is added to allow the platform code to map specific LED class_dev-s to specific device,function combinations this way. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/leds/led-class.c | 91 ++++++++++++++++++++++++++++++++++++++++ include/linux/leds.h | 18 ++++++++ 2 files changed, 109 insertions(+)