Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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 */
>