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 02:11:46PM +0200, Benjamin MUGNIER wrote:

...

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

So the LED is directly connected to this pin (perhaps in series with a
resistor)? That is an unusual solution.

-- 
Sakari Ailus