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

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

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