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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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?

-- 
Best Regards,
Jacek Anaszewski