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

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

On 11/19/2014 10:45 AM, 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
>>
>> ...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.
>
> 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.
>
>  From the media device perspective such an approach would
> be harmful, as the indicator led could be turned on right

I intended here "wouldn't be harmful".

> 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].
>
> Regarding DT documentation:
>
> I would also swap the segments of a property name to follow the
> convention as in case of "regulator-max-microamp".
>
> Updated version:
>
> ==========================================================
>
> Optional properties for child nodes:
> - max-microamp : maximum intensity in microamperes of the LED
>           (torch LED for flash devices)
> - flash-max-microamp : maximum intensity in microamperes of the
>                 flash LED; it is mandatory if the led should
>                 support the flash mode
> - flash-timeout-microsec : timeout in microseconds after which the flash
>                     led is turned off
> - indicator-pattern : identifier of the blinking pattern for the
>                indicator led
>
> ==========================================================
>
> Regards,
> Jacek
>
> [1]
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/84643
>