Re: [RFC 0/2] V4L2 API for exposing flash subdevs as LED class device

Hi Bryan,

On Thursday 10 October 2013 17:07:22 Bryan Wu wrote:

On Tue, May 21, 2013 at 3:54 AM, Sakari Ailus [off-list ref] wrote:

quoted

On Tue, May 21, 2013 at 10:34:53AM +0200, Andrzej Hajda wrote:

quoted

On 12.05.2013 23:12, Sakari Ailus wrote:

quoted

On Wed, May 08, 2013 at 09:32:17AM +0200, Andrzej Hajda wrote:

quoted

On 07.05.2013 17:07, Laurent Pinchart wrote:

quoted

On Tuesday 07 May 2013 02:11:27 Kim, Milo wrote:

quoted

On Monday, May 06, 2013 6:34 PM Andrzej Hajda wrote:

quoted

This RFC proposes generic API for exposing flash subdevices via LED
framework.

Rationale

Currently there are two frameworks which are used for exposing LED
flash to user space:
- V4L2 flash controls,
- LED framework(with custom sysfs attributes).

The list below shows flash drivers in mainline kernel with initial
commit date and typical chip application (according to producer):

LED API:
    lm3642: 2012-09-12, Cameras
    lm355x: 2012-09-05, Cameras
    max8997: 2011-12-14, Cameras (?)
    lp3944: 2009-06-19, Cameras, Lights, Indicators, Toys
    pca955x: 2008-07-16, Cameras, Indicators (?)

V4L2 API:
    as3645a:  2011-05-05, Cameras
    adp1653: 2011-05-05, Cameras

V4L2 provides richest functionality, but there is often demand from
application developers to provide already established LED API. We
would like to have an unified user interface for flash devices.
Some of devices already have the LED API driver exposing limited
set of a Flash IC functionality. In order to support all required
features the LED API would have to be extended or the V4L2 API
would need to be used. However when switching from a LED to a V4L2
Flash driver existing LED API interface would need to be retained.

Proposed solution

This patch adds V4L2 helper functions to register existing V4L2
flash subdev as LED class device. After registration via
v4l2_leddev_register appropriate entry in /sys/class/leds/ is
created. During registration all V4L2 flash controls are enumerated
and corresponding attributes are added.

I have attached also patch with new max77693-led driver using
v4l2_leddev. This patch requires presence of the patch "max77693:
added device tree support":
https://patchwork.kernel.org/patch/2414351/ .

Additional features

- simple API to access all V4L2 flash controls via sysfs,
- V4L2 subdevice should not be registered by V4L2 device to use it,
- LED triggers API can be used to control the device,
- LED device is optional - it will be created only if V4L2_LEDDEV
  configuration option is enabled and the subdev driver calls
  v4l2_leddev_register.

Doubts

This RFC is a result of a uncertainty which API developers should
expose by their flash drivers. It is a try to gluing together both
APIs. I am not sure if it is the best solution, but I hope there
will be some discussion and hopefully some decisions will be taken
which way we should follow.

The LED subsystem provides similar APIs for the Camera driver.
With LED trigger event, flash and torch are enabled/disabled.
I'm not sure this is applicable for you.
Could you take a look at LED camera trigger feature?

For the camera LED trigger,
https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git
/commit/ ?h=f or-next&id=48a1d032c954b9b06c3adbf35ef4735dd70ab757

Example of camera flash driver,
https://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git
/commit/ ?h=f or-next&id=313bf0b1a0eaeaac17ea8c4b748f16e28fce8b7a

I think we should decide on one API. Implementing two APIs for a
single device is usually messy, and will result in different feature
sets (and different bugs) being implemented through each API,
depending on the driver. Interactions between the APIs are also a
pain point on the kernel side to properly synchronize calls.

I don't like having two APIs either. Especially we shouldn't have
multiple drivers implementing different APIs for the same device.

That said, I wonder if it's possible to support camera-related use
cases using the LED API: it's originally designed for quite different
devices. Even if you could handle flash strobing using the LED API, the
functionality provided by the Media controller and subdev APIs will
always be missing: device enumeration and association with the right
camera.

Is there a generic way to associate flash and camera subdevs in
current V4L2 API? The only ways I see now are:
- both belongs to the same media controller, but this is not enough if
there is more than one camera subdev in that controller,

Yes, there is. That's the group_id field in struct media_entity_desc. The
lens subdev is associated to the rest of the devices the same way.

quoted

- using media links/pads - at first sight it seems to be
overkill/abuse...

No. Links describe the flow of data, not relations between entities.

...

quoted

quoted

quoted

quoted

The LED API is too limited for torch and flash usage, but I'm
definitely open to moving flash devices to the LED API is we can
extend it in a way that it covers all the use cases.

Extending LED API IMHO seems to be quite straightforward - by adding
attributes for supported functionalities. We just need a specification
for standard flash/torch attributes.
I could prepare an RFC about it if there is a will to explore this
direction.

I'm leaning towards providing a wrapper that provides torch
functionality using V4L2 flash API unless it's really proven to be
insane. ;-) The code supporting that in an individual flash driver
should be minimal --- which is what the patchset essentially already
does.

Providing only torch functionality do not require adding new attributes
(besides the ones already present in the led_classdev), so the patch will
be much simpler.

Yes. Attributes could be added later on to the LED API to support flash
and the wrapper could be extended accordingly. My thinking is however that
the main use case is torch, not strobing flash, so it would be fulfilled
already without extensions to the LED API.

Sorry for replying so late.

I think Milo Kim did some work in our LED subsystem by add LED Flash
trigger for camera device. I agree it doesn't satisfy the usage of
V4L2 Flash API and what I'm thinking about is expanding the LED Flash
trigger driver to export a well defined sysfs interface, so user space
libv4l2 can wrap it for applications.

I've replied to your comment in the media-summit agenda mail thread, but the 
discussion belongs here, so here's a repost. Let's try to keep replies in this 
mail thread.

The biggest reason why we're not fond of sysfs-based APIs for media devices is 
that they can't provide atomicity. There's no way to set multiple parameters 
in a single operation.

We can't get rid of the sysfs LEDs API, but maybe we could have a unified 
kernel LED/flash subsystem that would provide both a sysfs-based API to ensure 
compatibility with current userspace software and an ioctl-based API (possibly 
through V4L2 controls). That way LED/flash devices would be registered with a 
single subsystem, and the corresponding drivers won't have to care about the 
API exposed to userspace. That would require a major refactoring of the in-
kernel APIs though.

-- 
Regards,

Laurent Pinchart