| Message ID | 1397228216-6657-6-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacek, Thanks for the update! On Fri, Apr 11, 2014 at 04:56:56PM +0200, 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@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/media/v4l2-core/Kconfig | 10 + > drivers/media/v4l2-core/Makefile | 2 + > drivers/media/v4l2-core/v4l2-flash.c | 393 ++++++++++++++++++++++++++++++++++ > include/media/v4l2-flash.h | 119 ++++++++++ > 4 files changed, 524 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 2189bfb..1f8514d 100644 > --- a/drivers/media/v4l2-core/Kconfig > +++ b/drivers/media/v4l2-core/Kconfig > @@ -35,6 +35,16 @@ config V4L2_MEM2MEM_DEV > tristate > depends on VIDEOBUF2_CORE > > +# Used by LED subsystem flash drivers > +config V4L2_FLASH > + bool "Enable support for Flash sub-devices" > + depends on LEDS_CLASS_FLASH Also: depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && MEDIA_CAMERA_SUPPORT The last one might be a matter of opinion. > + ---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 c6ae7ba..8e37ab4 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) += 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..f1be332 > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-flash.c > @@ -0,0 +1,393 @@ > +/* > + * V4L2 Flash LED sub-device registration helpers. > + * > + * Copyright (C) 2014 Samsung Electronics Co., Ltd > + * Author: Jacek Anaszewski <j.anaszewski@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/leds_flash.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-dev.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-event.h> > +#include <media/v4l2-ioctl.h> > +#include <media/v4l2-flash.h> > + > +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > + struct led_ctrl *config, > + u32 intensity) Fits on a single line. > +{ > + return intensity / config->step; Shouldn't you first decrement the minimum before the division? > +} > + > +static inline u32 v4l2_flash_led_brightness_to_intensity( > + struct led_ctrl *config, > + enum led_brightness brightness) > +{ > + return brightness * config->step; And do the opposite here? > +} > + > +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 *led_cdev = v4l2_flash->led_cdev; > + struct led_flash *flash = led_cdev->flash; > + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; > + u32 fault; > + int ret; > + > + switch (c->id) { > + case V4L2_CID_FLASH_TORCH_INTENSITY: > + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { > + ret = v4l2_call_flash_op(brightness_update, led_cdev); > + if (ret < 0) > + return ret; > + ctrl->torch_intensity->val = > + v4l2_flash_led_brightness_to_intensity( > + &led_cdev->brightness_ctrl, > + led_cdev->brightness); > + } > + return 0; > + case V4L2_CID_FLASH_INTENSITY: > + ret = v4l2_call_flash_op(flash_brightness_update, led_cdev); > + if (ret < 0) > + return ret; > + /* no conversion is needed */ > + c->val = flash->brightness.val; > + return 0; > + case V4L2_CID_FLASH_INDICATOR_INTENSITY: > + ret = v4l2_call_flash_op(indicator_brightness_update, led_cdev); > + if (ret < 0) > + return ret; > + /* no conversion is needed */ > + c->val = flash->indicator_brightness->val; > + return 0; > + case V4L2_CID_FLASH_STROBE_STATUS: > + ret = v4l2_call_flash_op(strobe_get, led_cdev); > + if (ret < 0) > + return ret; > + c->val = !!ret; > + return 0; > + case V4L2_CID_FLASH_FAULT: > + /* led faults map directly to V4L2 flash faults */ > + ret = v4l2_call_flash_op(fault_get, led_cdev, &fault); > + if (!ret) The return value seems to come all the way from regmap_read() and the bus related implementatio. I would just consider negative values errors, as above. > + c->val = fault; > + return ret; > + default: > + return -EINVAL; > + } > +} > + > +static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c) > +{ > + struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c); > + struct led_classdev *led_cdev = v4l2_flash->led_cdev; > + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; > + enum led_brightness torch_brightness; > + bool external_strobe; > + int ret; > + > + switch (c->id) { > + case V4L2_CID_FLASH_LED_MODE: > + switch (c->val) { > + case V4L2_FLASH_LED_MODE_NONE: > + v4l2_call_flash_op(brightness_set, led_cdev, 0); > + return v4l2_call_flash_op(strobe_set, led_cdev, false); > + case V4L2_FLASH_LED_MODE_FLASH: > + /* Turn off torch LED */ > + v4l2_call_flash_op(brightness_set, led_cdev, 0); > + external_strobe = (ctrl->source->val == > + V4L2_FLASH_STROBE_SOURCE_EXTERNAL); > + return v4l2_call_flash_op(external_strobe_set, led_cdev, > + external_strobe); > + case V4L2_FLASH_LED_MODE_TORCH: > + /* Stop flash strobing */ > + ret = v4l2_call_flash_op(strobe_set, led_cdev, false); > + if (ret) > + return ret; > + /* torch is always triggered by software */ > + ret = v4l2_call_flash_op(external_strobe_set, led_cdev, > + false); Does the LED API not assume this at the moment? I'm not saying it should, though. :-) > + if (ret) > + return ret; > + > + torch_brightness = > + v4l2_flash_intensity_to_led_brightness( > + &led_cdev->brightness_ctrl, > + ctrl->torch_intensity->val); > + v4l2_call_flash_op(brightness_set, led_cdev, > + torch_brightness); > + return ret; > + } > + break; > + case V4L2_CID_FLASH_STROBE_SOURCE: > + return v4l2_call_flash_op(external_strobe_set, led_cdev, > + c->val == V4L2_FLASH_STROBE_SOURCE_EXTERNAL); > + case V4L2_CID_FLASH_STROBE: > + if (ctrl->led_mode->val != V4L2_FLASH_LED_MODE_FLASH || > + ctrl->source->val != V4L2_FLASH_STROBE_SOURCE_SOFTWARE) I vote for -EBUSY instead. > + return -EINVAL; > + return v4l2_call_flash_op(strobe_set, led_cdev, true); > + case V4L2_CID_FLASH_STROBE_STOP: > + return v4l2_call_flash_op(strobe_set, led_cdev, false); > + case V4L2_CID_FLASH_TIMEOUT: > + ret = v4l2_call_flash_op(timeout_set, led_cdev, c->val); > + case V4L2_CID_FLASH_INTENSITY: > + /* no conversion is needed */ > + return v4l2_call_flash_op(flash_brightness_set, led_cdev, > + c->val); > + case V4L2_CID_FLASH_INDICATOR_INTENSITY: > + /* no conversion is needed */ > + return v4l2_call_flash_op(indicator_brightness_set, led_cdev, > + c->val); > + case V4L2_CID_FLASH_TORCH_INTENSITY: > + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { > + torch_brightness = > + v4l2_flash_intensity_to_led_brightness( > + &led_cdev->brightness_ctrl, > + ctrl->torch_intensity->val); > + v4l2_call_flash_op(brightness_set, led_cdev, > + torch_brightness); I could be missing something but don't torch and indicator require similar handling? > + } > + 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 int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash) > + > +{ > + struct led_classdev *led_cdev = v4l2_flash->led_cdev; > + struct led_flash *flash = led_cdev->flash; > + bool has_indicator = flash->indicator_brightness; > + struct v4l2_ctrl *ctrl; > + struct led_ctrl *ctrl_cfg; > + unsigned int mask; > + int ret, max, num_ctrls; > + > + num_ctrls = flash->has_flash_led ? 8 : 2; > + if (flash->fault_flags) > + ++num_ctrls; > + if (has_indicator) > + ++num_ctrls; > + > + v4l2_ctrl_handler_init(&v4l2_flash->hdl, num_ctrls); > + > + mask = 1 << V4L2_FLASH_LED_MODE_NONE | > + 1 << V4L2_FLASH_LED_MODE_TORCH; > + if (flash->has_flash_led) > + mask |= 1 << V4L2_FLASH_LED_MODE_FLASH; > + > + /* Configure FLASH_LED_MODE ctrl */ > + v4l2_flash->ctrl.led_mode = v4l2_ctrl_new_std_menu( > + &v4l2_flash->hdl, > + &v4l2_flash_ctrl_ops, V4L2_CID_FLASH_LED_MODE, > + V4L2_FLASH_LED_MODE_TORCH, ~mask, > + V4L2_FLASH_LED_MODE_NONE); > + > + /* Configure TORCH_INTENSITY ctrl */ > + ctrl_cfg = &led_cdev->brightness_ctrl; > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_TORCH_INTENSITY, > + ctrl_cfg->min, ctrl_cfg->max, > + ctrl_cfg->step, ctrl_cfg->val); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + v4l2_flash->ctrl.torch_intensity = ctrl; > + > + if (flash->has_flash_led) { > + /* Configure FLASH_STROBE_SOURCE ctrl */ > + mask = 1 << V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > + > + if (flash->has_external_strobe) { > + mask |= 1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL; > + max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL; > + } else { > + max = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > + } > + > + v4l2_flash->ctrl.source = v4l2_ctrl_new_std_menu( > + &v4l2_flash->hdl, > + &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_STROBE_SOURCE, > + max, > + ~mask, > + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); > + > + /* Configure FLASH_STROBE ctrl */ > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_STROBE, 0, 1, 1, 0); > + > + /* Configure FLASH_STROBE_STOP ctrl */ > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_STROBE_STOP, > + 0, 1, 1, 0); > + > + /* Configure FLASH_STROBE_STATUS ctrl */ > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_STROBE_STATUS, > + 0, 1, 1, 1); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | > + V4L2_CTRL_FLAG_READ_ONLY; > + > + /* Configure FLASH_TIMEOUT ctrl */ > + ctrl_cfg = &flash->timeout; > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_TIMEOUT, ctrl_cfg->min, > + ctrl_cfg->max, ctrl_cfg->step, > + ctrl_cfg->val); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + > + /* Configure FLASH_INTENSITY ctrl */ > + ctrl_cfg = &flash->brightness; > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_INTENSITY, > + ctrl_cfg->min, ctrl_cfg->max, > + ctrl_cfg->step, ctrl_cfg->val); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + > + if (flash->fault_flags) { > + /* Configure FLASH_FAULT ctrl */ > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, > + &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_FAULT, 0, > + flash->fault_flags, > + 0, 0); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | > + V4L2_CTRL_FLAG_READ_ONLY; > + } > + if (has_indicator) { In theory it's possible to have an indicator without the flash. So I'd keep the two independent. > + /* Configure FLASH_INDICATOR_INTENSITY ctrl */ > + ctrl_cfg = flash->indicator_brightness; > + ctrl = v4l2_ctrl_new_std( > + &v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_INDICATOR_INTENSITY, > + ctrl_cfg->min, ctrl_cfg->max, > + ctrl_cfg->step, ctrl_cfg->val); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + } > + } > + > + if (v4l2_flash->hdl.error) { > + ret = v4l2_flash->hdl.error; > + goto error_free; > + } > + > + ret = v4l2_ctrl_handler_setup(&v4l2_flash->hdl); > + if (ret < 0) > + goto error_free; > + > + v4l2_flash->subdev.ctrl_handler = &v4l2_flash->hdl; > + > + return 0; > + > +error_free: > + v4l2_ctrl_handler_free(&v4l2_flash->hdl); > + 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 *led_cdev = v4l2_flash->led_cdev; > + > + mutex_lock(&led_cdev->led_lock); > + v4l2_call_flash_op(sysfs_lock, led_cdev); Have you thought about device power management yet? Traditionally, the existing flash controller drivers are powered when a file handle is opened. Many newer implementations seem to consume so little power that it's not worthwhile to try to power them off. Binding the power state to open file handles isn't very generic either but it's very simple and works. V4L2 sub-devices do not use runtime PM as of yet, so perhaps this could be left for later. > + mutex_unlock(&led_cdev->led_lock); > + > + return 0; > +} > + > +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 *led_cdev = v4l2_flash->led_cdev; > + > + mutex_lock(&led_cdev->led_lock); > + v4l2_call_flash_op(sysfs_unlock, led_cdev); > + mutex_unlock(&led_cdev->led_lock); > + > + return 0; > +} > + > +static const struct v4l2_subdev_internal_ops v4l2_flash_subdev_internal_ops = { > + .open = v4l2_flash_open, > + .close = v4l2_flash_close, > +}; > + > +static struct v4l2_subdev_ops v4l2_flash_subdev_ops = { > +}; > + > +int v4l2_flash_init(struct led_classdev *led_cdev, > + const struct v4l2_flash_ops *ops) > +{ > + struct v4l2_flash *flash = &led_cdev->flash->v4l2_flash; > + struct v4l2_subdev *sd = &flash->subdev; > + int ret; > + > + if (!led_cdev || !ops) > + return -EINVAL; > + > + flash->led_cdev = led_cdev; > + sd->dev = led_cdev->dev->parent; > + v4l2_subdev_init(sd, &v4l2_flash_subdev_ops); > + sd->internal_ops = &v4l2_flash_subdev_internal_ops; > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + snprintf(sd->name, sizeof(sd->name), led_cdev->name); It'd be nice to have the bus address in the name as well. But that would belong to the driver which calls v4l2_flash_init(). The existing flash controller drivers probably don't do this but many sensor drivers already do. The name is expected to be unique in the media device. > + flash->ops = ops; > + > + ret = v4l2_flash_init_controls(flash); > + if (ret < 0) > + goto err_init_controls; > + > + ret = media_entity_init(&sd->entity, 0, NULL, 0); > + if (ret < 0) > + goto err_init_entity; > + > + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > + > + ret = v4l2_async_register_subdev(sd); > + if (ret < 0) > + goto err_init_entity; > + > + return 0; > + > +err_init_entity: > + media_entity_cleanup(&sd->entity); > +err_init_controls: > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + return -EINVAL; > +} > +EXPORT_SYMBOL_GPL(v4l2_flash_init); > + > +void v4l2_flash_release(struct led_classdev *led_cdev) > +{ > + struct v4l2_flash *flash = &led_cdev->flash->v4l2_flash; > + > + v4l2_ctrl_handler_free(flash->subdev.ctrl_handler); > + v4l2_async_unregister_subdev(&flash->subdev); > + media_entity_cleanup(&flash->subdev.entity); > +} > +EXPORT_SYMBOL_GPL(v4l2_flash_release); > diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h > new file mode 100644 > index 0000000..fe16ddd > --- /dev/null > +++ b/include/media/v4l2-flash.h > @@ -0,0 +1,119 @@ > +/* > + * V4L2 Flash LED sub-device registration helpers. > + * > + * Copyright (C) 2014 Samsung Electronics Co., Ltd > + * Author: Jacek Anaszewski <j.anaszewski@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." > + */ > + > +#ifndef _V4L2_FLASH_H > +#define _V4L2_FLASH_H > + > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-dev.h> > +#include <media/v4l2-event.h> > +#include <media/v4l2-ioctl.h> > + > +#define v4l2_call_flash_op(op, args...) \ > + ((v4l2_flash)->ops->op(args)) \ Extra backslash above. I don't think you should assume the caller will have a local variable called v4l2_flash. :-) > +struct led_classdev; > +enum led_brightness; > + > +struct v4l2_flash_ops { > + void (*brightness_set)(struct led_classdev *led_cdev, > + enum led_brightness brightness); > + int (*brightness_update)(struct led_classdev *led_cdev); > + int (*flash_brightness_set)(struct led_classdev *led_cdev, > + u32 brightness); > + int (*flash_brightness_update)(struct led_classdev *led_cdev); > + int (*strobe_set)(struct led_classdev *led_cdev, > + bool state); > + int (*strobe_get)(struct led_classdev *led_cdev); > + int (*timeout_set)(struct led_classdev *led_cdev, > + u32 timeout); > + int (*indicator_brightness_set)(struct led_classdev *led_cdev, > + u32 brightness); > + int (*indicator_brightness_update)(struct led_classdev *led_cdev); > + int (*external_strobe_set)(struct led_classdev *led_cdev, > + bool enable); > + int (*fault_get)(struct led_classdev *led_cdev, > + u32 *fault); > + void (*sysfs_lock)(struct led_classdev *led_cdev); > + void (*sysfs_unlock)(struct led_classdev *led_cdev); > +}; > + > +/** > + * struct v4l2_flash_ctrl - controls that define the sub-dev's state > + * @source: V4L2_CID_FLASH_STROBE_SOURCE control > + * @led_mode: V4L2_CID_FLASH_LED_MODE control > + * @torch_intensity: V4L2_CID_FLASH_TORCH_INTENSITY control > + */ > +struct v4l2_flash_ctrl { > + struct v4l2_ctrl *source; > + struct v4l2_ctrl *led_mode; > + struct v4l2_ctrl *torch_intensity; > +}; > + > +/** > + * struct v4l2_flash - Flash sub-device context > + * @led_cdev: LED class device controlled by this sub-device > + * @ops: LED class device ops > + * @subdev: V4L2 sub-device > + * @hdl: flash controls handler > + * @ctrl: state defining controls > + */ > +struct v4l2_flash { > + struct led_classdev *led_cdev; > + const struct v4l2_flash_ops *ops; > + > + struct v4l2_subdev subdev; > + struct v4l2_ctrl_handler hdl; > + struct v4l2_flash_ctrl ctrl; > +}; > + > +static inline struct v4l2_flash *v4l2_subdev_to_v4l2_flash(struct v4l2_subdev *sd) Over 80 characters per line. > +{ > + return container_of(sd, struct v4l2_flash, subdev); > +} > + > +static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c) > +{ > + return container_of(c->handler, struct v4l2_flash, hdl); > +} > + > +#ifdef CONFIG_V4L2_FLASH > +/** > + * v4l2_flash_init - initialize V4L2 flash led sub-device > + * @led_cdev: the LED to create subdev upon > + * @ops: LED subsystem callbacks > + * > + * Create V4L2 subdev wrapping given LED subsystem device. > + */ > +int v4l2_flash_init(struct led_classdev *led_cdev, > + const struct v4l2_flash_ops *ops); > + > +/** > + * v4l2_flash_release - release V4L2 flash led sub-device > + * @flash: a structure representing V4L2 flash led device > + * > + * Release V4L2 flash led subdev. > + */ > +void v4l2_flash_release(struct led_classdev *led_cdev); > +#else > +static inline int v4l2_flash_init(struct led_classdev *led_cdev, > + const struct v4l2_flash_ops *ops) > +{ > + return 0; > +} > + > +static inline void v4l2_flash_release(struct led_classdev *led_cdev) > +{ > +} Could you put the two to the first patch? It won't compile otherwise. > +#endif /* CONFIG_V4L2_FLASH */ > + > +#endif /* _V4L2_FLASH_H */
Hi Sakari, Thanks for the review. On 04/16/2014 08:21 PM, Sakari Ailus wrote: > Hi Jacek, > > Thanks for the update! > [...] >> +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( >> + struct led_ctrl *config, >> + u32 intensity) > > Fits on a single line. > >> +{ >> + return intensity / config->step; > > Shouldn't you first decrement the minimum before the division? Brightness level 0 means that led is off. Let's consider following case: intensity - 15625 config->step - 15625 intensity / config->step = 1 (the lowest possible current level) >> +} >> + >> +static inline u32 v4l2_flash_led_brightness_to_intensity( >> + struct led_ctrl *config, >> + enum led_brightness brightness) >> +{ >> + return brightness * config->step; > > And do the opposite here? > >> +} >> + >> +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 *led_cdev = v4l2_flash->led_cdev; >> + struct led_flash *flash = led_cdev->flash; >> + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; >> + u32 fault; >> + int ret; >> + >> + switch (c->id) { >> + case V4L2_CID_FLASH_TORCH_INTENSITY: >> + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { >> + ret = v4l2_call_flash_op(brightness_update, led_cdev); >> + if (ret < 0) >> + return ret; >> + ctrl->torch_intensity->val = >> + v4l2_flash_led_brightness_to_intensity( >> + &led_cdev->brightness_ctrl, >> + led_cdev->brightness); >> + } >> + return 0; >> + case V4L2_CID_FLASH_INTENSITY: >> + ret = v4l2_call_flash_op(flash_brightness_update, led_cdev); >> + if (ret < 0) >> + return ret; >> + /* no conversion is needed */ >> + c->val = flash->brightness.val; >> + return 0; >> + case V4L2_CID_FLASH_INDICATOR_INTENSITY: >> + ret = v4l2_call_flash_op(indicator_brightness_update, led_cdev); >> + if (ret < 0) >> + return ret; >> + /* no conversion is needed */ >> + c->val = flash->indicator_brightness->val; >> + return 0; >> + case V4L2_CID_FLASH_STROBE_STATUS: >> + ret = v4l2_call_flash_op(strobe_get, led_cdev); >> + if (ret < 0) >> + return ret; >> + c->val = !!ret; >> + return 0; >> + case V4L2_CID_FLASH_FAULT: >> + /* led faults map directly to V4L2 flash faults */ >> + ret = v4l2_call_flash_op(fault_get, led_cdev, &fault); >> + if (!ret) > > The return value seems to come all the way from regmap_read() and the bus > related implementatio. I would just consider negative values errors, as > above. I think that ret would be returned if it wasn't equal to 0. But indeed it should be done the same way as for the other cases. >> + c->val = fault; >> + return ret; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c) >> +{ >> + struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c); >> + struct led_classdev *led_cdev = v4l2_flash->led_cdev; >> + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; >> + enum led_brightness torch_brightness; >> + bool external_strobe; >> + int ret; >> + >> + switch (c->id) { >> + case V4L2_CID_FLASH_LED_MODE: >> + switch (c->val) { >> + case V4L2_FLASH_LED_MODE_NONE: >> + v4l2_call_flash_op(brightness_set, led_cdev, 0); >> + return v4l2_call_flash_op(strobe_set, led_cdev, false); >> + case V4L2_FLASH_LED_MODE_FLASH: >> + /* Turn off torch LED */ >> + v4l2_call_flash_op(brightness_set, led_cdev, 0); >> + external_strobe = (ctrl->source->val == >> + V4L2_FLASH_STROBE_SOURCE_EXTERNAL); >> + return v4l2_call_flash_op(external_strobe_set, led_cdev, >> + external_strobe); >> + case V4L2_FLASH_LED_MODE_TORCH: >> + /* Stop flash strobing */ >> + ret = v4l2_call_flash_op(strobe_set, led_cdev, false); >> + if (ret) >> + return ret; >> + /* torch is always triggered by software */ >> + ret = v4l2_call_flash_op(external_strobe_set, led_cdev, >> + false); > > Does the LED API not assume this at the moment? I'm not saying it should, > though. :-) Actually strobe source should apply only to flash leds. I will remove this call. >> + if (ret) >> + return ret; >> + >> + torch_brightness = >> + v4l2_flash_intensity_to_led_brightness( >> + &led_cdev->brightness_ctrl, >> + ctrl->torch_intensity->val); >> + v4l2_call_flash_op(brightness_set, led_cdev, >> + torch_brightness); >> + return ret; >> + } >> + break; >> + case V4L2_CID_FLASH_STROBE_SOURCE: >> + return v4l2_call_flash_op(external_strobe_set, led_cdev, >> + c->val == V4L2_FLASH_STROBE_SOURCE_EXTERNAL); >> + case V4L2_CID_FLASH_STROBE: >> + if (ctrl->led_mode->val != V4L2_FLASH_LED_MODE_FLASH || >> + ctrl->source->val != V4L2_FLASH_STROBE_SOURCE_SOFTWARE) > > I vote for -EBUSY instead. I agree. >> + return -EINVAL; >> + return v4l2_call_flash_op(strobe_set, led_cdev, true); >> + case V4L2_CID_FLASH_STROBE_STOP: >> + return v4l2_call_flash_op(strobe_set, led_cdev, false); >> + case V4L2_CID_FLASH_TIMEOUT: >> + ret = v4l2_call_flash_op(timeout_set, led_cdev, c->val); >> + case V4L2_CID_FLASH_INTENSITY: >> + /* no conversion is needed */ >> + return v4l2_call_flash_op(flash_brightness_set, led_cdev, >> + c->val); >> + case V4L2_CID_FLASH_INDICATOR_INTENSITY: >> + /* no conversion is needed */ >> + return v4l2_call_flash_op(indicator_brightness_set, led_cdev, >> + c->val); >> + case V4L2_CID_FLASH_TORCH_INTENSITY: >> + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { >> + torch_brightness = >> + v4l2_flash_intensity_to_led_brightness( >> + &led_cdev->brightness_ctrl, >> + ctrl->torch_intensity->val); >> + v4l2_call_flash_op(brightness_set, led_cdev, >> + torch_brightness); > > I could be missing something but don't torch and indicator require similar > handling? Why? Torch units need conversion whereas indicator units don't. Moreover they have different LED API. >> + } >> + 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 int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash) >> + >> +{ >> + struct led_classdev *led_cdev = v4l2_flash->led_cdev; >> + struct led_flash *flash = led_cdev->flash; >> + bool has_indicator = flash->indicator_brightness; >> + struct v4l2_ctrl *ctrl; >> + struct led_ctrl *ctrl_cfg; >> + unsigned int mask; >> + int ret, max, num_ctrls; >> + >> + num_ctrls = flash->has_flash_led ? 8 : 2; >> + if (flash->fault_flags) >> + ++num_ctrls; >> + if (has_indicator) >> + ++num_ctrls; >> + >> + v4l2_ctrl_handler_init(&v4l2_flash->hdl, num_ctrls); >> + >> + mask = 1 << V4L2_FLASH_LED_MODE_NONE | >> + 1 << V4L2_FLASH_LED_MODE_TORCH; >> + if (flash->has_flash_led) >> + mask |= 1 << V4L2_FLASH_LED_MODE_FLASH; >> + >> + /* Configure FLASH_LED_MODE ctrl */ >> + v4l2_flash->ctrl.led_mode = v4l2_ctrl_new_std_menu( >> + &v4l2_flash->hdl, >> + &v4l2_flash_ctrl_ops, V4L2_CID_FLASH_LED_MODE, >> + V4L2_FLASH_LED_MODE_TORCH, ~mask, >> + V4L2_FLASH_LED_MODE_NONE); >> + >> + /* Configure TORCH_INTENSITY ctrl */ >> + ctrl_cfg = &led_cdev->brightness_ctrl; >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_TORCH_INTENSITY, >> + ctrl_cfg->min, ctrl_cfg->max, >> + ctrl_cfg->step, ctrl_cfg->val); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; >> + v4l2_flash->ctrl.torch_intensity = ctrl; >> + >> + if (flash->has_flash_led) { >> + /* Configure FLASH_STROBE_SOURCE ctrl */ >> + mask = 1 << V4L2_FLASH_STROBE_SOURCE_SOFTWARE; >> + >> + if (flash->has_external_strobe) { >> + mask |= 1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL; >> + max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL; >> + } else { >> + max = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; >> + } >> + >> + v4l2_flash->ctrl.source = v4l2_ctrl_new_std_menu( >> + &v4l2_flash->hdl, >> + &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_STROBE_SOURCE, >> + max, >> + ~mask, >> + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); >> + >> + /* Configure FLASH_STROBE ctrl */ >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_STROBE, 0, 1, 1, 0); >> + >> + /* Configure FLASH_STROBE_STOP ctrl */ >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_STROBE_STOP, >> + 0, 1, 1, 0); >> + >> + /* Configure FLASH_STROBE_STATUS ctrl */ >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_STROBE_STATUS, >> + 0, 1, 1, 1); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | >> + V4L2_CTRL_FLAG_READ_ONLY; >> + >> + /* Configure FLASH_TIMEOUT ctrl */ >> + ctrl_cfg = &flash->timeout; >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_TIMEOUT, ctrl_cfg->min, >> + ctrl_cfg->max, ctrl_cfg->step, >> + ctrl_cfg->val); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; >> + >> + /* Configure FLASH_INTENSITY ctrl */ >> + ctrl_cfg = &flash->brightness; >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_INTENSITY, >> + ctrl_cfg->min, ctrl_cfg->max, >> + ctrl_cfg->step, ctrl_cfg->val); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; >> + >> + if (flash->fault_flags) { >> + /* Configure FLASH_FAULT ctrl */ >> + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, >> + &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_FAULT, 0, >> + flash->fault_flags, >> + 0, 0); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | >> + V4L2_CTRL_FLAG_READ_ONLY; >> + } >> + if (has_indicator) { > > In theory it's possible to have an indicator without the flash. So I'd keep > the two independent. OK. >> + /* Configure FLASH_INDICATOR_INTENSITY ctrl */ >> + ctrl_cfg = flash->indicator_brightness; >> + ctrl = v4l2_ctrl_new_std( >> + &v4l2_flash->hdl, &v4l2_flash_ctrl_ops, >> + V4L2_CID_FLASH_INDICATOR_INTENSITY, >> + ctrl_cfg->min, ctrl_cfg->max, >> + ctrl_cfg->step, ctrl_cfg->val); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; >> + } >> + } >> + >> + if (v4l2_flash->hdl.error) { >> + ret = v4l2_flash->hdl.error; >> + goto error_free; >> + } >> + >> + ret = v4l2_ctrl_handler_setup(&v4l2_flash->hdl); >> + if (ret < 0) >> + goto error_free; >> + >> + v4l2_flash->subdev.ctrl_handler = &v4l2_flash->hdl; >> + >> + return 0; >> + >> +error_free: >> + v4l2_ctrl_handler_free(&v4l2_flash->hdl); >> + 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 *led_cdev = v4l2_flash->led_cdev; >> + >> + mutex_lock(&led_cdev->led_lock); >> + v4l2_call_flash_op(sysfs_lock, led_cdev); > > Have you thought about device power management yet? Having in mind that the V4L2 Flash sub-device is only a wrapper for LED driver, shouldn't power management be left to the drivers? > Traditionally, the existing flash controller drivers are powered when a file > handle is opened. Many newer implementations seem to consume so little power > that it's not worthwhile to try to power them off. > > Binding the power state to open file handles isn't very generic either but > it's very simple and works. > > V4L2 sub-devices do not use runtime PM as of yet, so perhaps this could be > left for later. > >> + mutex_unlock(&led_cdev->led_lock); >> + >> + return 0; >> +} >> + >> +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 *led_cdev = v4l2_flash->led_cdev; >> + >> + mutex_lock(&led_cdev->led_lock); >> + v4l2_call_flash_op(sysfs_unlock, led_cdev); >> + mutex_unlock(&led_cdev->led_lock); >> + >> + return 0; >> +} >> + >> +static const struct v4l2_subdev_internal_ops v4l2_flash_subdev_internal_ops = { >> + .open = v4l2_flash_open, >> + .close = v4l2_flash_close, >> +}; >> + >> +static struct v4l2_subdev_ops v4l2_flash_subdev_ops = { >> +}; >> + >> +int v4l2_flash_init(struct led_classdev *led_cdev, >> + const struct v4l2_flash_ops *ops) >> +{ >> + struct v4l2_flash *flash = &led_cdev->flash->v4l2_flash; >> + struct v4l2_subdev *sd = &flash->subdev; >> + int ret; >> + >> + if (!led_cdev || !ops) >> + return -EINVAL; >> + >> + flash->led_cdev = led_cdev; >> + sd->dev = led_cdev->dev->parent; >> + v4l2_subdev_init(sd, &v4l2_flash_subdev_ops); >> + sd->internal_ops = &v4l2_flash_subdev_internal_ops; >> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >> + snprintf(sd->name, sizeof(sd->name), led_cdev->name); > > It'd be nice to have the bus address in the name as well. But that would > belong to the driver which calls v4l2_flash_init(). The existing flash > controller drivers probably don't do this but many sensor drivers already > do. The name is expected to be unique in the media device. Will cover this. >> + flash->ops = ops; >> + >> + ret = v4l2_flash_init_controls(flash); >> + if (ret < 0) >> + goto err_init_controls; >> + >> + ret = media_entity_init(&sd->entity, 0, NULL, 0); >> + if (ret < 0) >> + goto err_init_entity; >> + >> + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; >> + >> + ret = v4l2_async_register_subdev(sd); >> + if (ret < 0) >> + goto err_init_entity; >> + >> + return 0; >> + >> +err_init_entity: >> + media_entity_cleanup(&sd->entity); >> +err_init_controls: >> + v4l2_ctrl_handler_free(sd->ctrl_handler); >> + return -EINVAL; >> +} >> +EXPORT_SYMBOL_GPL(v4l2_flash_init); >> + >> +void v4l2_flash_release(struct led_classdev *led_cdev) >> +{ >> + struct v4l2_flash *flash = &led_cdev->flash->v4l2_flash; >> + >> + v4l2_ctrl_handler_free(flash->subdev.ctrl_handler); >> + v4l2_async_unregister_subdev(&flash->subdev); >> + media_entity_cleanup(&flash->subdev.entity); >> +} >> +EXPORT_SYMBOL_GPL(v4l2_flash_release); >> diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h >> new file mode 100644 >> index 0000000..fe16ddd >> --- /dev/null >> +++ b/include/media/v4l2-flash.h >> @@ -0,0 +1,119 @@ >> +/* >> + * V4L2 Flash LED sub-device registration helpers. >> + * >> + * Copyright (C) 2014 Samsung Electronics Co., Ltd >> + * Author: Jacek Anaszewski <j.anaszewski@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." >> + */ >> + >> +#ifndef _V4L2_FLASH_H >> +#define _V4L2_FLASH_H >> + >> +#include <media/v4l2-ctrls.h> >> +#include <media/v4l2-device.h> >> +#include <media/v4l2-dev.h> >> +#include <media/v4l2-event.h> >> +#include <media/v4l2-ioctl.h> >> + >> +#define v4l2_call_flash_op(op, args...) \ >> + ((v4l2_flash)->ops->op(args)) \ > > Extra backslash above. > > I don't think you should assume the caller will have a local variable called > v4l2_flash. :-) I assumed that as this macro is only for the module internal use and aim at simplifying the calling code it can be implemented that way. But when I look at it now it indeed doesn't seem reasonable. >> +struct led_classdev; >> +enum led_brightness; >> + >> +struct v4l2_flash_ops { >> + void (*brightness_set)(struct led_classdev *led_cdev, >> + enum led_brightness brightness); >> + int (*brightness_update)(struct led_classdev *led_cdev); >> + int (*flash_brightness_set)(struct led_classdev *led_cdev, >> + u32 brightness); >> + int (*flash_brightness_update)(struct led_classdev *led_cdev); >> + int (*strobe_set)(struct led_classdev *led_cdev, >> + bool state); >> + int (*strobe_get)(struct led_classdev *led_cdev); >> + int (*timeout_set)(struct led_classdev *led_cdev, >> + u32 timeout); >> + int (*indicator_brightness_set)(struct led_classdev *led_cdev, >> + u32 brightness); >> + int (*indicator_brightness_update)(struct led_classdev *led_cdev); >> + int (*external_strobe_set)(struct led_classdev *led_cdev, >> + bool enable); >> + int (*fault_get)(struct led_classdev *led_cdev, >> + u32 *fault); >> + void (*sysfs_lock)(struct led_classdev *led_cdev); >> + void (*sysfs_unlock)(struct led_classdev *led_cdev); >> +}; >> + >> +/** >> + * struct v4l2_flash_ctrl - controls that define the sub-dev's state >> + * @source: V4L2_CID_FLASH_STROBE_SOURCE control >> + * @led_mode: V4L2_CID_FLASH_LED_MODE control >> + * @torch_intensity: V4L2_CID_FLASH_TORCH_INTENSITY control >> + */ >> +struct v4l2_flash_ctrl { >> + struct v4l2_ctrl *source; >> + struct v4l2_ctrl *led_mode; >> + struct v4l2_ctrl *torch_intensity; >> +}; >> + >> +/** >> + * struct v4l2_flash - Flash sub-device context >> + * @led_cdev: LED class device controlled by this sub-device >> + * @ops: LED class device ops >> + * @subdev: V4L2 sub-device >> + * @hdl: flash controls handler >> + * @ctrl: state defining controls >> + */ >> +struct v4l2_flash { >> + struct led_classdev *led_cdev; >> + const struct v4l2_flash_ops *ops; >> + >> + struct v4l2_subdev subdev; >> + struct v4l2_ctrl_handler hdl; >> + struct v4l2_flash_ctrl ctrl; >> +}; >> + >> +static inline struct v4l2_flash *v4l2_subdev_to_v4l2_flash(struct v4l2_subdev *sd) > > Over 80 characters per line. > >> +{ >> + return container_of(sd, struct v4l2_flash, subdev); >> +} >> + >> +static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c) >> +{ >> + return container_of(c->handler, struct v4l2_flash, hdl); >> +} >> + >> +#ifdef CONFIG_V4L2_FLASH >> +/** >> + * v4l2_flash_init - initialize V4L2 flash led sub-device >> + * @led_cdev: the LED to create subdev upon >> + * @ops: LED subsystem callbacks >> + * >> + * Create V4L2 subdev wrapping given LED subsystem device. >> + */ >> +int v4l2_flash_init(struct led_classdev *led_cdev, >> + const struct v4l2_flash_ops *ops); >> + >> +/** >> + * v4l2_flash_release - release V4L2 flash led sub-device >> + * @flash: a structure representing V4L2 flash led device >> + * >> + * Release V4L2 flash led subdev. >> + */ >> +void v4l2_flash_release(struct led_classdev *led_cdev); >> +#else >> +static inline int v4l2_flash_init(struct led_classdev *led_cdev, >> + const struct v4l2_flash_ops *ops) >> +{ >> + return 0; >> +} >> + >> +static inline void v4l2_flash_release(struct led_classdev *led_cdev) >> +{ >> +} > > Could you put the two to the first patch? It won't compile otherwise. Yeah. I didn't make test build after every patch :-/. >> +#endif /* CONFIG_V4L2_FLASH */ >> + >> +#endif /* _V4L2_FLASH_H */ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Thu, Apr 17, 2014 at 10:26:44AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. > > On 04/16/2014 08:21 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thanks for the update! > > > [...] > >>+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > >>+ struct led_ctrl *config, > >>+ u32 intensity) > > > >Fits on a single line. > > > >>+{ > >>+ return intensity / config->step; > > > >Shouldn't you first decrement the minimum before the division? > > Brightness level 0 means that led is off. Let's consider following case: > > intensity - 15625 > config->step - 15625 > intensity / config->step = 1 (the lowest possible current level) In V4L2 controls the minimum is not off, and zero might not be a possible value since minimum isn't divisible by step. I wonder how to best take that into account. > >>+} > >>+ > >>+static inline u32 v4l2_flash_led_brightness_to_intensity( > >>+ struct led_ctrl *config, > >>+ enum led_brightness brightness) > >>+{ > >>+ return brightness * config->step; > > > >And do the opposite here? .. > >>+ return -EINVAL; > >>+ return v4l2_call_flash_op(strobe_set, led_cdev, true); > >>+ case V4L2_CID_FLASH_STROBE_STOP: > >>+ return v4l2_call_flash_op(strobe_set, led_cdev, false); > >>+ case V4L2_CID_FLASH_TIMEOUT: > >>+ ret = v4l2_call_flash_op(timeout_set, led_cdev, c->val); > >>+ case V4L2_CID_FLASH_INTENSITY: > >>+ /* no conversion is needed */ > >>+ return v4l2_call_flash_op(flash_brightness_set, led_cdev, > >>+ c->val); > >>+ case V4L2_CID_FLASH_INDICATOR_INTENSITY: > >>+ /* no conversion is needed */ > >>+ return v4l2_call_flash_op(indicator_brightness_set, led_cdev, > >>+ c->val); > >>+ case V4L2_CID_FLASH_TORCH_INTENSITY: > >>+ if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { > >>+ torch_brightness = > >>+ v4l2_flash_intensity_to_led_brightness( > >>+ &led_cdev->brightness_ctrl, > >>+ ctrl->torch_intensity->val); > >>+ v4l2_call_flash_op(brightness_set, led_cdev, > >>+ torch_brightness); > > > >I could be missing something but don't torch and indicator require similar > >handling? > > Why? Torch units need conversion whereas indicator units don't. > Moreover they have different LED API. I missed it was already in micro-Amps. > >>+ } > >>+ 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 int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash) > >>+ > >>+{ > >>+ struct led_classdev *led_cdev = v4l2_flash->led_cdev; > >>+ struct led_flash *flash = led_cdev->flash; > >>+ bool has_indicator = flash->indicator_brightness; > >>+ struct v4l2_ctrl *ctrl; > >>+ struct led_ctrl *ctrl_cfg; > >>+ unsigned int mask; > >>+ int ret, max, num_ctrls; > >>+ > >>+ num_ctrls = flash->has_flash_led ? 8 : 2; > >>+ if (flash->fault_flags) > >>+ ++num_ctrls; > >>+ if (has_indicator) > >>+ ++num_ctrls; > >>+ > >>+ v4l2_ctrl_handler_init(&v4l2_flash->hdl, num_ctrls); > >>+ > >>+ mask = 1 << V4L2_FLASH_LED_MODE_NONE | > >>+ 1 << V4L2_FLASH_LED_MODE_TORCH; > >>+ if (flash->has_flash_led) > >>+ mask |= 1 << V4L2_FLASH_LED_MODE_FLASH; > >>+ > >>+ /* Configure FLASH_LED_MODE ctrl */ > >>+ v4l2_flash->ctrl.led_mode = v4l2_ctrl_new_std_menu( > >>+ &v4l2_flash->hdl, > >>+ &v4l2_flash_ctrl_ops, V4L2_CID_FLASH_LED_MODE, > >>+ V4L2_FLASH_LED_MODE_TORCH, ~mask, > >>+ V4L2_FLASH_LED_MODE_NONE); > >>+ > >>+ /* Configure TORCH_INTENSITY ctrl */ > >>+ ctrl_cfg = &led_cdev->brightness_ctrl; > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_TORCH_INTENSITY, > >>+ ctrl_cfg->min, ctrl_cfg->max, > >>+ ctrl_cfg->step, ctrl_cfg->val); > >>+ if (ctrl) > >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > >>+ v4l2_flash->ctrl.torch_intensity = ctrl; > >>+ > >>+ if (flash->has_flash_led) { > >>+ /* Configure FLASH_STROBE_SOURCE ctrl */ > >>+ mask = 1 << V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > >>+ > >>+ if (flash->has_external_strobe) { > >>+ mask |= 1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL; > >>+ max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL; > >>+ } else { > >>+ max = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > >>+ } > >>+ > >>+ v4l2_flash->ctrl.source = v4l2_ctrl_new_std_menu( > >>+ &v4l2_flash->hdl, > >>+ &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_STROBE_SOURCE, > >>+ max, > >>+ ~mask, > >>+ V4L2_FLASH_STROBE_SOURCE_SOFTWARE); > >>+ > >>+ /* Configure FLASH_STROBE ctrl */ > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_STROBE, 0, 1, 1, 0); > >>+ > >>+ /* Configure FLASH_STROBE_STOP ctrl */ > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_STROBE_STOP, > >>+ 0, 1, 1, 0); > >>+ > >>+ /* Configure FLASH_STROBE_STATUS ctrl */ > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_STROBE_STATUS, > >>+ 0, 1, 1, 1); > >>+ if (ctrl) > >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | > >>+ V4L2_CTRL_FLAG_READ_ONLY; > >>+ > >>+ /* Configure FLASH_TIMEOUT ctrl */ > >>+ ctrl_cfg = &flash->timeout; > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_TIMEOUT, ctrl_cfg->min, > >>+ ctrl_cfg->max, ctrl_cfg->step, > >>+ ctrl_cfg->val); > >>+ if (ctrl) > >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > >>+ > >>+ /* Configure FLASH_INTENSITY ctrl */ > >>+ ctrl_cfg = &flash->brightness; > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_INTENSITY, > >>+ ctrl_cfg->min, ctrl_cfg->max, > >>+ ctrl_cfg->step, ctrl_cfg->val); > >>+ if (ctrl) > >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > >>+ > >>+ if (flash->fault_flags) { > >>+ /* Configure FLASH_FAULT ctrl */ > >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, > >>+ &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_FAULT, 0, > >>+ flash->fault_flags, > >>+ 0, 0); > >>+ if (ctrl) > >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | > >>+ V4L2_CTRL_FLAG_READ_ONLY; > >>+ } > >>+ if (has_indicator) { > > > >In theory it's possible to have an indicator without the flash. So I'd keep > >the two independent. > > OK. > > >>+ /* Configure FLASH_INDICATOR_INTENSITY ctrl */ > >>+ ctrl_cfg = flash->indicator_brightness; > >>+ ctrl = v4l2_ctrl_new_std( > >>+ &v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > >>+ V4L2_CID_FLASH_INDICATOR_INTENSITY, > >>+ ctrl_cfg->min, ctrl_cfg->max, > >>+ ctrl_cfg->step, ctrl_cfg->val); > >>+ if (ctrl) > >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > >>+ } > >>+ } > >>+ > >>+ if (v4l2_flash->hdl.error) { > >>+ ret = v4l2_flash->hdl.error; > >>+ goto error_free; > >>+ } > >>+ > >>+ ret = v4l2_ctrl_handler_setup(&v4l2_flash->hdl); > >>+ if (ret < 0) > >>+ goto error_free; > >>+ > >>+ v4l2_flash->subdev.ctrl_handler = &v4l2_flash->hdl; > >>+ > >>+ return 0; > >>+ > >>+error_free: > >>+ v4l2_ctrl_handler_free(&v4l2_flash->hdl); > >>+ 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 *led_cdev = v4l2_flash->led_cdev; > >>+ > >>+ mutex_lock(&led_cdev->led_lock); > >>+ v4l2_call_flash_op(sysfs_lock, led_cdev); > > > >Have you thought about device power management yet? > > Having in mind that the V4L2 Flash sub-device is only a wrapper > for LED driver, shouldn't power management be left to the > drivers? How does the LED controller driver know it needs to power the device up in that case? I think an s_power() op which uses PM runtime to set the power state until V4L2 sub-device switches to it should be enough. But I'm fine leaving it out from this patchset.
Hi Sakari, On 04/23/2014 05:24 PM, Sakari Ailus wrote: > Hi Jacek, > > On Thu, Apr 17, 2014 at 10:26:44AM +0200, Jacek Anaszewski wrote: >> Hi Sakari, >> >> Thanks for the review. >> >> On 04/16/2014 08:21 PM, Sakari Ailus wrote: >>> Hi Jacek, >>> >>> Thanks for the update! >>> >> [...] >>>> +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( >>>> + struct led_ctrl *config, >>>> + u32 intensity) >>> >>> Fits on a single line. >>> >>>> +{ >>>> + return intensity / config->step; >>> >>> Shouldn't you first decrement the minimum before the division? >> >> Brightness level 0 means that led is off. Let's consider following case: >> >> intensity - 15625 >> config->step - 15625 >> intensity / config->step = 1 (the lowest possible current level) > > In V4L2 controls the minimum is not off, and zero might not be a possible > value since minimum isn't divisible by step. > > I wonder how to best take that into account. I've assumed that in MODE_TORCH a led is always on. Switching the mode to MODE_FLASH or MODE_OFF turns the led off. This way we avoid the problem with converting 0 uA value to led_brightness, as available torch brightness levels start from the minimum current level value and turning the led off is accomplished on transition to MODE_OFF or MODE_FLASH, by calling brightness_set op with led_brightness = 0. [...] >>>> +/* >>>> + * 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 *led_cdev = v4l2_flash->led_cdev; >>>> + >>>> + mutex_lock(&led_cdev->led_lock); >>>> + v4l2_call_flash_op(sysfs_lock, led_cdev); >>> >>> Have you thought about device power management yet? >> >> Having in mind that the V4L2 Flash sub-device is only a wrapper >> for LED driver, shouldn't power management be left to the >> drivers? > > How does the LED controller driver know it needs to power the device up in > that case? > > I think an s_power() op which uses PM runtime to set the power state until > V4L2 sub-device switches to it should be enough. But I'm fine leaving it out > from this patchset. > This solution looks reasonable. Regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Mon, Apr 28, 2014 at 01:25:09PM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 04/23/2014 05:24 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Thu, Apr 17, 2014 at 10:26:44AM +0200, Jacek Anaszewski wrote: > >>Hi Sakari, > >> > >>Thanks for the review. > >> > >>On 04/16/2014 08:21 PM, Sakari Ailus wrote: > >>>Hi Jacek, > >>> > >>>Thanks for the update! > >>> > >>[...] > >>>>+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > >>>>+ struct led_ctrl *config, > >>>>+ u32 intensity) > >>> > >>>Fits on a single line. > >>> > >>>>+{ > >>>>+ return intensity / config->step; > >>> > >>>Shouldn't you first decrement the minimum before the division? > >> > >>Brightness level 0 means that led is off. Let's consider following case: > >> > >>intensity - 15625 > >>config->step - 15625 > >>intensity / config->step = 1 (the lowest possible current level) > > > >In V4L2 controls the minimum is not off, and zero might not be a possible > >value since minimum isn't divisible by step. > > > >I wonder how to best take that into account. > > I've assumed that in MODE_TORCH a led is always on. Switching > the mode to MODE_FLASH or MODE_OFF turns the led off. > This way we avoid the problem with converting 0 uA value to > led_brightness, as available torch brightness levels start from > the minimum current level value and turning the led off is > accomplished on transition to MODE_OFF or MODE_FLASH, by > calling brightness_set op with led_brightness = 0. I'm not sure if we understood the issue the same way. My concern was that if the intensity isn't a multiple of step (but intensity - min is), the above formula won't return a valid result (unless I miss something).
Hi Sakari, On 05/02/2014 01:06 PM, Sakari Ailus wrote: >>>> [...] >>>>>> +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( >>>>>> + struct led_ctrl *config, >>>>>> + u32 intensity) >>>>> >>>>> Fits on a single line. >>>>> >>>>>> +{ >>>>>> + return intensity / config->step; >>>>> >>>>> Shouldn't you first decrement the minimum before the division? >>>> >>>> Brightness level 0 means that led is off. Let's consider following case: >>>> >>>> intensity - 15625 >>>> config->step - 15625 >>>> intensity / config->step = 1 (the lowest possible current level) >>> >>> In V4L2 controls the minimum is not off, and zero might not be a possible >>> value since minimum isn't divisible by step. >>> >>> I wonder how to best take that into account. >> >> I've assumed that in MODE_TORCH a led is always on. Switching >> the mode to MODE_FLASH or MODE_OFF turns the led off. >> This way we avoid the problem with converting 0 uA value to >> led_brightness, as available torch brightness levels start from >> the minimum current level value and turning the led off is >> accomplished on transition to MODE_OFF or MODE_FLASH, by >> calling brightness_set op with led_brightness = 0. > > I'm not sure if we understood the issue the same way. My concern was that if > the intensity isn't a multiple of step (but intensity - min is), the above > formula won't return a valid result (unless I miss something). > Please note that v4l2_flash_intensity_to_led_brightness is called only from s_ctrl callback, and thus it expects to get the intensity aligned to the step value, so it will always be a multiple of step. Is it possible that s_ctrl callback would be passed a non-aligned control value? Regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 05/02/2014 01:06 PM, Sakari Ailus wrote: > > >>>>[...] > >>>>>>+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > >>>>>>+ struct led_ctrl *config, > >>>>>>+ u32 intensity) > >>>>> > >>>>>Fits on a single line. > >>>>> > >>>>>>+{ > >>>>>>+ return intensity / config->step; > >>>>> > >>>>>Shouldn't you first decrement the minimum before the division? > >>>> > >>>>Brightness level 0 means that led is off. Let's consider following case: > >>>> > >>>>intensity - 15625 > >>>>config->step - 15625 > >>>>intensity / config->step = 1 (the lowest possible current level) > >>> > >>>In V4L2 controls the minimum is not off, and zero might not be a possible > >>>value since minimum isn't divisible by step. > >>> > >>>I wonder how to best take that into account. > >> > >>I've assumed that in MODE_TORCH a led is always on. Switching > >>the mode to MODE_FLASH or MODE_OFF turns the led off. > >>This way we avoid the problem with converting 0 uA value to > >>led_brightness, as available torch brightness levels start from > >>the minimum current level value and turning the led off is > >>accomplished on transition to MODE_OFF or MODE_FLASH, by > >>calling brightness_set op with led_brightness = 0. > > > >I'm not sure if we understood the issue the same way. My concern was that if > >the intensity isn't a multiple of step (but intensity - min is), the above > >formula won't return a valid result (unless I miss something). > > > > Please note that v4l2_flash_intensity_to_led_brightness is called only > from s_ctrl callback, and thus it expects to get the intensity aligned > to the step value, so it will always be a multiple of step. > Is it possible that s_ctrl callback would be passed a non-aligned > control value? In a nutshell: value - min is aligned but value is not. Please see validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c .
On 05/06/2014 11:10 AM, Sakari Ailus wrote: > Hi Jacek, > > On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote: >> Hi Sakari, >> >> On 05/02/2014 01:06 PM, Sakari Ailus wrote: >> >>>>>> [...] >>>>>>>> +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( >>>>>>>> + struct led_ctrl *config, >>>>>>>> + u32 intensity) >>>>>>> >>>>>>> Fits on a single line. >>>>>>> >>>>>>>> +{ >>>>>>>> + return intensity / config->step; >>>>>>> >>>>>>> Shouldn't you first decrement the minimum before the division? >>>>>> >>>>>> Brightness level 0 means that led is off. Let's consider following case: >>>>>> >>>>>> intensity - 15625 >>>>>> config->step - 15625 >>>>>> intensity / config->step = 1 (the lowest possible current level) >>>>> >>>>> In V4L2 controls the minimum is not off, and zero might not be a possible >>>>> value since minimum isn't divisible by step. >>>>> >>>>> I wonder how to best take that into account. >>>> >>>> I've assumed that in MODE_TORCH a led is always on. Switching >>>> the mode to MODE_FLASH or MODE_OFF turns the led off. >>>> This way we avoid the problem with converting 0 uA value to >>>> led_brightness, as available torch brightness levels start from >>>> the minimum current level value and turning the led off is >>>> accomplished on transition to MODE_OFF or MODE_FLASH, by >>>> calling brightness_set op with led_brightness = 0. >>> >>> I'm not sure if we understood the issue the same way. My concern was that if >>> the intensity isn't a multiple of step (but intensity - min is), the above >>> formula won't return a valid result (unless I miss something). >>> >> >> Please note that v4l2_flash_intensity_to_led_brightness is called only >> from s_ctrl callback, and thus it expects to get the intensity aligned >> to the step value, so it will always be a multiple of step. >> Is it possible that s_ctrl callback would be passed a non-aligned >> control value? > > In a nutshell: value - min is aligned but value is not. Please see > validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c . > Still, to my mind, value is aligned. Below I execute the calculation steps one by one according to the V4L2_CTRL_TYPE_INTEGER case in the validate_new function: c->value = 35000 val = c->value + step / 2; // 35000 + 15625 / 2 = 42812 val = clamp(val, min, max); // val = 42812 offset = val - min; // 42812 - 15625 = 27187 offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625 c->value = min + offset; // 15625 + 15625 = 31250 Value is aligned to the nearest step. Please spot any discrepancies in my way of thinking if there are any :) Regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Wed, May 07, 2014 at 09:20:17AM +0200, Jacek Anaszewski wrote: > On 05/06/2014 11:10 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote: > >>Hi Sakari, > >> > >>On 05/02/2014 01:06 PM, Sakari Ailus wrote: > >> > >>>>>>[...] > >>>>>>>>+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > >>>>>>>>+ struct led_ctrl *config, > >>>>>>>>+ u32 intensity) > >>>>>>> > >>>>>>>Fits on a single line. > >>>>>>> > >>>>>>>>+{ > >>>>>>>>+ return intensity / config->step; > >>>>>>> > >>>>>>>Shouldn't you first decrement the minimum before the division? > >>>>>> > >>>>>>Brightness level 0 means that led is off. Let's consider following case: > >>>>>> > >>>>>>intensity - 15625 > >>>>>>config->step - 15625 > >>>>>>intensity / config->step = 1 (the lowest possible current level) > >>>>> > >>>>>In V4L2 controls the minimum is not off, and zero might not be a possible > >>>>>value since minimum isn't divisible by step. > >>>>> > >>>>>I wonder how to best take that into account. > >>>> > >>>>I've assumed that in MODE_TORCH a led is always on. Switching > >>>>the mode to MODE_FLASH or MODE_OFF turns the led off. > >>>>This way we avoid the problem with converting 0 uA value to > >>>>led_brightness, as available torch brightness levels start from > >>>>the minimum current level value and turning the led off is > >>>>accomplished on transition to MODE_OFF or MODE_FLASH, by > >>>>calling brightness_set op with led_brightness = 0. > >>> > >>>I'm not sure if we understood the issue the same way. My concern was that if > >>>the intensity isn't a multiple of step (but intensity - min is), the above > >>>formula won't return a valid result (unless I miss something). > >>> > >> > >>Please note that v4l2_flash_intensity_to_led_brightness is called only > >>from s_ctrl callback, and thus it expects to get the intensity aligned > >>to the step value, so it will always be a multiple of step. > >>Is it possible that s_ctrl callback would be passed a non-aligned > >>control value? > > > >In a nutshell: value - min is aligned but value is not. Please see > >validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c . > > > > Still, to my mind, value is aligned. > > Below I execute the calculation steps one by one > according to the V4L2_CTRL_TYPE_INTEGER case in the > validate_new function: > > c->value = 35000 > > val = c->value + step / 2; // 35000 + 15625 / 2 = 42812 > val = clamp(val, min, max); // val = 42812 > offset = val - min; // 42812 - 15625 = 27187 > offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625 > c->value = min + offset; // 15625 + 15625 = 31250 > > Value is aligned to the nearest step. > > Please spot any discrepancies in my way of thinking if there > are any :) min is aligned to step above. This is not necessarily the case. And if min is not aligned, neither is value.
Hi Sakari, On 05/07/2014 09:58 AM, Sakari Ailus wrote: > Hi Jacek, > > On Wed, May 07, 2014 at 09:20:17AM +0200, Jacek Anaszewski wrote: >> On 05/06/2014 11:10 AM, Sakari Ailus wrote: >>> Hi Jacek, >>> >>> On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote: >>>> Hi Sakari, >>>> >>>> On 05/02/2014 01:06 PM, Sakari Ailus wrote: >>>> >>>>>>>> [...] >>>>>>>>>> +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( >>>>>>>>>> + struct led_ctrl *config, >>>>>>>>>> + u32 intensity) >>>>>>>>> >>>>>>>>> Fits on a single line. >>>>>>>>> >>>>>>>>>> +{ >>>>>>>>>> + return intensity / config->step; >>>>>>>>> >>>>>>>>> Shouldn't you first decrement the minimum before the division? >>>>>>>> >>>>>>>> Brightness level 0 means that led is off. Let's consider following case: >>>>>>>> >>>>>>>> intensity - 15625 >>>>>>>> config->step - 15625 >>>>>>>> intensity / config->step = 1 (the lowest possible current level) >>>>>>> >>>>>>> In V4L2 controls the minimum is not off, and zero might not be a possible >>>>>>> value since minimum isn't divisible by step. >>>>>>> >>>>>>> I wonder how to best take that into account. >>>>>> >>>>>> I've assumed that in MODE_TORCH a led is always on. Switching >>>>>> the mode to MODE_FLASH or MODE_OFF turns the led off. >>>>>> This way we avoid the problem with converting 0 uA value to >>>>>> led_brightness, as available torch brightness levels start from >>>>>> the minimum current level value and turning the led off is >>>>>> accomplished on transition to MODE_OFF or MODE_FLASH, by >>>>>> calling brightness_set op with led_brightness = 0. >>>>> >>>>> I'm not sure if we understood the issue the same way. My concern was that if >>>>> the intensity isn't a multiple of step (but intensity - min is), the above >>>>> formula won't return a valid result (unless I miss something). >>>>> >>>> >>>> Please note that v4l2_flash_intensity_to_led_brightness is called only >>> >from s_ctrl callback, and thus it expects to get the intensity aligned >>>> to the step value, so it will always be a multiple of step. >>>> Is it possible that s_ctrl callback would be passed a non-aligned >>>> control value? >>> >>> In a nutshell: value - min is aligned but value is not. Please see >>> validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c . >>> >> >> Still, to my mind, value is aligned. >> >> Below I execute the calculation steps one by one >> according to the V4L2_CTRL_TYPE_INTEGER case in the >> validate_new function: >> >> c->value = 35000 >> >> val = c->value + step / 2; // 35000 + 15625 / 2 = 42812 >> val = clamp(val, min, max); // val = 42812 >> offset = val - min; // 42812 - 15625 = 27187 >> offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625 >> c->value = min + offset; // 15625 + 15625 = 31250 >> >> Value is aligned to the nearest step. >> >> Please spot any discrepancies in my way of thinking if there >> are any :) > > min is aligned to step above. This is not necessarily the case. And if min > is not aligned, neither is value. > Thanks for spotting this. Below are improved versions of the conversion functions. Please let me know if you have any comments. static inline enum led_brightnessv4l2_flash_intensity_to_led_brightness( struct led_ctrl *config, u32 intensity) { return ((intensity - config->min) / config->step) + 1; } static inline u32 v4l2_flash_led_brightness_to_intensity( struct led_ctrl *config, enum led_brightness brightness) { return ((brightness - 1) * config->step) + config->min; } Regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jacek, On Fri, May 09, 2014 at 09:18:55AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 05/07/2014 09:58 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Wed, May 07, 2014 at 09:20:17AM +0200, Jacek Anaszewski wrote: > >>On 05/06/2014 11:10 AM, Sakari Ailus wrote: > >>>Hi Jacek, > >>> > >>>On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote: > >>>>Hi Sakari, > >>>> > >>>>On 05/02/2014 01:06 PM, Sakari Ailus wrote: > >>>> > >>>>>>>>[...] > >>>>>>>>>>+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > >>>>>>>>>>+ struct led_ctrl *config, > >>>>>>>>>>+ u32 intensity) > >>>>>>>>> > >>>>>>>>>Fits on a single line. > >>>>>>>>> > >>>>>>>>>>+{ > >>>>>>>>>>+ return intensity / config->step; > >>>>>>>>> > >>>>>>>>>Shouldn't you first decrement the minimum before the division? > >>>>>>>> > >>>>>>>>Brightness level 0 means that led is off. Let's consider following case: > >>>>>>>> > >>>>>>>>intensity - 15625 > >>>>>>>>config->step - 15625 > >>>>>>>>intensity / config->step = 1 (the lowest possible current level) > >>>>>>> > >>>>>>>In V4L2 controls the minimum is not off, and zero might not be a possible > >>>>>>>value since minimum isn't divisible by step. > >>>>>>> > >>>>>>>I wonder how to best take that into account. > >>>>>> > >>>>>>I've assumed that in MODE_TORCH a led is always on. Switching > >>>>>>the mode to MODE_FLASH or MODE_OFF turns the led off. > >>>>>>This way we avoid the problem with converting 0 uA value to > >>>>>>led_brightness, as available torch brightness levels start from > >>>>>>the minimum current level value and turning the led off is > >>>>>>accomplished on transition to MODE_OFF or MODE_FLASH, by > >>>>>>calling brightness_set op with led_brightness = 0. > >>>>> > >>>>>I'm not sure if we understood the issue the same way. My concern was that if > >>>>>the intensity isn't a multiple of step (but intensity - min is), the above > >>>>>formula won't return a valid result (unless I miss something). > >>>>> > >>>> > >>>>Please note that v4l2_flash_intensity_to_led_brightness is called only > >>>>from s_ctrl callback, and thus it expects to get the intensity aligned > >>>>to the step value, so it will always be a multiple of step. > >>>>Is it possible that s_ctrl callback would be passed a non-aligned > >>>>control value? > >>> > >>>In a nutshell: value - min is aligned but value is not. Please see > >>>validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c . > >>> > >> > >>Still, to my mind, value is aligned. > >> > >>Below I execute the calculation steps one by one > >>according to the V4L2_CTRL_TYPE_INTEGER case in the > >>validate_new function: > >> > >>c->value = 35000 > >> > >>val = c->value + step / 2; // 35000 + 15625 / 2 = 42812 > >>val = clamp(val, min, max); // val = 42812 > >>offset = val - min; // 42812 - 15625 = 27187 > >>offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625 > >>c->value = min + offset; // 15625 + 15625 = 31250 > >> > >>Value is aligned to the nearest step. > >> > >>Please spot any discrepancies in my way of thinking if there > >>are any :) > > > >min is aligned to step above. This is not necessarily the case. And if min > >is not aligned, neither is value. > > > > Thanks for spotting this. Below are improved versions of the conversion > functions. Please let me know if you have any comments. > > static inline > enum led_brightnessv4l2_flash_intensity_to_led_brightness( > struct led_ctrl *config, > u32 intensity) > { > return ((intensity - config->min) / config->step) + 1; > } > > static inline > u32 v4l2_flash_led_brightness_to_intensity( > struct led_ctrl *config, > enum led_brightness brightness) > { > return ((brightness - 1) * config->step) + config->min; V4L2 control integer values are signed, thus s32 instead of u32. Otherwise looks good to me.
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig index 2189bfb..1f8514d 100644 --- a/drivers/media/v4l2-core/Kconfig +++ b/drivers/media/v4l2-core/Kconfig @@ -35,6 +35,16 @@ config V4L2_MEM2MEM_DEV tristate depends on VIDEOBUF2_CORE +# Used by LED subsystem flash drivers +config V4L2_FLASH + bool "Enable support for Flash sub-devices" + 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 c6ae7ba..8e37ab4 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) += 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..f1be332 --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-flash.c @@ -0,0 +1,393 @@ +/* + * V4L2 Flash LED sub-device registration helpers. + * + * Copyright (C) 2014 Samsung Electronics Co., Ltd + * Author: Jacek Anaszewski <j.anaszewski@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/leds_flash.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-dev.h> +#include <media/v4l2-device.h> +#include <media/v4l2-event.h> +#include <media/v4l2-ioctl.h> +#include <media/v4l2-flash.h> + +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( + struct led_ctrl *config, + u32 intensity) +{ + return intensity / config->step; +} + +static inline u32 v4l2_flash_led_brightness_to_intensity( + struct led_ctrl *config, + enum led_brightness brightness) +{ + return brightness * config->step; +} + +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 *led_cdev = v4l2_flash->led_cdev; + struct led_flash *flash = led_cdev->flash; + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; + u32 fault; + int ret; + + switch (c->id) { + case V4L2_CID_FLASH_TORCH_INTENSITY: + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { + ret = v4l2_call_flash_op(brightness_update, led_cdev); + if (ret < 0) + return ret; + ctrl->torch_intensity->val = + v4l2_flash_led_brightness_to_intensity( + &led_cdev->brightness_ctrl, + led_cdev->brightness); + } + return 0; + case V4L2_CID_FLASH_INTENSITY: + ret = v4l2_call_flash_op(flash_brightness_update, led_cdev); + if (ret < 0) + return ret; + /* no conversion is needed */ + c->val = flash->brightness.val; + return 0; + case V4L2_CID_FLASH_INDICATOR_INTENSITY: + ret = v4l2_call_flash_op(indicator_brightness_update, led_cdev); + if (ret < 0) + return ret; + /* no conversion is needed */ + c->val = flash->indicator_brightness->val; + return 0; + case V4L2_CID_FLASH_STROBE_STATUS: + ret = v4l2_call_flash_op(strobe_get, led_cdev); + if (ret < 0) + return ret; + c->val = !!ret; + return 0; + case V4L2_CID_FLASH_FAULT: + /* led faults map directly to V4L2 flash faults */ + ret = v4l2_call_flash_op(fault_get, led_cdev, &fault); + if (!ret) + c->val = fault; + return ret; + default: + return -EINVAL; + } +} + +static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c) +{ + struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c); + struct led_classdev *led_cdev = v4l2_flash->led_cdev; + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; + enum led_brightness torch_brightness; + bool external_strobe; + int ret; + + switch (c->id) { + case V4L2_CID_FLASH_LED_MODE: + switch (c->val) { + case V4L2_FLASH_LED_MODE_NONE: + v4l2_call_flash_op(brightness_set, led_cdev, 0); + return v4l2_call_flash_op(strobe_set, led_cdev, false); + case V4L2_FLASH_LED_MODE_FLASH: + /* Turn off torch LED */ + v4l2_call_flash_op(brightness_set, led_cdev, 0); + external_strobe = (ctrl->source->val == + V4L2_FLASH_STROBE_SOURCE_EXTERNAL); + return v4l2_call_flash_op(external_strobe_set, led_cdev, + external_strobe); + case V4L2_FLASH_LED_MODE_TORCH: + /* Stop flash strobing */ + ret = v4l2_call_flash_op(strobe_set, led_cdev, false); + if (ret) + return ret; + /* torch is always triggered by software */ + ret = v4l2_call_flash_op(external_strobe_set, led_cdev, + false); + if (ret) + return ret; + + torch_brightness = + v4l2_flash_intensity_to_led_brightness( + &led_cdev->brightness_ctrl, + ctrl->torch_intensity->val); + v4l2_call_flash_op(brightness_set, led_cdev, + torch_brightness); + return ret; + } + break; + case V4L2_CID_FLASH_STROBE_SOURCE: + return v4l2_call_flash_op(external_strobe_set, led_cdev, + c->val == V4L2_FLASH_STROBE_SOURCE_EXTERNAL); + case V4L2_CID_FLASH_STROBE: + if (ctrl->led_mode->val != V4L2_FLASH_LED_MODE_FLASH || + ctrl->source->val != V4L2_FLASH_STROBE_SOURCE_SOFTWARE) + return -EINVAL; + return v4l2_call_flash_op(strobe_set, led_cdev, true); + case V4L2_CID_FLASH_STROBE_STOP: + return v4l2_call_flash_op(strobe_set, led_cdev, false); + case V4L2_CID_FLASH_TIMEOUT: + ret = v4l2_call_flash_op(timeout_set, led_cdev, c->val); + case V4L2_CID_FLASH_INTENSITY: + /* no conversion is needed */ + return v4l2_call_flash_op(flash_brightness_set, led_cdev, + c->val); + case V4L2_CID_FLASH_INDICATOR_INTENSITY: + /* no conversion is needed */ + return v4l2_call_flash_op(indicator_brightness_set, led_cdev, + c->val); + case V4L2_CID_FLASH_TORCH_INTENSITY: + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { + torch_brightness = + v4l2_flash_intensity_to_led_brightness( + &led_cdev->brightness_ctrl, + ctrl->torch_intensity->val); + v4l2_call_flash_op(brightness_set, led_cdev, + torch_brightness); + } + 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 int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash) + +{ + struct led_classdev *led_cdev = v4l2_flash->led_cdev; + struct led_flash *flash = led_cdev->flash; + bool has_indicator = flash->indicator_brightness; + struct v4l2_ctrl *ctrl; + struct led_ctrl *ctrl_cfg; + unsigned int mask; + int ret, max, num_ctrls; + + num_ctrls = flash->has_flash_led ? 8 : 2; + if (flash->fault_flags) + ++num_ctrls; + if (has_indicator) + ++num_ctrls; + + v4l2_ctrl_handler_init(&v4l2_flash->hdl, num_ctrls); + + mask = 1 << V4L2_FLASH_LED_MODE_NONE | + 1 << V4L2_FLASH_LED_MODE_TORCH; + if (flash->has_flash_led) + mask |= 1 << V4L2_FLASH_LED_MODE_FLASH; + + /* Configure FLASH_LED_MODE ctrl */ + v4l2_flash->ctrl.led_mode = v4l2_ctrl_new_std_menu( + &v4l2_flash->hdl, + &v4l2_flash_ctrl_ops, V4L2_CID_FLASH_LED_MODE, + V4L2_FLASH_LED_MODE_TORCH, ~mask, + V4L2_FLASH_LED_MODE_NONE); + + /* Configure TORCH_INTENSITY ctrl */ + ctrl_cfg = &led_cdev->brightness_ctrl; + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_TORCH_INTENSITY, + ctrl_cfg->min, ctrl_cfg->max, + ctrl_cfg->step, ctrl_cfg->val); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; + v4l2_flash->ctrl.torch_intensity = ctrl; + + if (flash->has_flash_led) { + /* Configure FLASH_STROBE_SOURCE ctrl */ + mask = 1 << V4L2_FLASH_STROBE_SOURCE_SOFTWARE; + + if (flash->has_external_strobe) { + mask |= 1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL; + max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL; + } else { + max = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; + } + + v4l2_flash->ctrl.source = v4l2_ctrl_new_std_menu( + &v4l2_flash->hdl, + &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_STROBE_SOURCE, + max, + ~mask, + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); + + /* Configure FLASH_STROBE ctrl */ + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_STROBE, 0, 1, 1, 0); + + /* Configure FLASH_STROBE_STOP ctrl */ + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_STROBE_STOP, + 0, 1, 1, 0); + + /* Configure FLASH_STROBE_STATUS ctrl */ + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_STROBE_STATUS, + 0, 1, 1, 1); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | + V4L2_CTRL_FLAG_READ_ONLY; + + /* Configure FLASH_TIMEOUT ctrl */ + ctrl_cfg = &flash->timeout; + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_TIMEOUT, ctrl_cfg->min, + ctrl_cfg->max, ctrl_cfg->step, + ctrl_cfg->val); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; + + /* Configure FLASH_INTENSITY ctrl */ + ctrl_cfg = &flash->brightness; + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_INTENSITY, + ctrl_cfg->min, ctrl_cfg->max, + ctrl_cfg->step, ctrl_cfg->val); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; + + if (flash->fault_flags) { + /* Configure FLASH_FAULT ctrl */ + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, + &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_FAULT, 0, + flash->fault_flags, + 0, 0); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | + V4L2_CTRL_FLAG_READ_ONLY; + } + if (has_indicator) { + /* Configure FLASH_INDICATOR_INTENSITY ctrl */ + ctrl_cfg = flash->indicator_brightness; + ctrl = v4l2_ctrl_new_std( + &v4l2_flash->hdl, &v4l2_flash_ctrl_ops, + V4L2_CID_FLASH_INDICATOR_INTENSITY, + ctrl_cfg->min, ctrl_cfg->max, + ctrl_cfg->step, ctrl_cfg->val); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; + } + } + + if (v4l2_flash->hdl.error) { + ret = v4l2_flash->hdl.error; + goto error_free; + } + + ret = v4l2_ctrl_handler_setup(&v4l2_flash->hdl); + if (ret < 0) + goto error_free; + + v4l2_flash->subdev.ctrl_handler = &v4l2_flash->hdl; + + return 0; + +error_free: + v4l2_ctrl_handler_free(&v4l2_flash->hdl); + 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 *led_cdev = v4l2_flash->led_cdev; + + mutex_lock(&led_cdev->led_lock); + v4l2_call_flash_op(sysfs_lock, led_cdev); + mutex_unlock(&led_cdev->led_lock); + + return 0; +} + +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 *led_cdev = v4l2_flash->led_cdev; + + mutex_lock(&led_cdev->led_lock); + v4l2_call_flash_op(sysfs_unlock, led_cdev); + mutex_unlock(&led_cdev->led_lock); + + return 0; +} + +static const struct v4l2_subdev_internal_ops v4l2_flash_subdev_internal_ops = { + .open = v4l2_flash_open, + .close = v4l2_flash_close, +}; + +static struct v4l2_subdev_ops v4l2_flash_subdev_ops = { +}; + +int v4l2_flash_init(struct led_classdev *led_cdev, + const struct v4l2_flash_ops *ops) +{ + struct v4l2_flash *flash = &led_cdev->flash->v4l2_flash; + struct v4l2_subdev *sd = &flash->subdev; + int ret; + + if (!led_cdev || !ops) + return -EINVAL; + + flash->led_cdev = led_cdev; + sd->dev = led_cdev->dev->parent; + v4l2_subdev_init(sd, &v4l2_flash_subdev_ops); + sd->internal_ops = &v4l2_flash_subdev_internal_ops; + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + snprintf(sd->name, sizeof(sd->name), led_cdev->name); + + flash->ops = ops; + + ret = v4l2_flash_init_controls(flash); + if (ret < 0) + goto err_init_controls; + + ret = media_entity_init(&sd->entity, 0, NULL, 0); + if (ret < 0) + goto err_init_entity; + + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; + + ret = v4l2_async_register_subdev(sd); + if (ret < 0) + goto err_init_entity; + + return 0; + +err_init_entity: + media_entity_cleanup(&sd->entity); +err_init_controls: + v4l2_ctrl_handler_free(sd->ctrl_handler); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(v4l2_flash_init); + +void v4l2_flash_release(struct led_classdev *led_cdev) +{ + struct v4l2_flash *flash = &led_cdev->flash->v4l2_flash; + + v4l2_ctrl_handler_free(flash->subdev.ctrl_handler); + v4l2_async_unregister_subdev(&flash->subdev); + media_entity_cleanup(&flash->subdev.entity); +} +EXPORT_SYMBOL_GPL(v4l2_flash_release); diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h new file mode 100644 index 0000000..fe16ddd --- /dev/null +++ b/include/media/v4l2-flash.h @@ -0,0 +1,119 @@ +/* + * V4L2 Flash LED sub-device registration helpers. + * + * Copyright (C) 2014 Samsung Electronics Co., Ltd + * Author: Jacek Anaszewski <j.anaszewski@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." + */ + +#ifndef _V4L2_FLASH_H +#define _V4L2_FLASH_H + +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-dev.h> +#include <media/v4l2-event.h> +#include <media/v4l2-ioctl.h> + +#define v4l2_call_flash_op(op, args...) \ + ((v4l2_flash)->ops->op(args)) \ + +struct led_classdev; +enum led_brightness; + +struct v4l2_flash_ops { + void (*brightness_set)(struct led_classdev *led_cdev, + enum led_brightness brightness); + int (*brightness_update)(struct led_classdev *led_cdev); + int (*flash_brightness_set)(struct led_classdev *led_cdev, + u32 brightness); + int (*flash_brightness_update)(struct led_classdev *led_cdev); + int (*strobe_set)(struct led_classdev *led_cdev, + bool state); + int (*strobe_get)(struct led_classdev *led_cdev); + int (*timeout_set)(struct led_classdev *led_cdev, + u32 timeout); + int (*indicator_brightness_set)(struct led_classdev *led_cdev, + u32 brightness); + int (*indicator_brightness_update)(struct led_classdev *led_cdev); + int (*external_strobe_set)(struct led_classdev *led_cdev, + bool enable); + int (*fault_get)(struct led_classdev *led_cdev, + u32 *fault); + void (*sysfs_lock)(struct led_classdev *led_cdev); + void (*sysfs_unlock)(struct led_classdev *led_cdev); +}; + +/** + * struct v4l2_flash_ctrl - controls that define the sub-dev's state + * @source: V4L2_CID_FLASH_STROBE_SOURCE control + * @led_mode: V4L2_CID_FLASH_LED_MODE control + * @torch_intensity: V4L2_CID_FLASH_TORCH_INTENSITY control + */ +struct v4l2_flash_ctrl { + struct v4l2_ctrl *source; + struct v4l2_ctrl *led_mode; + struct v4l2_ctrl *torch_intensity; +}; + +/** + * struct v4l2_flash - Flash sub-device context + * @led_cdev: LED class device controlled by this sub-device + * @ops: LED class device ops + * @subdev: V4L2 sub-device + * @hdl: flash controls handler + * @ctrl: state defining controls + */ +struct v4l2_flash { + struct led_classdev *led_cdev; + const struct v4l2_flash_ops *ops; + + struct v4l2_subdev subdev; + struct v4l2_ctrl_handler hdl; + struct v4l2_flash_ctrl ctrl; +}; + +static inline struct v4l2_flash *v4l2_subdev_to_v4l2_flash(struct v4l2_subdev *sd) +{ + return container_of(sd, struct v4l2_flash, subdev); +} + +static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c) +{ + return container_of(c->handler, struct v4l2_flash, hdl); +} + +#ifdef CONFIG_V4L2_FLASH +/** + * v4l2_flash_init - initialize V4L2 flash led sub-device + * @led_cdev: the LED to create subdev upon + * @ops: LED subsystem callbacks + * + * Create V4L2 subdev wrapping given LED subsystem device. + */ +int v4l2_flash_init(struct led_classdev *led_cdev, + const struct v4l2_flash_ops *ops); + +/** + * v4l2_flash_release - release V4L2 flash led sub-device + * @flash: a structure representing V4L2 flash led device + * + * Release V4L2 flash led subdev. + */ +void v4l2_flash_release(struct led_classdev *led_cdev); +#else +static inline int v4l2_flash_init(struct led_classdev *led_cdev, + const struct v4l2_flash_ops *ops) +{ + return 0; +} + +static inline void v4l2_flash_release(struct led_classdev *led_cdev) +{ +} +#endif /* CONFIG_V4L2_FLASH */ + +#endif /* _V4L2_FLASH_H */