Re: [PATCH v10 1/3] media: dt-bindings: ov8856: Document YAML bindings

[Robert Foss <robert.foss@linaro.org>]

Hey Dongchun,

Thanks for having a look at this series.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov8856: camera@10 {
> > +            compatible = "ovti,ov8856";
> > +            reg = <0x10>;
> > +
> > +            reset-gpios = <&pio 111 GPIO_ACTIVE_LOW>;
>
> Apologies for missing to follow the earlier discussion related to this.
> I noticed the GPIO flag para and __ov8856_power_on() are aligned using
> ACTIVE_LOW.
>
> But from the datasheet, XSHUTDN pin is active-high for OV8856.
> It means devm_gpiod_get API (in probe func) should use GPIOD_OUT_LOW to
> initialize the GPIO as output with a value of 0.
> Otherwise it should use GPIOD_OUT_HIGH.
>
> There is one case for GPIO_ACTIVE_LOW setting:
> https://patchwork.linuxtv.org/patch/63460/
> https://patchwork.linuxtv.org/patch/63461/

We went back and forth about this a few times, and I switched to this
gpio setting after having worked through the device probing reset gpio
toggling. Semantically it seemed easier to understand in the driver,
since the gpio is called reset and not !shutdown.

Looking into devm_gpiod_get_optional(), the flag argument
GPIOD_OUT_LOW or HIGH for that matter is actually not used initialize
the output, but only used for an exclusivity check.
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib-devres.c#L109

If you prefer, I can invert the logic again. To me making the reset
gpio active resulting in the device being actually reset seems like
the most intuitive and easy to understand option.
The different OmniVision drivers seem to have different approaches to
this. The ov9640 driver for example is doing what this series
currently is doing.

>
> Sakari, Tomasz, am I right?
>
> > +            pinctrl-names = "default";
> > +            pinctrl-0 = <&clk_24m_cam>;
> > +
> > +            clocks = <&cam_osc>;
> > +            clock-names = "xvclk";
> > +            clock-frequency = <19200000>;
> > +
> > +            avdd-supply = <&mt6358_vcama2_reg>;
> > +            dvdd-supply = <&mt6358_vcamd_reg>;
> > +            dovdd-supply = <&mt6358_vcamio_reg>;
> > +
> > +            port {
> > +                wcam_out: endpoint {
> > +                    remote-endpoint = <&mipi_in_wcam>;
> > +                    data-lanes = <1 2 3 4>;
> > +                    link-frequencies = /bits/ 64 <360000000>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +...
> > \ No newline at end of file
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 26f281d9f32a..84b262afd13d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12489,6 +12489,7 @@ L:    linux-media@vger.kernel.org
> >  S:   Maintained
> >  T:   git git://linuxtv.org/media_tree.git
> >  F:   drivers/media/i2c/ov8856.c
> > +F:   Documentation/devicetree/bindings/media/i2c/ov8856.yaml
> >
>
> Had you run parse-maintainers.pl?
> The new item is supposed to be arranged in alphabetical order.

No, I have not. But upon running it now, it doesn't make suggest any
changes. But let me order the files manually in the next revision.

However, I noticed I removed the wrong person from the maintainers
file in this revision.
So, I'll correct that and add you Dongchun as the maintainer if that's ok.

>
> >  OMNIVISION OV9640 SENSOR DRIVER
> >  M:   Petr Cvek <petrcvekcz@gmail.com>
>