Message-id: <546B40FA.2070409@samsung.com>
Date: Tue, 18 Nov 2014 13:52:10 +0100
From: Jacek Anaszewski <j.anaszewski@samsung.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804
 Thunderbird/17.0.8
MIME-version: 1.0
To: Pavel Machek <pavel@ucw.cz>
Cc: Sakari Ailus <sakari.ailus@iki.fi>, pali.rohar@gmail.com, sre@debian.org,
        sre@ring0.de, kernel list <linux-kernel@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        linux-omap@vger.kernel.org, tony@atomide.com, khilman@kernel.org,
        aaro.koskinen@iki.fi, freemangordon@abv.bg, bcousson@baylibre.com,
        robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com,
        ijc+devicetree@hellion.org.uk, galak@codeaurora.org,
        devicetree@vger.kernel.org, linux-media@vger.kernel.org,
        Linux LED Subsystem <linux-leds@vger.kernel.org>
Subject: Re: [RFC] adp1653: Add device tree bindings for LED controller
References: <20141116075928.GA9763@amd>
 <20141117145857.GO8907@valkosipuli.retiisi.org.uk>
 <546AFEA5.9020000@samsung.com> <20141118084603.GC4059@amd>
 <546B19C8.2090008@samsung.com> <20141118113256.GA10022@amd>
In-reply-to: <20141118113256.GA10022@amd>
Content-type: text/plain; charset=ISO-8859-1; format=flowed
Content-transfer-encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/