Laurent Pinchart wrote:
> Hi Sakari,
Hi Laurent,
> On Tuesday 29 March 2011 11:35:19 Sakari Ailus wrote:
>> Hans Verkuil wrote:
>>> On Monday, March 28, 2011 14:55:40 Sakari Ailus wrote:
>
> [snip]
>
>>>> V4L2_CID_FLASH_TIMEOUT (integer; LED)
>>>>
>>>> The flash controller provides timeout functionality to shut down the led
>>>> in case the host fails to do that. For hardware strobe, this is the
>>>> maximum amount of time the flash should stay on, and the purpose of the
>>>> setting is to prevent the LED from catching fire.
>>>>
>>>> For software strobe, the setting may be used to limit the length of the
>>>> strobe in case a driver does not implement it itself. The granularity of
>>>> the timeout in [1, 2, 3] is very coarse. However, the length of a
>>>> driver-implemented LED strobe shutoff is very dependent on host.
>>>> Possibly V4L2_CID_FLASH_DURATION should be added, and
>>>> V4L2_CID_FLASH_TIMEOUT would be read-only so that the user would be able
>>>> to obtain the actual hardware implemented safety timeout.
>>>>
>>>> Likely a standard unit such as ms or µs should be used.
>>>
>>> It seems to me that this control should always be read-only. A setting
>>> like this is very much hardware specific and you don't want an attacker
>>> changing the timeout to the max value that might cause a LED catching
>>> fire.
>>
>> I'm not sure about that.
>>
>> The driver already must take care of protecting the hardware in my
>> opinion. Besides, at least one control is required to select the
>> duration for the flash if there's no hardware synchronisation.
>>
>> What about this:
>>
>> V4L2_CID_FLASH_TIMEOUT
>>
>> Hardware timeout, read-only. Programmed to the maximum value allowed by
>> the hardware for the external strobe, greater or equal to
>> V4L2_CID_FLASH_DURATION for software strobe.
>>
>> V4L2_CID_FLASH_DURATION
>>
>> Software implemented timeout when V4L2_CID_FLASH_STROBE_MODE ==
>> V4L2_FLASH_STROBE_MODE_SOFTWARE.
>
> Why would we need two controls here ? My understanding is that the maximum
> strobe duration length can be limited by
>
> - the flash controller itself
> - platform-specific constraints to avoid over-heating the flash
>
> The platform-specific constraints come from board code, and the flash driver
> needs to ensure that the flash is never strobed for a duration longer than the
> limit. This requires implementing a software timer if the hardware has no
> timeout control, and programming the hardware with the correct timeout value
> otherwise. The limit can be queried with QUERYCTRL on the duration control.
That's true.
The alternative would be software timeout since the hardware timeout is
rather coarse. Its intention is to protect the hardware from catching
fire mostly.
But as I commented in the other e-mail, there likely isn't a need to be
able to control this very precisely. The user just shuts down the flash
whenever (s)he no longer needs it rather than knows beforehand how long
it needs to stay on.
>> I have to say I'm not entirely sure the duration control is required.
>> The timeout could be writable for software strobe in the case drivers do
>> not implement software timeout. The granularity isn't _that_ much
>> anyway. Also, a timeout fault should be produced whenever the duration
>> would expire.
>>
>> Perhaps it would be best to just leave that out for now.
>>
>>>> V4L2_CID_FLASH_LED_MODE (menu; LED)
>>>>
>>>> enum v4l2_flash_led_mode {
>>>>
>>>> V4L2_FLASH_LED_MODE_FLASH = 1,
>>>> V4L2_FLASH_LED_MODE_TORCH,
>
> "torch" mode can also be used for video, should we rename TORCH to something
> more generic ? Maybe a "manual" mode ?
The controllers recognise a torch mode and I think it describes the
functionality quite well. Some appear to make a difference between torch
and video light --- but I can't imagine a purpose in which this could be
useful.
...
Regards,
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
