[v8,2/5] leds: Add of_led_get() and led_put()

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

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>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  4 ++++
 2 files changed, 48 insertions(+)

Hi Sebastian,

On 03/10/2019 12:42, Sebastian Reichel wrote:
> Hi,
>
> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> 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>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> ---
>>   drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/leds.h     |  4 ++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index c2167b66b61f..455545f5d663 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/timer.h>
>>   #include <uapi/linux/uleds.h>
>> +#include <linux/of.h>
>>   #include "leds.h"
>>   
>>   static struct class *leds_class;
>> @@ -214,6 +215,49 @@ static int led_resume(struct device *dev)
>>   
>>   static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>>   
>> +/**
>> + * of_led_get() - request a LED device via the LED framework
>> + * @np: device node to get the LED device from
>> + * @index: the index of the LED
>> + *
>> + * 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, int index)
>> +{
>> +	struct device *led_dev;
>> +	struct led_classdev *led_cdev;
>> +	struct device_node *led_node;
>> +
>> +	led_node = of_parse_phandle(np, "leds", index);
>> +	if (!led_node)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	led_dev = class_find_device_by_of_node(leds_class, led_node);
> If you convert led_node into a fwnode, you can use
> class_find_device_by_fwnode() instead. That way the
> first patch can just be dropped.

Thanks for the reviews.

The first patch could be dropped  indeed, but it would break something 
else I'm working on: getting regulator support in the LED core.

This has been discussed during the v7 iteration of this series.

JJ


>
> -- Sebastian
>
>> +	of_node_put(led_node);
>> +
>> +	if (!led_dev)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	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 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 b8df71193329..6f7371bc7757 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -20,6 +20,7 @@
>>   
>>   struct device;
>>   struct led_pattern;
>> +struct device_node;
>>   /*
>>    * LED Core
>>    */
>> @@ -196,6 +197,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, int index);
>> +extern void led_put(struct led_classdev *led_cdev);
>> +
>>   /**
>>    * led_blink_set - set blinking with software fallback
>>    * @led_cdev: the LED to start blinking
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:
> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:

> > On 03/10/2019 12:42, Sebastian Reichel wrote:

> >> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:


This mail has nothing relevant in the subject line and pages of quotes
before the question for me, it's kind of lucky I noticed it....

> I wonder if it wouldn't make sense to add support for fwnode

> parsing to regulator core. Or maybe it is either somehow supported

> or not supported on purpose?


Anything attempting to use the regulator DT bindings in ACPI has very
serious problems, ACPI has its own power model which isn't compatible
with that used in DT.

On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote:
> On 10/3/19 8:35 PM, Mark Brown wrote:

> > On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:

> >> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:

> >>> On 03/10/2019 12:42, Sebastian Reichel wrote:

> >>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:


> > This mail has nothing relevant in the subject line and pages of quotes

> > before the question for me, it's kind of lucky I noticed it....


> Isn't it all about creating proper filters?


My point there is that there's nothing obvious in the mail that suggests
it should get past filters - just being CCed on a mail isn't super
reliable, people often get pulled in due to things like checkpatch or
someone copying a CC list from an earlier patch series where there were
things were relevant.

> >> I wonder if it wouldn't make sense to add support for fwnode

> >> parsing to regulator core. Or maybe it is either somehow supported

> >> or not supported on purpose?


> > Anything attempting to use the regulator DT bindings in ACPI has very

> > serious problems, ACPI has its own power model which isn't compatible

> > with that used in DT.


> We have a means for checking if fwnode refers to of_node:


> is_of_node(const struct fwnode_handle *fwnode)


> Couldn't it be employed for OF case?


Why would we want to do that?  We'd continue to support only DT systems,
just with code that's less obviously DT only and would need to put
checks in.  I'm not seeing an upside here.

On Thu, Oct 03, 2019 at 10:27:26PM +0200, Jacek Anaszewski wrote:
> On 10/3/19 9:41 PM, Mark Brown wrote:


> > Why would we want to do that?  We'd continue to support only DT systems,

> > just with code that's less obviously DT only and would need to put

> > checks in.  I'm not seeing an upside here.


> For instance few weeks ago we had a patch [0] in the LED core switching

> from using struct device's of_node property to fwnode for conveying

> device property data. And this transition to fwnode property API can be

> observed as a frequent pattern across subsystems.


For most subsystems the intent is to reuse DT bindings on embedded ACPI
systems via _DSD.

> Recently there is an ongoing effort aiming to add generic support for

> handling regulators in the LED core [1], but it turns out to require

> bringing back initialization of of_node property for

> devm_regulator_get_optional() to work properly.


Consumers should just be able to request a regulator without having to
worry about how that's being provided - they should have no knowledge at
all of firmware bindings or platform data for defining this.  If they
do that suggests there's an abstraction issue somewhere, what makes you
think that doing something with of_node is required?

Further, unless you have LEDs that work without power you probably
shouldn't be using _get_optional() for their supply.  That interface is
intended only for supplies that may be physically absent.

On Fri, Oct 04, 2019 at 03:33:13PM +0200, Jean-Jacques Hiblot wrote:
> On 04/10/2019 13:39, Mark Brown wrote:


> > Consumers should just be able to request a regulator without having to

> > worry about how that's being provided - they should have no knowledge at

> > all of firmware bindings or platform data for defining this.  If they

> > do that suggests there's an abstraction issue somewhere, what makes you

> > think that doing something with of_node is required?


> The regulator core accesses consumer->of_node to get a phandle to a

> regulator's node. The trouble arises from the fact that the LED core does

> not populate of_node anymore, instead it populates fwnode. This allows the

> LED core to be agnostic of ACPI or OF to get the properties of a LED.


Why is the LED core populating anything?  Is the LED core copying bits
out of the struct device for the actual device into a synthetic device
rather than passing the actual device in?  That really doesn't seem like
a good idea, it's likely to lead to things like this where you don't
copy something that's required (or worse where something directly in the
struct device that can't be copied is needed).

> IMO it is better to populate both of_node and fwnode in the LED core at the

> moment. It has already been fixed this way for the platform driver [0], MTD

> [1] and PCI-OF [2].


Yeah, if you're going to be copying stuff out of the real device I'd
copy the of_node as well.

> > Further, unless you have LEDs that work without power you probably

> > shouldn't be using _get_optional() for their supply.  That interface is

> > intended only for supplies that may be physically absent.


> Not all LEDs have a regulator to provide the power. The power can be

> supplied by the LED controller for example.


This code probably shouldn't be being run at all for LEDs like that, I
was assuming this was just for GPIO LEDs and similar rather than all
LEDs.