Re: [RFC] adp1653: Add device tree bindings for LED controller

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Jacek Anaszewski]

On 11/18/2014 12:32 PM, Pavel Machek wrote:
>
>>>> I've already submitted a patch [1] that updates leds common bindings.
>>>> I hasn't been merged yet, as the related LED Flash class patch [2]
>>>> still needs some indicator leds related discussion [3].
>>>>
>>>> I think this is a good moment to discuss the flash related led common
>>>> bindings.
>>>
>>> Part of problem is that adp1653 is not regarded as "LED" device by
>>> current kernel driver.
>>
>> It doesn't prevent us from keeping the flash devices related
>> DT bindings unified across kernel subsystems. The DT bindings
>> docs for the adp1653 could just provide a reference to the
>> led/common.txt bindings. In the future, when LED Flash
>> class will be merged, all the V4L2 Flash drivers might be
>> moved to the LED subsystem to gain the LED subsystem support.
>
> Yeah, that makses sense.
>
>>> @@ -19,5 +30,10 @@ Examples:
>>>   system-status {
>>>   	       label = "Status";
>>>   	       linux,default-trigger = "heartbeat";
>>> +	       iout-torch = <500 500>;
>>> +	       iout-flash = <1000 1000>;
>>> +	       iout-indicator = <100 100>;
>>> +	       flash-timeout = <1000>;
>>> +
>>> 	...
>>>   };
>>>
>>> I don't get it; system-status describes single LED, why are iout-torch
>>> (and friends) arrays of two?
>>
>> Some devices can control more than one led. The array is for such
>> purposes. The system-status should be probably renamed to
>> something more generic for both common leds and flash leds,
>> e.g. system-led.
>
> No, sorry. The Documentation/devicetree/bindings/leds/common.txt
> describes binding for _one LED_. Yes, your device can have two leds,
> so your devices should have two such blocks in the device tree... Each
> led should have its own label and default trigger, for example. And I
> guess flash-timeout be per-LED, too.

I think that a device tree binding describes a single physical device.
No matter how many sub-leds a device controls, it is still one
piece of hardware.
default-trigger property should also be an array of strings.
I agree that flash-timeout should be per led - an array, similarly
as in case of iout's.

> Would it make sense to include "-uA" and "-usec" suffixes in the dt
> property names?

I don't see this type of naming anywhere. The documentation should
contain the information about units. Nonetheless I am not a device tree
specialist, so an opinion of the relevant maintainer will be welcome.

>>> Also, at least on adp1653, these are actually two leds -- white and
>>> red. Torch and flash is white led, indicator is red led.
>>
>> Then you should define three properties:
>>
>> iout-torch = <[uA]>;
>> iout-flash = <[uA]>;
>> iout-indicator = <[uA]>;
>>
>> iout-torch and iout-flash properties would determine the current
>> for the white led in the torch and flash modes respectively and
>> the iout-indicator property would determine the current for
>> the indicator led.
>
> Yes, that would work. I have used
>
> +               max-flash-timeout-usec = <500000>;
> +               max-flash-intensity-uA     = <320000>;
> +               max-torch-intensity-uA     =  <50000>;
> +               max-indicator-intensity-uA =  <17500>;
>
> . Which is pretty similar. (Actually, maybe the longer property names
> are easier to understand for more people?) (And yes, I should probably
> separate red and white led into separate groups).

Documentation of a binding should provide sufficient explanation
for the people to understand what a property is for.
And I would stay by arrays as argued above.

>>>> [2] http://www.spinics.net/lists/linux-media/msg83100.html
>>>> [3] http://www.spinics.net/lists/linux-leds/msg02472.html
>>>
>>> What device are you using for testing? Can you cc me on future
>>> patches?
>>
>> I am using max77693 [1] and aat1290 [2]. OK, I will add you on cc.
>
> Thanks for cc and thanks for links!
>
> I see max77693 has two different "white" leds, aat1290 has one "white"
> led, and neither of them has secondary red led for indication?

You're right.

>> The v4l2-flash sub-device registers with v4l2-async API
>> in a media device. Exemplary support for v4l2-flash
>> sub-devices is added to the exynos4-is driver in the patch [5].
>
> Thanks for the links. It seems that aside from moving adp1653 driver
> to device tree, it should be moved to the LED framework, but that's a
> topic for another patch.

Like I mentioned in the previous message the LED Flash class patch
isn't in its final shape yet. Nonetheless I think that we should
agree on the leds/common.txt documentation improvements and
define DT documentation for adp1653 accordingly.

Regards,
Jacek

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Pavel Machek]

Hi!

> >>>@@ -19,5 +30,10 @@ Examples:
> >>>  system-status {
> >>>  	       label = "Status";
> >>>  	       linux,default-trigger = "heartbeat";
> >>>+	       iout-torch = <500 500>;
> >>>+	       iout-flash = <1000 1000>;
> >>>+	       iout-indicator = <100 100>;
> >>>+	       flash-timeout = <1000>;
> >>>+
> >>>	...
> >>>  };
> >>>
> >>>I don't get it; system-status describes single LED, why are iout-torch
> >>>(and friends) arrays of two?
> >>
> >>Some devices can control more than one led. The array is for such
> >>purposes. The system-status should be probably renamed to
> >>something more generic for both common leds and flash leds,
> >>e.g. system-led.
> >
> >No, sorry. The Documentation/devicetree/bindings/leds/common.txt
> >describes binding for _one LED_. Yes, your device can have two leds,
> >so your devices should have two such blocks in the device tree... Each
> >led should have its own label and default trigger, for example. And I
> >guess flash-timeout be per-LED, too.
> 
> I think that a device tree binding describes a single physical device.
> No matter how many sub-leds a device controls, it is still one
> piece of hardware.

You got this wrong, sorry.

In my case, there are three physical devices:

adp1653
	white LED
	red LED

Each LED should have an label, and probably default trigger -- default
trigger for red one should be "we are recording video" and for white
should be "this is flash for default camera".

If the hardware LED changes with one that needs different current, the
block for the adp1653 stays the same, but white LED block should be
updated with different value.

> default-trigger property should also be an array of strings.

That is not how it currently works.

> I agree that flash-timeout should be per led - an array, similarly
> as in case of iout's.

Agreed about per-led, disagreed about the array. As all the fields
would need arrays, and as LED system currently does not use arrays for
label and linux,default-trigger, I believe we should follow existing
design and model it as three devices. (It _is_ physically three devices.)

> >>The v4l2-flash sub-device registers with v4l2-async API
> >>in a media device. Exemplary support for v4l2-flash
> >>sub-devices is added to the exynos4-is driver in the patch [5].
> >
> >Thanks for the links. It seems that aside from moving adp1653 driver
> >to device tree, it should be moved to the LED framework, but that's a
> >topic for another patch.
> 
> Like I mentioned in the previous message the LED Flash class patch
> isn't in its final shape yet. Nonetheless I think that we should
> agree on the leds/common.txt documentation improvements and
> define DT documentation for adp1653 accordingly.

Agreed.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Jacek Anaszewski]

Hi Pavel,

On 11/18/2014 02:21 PM, Pavel Machek wrote:
> Hi!
>
>>>>> @@ -19,5 +30,10 @@ Examples:
>>>>>   system-status {
>>>>>   	       label = "Status";
>>>>>   	       linux,default-trigger = "heartbeat";
>>>>> +	       iout-torch = <500 500>;
>>>>> +	       iout-flash = <1000 1000>;
>>>>> +	       iout-indicator = <100 100>;
>>>>> +	       flash-timeout = <1000>;
>>>>> +
>>>>> 	...
>>>>>   };
>>>>>
>>>>> I don't get it; system-status describes single LED, why are iout-torch
>>>>> (and friends) arrays of two?
>>>>
>>>> Some devices can control more than one led. The array is for such
>>>> purposes. The system-status should be probably renamed to
>>>> something more generic for both common leds and flash leds,
>>>> e.g. system-led.
>>>
>>> No, sorry. The Documentation/devicetree/bindings/leds/common.txt
>>> describes binding for _one LED_. Yes, your device can have two leds,
>>> so your devices should have two such blocks in the device tree... Each
>>> led should have its own label and default trigger, for example. And I
>>> guess flash-timeout be per-LED, too.
>>
>> I think that a device tree binding describes a single physical device.
>> No matter how many sub-leds a device controls, it is still one
>> piece of hardware.
>
> You got this wrong, sorry.
>
> In my case, there are three physical devices:
>
> adp1653
> 	white LED
> 	red LED

You've mentioned that your white led is torch/flash and indicator
is the red led. They are probably connected to the HPLED and
ILED pins of the ADP1653 device respectively. The device is just
a regulator, that delivers electric current to the leds connected
to it. Kernel cannot directly activate leds, but has to talk
to the device through I2C bus. One I2C device can have only one
related device tree binding.

> Each LED should have an label, and probably default trigger -- default
> trigger for red one should be "we are recording video" and for white
> should be "this is flash for default camera".

default-trigger is not mandatory, the device doesn't have to have
associated led-trigger. I think that you should look at
Documentation/leds/leds-class.txt and drivers/leds/triggers for
more detailed information. In a nutshell triggers are kernel
sources of led events. You can set e.g. "heartbeat", "timer"
trigger etc.
As for now the driver belongs to the V4L2 subsystem it doesn't
support triggers. Moreover your event "we are recording a video"
should be activated by setting V4L2_CID_FLASH_INDICATOR_INTENSITY
v4l2 control followed by V4L2_FLASH_LED_MODE_TORCH. Your event
"this is flash for default camera" seems to be flash strobe,
that can be activated by setting V4L2_CID_FLASH_STROBE control.
The driver by default sets the indicator current for both actions
to the value previously set with V4L2_CID_FLASH_INDICATOR_INTENSITY.

> If the hardware LED changes with one that needs different current, the
> block for the adp1653 stays the same, but white LED block should be
> updated with different value.

I think that you are talking about sub nodes. Indeed I am leaning
towards this type of design.

>> default-trigger property should also be an array of strings.
>
> That is not how it currently works.

OK, I agree.

>
>> I agree that flash-timeout should be per led - an array, similarly
>> as in case of iout's.
>
> Agreed about per-led, disagreed about the array. As all the fields
> would need arrays, and as LED system currently does not use arrays for
> label and linux,default-trigger, I believe we should follow existing
> design and model it as three devices. (It _is_ physically three devices.)

Right, I missed that the leds/common.txt describes child node.

I propose following modifications to the binding:

Optional properties for child nodes:
- iout-mode-led : 	maximum intensity in microamperes of the LED
		  	(torch LED for flash devices)
- iout-mode-flash : 	initial intensity in microamperes of the
			flash LED; it is required to enable support
			for the flash led
- iout-mode-indicator : initial intensity in microamperes of the
			indicator LED; it is required to enable support
			for the indicator led
- max-iout-mode-led : 	maximum intensity in microamperes of the LED
		  	(torch LED for flash devices)
- max-iout-mode-flash : maximum intensity in microamperes of the
			flash LED
- max-iout-mode-indicator : maximum intensity in microamperes of the
			indicator LED
- flash-timeout :	timeout in microseconds after which flash
			led is turned off

system-status {
         label = "max77693_1";
         iout-mode-led = <500>;
         max-iout-mode-led = <500>;
         ...
};

camera-flash1 {
         label = "max77693_2";
         iout-mode-led = <500>;
         iout-mode-flash = <1000>;
	iout-mode-indicator = <100>;
         max-iout-mode-led = <500>;
         max-iout-mode-flash = <1000>;
         max-iout-mode-indicator = <100>;
         flash-timeout = <1000>;
         ...
};


I propose to avoid name "torch", as for ordinary leds it would
be misleading.

Regards,
Jacek

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Pavel Machek]

Hi!

> >If the hardware LED changes with one that needs different current, the
> >block for the adp1653 stays the same, but white LED block should be
> >updated with different value.
> 
> I think that you are talking about sub nodes. Indeed I am leaning
> towards this type of design.

I think I am :-).

> >>I agree that flash-timeout should be per led - an array, similarly
> >>as in case of iout's.
> >
> >Agreed about per-led, disagreed about the array. As all the fields
> >would need arrays, and as LED system currently does not use arrays for
> >label and linux,default-trigger, I believe we should follow existing
> >design and model it as three devices. (It _is_ physically three devices.)
> 
> Right, I missed that the leds/common.txt describes child node.
> 
> I propose following modifications to the binding:
> 
> Optional properties for child nodes:
> - iout-mode-led : 	maximum intensity in microamperes of the LED
> 		  	(torch LED for flash devices)
> - iout-mode-flash : 	initial intensity in microamperes of the
> 			flash LED; it is required to enable support
> 			for the flash led
> - iout-mode-indicator : initial intensity in microamperes of the
> 			indicator LED; it is required to enable support
> 			for the indicator led
> - max-iout-mode-led : 	maximum intensity in microamperes of the LED
> 		  	(torch LED for flash devices)
> - max-iout-mode-flash : maximum intensity in microamperes of the
> 			flash LED
> - max-iout-mode-indicator : maximum intensity in microamperes of the
> 			indicator LED
> - flash-timeout :	timeout in microseconds after which flash
> 			led is turned off

Ok, I took a look, and "iout" is notation I understand, but people may
have trouble with and I don't see it used anywhere else.

Also... do we need both "current" and "max-current" properties?

But regulators already have "regulator-max-microamp" property. So what
about:

max-microamp : 	maximum intensity in microamperes of the LED
 		  	(torch LED for flash devices)
max-flash-microamp : 	initial intensity in microamperes of the
 			flash LED; it is required to enable support
 			for the flash led
flash-timeout-microseconds : timeout in microseconds after which flash
 			led is turned off

If you had indicator on the same led, I guess

indicator-microamp : recommended intensity in microamperes of the LED
		         for indication

...would do?

> I propose to avoid name "torch", as for ordinary leds it would
> be misleading.

Agreed.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Jacek Anaszewski]

Hi Pavel, Sakari,

On 11/18/2014 05:51 PM, Pavel Machek wrote:
> Hi!
>
>>> If the hardware LED changes with one that needs different current, the
>>> block for the adp1653 stays the same, but white LED block should be
>>> updated with different value.
>>
>> I think that you are talking about sub nodes. Indeed I am leaning
>> towards this type of design.
>
> I think I am :-).
>
>>>> I agree that flash-timeout should be per led - an array, similarly
>>>> as in case of iout's.
>>>
>>> Agreed about per-led, disagreed about the array. As all the fields
>>> would need arrays, and as LED system currently does not use arrays for
>>> label and linux,default-trigger, I believe we should follow existing
>>> design and model it as three devices. (It _is_ physically three devices.)
>>
>> Right, I missed that the leds/common.txt describes child node.
>>
>> I propose following modifications to the binding:
>>
>> Optional properties for child nodes:
>> - iout-mode-led : 	maximum intensity in microamperes of the LED
>> 		  	(torch LED for flash devices)
>> - iout-mode-flash : 	initial intensity in microamperes of the
>> 			flash LED; it is required to enable support
>> 			for the flash led
>> - iout-mode-indicator : initial intensity in microamperes of the
>> 			indicator LED; it is required to enable support
>> 			for the indicator led
>> - max-iout-mode-led : 	maximum intensity in microamperes of the LED
>> 		  	(torch LED for flash devices)
>> - max-iout-mode-flash : maximum intensity in microamperes of the
>> 			flash LED
>> - max-iout-mode-indicator : maximum intensity in microamperes of the
>> 			indicator LED
>> - flash-timeout :	timeout in microseconds after which flash
>> 			led is turned off
>
> Ok, I took a look, and "iout" is notation I understand, but people may
> have trouble with and I don't see it used anywhere else.
>
> Also... do we need both "current" and "max-current" properties?
>
> But regulators already have "regulator-max-microamp" property. So what
> about:
>
> max-microamp : 	maximum intensity in microamperes of the LED
>   		  	(torch LED for flash devices)
> max-flash-microamp : 	initial intensity in microamperes of the
>   			flash LED; it is required to enable support
>   			for the flash led
> flash-timeout-microseconds : timeout in microseconds after which flash
>   			led is turned off
>
> If you had indicator on the same led, I guess
>
> indicator-microamp : recommended intensity in microamperes of the LED
> 		         for indication
>
> ...would do?


Ongoing discussion allowed me for taking a look at the indicator issue
from different perspective. This is also vital for the issue of
whether a v4l2-flash sub-device should be created per device or
per sub-led [1].

Currently each sub-led is represented as a separate device tree
sub node and the led drivers create separate LED class device for the
sub nodes. What this implies is that indicator led also must be
represented by the separate LED class device.

This is contrary to the way how V4L2 Flash API approaches this issue,
as it considers a flash device as a regulator chip driven through
a bus. The API allows to set the led in torch or flash mode and
implicitly assumes that there can be additional indicator led
supported, which can't be turned on separately, but the drivers apply
the indicator current to the indicator led when the torch or flash led
is activated.

I propose to create separate v4l2-flash device for the indicator led,
and treat it as a regular sub-led similarly like it is done in the
LED subsystem. LED Flash class driver would only add a flag
LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device
would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it.
There could ba also additional control added:
V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature
supported by some LED class drivers.

 From the media device perspective such an approach would
be harmful, as the indicator led could be turned on right
before strobing the flash or turning the torch on, by
separate calls to different v4l2-flash sub-devices.

The design described above would allow for avoiding issues I touched
in the message [1].

Regarding DT documentation:

I would also swap the segments of a property name to follow the 
convention as in case of "regulator-max-microamp".

Updated version:

==========================================================

Optional properties for child nodes:
- max-microamp : maximum intensity in microamperes of the LED
		 (torch LED for flash devices)
- flash-max-microamp : maximum intensity in microamperes of the
		       flash LED; it is mandatory if the led should
		       support the flash mode
- flash-timeout-microsec : timeout in microseconds after which the flash
		           led is turned off
- indicator-pattern : identifier of the blinking pattern for the
		      indicator led

==========================================================

Regards,
Jacek

[1] 
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/84643
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Sakari Ailus]

Hi Jacek and Pavel,

Jacek Anaszewski wrote:
> Hi Pavel, Sakari,
> 
> On 11/18/2014 05:51 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> If the hardware LED changes with one that needs different current, the
>>>> block for the adp1653 stays the same, but white LED block should be
>>>> updated with different value.
>>>
>>> I think that you are talking about sub nodes. Indeed I am leaning
>>> towards this type of design.
>>
>> I think I am :-).
>>
>>>>> I agree that flash-timeout should be per led - an array, similarly
>>>>> as in case of iout's.
>>>>
>>>> Agreed about per-led, disagreed about the array. As all the fields
>>>> would need arrays, and as LED system currently does not use arrays for
>>>> label and linux,default-trigger, I believe we should follow existing
>>>> design and model it as three devices. (It _is_ physically three
>>>> devices.)
>>>
>>> Right, I missed that the leds/common.txt describes child node.
>>>
>>> I propose following modifications to the binding:
>>>
>>> Optional properties for child nodes:
>>> - iout-mode-led :     maximum intensity in microamperes of the LED
>>>               (torch LED for flash devices)
>>> - iout-mode-flash :     initial intensity in microamperes of the
>>>             flash LED; it is required to enable support
>>>             for the flash led
>>> - iout-mode-indicator : initial intensity in microamperes of the
>>>             indicator LED; it is required to enable support
>>>             for the indicator led
>>> - max-iout-mode-led :     maximum intensity in microamperes of the LED
>>>               (torch LED for flash devices)
>>> - max-iout-mode-flash : maximum intensity in microamperes of the
>>>             flash LED
>>> - max-iout-mode-indicator : maximum intensity in microamperes of the
>>>             indicator LED
>>> - flash-timeout :    timeout in microseconds after which flash
>>>             led is turned off
>>
>> Ok, I took a look, and "iout" is notation I understand, but people may
>> have trouble with and I don't see it used anywhere else.
>>
>> Also... do we need both "current" and "max-current" properties?
>>
>> But regulators already have "regulator-max-microamp" property. So what
>> about:
>>
>> max-microamp :     maximum intensity in microamperes of the LED
>>                 (torch LED for flash devices)
>> max-flash-microamp :     initial intensity in microamperes of the
>>               flash LED; it is required to enable support
>>               for the flash led
>> flash-timeout-microseconds : timeout in microseconds after which flash
>>               led is turned off
>>
>> If you had indicator on the same led, I guess
>>
>> indicator-microamp : recommended intensity in microamperes of the LED
>>                  for indication

The value for the indicator is maximum as well, not just a recommendation.

>>
>> ...would do?
> 
> 
> Ongoing discussion allowed me for taking a look at the indicator issue
> from different perspective. This is also vital for the issue of
> whether a v4l2-flash sub-device should be created per device or
> per sub-led [1].
> 
> Currently each sub-led is represented as a separate device tree
> sub node and the led drivers create separate LED class device for the
> sub nodes. What this implies is that indicator led also must be
> represented by the separate LED class device.
> 
> This is contrary to the way how V4L2 Flash API approaches this issue,
> as it considers a flash device as a regulator chip driven through
> a bus. The API allows to set the led in torch or flash mode and
> implicitly assumes that there can be additional indicator led
> supported, which can't be turned on separately, but the drivers apply
> the indicator current to the indicator led when the torch or flash led
> is activated.

The indicator is independent of the flash LED in V4L2 flash API. At
least that's how it should be, and in adp1653 the two are independent,
but the as3645a can't use indicator with the flash AFAIR.

> I propose to create separate v4l2-flash device for the indicator led,
> and treat it as a regular sub-led similarly like it is done in the
> LED subsystem. LED Flash class driver would only add a flag
> LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device
> would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it.
> There could ba also additional control added:
> V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature
> supported by some LED class drivers.

Interesting idea.

The flash controller is still a single I2C device with common set of
faults, for instance. Some devices refuse to work again in case of
faults until they are cleared (= read).

Could the indicator pattern control be present in the same sub-device?

Are there any flash LED controllers that support such functionality?

> From the media device perspective such an approach would
> be harmful, as the indicator led could be turned on right
> before strobing the flash or turning the torch on, by
> separate calls to different v4l2-flash sub-devices.
> 
> The design described above would allow for avoiding issues I touched
> in the message [1].

Let me reply this separately. Feel free to ping me if I obviously appear
to miss something.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Jacek Anaszewski]

Hi Pavel, Sakari,

On 11/19/2014 06:53 PM, Sakari Ailus wrote:
> Hi Jacek and Pavel,
>
> Jacek Anaszewski wrote:
>> Hi Pavel, Sakari,
>>
>> On 11/18/2014 05:51 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> If the hardware LED changes with one that needs different current, the
>>>>> block for the adp1653 stays the same, but white LED block should be
>>>>> updated with different value.
>>>>
>>>> I think that you are talking about sub nodes. Indeed I am leaning
>>>> towards this type of design.
>>>
>>> I think I am :-).
>>>
>>>>>> I agree that flash-timeout should be per led - an array, similarly
>>>>>> as in case of iout's.
>>>>>
>>>>> Agreed about per-led, disagreed about the array. As all the fields
>>>>> would need arrays, and as LED system currently does not use arrays for
>>>>> label and linux,default-trigger, I believe we should follow existing
>>>>> design and model it as three devices. (It _is_ physically three
>>>>> devices.)
>>>>
>>>> Right, I missed that the leds/common.txt describes child node.
>>>>
>>>> I propose following modifications to the binding:
>>>>
>>>> Optional properties for child nodes:
>>>> - iout-mode-led :     maximum intensity in microamperes of the LED
>>>>                (torch LED for flash devices)
>>>> - iout-mode-flash :     initial intensity in microamperes of the
>>>>              flash LED; it is required to enable support
>>>>              for the flash led
>>>> - iout-mode-indicator : initial intensity in microamperes of the
>>>>              indicator LED; it is required to enable support
>>>>              for the indicator led
>>>> - max-iout-mode-led :     maximum intensity in microamperes of the LED
>>>>                (torch LED for flash devices)
>>>> - max-iout-mode-flash : maximum intensity in microamperes of the
>>>>              flash LED
>>>> - max-iout-mode-indicator : maximum intensity in microamperes of the
>>>>              indicator LED
>>>> - flash-timeout :    timeout in microseconds after which flash
>>>>              led is turned off
>>>
>>> Ok, I took a look, and "iout" is notation I understand, but people may
>>> have trouble with and I don't see it used anywhere else.
>>>
>>> Also... do we need both "current" and "max-current" properties?
>>>
>>> But regulators already have "regulator-max-microamp" property. So what
>>> about:
>>>
>>> max-microamp :     maximum intensity in microamperes of the LED
>>>                  (torch LED for flash devices)
>>> max-flash-microamp :     initial intensity in microamperes of the
>>>                flash LED; it is required to enable support
>>>                for the flash led
>>> flash-timeout-microseconds : timeout in microseconds after which flash
>>>                led is turned off
>>>
>>> If you had indicator on the same led, I guess
>>>
>>> indicator-microamp : recommended intensity in microamperes of the LED
>>>                   for indication
>
> The value for the indicator is maximum as well, not just a recommendation.
>
>>>
>>> ...would do?
>>
>>
>> Ongoing discussion allowed me for taking a look at the indicator issue
>> from different perspective. This is also vital for the issue of
>> whether a v4l2-flash sub-device should be created per device or
>> per sub-led [1].
>>
>> Currently each sub-led is represented as a separate device tree
>> sub node and the led drivers create separate LED class device for the
>> sub nodes. What this implies is that indicator led also must be
>> represented by the separate LED class device.
>>
>> This is contrary to the way how V4L2 Flash API approaches this issue,
>> as it considers a flash device as a regulator chip driven through
>> a bus. The API allows to set the led in torch or flash mode and
>> implicitly assumes that there can be additional indicator led
>> supported, which can't be turned on separately, but the drivers apply
>> the indicator current to the indicator led when the torch or flash led
>> is activated.
>
> The indicator is independent of the flash LED in V4L2 flash API. At
> least that's how it should be, and in adp1653 the two are independent,
> but the as3645a can't use indicator with the flash AFAIR.

Right.

>> I propose to create separate v4l2-flash device for the indicator led,
>> and treat it as a regular sub-led similarly like it is done in the
>> LED subsystem. LED Flash class driver would only add a flag
>> LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device
>> would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it.
>> There could ba also additional control added:
>> V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature
>> supported by some LED class drivers.
>
> Interesting idea.
>
> The flash controller is still a single I2C device with common set of
> faults, for instance. Some devices refuse to work again in case of
> faults until they are cleared (= read).

The V4L2_CID_FLASH_FAULT control should be also supported by the
indicator v4l2-flash sub-device then.

> Could the indicator pattern control be present in the same sub-device?

Yes, this was my intention. To conclude - the indicator v4l2-flash 
sub-device should support up to three controls:
- V4L2_CID_FLASH_TORCH_INTENSITY
- V4L2_CID_FLASH_FAULT
- V4L2_CID_FLASH_INDICATOR_PATTERN (if supported by the LED Flash
				    class driver)

V4L2_CID_FLASH_INDICATOR_PATTERN would be a menu control with
custom menu items.

> Are there any flash LED controllers that support such functionality?

There is: lm3556. LED subsystem leds-lm355x.c driver supports it.
The driver exposes dedicated sysfs attribute for setting the blinking
pattern (four possible options).

>>  From the media device perspective such an approach would
>> be harmful, as the indicator led could be turned on right
>> before strobing the flash or turning the torch on, by
>> separate calls to different v4l2-flash sub-devices.
>>
>> The design described above would allow for avoiding issues I touched
>> in the message [1].
>
> Let me reply this separately. Feel free to ping me if I obviously appear
> to miss something.
>

Best Regards,
Jacek Anaszewski

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Pavel Machek]

Hi!

> I would also swap the segments of a property name to follow the convention
> as in case of "regulator-max-microamp".
> 
> Updated version:
> 
> ==========================================================
> 
> Optional properties for child nodes:
> - max-microamp : maximum intensity in microamperes of the LED
> 		 (torch LED for flash devices)
> - flash-max-microamp : maximum intensity in microamperes of the
> 		       flash LED; it is mandatory if the led should
> 		       support the flash mode
> - flash-timeout-microsec : timeout in microseconds after which the flash
> 		           led is turned off

Works for me. Do you want to submit a patch or should I do it?

> - indicator-pattern : identifier of the blinking pattern for the
> 		      indicator led
> 

This would need a bit more documentation, no?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Pavel Machek]

Hi!

> >> But regulators already have "regulator-max-microamp" property. So what
> >> about:
> >>
> >> max-microamp :     maximum intensity in microamperes of the LED
> >>                 (torch LED for flash devices)
> >> max-flash-microamp :     initial intensity in microamperes of the
> >>               flash LED; it is required to enable support
> >>               for the flash led
> >> flash-timeout-microseconds : timeout in microseconds after which flash
> >>               led is turned off
> >>
> >> If you had indicator on the same led, I guess
> >>
> >> indicator-microamp : recommended intensity in microamperes of the LED
> >>                  for indication
> 
> The value for the indicator is maximum as well, not just a
> recommendation.

Actually, no.

This is all for one LED, if you want to use it as a flash, torch and
indicator. You already know the maximum value with max-microamp.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Jacek Anaszewski]

On 11/19/2014 10:45 AM, Jacek Anaszewski wrote:
> Hi Pavel, Sakari,
>
> On 11/18/2014 05:51 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> If the hardware LED changes with one that needs different current, the
>>>> block for the adp1653 stays the same, but white LED block should be
>>>> updated with different value.
>>>
>>> I think that you are talking about sub nodes. Indeed I am leaning
>>> towards this type of design.
>>
>> I think I am :-).
>>
>>>>> I agree that flash-timeout should be per led - an array, similarly
>>>>> as in case of iout's.
>>>>
>>>> Agreed about per-led, disagreed about the array. As all the fields
>>>> would need arrays, and as LED system currently does not use arrays for
>>>> label and linux,default-trigger, I believe we should follow existing
>>>> design and model it as three devices. (It _is_ physically three
>>>> devices.)
>>>
>>> Right, I missed that the leds/common.txt describes child node.
>>>
>>> I propose following modifications to the binding:
>>>
>>> Optional properties for child nodes:
>>> - iout-mode-led :     maximum intensity in microamperes of the LED
>>>               (torch LED for flash devices)
>>> - iout-mode-flash :     initial intensity in microamperes of the
>>>             flash LED; it is required to enable support
>>>             for the flash led
>>> - iout-mode-indicator : initial intensity in microamperes of the
>>>             indicator LED; it is required to enable support
>>>             for the indicator led
>>> - max-iout-mode-led :     maximum intensity in microamperes of the LED
>>>               (torch LED for flash devices)
>>> - max-iout-mode-flash : maximum intensity in microamperes of the
>>>             flash LED
>>> - max-iout-mode-indicator : maximum intensity in microamperes of the
>>>             indicator LED
>>> - flash-timeout :    timeout in microseconds after which flash
>>>             led is turned off
>>
>> Ok, I took a look, and "iout" is notation I understand, but people may
>> have trouble with and I don't see it used anywhere else.
>>
>> Also... do we need both "current" and "max-current" properties?
>>
>> But regulators already have "regulator-max-microamp" property. So what
>> about:
>>
>> max-microamp :     maximum intensity in microamperes of the LED
>>                 (torch LED for flash devices)
>> max-flash-microamp :     initial intensity in microamperes of the
>>               flash LED; it is required to enable support
>>               for the flash led
>> flash-timeout-microseconds : timeout in microseconds after which flash
>>               led is turned off
>>
>> If you had indicator on the same led, I guess
>>
>> indicator-microamp : recommended intensity in microamperes of the LED
>>                  for indication
>>
>> ...would do?
>
>
> Ongoing discussion allowed me for taking a look at the indicator issue
> from different perspective. This is also vital for the issue of
> whether a v4l2-flash sub-device should be created per device or
> per sub-led [1].
>
> Currently each sub-led is represented as a separate device tree
> sub node and the led drivers create separate LED class device for the
> sub nodes. What this implies is that indicator led also must be
> represented by the separate LED class device.
>
> This is contrary to the way how V4L2 Flash API approaches this issue,
> as it considers a flash device as a regulator chip driven through
> a bus. The API allows to set the led in torch or flash mode and
> implicitly assumes that there can be additional indicator led
> supported, which can't be turned on separately, but the drivers apply
> the indicator current to the indicator led when the torch or flash led
> is activated.
>
> I propose to create separate v4l2-flash device for the indicator led,
> and treat it as a regular sub-led similarly like it is done in the
> LED subsystem. LED Flash class driver would only add a flag
> LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device
> would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it.
> There could ba also additional control added:
> V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature
> supported by some LED class drivers.
>
>  From the media device perspective such an approach would
> be harmful, as the indicator led could be turned on right

I intended here "wouldn't be harmful".

> before strobing the flash or turning the torch on, by
> separate calls to different v4l2-flash sub-devices.
>
> The design described above would allow for avoiding issues I touched
> in the message [1].
>
> Regarding DT documentation:
>
> I would also swap the segments of a property name to follow the
> convention as in case of "regulator-max-microamp".
>
> Updated version:
>
> ==========================================================
>
> Optional properties for child nodes:
> - max-microamp : maximum intensity in microamperes of the LED
>           (torch LED for flash devices)
> - flash-max-microamp : maximum intensity in microamperes of the
>                 flash LED; it is mandatory if the led should
>                 support the flash mode
> - flash-timeout-microsec : timeout in microseconds after which the flash
>                     led is turned off
> - indicator-pattern : identifier of the blinking pattern for the
>                indicator led
>
> ==========================================================
>
> Regards,
> Jacek
>
> [1]
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/84643
>

Re: [RFC] adp1653: Add device tree bindings for LED controller

[Jacek Anaszewski]

Hi Pavel,

On 11/20/2014 01:12 PM, Pavel Machek wrote:
> Hi!
>
>> I would also swap the segments of a property name to follow the convention
>> as in case of "regulator-max-microamp".
>>
>> Updated version:
>>
>> ==========================================================
>>
>> Optional properties for child nodes:
>> - max-microamp : maximum intensity in microamperes of the LED
>> 		 (torch LED for flash devices)
>> - flash-max-microamp : maximum intensity in microamperes of the
>> 		       flash LED; it is mandatory if the led should
>> 		       support the flash mode
>> - flash-timeout-microsec : timeout in microseconds after which the flash
>> 		           led is turned off
>
> Works for me. Do you want to submit a patch or should I do it?

You can submit a patch for leds/common.txt and a separate patch for the
adp1653 with a reference to the leds/common.txt for the child nodes.

>
>> - indicator-pattern : identifier of the blinking pattern for the
>> 		      indicator led
>>
>
> This would need a bit more documentation, no?

- indicator-pattern : identifier of the blinking pattern for the
  		      indicator led; valid identifiers should be
		      defined in the documentation of the parent
		      node.

I wouldn't go for pre-defined identifiers as the pattern
can be a combination of various settings like ramp-up, ramp-down,
pulse time etc. Drivers should expose only few combinations of
these settings in my opinion, like e.g. leds-lm355x.c does.

Regards,
Jacek