Re: [PATCH/RFC v10 15/19] media: Add registration helpers for V4L2 flash sub-devices
Hi Sakari,
Thanks for the review.
On 02/05/2015 06:59 PM, Sakari Ailus wrote:
Hi Jacek,
Thank you for your continuous efforts on this! I think this ended up being
more complicated than I originally anticipated.
On Fri, Jan 09, 2015 at 04:23:05PM +0100, Jacek Anaszewski wrote:
This patch adds helper functions for registering/unregistering
LED Flash class devices as V4L2 sub-devices. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/v4l2-core/Kconfig | 11 +
drivers/media/v4l2-core/Makefile |2 +
drivers/media/v4l2-core/v4l2-flash.c | 581 ++
include/media/v4l2-flash.h | 139
4 files changed, 733 insertions(+)
create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
create mode 100644 include/media/v4l2-flash.h
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index ba7e21a..f034f1a 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -44,6 +44,17 @@ config V4L2_MEM2MEM_DEV
tristate
depends on VIDEOBUF2_CORE
+# Used by LED subsystem flash drivers
+config V4L2_FLASH_LED_CLASS
+ tristate Enable support for Flash sub-devices
+ depends on VIDEO_V4L2_SUBDEV_API
+ depends on LEDS_CLASS_FLASH
+ ---help---
+ Say Y here to enable support for Flash sub-devices, which allow
+ to control LED class devices with use of V4L2 Flash controls.
+
+ When in doubt, say N.
+
# Used by drivers that need Videobuf modules
config VIDEOBUF_GEN
tristate
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 63d29f2..44e858c 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o
+
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
diff --git a/drivers/media/v4l2-core/v4l2-flash.c
b/drivers/media/v4l2-core/v4l2-flash.c
new file mode 100644
index 000..3fd6a08
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-flash.c
@@ -0,0 +1,581 @@
+/*
+ * V4L2 Flash LED sub-device registration helpers.
+ *
+ * Copyright (C) 2015 Samsung Electronics Co., Ltd
+ * Author: Jacek Anaszewski j.anaszew...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/led-class-flash.h
+#include linux/module.h
+#include linux/mutex.h
+#include linux/slab.h
+#include linux/types.h
+#include media/v4l2-flash.h
+
+#define has_flash_op(v4l2_flash, op) \
+ (v4l2_flash v4l2_flash-ops-op)
+
+#define call_flash_op(v4l2_flash, op, args...) \
+ (has_flash_op(v4l2_flash, op) ? \
+ v4l2_flash-ops-op(args) : \
Wouldn't you need __VA_ARGS__ here to deliver the variable argument to the
callee? Do you need variable arguments for the current flash ops?
Indeed, it would allow to avoid the need for passing v4l2_flash
argument twice. Nonetheless, as currently we have only one op, the macro
can have fixed number of arguments.
+ -EINVAL)
+
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
I'd drop inline and let the compiler decide. Same below.
Yes, this is a residue from the previous versions when the function
had single line probably.
+ struct v4l2_ctrl **ctrls,
+ enum ctrl_init_data_id cdata_id,
+ s32 intensity)
+{
+ struct v4l2_ctrl *ctrl = ctrls[cdata_id];
+ s64 __intensity = intensity - ctrl-minimum;
+
+ do_div(__intensity, ctrl-step);
+
+ /*
+* Indicator leds, unlike torch leds, are turned on/off basing on
+* the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+* Therefore it must be possible to set it to 0 level which in
+* the LED subsystem reflects LED_OFF state.
+*/
+ if (cdata_id != INDICATOR_INTENSITY)
+ ++__intensity;
+
+ return __intensity;
+}
+
+static inline s32 v4l2_flash_led_brightness_to_intensity(
+ struct v4l2_ctrl **ctrls,
+ enum ctrl_init_data_id cdata_id,
Could you
Re: [PATCH/RFC v10 15/19] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
Thank you for your continuous efforts on this! I think this ended up being
more complicated than I originally anticipated.
On Fri, Jan 09, 2015 at 04:23:05PM +0100, Jacek Anaszewski wrote:
This patch adds helper functions for registering/unregistering
LED Flash class devices as V4L2 sub-devices. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/v4l2-core/Kconfig | 11 +
drivers/media/v4l2-core/Makefile |2 +
drivers/media/v4l2-core/v4l2-flash.c | 581
++
include/media/v4l2-flash.h | 139
4 files changed, 733 insertions(+)
create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
create mode 100644 include/media/v4l2-flash.h
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index ba7e21a..f034f1a 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -44,6 +44,17 @@ config V4L2_MEM2MEM_DEV
tristate
depends on VIDEOBUF2_CORE
+# Used by LED subsystem flash drivers
+config V4L2_FLASH_LED_CLASS
+ tristate Enable support for Flash sub-devices
+ depends on VIDEO_V4L2_SUBDEV_API
+ depends on LEDS_CLASS_FLASH
+ ---help---
+ Say Y here to enable support for Flash sub-devices, which allow
+ to control LED class devices with use of V4L2 Flash controls.
+
+ When in doubt, say N.
+
# Used by drivers that need Videobuf modules
config VIDEOBUF_GEN
tristate
diff --git a/drivers/media/v4l2-core/Makefile
b/drivers/media/v4l2-core/Makefile
index 63d29f2..44e858c 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o
+
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
diff --git a/drivers/media/v4l2-core/v4l2-flash.c
b/drivers/media/v4l2-core/v4l2-flash.c
new file mode 100644
index 000..3fd6a08
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-flash.c
@@ -0,0 +1,581 @@
+/*
+ * V4L2 Flash LED sub-device registration helpers.
+ *
+ * Copyright (C) 2015 Samsung Electronics Co., Ltd
+ * Author: Jacek Anaszewski j.anaszew...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/led-class-flash.h
+#include linux/module.h
+#include linux/mutex.h
+#include linux/slab.h
+#include linux/types.h
+#include media/v4l2-flash.h
+
+#define has_flash_op(v4l2_flash, op) \
+ (v4l2_flash v4l2_flash-ops-op)
+
+#define call_flash_op(v4l2_flash, op, args...) \
+ (has_flash_op(v4l2_flash, op) ? \
+ v4l2_flash-ops-op(args) : \
Wouldn't you need __VA_ARGS__ here to deliver the variable argument to the
callee? Do you need variable arguments for the current flash ops?
+ -EINVAL)
+
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
I'd drop inline and let the compiler decide. Same below.
+ struct v4l2_ctrl **ctrls,
+ enum ctrl_init_data_id cdata_id,
+ s32 intensity)
+{
+ struct v4l2_ctrl *ctrl = ctrls[cdata_id];
+ s64 __intensity = intensity - ctrl-minimum;
+
+ do_div(__intensity, ctrl-step);
+
+ /*
+ * Indicator leds, unlike torch leds, are turned on/off basing on
+ * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+ * Therefore it must be possible to set it to 0 level which in
+ * the LED subsystem reflects LED_OFF state.
+ */
+ if (cdata_id != INDICATOR_INTENSITY)
+ ++__intensity;
+
+ return __intensity;
+}
+
+static inline s32 v4l2_flash_led_brightness_to_intensity(
+ struct v4l2_ctrl **ctrls,
+ enum ctrl_init_data_id cdata_id,
Could you pass a pointer to struct v4l2_ctrl instead?
+ enum led_brightness brightness)
+{
+ struct v4l2_ctrl *ctrl = ctrls[cdata_id];
+
+ /*
+ * Indicator leds, unlike torch leds, are turned on/off basing on
+ * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY
Re: [PATCH/RFC v10 15/19] media: Add registration helpers for V4L2 flash sub-devices
Hi Pavel,
Thanks for the review.
On 01/09/2015 09:54 PM, Pavel Machek wrote:
On Fri 2015-01-09 16:23:05, Jacek Anaszewski wrote:
This patch adds helper functions for registering/unregistering
LED Flash class devices as V4L2 sub-devices. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pavel Machek pa...@ucw.cz
+ /*
+* Indicator leds, unlike torch leds, are turned on/off basing
on
leds - LEDs.
Sure.
+* the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+* Therefore it must be possible to set it to 0 level which in
+* the LED subsystem reflects LED_OFF state.
+*/
+ if (cdata_id != INDICATOR_INTENSITY)
+ ++__intensity;
And normally we'd do i++ instead of ++i, and avoid __ for local
variables...?
Pre-incrementation operator is favourable over the post-incrementation
one if we don't want to have an access to the value of a variable before
incrementation, which is the case here.
Maybe gcc detects the cases when the value of a variable is not assigned
and doesn't copy it before incrementing, however I haven't found any
reference. I see that often in the for loops the i++ version
is used, but I am not sure if this is done because developers are
aware that gcc will optimize it anyway or it is just an omission.
And regarding __ for local variable - I haven't found any restriction
about it in the Documentation/CodingStyle. Nevertheless I can rename
it to tmp or something.
+/**
+ * struct v4l2_flash_ctrl_config - V4L2 Flash controls initialization data
+ * @intensity: constraints for the led in a non-flash mode
+ * @flash_intensity: V4L2_CID_FLASH_INTENSITY settings constraints
+ * @flash_timeout: V4L2_CID_FLASH_TIMEOUT constraints
+ * @flash_faults: possible flash faults
+ * @has_external_strobe: external strobe capability
+ * @indicator_led: signifies that a led is of indicator type
+ */
+struct v4l2_flash_ctrl_config {
+ struct v4l2_ctrl_config intensity;
+ struct v4l2_ctrl_config flash_intensity;
+ struct v4l2_ctrl_config flash_timeout;
+ u32 flash_faults;
+ bool has_external_strobe:1;
+ bool indicator_led:1;
+};
I don't think you are supposed to do boolean bit arrays.
These bit fields allow to reduce memory usage. If they were not bit
fields, the address of the next variable would be aligned to the
multiply of the CPU word size.
Please look e.g. at struct dev_pm_info in the file include/linux/pm.h.
It also contains boolean bit fields.
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v10 15/19] media: Add registration helpers for V4L2 flash sub-devices
Hi!
+* the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+* Therefore it must be possible to set it to 0 level which in
+* the LED subsystem reflects LED_OFF state.
+*/
+ if (cdata_id != INDICATOR_INTENSITY)
+ ++__intensity;
And normally we'd do i++ instead of ++i, and avoid __ for local
variables...?
Pre-incrementation operator is favourable over the post-incrementation
one if we don't want to have an access to the value of a variable before
incrementation, which is the case here.
That may be some old C++ convention, but I'm pretty sure gcc does not
care.
Maybe gcc detects the cases when the value of a variable is not assigned
and doesn't copy it before incrementing, however I haven't found any
reference. I see that often in the for loops the i++ version
is used, but I am not sure if this is done because developers are
aware that gcc will optimize it anyway or it is just an omission.
The code is equivalent, and normally the n++ version is used. gcc will
get it right.
+struct v4l2_flash_ctrl_config {
+ struct v4l2_ctrl_config intensity;
+ struct v4l2_ctrl_config flash_intensity;
+ struct v4l2_ctrl_config flash_timeout;
+ u32 flash_faults;
+ bool has_external_strobe:1;
+ bool indicator_led:1;
+};
I don't think you are supposed to do boolean bit arrays.
These bit fields allow to reduce memory usage. If they were not bit
fields, the address of the next variable would be aligned to the
multiply of the CPU word size.
Please look e.g. at struct dev_pm_info in the file include/linux/pm.h.
It also contains boolean bit fields.
Looks like you are right. I guess I confused bool foo:1 with int
foo:1.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC v10 15/19] media: Add registration helpers for V4L2 flash sub-devices
This patch adds helper functions for registering/unregistering
LED Flash class devices as V4L2 sub-devices. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Hans Verkuil hans.verk...@cisco.com
---
drivers/media/v4l2-core/Kconfig | 11 +
drivers/media/v4l2-core/Makefile |2 +
drivers/media/v4l2-core/v4l2-flash.c | 581 ++
include/media/v4l2-flash.h | 139
4 files changed, 733 insertions(+)
create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
create mode 100644 include/media/v4l2-flash.h
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index ba7e21a..f034f1a 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -44,6 +44,17 @@ config V4L2_MEM2MEM_DEV
tristate
depends on VIDEOBUF2_CORE
+# Used by LED subsystem flash drivers
+config V4L2_FLASH_LED_CLASS
+ tristate Enable support for Flash sub-devices
+ depends on VIDEO_V4L2_SUBDEV_API
+ depends on LEDS_CLASS_FLASH
+ ---help---
+ Say Y here to enable support for Flash sub-devices, which allow
+ to control LED class devices with use of V4L2 Flash controls.
+
+ When in doubt, say N.
+
# Used by drivers that need Videobuf modules
config VIDEOBUF_GEN
tristate
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 63d29f2..44e858c 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o
+
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
diff --git a/drivers/media/v4l2-core/v4l2-flash.c
b/drivers/media/v4l2-core/v4l2-flash.c
new file mode 100644
index 000..3fd6a08
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-flash.c
@@ -0,0 +1,581 @@
+/*
+ * V4L2 Flash LED sub-device registration helpers.
+ *
+ * Copyright (C) 2015 Samsung Electronics Co., Ltd
+ * Author: Jacek Anaszewski j.anaszew...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/led-class-flash.h
+#include linux/module.h
+#include linux/mutex.h
+#include linux/slab.h
+#include linux/types.h
+#include media/v4l2-flash.h
+
+#define has_flash_op(v4l2_flash, op) \
+ (v4l2_flash v4l2_flash-ops-op)
+
+#define call_flash_op(v4l2_flash, op, args...) \
+ (has_flash_op(v4l2_flash, op) ? \
+ v4l2_flash-ops-op(args) : \
+ -EINVAL)
+
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct v4l2_ctrl **ctrls,
+ enum ctrl_init_data_id cdata_id,
+ s32 intensity)
+{
+ struct v4l2_ctrl *ctrl = ctrls[cdata_id];
+ s64 __intensity = intensity - ctrl-minimum;
+
+ do_div(__intensity, ctrl-step);
+
+ /*
+* Indicator leds, unlike torch leds, are turned on/off basing on
+* the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+* Therefore it must be possible to set it to 0 level which in
+* the LED subsystem reflects LED_OFF state.
+*/
+ if (cdata_id != INDICATOR_INTENSITY)
+ ++__intensity;
+
+ return __intensity;
+}
+
+static inline s32 v4l2_flash_led_brightness_to_intensity(
+ struct v4l2_ctrl **ctrls,
+ enum ctrl_init_data_id cdata_id,
+ enum led_brightness brightness)
+{
+ struct v4l2_ctrl *ctrl = ctrls[cdata_id];
+
+ /*
+* Indicator leds, unlike torch leds, are turned on/off basing on
+* the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+* Do not decrement brightness read from the LED subsystem for
+* indicator led as it may equal 0. For torch leds this function
+* is called only when V4L2_FLASH_LED_MODE_TORCH is set and the
+* brightness read is guaranteed to be greater than 0. In the mode
+* V4L2_FLASH_LED_MODE_NONE the cached torch intensity value is used.
+*/
+ if (cdata_id != INDICATOR_INTENSITY)
+ --brightness;
+
+ return (brightness * ctrl-step) +
Re: [PATCH/RFC v10 15/19] media: Add registration helpers for V4L2 flash sub-devices
On Fri 2015-01-09 16:23:05, Jacek Anaszewski wrote:
This patch adds helper functions for registering/unregistering
LED Flash class devices as V4L2 sub-devices. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Hans Verkuil hans.verk...@cisco.com
Acked-by: Pavel Machek pa...@ucw.cz
+ /*
+ * Indicator leds, unlike torch leds, are turned on/off basing
on
leds - LEDs.
+ * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
+ * Therefore it must be possible to set it to 0 level which in
+ * the LED subsystem reflects LED_OFF state.
+ */
+ if (cdata_id != INDICATOR_INTENSITY)
+ ++__intensity;
And normally we'd do i++ instead of ++i, and avoid __ for local
variables...?
+/**
+ * struct v4l2_flash_ctrl_config - V4L2 Flash controls initialization data
+ * @intensity: constraints for the led in a non-flash
mode
+ * @flash_intensity: V4L2_CID_FLASH_INTENSITY settings constraints
+ * @flash_timeout: V4L2_CID_FLASH_TIMEOUT constraints
+ * @flash_faults:possible flash faults
+ * @has_external_strobe: external strobe capability
+ * @indicator_led: signifies that a led is of indicator type
+ */
+struct v4l2_flash_ctrl_config {
+ struct v4l2_ctrl_config intensity;
+ struct v4l2_ctrl_config flash_intensity;
+ struct v4l2_ctrl_config flash_timeout;
+ u32 flash_faults;
+ bool has_external_strobe:1;
+ bool indicator_led:1;
+};
I don't think you are supposed to do boolean bit arrays.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
