[v4,04/11] leds: led-class: Add generic [devm_]led_get()
Commit Message
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(+)
Comments
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.
Hi,
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:
>>
>> 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[];
> };
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.
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.
Regards,
Hans
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?
Hi,
On 1/19/23 15:54, Andy Shevchenko wrote:
> 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.
That will probably work, but only if there is only 1 variable sized
struct which you want to embed.
Also note that in the current use-case the struct is embedded in
a sub-struct of the main driver_data struct, so then not only
would this need to be the last member of the sub-struct, but
the sub-struct itself would need to be the last member of
the main driver_data struct.
Variable sized structs can be nice sometimes, but in cases where
we may want to embed them they are not always ideal.
>> 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.
Right, so there is precedent for this variant too.
> 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?
I see that the gpio and reset lookups indeed both use dev_id and con_id
I will change the LED lookups to use this to for version 5 of this
patch-set.
Regards,
Hans
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.
@@ -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)
{
@@ -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,