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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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, ...);

> > 
> > 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.

-- 
Kind regards,

Sakari Ailus