Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 RESEND] media: vimc: fla: Add virtual flash subdevice
To:     =?UTF-8?Q?Lucas_A=2e_M=2e_Magalh=c3=a3es?= <lucmaga@gmail.com>,
        linux-media@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, helen.koike@collabora.com,
        edusbarretto@gmail.com, lkcamp@lists.libreplanetbr.org
References: <20191203230148.2442-1-lucmaga@gmail.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <96e82f03-99a0-ad24-4145-242c16eacdc4@xs4all.nl>
Date:   Wed, 15 Jan 2020 18:53:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191203230148.2442-1-lucmaga@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Lucas,

On 12/4/19 12:01 AM, Lucas A. M. Magalhães wrote:
> From: Lucas A. M. Magalhaes <lucmaga@gmail.com>
> 
> 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@gmail.com>
> Signed-off-by: Eduardo Barretto <edusbarretto@gmail.com>
> Signed-off-by: Lucas A. M. Magalhães <lucmaga@gmail.com>
> 
> ---
> 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