| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 11 Aug 2014 15:27:22 +0200
From: Jacek Anaszewski <j.anaszewski@...sung.com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
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 Sakari
On 08/11/2014 02:26 PM, Sakari Ailus wrote:
>
> Hi Jacek,
>
> On Mon, Aug 04, 2014 at 04:43:54PM +0200, Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> Thanks for the review.
>
> You're welcome! :)
>
>> On 07/21/2014 01:12 PM, Sakari Ailus wrote:
>>> 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. :-)
>>
>> The STROBE_SOURCE_EXTERNAL control state is volatile if a flash device
>> depends on muxes that route strobe signals to more then one flash
>> device. In such a case it behaves similarly to FLASH_STROBE control,
>> i.e. it activates external strobe only for the flash timeout period.
>> I touched this issue in the cover letter of this patch series,
>> paragraph 2.
>
> I meant that flash->external_strobe is directly used as
> V4L2_CID_FLASH_STROBE_SOURCE. Are the two guaranteed to be the same?
>
> ...
>
>>>> +/*
>>>> + * 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.
>>
>> I assume you propose to implement such a function? I don't see it in
>> the mainline kernel.
>
> Uh, I thought of v4l2_subdev_fh_is_singular() but what I really meant was
> v4l2_fh_is_singular(). There's no sub-device variant of it, as the
> sub-device file handle struct embeds struct v4l2_fh.
OK, I will use it here.
> ...
>
>>>> +{
>>>> + 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.
>>
>> I addressed this issue in the cover letter of this patch series,
>> paragraph 1. If strobe signals are routed to a flash device through
>> a multiplexer then assignment of the related V4L2 Flash sub-device
>> to a particular media controller may dynamically change in time.
>> Actually, if a flash device can be shared between media systems
>> (i.e. it can be configured to react on strobe signals from different
>> camera sensors, basing on muxes configuration), than it becomes
>> bound to a particular media system only for the time of strobing.
>> Please refer to [1].
>
> Replied to the cover letter.
Replied to your reply.
> ...
>
>>>> 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>le
>>>> +#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?
>>
>> These ops are required to make possible building led-class-flash as
>> a kernel module.
>
> Assuming you'd use the actual implementation directly, what would be the
> dependencies? I don't think the LED flash framework has any callbacks
> towards the V4L2 (LED) flash framework, does it? Please correct my
> understanding if I'm missing something. In Makefile format, assume all
> targets are .PHONY:
>
> led-flash-api: led-api
>
> v4l2-flash: led-flash-api
>
> driver: led-flash-api v4l2-flash
LED Class Flash driver gains V4L2 Flash API when
CONFIG_V4L2_FLASH_LED_CLASS is defined. This is accomplished in
the probe function by either calling v4l2_flash_init function
or the macro of this name, when the CONFIG_V4L2_FLASH_LED_CLASS
macro isn't defined.
If the v4l2-flash.c was to call the LED API directly, then the
led-class-flash module symbols would have to be available at
v4l2-flash.o linking time.
This requirement cannot be met if the led-class-flash is built
as a module.
Use of function pointers in the v4l2-flash.c allows to compile it
into the kernel and enables the possibility of adding the V4L2 Flash
support conditionally, during driver probing.
Best Regards,
Jacek Anaszewski
--
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