Date:	Mon, 21 Jul 2014 14:12:30 +0300
From:	Sakari Ailus <sakari.ailus@....fi>
To:	Jacek Anaszewski <j.anaszewski@...sung.com>,
	linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
CC:	kyungmin.park@...sung.com, b.zolnierkie@...sung.com,
	Hans Verkuil <hans.verkuil@...co.com>
Subject: Re: [PATCH/RFC v4 15/21] media: Add registration helpers for V4L2
 flash

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 <j.anaszewski@...sung.com>
> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
> Cc: Sakari Ailus <sakari.ailus@....fi>
> Cc: Hans Verkuil <hans.verkuil@...co.com>
> ---
>  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 <j.anaszewski@...sung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation."
> + */
> +
> +#include <linux/led-class-flash.h>
> +#include <linux/mutex.h>
> +#include <linux/of_led_flash_manager.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-flash.h>
> +
> +#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 <j.anaszewski@...sung.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>
> +
> +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@....fi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists