Hi Benjamin, On Mon, Oct 10, 2022 at 03:12:30PM +0200, Benjamin MUGNIER wrote: > Hi Sakari, > > On 10/10/22 14:52, Sakari Ailus wrote: > > 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. > > > > Yes, the pin is connected to a transistor responsible of powering on and > off the LEDs according to the pin value, so that they have correct > voltage from another power supply. But yes that's basically it. > > Thanks a lot. I think it's fine as-is. Maybe the property could be called "st,strobe-polarity" as this isn't a GPIO from software point of view? Virtually all other users have a flash driver chip. But the flash LED isn't part of the module in those cases either. -- Kind regards, Sakari Ailus
