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

From: Jacek Anaszewski
Date: Thu Nov 20 2014 - 04:22:17 EST

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
--
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/