Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice

[Hans Verkuil]

Hi Lucas,

On 12/4/19 12:01 AM, Lucas A. M. MagalhÃes wrote:
> From: Lucas A. M. Magalhaes <lucmaga@xxxxxxxxx>
> 
> Add a virtual subdevice to simulate the flash control API.
> Those are the supported controls:
> v4l2-ctl -d /dev/v4l-subdev6 -L
> Flash Controls
> 
>                        led_mode 0x009c0901 (menu)   : min=0 max=2 default=1 value=1
>                                 0: Off
>                                 1: Flash
>                                 2: Torch
>                   strobe_source 0x009c0902 (menu)   : min=0 max=1 default=0 value=0
>                                 0: Software
>                                 1: External
>                          strobe 0x009c0903 (button) : flags=write-only, execute-on-write
>                     stop_strobe 0x009c0904 (button) : flags=write-only, execute-on-write
>                   strobe_status 0x009c0905 (bool)   : default=0 value=0 flags=read-only
>                  strobe_timeout 0x009c0906 (int)    : min=50 max=400 step=50 default=50 value=400
>            intensity_flash_mode 0x009c0907 (int)    : min=23040 max=1499600 step=11718 default=23040 value=23040
>            intensity_torch_mode 0x009c0908 (int)    : min=2530 max=187100 step=1460 default=2530 value=2530
>             intensity_indicator 0x009c0909 (int)    : min=0 max=255 step=1 default=0 value=0
>                          faults 0x009c090a (bitmask): max=0x00000002 default=0x00000000 value=0x00000000
> 
> Co-authored-by: Eduardo Barretto <edusbarretto@xxxxxxxxx>
> Signed-off-by: Eduardo Barretto <edusbarretto@xxxxxxxxx>
> Signed-off-by: Lucas A. M. MagalhÃes <lucmaga@xxxxxxxxx>
> 
> ---
> Hi,
> 
> I've copied some values from another driver (lm3646) to make it more
> realistic, as suggested by Hans. All values except for
> V4L2_CID_FLASH_INDICATOR_INTENSITY, which I couldn't find any
> implementation.
> 
> The v4l-compliance is failing. From the documentation
> V4L2_CID_FLASH_STROBE should just work if the
> V4L2_CID_FLASH_STROBE_SOURCE is "Software" and the
> V4L2_CID_FLASH_LED_MODE is "Flash", otherwise it should fail. With the
> standard values configured for the V4L2_CID_FLASH_STROBE will not fail.
> But during the tests v4l-compliance sets V4L2_CID_FLASH_LED_MODE to
> "Torch" and V4L2_CID_FLASH_STROBE_SOURCE to "External" which makes
> V4L2_CID_FLASH_STROBE to fail. How do I proceed? Should the
> v4l-compliance be changed?

I completely missed this question, my apologies.

The best way is to set the flash_strobe control V4L2_CTRL_FLAG_GRABBED whenever
this control is invalid. I'd also return EBUSY instead of EINVAL in
'case V4L2_CID_FLASH_STROBE:' for consistency with the GRABBED behavior.

v4l2-compliance will probably still fail since it doesn't check this flag
when testing S_CTRL/S_EXT_CTRLS, that should be added: in testSimpleControls()
it should check that the return value of S_CTRL is EBUSY if GRABBED was set.
In testExtendedControls() it should do the same when it tests VIDIOC_S_EXT_CTRLS
for a single control. And in the later test where it sets all controls in a single
VIDIOC_S_EXT_CTRL ioctl it should skip any controls marked GRABBED.

I think that's a reasonable approach.

Regards,

	Hans