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

[Rob Herring <robh@kernel.org>]

On Thu, Apr 09, 2020 at 09:03:28PM +0800, Dongchun Zhu wrote:
> 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.

Yes.

> 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

    patternProperties:
      endpoint:
      type: object
      additionalProperties: false

You need more indentation under 'endpoint'. Also, 'endpoint' is a fixed 
string, so it should be under 'properties' rather than 'patternProperties'.


> 
> 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.

Like any other property. Submit something for review.

Rob