[PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This allows every led_classdev_register user to set of_node before
calling that function. Thanks to this LEDs core code may access DT node
during LED registration (e.g. to read extra info).

A small disadvantage of this imlementation is keeping of_node pointer in
two places: struct led_classdev and struct device.
Another solutions were:
1) Passing pointer with a led_classdev_register call
   This would require changing & complicating the API.
2) Moving allocation part of led_classdev_register to separated function
   This would make registering LED even more complicated.

Having duplicated pointer semms like a reasonable trade-off.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/leds/led-class.c | 1 +
 include/linux/leds.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 326ee6e925a2..9db9d05e4832 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -199,6 +199,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 				led_cdev, led_cdev->groups, "%s", name);
 	if (IS_ERR(led_cdev->dev))
 		return PTR_ERR(led_cdev->dev);
+	led_cdev->dev->of_node = led_cdev->of_node;
 
 	if (ret)
 		dev_warn(parent, "Led %s renamed to %s due to name collision",
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 569cb531094c..7dc4a679f376 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -85,6 +85,7 @@ struct led_classdev {
 				     unsigned long *delay_off);
 
 	struct device		*dev;
+	struct device_node	*of_node;
 	const struct attribute_group	**groups;
 
 	struct list_head	 node;			/* LED Device list */
-- 
2.11.0

[PATCH 2/2] leds: gpio: set of_node before calling led_classdev_register

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This allows core code to access DT info earlier, during LED registration
process. Thanks to this we will be able to support DT bindings in LEDs
core code.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/leds/leds-gpio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index d400dcaf4d29..7bdce5df658b 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -172,7 +172,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		struct gpio_led_data *led_dat = &priv->leds[priv->num_leds];
 		struct gpio_led led = {};
 		const char *state = NULL;
-		struct device_node *np = to_of_node(child);
+
+		led_dat->cdev.of_node = to_of_node(child);
 
 		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
@@ -181,8 +182,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		}
 
 		ret = fwnode_property_read_string(child, "label", &led.name);
-		if (ret && IS_ENABLED(CONFIG_OF) && np)
-			led.name = np->name;
+		if (ret && IS_ENABLED(CONFIG_OF) && led_dat->cdev.of_node)
+			led.name = led_dat->cdev.of_node->name;
 		if (!led.name) {
 			fwnode_handle_put(child);
 			return ERR_PTR(-EINVAL);
@@ -211,7 +212,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 			fwnode_handle_put(child);
 			return ERR_PTR(ret);
 		}
-		led_dat->cdev.dev->of_node = np;
 		priv->num_leds++;
 	}
 
-- 
2.11.0

Re: [PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Rafał Miłecki]

On 02/27/2017 11:22 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This allows every led_classdev_register user to set of_node before
> calling that function. Thanks to this LEDs core code may access DT node
> during LED registration (e.g. to read extra info).
>
> A small disadvantage of this imlementation is keeping of_node pointer in
> two places: struct led_classdev and struct device.
> Another solutions were:
> 1) Passing pointer with a led_classdev_register call
>    This would require changing & complicating the API.
> 2) Moving allocation part of led_classdev_register to separated function
>    This would make registering LED even more complicated.
>
> Having duplicated pointer semms like a reasonable trade-off.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/leds/led-class.c | 1 +
>  include/linux/leds.h     | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 326ee6e925a2..9db9d05e4832 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -199,6 +199,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  				led_cdev, led_cdev->groups, "%s", name);
>  	if (IS_ERR(led_cdev->dev))
>  		return PTR_ERR(led_cdev->dev);
> +	led_cdev->dev->of_node = led_cdev->of_node;
>
>  	if (ret)
>  		dev_warn(parent, "Led %s renamed to %s due to name collision",
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 569cb531094c..7dc4a679f376 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -85,6 +85,7 @@ struct led_classdev {
>  				     unsigned long *delay_off);
>
>  	struct device		*dev;
> +	struct device_node	*of_node;
>  	const struct attribute_group	**groups;
>
>  	struct list_head	 node;			/* LED Device list */
>

Sorry Jacek, I used your old e-mail accidentally. Hope you got this patchset.

Re: [PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Jacek Anaszewski]

Hi Rafał,

Thanks for the patch. How about following what has been already
done for spi, i.e. adding a wrapper of_led_classdev_register()
(please refer to of_register_spi_device() in drivers/spi/spi.c)?

Best regards,
Jacek Anaszewski

On 02/27/2017 11:22 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This allows every led_classdev_register user to set of_node before
> calling that function. Thanks to this LEDs core code may access DT node
> during LED registration (e.g. to read extra info).
> 
> A small disadvantage of this imlementation is keeping of_node pointer in
> two places: struct led_classdev and struct device.
> Another solutions were:
> 1) Passing pointer with a led_classdev_register call
>    This would require changing & complicating the API.
> 2) Moving allocation part of led_classdev_register to separated function
>    This would make registering LED even more complicated.
> 
> Having duplicated pointer semms like a reasonable trade-off.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/leds/led-class.c | 1 +
>  include/linux/leds.h     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 326ee6e925a2..9db9d05e4832 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -199,6 +199,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  				led_cdev, led_cdev->groups, "%s", name);
>  	if (IS_ERR(led_cdev->dev))
>  		return PTR_ERR(led_cdev->dev);
> +	led_cdev->dev->of_node = led_cdev->of_node;
>  
>  	if (ret)
>  		dev_warn(parent, "Led %s renamed to %s due to name collision",
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 569cb531094c..7dc4a679f376 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -85,6 +85,7 @@ struct led_classdev {
>  				     unsigned long *delay_off);
>  
>  	struct device		*dev;
> +	struct device_node	*of_node;
>  	const struct attribute_group	**groups;
>  
>  	struct list_head	 node;			/* LED Device list */
> 

Re: [PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Rafał Miłecki]

On 28 February 2017 at 22:20, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> Thanks for the patch. How about following what has been already
> done for spi, i.e. adding a wrapper of_led_classdev_register()
> (please refer to of_register_spi_device() in drivers/spi/spi.c)?

I like this idea, but I'll need to split led_classdev_register into
two functions: allocation & actual registering function. That way I
can alloc, then read DT proprties and finally register LED (just like
it's done in of_register_spi_device).

Does it sound OK?

Re: [PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Jacek Anaszewski]

On 03/02/2017 06:42 PM, Rafał Miłecki wrote:
> On 28 February 2017 at 22:20, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> Thanks for the patch. How about following what has been already
>> done for spi, i.e. adding a wrapper of_led_classdev_register()
>> (please refer to of_register_spi_device() in drivers/spi/spi.c)?
> 
> I like this idea, but I'll need to split led_classdev_register into
> two functions: allocation & actual registering function. That way I
> can alloc, then read DT proprties and finally register LED (just like
> it's done in of_register_spi_device).
> 
> Does it sound OK?

Yes, after thinking a bit more about that I came to the same
conclusion.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Rafał Miłecki]

On 2 March 2017 at 21:29, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> On 03/02/2017 06:42 PM, Rafał Miłecki wrote:
>> On 28 February 2017 at 22:20, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>> Thanks for the patch. How about following what has been already
>>> done for spi, i.e. adding a wrapper of_led_classdev_register()
>>> (please refer to of_register_spi_device() in drivers/spi/spi.c)?
>>
>> I like this idea, but I'll need to split led_classdev_register into
>> two functions: allocation & actual registering function. That way I
>> can alloc, then read DT proprties and finally register LED (just like
>> it's done in of_register_spi_device).
>>
>> Does it sound OK?
>
> Yes, after thinking a bit more about that I came to the same
> conclusion.

OK, I spent some extra time on this today, this seem a bit more complex.

I was starring at of_register_spi_device for quite some time and it
doesn't seem really related to this situation. In case of
of_register_spi_device:
1) It's used internally in spi core code only
2) It handles DT controller child node totally on its own

This patch I sent is about interaction between drivers registering
LEDs and LED core code. There are few drivers (that register LEDs)
parsing DT on their own. I'm afraid comparing this situation to
of_register_spi_device is a bit confusing for me. I really don't see a
place for of_led_classdev_register that would be called for every
child of LED controller (?!).

So I'd like to suggest not comparing this situation to
of_register_spi_device and discussing this problem on our own.

My current problem is that led_classdev_register:
1) Allocates struct device & registers LED
2) It means drivers can't touch struct device before having LED registered
3) It doesn't allow passing of_node
4) It is used in so many places it's almost impossible to modify all
callers at once

So maybe I could work on of_led_classdev_register that:
1) Would be almost identical to led_classdev_register
2) It would accept extra struct device_node argument

Does it make sense? Or maybe it's even what you meant from the beginning?

Two things I'll need to take in mind while working on this:
1) I'll need devm variant
2) I should share code between (of_)led_classdev_register functions

-- 
Rafał

Re: [PATCH 1/2] leds: core: add struct device_node pointer to the struct led_classdev

[Jacek Anaszewski]

On 03/02/2017 11:45 PM, Rafał Miłecki wrote:
> On 2 March 2017 at 21:29, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> On 03/02/2017 06:42 PM, Rafał Miłecki wrote:
>>> On 28 February 2017 at 22:20, Jacek Anaszewski
>>> <jacek.anaszewski@gmail.com> wrote:
>>>> Thanks for the patch. How about following what has been already
>>>> done for spi, i.e. adding a wrapper of_led_classdev_register()
>>>> (please refer to of_register_spi_device() in drivers/spi/spi.c)?
>>>
>>> I like this idea, but I'll need to split led_classdev_register into
>>> two functions: allocation & actual registering function. That way I
>>> can alloc, then read DT proprties and finally register LED (just like
>>> it's done in of_register_spi_device).
>>>
>>> Does it sound OK?
>>
>> Yes, after thinking a bit more about that I came to the same
>> conclusion.
> 
> OK, I spent some extra time on this today, this seem a bit more complex.
> 
> I was starring at of_register_spi_device for quite some time and it
> doesn't seem really related to this situation. In case of
> of_register_spi_device:
> 1) It's used internally in spi core code only
> 2) It handles DT controller child node totally on its own
> 
> This patch I sent is about interaction between drivers registering
> LEDs and LED core code. There are few drivers (that register LEDs)
> parsing DT on their own. I'm afraid comparing this situation to
> of_register_spi_device is a bit confusing for me. I really don't see a
> place for of_led_classdev_register that would be called for every
> child of LED controller (?!).
> 
> So I'd like to suggest not comparing this situation to
> of_register_spi_device and discussing this problem on our own.
> 
> My current problem is that led_classdev_register:
> 1) Allocates struct device & registers LED
> 2) It means drivers can't touch struct device before having LED registered
> 3) It doesn't allow passing of_node
> 4) It is used in so many places it's almost impossible to modify all
> callers at once
> 
> So maybe I could work on of_led_classdev_register that:
> 1) Would be almost identical to led_classdev_register
> 2) It would accept extra struct device_node argument
> 
> Does it make sense? Or maybe it's even what you meant from the beginning?

I intended that of_led_classdev_register() was almost identical to
led_classdev_register() but additionally had device_node parameter.

When agreeing with you on splitting led_classdev_register() to
allocation and registering function I meant creating two helper
functions that could be used as follows:

[pseudocode]

led_classdev_register(...):
    led_cdev->dev = alloc_led_classdev
    register_led_classdev(led_cdev)

of_led_classdev_register(..., of_node):
    led_cdev->dev = alloc_led_classdev
    led_cdev->of_node = of_node
    register_led_classdev(led_cdev)

I didn't mean updating all existing drivers to use new of_
prefixed API.

> Two things I'll need to take in mind while working on this:
> 1) I'll need devm variant

Right.

> 2) I should share code between (of_)led_classdev_register functions

Exactly.

-- 
Best regards,
Jacek Anaszewski