| Message ID | 20230119130053.111344-5-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
On Thu, Jan 19, 2023 at 3:01 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 than > 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. > > This uses a lookup-table mechanism for non devicetree platforms. > This allows the platform code to map specific LED class_dev-s to a specific > device,function combinations this way. > > For devicetree platforms getting the LED by function-name could be made > to work using the standard devicetree pattern of adding a -names string > array to map names to the indexes. ... > +/* > + * 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; > +}; I'm wondering if it would be better to have something like struct led_function_map { const char *name; const char *function; }; struct led_lookup_data { struct list_head list; const char *dev_name; const struct led_function_map map[]; }; as you may have more than one LED per the device and it would be a more compressed list in this case. I'm basically referring to the GPIO lookup table.
On Thu, Jan 19, 2023 at 4:16 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 1/19/23 15:04, Andy Shevchenko wrote: > > On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> +/* > >> + * 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; > >> +}; > > > > I'm wondering if it would be better to have something like > > > > struct led_function_map { > > const char *name; > > const char *function; > > }; > > > > struct led_lookup_data { > > struct list_head list; > > const char *dev_name; > > const struct led_function_map map[]; > > }; > > Thank you for the review. > > Since led_lookup_data now is variable sized, AFAIK this means that > the led_lookup_data now can no longer be embedded in another struct and > instead it must always be dynamically allocated, including adding error > checking + rollback for said allocation. I'm not sure what you are talking about here. GPIO lookup table defined in the same way and doesn't strictly require heap allocation. For the embedding into another structure, it can be as the last entry AFAIU. > If you look at the only current consumer of this: > > [PATCH v4 09/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED > > then the code there would become more complicated. > > as you may have more than one LED per the device and it would be a > > more compressed list in this case. I'm basically referring to the GPIO > > lookup table. > > Right, but having more then 1 GPIO per device is quite common while > I expect having more then 1 (or maybe 2) LEDs per device to be rare while > at the same time the suggested change makes things slightly more > complicated for consumers of the API which know before hand how much > lookup entries they will need (typically 1). > > So all in all I believe staying with the current implementation is better > but if there is a strong preference to switch to the structure you suggest > then I have no objection against that. I have no strong opinion, I just want to have fewer variants of the lookup tables. Anyway, reset framework has something similar to yours. Question: can you rename fields to be something like dev_id, con_id, etc as it's done in the most of the lookup data types?
On Thu, Jan 19, 2023 at 2:01 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. > > This uses a lookup-table mechanism for non devicetree platforms. > This allows the platform code to map specific LED class_dev-s to a specific > device,function combinations this way. > > For devicetree platforms getting the LED by function-name could be made > to work using the standard devicetree pattern of adding a -names string > array to map names to the indexes. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v4: > - Split out support for led_get() devicetree name-based lookup support > into a separate RFC patch as there currently are no user for this > - Use kstrdup_const() / kfree_const() for the led_name This is how I would implement it so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Fri, 20 Jan 2023, Linus Walleij wrote: > On Thu, Jan 19, 2023 at 2:01 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. > > > > This uses a lookup-table mechanism for non devicetree platforms. > > This allows the platform code to map specific LED class_dev-s to a specific > > device,function combinations this way. > > > > For devicetree platforms getting the LED by function-name could be made > > to work using the standard devicetree pattern of adding a -names string > > array to map names to the indexes. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > Changes in v4: > > - Split out support for led_get() devicetree name-based lookup support > > into a separate RFC patch as there currently are no user for this > > - Use kstrdup_const() / kfree_const() for the led_name > > This is how I would implement it so: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thanks Linus, this is all really helpful.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 4904d140a560..6dff57c41e96 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) @@ -317,6 +319,88 @@ 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; + const char *led_name = NULL; + struct device *led_dev; + + 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_const(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_const(led_name); + + return led_module_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. This uses a lookup-table mechanism for non devicetree platforms. This allows the platform code to map specific LED class_dev-s to a specific device,function combinations this way. For devicetree platforms getting the LED by function-name could be made to work using the standard devicetree pattern of adding a -names string array to map names to the indexes. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v4: - Split out support for led_get() devicetree name-based lookup support into a separate RFC patch as there currently are no user for this - Use kstrdup_const() / kfree_const() for the led_name --- drivers/leds/led-class.c | 84 ++++++++++++++++++++++++++++++++++++++++ include/linux/leds.h | 18 +++++++++ 2 files changed, 102 insertions(+)