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

[Jacek Anaszewski]

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/