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

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

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

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

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

Sakari Ailus