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

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

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