Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor

[Benjamin MUGNIER <benjamin.mugnier@foss.st.com>]

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@foss.st.com/
>>
>> 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.

Thank you.

-- 
Regards,

Benjamin