Message-ID: <20140331093721.GC4522@valkosipuli.retiisi.org.uk>
Date:	Mon, 31 Mar 2014 12:37:21 +0300
From:	Sakari Ailus <sakari.ailus@....fi>
To:	Jacek Anaszewski <j.anaszewski@...sung.com>
Cc:	linux-media@...r.kernel.org, linux-leds@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	s.nawrocki@...sung.com, a.hajda@...sung.com,
	kyungmin.park@...sung.com
Subject: Re: [PATCH/RFC 4/8] media: Add registration helpers for V4L2 flash
 sub-devices

Hi jacek,

On Fri, Mar 28, 2014 at 04:30:49PM +0100, Jacek Anaszewski wrote:
...
> >>+static int v4l2_flash_set_intensity(struct v4l2_flash *flash,
> >>+				    unsigned int intensity)
> >>+{
> >>+	struct led_classdev *led_cdev = flash->led_cdev;
> >>+	unsigned int fault;
> >>+	int ret;
> >>+
> >>+	ret = led_get_flash_fault(led_cdev, &fault);
> >>+	if (ret < 0 || fault)
> >>+		return -EINVAL;
> >
> >Is it meaningful to check the faults here?
> >
> >The existing flash controller drivers mostly do not. The responsibility is
> >left to the user --- something the user should probably do after the strobe
> >has expectedly finished. This isn't particularly very well documented in the
> >spec, though.
> 
> I was influenced by the documentation which says that sometimes strobing
> the flash may not be possible due to faults. But I agree that checking
> the faults should be user's responsibility.

What that means is that the presence of the fault *on the chip* may limit
the device's willingness to actually strobe. The driver does not need to
enforce it; the chip does.

> >Also, the presence of every fault does not prevent using the flash.
> >
> >>+	led_set_brightness(led_cdev, intensity);
> >
> >Where do you convert between the LED framework brightness and the value used
> >by the V4L2 controls?
> 
> I think that there is no need for conversion. AFAIK the LED subsystem
> doesn't specify units of the brightness. It specifies LED_FULL enum
> value but not all LED drivers stick to it. Moreover it limits
> brightness resolution to 256 levels.

The V4L2 control's unit is mA. Does this mean that the unit in the LED API
also becomes mA then?

> >>+
> >>+	return ret;
> >>+}
> >>+
> >>+static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> >>+{
> >>+	struct v4l2_flash *flash = ctrl_to_flash(c);
> >>+	struct led_classdev *led_cdev = flash->led_cdev;
> >>+	int ret = 0;
> >>+
> >>+	switch (c->id) {
> >>+	case V4L2_CID_FLASH_LED_MODE:
> >>+		switch (c->val) {
> >>+		case V4L2_FLASH_LED_MODE_NONE:
> >>+			/* clear flash mode on releae */
> >
> >It's not uncommon for the user to leave the mode to something else than none
> >when the user goes away. Could there be other ways to mediate access?
> 
> IMHO user space application should release the mode on exit as any other
> resource it acquires. However if it is terminated unexpectedly

The application's resources are released by the kernel when the application
exists but V4L2 control's don't behave that way.

> the sysfs will remain locked forever. Maybe a dedicated sysfs
> attribute should be provided in the LED subsystem for controlling the
> sysfs lock state? It would have to be always available for the user
> though.

How about using the information on open file handles to the flash sub-device
instead? If someone holds a file handle there, sure then the device is in
use by that program? The advantage compared to the control value is that the
file handles are released when the application quits whether or not any
cleanup is performed by the application. I don't immediately see other
reasons to keep a file handle to a V4L2 flash sub-device open, except, well,
to find about its capabilities.

> >>+static int v4l2_flash_init_controls(struct v4l2_flash *flash,
> >>+				struct v4l2_flash_ctrl_config *config)
> >>+
> >>+{
> >>+	unsigned int mask;
> >>+	struct v4l2_ctrl *ctrl;
> >>+	struct v4l2_ctrl_config *ctrl_cfg;
> >>+	bool has_flash = config->flags & V4L2_FLASH_CFG_LED_FLASH;
> >>+	bool has_torch = config->flags & V4L2_FLASH_CFG_LED_TORCH;
> >>+	int ret, num_ctrls;
> >>+
> >>+	if (!has_flash && !has_torch)
> >>+		return -EINVAL;
> >>+
> >>+	num_ctrls = has_flash ? 8 : 2;
> >>+	if (config->flags & V4L2_FLASH_CFG_FAULTS_MASK)
> >>+		++num_ctrls;
> >>+
> >>+	v4l2_ctrl_handler_init(&flash->hdl, num_ctrls);
> >>+
> >>+	mask = 1 << V4L2_FLASH_LED_MODE_NONE;
> >>+	if (has_flash)
> >>+		mask |= 1 << V4L2_FLASH_LED_MODE_FLASH;
> >>+	if (has_torch)
> >>+		mask |= 1 << V4L2_FLASH_LED_MODE_TORCH;
> >
> >I don't expect to see this on LED flash devices. :-)
> 
> I don't get your point here. Could you be more specific? :)
> Torch only mode is supported by V4L2_CID_FLASH_LED_MODE control,
> isn't it?

Have you seen a LED flash controller which does not implement the torch
mode?

I have no objections to keeping the check though, but there were more
important things missing at the time.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@....fi	XMPP: sailus@...iisi.org.uk
--
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