Re: [PATCH v5 06/10] media: Add registration helpers for V4L2 flash sub-devices

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Jacek,

Jacek Anaszewski wrote:
...
>>> +#define call_flash_op(v4l2_flash, op, arg)			\
>>> +		(has_flash_op(v4l2_flash,
>>> op) ?			\
>>> +			v4l2_flash->ops->op(v4l2_flash,
>>> arg) :	\
>>> +			-EINVAL)
>>> +
>>> +static enum led_brightness __intensity_to_led_brightness(
>>> +					struct v4l2_ctrl *ctrl,
>>> +					s32 intensity)
>>
>> Fits on previous line.
>>
>>> +{
>>> +	s64 intensity64 = intensity - ctrl->minimum;
>>
>> intensity, ctrl->step and ctrl->minimum are 32-bit signed integers.
>> Do you need a 64-bit integer here?
> 
> step is u64.

Nevertheless integer controls will not have values outside the s32
range, using a step value that's outside the range makes no sense
either. I think you should use s32 instead.

> 
>>
>>> +
>>> +	do_div(intensity64, 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 (ctrl->id != V4L2_CID_FLASH_INDICATOR_INTENSITY)
>>> +		++intensity64;
>>
>> I think the condition could simply be ctrl->minimum instead, that way
>> I find it easier to understand what's happening here. I'd expect the
>> minimum for non-intensity controls always to be non-zero, though, so
>> the end result is the same. Up to you.
> 
> Minimum for indicator control must be 0 to make possible
> turning the indicator LED off only with this control.

Would torch be still on if the minimum torch current was 0 mA? I'd say no.

Although in that case I'd expect the driver to use a different range,
and selecting the off mode would then turn it off, I still think that's
a better condition than relying on the control id.

...

>>> +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;
>>> +	bool is_strobing;
>>> +	int ret;
>>> +
>>> +	switch (c->id) {
>>> +	case V4L2_CID_FLASH_TORCH_INTENSITY:
>>> +	case V4L2_CID_FLASH_INDICATOR_INTENSITY:
>>> +		return
>>> v4l2_flash_update_led_brightness(v4l2_flash, c);
>>> +	case V4L2_CID_FLASH_INTENSITY:
>>> +		ret = led_update_flash_brightness(fled_cdev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		/* no conversion is needed */
>>
>> Maybe a stupid question, but why is it not needed?
> 
> Because LED Flash class also uses microamperes.

Right, I had missed that. It'd be nice if that was said in the comment,
it might not be obvious to others either.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi