Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755033AbaGULMA (ORCPT ); Mon, 21 Jul 2014 07:12:00 -0400 Received: from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:36014 "EHLO hillosipuli.retiisi.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755004AbaGULL5 (ORCPT ); Mon, 21 Jul 2014 07:11:57 -0400 Message-ID: <53CCF59E.3070200@iki.fi> Date: Mon, 21 Jul 2014 14:12:30 +0300 From: Sakari Ailus User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:29.0) Gecko/20100101 SeaMonkey/2.26.1 MIME-Version: 1.0 To: Jacek Anaszewski , linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org CC: kyungmin.park@samsung.com, b.zolnierkie@samsung.com, Hans Verkuil Subject: Re: [PATCH/RFC v4 15/21] media: Add registration helpers for V4L2 flash References: <1405087464-13762-1-git-send-email-j.anaszewski@samsung.com> <1405087464-13762-16-git-send-email-j.anaszewski@samsung.com> In-Reply-To: <1405087464-13762-16-git-send-email-j.anaszewski@samsung.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jacek, 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 > Acked-by: Kyungmin Park > Cc: Sakari Ailus > Cc: Hans Verkuil > --- > drivers/media/v4l2-core/Kconfig | 11 + > drivers/media/v4l2-core/Makefile | 2 + > drivers/media/v4l2-core/v4l2-flash.c | 580 ++++++++++++++++++++++++++++++++++ > include/media/v4l2-flash.h | 137 ++++++++ > 4 files changed, 730 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 9ca0f8d..3ae3f0f 100644 > --- a/drivers/media/v4l2-core/Kconfig > +++ b/drivers/media/v4l2-core/Kconfig > @@ -35,6 +35,17 @@ config V4L2_MEM2MEM_DEV > tristate > depends on VIDEOBUF2_CORE > > +# Used by LED subsystem flash drivers > +config V4L2_FLASH_LED_CLASS > + bool "Enable support for Flash sub-devices" > + depends on VIDEO_V4L2_SUBDEV_API > + depends on LEDS_CLASS_FLASH > + ---help--- > + Say Y here to enable support for Flash sub-devices, which allow > + to control LED class devices with use of V4L2 Flash controls. > + > + When in doubt, say N. > + > # Used by drivers that need Videobuf modules > config VIDEOBUF_GEN > tristate > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile > index 63d29f2..44e858c 100644 > --- a/drivers/media/v4l2-core/Makefile > +++ b/drivers/media/v4l2-core/Makefile > @@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o > > obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o > > +obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o > + > obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o > obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o > obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o > diff --git a/drivers/media/v4l2-core/v4l2-flash.c b/drivers/media/v4l2-core/v4l2-flash.c > new file mode 100644 > index 0000000..21080c6 > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-flash.c > @@ -0,0 +1,580 @@ > +/* > + * V4L2 Flash LED sub-device registration helpers. > + * > + * Copyright (C) 2014 Samsung Electronics Co., Ltd > + * Author: Jacek Anaszewski > + * > + * 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 > +#include > +#include > +#include > +#include > + > +#define call_flash_op(v4l2_flash, op, args...) \ > + (v4l2_flash->ops->op ? \ > + v4l2_flash->ops->op(args) : \ > + -EINVAL) > + > +static struct v4l2_device *v4l2_dev; > +static int registered_flashes; > + > +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness( > + struct v4l2_ctrl_config *config, > + s32 intensity) > +{ > + return ((intensity - config->min) / config->step) + 1; > +} > + > +static inline s32 v4l2_flash_led_brightness_to_intensity( > + struct v4l2_ctrl_config *config, > + enum led_brightness brightness) > +{ > + return ((brightness - 1) * config->step) + config->min; > +} > + > +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 *flash = v4l2_flash->flash; > + struct led_classdev *led_cdev = &flash->led_cdev; > + struct v4l2_flash_ctrl_config *config = &v4l2_flash->config; > + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; > + bool is_strobing; > + int ret; > + > + switch (c->id) { > + case V4L2_CID_FLASH_TORCH_INTENSITY: > + /* > + * Update torch brightness only if in TORCH_MODE, > + * as otherwise brightness_update op returns 0, > + * which would spuriously inform user space that > + * V4L2_CID_FLASH_TORCH_INTENSITY control value > + * has changed. > + */ > + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { > + ret = call_flash_op(v4l2_flash, torch_brightness_update, > + led_cdev); > + if (ret < 0) > + return ret; > + ctrl->torch_intensity->val = > + v4l2_flash_led_brightness_to_intensity( > + &config->torch_intensity, > + led_cdev->brightness); > + } > + return 0; > + case V4L2_CID_FLASH_INTENSITY: > + ret = call_flash_op(v4l2_flash, flash_brightness_update, > + flash); > + if (ret < 0) > + return ret; > + /* no conversion is needed */ > + c->val = flash->brightness.val; > + return 0; > + case V4L2_CID_FLASH_INDICATOR_INTENSITY: > + ret = call_flash_op(v4l2_flash, indicator_brightness_update, > + flash); > + if (ret < 0) > + return ret; > + /* no conversion is needed */ > + c->val = flash->indicator_brightness->val; > + return 0; > + case V4L2_CID_FLASH_STROBE_STATUS: > + ret = call_flash_op(v4l2_flash, strobe_get, flash, > + &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 */ > + ret = call_flash_op(v4l2_flash, fault_get, flash, &c->val); > + return ret; > + case V4L2_CID_FLASH_STROBE_SOURCE: > + c->val = flash->external_strobe; > + return 0; > + case V4L2_CID_FLASH_STROBE_PROVIDER: > + c->val = flash->strobe_provider_id; > + return 0; > + 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_flash *flash = v4l2_flash->flash; > + struct v4l2_flash_ctrl *ctrl = &v4l2_flash->ctrl; > + struct v4l2_flash_ctrl_config *config = &v4l2_flash->config; > + 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: > + call_flash_op(v4l2_flash, torch_brightness_set, > + &flash->led_cdev, 0); > + return call_flash_op(v4l2_flash, strobe_set, flash, > + false); > + case V4L2_FLASH_LED_MODE_FLASH: > + /* Turn off torch LED */ > + call_flash_op(v4l2_flash, torch_brightness_set, > + &flash->led_cdev, 0); > + external_strobe = (ctrl->source->val == > + V4L2_FLASH_STROBE_SOURCE_EXTERNAL); > + return call_flash_op(v4l2_flash, external_strobe_set, > + flash, external_strobe); > + case V4L2_FLASH_LED_MODE_TORCH: > + /* Stop flash strobing */ > + ret = call_flash_op(v4l2_flash, strobe_set, flash, > + false); > + if (ret) > + return ret; > + > + torch_brightness = > + v4l2_flash_intensity_to_led_brightness( > + &config->torch_intensity, > + ctrl->torch_intensity->val); > + call_flash_op(v4l2_flash, torch_brightness_set, > + &flash->led_cdev, torch_brightness); > + return ret; > + } > + break; > + case V4L2_CID_FLASH_STROBE_SOURCE: > + external_strobe = (c->val == V4L2_FLASH_STROBE_SOURCE_EXTERNAL); Is the external_strobe argument match exactly to the strobe source control? You seem to assume that in g_volatile_ctrl() above. I think having it the either way is fine but not both. :-) > + return call_flash_op(v4l2_flash, external_strobe_set, flash, > + external_strobe); > + 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 call_flash_op(v4l2_flash, strobe_set, flash, true); > + case V4L2_CID_FLASH_STROBE_STOP: Should we check the flash mode here? I guess so. How about strobe source as well? > + return call_flash_op(v4l2_flash, strobe_set, flash, false); > + case V4L2_CID_FLASH_TIMEOUT: > + return call_flash_op(v4l2_flash, timeout_set, flash, c->val); > + case V4L2_CID_FLASH_INTENSITY: > + /* no conversion is needed */ > + return call_flash_op(v4l2_flash, flash_brightness_set, flash, > + c->val); > + case V4L2_CID_FLASH_INDICATOR_INTENSITY: > + /* no conversion is needed */ > + return call_flash_op(v4l2_flash, indicator_brightness_set, > + flash, c->val); > + case V4L2_CID_FLASH_TORCH_INTENSITY: > + /* > + * If not in MODE_TORCH don't call led-class brightness_set > + * op, as it would result in turning the torch led on. > + * Instead the value is cached only and will be written > + * to the device upon transition to MODE_TORCH. > + */ > + if (ctrl->led_mode->val == V4L2_FLASH_LED_MODE_TORCH) { > + torch_brightness = > + v4l2_flash_intensity_to_led_brightness( > + &config->torch_intensity, > + ctrl->torch_intensity->val); > + call_flash_op(v4l2_flash, torch_brightness_set, > + &flash->led_cdev, torch_brightness); > + } > + return 0; > + case V4L2_CID_FLASH_STROBE_PROVIDER: > + flash->strobe_provider_id = c->val; > + 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_strobe_providers_menu(struct v4l2_flash *v4l2_flash) > +{ > + struct led_classdev_flash *flash = v4l2_flash->flash; > + struct led_flash_strobe_provider *provider; > + struct v4l2_ctrl *ctrl; > + int i = 0; > + > + v4l2_flash->strobe_providers_menu = > + kzalloc(sizeof(char *) * (flash->num_strobe_providers), > + GFP_KERNEL); > + if (!v4l2_flash->strobe_providers_menu) > + return -ENOMEM; > + > + list_for_each_entry(provider, &flash->strobe_providers, list) > + v4l2_flash->strobe_providers_menu[i++] = > + (char *) provider->name; > + > + ctrl = v4l2_ctrl_new_std_menu_items( > + &v4l2_flash->hdl, &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_STROBE_PROVIDER, > + flash->num_strobe_providers - 1, > + 0, 0, > + (const char * const *) v4l2_flash->strobe_providers_menu); > + > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + > + return 0; > +} > + > +static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash) > + > +{ > + struct led_classdev_flash *flash = v4l2_flash->flash; > + const struct led_flash_ops *flash_ops = flash->ops; > + struct led_classdev *led_cdev = &flash->led_cdev; > + struct v4l2_flash_ctrl_config *config = &v4l2_flash->config; > + struct v4l2_ctrl *ctrl; > + struct v4l2_ctrl_config *ctrl_cfg; > + bool has_flash = led_cdev->flags & LED_DEV_CAP_FLASH; > + bool has_indicator = led_cdev->flags & LED_DEV_CAP_INDICATOR; > + bool has_strobe_providers = (flash->num_strobe_providers > 1); > + unsigned int mask; > + int ret, max, num_ctrls; > + > + num_ctrls = has_flash ? 5 : 2; > + if (has_flash) { > + if (flash_ops->flash_brightness_set) > + ++num_ctrls; > + if (flash_ops->timeout_set) > + ++num_ctrls; > + if (flash_ops->strobe_get) > + ++num_ctrls; > + if (has_indicator) > + ++num_ctrls; > + if (config->flash_faults) > + ++num_ctrls; > + if (has_strobe_providers) > + ++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) > + 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 = &config->torch_intensity; > + 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->def); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + v4l2_flash->ctrl.torch_intensity = ctrl; > + > + if (has_flash) { > + /* 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); > + if (v4l2_flash->ctrl.source) > + v4l2_flash->ctrl.source->flags |= > + V4L2_CTRL_FLAG_VOLATILE; > + > + /* 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); I think you only should implement the strobe status control if you really can know it, i.e. have strobe_get(). > + if (flash_ops->strobe_get) > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE | > + V4L2_CTRL_FLAG_READ_ONLY; > + > + if (flash_ops->timeout_set) { > + /* Configure FLASH_TIMEOUT ctrl */ > + ctrl_cfg = &config->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->def); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + } > + > + if (flash_ops->flash_brightness_set) { > + /* Configure FLASH_INTENSITY ctrl */ > + ctrl_cfg = &config->flash_intensity; > + 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->def); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + } > + > + if (config->flash_faults) { > + /* Configure FLASH_FAULT ctrl */ > + ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, > + &v4l2_flash_ctrl_ops, > + V4L2_CID_FLASH_FAULT, 0, > + config->flash_faults, > + 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 = &config->indicator_intensity; > + 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->def); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + } Could you implement the above in a loop? You're essentially repeating the same but with different parameters. > + if (has_strobe_providers) { > + /* Configure V4L2_CID_FLASH_STROBE_PROVIDERS ctrl */ > + ret = v4l2_flash_init_strobe_providers_menu(v4l2_flash); > + if (ret < 0) > + goto error_free_handler; > + } > + } > + > + if (v4l2_flash->hdl.error) { > + ret = v4l2_flash->hdl.error; > + goto error_free_handler; > + } > + > + ret = v4l2_ctrl_handler_setup(&v4l2_flash->hdl); > + if (ret < 0) > + goto error_free_handler; > + > + v4l2_flash->sd.ctrl_handler = &v4l2_flash->hdl; > + > + return 0; > + > +error_free_handler: > + 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_flash *flash = v4l2_flash->flash; > + struct led_classdev *led_cdev = &flash->led_cdev; > + > + mutex_lock(&led_cdev->led_lock); > + call_flash_op(v4l2_flash, sysfs_lock, led_cdev); What if you have the device open through multiple file handles? I believe v4l2_subdev_fh_is_singular(&fh->vfh) would prove helpful here. > + 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_flash *flash = v4l2_flash->flash; > + struct led_classdev *led_cdev = &flash->led_cdev; > + > + mutex_lock(&led_cdev->led_lock); > + call_flash_op(v4l2_flash, sysfs_unlock, led_cdev); > + mutex_unlock(&led_cdev->led_lock); > + > + return 0; > +} > + > +int v4l2_flash_register(struct v4l2_flash *v4l2_flash) Do you expect to call this from elsewhere than this file? > +{ > + struct led_classdev_flash *flash = v4l2_flash->flash; > + struct led_classdev *led_cdev = &flash->led_cdev; > + int ret; > + > + if (!v4l2_dev) { > + v4l2_dev = kzalloc(sizeof(*v4l2_dev), GFP_KERNEL); > + if (!v4l2_dev) > + return -ENOMEM; > + > + strlcpy(v4l2_dev->name, "v4l2-flash-manager", > + sizeof(v4l2_dev->name)); > + ret = v4l2_device_register(NULL, v4l2_dev); > + if (ret < 0) { > + dev_err(led_cdev->dev->parent, > + "Failed to register v4l2_device: %d\n", ret); > + goto err_v4l2_device_register; > + } > + } > + > + ret = v4l2_device_register_subdev(v4l2_dev, &v4l2_flash->sd); > + if (ret < 0) { > + dev_err(led_cdev->dev->parent, > + "Failed to register v4l2_subdev: %d\n", ret); > + goto err_v4l2_device_register; > + } > + > + ret = v4l2_device_register_subdev_node(&v4l2_flash->sd, v4l2_dev); > + if (ret < 0) { > + dev_err(led_cdev->dev->parent, > + "Failed to register v4l2_subdev node: %d\n", ret); > + goto err_register_subdev_node; > + } This way you can create a V4L2 sub-device node. However, flash devices are seldom alone in the system. They are physically close to a sensor, and this connection is shown in the Media controller interface. This means that the flash sub-device (entity) should be part of the Media device created by the driver in control of it. This can be achieved by the master driver creating the sub-device. You should register an async sub-device here. This results in the sub-device not being registered if there's no such master driver, but I wouldn't expect that to be an issue since the V4L2 flash API is mostly relevant in such cases. > + ++registered_flashes; > + > + return 0; > + > +err_register_subdev_node: > + v4l2_device_unregister_subdev(&v4l2_flash->sd); > +err_v4l2_device_register: > + kfree(v4l2_flash->strobe_providers_menu); > + if (v4l2_dev && registered_flashes == 0) { > + v4l2_device_unregister(v4l2_dev); > + kfree(v4l2_dev); > + v4l2_dev = NULL; > + } > + > + return ret; > +} > + > +static void v4l2_flash_unregister(struct v4l2_flash *v4l2_flash) > +{ > + if (registered_flashes == 0) > + return; > + > + v4l2_device_unregister_subdev(&v4l2_flash->sd); > + > + --registered_flashes; > + > + if (registered_flashes == 0) { > + v4l2_device_unregister(v4l2_dev); > + kfree(v4l2_dev); > + v4l2_dev = NULL; > + } > +} > + > +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, > +}; > + > +int v4l2_flash_init(struct led_classdev_flash *flash, > + struct v4l2_flash_ctrl_config *config, > + const struct v4l2_flash_ops *flash_ops, > + struct v4l2_flash **out_flash) > +{ > + struct v4l2_flash *v4l2_flash; > + struct led_classdev *led_cdev = &flash->led_cdev; > + struct v4l2_subdev *sd; > + int ret; > + > + if (!flash || !config || !out_flash) > + return -EINVAL; > + > + v4l2_flash = kzalloc(sizeof(*v4l2_flash), GFP_KERNEL); > + if (!v4l2_flash) > + return -ENOMEM; > + > + sd = &v4l2_flash->sd; > + v4l2_flash->flash = flash; > + v4l2_flash->ops = flash_ops; > + 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); > + > + v4l2_flash->config = *config; > + ret = v4l2_flash_init_controls(v4l2_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_flash_register(v4l2_flash); > + if (ret < 0) > + goto err_init_entity; > + > + *out_flash = v4l2_flash; How about returning the pointer instead? > + return 0; > + > +err_init_entity: > + media_entity_cleanup(&sd->entity); > +err_init_controls: > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + kfree(v4l2_flash); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(v4l2_flash_init); > + > +void v4l2_flash_release(struct v4l2_flash *v4l2_flash) > +{ > + if (!v4l2_flash) > + return; > + > + v4l2_flash_unregister(v4l2_flash); > + v4l2_ctrl_handler_free(v4l2_flash->sd.ctrl_handler); > + media_entity_cleanup(&v4l2_flash->sd.entity); > + kfree(v4l2_flash->strobe_providers_menu); > + kfree(v4l2_flash); > +} > +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..effa46b > --- /dev/null > +++ b/include/media/v4l2-flash.h > @@ -0,0 +1,137 @@ > +/* > + * V4L2 Flash LED sub-device registration helpers. > + * > + * Copyright (C) 2014 Samsung Electronics Co., Ltd > + * Author: Jacek Anaszewski > + * > + * 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 > +#include > +#include > +#include > +#include > + > +struct led_classdev_flash; > +struct led_classdev; > +enum led_brightness; > + > +struct v4l2_flash_ops { > + int (*torch_brightness_set)(struct led_classdev *led_cdev, > + enum led_brightness brightness); > + int (*torch_brightness_update)(struct led_classdev *led_cdev); > + int (*flash_brightness_set)(struct led_classdev_flash *flash, > + u32 brightness); > + int (*flash_brightness_update)(struct led_classdev_flash *flash); > + int (*strobe_set)(struct led_classdev_flash *flash, bool state); > + int (*strobe_get)(struct led_classdev_flash *flash, bool *state); > + int (*timeout_set)(struct led_classdev_flash *flash, u32 timeout); > + int (*indicator_brightness_set)(struct led_classdev_flash *flash, > + u32 brightness); > + int (*indicator_brightness_update)(struct led_classdev_flash *flash); > + int (*external_strobe_set)(struct led_classdev_flash *flash, > + bool enable); > + int (*fault_get)(struct led_classdev_flash *flash, u32 *fault); > + void (*sysfs_lock)(struct led_classdev *led_cdev); > + void (*sysfs_unlock)(struct led_classdev *led_cdev); These functions are not driver specific and there's going to be just one implementation (I suppose). Could you refresh my memory regarding why the LED framework functions aren't called directly? > +}; > + > +/** > + * 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_ctrl_config - V4L2 Flash controls initialization data > + * @torch_intensity: V4L2_CID_FLASH_TORCH_INTENSITY constraints > + * @flash_intensity: V4L2_CID_FLASH_INTENSITY constraints > + * @indicator_intensity: V4L2_CID_FLASH_INDICATOR_INTENSITY constraints > + * @flash_timeout: V4L2_CID_FLASH_TIMEOUT constraints > + * @flash_fault: possible flash faults > + */ > +struct v4l2_flash_ctrl_config { > + struct v4l2_ctrl_config torch_intensity; > + struct v4l2_ctrl_config flash_intensity; > + struct v4l2_ctrl_config indicator_intensity; > + struct v4l2_ctrl_config flash_timeout; > + u32 flash_faults; > +}; > + > +/** > + * struct v4l2_flash - Flash sub-device context > + * @flash: LED Flash Class device controlled by this sub-device > + * @ops: LED Flash Class device ops > + * @sd: V4L2 sub-device > + * @hdl: flash controls handler > + * @ctrl: state defining controls > + * @config: V4L2 Flash controlsrconfiguration data > + * @software_strobe_gates: route to the software strobe signal > + * @external_strobe_gates: route to the external strobe signal > + * @sensors: available external strobe sources > + */ > +struct v4l2_flash { > + struct led_classdev_flash *flash; > + const struct v4l2_flash_ops *ops; > + > + struct v4l2_subdev sd; > + struct v4l2_ctrl_handler hdl; > + struct v4l2_flash_ctrl ctrl; > + struct v4l2_flash_ctrl_config config; > + char **strobe_providers_menu; > +}; > + > +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); > +} > + > +#ifdef CONFIG_V4L2_FLASH_LED_CLASS > +/** > + * v4l2_flash_init - initialize V4L2 flash led sub-device > + * @led_fdev: the LED Flash Class device to wrap > + * @config: initialization data for V4L2 Flash controls > + * @flash_ops: V4L2 Flash device ops > + * @out_flash: handler to the new V4L2 Flash device > + * > + * Create V4L2 subdev wrapping given LED subsystem device. > + > + * Returns: 0 on success or negative error value on failure > + */ > +int v4l2_flash_init(struct led_classdev_flash *led_fdev, > + struct v4l2_flash_ctrl_config *config, > + const struct v4l2_flash_ops *flash_ops, > + struct v4l2_flash **out_flash); > + > +/** > + * v4l2_flash_release - release V4L2 Flash sub-device > + * @flash: the V4L2 Flash device to release > + * > + * Release V4L2 flash led subdev. > + */ > +void v4l2_flash_release(struct v4l2_flash *v4l2_flash); > + > +#else > +#define v4l2_flash_init(led_cdev, config, flash_ops, out_flash) (0) > +#define v4l2_flash_release(v4l2_flash) > +#endif /* CONFIG_V4L2_FLASH_LED_CLASS */ > + > +#endif /* _V4L2_FLASH_H */ > -- Kind regards, Sakari Ailus sakari.ailus@iki.fi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/