diff mbox

[PATCH/RFC,v9,04/19] mfd: max77693: adjust max77693_led_platform_data

Message ID 1417622814-10845-5-git-send-email-j.anaszewski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jacek Anaszewski Dec. 3, 2014, 4:06 p.m. UTC
Add "label" array for Device Tree strings with the name of a LED device
and make flash_timeout a two element array, for caching the sub-led
related flash timeout. Added is also an array for caching pointers to the
sub-nodes representing sub-leds.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
 include/linux/mfd/max77693.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Lee Jones Dec. 9, 2014, 8:50 a.m. UTC | #1
On Wed, 03 Dec 2014, Jacek Anaszewski wrote:

> Add "label" array for Device Tree strings with the name of a LED device
> and make flash_timeout a two element array, for caching the sub-led
> related flash timeout. Added is also an array for caching pointers to the
> sub-nodes representing sub-leds.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
>  include/linux/mfd/max77693.h |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
> index f0b6585..c80ee99 100644
> --- a/include/linux/mfd/max77693.h
> +++ b/include/linux/mfd/max77693.h
> @@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
>  };
>  
>  struct max77693_led_platform_data {
> +	const char *label[2];
>  	u32 fleds[2];
>  	u32 iout_torch[2];
>  	u32 iout_flash[2];
>  	u32 trigger[2];
>  	u32 trigger_type[2];
> +	u32 flash_timeout[2];
>  	u32 num_leds;
>  	u32 boost_mode;
> -	u32 flash_timeout;
>  	u32 boost_vout;
>  	u32 low_vsys;
> +	struct device_node *sub_nodes[2];

I haven't seen anyone do this before.  Why can't you use the provided
OF functions to traverse through your tree?

>  };
>  
>  /* MAX77693 */
Jacek Anaszewski Dec. 9, 2014, 9:09 a.m. UTC | #2
On 12/09/2014 09:50 AM, Lee Jones wrote:
> On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
>
>> Add "label" array for Device Tree strings with the name of a LED device
>> and make flash_timeout a two element array, for caching the sub-led
>> related flash timeout. Added is also an array for caching pointers to the
>> sub-nodes representing sub-leds.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> ---
>>   include/linux/mfd/max77693.h |    4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
>> index f0b6585..c80ee99 100644
>> --- a/include/linux/mfd/max77693.h
>> +++ b/include/linux/mfd/max77693.h
>> @@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
>>   };
>>
>>   struct max77693_led_platform_data {
>> +	const char *label[2];
>>   	u32 fleds[2];
>>   	u32 iout_torch[2];for_each_available_child_of_node
>>   	u32 iout_flash[2];
>>   	u32 trigger[2];
>>   	u32 trigger_type[2];
>> +	u32 flash_timeout[2];
>>   	u32 num_leds;
>>   	u32 boost_mode;
>> -	u32 flash_timeout;
>>   	u32 boost_vout;
>>   	u32 low_vsys;
>> +	struct device_node *sub_nodes[2];
>
> I haven't seen anyone do this before.  Why can't you use the provided
> OF functions to traverse through your tree?

I use for_each_available_child_of_node when parsing DT node, but I
need to cache the pointer to sub-node to be able to use it later
when it needs to be passed to V4L2 sub-device which is then
asynchronously matched by the phandle to sub-node.

If it is not well seen to cache it in the platform data then
I will find different way to accomplish this.

Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Dec. 9, 2014, 10:04 a.m. UTC | #3
On Tue, 09 Dec 2014, Jacek Anaszewski wrote:

> On 12/09/2014 09:50 AM, Lee Jones wrote:
> >On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
> >
> >>Add "label" array for Device Tree strings with the name of a LED device
> >>and make flash_timeout a two element array, for caching the sub-led
> >>related flash timeout. Added is also an array for caching pointers to the
> >>sub-nodes representing sub-leds.
> >>
> >>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> >>Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>Cc: Chanwoo Choi <cw00.choi@samsung.com>
> >>Cc: Lee Jones <lee.jones@linaro.org>
> >>---
> >>  include/linux/mfd/max77693.h |    4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
> >>index f0b6585..c80ee99 100644
> >>--- a/include/linux/mfd/max77693.h
> >>+++ b/include/linux/mfd/max77693.h
> >>@@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
> >>  };
> >>
> >>  struct max77693_led_platform_data {
> >>+	const char *label[2];
> >>  	u32 fleds[2];
> >>  	u32 iout_torch[2];for_each_available_child_of_node
> >>  	u32 iout_flash[2];
> >>  	u32 trigger[2];
> >>  	u32 trigger_type[2];
> >>+	u32 flash_timeout[2];
> >>  	u32 num_leds;
> >>  	u32 boost_mode;
> >>-	u32 flash_timeout;
> >>  	u32 boost_vout;
> >>  	u32 low_vsys;
> >>+	struct device_node *sub_nodes[2];
> >
> >I haven't seen anyone do this before.  Why can't you use the provided
> >OF functions to traverse through your tree?
> 
> I use for_each_available_child_of_node when parsing DT node, but I
> need to cache the pointer to sub-node to be able to use it later
> when it needs to be passed to V4L2 sub-device which is then
> asynchronously matched by the phandle to sub-node.
> 
> If it is not well seen to cache it in the platform data then
> I will find different way to accomplish this.

I haven't seen the end-driver for this, but why can't you use that
device's of_node pointer?
Jacek Anaszewski Dec. 9, 2014, 10:25 a.m. UTC | #4
On 12/09/2014 11:04 AM, Lee Jones wrote:
> On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
>
>> On 12/09/2014 09:50 AM, Lee Jones wrote:
>>> On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
>>>
>>>> Add "label" array for Device Tree strings with the name of a LED device
>>>> and make flash_timeout a two element array, for caching the sub-led
>>>> related flash timeout. Added is also an array for caching pointers to the
>>>> sub-nodes representing sub-leds.
>>>>
>>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Cc: Lee Jones <lee.jones@linaro.org>
>>>> ---
>>>>   include/linux/mfd/max77693.h |    4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
>>>> index f0b6585..c80ee99 100644
>>>> --- a/include/linux/mfd/max77693.h
>>>> +++ b/include/linux/mfd/max77693.h
>>>> @@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
>>>>   };
>>>>
>>>>   struct max77693_led_platform_data {
>>>> +	const char *label[2];
>>>>   	u32 fleds[2];
>>>>   	u32 iout_torch[2];for_each_available_child_of_node
>>>>   	u32 iout_flash[2];
>>>>   	u32 trigger[2];
>>>>   	u32 trigger_type[2];
>>>> +	u32 flash_timeout[2];
>>>>   	u32 num_leds;
>>>>   	u32 boost_mode;
>>>> -	u32 flash_timeout;
>>>>   	u32 boost_vout;
>>>>   	u32 low_vsys;
>>>> +	struct device_node *sub_nodes[2];
>>>
>>> I haven't seen anyone do this before.  Why can't you use the provided
>>> OF functions to traverse through your tree?
>>
>> I use for_each_available_child_of_node when parsing DT node, but I
>> need to cache the pointer to sub-node to be able to use it later
>> when it needs to be passed to V4L2 sub-device which is then
>> asynchronously matched by the phandle to sub-node.
>>
>> If it is not well seen to cache it in the platform data then
>> I will find different way to accomplish this.
>
> I haven't seen the end-driver for this, but why can't you use that
> device's of_node pointer?

Maybe it is indeed a good idea. I could pass the of_node pointer
and the sub-led identifier to the V4L2 sub-device and there look
for the sub-node containing relevant identifier. The downside
would be only that for_each_available_child_of_node would
have to be called twice - in the led driver and in the V4L2 sub-device.
I think that we can live with it.

Thanks for the hint.

Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Dec. 9, 2014, 1:50 p.m. UTC | #5
On Tue, 09 Dec 2014, Jacek Anaszewski wrote:

> On 12/09/2014 11:04 AM, Lee Jones wrote:
> >On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
> >
> >>On 12/09/2014 09:50 AM, Lee Jones wrote:
> >>>On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
> >>>
> >>>>Add "label" array for Device Tree strings with the name of a LED device
> >>>>and make flash_timeout a two element array, for caching the sub-led
> >>>>related flash timeout. Added is also an array for caching pointers to the
> >>>>sub-nodes representing sub-leds.
> >>>>
> >>>>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> >>>>Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>>>Cc: Chanwoo Choi <cw00.choi@samsung.com>
> >>>>Cc: Lee Jones <lee.jones@linaro.org>
> >>>>---
> >>>>  include/linux/mfd/max77693.h |    4 +++-
> >>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
> >>>>index f0b6585..c80ee99 100644
> >>>>--- a/include/linux/mfd/max77693.h
> >>>>+++ b/include/linux/mfd/max77693.h
> >>>>@@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
> >>>>  };
> >>>>
> >>>>  struct max77693_led_platform_data {
> >>>>+	const char *label[2];
> >>>>  	u32 fleds[2];
> >>>>  	u32 iout_torch[2];for_each_available_child_of_node
> >>>>  	u32 iout_flash[2];
> >>>>  	u32 trigger[2];
> >>>>  	u32 trigger_type[2];
> >>>>+	u32 flash_timeout[2];
> >>>>  	u32 num_leds;
> >>>>  	u32 boost_mode;
> >>>>-	u32 flash_timeout;
> >>>>  	u32 boost_vout;
> >>>>  	u32 low_vsys;
> >>>>+	struct device_node *sub_nodes[2];
> >>>
> >>>I haven't seen anyone do this before.  Why can't you use the provided
> >>>OF functions to traverse through your tree?
> >>
> >>I use for_each_available_child_of_node when parsing DT node, but I
> >>need to cache the pointer to sub-node to be able to use it later
> >>when it needs to be passed to V4L2 sub-device which is then
> >>asynchronously matched by the phandle to sub-node.
> >>
> >>If it is not well seen to cache it in the platform data then
> >>I will find different way to accomplish this.
> >
> >I haven't seen the end-driver for this, but why can't you use that
> >device's of_node pointer?
> 
> Maybe it is indeed a good idea. I could pass the of_node pointer
> and the sub-led identifier to the V4L2 sub-device and there look
> for the sub-node containing relevant identifier. The downside
> would be only that for_each_available_child_of_node would
> have to be called twice - in the led driver and in the V4L2 sub-device.
> I think that we can live with it.

Are the LED and V4L2 drivers children of this MFD?  If so, you can use
the of_compatible attribute in struct mfd_cell to populate the each
child's of_node dynamically i.e. the MFD core will do that for you.
Jacek Anaszewski Dec. 9, 2014, 2:02 p.m. UTC | #6
On 12/09/2014 02:50 PM, Lee Jones wrote:
> On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
>
>> On 12/09/2014 11:04 AM, Lee Jones wrote:
>>> On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
>>>
>>>> On 12/09/2014 09:50 AM, Lee Jones wrote:
>>>>> On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
>>>>>
>>>>>> Add "label" array for Device Tree strings with the name of a LED device
>>>>>> and make flash_timeout a two element array, for caching the sub-led
>>>>>> related flash timeout. Added is also an array for caching pointers to the
>>>>>> sub-nodes representing sub-leds.
>>>>>>
>>>>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>> Cc: Lee Jones <lee.jones@linaro.org>
>>>>>> ---
>>>>>>   include/linux/mfd/max77693.h |    4 +++-
>>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
>>>>>> index f0b6585..c80ee99 100644
>>>>>> --- a/include/linux/mfd/max77693.h
>>>>>> +++ b/include/linux/mfd/max77693.h
>>>>>> @@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
>>>>>>   };
>>>>>>
>>>>>>   struct max77693_led_platform_data {
>>>>>> +	const char *label[2];
>>>>>>   	u32 fleds[2];
>>>>>>   	u32 iout_torch[2];for_each_available_child_of_node
>>>>>>   	u32 iout_flash[2];
>>>>>>   	u32 trigger[2];
>>>>>>   	u32 trigger_type[2];
>>>>>> +	u32 flash_timeout[2];
>>>>>>   	u32 num_leds;
>>>>>>   	u32 boost_mode;
>>>>>> -	u32 flash_timeout;
>>>>>>   	u32 boost_vout;
>>>>>>   	u32 low_vsys;
>>>>>> +	struct device_node *sub_nodes[2];
>>>>>
>>>>> I haven't seen anyone do this before.  Why can't you use the provided
>>>>> OF functions to traverse through your tree?
>>>>
>>>> I use for_each_available_child_of_node when parsing DT node, but I
>>>> need to cache the pointer to sub-node to be able to use it later
>>>> when it needs to be passed to V4L2 sub-device which is then
>>>> asynchronously matched by the phandle to sub-node.
>>>>
>>>> If it is not well seen to cache it in the platform data then
>>>> I will find different way to accomplish this.
>>>
>>> I haven't seen the end-driver for this, but why can't you use that
>>> device's of_node pointer?
>>
>> Maybe it is indeed a good idea. I could pass the of_node pointer
>> and the sub-led identifier to the V4L2 sub-device and there look
>> for the sub-node containing relevant identifier. The downside
>> would be only that for_each_available_child_of_node would
>> have to be called twice - in the led driver and in the V4L2 sub-device.
>> I think that we can live with it.
>
> Are the LED and V4L2 drivers children of this MFD?  If so, you can use
> the of_compatible attribute in struct mfd_cell to populate the each
> child's of_node dynamically i.e. the MFD core will do that for you.
>

V4L2 driver wraps LED driver. This way the LED device can be
controlled with use of two interfaces - LED subsystem sysfs
and V4L2 Flash. This is the aim of the whole patch set.

I've thought it over again and it seems that I will need to cache
somewhere these sub_nodes pointers. They have to be easily accessible
for the V4L2 sub-device as it can be asynchronously registered
or unregistered within V4L2 media device. Sub-devices are matched
basing on the sub-node phandle.

Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Dec. 9, 2014, 2:41 p.m. UTC | #7
On Tue, 09 Dec 2014, Jacek Anaszewski wrote:

> On 12/09/2014 02:50 PM, Lee Jones wrote:
> >On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
> >
> >>On 12/09/2014 11:04 AM, Lee Jones wrote:
> >>>On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
> >>>
> >>>>On 12/09/2014 09:50 AM, Lee Jones wrote:
> >>>>>On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
> >>>>>
> >>>>>>Add "label" array for Device Tree strings with the name of a LED device
> >>>>>>and make flash_timeout a two element array, for caching the sub-led
> >>>>>>related flash timeout. Added is also an array for caching pointers to the
> >>>>>>sub-nodes representing sub-leds.
> >>>>>>
> >>>>>>Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> >>>>>>Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>>>>>Cc: Chanwoo Choi <cw00.choi@samsung.com>
> >>>>>>Cc: Lee Jones <lee.jones@linaro.org>
> >>>>>>---
> >>>>>>  include/linux/mfd/max77693.h |    4 +++-
> >>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>>diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
> >>>>>>index f0b6585..c80ee99 100644
> >>>>>>--- a/include/linux/mfd/max77693.h
> >>>>>>+++ b/include/linux/mfd/max77693.h
> >>>>>>@@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
> >>>>>>  };
> >>>>>>
> >>>>>>  struct max77693_led_platform_data {
> >>>>>>+	const char *label[2];
> >>>>>>  	u32 fleds[2];
> >>>>>>  	u32 iout_torch[2];for_each_available_child_of_node
> >>>>>>  	u32 iout_flash[2];
> >>>>>>  	u32 trigger[2];
> >>>>>>  	u32 trigger_type[2];
> >>>>>>+	u32 flash_timeout[2];
> >>>>>>  	u32 num_leds;
> >>>>>>  	u32 boost_mode;
> >>>>>>-	u32 flash_timeout;
> >>>>>>  	u32 boost_vout;
> >>>>>>  	u32 low_vsys;
> >>>>>>+	struct device_node *sub_nodes[2];
> >>>>>
> >>>>>I haven't seen anyone do this before.  Why can't you use the provided
> >>>>>OF functions to traverse through your tree?
> >>>>
> >>>>I use for_each_available_child_of_node when parsing DT node, but I
> >>>>need to cache the pointer to sub-node to be able to use it later
> >>>>when it needs to be passed to V4L2 sub-device which is then
> >>>>asynchronously matched by the phandle to sub-node.
> >>>>
> >>>>If it is not well seen to cache it in the platform data then
> >>>>I will find different way to accomplish this.
> >>>
> >>>I haven't seen the end-driver for this, but why can't you use that
> >>>device's of_node pointer?
> >>
> >>Maybe it is indeed a good idea. I could pass the of_node pointer
> >>and the sub-led identifier to the V4L2 sub-device and there look
> >>for the sub-node containing relevant identifier. The downside
> >>would be only that for_each_available_child_of_node would
> >>have to be called twice - in the led driver and in the V4L2 sub-device.
> >>I think that we can live with it.
> >
> >Are the LED and V4L2 drivers children of this MFD?  If so, you can use
> >the of_compatible attribute in struct mfd_cell to populate the each
> >child's of_node dynamically i.e. the MFD core will do that for you.
> >
> 
> V4L2 driver wraps LED driver. This way the LED device can be
> controlled with use of two interfaces - LED subsystem sysfs
> and V4L2 Flash. This is the aim of the whole patch set.
> 
> I've thought it over again and it seems that I will need to cache
> somewhere these sub_nodes pointers. They have to be easily accessible
> for the V4L2 sub-device as it can be asynchronously registered
> or unregistered within V4L2 media device. Sub-devices are matched
> basing on the sub-node phandle.

Not quite getting this.  Can you explain this in another way please?
On 09/12/14 15:41, Lee Jones wrote:
>>>>>>>> struct max77693_led_platform_data {
>>>>>>>> > >>>>>>+	const char *label[2];
>>>>>>>> > >>>>>>  	u32 fleds[2];
>>>>>>>> > >>>>>>  	u32 iout_torch[2];for_each_available_child_of_node
>>>>>>>> > >>>>>>  	u32 iout_flash[2];
>>>>>>>> > >>>>>>  	u32 trigger[2];
>>>>>>>> > >>>>>>  	u32 trigger_type[2];
>>>>>>>> > >>>>>>+	u32 flash_timeout[2];
>>>>>>>> > >>>>>>  	u32 num_leds;
>>>>>>>> > >>>>>>  	u32 boost_mode;
>>>>>>>> > >>>>>>-	u32 flash_timeout;
>>>>>>>> > >>>>>>  	u32 boost_vout;
>>>>>>>> > >>>>>>  	u32 low_vsys;
>>>>>>>> > >>>>>>+	struct device_node *sub_nodes[2];
>>>>>>> > >>>>>
>>>>>>> > >>>>>I haven't seen anyone do this before.  Why can't you use the provided
>>>>>>> > >>>>>OF functions to traverse through your tree?
>>>>>> > >>>>
>>>>>> > >>>>I use for_each_available_child_of_node when parsing DT node, but I
>>>>>> > >>>>need to cache the pointer to sub-node to be able to use it later
>>>>>> > >>>>when it needs to be passed to V4L2 sub-device which is then
>>>>>> > >>>>asynchronously matched by the phandle to sub-node.
>>>>>> > >>>>
>>>>>> > >>>>If it is not well seen to cache it in the platform data then
>>>>>> > >>>>I will find different way to accomplish this.
>>>>> > >>>
>>>>> > >>>I haven't seen the end-driver for this, but why can't you use that
>>>>> > >>>device's of_node pointer?
>>>> > >>
>>>> > >>Maybe it is indeed a good idea. I could pass the of_node pointer
>>>> > >>and the sub-led identifier to the V4L2 sub-device and there look
>>>> > >>for the sub-node containing relevant identifier. The downside
>>>> > >>would be only that for_each_available_child_of_node would
>>>> > >>have to be called twice - in the led driver and in the V4L2 sub-device.
>>>> > >>I think that we can live with it.
>>> > >
>>> > >Are the LED and V4L2 drivers children of this MFD?  If so, you can use
>>> > >the of_compatible attribute in struct mfd_cell to populate the each
>>> > >child's of_node dynamically i.e. the MFD core will do that for you.
>>> > >
>> > 
>> > V4L2 driver wraps LED driver. This way the LED device can be
>> > controlled with use of two interfaces - LED subsystem sysfs
>> > and V4L2 Flash. This is the aim of the whole patch set.
>> > 
>> > I've thought it over again and it seems that I will need to cache
>> > somewhere these sub_nodes pointers. They have to be easily accessible
>> > for the V4L2 sub-device as it can be asynchronously registered
>> > or unregistered within V4L2 media device. Sub-devices are matched
>> > basing on the sub-node phandle.
>
> Not quite getting this.  Can you explain this in another way please?

Only the LED controller driver is a child the MFD. The LED controller
can contain multiple outputs with a physical LED attached to it. AFAICS
this binding is modelling each such an output as a the LED's controller
node child node.

I'm not sure though why storing the device node pointers is required,
rather than traversing OF tree when needed.
I guess we only need the list of the node pointer to populate struct
v4l2_async_subdev array for v4l2_async_notifier_register() call ?
Jacek Anaszewski Dec. 10, 2014, 12:38 p.m. UTC | #9
On 12/09/2014 03:41 PM, Lee Jones wrote:
> On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
>
>> On 12/09/2014 02:50 PM, Lee Jones wrote:
>>> On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
>>>
>>>> On 12/09/2014 11:04 AM, Lee Jones wrote:
>>>>> On Tue, 09 Dec 2014, Jacek Anaszewski wrote:
>>>>>
>>>>>> On 12/09/2014 09:50 AM, Lee Jones wrote:
>>>>>>> On Wed, 03 Dec 2014, Jacek Anaszewski wrote:
>>>>>>>
>>>>>>>> Add "label" array for Device Tree strings with the name of a LED device
>>>>>>>> and make flash_timeout a two element array, for caching the sub-led
>>>>>>>> related flash timeout. Added is also an array for caching pointers to the
>>>>>>>> sub-nodes representing sub-leds.
>>>>>>>>
>>>>>>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>>>>>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>>>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>>> Cc: Lee Jones <lee.jones@linaro.org>
>>>>>>>> ---
>>>>>>>>   include/linux/mfd/max77693.h |    4 +++-
>>>>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
>>>>>>>> index f0b6585..c80ee99 100644
>>>>>>>> --- a/include/linux/mfd/max77693.h
>>>>>>>> +++ b/include/linux/mfd/max77693.h
>>>>>>>> @@ -88,16 +88,18 @@ enum max77693_led_boost_mode {
>>>>>>>>   };
>>>>>>>>
>>>>>>>>   struct max77693_led_platform_data {
>>>>>>>> +	const char *label[2];
>>>>>>>>   	u32 fleds[2];
>>>>>>>>   	u32 iout_torch[2];for_each_available_child_of_node
>>>>>>>>   	u32 iout_flash[2];
>>>>>>>>   	u32 trigger[2];
>>>>>>>>   	u32 trigger_type[2];
>>>>>>>> +	u32 flash_timeout[2];
>>>>>>>>   	u32 num_leds;
>>>>>>>>   	u32 boost_mode;
>>>>>>>> -	u32 flash_timeout;
>>>>>>>>   	u32 boost_vout;
>>>>>>>>   	u32 low_vsys;
>>>>>>>> +	struct device_node *sub_nodes[2];
>>>>>>>
>>>>>>> I haven't seen anyone do this before.  Why can't you use the provided
>>>>>>> OF functions to traverse through your tree?
>>>>>>
>>>>>> I use for_each_available_child_of_node when parsing DT node, but I
>>>>>> need to cache the pointer to sub-node to be able to use it later
>>>>>> when it needs to be passed to V4L2 sub-device which is then
>>>>>> asynchronously matched by the phandle to sub-node.
>>>>>>
>>>>>> If it is not well seen to cache it in the platform data then
>>>>>> I will find different way to accomplish this.
>>>>>
>>>>> I haven't seen the end-driver for this, but why can't you use that
>>>>> device's of_node pointer?
>>>>
>>>> Maybe it is indeed a good idea. I could pass the of_node pointer
>>>> and the sub-led identifier to the V4L2 sub-device and there look
>>>> for the sub-node containing relevant identifier. The downside
>>>> would be only that for_each_available_child_of_node would
>>>> have to be called twice - in the led driver and in the V4L2 sub-device.
>>>> I think that we can live with it.
>>>
>>> Are the LED and V4L2 drivers children of this MFD?  If so, you can use
>>> the of_compatible attribute in struct mfd_cell to populate the each
>>> child's of_node dynamically i.e. the MFD core will do that for you.
>>>
>>
>> V4L2 driver wraps LED driver. This way the LED device can be
>> controlled with use of two interfaces - LED subsystem sysfs
>> and V4L2 Flash. This is the aim of the whole patch set.
>>
>> I've thought it over again and it seems that I will need to cache
>> somewhere these sub_nodes pointers. They have to be easily accessible
>> for the V4L2 sub-device as it can be asynchronously registered
>> or unregistered within V4L2 media device. Sub-devices are matched
>> basing on the sub-node phandle.
>
> Not quite getting this.  Can you explain this in another way please?
>

It turned out that it is possible to store the sub-node pointer in the
struct device returned by device_create_with_groups, while creating LED
class device. Its of_node property is uninitialized.
Regardless of it - there will be next version of this patch.

Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/mfd/max77693.h b/include/linux/mfd/max77693.h
index f0b6585..c80ee99 100644
--- a/include/linux/mfd/max77693.h
+++ b/include/linux/mfd/max77693.h
@@ -88,16 +88,18 @@  enum max77693_led_boost_mode {
 };
 
 struct max77693_led_platform_data {
+	const char *label[2];
 	u32 fleds[2];
 	u32 iout_torch[2];
 	u32 iout_flash[2];
 	u32 trigger[2];
 	u32 trigger_type[2];
+	u32 flash_timeout[2];
 	u32 num_leds;
 	u32 boost_mode;
-	u32 flash_timeout;
 	u32 boost_vout;
 	u32 low_vsys;
+	struct device_node *sub_nodes[2];
 };
 
 /* MAX77693 */