MIME-version: 1.0
Content-type: text/plain; charset=windows-1252; format=flowed
Content-transfer-encoding: 8BIT
Message-id: <54D896EA.4090704@samsung.com>
Date: Mon, 09 Feb 2015 12:15:54 +0100
From: Jacek Anaszewski <j.anaszewski@samsung.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804
 Thunderbird/17.0.8
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: linux-leds@vger.kernel.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        kyungmin.park@samsung.com, b.zolnierkie@samsung.com, pavel@ucw.cz,
        cooloney@gmail.com, rpurdie@rpsys.net, s.nawrocki@samsung.com,
        Hans Verkuil <hans.verkuil@cisco.com>
Subject: Re: [PATCH/RFC v10 15/19] media: Add registration helpers for V4L2
 flash sub-devices
References: <1420816989-1808-1-git-send-email-j.anaszewski@samsung.com>
 <1420816989-1808-16-git-send-email-j.anaszewski@samsung.com>
 <20150205175901.GG32575@valkosipuli.retiisi.org.uk>
In-reply-to: <20150205175901.GG32575@valkosipuli.retiisi.org.uk>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 29267
Lines: 888

Hi Sakari,

Thanks for the review.

On 02/05/2015 06:59 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> Thank you for your continuous efforts on this! I think this ended up being
> more complicated than I originally anticipated.
>
> On Fri, Jan 09, 2015 at 04:23:05PM +0100, Jacek Anaszewski wrote:
>> This patch adds helper functions for registering/unregistering
>> LED Flash class devices as V4L2 sub-devices. The functions should
>> be called from the LED subsystem device driver. In case the
>> support for V4L2 Flash sub-devices is disabled in the kernel
>> config the functions' empty versions will be used.
>>
>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>   drivers/media/v4l2-core/Kconfig      |   11 +
>>   drivers/media/v4l2-core/Makefile     |    2 +
>>   drivers/media/v4l2-core/v4l2-flash.c |  581 ++++++++++++++++++++++++++++++++++
>>   include/media/v4l2-flash.h           |  139 ++++++++
>>   4 files changed, 733 insertions(+)
>>   create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
>>   create mode 100644 include/media/v4l2-flash.h
>>
>> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
>> index ba7e21a..f034f1a 100644
>> --- a/drivers/media/v4l2-core/Kconfig
>> +++ b/drivers/media/v4l2-core/Kconfig
>> @@ -44,6 +44,17 @@ config V4L2_MEM2MEM_DEV
>>           tristate
>>           depends on VIDEOBUF2_CORE
>>
>> +# Used by LED subsystem flash drivers
>> +config V4L2_FLASH_LED_CLASS
>> +	tristate "Enable support for Flash sub-devices"
>> +	depends on VIDEO_V4L2_SUBDEV_API
>> +	depends on LEDS_CLASS_FLASH
>> +	---help---
>> +	  Say Y here to enable support for Flash sub-devices, which allow
>> +	  to control LED class devices with use of V4L2 Flash controls.
>> +
>> +	  When in doubt, say N.
>> +
>>   # Used by drivers that need Videobuf modules
>>   config VIDEOBUF_GEN
>>   	tristate
>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> index 63d29f2..44e858c 100644
>> --- a/drivers/media/v4l2-core/Makefile
>> +++ b/drivers/media/v4l2-core/Makefile
>> @@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
>>
>>   obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
>>
>> +obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o
>> +
>>   obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
>>   obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
>>   obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
>> diff --git a/drivers/media/v4l2-core/v4l2-flash.c b/drivers/media/v4l2-core/v4l2-flash.c
>> new file mode 100644
>> index 0000000..3fd6a08
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-flash.c
>> @@ -0,0 +1,581 @@
>> +/*
>> + * V4L2 Flash LED sub-device registration helpers.
>> + *
>> + *	Copyright (C) 2015 Samsung Electronics Co., Ltd
>> + *	Author: Jacek Anaszewski <j.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/led-class-flash.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <media/v4l2-flash.h>
>> +
>> +#define has_flash_op(v4l2_flash, op)			\
>> +	(v4l2_flash && v4l2_flash->ops->op)
>> +
>> +#define call_flash_op(v4l2_flash, op, args...)		\
>> +		(has_flash_op(v4l2_flash, op) ?		\
>> +			v4l2_flash->ops->op(args) :	\
>
> Wouldn't you need __VA_ARGS__ here to deliver the variable argument to the
> callee? Do you need variable arguments for the current flash ops?

Indeed, it would allow to avoid the need for passing v4l2_flash
argument twice. Nonetheless, as currently we have only one op, the macro
can have fixed number of arguments.

>> +			-EINVAL)
>> +
>> +static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
>
> I'd drop inline and let the compiler decide. Same below.

Yes, this is a residue from the previous versions when the function
had single line probably.

>> +					struct v4l2_ctrl **ctrls,
>> +					enum ctrl_init_data_id cdata_id,
>> +					s32 intensity)
>> +{
>> +	struct v4l2_ctrl *ctrl = ctrls[cdata_id];
>> +	s64 __intensity = intensity - ctrl->minimum;
>> +
>> +	do_div(__intensity, ctrl->step);
>> +
>> +	/*
>> +	 * Indicator leds, unlike torch leds, are turned on/off basing on
>> +	 * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
>> +	 * Therefore it must be possible to set it to 0 level which in
>> +	 * the LED subsystem reflects LED_OFF state.
>> +	 */
>> +	if (cdata_id != INDICATOR_INTENSITY)
>> +		++__intensity;
>> +
>> +	return __intensity;
>> +}
>> +
>> +static inline s32 v4l2_flash_led_brightness_to_intensity(
>> +					struct v4l2_ctrl **ctrls,
>> +					enum ctrl_init_data_id cdata_id,
>
> Could you pass a pointer to struct v4l2_ctrl instead?

Good point.

>> +					enum led_brightness brightness)
>> +{
>> +	struct v4l2_ctrl *ctrl = ctrls[cdata_id];
>> +
>> +	/*
>> +	 * Indicator leds, unlike torch leds, are turned on/off basing on
>> +	 * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
>> +	 * Do not decrement brightness read from the LED subsystem for
>> +	 * indicator led as it may equal 0. For torch leds this function
>> +	 * is called only when V4L2_FLASH_LED_MODE_TORCH is set and the
>> +	 * brightness read is guaranteed to be greater than 0. In the mode
>> +	 * V4L2_FLASH_LED_MODE_NONE the cached torch intensity value is used.
>> +	 */
>> +	if (cdata_id != INDICATOR_INTENSITY)
>> +		--brightness;
>> +
>> +	return (brightness * ctrl->step) + ctrl->minimum;
>> +}
>> +
>> +static int v4l2_flash_g_volatile_ctrl(struct v4l2_ctrl *c)
>> +{
>> +	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>> +	bool is_strobing;
>> +	int ret;
>> +
>> +	switch (c->id) {
>> +	case V4L2_CID_FLASH_TORCH_INTENSITY:
>> +		/*
>> +		 * Update torch brightness only if in TORCH_MODE.
>> +		 * In other modes torch led is turned off, which
>> +		 * would spuriously inform the user space that
>> +		 * V4L2_CID_FLASH_TORCH_INTENSITY control setting
>> +		 * has changed.
>> +		 */
>> +		if (ctrls[LED_MODE]->val == V4L2_FLASH_LED_MODE_TORCH) {
>> +			ret = led_update_brightness(led_cdev);
>> +			if (ret < 0)
>> +				return ret;
>> +			c->val = v4l2_flash_led_brightness_to_intensity(
>> +							ctrls, TORCH_INTENSITY,
>> +							led_cdev->brightness);
>> +		}
>> +		return 0;
>> +	case V4L2_CID_FLASH_INDICATOR_INTENSITY:
>> +		ret = led_update_brightness(led_cdev);
>> +		if (ret < 0)
>> +			return ret;
>> +		c->val = v4l2_flash_led_brightness_to_intensity(
>> +						ctrls, INDICATOR_INTENSITY,
>> +						led_cdev->brightness);
>> +		return 0;
>> +	case V4L2_CID_FLASH_INTENSITY:
>> +		ret = led_update_flash_brightness(fled_cdev);
>> +		if (ret < 0)
>> +			return ret;
>> +		/* no conversion is needed */
>> +		c->val = fled_cdev->brightness.val;
>> +		return 0;
>> +	case V4L2_CID_FLASH_STROBE_STATUS:
>> +		ret = led_get_flash_strobe(fled_cdev, &is_strobing);
>> +		if (ret < 0)
>> +			return ret;
>> +		c->val = is_strobing;
>> +		return 0;
>> +	case V4L2_CID_FLASH_FAULT:
>> +		/* led faults map directly to V4L2 flash faults */
>> +		return led_get_flash_fault(fled_cdev, &c->val);
>> +	case V4L2_CID_FLASH_SYNC_STROBE:
>> +		c->val = fled_cdev->sync_led_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 *fled_cdev = v4l2_flash->fled_cdev;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>> +	enum led_brightness brightness;
>> +	bool external_strobe;
>> +	int ret = 0;
>> +
>> +	switch (c->id) {
>> +	case V4L2_CID_FLASH_LED_MODE:
>> +		switch (c->val) {
>> +		case V4L2_FLASH_LED_MODE_NONE:
>> +			led_set_brightness(led_cdev, LED_OFF);
>> +			return led_set_flash_strobe(fled_cdev, false);
>> +		case V4L2_FLASH_LED_MODE_FLASH:
>> +			/* Turn the torch LED off */
>> +			led_set_brightness(led_cdev, LED_OFF);
>> +			external_strobe = (ctrls[STROBE_SOURCE]->val ==
>> +					V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
>> +
>> +			if (has_flash_op(v4l2_flash, external_strobe_set))
>> +				ret = call_flash_op(v4l2_flash,
>> +						external_strobe_set, v4l2_flash,
>> +						external_strobe);
>> +			return ret;
>> +		case V4L2_FLASH_LED_MODE_TORCH:
>> +			/* Stop flash strobing */
>> +			ret = led_set_flash_strobe(fled_cdev, false);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			brightness =
>> +				v4l2_flash_intensity_to_led_brightness(
>> +						ctrls, TORCH_INTENSITY,
>> +						ctrls[TORCH_INTENSITY]->val);
>> +			led_set_brightness(led_cdev, brightness);
>> +			return 0;
>> +		}
>> +		break;
>> +	case V4L2_CID_FLASH_STROBE_SOURCE:
>> +		external_strobe = (c->val == V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
>> +
>> +		return call_flash_op(v4l2_flash, external_strobe_set,
>> +					v4l2_flash, external_strobe);
>> +	case V4L2_CID_FLASH_STROBE:
>> +		if (ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_FLASH ||
>> +		    ctrls[STROBE_SOURCE]->val !=
>> +					V4L2_FLASH_STROBE_SOURCE_SOFTWARE)
>> +			return -EINVAL;
>> +		return led_set_flash_strobe(fled_cdev, true);
>> +	case V4L2_CID_FLASH_STROBE_STOP:
>> +		if (ctrls[LED_MODE]->val != V4L2_FLASH_LED_MODE_FLASH ||
>> +		    ctrls[STROBE_SOURCE]->val !=
>> +					V4L2_FLASH_STROBE_SOURCE_SOFTWARE)
>> +			return -EINVAL;
>> +		return led_set_flash_strobe(fled_cdev, false);
>> +	case V4L2_CID_FLASH_TIMEOUT:
>> +		/* no conversion is needed */
>> +		return led_set_flash_timeout(fled_cdev, c->val);
>> +	case V4L2_CID_FLASH_INTENSITY:
>> +		/* no conversion is needed */
>> +		return led_set_flash_brightness(fled_cdev, c->val);
>> +	case V4L2_CID_FLASH_INDICATOR_INTENSITY:
>> +		brightness = v4l2_flash_intensity_to_led_brightness(
>> +						ctrls, INDICATOR_INTENSITY,
>> +						c->val);
>> +		led_set_brightness(led_cdev, brightness);
>> +		return 0;
>> +	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 (ctrls[LED_MODE]->val == V4L2_FLASH_LED_MODE_TORCH) {
>> +			brightness =
>> +				v4l2_flash_intensity_to_led_brightness(
>> +							ctrls, TORCH_INTENSITY,
>> +							c->val);
>> +			led_set_brightness(led_cdev, brightness);
>> +		}
>> +		return 0;
>> +	case V4L2_CID_FLASH_SYNC_STROBE:
>> +		fled_cdev->sync_led_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 void fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
>> +			  struct v4l2_flash_ctrl_config *flash_ctrl_cfg,
>> +			  struct v4l2_flash_ctrl_data *ctrl_init_data)
>> +{
>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>> +	const struct led_flash_ops *fled_cdev_ops = fled_cdev->ops;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	struct v4l2_ctrl_config *ctrl_cfg;
>> +	u32 mask;
>> +	s64 max;
>> +
>> +	/* Init FLASH_FAULT ctrl data */
>> +	if (flash_ctrl_cfg->flash_faults) {
>> +		ctrl_init_data[FLASH_FAULT].supported = true;
>> +		ctrl_cfg = &ctrl_init_data[FLASH_FAULT].config;
>> +		ctrl_cfg->id = V4L2_CID_FLASH_FAULT;
>> +		ctrl_cfg->max = flash_ctrl_cfg->flash_faults;
>> +		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
>> +				  V4L2_CTRL_FLAG_READ_ONLY;
>> +	}
>> +
>> +	/* Init INDICATOR_INTENSITY ctrl data */
>> +	if (flash_ctrl_cfg->indicator_led) {
>> +		ctrl_init_data[INDICATOR_INTENSITY].supported = true;
>> +		ctrl_init_data[INDICATOR_INTENSITY].config =
>> +						flash_ctrl_cfg->intensity;
>> +		ctrl_cfg = &ctrl_init_data[INDICATOR_INTENSITY].config;
>> +		ctrl_cfg->id = V4L2_CID_FLASH_INDICATOR_INTENSITY;
>> +		ctrl_cfg->min = 0;
>> +		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE;
>> +
>> +		/* Indicator LED can have only faults and intensity controls. */
>> +		return;
>> +	}
>> +
>> +	/* Init FLASH_LED_MODE ctrl data */
>> +	mask = 1 << V4L2_FLASH_LED_MODE_NONE |
>> +	       1 << V4L2_FLASH_LED_MODE_TORCH;
>> +	if (led_cdev->flags & LED_DEV_CAP_FLASH)
>> +		mask |= 1 << V4L2_FLASH_LED_MODE_FLASH;
>> +
>> +	ctrl_init_data[LED_MODE].supported = true;
>> +	ctrl_cfg = &ctrl_init_data[LED_MODE].config;
>> +	ctrl_cfg->id = V4L2_CID_FLASH_LED_MODE;
>> +	ctrl_cfg->max = V4L2_FLASH_LED_MODE_TORCH;
>> +	ctrl_cfg->menu_skip_mask = ~mask;
>> +	ctrl_cfg->def = V4L2_FLASH_LED_MODE_NONE;
>> +	ctrl_cfg->flags = 0;
>> +
>> +	/* Init TORCH_INTENSITY ctrl data */
>> +	ctrl_init_data[TORCH_INTENSITY].supported = true;
>> +	ctrl_init_data[TORCH_INTENSITY].config = flash_ctrl_cfg->intensity;
>> +	ctrl_cfg = &ctrl_init_data[TORCH_INTENSITY].config;
>> +	ctrl_cfg->id = V4L2_CID_FLASH_TORCH_INTENSITY;
>> +	ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE;
>> +
>> +	if (!(led_cdev->flags & LED_DEV_CAP_FLASH))
>> +		return;
>> +
>> +	/* Init FLASH_STROBE_SOURCE ctrl data */
>> +	mask = 1 << V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
>> +	if (flash_ctrl_cfg->has_external_strobe) {
>> +		mask |= 1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL;
>> +		max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL;
>> +	} else {
>> +		max = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
>> +	}
>> +
>> +	ctrl_init_data[STROBE_SOURCE].supported = true;
>> +	ctrl_cfg = &ctrl_init_data[STROBE_SOURCE].config;
>> +	ctrl_cfg->id = V4L2_CID_FLASH_STROBE_SOURCE;
>> +	ctrl_cfg->max = max;
>> +	ctrl_cfg->menu_skip_mask = ~mask;
>> +	ctrl_cfg->def = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
>> +
>> +	/* Init FLASH_STROBE ctrl data */
>> +	ctrl_init_data[FLASH_STROBE].supported = true;
>> +	ctrl_cfg = &ctrl_init_data[FLASH_STROBE].config;
>> +	ctrl_cfg->id = V4L2_CID_FLASH_STROBE;
>> +
>> +	/* Init STROBE_STOP ctrl data */
>> +	ctrl_init_data[STROBE_STOP].supported = true;
>> +	ctrl_cfg = &ctrl_init_data[STROBE_STOP].config;
>> +	ctrl_cfg->id = V4L2_CID_FLASH_STROBE_STOP;
>> +
>> +	/* Init STROBE_STATUS ctrl data */
>> +	if (fled_cdev_ops->strobe_get) {
>> +		ctrl_init_data[STROBE_STATUS].supported = true;
>> +		ctrl_cfg = &ctrl_init_data[STROBE_STATUS].config;
>> +		ctrl_cfg->id = V4L2_CID_FLASH_STROBE_STATUS;
>> +		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
>> +				  V4L2_CTRL_FLAG_READ_ONLY;
>> +	}
>> +
>> +	/* Init FLASH_TIMEOUT ctrl data */
>> +	if (fled_cdev_ops->timeout_set) {
>> +		ctrl_init_data[FLASH_TIMEOUT].supported = true;
>> +		ctrl_init_data[FLASH_TIMEOUT].config =
>> +					flash_ctrl_cfg->flash_timeout;
>> +		ctrl_cfg = &ctrl_init_data[FLASH_TIMEOUT].config;
>> +		ctrl_cfg->id = V4L2_CID_FLASH_TIMEOUT;
>> +		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE;
>> +	}
>> +
>> +	/* Init FLASH_INTENSITY ctrl data */
>> +	if (fled_cdev_ops->flash_brightness_set) {
>> +		ctrl_init_data[FLASH_INTENSITY].supported = true;
>> +		ctrl_init_data[FLASH_INTENSITY].config =
>> +					flash_ctrl_cfg->flash_intensity;
>> +		ctrl_cfg = &ctrl_init_data[FLASH_INTENSITY].config;
>> +		ctrl_cfg->id = V4L2_CID_FLASH_INTENSITY;
>> +		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE;
>> +	}
>> +}
>> +
>> +static int v4l2_flash_init_sync_strobe_menu(struct v4l2_flash *v4l2_flash)
>> +{
>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>> +	struct v4l2_ctrl *ctrl;
>> +	int i = 0;
>> +
>> +	v4l2_flash->sync_strobe_menu =
>> +			devm_kcalloc(fled_cdev->led_cdev.dev->parent,
>> +					fled_cdev->num_sync_leds + 1,
>> +					sizeof(*fled_cdev),
>> +					GFP_KERNEL);
>> +
>> +	if (!v4l2_flash->sync_strobe_menu)
>> +		return -ENOMEM;
>> +
>> +	v4l2_flash->sync_strobe_menu[0] = "none";
>> +
>> +	for (i = 0; i < fled_cdev->num_sync_leds; ++i)
>> +		v4l2_flash->sync_strobe_menu[i + 1] =
>> +				(char *) fled_cdev->sync_leds[i]->led_cdev.name;
>
> What's here, and where does it come from?
>
> The rest of the sub-devices typically have a device's (chip's) name and the
> bus address (usually i2c).

It comes from the struct led_classdev's 'name' property, which is
assigned a pointer to the parsed 'label' DT property (LEDs common 
bindings) string. It has to be unique as it is passed to the
device_create_with_groups in the led-class.c

We can't use device's name and I2C bus address, as a v4l2-flash sub-dev
is created per discrete LED element, not per device (many v4l2-flash
sub-devs can be created per one flash LED device).

>> +	ctrl = v4l2_ctrl_new_std_menu_items(
>> +		&v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
>> +		V4L2_CID_FLASH_SYNC_STROBE,
>> +		fled_cdev->num_sync_leds,
>> +		0, 0,
>> +		(const char * const *) v4l2_flash->sync_strobe_menu);
>
> Do you need the casting here?

Yes, there is warning generated without it:

drivers/media/v4l2-core/v4l2-flash.c:364:3: warning: passing argument 7 
of ?v4l2_ctrl_new_std_menu_items? from incompatible pointer type 
[enabled by default]
include/media/v4l2-ctrls.h:408:19: note: expected ?const char * const*? 
but argument is of type ?char **?

>> +
>> +	if (ctrl)
>> +		ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
>> +
>> +	return 0;
>> +}
>> +
>> +static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
>> +				struct v4l2_flash_ctrl_config *flash_ctrl_cfg)
>> +
>> +{
>> +	struct led_classdev *led_cdev = &v4l2_flash->fled_cdev->led_cdev;
>> +	struct v4l2_flash_ctrl_data *ctrl_init_data;
>> +	struct v4l2_ctrl *ctrl;
>> +	struct v4l2_ctrl_config *ctrl_cfg;
>> +	int i, ret, num_ctrls = 0;
>> +
>> +	/* allocate memory dynamically so as not to exceed stack frame size */
>> +	ctrl_init_data = kcalloc(NUM_FLASH_CTRLS, sizeof(*ctrl_init_data),
>> +					GFP_KERNEL);
>> +	if (!ctrl_init_data)
>> +		return -ENOMEM;
>> +
>> +	memset(ctrl_init_data, 0, sizeof(*ctrl_init_data));
>
> kcalloc() clears the memory, no need to do it again.

Right.

>> +	fill_ctrl_init_data(v4l2_flash, flash_ctrl_cfg, ctrl_init_data);
>> +
>> +	for (i = 0; i < NUM_FLASH_CTRLS; ++i)
>> +		if (ctrl_init_data[i].supported)
>> +			++num_ctrls;
>> +
>> +	v4l2_ctrl_handler_init(&v4l2_flash->hdl, num_ctrls);
>> +
>> +	for (i = 0; i < NUM_FLASH_CTRLS; ++i) {
>> +		ctrl_cfg = &ctrl_init_data[i].config;
>> +		if (!ctrl_init_data[i].supported)
>> +			continue;
>> +
>> +		if (ctrl_cfg->id == V4L2_CID_FLASH_LED_MODE ||
>> +		    ctrl_cfg->id == V4L2_CID_FLASH_STROBE_SOURCE)
>> +			ctrl = v4l2_ctrl_new_std_menu(&v4l2_flash->hdl,
>> +						&v4l2_flash_ctrl_ops,
>> +						ctrl_cfg->id,
>> +						ctrl_cfg->max,
>> +						ctrl_cfg->menu_skip_mask,
>> +						ctrl_cfg->def);
>> +		else
>> +			ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl,
>> +						&v4l2_flash_ctrl_ops,
>> +						ctrl_cfg->id,
>> +						ctrl_cfg->min,
>> +						ctrl_cfg->max,
>> +						ctrl_cfg->step,
>> +						ctrl_cfg->def);
>> +
>> +		if (ctrl)
>> +			ctrl->flags |= ctrl_cfg->flags;
>> +
>> +		if (i <= STROBE_SOURCE)
>> +			v4l2_flash->ctrls[i] = ctrl;
>> +	}
>> +
>> +	kfree(ctrl_init_data);
>> +
>> +	if (led_cdev->flags & LED_DEV_CAP_SYNC_STROBE) {
>> +		ret = v4l2_flash_init_sync_strobe_menu(v4l2_flash);
>> +		if (ret < 0)
>> +			goto error_free_handler;
>> +	}
>> +
>> +	if (v4l2_flash->hdl.error) {
>> +		ret = v4l2_flash->hdl.error;
>> +		goto error_free_handler;
>> +	}
>> +
>> +	v4l2_ctrl_handler_setup(&v4l2_flash->hdl);
>> +
>> +	v4l2_flash->sd.ctrl_handler = &v4l2_flash->hdl;
>> +
>> +	return 0;
>> +
>> +error_free_handler:
>> +	v4l2_ctrl_handler_free(&v4l2_flash->hdl);
>> +	return ret;
>> +}
>> +
>> +/*
>> + * V4L2 subdev internal operations
>> + */
>> +
>> +static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&led_cdev->led_access);
>> +
>> +	if (!v4l2_fh_is_singular(&fh->vfh)) {
>> +		ret = -EBUSY;
>> +		goto unlock;
>> +	}
>> +
>> +	led_sysfs_disable(led_cdev);
>> +
>> +unlock:
>> +	mutex_unlock(&led_cdev->led_access);
>> +	return ret;
>> +}
>> +
>> +static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&led_cdev->led_access);
>> +
>> +	if (has_flash_op(v4l2_flash, external_strobe_set))
>> +		ret = call_flash_op(v4l2_flash, external_strobe_set,
>> +				v4l2_flash, false);
>> +	led_sysfs_enable(led_cdev);
>> +
>> +	mutex_unlock(&led_cdev->led_access);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct v4l2_subdev_internal_ops v4l2_flash_subdev_internal_ops = {
>> +	.open = v4l2_flash_open,
>> +	.close = v4l2_flash_close,
>> +};
>> +
>> +static const struct v4l2_subdev_core_ops v4l2_flash_core_ops = {
>> +	.queryctrl = v4l2_subdev_queryctrl,
>> +	.querymenu = v4l2_subdev_querymenu,
>> +};
>> +
>> +static const struct v4l2_subdev_ops v4l2_flash_subdev_ops = {
>> +	.core = &v4l2_flash_core_ops,
>> +};
>> +
>> +struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
>> +				   const struct v4l2_flash_ops *ops,
>> +				   struct v4l2_flash_ctrl_config *config)
>> +{
>> +	struct v4l2_flash *v4l2_flash;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	struct v4l2_subdev *sd;
>> +	int ret;
>> +
>> +	if (!fled_cdev || !ops || !config)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	v4l2_flash = kzalloc(sizeof(*v4l2_flash), GFP_KERNEL);
>> +	if (!v4l2_flash)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	sd = &v4l2_flash->sd;
>> +	v4l2_flash->fled_cdev = fled_cdev;
>> +	v4l2_flash->ops = ops;
>> +	sd->dev = led_cdev->dev;
>> +	v4l2_subdev_init(sd, &v4l2_flash_subdev_ops);
>> +	sd->internal_ops = &v4l2_flash_subdev_internal_ops;
>> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	snprintf(sd->name, sizeof(sd->name), led_cdev->name);
>
> strlcpy() or snprintf(sd->name, sizeof(), "%s", led_cdev->name).

I think that strlcpy will fit here best.

>> +
>> +	ret = v4l2_flash_init_controls(v4l2_flash, config);
>> +	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 v4l2_flash;
>> +
>> +err_init_entity:
>> +	media_entity_cleanup(&sd->entity);
>
> media_entity_cleanup() can be called on an uninitialised entity (as long as
> it's zeroed). So you could simplity this a little. Up to you.
>
>> +err_init_controls:
>> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
>> +	kfree(v4l2_flash);
>> +
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_flash_init);
>> +
>> +void v4l2_flash_release(struct v4l2_flash *v4l2_flash)
>> +{
>> +	struct v4l2_subdev *sd = &v4l2_flash->sd;
>> +
>> +	if (!v4l2_flash)
>> +		return;
>> +
>> +	v4l2_async_unregister_subdev(sd);
>> +	media_entity_cleanup(&sd->entity);
>> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
>> +	kfree(v4l2_flash);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_flash_release);
>> +
>> +MODULE_AUTHOR("Jacek Anaszewski <j.anaszewski@samsung.com>");
>> +MODULE_DESCRIPTION("V4L2 Flash sub-device helpers");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h
>> new file mode 100644
>> index 0000000..404b5a8
>> --- /dev/null
>> +++ b/include/media/v4l2-flash.h
>> @@ -0,0 +1,139 @@
>> +/*
>> + * V4L2 Flash LED sub-device registration helpers.
>> + *
>> + *	Copyright (C) 2015 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."
>
> There's an unbalanced quote here. Is that intended? Same in the .c file.

This is unintended, to be removed.

>> + */
>> +
>> +#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>
>
> Do you need all the above headers here?

Actually no - only media/v4l2-ctrls.h nad media/v4l2-subdev.h are
required. I will fix this.

>> +
>> +struct led_classdev_flash;
>> +struct led_classdev;
>> +struct v4l2_flash;
>> +enum led_brightness;
>> +
>> +enum ctrl_init_data_id {
>> +	LED_MODE,
>> +	TORCH_INTENSITY,
>> +	INDICATOR_INTENSITY,
>> +	STROBE_SOURCE,
>> +	FLASH_STROBE,
>> +	STROBE_STOP,
>> +	STROBE_STATUS,
>
> I think it'd be good to say e.g. in a comment here which values are still
> applicable to the struct v4l2_flash field ctrls.

OK.

>> +	FLASH_TIMEOUT,
>> +	FLASH_INTENSITY,
>> +	FLASH_FAULT,
>> +	SYNC_STROBE,
>> +	NUM_FLASH_CTRLS,
>> +};
>> +
>> +/*
>> + * struct v4l2_flash_ctrl_data - flash control initialization data -
>> + *				 filled basing on the features declared
>> + *				 by the LED Flash class driver
>> + * @config:	initialization data for a control
>> + * @supported:	indicates whether a control is supported
>> + *		by the LED Flash class driver
>> + */
>> +struct v4l2_flash_ctrl_data {
>> +	struct v4l2_ctrl_config config;
>> +	bool supported;
>
> Every control has an id, right? Could you use that one instead of
> "supported"?

OK.

>> +};
>> +
>> +struct v4l2_flash_ops {
>> +	/* setup strobing the flash by hardware pin state assertion */
>> +	int (*external_strobe_set)(struct v4l2_flash *v4l2_flash,
>> +					bool enable);
>> +};
>> +
>> +/**
>> + * struct v4l2_flash_ctrl_config - V4L2 Flash controls initialization data
>> + * @intensity:			constraints for the led in a non-flash mode
>> + * @flash_intensity:		V4L2_CID_FLASH_INTENSITY settings constraints
>> + * @flash_timeout:		V4L2_CID_FLASH_TIMEOUT constraints
>> + * @flash_faults:		possible flash faults
>> + * @has_external_strobe:	external strobe capability
>> + * @indicator_led:		signifies that a led is of indicator type
>> + */
>> +struct v4l2_flash_ctrl_config {
>> +	struct v4l2_ctrl_config intensity;
>> +	struct v4l2_ctrl_config flash_intensity;
>> +	struct v4l2_ctrl_config flash_timeout;
>> +	u32 flash_faults;
>> +	bool has_external_strobe:1;
>> +	bool indicator_led:1;
>> +};
>> +
>> +/**
>> + * struct v4l2_flash - Flash sub-device context
>> + * @fled_cdev:		LED Flash Class device controlled by this sub-device
>> + * @ops:		V4L2 specific flash ops
>> + * @sd:			V4L2 sub-device
>> + * @hdl:		flash controls handler
>> + * @ctrls:		array of pointers to controls, whose values define
>> + *			the sub-device state
>> + * @sync_strobe_menu	leds available for flash strobe synchronization
>> + */
>> +struct v4l2_flash {
>> +	struct led_classdev_flash *fled_cdev;
>> +	const struct v4l2_flash_ops *ops;
>> +
>> +	struct v4l2_subdev sd;
>> +	struct v4l2_ctrl_handler hdl;
>> +	struct v4l2_ctrl *ctrls[STROBE_SOURCE + 1];
>> +	char **sync_strobe_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);
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
>> +/**
>> + * v4l2_flash_init - initialize V4L2 flash led sub-device
>> + * @fled_cdev:	the LED Flash Class device to wrap
>> + * @flash_ops:	V4L2 Flash device ops
>> + * @config:	initialization data for V4L2 Flash controls
>> + *
>> + * Create V4L2 subdev wrapping given LED subsystem device.
>> + *
>> + * Returns: A valid pointer, or, when an error occurs, the return
>> + * value is encoded using ERR_PTR(). Use IS_ERR() to check and
>> + * PTR_ERR() to obtain the numeric return value.
>> + */
>> +struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
>> +				   const struct v4l2_flash_ops *ops,
>> +				   struct v4l2_flash_ctrl_config *config);
>> +
>> +/**
>> + * v4l2_flash_release - release V4L2 Flash sub-device
>> + * @flash: the V4L2 Flash device to release
>> + *
>> + * Release V4L2 flash led subdev.
>> + */
>> +void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
>> +
>> +#else
>> +#define v4l2_flash_init(fled_cdev, ops, config) (NULL)
>> +#define v4l2_flash_release(v4l2_flash)
>> +#endif /* CONFIG_V4L2_FLASH_LED_CLASS */
>> +
>> +#endif /* _V4L2_FLASH_H */
>


-- 
Best Regards,
Jacek Anaszewski
--
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/