[4/5] leds: aat1290: Pass dev and dev->of_node to v4l2_flash_init()
diff mbox

Message ID 1432076645-4799-5-git-send-email-sakari.ailus@iki.fi
State New
Headers show

Commit Message

Sakari Ailus May 19, 2015, 11:04 p.m. UTC
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/leds/leds-aat1290.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Jacek Anaszewski May 20, 2015, 9:47 a.m. UTC | #1
Hi Sakari,

On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>   drivers/leds/leds-aat1290.c |    5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> index c656a2d..71bf6bb 100644
> --- a/drivers/leds/leds-aat1290.c
> +++ b/drivers/leds/leds-aat1290.c
> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct platform_device *pdev)
>   	led_cdev->dev->of_node = sub_node;
>
>   	/* Create V4L2 Flash subdev. */
> -	led->v4l2_flash = v4l2_flash_init(fled_cdev,
> -					  &v4l2_flash_ops,
> -					  &v4l2_sd_cfg);
> +	led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> +					  &v4l2_flash_ops, &v4l2_sd_cfg);

Here the first argument should be led_cdev->dev, not dev, which is
&pdev->dev, whereas led_cdev->dev is returned by
device_create_with_groups (it takes dev as a parent) called from 
led_classdev_register.

>   	if (IS_ERR(led->v4l2_flash)) {
>   		ret = PTR_ERR(led->v4l2_flash);
>   		goto error_v4l2_flash_init;
>
Sakari Ailus May 20, 2015, 10:19 a.m. UTC | #2
Hi Jacek,

On Wed, May 20, 2015 at 11:47:26AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> >Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >---
> >  drivers/leds/leds-aat1290.c |    5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> >index c656a2d..71bf6bb 100644
> >--- a/drivers/leds/leds-aat1290.c
> >+++ b/drivers/leds/leds-aat1290.c
> >@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct platform_device *pdev)
> >  	led_cdev->dev->of_node = sub_node;
> >
> >  	/* Create V4L2 Flash subdev. */
> >-	led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >-					  &v4l2_flash_ops,
> >-					  &v4l2_sd_cfg);
> >+	led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >+					  &v4l2_flash_ops, &v4l2_sd_cfg);
> 
> Here the first argument should be led_cdev->dev, not dev, which is
> &pdev->dev, whereas led_cdev->dev is returned by
> device_create_with_groups (it takes dev as a parent) called from
> led_classdev_register.

Should it? The underlying hardware is still the I2C device, not the LED
class device node.
Jacek Anaszewski May 20, 2015, 10:37 a.m. UTC | #3
On 05/20/2015 11:47 AM, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 05/20/2015 01:04 AM, Sakari Ailus wrote:
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> ---
>>   drivers/leds/leds-aat1290.c |    5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
>> index c656a2d..71bf6bb 100644
>> --- a/drivers/leds/leds-aat1290.c
>> +++ b/drivers/leds/leds-aat1290.c
>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>> platform_device *pdev)
>>       led_cdev->dev->of_node = sub_node;
>>
>>       /* Create V4L2 Flash subdev. */
>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>> -                      &v4l2_flash_ops,
>> -                      &v4l2_sd_cfg);
>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>
> Here the first argument should be led_cdev->dev, not dev, which is
> &pdev->dev, whereas led_cdev->dev is returned by
> device_create_with_groups (it takes dev as a parent) called from
> led_classdev_register.

The reason for this is the fact that pdev->dev has its of_node
field initialized, which makes v4l2_async trying to match
subdev by parent node of a LED device, not by sub-LED related
DT node.

 From your patches it looks like you want to guarantee that
v4l2_subdev's dev field will point to the struct device
related to the LED controller, not to the sub-LED.

If so, then the second argument of v4l2_flash_init mustn't
be NULL.

I am however wondering if in case of LEDs the struct v4l2_subdev's dev
field couldn't be made pointing to sub-LEDs related struct device,
which would allow for avoiding the modifications in v4l2-async.

of_node field of struct device returned by device_create_with_groups
is NULL, and basically the remaining fields are copied from the parent.
In my approach I was just assigning to it the sub-LED related of_node.

>>       if (IS_ERR(led->v4l2_flash)) {
>>           ret = PTR_ERR(led->v4l2_flash);
>>           goto error_v4l2_flash_init;
>>
>
>
Sakari Ailus May 20, 2015, 12:27 p.m. UTC | #4
Hi Jacek,

On Wed, May 20, 2015 at 12:37:03PM +0200, Jacek Anaszewski wrote:
> On 05/20/2015 11:47 AM, Jacek Anaszewski wrote:
> >Hi Sakari,
> >
> >On 05/20/2015 01:04 AM, Sakari Ailus wrote:
> >>Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> >>---
> >>  drivers/leds/leds-aat1290.c |    5 ++---
> >>  1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
> >>index c656a2d..71bf6bb 100644
> >>--- a/drivers/leds/leds-aat1290.c
> >>+++ b/drivers/leds/leds-aat1290.c
> >>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>platform_device *pdev)
> >>      led_cdev->dev->of_node = sub_node;
> >>
> >>      /* Create V4L2 Flash subdev. */
> >>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>-                      &v4l2_flash_ops,
> >>-                      &v4l2_sd_cfg);
> >>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >
> >Here the first argument should be led_cdev->dev, not dev, which is
> >&pdev->dev, whereas led_cdev->dev is returned by
> >device_create_with_groups (it takes dev as a parent) called from
> >led_classdev_register.
> 
> The reason for this is the fact that pdev->dev has its of_node
> field initialized, which makes v4l2_async trying to match
> subdev by parent node of a LED device, not by sub-LED related
> DT node.

If v4l2_subdev->of_node is set, then it won't be replaced with one from
struct device. I.e. you need to provide of_node pointer only if it's
different from dev->of_node.
Jacek Anaszewski May 20, 2015, 1:47 p.m. UTC | #5
On 05/20/2015 02:27 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, May 20, 2015 at 12:37:03PM +0200, Jacek Anaszewski wrote:
>> On 05/20/2015 11:47 AM, Jacek Anaszewski wrote:
>>> Hi Sakari,
>>>
>>> On 05/20/2015 01:04 AM, Sakari Ailus wrote:
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>> ---
>>>>   drivers/leds/leds-aat1290.c |    5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
>>>> index c656a2d..71bf6bb 100644
>>>> --- a/drivers/leds/leds-aat1290.c
>>>> +++ b/drivers/leds/leds-aat1290.c
>>>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>>>> platform_device *pdev)
>>>>       led_cdev->dev->of_node = sub_node;
>>>>
>>>>       /* Create V4L2 Flash subdev. */
>>>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>>>> -                      &v4l2_flash_ops,
>>>> -                      &v4l2_sd_cfg);
>>>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>>>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>>>
>>> Here the first argument should be led_cdev->dev, not dev, which is
>>> &pdev->dev, whereas led_cdev->dev is returned by
>>> device_create_with_groups (it takes dev as a parent) called from
>>> led_classdev_register.
>>
>> The reason for this is the fact that pdev->dev has its of_node
>> field initialized, which makes v4l2_async trying to match
>> subdev by parent node of a LED device, not by sub-LED related
>> DT node.
>
> If v4l2_subdev->of_node is set, then it won't be replaced with one from
> struct device. I.e. you need to provide of_node pointer only if it's
> different from dev->of_node.
>

It will always be different since dev->of_node pointer is related
to the main DT node of LED device, whereas each LED connected to it
must be expressed in the form of sub-node, as
Documentation/devicetree/bindings/leds/common.txt DT states.
Sakari Ailus May 20, 2015, 2:31 p.m. UTC | #6
Hi Jacek,

On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
...
> >>>>--- a/drivers/leds/leds-aat1290.c
> >>>>+++ b/drivers/leds/leds-aat1290.c
> >>>>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>>>platform_device *pdev)
> >>>>      led_cdev->dev->of_node = sub_node;
> >>>>
> >>>>      /* Create V4L2 Flash subdev. */
> >>>>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>>>-                      &v4l2_flash_ops,
> >>>>-                      &v4l2_sd_cfg);
> >>>>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>>>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >>>
> >>>Here the first argument should be led_cdev->dev, not dev, which is
> >>>&pdev->dev, whereas led_cdev->dev is returned by
> >>>device_create_with_groups (it takes dev as a parent) called from
> >>>led_classdev_register.
> >>
> >>The reason for this is the fact that pdev->dev has its of_node
> >>field initialized, which makes v4l2_async trying to match
> >>subdev by parent node of a LED device, not by sub-LED related
> >>DT node.
> >
> >If v4l2_subdev->of_node is set, then it won't be replaced with one from
> >struct device. I.e. you need to provide of_node pointer only if it's
> >different from dev->of_node.
> >
> 
> It will always be different since dev->of_node pointer is related
> to the main DT node of LED device, whereas each LED connected to it
> must be expressed in the form of sub-node, as
> Documentation/devicetree/bindings/leds/common.txt DT states.

You can still refer to the device's root device_node using a phandle.

Say, if you have a LED flash controller with an indicator. It's intended to
be used together with the flash LED, and the existing as3645a driver exposes
it through the same sub-device. I think that'd make sense with LED class
driver as well (i.e. you'd have two LED class devices but a single
sub-device). Small changes to the wrapper would be needed.
Jacek Anaszewski May 21, 2015, 8:54 a.m. UTC | #7
Hi Sakari,

On 05/20/2015 04:31 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
> ...
>>>>>> --- a/drivers/leds/leds-aat1290.c
>>>>>> +++ b/drivers/leds/leds-aat1290.c
>>>>>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>>>>>> platform_device *pdev)
>>>>>>       led_cdev->dev->of_node = sub_node;
>>>>>>
>>>>>>       /* Create V4L2 Flash subdev. */
>>>>>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>>>>>> -                      &v4l2_flash_ops,
>>>>>> -                      &v4l2_sd_cfg);
>>>>>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>>>>>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>>>>>
>>>>> Here the first argument should be led_cdev->dev, not dev, which is
>>>>> &pdev->dev, whereas led_cdev->dev is returned by
>>>>> device_create_with_groups (it takes dev as a parent) called from
>>>>> led_classdev_register.
>>>>
>>>> The reason for this is the fact that pdev->dev has its of_node
>>>> field initialized, which makes v4l2_async trying to match
>>>> subdev by parent node of a LED device, not by sub-LED related
>>>> DT node.
>>>
>>> If v4l2_subdev->of_node is set, then it won't be replaced with one from
>>> struct device. I.e. you need to provide of_node pointer only if it's
>>> different from dev->of_node.
>>>
>>
>> It will always be different since dev->of_node pointer is related
>> to the main DT node of LED device, whereas each LED connected to it
>> must be expressed in the form of sub-node, as
>> Documentation/devicetree/bindings/leds/common.txt DT states.
>
> You can still refer to the device's root device_node using a phandle.

Why should I need to refer to the device's root node?

What I meant here was that DT documentation enforces that even if
there is a single LED connected to the device it has to be expressed
as a sub-node anyway. Each LED will have to be matched by the phandle
to the sub-node representing it. This implies that v4l2_subdev->of_node
(related to sub-LED DT node) will be always different from dev->of_node
(related to LED controller DT node).

> Say, if you have a LED flash controller with an indicator. It's intended to
> be used together with the flash LED, and the existing as3645a driver exposes
> it through the same sub-device. I think that'd make sense with LED class
> driver as well (i.e. you'd have two LED class devices but a single
> sub-device). Small changes to the wrapper would be needed.
>

How the sub-device name should look like then? We would have to
concatenate somehow both LED class device names?
Sakari Ailus May 21, 2015, 10:06 a.m. UTC | #8
Hi Jacek,

On Thu, May 21, 2015 at 10:54:07AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 05/20/2015 04:31 PM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
> >...
> >>>>>>--- a/drivers/leds/leds-aat1290.c
> >>>>>>+++ b/drivers/leds/leds-aat1290.c
> >>>>>>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>>>>>platform_device *pdev)
> >>>>>>      led_cdev->dev->of_node = sub_node;
> >>>>>>
> >>>>>>      /* Create V4L2 Flash subdev. */
> >>>>>>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>>>>>-                      &v4l2_flash_ops,
> >>>>>>-                      &v4l2_sd_cfg);
> >>>>>>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>>>>>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >>>>>
> >>>>>Here the first argument should be led_cdev->dev, not dev, which is
> >>>>>&pdev->dev, whereas led_cdev->dev is returned by
> >>>>>device_create_with_groups (it takes dev as a parent) called from
> >>>>>led_classdev_register.
> >>>>
> >>>>The reason for this is the fact that pdev->dev has its of_node
> >>>>field initialized, which makes v4l2_async trying to match
> >>>>subdev by parent node of a LED device, not by sub-LED related
> >>>>DT node.
> >>>
> >>>If v4l2_subdev->of_node is set, then it won't be replaced with one from
> >>>struct device. I.e. you need to provide of_node pointer only if it's
> >>>different from dev->of_node.
> >>>
> >>
> >>It will always be different since dev->of_node pointer is related
> >>to the main DT node of LED device, whereas each LED connected to it
> >>must be expressed in the form of sub-node, as
> >>Documentation/devicetree/bindings/leds/common.txt DT states.
> >
> >You can still refer to the device's root device_node using a phandle.
> 
> Why should I need to refer to the device's root node?
> 
> What I meant here was that DT documentation enforces that even if
> there is a single LED connected to the device it has to be expressed
> as a sub-node anyway. Each LED will have to be matched by the phandle
> to the sub-node representing it. This implies that v4l2_subdev->of_node
> (related to sub-LED DT node) will be always different from dev->of_node
> (related to LED controller DT node).

From driver point of view this makes no difference; it's just easier to
parse if you don't refer to the LEDs separately. I think this is a bit
special case; nowadays many LED flash controllers drive two LEDs.

> 
> >Say, if you have a LED flash controller with an indicator. It's intended to
> >be used together with the flash LED, and the existing as3645a driver exposes
> >it through the same sub-device. I think that'd make sense with LED class
> >driver as well (i.e. you'd have two LED class devices but a single
> >sub-device). Small changes to the wrapper would be needed.
> >
> 
> How the sub-device name should look like then? We would have to
> concatenate somehow both LED class device names?

It'd be different, i.e. there would be no flash or indicator in the name.
Jacek Anaszewski May 21, 2015, 12:13 p.m. UTC | #9
Hi Sakari

On 05/21/2015 12:06 PM, Sakari Ailus wrote:
[...]
>>> On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
>>> ...
>>>>>>>> --- a/drivers/leds/leds-aat1290.c
>>>>>>>> +++ b/drivers/leds/leds-aat1290.c
>>>>>>>> @@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>       led_cdev->dev->of_node = sub_node;
>>>>>>>>
>>>>>>>>       /* Create V4L2 Flash subdev. */
>>>>>>>> -    led->v4l2_flash = v4l2_flash_init(fled_cdev,
>>>>>>>> -                      &v4l2_flash_ops,
>>>>>>>> -                      &v4l2_sd_cfg);
>>>>>>>> +    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
>>>>>>>> +                      &v4l2_flash_ops, &v4l2_sd_cfg);
>>>>>>>
>>>>>>> Here the first argument should be led_cdev->dev, not dev, which is
>>>>>>> &pdev->dev, whereas led_cdev->dev is returned by
>>>>>>> device_create_with_groups (it takes dev as a parent) called from
>>>>>>> led_classdev_register.
>>>>>>
>>>>>> The reason for this is the fact that pdev->dev has its of_node
>>>>>> field initialized, which makes v4l2_async trying to match
>>>>>> subdev by parent node of a LED device, not by sub-LED related
>>>>>> DT node.
>>>>>
>>>>> If v4l2_subdev->of_node is set, then it won't be replaced with one from
>>>>> struct device. I.e. you need to provide of_node pointer only if it's
>>>>> different from dev->of_node.
>>>>>
>>>>
>>>> It will always be different since dev->of_node pointer is related
>>>> to the main DT node of LED device, whereas each LED connected to it
>>>> must be expressed in the form of sub-node, as
>>>> Documentation/devicetree/bindings/leds/common.txt DT states.
>>>
>>> You can still refer to the device's root device_node using a phandle.
>>
>> Why should I need to refer to the device's root node?
>>
>> What I meant here was that DT documentation enforces that even if
>> there is a single LED connected to the device it has to be expressed
>> as a sub-node anyway. Each LED will have to be matched by the phandle
>> to the sub-node representing it. This implies that v4l2_subdev->of_node
>> (related to sub-LED DT node) will be always different from dev->of_node
>> (related to LED controller DT node).
>
>>From driver point of view this makes no difference; it's just easier to
> parse if you don't refer to the LEDs separately. I think this is a bit
> special case; nowadays many LED flash controllers drive two LEDs.

As I understand, your stance is as follows:
- second argument to v4l2_flash_init needn't always be initialized
   because some LEDs could be referred to by the phandle to the parent
   node (e.g. flash LED and indicator under common sub-device)

If this is true, than how we would handle the situation where
there is a flash LED controller with two separate flash LEDs
and one of them is associated with indicator LED?

>>
>>> Say, if you have a LED flash controller with an indicator. It's intended to
>>> be used together with the flash LED, and the existing as3645a driver exposes
>>> it through the same sub-device. I think that'd make sense with LED class
>>> driver as well (i.e. you'd have two LED class devices but a single
>>> sub-device). Small changes to the wrapper would be needed.
>>>
>>
>> How the sub-device name should look like then? We would have to
>> concatenate somehow both LED class device names?
>
> It'd be different, i.e. there would be no flash or indicator in the name.
>

Currently there is no such a requirement too. As we discussed it few
months ago v4l2-flash sub-device name should be composed:
- for I2C devices "<LED class dev name> <i2c_adapter_id>-<i2c_addr>"
- for GPIO driven devices: <LED class dev name>
Sakari Ailus May 21, 2015, 1:14 p.m. UTC | #10
On Thu, May 21, 2015 at 02:13:12PM +0200, Jacek Anaszewski wrote:
> Hi Sakari
> 
> On 05/21/2015 12:06 PM, Sakari Ailus wrote:
> [...]
> >>>On Wed, May 20, 2015 at 03:47:25PM +0200, Jacek Anaszewski wrote:
> >>>...
> >>>>>>>>--- a/drivers/leds/leds-aat1290.c
> >>>>>>>>+++ b/drivers/leds/leds-aat1290.c
> >>>>>>>>@@ -524,9 +524,8 @@ static int aat1290_led_probe(struct
> >>>>>>>>platform_device *pdev)
> >>>>>>>>      led_cdev->dev->of_node = sub_node;
> >>>>>>>>
> >>>>>>>>      /* Create V4L2 Flash subdev. */
> >>>>>>>>-    led->v4l2_flash = v4l2_flash_init(fled_cdev,
> >>>>>>>>-                      &v4l2_flash_ops,
> >>>>>>>>-                      &v4l2_sd_cfg);
> >>>>>>>>+    led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
> >>>>>>>>+                      &v4l2_flash_ops, &v4l2_sd_cfg);
> >>>>>>>
> >>>>>>>Here the first argument should be led_cdev->dev, not dev, which is
> >>>>>>>&pdev->dev, whereas led_cdev->dev is returned by
> >>>>>>>device_create_with_groups (it takes dev as a parent) called from
> >>>>>>>led_classdev_register.
> >>>>>>
> >>>>>>The reason for this is the fact that pdev->dev has its of_node
> >>>>>>field initialized, which makes v4l2_async trying to match
> >>>>>>subdev by parent node of a LED device, not by sub-LED related
> >>>>>>DT node.
> >>>>>
> >>>>>If v4l2_subdev->of_node is set, then it won't be replaced with one from
> >>>>>struct device. I.e. you need to provide of_node pointer only if it's
> >>>>>different from dev->of_node.
> >>>>>
> >>>>
> >>>>It will always be different since dev->of_node pointer is related
> >>>>to the main DT node of LED device, whereas each LED connected to it
> >>>>must be expressed in the form of sub-node, as
> >>>>Documentation/devicetree/bindings/leds/common.txt DT states.
> >>>
> >>>You can still refer to the device's root device_node using a phandle.
> >>
> >>Why should I need to refer to the device's root node?
> >>
> >>What I meant here was that DT documentation enforces that even if
> >>there is a single LED connected to the device it has to be expressed
> >>as a sub-node anyway. Each LED will have to be matched by the phandle
> >>to the sub-node representing it. This implies that v4l2_subdev->of_node
> >>(related to sub-LED DT node) will be always different from dev->of_node
> >>(related to LED controller DT node).
> >
> >>From driver point of view this makes no difference; it's just easier to
> >parse if you don't refer to the LEDs separately. I think this is a bit
> >special case; nowadays many LED flash controllers drive two LEDs.
> 
> As I understand, your stance is as follows:
> - second argument to v4l2_flash_init needn't always be initialized
>   because some LEDs could be referred to by the phandle to the parent
>   node (e.g. flash LED and indicator under common sub-device)
> 
> If this is true, than how we would handle the situation where
> there is a flash LED controller with two separate flash LEDs
> and one of them is associated with indicator LED?

I referred to cases where there's exactly one flash LED and one indicator
LED. 

The as3645a driver already does expose them through the same sub-device, as
does the adp1653 driver. I wouldn't attempt to change this, and I think it
makes sense for other drivers that have one of each LEDs.

> >>>Say, if you have a LED flash controller with an indicator. It's intended to
> >>>be used together with the flash LED, and the existing as3645a driver exposes
> >>>it through the same sub-device. I think that'd make sense with LED class
> >>>driver as well (i.e. you'd have two LED class devices but a single
> >>>sub-device). Small changes to the wrapper would be needed.
> >>>
> >>
> >>How the sub-device name should look like then? We would have to
> >>concatenate somehow both LED class device names?
> >
> >It'd be different, i.e. there would be no flash or indicator in the name.
> >
> 
> Currently there is no such a requirement too. As we discussed it few
> months ago v4l2-flash sub-device name should be composed:
> - for I2C devices "<LED class dev name> <i2c_adapter_id>-<i2c_addr>"
> - for GPIO driven devices: <LED class dev name>

The indicator and the flash may not have the same name, so it wouldn't be a
bad idea to include that to the name of the LED device. I2C devices the
names are always different anyway due to the bus address. It's up to the
driver to set the name correctly.

Patch
diff mbox

diff --git a/drivers/leds/leds-aat1290.c b/drivers/leds/leds-aat1290.c
index c656a2d..71bf6bb 100644
--- a/drivers/leds/leds-aat1290.c
+++ b/drivers/leds/leds-aat1290.c
@@ -524,9 +524,8 @@  static int aat1290_led_probe(struct platform_device *pdev)
 	led_cdev->dev->of_node = sub_node;
 
 	/* Create V4L2 Flash subdev. */
-	led->v4l2_flash = v4l2_flash_init(fled_cdev,
-					  &v4l2_flash_ops,
-					  &v4l2_sd_cfg);
+	led->v4l2_flash = v4l2_flash_init(dev, NULL, fled_cdev,
+					  &v4l2_flash_ops, &v4l2_sd_cfg);
 	if (IS_ERR(led->v4l2_flash)) {
 		ret = PTR_ERR(led->v4l2_flash);
 		goto error_v4l2_flash_init;