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

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

On 11/18/2014 09:46 AM, Pavel Machek wrote:
> On Tue 2014-11-18 09:09:09, Jacek Anaszewski wrote:
>> Hi Pavel, Sakari,
>>
>> On 11/17/2014 03:58 PM, Sakari Ailus wrote:
>>> Hi Pavel,
>>>
>>> On Sun, Nov 16, 2014 at 08:59:28AM +0100, Pavel Machek wrote:
>>>> For device tree people: Yes, I know I'll have to create file in
>>>> documentation, but does the binding below look acceptable?
>>>>
>>>> I'll clean up driver code a bit more, remove the printks. Anything
>>>> else obviously wrong?
>>>
>>> Jacek Anaszewski is working on flash support for LED devices. I think it'd
>>> be good to sync the DT bindings for the two, as the types of devices
>>> supported by the LED API and the V4L2 flash API are quite similar.
>>>
>>> Cc Jacek.
>>
>> 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.

>> [1] http://www.spinics.net/lists/linux-leds/msg02121.html
>
> @@ -3,6 +3,17 @@ Common leds properties.
>   Optional properties for child nodes:
>   - label : The label for this LED.  If omitted, the label is
>     taken from the node name (excluding the unit address).
> +- iout-torch : Array of maximum intensities in microamperes of the
>   torch
> +	led currents in order from sub-led 0 to N-1, where N is the
>   number
> +	of torch sub-leds exposed by the device
> +- iout-flash : Array of maximum intensities in microamperes of the
>   flash
> +	led currents in order from sub-led 0 to N-1, where N is the
>   number
> +	of flash sub-leds exposed by the device
> +- iout-indicator : Array of maximum intensities in microamperes of
> +  the indicator led currents in order from sub-led 0 to N-1,
> +  where N is the number of indicator sub-leds exposed by the device
> +- flash-timeout : timeout in microseconds after which flash led
> +  is turned off
>
>   - linux,default-trigger :  This parameter, if present, is a
>       string defining the trigger assigned to the LED.  Current
>       triggers are:
> @@ -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.

> 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.

>> [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.

> Why do we need complex "flash LED class" support, and where is the
> V4L2 glue?

The rationale for unification of the LED and V4L2 Flash API
can be found in the discussion [3]. The glue is the v4l2-flash [4]
module which exposes a sub-device, that controls a LED Flash class
device with use of LED Flash class API.

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].

Best Regards,
Jacek Anaszewski

[1] http://www.spinics.net/lists/linux-leds/msg02326.html
[2] http://www.spinics.net/lists/linux-media/msg81079.html
[3] http://www.spinics.net/lists/linux-media/msg69012.html
[4] http://www.spinics.net/lists/linux-leds/msg02322.html
[5] http://www.spinics.net/lists/linux-leds/msg02323.html