Hi Sakari, On 10/13/22 10:00, Sakari Ailus wrote: > 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? You're right. As I already published v7 I queued this for v8 in order to reduce the noise on the mailing list, if you don't mind. Thanks a ton for your explanations. > > Virtually all other users have a flash driver chip. But the flash LED isn't > part of the module in those cases either. > -- Regards, Benjamin
