Hi Jacek, On Mon, Mar 23, 2015 at 04:08:10PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. > > On 03/22/2015 01:22 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thanks for the updated set. Some comments below. > > > >On Fri, Mar 20, 2015 at 04:03:27PM +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 <[email protected]> > >>Acked-by: Kyungmin Park <[email protected]> > >>Cc: Sakari Ailus <[email protected]> > >>Cc: Hans Verkuil <[email protected]> > >>--- > >> drivers/media/v4l2-core/Kconfig | 12 + > >> drivers/media/v4l2-core/Makefile | 2 + > >> drivers/media/v4l2-core/v4l2-flash.c | 607 > >> ++++++++++++++++++++++++++++++++++ > >> include/media/v4l2-flash.h | 145 ++++++++ > >> 4 files changed, 766 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..0659598 100644 > >>--- a/drivers/media/v4l2-core/Kconfig > >>+++ b/drivers/media/v4l2-core/Kconfig > >>@@ -44,6 +44,18 @@ 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 > >>+ depends on OF > > > >I think you can drop OF dependency. A no-op implementation will be used if > >it's disabled. > > The sub-devices are matched by a sub-LED's device_node. This is the only > way of matching the v4l2-flash sub-devs. Without OF a v4l2-flash > sub-device will not get registered at all.
Not all systems use OF, and for those there are different async sub-device matching options such as i2c or device name. A sub-device driver should not mandate using a particular one. > What do you mean by "no-op implementation"? There are variants of most of_*() functions that do nothing. These variants are used if CONFIG_OF is disabled, so the drivers won't need lots of ugly #ifdefs. > >>+ ---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 0000000..804c2e4 > >>--- /dev/null > >>+++ b/drivers/media/v4l2-core/v4l2-flash.c > >>@@ -0,0 +1,607 @@ > >>+/* > >>+ * V4L2 Flash LED sub-device registration helpers. > >>+ * > >>+ * Copyright (C) 2015 Samsung Electronics Co., Ltd > >>+ * Author: Jacek Anaszewski <[email protected]> > >>+ * > >>+ * 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/of.h> > >>+#include <linux/slab.h> > >>+#include <linux/types.h> > >>+#include <media/v4l2-flash.h> > >>+#include "../../leds/leds.h" > > > >What do you need from leds.h? Shouldn't this be e.g. under include/linux > >instead? > > > >>+#define has_flash_op(v4l2_flash, op) \ > >>+ (v4l2_flash && v4l2_flash->ops->op) > >>+ > >>+#define call_flash_op(v4l2_flash, op, arg) \ > >>+ (has_flash_op(v4l2_flash, op) ? \ > >>+ v4l2_flash->ops->op(v4l2_flash, arg) : \ > >>+ -EINVAL) > >>+ > >>+static enum led_brightness __intensity_to_led_brightness( > >>+ struct v4l2_ctrl *ctrl, > >>+ s32 intensity) > >>+{ > >>+ s64 intensity64 = intensity - ctrl->minimum; > >>+ > >>+ do_div(intensity64, 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 (ctrl->id != V4L2_CID_FLASH_INDICATOR_INTENSITY) > >>+ ++intensity64; > >>+ > >>+ return intensity64; > >>+} > >>+ > >>+static s32 __led_brightness_to_intensity(struct v4l2_ctrl *ctrl, > >>+ enum led_brightness brightness) > >>+{ > >>+ /* > >>+ * 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 (ctrl->id != V4L2_CID_FLASH_INDICATOR_INTENSITY) > >>+ --brightness; > >>+ > >>+ return (brightness * ctrl->step) + ctrl->minimum; > >>+} > >>+ > >>+static void v4l2_flash_set_led_brightness(struct v4l2_flash *v4l2_flash, > >>+ struct v4l2_ctrl *ctrl) > >>+{ > >>+ struct v4l2_ctrl **ctrls = v4l2_flash->ctrls; > >>+ enum led_brightness brightness; > >>+ > >>+ if (has_flash_op(v4l2_flash, intensity_to_led_brightness)) > >>+ brightness = call_flash_op(v4l2_flash, > >>+ intensity_to_led_brightness, > >>+ ctrl->val); > >>+ else > >>+ brightness = __intensity_to_led_brightness(ctrl, ctrl->val); > >>+ /* > >>+ * In case a LED Flash class driver provides ops for custom > >>+ * brightness <-> intensity conversion, it also must have defined > >>+ * related v4l2 control step == 1. In such a case a backward conversion > >>+ * from led brightness to v4l2 intensity is required to find out the > >>+ * the aligned intensity value. > >>+ */ > >>+ if (has_flash_op(v4l2_flash, led_brightness_to_intensity)) > >>+ ctrl->val = call_flash_op(v4l2_flash, > >>+ led_brightness_to_intensity, > >>+ brightness); > >>+ > >>+ if (ctrl == ctrls[TORCH_INTENSITY] && > >>+ ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_TORCH) > >>+ return; > >>+ > >>+ led_set_brightness(&v4l2_flash->fled_cdev->led_cdev, brightness); > >>+} > >>+ > >>+static int v4l2_flash_update_led_brightness(struct v4l2_flash *v4l2_flash, > >>+ struct v4l2_ctrl *ctrl) > >>+{ > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ struct led_classdev *led_cdev = &fled_cdev->led_cdev; > >>+ struct v4l2_ctrl **ctrls = v4l2_flash->ctrls; > >>+ int ret; > >>+ > >>+ /* > >>+ * Update torch brightness only if in TORCH_MODE. In other modes torch > >>+ * led is turned off, which would spuriously inform the user space that > >>+ * V4L2_CID_FLASH_TORCH_INTENSITY control value has changed to 0. > >>+ */ > >>+ if (ctrl == ctrls[TORCH_INTENSITY] && > >>+ ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_TORCH) > >>+ return 0; > >>+ > >>+ ret = led_update_brightness(led_cdev); > >>+ if (ret < 0) > >>+ return ret; > >>+ > >>+ if (has_flash_op(v4l2_flash, led_brightness_to_intensity)) > >>+ ctrl->val = call_flash_op(v4l2_flash, > >>+ led_brightness_to_intensity, > >>+ led_cdev->brightness); > >>+ else > >>+ ctrl->val = __led_brightness_to_intensity(ctrl, > >>+ led_cdev->brightness); > >>+ > >>+ return 0; > >>+} > >>+ > >>+static int v4l2_flash_g_volatile_ctrl(struct v4l2_ctrl *c) > >>+{ > >>+ struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c); > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ bool is_strobing; > >>+ int ret; > >>+ > >>+ switch (c->id) { > >>+ case V4L2_CID_FLASH_TORCH_INTENSITY: > >>+ case V4L2_CID_FLASH_INDICATOR_INTENSITY: > >>+ return v4l2_flash_update_led_brightness(v4l2_flash, c); > >>+ case V4L2_CID_FLASH_INTENSITY: > >>+ ret = led_update_flash_brightness(fled_cdev); > >>+ if (ret < 0) > >>+ return ret; > >>+ /* no conversion is needed */ > >>+ c->val = fled_cdev->brightness.val; > >>+ return 0; > >>+ case V4L2_CID_FLASH_STROBE_STATUS: > >>+ ret = led_get_flash_strobe(fled_cdev, &is_strobing); > >>+ if (ret < 0) > >>+ return ret; > >>+ c->val = is_strobing; > >>+ return 0; > >>+ case V4L2_CID_FLASH_FAULT: > >>+ /* LED faults map directly to V4L2 flash faults */ > >>+ return led_get_flash_fault(fled_cdev, &c->val); > >>+ default: > >>+ return -EINVAL; > >>+ } > >>+} > >>+ > >>+static bool __software_strobe_mode_inactive(struct v4l2_ctrl **ctrls) > >>+{ > >>+ return ((ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_FLASH) || > >>+ (ctrls[STROBE_SOURCE] && (ctrls[STROBE_SOURCE]->val != > >>+ V4L2_FLASH_STROBE_SOURCE_SOFTWARE))); > >>+} > >>+ > >>+static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c) > >>+{ > >>+ struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c); > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ struct led_classdev *led_cdev = &fled_cdev->led_cdev; > >>+ struct v4l2_ctrl **ctrls = v4l2_flash->ctrls; > >>+ bool external_strobe; > >>+ int ret = 0; > >>+ > >>+ switch (c->id) { > >>+ case V4L2_CID_FLASH_LED_MODE: > >>+ switch (c->val) { > >>+ case V4L2_FLASH_LED_MODE_NONE: > >>+ led_set_brightness(led_cdev, LED_OFF); > >>+ return led_set_flash_strobe(fled_cdev, false); > >>+ case V4L2_FLASH_LED_MODE_FLASH: > >>+ /* Turn the torch LED off */ > >>+ led_set_brightness(led_cdev, LED_OFF); > >>+ if (ctrls[STROBE_SOURCE]) { > >>+ external_strobe = (ctrls[STROBE_SOURCE]->val == > >>+ V4L2_FLASH_STROBE_SOURCE_EXTERNAL); > >>+ > >>+ ret = call_flash_op(v4l2_flash, > >>+ external_strobe_set, > >>+ external_strobe); > >>+ } > >>+ return ret; > >>+ case V4L2_FLASH_LED_MODE_TORCH: > >>+ if (ctrls[STROBE_SOURCE]) { > >>+ ret = call_flash_op(v4l2_flash, > >>+ external_strobe_set, > >>+ false); > >>+ if (ret < 0) > >>+ return ret; > >>+ } > >>+ /* Stop flash strobing */ > >>+ ret = led_set_flash_strobe(fled_cdev, false); > >>+ if (ret < 0) > >>+ return ret; > >>+ > >>+ v4l2_flash_set_led_brightness(v4l2_flash, > >>+ ctrls[TORCH_INTENSITY]); > >>+ return 0; > >>+ } > >>+ break; > >>+ case V4L2_CID_FLASH_STROBE_SOURCE: > >>+ external_strobe = (c->val == V4L2_FLASH_STROBE_SOURCE_EXTERNAL); > >>+ /* > >>+ * For some hardware arrangements setting strobe source may > >>+ * affect torch mode. Therefore, if not in the flash mode, > >>+ * cache only this setting. It will be applied upon switching > >>+ * to flash mode. > >>+ */ > >>+ if (ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_FLASH) > >>+ return 0; > >>+ > >>+ return call_flash_op(v4l2_flash, external_strobe_set, > >>+ external_strobe); > >>+ case V4L2_CID_FLASH_STROBE: > >>+ if (__software_strobe_mode_inactive(ctrls)) > >>+ return -EBUSY; > >>+ return led_set_flash_strobe(fled_cdev, true); > >>+ case V4L2_CID_FLASH_STROBE_STOP: > >>+ if (__software_strobe_mode_inactive(ctrls)) > >>+ return -EBUSY; > >>+ return led_set_flash_strobe(fled_cdev, false); > >>+ case V4L2_CID_FLASH_TIMEOUT: > >>+ /* no conversion is needed */ > >>+ return led_set_flash_timeout(fled_cdev, c->val); > >>+ case V4L2_CID_FLASH_INTENSITY: > >>+ /* no conversion is needed */ > >>+ return led_set_flash_brightness(fled_cdev, c->val); > >>+ case V4L2_CID_FLASH_TORCH_INTENSITY: > >>+ case V4L2_CID_FLASH_INDICATOR_INTENSITY: > >>+ v4l2_flash_set_led_brightness(v4l2_flash, c); > >>+ return 0; > >>+ } > >>+ > >>+ return -EINVAL; > >>+} > >>+ > >>+static const struct v4l2_ctrl_ops v4l2_flash_ctrl_ops = { > >>+ .g_volatile_ctrl = v4l2_flash_g_volatile_ctrl, > >>+ .s_ctrl = v4l2_flash_s_ctrl, > >>+}; > >>+ > >>+static void fill_ctrl_init_data(struct v4l2_flash *v4l2_flash, > >>+ struct v4l2_flash_ctrl_config *flash_ctrl_cfg, > >>+ struct v4l2_flash_ctrl_data *ctrl_init_data) > >>+{ > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ const struct led_flash_ops *fled_cdev_ops = fled_cdev->ops; > >>+ struct led_classdev *led_cdev = &fled_cdev->led_cdev; > >>+ struct v4l2_ctrl_config *ctrl_cfg; > >>+ u32 mask; > >>+ > >>+ /* Init FLASH_FAULT ctrl data */ > >>+ if (flash_ctrl_cfg->flash_faults) { > >>+ ctrl_init_data[FLASH_FAULT].cid = V4L2_CID_FLASH_FAULT; > >>+ ctrl_cfg = &ctrl_init_data[FLASH_FAULT].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_FAULT; > >>+ ctrl_cfg->max = flash_ctrl_cfg->flash_faults; > >>+ ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE | > >>+ V4L2_CTRL_FLAG_READ_ONLY; > >>+ } > >>+ > >>+ /* Init INDICATOR_INTENSITY ctrl data */ > >>+ if (flash_ctrl_cfg->indicator_led) { > >>+ ctrl_init_data[INDICATOR_INTENSITY].cid = > >>+ V4L2_CID_FLASH_INDICATOR_INTENSITY; > >>+ ctrl_init_data[INDICATOR_INTENSITY].config = > >>+ flash_ctrl_cfg->intensity; > >>+ ctrl_cfg = &ctrl_init_data[INDICATOR_INTENSITY].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_INDICATOR_INTENSITY; > >>+ ctrl_cfg->min = 0; > >>+ ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE; > >>+ > >>+ /* Indicator LED can have only faults and intensity controls. */ > >>+ return; > >>+ } > >>+ > >>+ /* Init FLASH_LED_MODE ctrl data */ > >>+ mask = 1 << V4L2_FLASH_LED_MODE_NONE | > >>+ 1 << V4L2_FLASH_LED_MODE_TORCH; > >>+ if (led_cdev->flags & LED_DEV_CAP_FLASH) > >>+ mask |= 1 << V4L2_FLASH_LED_MODE_FLASH; > >>+ > >>+ ctrl_init_data[LED_MODE].cid = V4L2_CID_FLASH_LED_MODE; > >>+ ctrl_cfg = &ctrl_init_data[LED_MODE].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_LED_MODE; > >>+ ctrl_cfg->max = V4L2_FLASH_LED_MODE_TORCH; > >>+ ctrl_cfg->menu_skip_mask = ~mask; > >>+ ctrl_cfg->def = V4L2_FLASH_LED_MODE_NONE; > >>+ ctrl_cfg->flags = 0; > >>+ > >>+ /* Init TORCH_INTENSITY ctrl data */ > >>+ ctrl_init_data[TORCH_INTENSITY].cid = V4L2_CID_FLASH_TORCH_INTENSITY; > >>+ ctrl_init_data[TORCH_INTENSITY].config = flash_ctrl_cfg->intensity; > >>+ ctrl_cfg = &ctrl_init_data[TORCH_INTENSITY].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_TORCH_INTENSITY; > >>+ ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE; > >>+ > >>+ if (!(led_cdev->flags & LED_DEV_CAP_FLASH)) > >>+ return; > >>+ > >>+ /* Init FLASH_STROBE ctrl data */ > >>+ ctrl_init_data[FLASH_STROBE].cid = V4L2_CID_FLASH_STROBE; > >>+ ctrl_cfg = &ctrl_init_data[FLASH_STROBE].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_STROBE; > >>+ > >>+ /* Init STROBE_STOP ctrl data */ > >>+ ctrl_init_data[STROBE_STOP].cid = V4L2_CID_FLASH_STROBE_STOP; > >>+ ctrl_cfg = &ctrl_init_data[STROBE_STOP].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_STROBE_STOP; > >>+ > >>+ /* Init FLASH_STROBE_SOURCE ctrl data */ > >>+ if (flash_ctrl_cfg->has_external_strobe) { > >>+ mask = (1 << V4L2_FLASH_STROBE_SOURCE_SOFTWARE) | > >>+ (1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL); > >>+ ctrl_init_data[STROBE_SOURCE].cid = > >>+ V4L2_CID_FLASH_STROBE_SOURCE; > >>+ ctrl_cfg = &ctrl_init_data[STROBE_SOURCE].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_STROBE_SOURCE; > >>+ ctrl_cfg->max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL; > >>+ ctrl_cfg->menu_skip_mask = ~mask; > >>+ ctrl_cfg->def = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > >>+ } > >>+ > >>+ /* Init STROBE_STATUS ctrl data */ > >>+ if (fled_cdev_ops->strobe_get) { > >>+ ctrl_init_data[STROBE_STATUS].cid = > >>+ V4L2_CID_FLASH_STROBE_STATUS; > >>+ ctrl_cfg = &ctrl_init_data[STROBE_STATUS].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_STROBE_STATUS; > >>+ ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE | > >>+ V4L2_CTRL_FLAG_READ_ONLY; > >>+ } > >>+ > >>+ /* Init FLASH_TIMEOUT ctrl data */ > >>+ if (fled_cdev_ops->timeout_set) { > >>+ ctrl_init_data[FLASH_TIMEOUT].cid = V4L2_CID_FLASH_TIMEOUT; > >>+ ctrl_init_data[FLASH_TIMEOUT].config = > >>+ flash_ctrl_cfg->flash_timeout; > >>+ ctrl_cfg = &ctrl_init_data[FLASH_TIMEOUT].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_TIMEOUT; > >>+ } > >>+ > >>+ /* Init FLASH_INTENSITY ctrl data */ > >>+ if (fled_cdev_ops->flash_brightness_set) { > >>+ ctrl_init_data[FLASH_INTENSITY].cid = V4L2_CID_FLASH_INTENSITY; > >>+ ctrl_init_data[FLASH_INTENSITY].config = > >>+ flash_ctrl_cfg->flash_intensity; > >>+ ctrl_cfg = &ctrl_init_data[FLASH_INTENSITY].config; > >>+ ctrl_cfg->id = V4L2_CID_FLASH_INTENSITY; > >>+ ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE; > >>+ } > >>+} > >>+ > >>+static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash, > >>+ struct v4l2_flash_ctrl_config *flash_ctrl_cfg) > >>+ > >>+{ > >>+ struct v4l2_flash_ctrl_data *ctrl_init_data; > >>+ struct v4l2_ctrl *ctrl; > >>+ struct v4l2_ctrl_config *ctrl_cfg; > >>+ int i, ret, num_ctrls = 0; > >>+ > >>+ /* allocate memory dynamically so as not to exceed stack frame size */ > >>+ ctrl_init_data = kcalloc(NUM_FLASH_CTRLS, sizeof(*ctrl_init_data), > >>+ GFP_KERNEL); > >>+ if (!ctrl_init_data) > >>+ return -ENOMEM; > >>+ > >>+ fill_ctrl_init_data(v4l2_flash, flash_ctrl_cfg, ctrl_init_data); > >>+ > >>+ for (i = 0; i < NUM_FLASH_CTRLS; ++i) > >>+ if (ctrl_init_data[i].cid) > >>+ ++num_ctrls; > >>+ > >>+ v4l2_ctrl_handler_init(&v4l2_flash->hdl, num_ctrls); > >>+ > >>+ for (i = 0; i < NUM_FLASH_CTRLS; ++i) { > >>+ ctrl_cfg = &ctrl_init_data[i].config; > >>+ if (!ctrl_init_data[i].cid) > >>+ continue; > >>+ > >>+ if (ctrl_cfg->id == V4L2_CID_FLASH_LED_MODE || > >>+ ctrl_cfg->id == V4L2_CID_FLASH_STROBE_SOURCE) > >>+ ctrl = v4l2_ctrl_new_std_menu(&v4l2_flash->hdl, > >>+ &v4l2_flash_ctrl_ops, > >>+ ctrl_cfg->id, > >>+ ctrl_cfg->max, > >>+ ctrl_cfg->menu_skip_mask, > >>+ ctrl_cfg->def); > >>+ else > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, > >>+ &v4l2_flash_ctrl_ops, > >>+ ctrl_cfg->id, > >>+ ctrl_cfg->min, > >>+ ctrl_cfg->max, > >>+ ctrl_cfg->step, > >>+ ctrl_cfg->def); > >>+ > >>+ if (ctrl) > >>+ ctrl->flags |= ctrl_cfg->flags; > >>+ > >>+ if (i <= STROBE_SOURCE) > >>+ v4l2_flash->ctrls[i] = ctrl; > >>+ } > >>+ > >>+ kfree(ctrl_init_data); > >>+ > >>+ if (v4l2_flash->hdl.error) { > >>+ ret = v4l2_flash->hdl.error; > >>+ goto error_free_handler; > >>+ } > >>+ > >>+ v4l2_ctrl_handler_setup(&v4l2_flash->hdl); > >>+ > >>+ v4l2_flash->sd.ctrl_handler = &v4l2_flash->hdl; > >>+ > >>+ return 0; > >>+ > >>+error_free_handler: > >>+ v4l2_ctrl_handler_free(&v4l2_flash->hdl); > >>+ return ret; > >>+} > >>+ > >>+static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash) > >>+{ > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ struct v4l2_ctrl **ctrls = v4l2_flash->ctrls; > >>+ int ret = 0; > >>+ > >>+ if (ctrls[FLASH_TIMEOUT]) { > >>+ ret = led_set_flash_timeout(fled_cdev, > >>+ ctrls[FLASH_TIMEOUT]->val); > >>+ if (ret < 0) > >>+ return ret; > >>+ } > >>+ > >>+ if (ctrls[FLASH_INTENSITY]) { > >>+ ret = led_set_flash_brightness(fled_cdev, > >>+ ctrls[FLASH_INTENSITY]->val); > >>+ if (ret < 0) > >>+ return ret; > >>+ } > >>+ > >>+ /* > >>+ * For some hardware arrangements setting strobe source may affect > >>+ * torch mode. Synchronize strobe source setting only if not in torch > >>+ * mode. For torch mode case it will get synchronized upon switching > >>+ * to flash mode. > >>+ */ > >>+ if (ctrls[STROBE_SOURCE] && > >>+ ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_TORCH) > >>+ ret = call_flash_op(v4l2_flash, external_strobe_set, > >>+ ctrls[STROBE_SOURCE]->val); > >>+ > >>+ if (ctrls[INDICATOR_INTENSITY]) > >>+ v4l2_flash_set_led_brightness(v4l2_flash, > >>+ ctrls[INDICATOR_INTENSITY]); > >>+ else > >>+ v4l2_flash_set_led_brightness(v4l2_flash, > >>+ ctrls[TORCH_INTENSITY]); > >>+ > >>+ return ret; > >>+} > >>+ > >>+/* > >>+ * V4L2 subdev internal operations > >>+ */ > >>+ > >>+static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh > >>*fh) > >>+{ > >>+ struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd); > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ struct led_classdev *led_cdev = &fled_cdev->led_cdev; > >>+ int ret = 0; > > > >No need to initialise ret. > > > >>+ > >>+ mutex_lock(&led_cdev->led_access); > >>+ > >>+ if (!v4l2_fh_is_singular(&fh->vfh)) { > > > >I'm not sure about this --- no current flash driver AFAIK provides exclusive > >access. > > v4l2_flash_open has a side effect in the form of writing device > registers with v4l2-flash controls state. Generally it should not > affect e.g. the flash strobe if it took place in the same moment. > > What should be added, if we are to allow multiple users, is the > reference counter to be able to detect that last user is closing > a sub-dev and only then set the strobe source to software. You shouldn't need a counter, just use v4l2_if_is_singular() as you already do. But instead of returning -EBUSY, return 0. :-) > >>+ ret = -EBUSY; > >>+ goto unlock; > >>+ } > >>+ > >>+ led_sysfs_disable(led_cdev); > >>+ led_trigger_remove(led_cdev); > >>+ > >>+ ret = __sync_device_with_v4l2_controls(v4l2_flash); > >>+ > >>+unlock: > >>+ mutex_unlock(&led_cdev->led_access); > >>+ return ret; > >>+} > >>+ > >>+static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh > >>*fh) > >>+{ > >>+ struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd); > >>+ struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > >>+ struct led_classdev *led_cdev = &fled_cdev->led_cdev; > >>+ int ret = 0; > > > >ret is redundant. > > I think that I forgot to assign the v4l2_ctrl_s_ctrl result to it. > > >>+ > >>+ mutex_lock(&led_cdev->led_access); > >>+ > >>+ if (v4l2_flash->ctrls[STROBE_SOURCE]) > >>+ v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SOURCE], > >>+ V4L2_FLASH_STROBE_SOURCE_SOFTWARE); > >>+ > >>+ led_sysfs_enable(led_cdev); > >>+ > >>+ mutex_unlock(&led_cdev->led_access); > >>+ > >>+ return ret; > >>+} > >>+ > >>+static const struct v4l2_subdev_internal_ops > >>v4l2_flash_subdev_internal_ops = { > >>+ .open = v4l2_flash_open, > >>+ .close = v4l2_flash_close, > >>+}; > >>+ > >>+static const struct v4l2_subdev_core_ops v4l2_flash_core_ops = { > >>+ .queryctrl = v4l2_subdev_queryctrl, > >>+ .querymenu = v4l2_subdev_querymenu, > >>+}; > >>+ > >>+static const struct v4l2_subdev_ops v4l2_flash_subdev_ops = { > >>+ .core = &v4l2_flash_core_ops, > >>+}; > >>+ > >>+struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev, > >>+ const struct v4l2_flash_ops *ops, > >>+ struct v4l2_flash_ctrl_config *config) > >>+{ > >>+ struct v4l2_flash *v4l2_flash; > >>+ struct led_classdev *led_cdev = &fled_cdev->led_cdev; > >>+ struct v4l2_subdev *sd; > >>+ int ret; > >>+ > >>+ if (!fled_cdev || !ops || !config) > >>+ return ERR_PTR(-EINVAL); > >>+ > >>+ v4l2_flash = devm_kzalloc(led_cdev->dev, sizeof(*v4l2_flash), > >>+ GFP_KERNEL); > >>+ if (!v4l2_flash) > >>+ return ERR_PTR(-ENOMEM); > >>+ > >>+ sd = &v4l2_flash->sd; > >>+ v4l2_flash->fled_cdev = fled_cdev; > >>+ v4l2_flash->ops = ops; > >>+ sd->dev = led_cdev->dev; > >>+ v4l2_subdev_init(sd, &v4l2_flash_subdev_ops); > >>+ sd->internal_ops = &v4l2_flash_subdev_internal_ops; > >>+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >>+ strlcpy(sd->name, config->dev_name, sizeof(sd->name)); > >>+ > >>+ ret = media_entity_init(&sd->entity, 0, NULL, 0); > >>+ if (ret < 0) > >>+ return ERR_PTR(ret); > >>+ > >>+ sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > >>+ > >>+ ret = v4l2_flash_init_controls(v4l2_flash, config); > >>+ if (ret < 0) > > > >You're missing media_entity_cleanup() here. How about adding a new label > >below for that? > > Right, thanks. > > >>+ return ERR_PTR(ret); > >>+ > >>+ of_node_get(led_cdev->dev->of_node); > >>+ > >>+ ret = v4l2_async_register_subdev(sd); > >>+ if (ret < 0) > >>+ goto err_async_register_sd; > >>+ > >>+ return v4l2_flash; > >>+ > >>+err_async_register_sd: > >>+ of_node_put(led_cdev->dev->of_node); > >>+ v4l2_ctrl_handler_free(sd->ctrl_handler); > >>+ media_entity_cleanup(&sd->entity); > >>+ > >>+ return ERR_PTR(ret); > >>+} > >>+EXPORT_SYMBOL_GPL(v4l2_flash_init); > >>+ > >>+void v4l2_flash_release(struct v4l2_flash *v4l2_flash) > >>+{ > >>+ struct v4l2_subdev *sd = &v4l2_flash->sd; > >>+ struct led_classdev *led_cdev = &v4l2_flash->fled_cdev->led_cdev; > >>+ > >>+ v4l2_async_unregister_subdev(sd); > >>+ of_node_put(led_cdev->dev->of_node); > >>+ v4l2_ctrl_handler_free(sd->ctrl_handler); > >>+ media_entity_cleanup(&sd->entity); > >>+} > >>+EXPORT_SYMBOL_GPL(v4l2_flash_release); > >>+ > >>+MODULE_AUTHOR("Jacek Anaszewski <[email protected]>"); > >>+MODULE_DESCRIPTION("V4L2 Flash sub-device helpers"); > >>+MODULE_LICENSE("GPL v2"); > >>diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h > >>new file mode 100644 > >>index 0000000..cc0138d > >>--- /dev/null > >>+++ b/include/media/v4l2-flash.h > >>@@ -0,0 +1,145 @@ > >>+/* > >>+ * V4L2 Flash LED sub-device registration helpers. > >>+ * > >>+ * Copyright (C) 2015 Samsung Electronics Co., Ltd > >>+ * Author: Jacek Anaszewski <[email protected]> > >>+ * > >>+ * 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. > >>+ */ > >>+ > >>+#ifndef _V4L2_FLASH_H > >>+#define _V4L2_FLASH_H > >>+ > >>+#include <media/v4l2-ctrls.h> > >>+#include <media/v4l2-subdev.h> > >>+ > >>+struct led_classdev_flash; > >>+struct led_classdev; > >>+struct v4l2_flash; > >>+enum led_brightness; > >>+ > >>+enum ctrl_init_data_id { > >>+ LED_MODE, > >>+ TORCH_INTENSITY, > >>+ FLASH_INTENSITY, > >>+ INDICATOR_INTENSITY, > >>+ FLASH_TIMEOUT, > >>+ STROBE_SOURCE, > >>+ /* > >>+ * Only above values are applicable to > >>+ * the 'ctrls' array in the struct v4l2_flash. > >>+ */ > >>+ FLASH_STROBE, > >>+ STROBE_STOP, > >>+ STROBE_STATUS, > >>+ FLASH_FAULT, > >>+ NUM_FLASH_CTRLS, > >>+}; > >>+ > >>+/* > >>+ * struct v4l2_flash_ctrl_data - flash control initialization data, filled > >>+ * basing on the features declared by the LED Flash > >>+ * class driver in the v4l2_flash_ctrl_config > >>+ * @config: initialization data for a control > >>+ * @cid: contains v4l2 flash control id if the config > >>+ * field was initialized, 0 otherwise > >>+ */ > >>+struct v4l2_flash_ctrl_data { > >>+ struct v4l2_ctrl_config config; > >>+ u32 cid; > >>+}; > >>+ > >>+struct v4l2_flash_ops { > >>+ /* setup strobing the flash by hardware pin state assertion */ > >>+ int (*external_strobe_set)(struct v4l2_flash *v4l2_flash, > >>+ bool enable); > >>+ /* convert intensity to brightness in a device specific manner */ > >>+ enum led_brightness (*intensity_to_led_brightness) > >>+ (struct v4l2_flash *v4l2_flash, s32 intensity); > >>+ /* convert brightness to intensity in a device specific manner */ > >>+ s32 (*led_brightness_to_intensity) > >>+ (struct v4l2_flash *v4l2_flash, enum led_brightness); > >>+}; > >>+ > >>+/** > >>+ * struct v4l2_flash_ctrl_config - V4L2 Flash controls initialization data > > > >This is a bit more than just about controls. How about dropping "_ctrl" from > >the name? > > Good point. > > >>+ * @dev_name: human readable device name > > > >Could you use "the name of the media entity, unique in the system" instead, > >please? > > > >>+ * @intensity: constraints for the led in a non-flash > >>mode > >>+ * @flash_intensity: V4L2_CID_FLASH_INTENSITY constraints > >>+ * @flash_timeout: V4L2_CID_FLASH_TIMEOUT constraints > >>+ * @flash_faults: possible flash faults > > > >Please refer to documentation > > Do you want to put here whole description from documentation? > Or maybe only mention that faults can "prevent further use of some of > the flash controls"? No, just a pointer where the flash faults are defined, or which #defines they use for example so the reader can find them easily. > > > > >>+ * @has_external_strobe: external strobe capability > >>+ * @indicator_led: signifies that a led is of indicator type > >>+ */ > >>+struct v4l2_flash_ctrl_config { > >>+ char dev_name[32]; > >>+ struct v4l2_ctrl_config intensity; > >>+ struct v4l2_ctrl_config flash_intensity; > >>+ struct v4l2_ctrl_config flash_timeout; > > > >I think I may have suggested using v4l2_ctrl_config here, but currently the > >drivers need to copy the values to v4l2_ctrl_config struct, instead of using > >struct led_flash_setting which they have to use anyway. > > > >What would you think it'd be feasible to use led_flash_setting here as well? > > I tried and it turned out that there was a room for even greater number > of optimizations around flash settings and config initialization. :-) > > > >>+ u32 flash_faults; > >>+ unsigned int has_external_strobe:1; > >>+ unsigned int indicator_led:1; > >>+}; > >>+ > >>+/** > >>+ * struct v4l2_flash - Flash sub-device context > >>+ * @fled_cdev: LED Flash class device controlled by this > >>sub-device > >>+ * @ops: V4L2 specific flash ops > >>+ * @sd: V4L2 sub-device > >>+ * @hdl: flash controls handler > >>+ * @ctrls: array of pointers to controls, whose values define > >>+ * the sub-device state > >>+ */ > >>+struct v4l2_flash { > >>+ struct led_classdev_flash *fled_cdev; > >>+ const struct v4l2_flash_ops *ops; > >>+ > >>+ struct v4l2_subdev sd; > >>+ struct v4l2_ctrl_handler hdl; > >>+ struct v4l2_ctrl *ctrls[STROBE_SOURCE + 1]; > >>+}; > >>+ > >>+static inline struct v4l2_flash *v4l2_subdev_to_v4l2_flash( > >>+ struct v4l2_subdev *sd) > >>+{ > >>+ return container_of(sd, struct v4l2_flash, sd); > >>+} > >>+ > >>+static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl > >>*c) > >>+{ > >>+ return container_of(c->handler, struct v4l2_flash, hdl); > >>+} > >>+ > >>+#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS) > >>+/** > >>+ * v4l2_flash_init - initialize V4L2 flash led sub-device > >>+ * @fled_cdev: the LED Flash class device to wrap > >>+ * @flash_ops: V4L2 Flash device ops > >>+ * @config: initialization data for V4L2 Flash controls > >>+ * > >>+ * Create V4L2 Flash sub-device wrapping given LED subsystem device. > >>+ * > >>+ * Returns: A valid pointer, or, when an error occurs, the return > >>+ * value is encoded using ERR_PTR(). Use IS_ERR() to check and > >>+ * PTR_ERR() to obtain the numeric return value. > >>+ */ > >>+struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev, > >>+ const struct v4l2_flash_ops *ops, > >>+ struct v4l2_flash_ctrl_config *config); > >>+ > >>+/** > >>+ * v4l2_flash_release - release V4L2 Flash sub-device > >>+ * @flash: the V4L2 Flash sub-device to release > >>+ * > >>+ * Release V4L2 Flash sub-device. > >>+ */ > >>+void v4l2_flash_release(struct v4l2_flash *v4l2_flash); > >>+ > >>+#else > >>+#define v4l2_flash_init(fled_cdev, ops, config) (NULL) > >>+#define v4l2_flash_release(v4l2_flash) > >>+#endif /* CONFIG_V4L2_FLASH_LED_CLASS */ > >>+ > >>+#endif /* _V4L2_FLASH_H */ > > > > -- Kind regards, Sakari Ailus e-mail: [email protected] XMPP: [email protected] -- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
