Hi Sakari,
On 11/20/2014 10:36 AM, Sakari Ailus wrote:
Hi Jacek,
Thank you for your thoughtful writing on the subject.
I am just doing my best to bring it to a successful end :)
Jacek Anaszewski wrote:
Hi Sakari,
On 09/22/2014 05:21 PM, Jacek Anaszewski wrote:
This patch adds helper functions for registering/unregistering
LED class flash devices as V4L2 subdevs. The functions should
be called from the LED subsystem device driver. In case the
support for V4L2 Flash sub-devices is disabled in the kernel
config the functions' empty versions will be used.
Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Cc: Sakari Ailus <sakari.ailus@xxxxxx>
Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
Cc: Bryan Wu <cooloney@xxxxxxxxx>
Cc: Richard Purdie <rpurdie@xxxxxxxxx>
---
drivers/media/v4l2-core/Kconfig | 11 +
drivers/media/v4l2-core/Makefile | 2 +
drivers/media/v4l2-core/v4l2-flash.c | 502
++++++++++++++++++++++++++++++++++
include/media/v4l2-flash.h | 135 +++++++++
4 files changed, 650 insertions(+)
create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
create mode 100644 include/media/v4l2-flash.h
[...]
After discussing on IRC the way of using compound controls for
v4l2-flash sub-device I started to re-implement the patch but
encountered subsequent issues, which make my inclination for
abiding by the current version of the patch (separate v4l2-flash
device for each sub-led) even stronger.
Let's list arguments for both options:
1. Single v4l2-flash sub-device for a flash device that can control
several sub-leds:
a) a flash device driver has one related i2c device
b) there exist hardware designs where some registers are
shared between sub-leds (e.g. flash timeout, flash status)
2. Separate v4l2-flash sub-device for each sub-led of a flash device
a) LED Flash class drivers create separate LED Flash device for
sub-leds (enforced by led-triggers design). This way there is
a simple one-to-one "LED Flash device" - "v4l2-flash sub-device"
relation.
b) if a single v4l2-flash sub-device was to control several
LED Flash devices then array controls would have to be
used for accessing the settings of every LED Flash device.
This poses following issues:
- the type of each V4L2 Flash control would have to be
set to the compound one (e.g. V4L2_CTRL_TYPE_U32), which in
turn would make the menu controls unavailable for querying
and displaying e.g. in qv4l2. Also the types as bitmask, button
would have to be avoided.
Good point. Currently the button control type is used for the strobe
control. For two leds we'd need an array of two button controls.
- All elements of an array control have to have the same
constraints and it would make impossible setting different min,
max values (e.g. current, timeout, external strobe) for each
sub-led. All the advantageous v4l2-ctrl mechanism related
to validating and caching controls would have to be avoided
and the user space would only get feedback in the form of
failing ioctl when the value to be set is not properly aligned
True. This is quite unpleasant to the user indeed.
- it is not possible to set only one element of the control
array and thus the settings of each sub-led would have to be
cached to avoid superfluous device register access
(functionality already secured by non-array v4l2-controls)
Agreed. But this is still a relatively minor nuisance in the implementation.
- the flash devices supporting single led could be provided
with standard non-array controls, but it would produce
cumbersome v4l2-flash code and inconsistent user space interface
From the above it looks like the option 2. has much more advantages.
The argument 1.a doesn't seem to be so vital in view of the fact
that LED subsystem already breaks it. The argument 1.b can be obviated
by caching the relevant values in the driver as it is for max77693-led.
I think that choosing option 2. would allow for avoiding much work
that is already done in v4l2-ctrls.c. Moreover it would keep the
V4L2 Flash controls maintainable with qv4l2.
Fair enough.
My remaining concerns in using two sub-devices to expose the LEDs to user
space are thus:
- Software strobe synchronisation. This one is important. There's no way to
push a button control from two sub-devices at the same time. AFAIR your
device lets the user to strobe the LEDs separately, but they are still
controlled through a single register. Either we could implement the strobe
only for the first LED, and it'd also affect the other. Alternatively we
could add one more boolean control to the second LED (while both
sub-devices would have the strobe button) to tell the strobe is
synchronous with the other sub-device.
In the proposed max77693-led DT bindings it is possible to define
whether the led iout pins are connected or not. If they are connected,
i.e. an anode of a single led is connected to both iout's, the
driver creates only one LED Flash class device, and one v4l2-flash
sub-device. All the driver logic is prepared for these two options
and configures both leds for joint iout's mode, and for separate
iout's mode it configures each led separately.
- Faults. There's usually just a single set of faults. Do we expose them for
both sub-devices, even if they are the same? I think I'd do just that.
Reading the faults on either sub-device will reset both.
For joint iout's mode there will be a single sub-device
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
