Hi Sakari,
On 10/10/22 13:55, Sakari Ailus wrote:
> Hi Benjamin,
>
> On Mon, Oct 10, 2022 at 11:50:25AM +0200, Benjamin MUGNIER wrote:
>> 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?
>>>
>>
>
> Please wrap the lines at around 74. Rewrapped now...
>
Done. Thank you.
>> 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
>
> This is what sensors generally do.
>
>> 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.
>
> Those are usually configured for the flash driver, not on the sensor.
>
Ok, I guess in this case there is no flash driver. Should I keep the
'st,strobe-gpios-polarity' property or are you aware of an already
defined property for this behavior?
>>
>> 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
