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, ...);
> >
> > 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.
--
Kind regards,
Sakari Ailus
