[PATCH/RFC,v8,08/14] DT: Add documentation for exynos4-is 'flashes' property

Commit Message

Jacek Anaszewski Nov. 28, 2014, 9:18 a.m. UTC

This patch adds a description of 'flashes' property
to the samsung-fimc.txt.

Signed-off-by: Jacek Anaszewski <[e-mail]>
Acked-by: Kyungmin Park <[e-mail]>
Cc: Sylwester Nawrocki <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Pawel Moll <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Ian Campbell <[e-mail]>
Cc: Kumar Gala <[e-mail]>
Cc: <[e-mail]>
---
 .../devicetree/bindings/media/samsung-fimc.txt     |    7 +++++++
 1 file changed, 7 insertions(+)

Comments

Mark Rutland Nov. 28, 2014, 11:14 a.m. UTC | #1

On Fri, Nov 28, 2014 at 09:18:00AM +0000, Jacek Anaszewski wrote:
> This patch adds a description of 'flashes' property
> to the samsung-fimc.txt.
> 
> Signed-off-by: Jacek Anaszewski <[e-mail]>
> Acked-by: Kyungmin Park <[e-mail]>
> Cc: Sylwester Nawrocki <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Pawel Moll <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Ian Campbell <[e-mail]>
> Cc: Kumar Gala <[e-mail]>
> Cc: <[e-mail]>
> ---
>  .../devicetree/bindings/media/samsung-fimc.txt     |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> index 922d6f8..4b7ed03 100644
> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> @@ -40,6 +40,12 @@ should be inactive. For the "active-a" state the camera port A must be activated
>  and the port B deactivated and for the state "active-b" it should be the other
>  way around.
>  
> +Optional properties:
> +
> +- flashes - array of strings with flash led names; the name has to
> +	    be same with the related led label
> +	    (see Documentation/devicetree/bindings/leds/common.txt)
> +

Why is this not an array of phandles to the LED nodes? That's much
better than strings.

Also, I only seem to have recevied the documentation patches and none of
the code -- in future when posting RFC DT patches, please Cc for the
code too as it's useful context.

Mark.

>  The 'camera' node must include at least one 'fimc' child node.
>  
>  
> @@ -166,6 +172,7 @@ Example:
>  		clock-output-names = "cam_a_clkout", "cam_b_clkout";
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&cam_port_a_clk_active>;
> +		flashes = "max77693-flash1", "max77693-flash2";
>  		status = "okay";
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jacek Anaszewski Nov. 28, 2014, 12:09 p.m. UTC | #2

On 11/28/2014 12:14 PM, Mark Rutland wrote:
> On Fri, Nov 28, 2014 at 09:18:00AM +0000, Jacek Anaszewski wrote:
>> This patch adds a description of 'flashes' property
>> to the samsung-fimc.txt.
>>
>> Signed-off-by: Jacek Anaszewski <[e-mail]>
>> Acked-by: Kyungmin Park <[e-mail]>
>> Cc: Sylwester Nawrocki <[e-mail]>
>> Cc: Rob Herring <[e-mail]>
>> Cc: Pawel Moll <[e-mail]>
>> Cc: Mark Rutland <[e-mail]>
>> Cc: Ian Campbell <[e-mail]>
>> Cc: Kumar Gala <[e-mail]>
>> Cc: <[e-mail]>
>> ---
>>   .../devicetree/bindings/media/samsung-fimc.txt     |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> index 922d6f8..4b7ed03 100644
>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> @@ -40,6 +40,12 @@ should be inactive. For the "active-a" state the camera port A must be activated
>>   and the port B deactivated and for the state "active-b" it should be the other
>>   way around.
>>
>> +Optional properties:
>> +
>> +- flashes - array of strings with flash led names; the name has to
>> +	    be same with the related led label
>> +	    (see Documentation/devicetree/bindings/leds/common.txt)
>> +
>
> Why is this not an array of phandles to the LED nodes? That's much
> better than strings.

This is because a single flash led device can control many sub-leds,
which are represented by child nodes in the Device Tree.
Every sub-led is registered as a separate LED Flash class device
in the LED subsystem, but in fact they share the same struct device
and thus have access only to the parent's phandle. The LED Flash
class devices are wrapped by V4L2 sub-devices and register
asynchronously within a media device. Since the v4l2_subdev structure
has a 'name' field, it is convenient to initialize it with
parsed 'label' property of a child led node and match the
sub-devices in the media device basing on it.

> Also, I only seem to have recevied the documentation patches and none of
> the code -- in future when posting RFC DT patches, please Cc for the
> code too as it's useful context.

Of course, I'll keep it in mind.

Regards,
Jacek
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland Nov. 28, 2014, 12:30 p.m. UTC | #3

On Fri, Nov 28, 2014 at 12:09:14PM +0000, Jacek Anaszewski wrote:
> On 11/28/2014 12:14 PM, Mark Rutland wrote:
> > On Fri, Nov 28, 2014 at 09:18:00AM +0000, Jacek Anaszewski wrote:
> >> This patch adds a description of 'flashes' property
> >> to the samsung-fimc.txt.
> >>
> >> Signed-off-by: Jacek Anaszewski <[e-mail]>
> >> Acked-by: Kyungmin Park <[e-mail]>
> >> Cc: Sylwester Nawrocki <[e-mail]>
> >> Cc: Rob Herring <[e-mail]>
> >> Cc: Pawel Moll <[e-mail]>
> >> Cc: Mark Rutland <[e-mail]>
> >> Cc: Ian Campbell <[e-mail]>
> >> Cc: Kumar Gala <[e-mail]>
> >> Cc: <[e-mail]>
> >> ---
> >>   .../devicetree/bindings/media/samsung-fimc.txt     |    7 +++++++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >> index 922d6f8..4b7ed03 100644
> >> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >> @@ -40,6 +40,12 @@ should be inactive. For the "active-a" state the camera port A must be activated
> >>   and the port B deactivated and for the state "active-b" it should be the other
> >>   way around.
> >>
> >> +Optional properties:
> >> +
> >> +- flashes - array of strings with flash led names; the name has to
> >> +	    be same with the related led label
> >> +	    (see Documentation/devicetree/bindings/leds/common.txt)
> >> +
> >
> > Why is this not an array of phandles to the LED nodes? That's much
> > better than strings.
> 
> This is because a single flash led device can control many sub-leds,
> which are represented by child nodes in the Device Tree.
> Every sub-led is registered as a separate LED Flash class device
> in the LED subsystem, but in fact they share the same struct device
> and thus have access only to the parent's phandle.

But that's a Linux infrastrcture issue, no? You don't have to use the
node from the struct device to find the relevant phandle.

> The LED Flash
> class devices are wrapped by V4L2 sub-devices and register
> asynchronously within a media device. Since the v4l2_subdev structure
> has a 'name' field, it is convenient to initialize it with
> parsed 'label' property of a child led node and match the
> sub-devices in the media device basing on it.

While that might be convenient, I don't think it's fantastic to use that
to describe the relationship, as this leaks Linux internals (e.g. I can
refer to a name that doesn't exist in the DT but happens to be what
Linux used, and it would work). Also, are the labels guaranteed to be
globally unique?

Using phandles is much better for the binding. I appreciate that this
may require more code, but IMO it's worth that for the safety and
uniformity given by the use of phandles for referring to nodes.

> > Also, I only seem to have recevied the documentation patches and none of
> > the code -- in future when posting RFC DT patches, please Cc for the
> > code too as it's useful context.
> 
> Of course, I'll keep it in mind.

Thanks!

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jacek Anaszewski Nov. 28, 2014, 2:11 p.m. UTC | #4

On 11/28/2014 01:30 PM, Mark Rutland wrote:
> On Fri, Nov 28, 2014 at 12:09:14PM +0000, Jacek Anaszewski wrote:
>> On 11/28/2014 12:14 PM, Mark Rutland wrote:
>>> On Fri, Nov 28, 2014 at 09:18:00AM +0000, Jacek Anaszewski wrote:
>>>> This patch adds a description of 'flashes' property
>>>> to the samsung-fimc.txt.
>>>>
>>>> Signed-off-by: Jacek Anaszewski <[e-mail]>
>>>> Acked-by: Kyungmin Park <[e-mail]>
>>>> Cc: Sylwester Nawrocki <[e-mail]>
>>>> Cc: Rob Herring <[e-mail]>
>>>> Cc: Pawel Moll <[e-mail]>
>>>> Cc: Mark Rutland <[e-mail]>
>>>> Cc: Ian Campbell <[e-mail]>
>>>> Cc: Kumar Gala <[e-mail]>
>>>> Cc: <[e-mail]>
>>>> ---
>>>>    .../devicetree/bindings/media/samsung-fimc.txt     |    7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>>>> index 922d6f8..4b7ed03 100644
>>>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>>>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>>>> @@ -40,6 +40,12 @@ should be inactive. For the "active-a" state the camera port A must be activated
>>>>    and the port B deactivated and for the state "active-b" it should be the other
>>>>    way around.
>>>>
>>>> +Optional properties:
>>>> +
>>>> +- flashes - array of strings with flash led names; the name has to
>>>> +	    be same with the related led label
>>>> +	    (see Documentation/devicetree/bindings/leds/common.txt)
>>>> +
>>>
>>> Why is this not an array of phandles to the LED nodes? That's much
>>> better than strings.
>>
>> This is because a single flash led device can control many sub-leds,
>> which are represented by child nodes in the Device Tree.
>> Every sub-led is registered as a separate LED Flash class device
>> in the LED subsystem, but in fact they share the same struct device
>> and thus have access only to the parent's phandle.
>
> But that's a Linux infrastrcture issue, no? You don't have to use the
> node from the struct device to find the relevant phandle.

Right.

>> The LED Flash
>> class devices are wrapped by V4L2 sub-devices and register
>> asynchronously within a media device. Since the v4l2_subdev structure
>> has a 'name' field, it is convenient to initialize it with
>> parsed 'label' property of a child led node and match the
>> sub-devices in the media device basing on it.
>
> While that might be convenient, I don't think it's fantastic to use that
> to describe the relationship, as this leaks Linux internals (e.g. I can
> refer to a name that doesn't exist in the DT but happens to be what
> Linux used, and it would work). Also, are the labels guaranteed to be
> globally unique?

The labels are used for initializing class device name and kernel
doesn't allow to initialize two devices with same names.
This implies that labels are guaranteed to be globally unique.
Unless I am missing something.

Regards,
Jacek
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland Nov. 28, 2014, 2:38 p.m. UTC | #5

On Fri, Nov 28, 2014 at 02:11:04PM +0000, Jacek Anaszewski wrote:
> On 11/28/2014 01:30 PM, Mark Rutland wrote:
> > On Fri, Nov 28, 2014 at 12:09:14PM +0000, Jacek Anaszewski wrote:
> >> On 11/28/2014 12:14 PM, Mark Rutland wrote:
> >>> On Fri, Nov 28, 2014 at 09:18:00AM +0000, Jacek Anaszewski wrote:
> >>>> This patch adds a description of 'flashes' property
> >>>> to the samsung-fimc.txt.
> >>>>
> >>>> Signed-off-by: Jacek Anaszewski <[e-mail]>
> >>>> Acked-by: Kyungmin Park <[e-mail]>
> >>>> Cc: Sylwester Nawrocki <[e-mail]>
> >>>> Cc: Rob Herring <[e-mail]>
> >>>> Cc: Pawel Moll <[e-mail]>
> >>>> Cc: Mark Rutland <[e-mail]>
> >>>> Cc: Ian Campbell <[e-mail]>
> >>>> Cc: Kumar Gala <[e-mail]>
> >>>> Cc: <[e-mail]>
> >>>> ---
> >>>>    .../devicetree/bindings/media/samsung-fimc.txt     |    7 +++++++
> >>>>    1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>>> index 922d6f8..4b7ed03 100644
> >>>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>>> @@ -40,6 +40,12 @@ should be inactive. For the "active-a" state the camera port A must be activated
> >>>>    and the port B deactivated and for the state "active-b" it should be the other
> >>>>    way around.
> >>>>
> >>>> +Optional properties:
> >>>> +
> >>>> +- flashes - array of strings with flash led names; the name has to
> >>>> +	    be same with the related led label
> >>>> +	    (see Documentation/devicetree/bindings/leds/common.txt)
> >>>> +
> >>>
> >>> Why is this not an array of phandles to the LED nodes? That's much
> >>> better than strings.
> >>
> >> This is because a single flash led device can control many sub-leds,
> >> which are represented by child nodes in the Device Tree.
> >> Every sub-led is registered as a separate LED Flash class device
> >> in the LED subsystem, but in fact they share the same struct device
> >> and thus have access only to the parent's phandle.
> >
> > But that's a Linux infrastrcture issue, no? You don't have to use the
> > node from the struct device to find the relevant phandle.
> 
> Right.
> 
> >> The LED Flash
> >> class devices are wrapped by V4L2 sub-devices and register
> >> asynchronously within a media device. Since the v4l2_subdev structure
> >> has a 'name' field, it is convenient to initialize it with
> >> parsed 'label' property of a child led node and match the
> >> sub-devices in the media device basing on it.
> >
> > While that might be convenient, I don't think it's fantastic to use that
> > to describe the relationship, as this leaks Linux internals (e.g. I can
> > refer to a name that doesn't exist in the DT but happens to be what
> > Linux used, and it would work). Also, are the labels guaranteed to be
> > globally unique?
> 
> The labels are used for initializing class device name and kernel
> doesn't allow to initialize two devices with same names.
> This implies that labels are guaranteed to be globally unique.

On Linux, yes, but that's an implementation detail, not a property of
the binding·

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
index 922d6f8..4b7ed03 100644
--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
@@ -40,6 +40,12 @@  should be inactive. For the "active-a" state the camera port A must be activated
 and the port B deactivated and for the state "active-b" it should be the other
 way around.
 
+Optional properties:
+
+- flashes - array of strings with flash led names; the name has to
+	    be same with the related led label
+	    (see Documentation/devicetree/bindings/leds/common.txt)
+
 The 'camera' node must include at least one 'fimc' child node.
 
 
@@ -166,6 +172,7 @@  Example:
 		clock-output-names = "cam_a_clkout", "cam_b_clkout";
 		pinctrl-names = "default";
 		pinctrl-0 = <&cam_port_a_clk_active>;
+		flashes = "max77693-flash1", "max77693-flash2";
 		status = "okay";
 		#address-cells = <1>;
 		#size-cells = <1>;