Hi Sakari,
On 10/10/22 11:33, Sakari Ailus wrote:
> Hi Benjamin,
>
> On Mon, Oct 10, 2022 at 11:19:01AM +0200, Benjamin MUGNIER wrote:
>> Hi Sakari,
>>
>> On 10/10/22 10:29, Sakari Ailus wrote:
>>> Hi Benjamin,
>>>
>>> On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote:
>>>> Hi Sakari,
>>>>
>>>> Thank you for your review.
>>>> Please consider everything not commented as queued for v7.
>>>>
>>>> On 10/6/22 21:14, Sakari Ailus wrote:
>>>>> Hi Benjamin,
>>>>>
>>>>> Thanks for the update. A few more comments below...
>>>>>
>>>>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote:
>>>>>> +static const char * const vgxy61_hdr_mode_menu[] = {
>>>>>> + "HDR linearize",
>>>>>> + "HDR substraction",
>>>>>> + "No HDR",
>>>>>> +};
>>>>>
>>>>> I think more documentation is needed on the HDR modes. What do these mean?
>>>>> For instance, are they something that requires further processing or is the
>>>>> result e.g. a ready HDR image?
>>>>>
>>>>> This should probably make it to driver specific documentation.
>>>>>
>>>>
>>>> Sure, in fact I did something like this in v3:
>>>> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@xxxxxxxxxxx/
>>>>
>>>> Since I standardized the control I was unsure what to do with this documentation, and dropped it.
>>>> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one.
>>>
>>> Yes, please. That seems reasonably good.
>>>
>>> I think Laurent's proposal on a HDR control for sensor-implemented HDR is a
>>> good one. Further controls can be added later on.
>>>
>>>>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node,
>>>>>> + "invert-gpios-polarity");
>>>
>>> How about:
>>>
>>> device_property_read_bool(dev, ...);
>>>
>>
>> Sounds great.
>>
>>>>>
>>>>> I thought we did discuss dropping support for sensor synchronisation in
>>>>> this version?
>>>>>
>>>>
>>>> This properties affects strobing lights gpios polarities as you can see
>>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios
>>>> are inverted. This has nothing to do with the sensor synchronization.
>>>
>>> So this is for strobing a LED flash? It would be good to mention this in
>>> DT bindings.
>>>
>>>>
>>>> Now I realize this is poorly named, and I even forgot to document it in
>>>> the device tree bindings file. I apologize.
>>>
>>> No problem.
>>>
>>>>
>>>> I would like to rename it to 'st,strobe-polarity' since this is vendor
>>>> specific and to better reflect that it affects strobing gpios. I'll make
>>>> this change for v7 and document this in the bindings file too. Tell me if
>>>> there is any issues with that.
>>>
>>> That name seems reasonable to me. Although, *if* this is actually usable as
>>> a GPIO as the bindings suggest, then the GPIO flags would probably be a
>>> better alternative.
>>>
>>
>> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :)
>> I'll add a bit of comments on the bindings. It seems to be the best choice.
>
> Is this a GPIO or is it not (e.g. strobe signal only)?
>
> For the latter the this should be fine. And "flash-leds" property should be
> there as well I guess?
>
This property controls the polarity of and output GPIO connected to the sensor. This output GPIO is driven by the sensor firmware in order to illuminate the scene whenever necessary.
I'm not sure this goes under the "flash-leds" category, as it only provides a signal with either "0" (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal following the programmed "strobe-mode" as you can see in vgxy61_strobe_mode according to the HDR mode. It does not have a max-microamp or timeout values as a flash I suppose, it is really a simple signal.
Practically we have what we call "illuminators" that are either built in the sensor'smodule, are externally connected to the sensor's module.
Hope this is clearer.
--
Regards,
Benjamin
