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@xxxxxxxxxxx/
> >>
> >> 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?
--
Sakari Ailus
