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

From: Jacek Anaszewski
Date: Thu Nov 20 2014 - 07:38:21 EST


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


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