Re: [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings

[Dongchun Zhu <dongchun.zhu@mediatek.com>]

Hi Mauro, Sakari, Rob,

On Wed, 2020-04-08 at 14:49 +0200, Tomasz Figa wrote:
> On Tue, Dec 17, 2019 at 4:15 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Rob, Dongchun,
> >
> > On Wed, Dec 11, 2019 at 8:29 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > >
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/media/i2c/ov02a10.txt      | 54 ++++++++++++++++++++++
> > >  MAINTAINERS                                        |  7 +++
> > >  2 files changed, 61 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov02a10.txt b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > new file mode 100644
> > > index 0000000..18acc4f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > @@ -0,0 +1,54 @@
> > > +* Omnivision OV02A10 MIPI CSI-2 sensor
> > > +
> > > +Required Properties:
> > > +- compatible: shall be "ovti,ov02a10"
> > > +- clocks: reference to the eclk input clock
> > > +- clock-names: shall be "eclk"
> > > +- dovdd-supply: Digital I/O voltage supply, 1.8 volts
> > > +- avdd-supply: Analog voltage supply, 2.8 volts
> > > +- dvdd-supply: Digital core voltage supply, 1.8 volts
> > > +- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> > > +                  if any. This is an active low signal to the OV02A10.
> >
> > On the hardware level this pin is active high, i.e. the device is
> > powered down when the signal is high.
> >
> > > +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> > > +              This is an active high signal to the OV02A10.
> >
> > On the hardware level this pin is active low, i.e. the device is held
> > in reset when the signal is low.
> >
> > However, there is some confusion around how the polarity flag in the
> > GPIO specifier is supposed to be used.
> >
> > As per [1],
> >
> > "The gpio-specifier's polarity flag should represent the physical
> > level at the GPIO controller that achieves (or represents, for inputs)
> > a logically asserted value at the device. The exact definition of
> > logically asserted should be defined by the binding for the device."
> >
> > In this case it sounds like "logically asserted" means the device is
> > powered down or held in reset, respectively, which would suggest that
> > the specifiers should have GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW
> > respectively. The latter would cause the GPIO subsystem to invert the
> > values set by the consumers, which would then be confusing from the
> > driver implementation point of view.
> >
> > Should the pin be renamed to "nreset"? It would change the meaning of
> > "logically asserted" to "device is not held in reset" and so
> > GPIO_ACTIVE_HIGH (or 0) would be the right value to use.
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L83
> 
> + Bartosz, Linus, Sakari and the linux-gpio ML for a broader audience.
> 
> Would appreciate some feedback on what's the proper way of defining
> GPIO polarity. Thanks!
> 
> Best regards,
> Tomasz
> 

I have another question about OV02A10 CMOS sensor dt-binding.
As its text documentation was already reviewed by Rob on earlier
version:
https://patchwork.linuxtv.org/patch/59787/
I wonder whether we need to convert it to DT in YAML.
In fact, I just submitted one conversion version.
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2143922

Unluckily make dt_binding_check still report errors temporarily.
It seems there is something wrong with the port property in DT.
Could anyone help provide some tips?
$make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
  SCHEMA  Documentation/devicetree/bindings/processed-schema.yaml
Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: ignoring,
error in schema: properties: port: patternProperties: endpoint
warning: no schema found in file:
Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
make[2]: *** [Documentation/devicetree/bindings/processed-schema.yaml]
Error 255
make[1]: *** [dt_binding_check] Error 2
make: *** [sub-make] Error 2

In addition, as OV02A10 use one private property to distinguish
different projects that adopting different register settings,
I would appreciate the feedback on how to add private property to DT in
YAML.

> >
> > Best regards,
> > Tomasz
> >
> > > +
> > > +Optional Properties:
> > > +- rotation: as defined in
> > > +           Documentation/devicetree/bindings/media/video-interfaces.txt,
> > > +           valid values are 0 (sensor mounted upright) and 180 (sensor
> > > +           mounted upside down).
> > > +
> > > +The device node shall contain one 'port' child node with an
> > > +'endpoint' subnode for its digital output video port,
> > > +in accordance with the video interface bindings defined in
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +
> > > +Example:
> > > +&i2c4 {
> > > +       ov02a10: camera-sensor@3d {
> > > +               compatible = "ovti,ov02a10";
> > > +               reg = <0x3d>;
> > > +               pinctrl-names = "default";
> > > +               pinctrl-0 = <&camera_pins_cam1_mclk_on>;
> > > +
> > > +               clocks = <&topckgen CLK_TOP_MUX_CAMTG2>,
> > > +                       <&topckgen CLK_TOP_UNIVP_192M_D8>;
> > > +               clock-names = "eclk", "freq_mux";
> > > +               clock-frequency = <24000000>;
> > > +
> > > +               dovdd-supply = <&mt6358_vcamio_reg>;
> > > +               avdd-supply = <&mt6358_vcama1_reg>;
> > > +               dvdd-supply = <&mt6358_vcn18_reg>;
> > > +               powerdown-gpios = <&pio 107 GPIO_ACTIVE_LOW>;
> > > +               reset-gpios = <&pio 109 GPIO_ACTIVE_HIGH>;
> > > +               rotation = <180>;
> > > +
> > > +               port {
> > > +                       /* MIPI CSI-2 bus endpoint */
> > > +                       ov02a10_core: endpoint {
> > > +                               remote-endpoint = <&ov02a10_0>;
> > > +                               link-frequencies = /bits/ 64 <390000000>;
> > > +                       };
> > > +               };
> > > +       };
> > > +};
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index bd5847e..92a868c 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -12130,6 +12130,13 @@ T:     git git://linuxtv.org/media_tree.git
> > >  S:     Maintained
> > >  F:     drivers/media/i2c/ov13858.c
> > >
> > > +OMNIVISION OV02A10 SENSOR DRIVER
> > > +M:     Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > +L:     linux-media@vger.kernel.org
> > > +T:     git git://linuxtv.org/media_tree.git
> > > +S:     Maintained
> > > +F:     Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> > > +
> > >  OMNIVISION OV2680 SENSOR DRIVER
> > >  M:     Rui Miguel Silva <rmfrfs@gmail.com>
> > >  L:     linux-media@vger.kernel.org
> > > --
> > > 2.9.2