Message ID | 20230119130053.111344-1-hdegoede@redhat.com |
---|---|
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: > > led_put() is used to "undo" a successful of_led_get() call, > of_led_get() uses class_find_device_by_of_node() which returns > a reference to the device which must be free-ed with put_device() > when the caller is done with it. > > Add a put_device() call to led_put() to free the reference returned > by class_find_device_by_of_node(). > > And also add a put_device() in the error-exit case of try_module_get() > failing. This sounds to me like a bugfix. Why not the Fixes tag? Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/leds/led-class.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 6a8ea94834fa..7391d2cf1370 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -241,8 +241,10 @@ struct led_classdev *of_led_get(struct device_node *np, int index) > > led_cdev = dev_get_drvdata(led_dev); > > - if (!try_module_get(led_cdev->dev->parent->driver->owner)) > + if (!try_module_get(led_cdev->dev->parent->driver->owner)) { > + put_device(led_cdev->dev); > return ERR_PTR(-ENODEV); > + } > > return led_cdev; > } > @@ -255,6 +257,7 @@ EXPORT_SYMBOL_GPL(of_led_get); > void led_put(struct led_classdev *led_cdev) > { > module_put(led_cdev->dev->parent->driver->owner); > + put_device(led_cdev->dev); > } > EXPORT_SYMBOL_GPL(led_put); > > -- > 2.39.0 >
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: > led_put() is used to "undo" a successful of_led_get() call, > of_led_get() uses class_find_device_by_of_node() which returns > a reference to the device which must be free-ed with put_device() > when the caller is done with it. > > Add a put_device() call to led_put() to free the reference returned > by class_find_device_by_of_node(). > > And also add a put_device() in the error-exit case of try_module_get() > failing. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote: > Split out part of of_led_get() into a generic led_module_get() helper > function. > > This is a preparation patch for adding a generic (non devicetree specific) > led_get() function. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v4: > - Rename helper from __led_get() to led_module_get() Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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 Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote: > Here is my version 4 of my series to adjust the INT3472 code's handling of > the privacy LED on x86 laptops with MIPI camera(s) so that it will also > work on devices which have a privacy-LED GPIO but not a clk-enable GPIO > (so that we cannot just tie the LED state to the clk-enable state). > > Changes in v4: I think this is good for merge, I reviewed the LED stuff that I understand, but for the rest in drivers/media FWIW: Acked-by: Linus Walleij <linus.walleij@linaro.org> as well. I really like how this developed to solve a real old outstanding hole in the implementation. 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.