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

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

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