linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
       [not found]   ` <CAAFQd5AnWZqjQEVvw8gv7JzOBHxJvsOWaGrbY8CXQ_87ap-ahA@mail.gmail.com>
@ 2020-04-08 12:49     ` Tomasz Figa
  2020-04-09 13:03       ` Dongchun Zhu
  0 siblings, 1 reply; 4+ messages in thread
From: Tomasz Figa @ 2020-04-08 12:49 UTC (permalink / raw)
  To: Rob Herring, Sakari Ailus, Linus Walleij, Bartosz Golaszewski
  Cc: Dongchun Zhu, Mauro Carvalho Chehab, Andy Shevchenko,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男), linux-gpio

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
  2020-04-08 12:49     ` [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings Tomasz Figa
@ 2020-04-09 13:03       ` Dongchun Zhu
  2020-04-15 16:14         ` Rob Herring
  0 siblings, 1 reply; 4+ messages in thread
From: Dongchun Zhu @ 2020-04-09 13:03 UTC (permalink / raw)
  To: Tomasz Figa, Mauro Carvalho Chehab, Bartosz Golaszewski,
	Rob Herring, Sakari Ailus, broonie
  Cc: Linus Walleij, Andy Shevchenko, Mark Rutland, Nicolas Boichat,
	Matthias Brugger, Cao Bing Bu, srv_heupstream,
	moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男), linux-gpio

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
  2020-04-09 13:03       ` Dongchun Zhu
@ 2020-04-15 16:14         ` Rob Herring
  2020-04-20  7:21           ` Dongchun Zhu
  0 siblings, 1 reply; 4+ messages in thread
From: Rob Herring @ 2020-04-15 16:14 UTC (permalink / raw)
  To: Dongchun Zhu
  Cc: Tomasz Figa, Mauro Carvalho Chehab, Bartosz Golaszewski,
	Sakari Ailus, broonie, Linus Walleij, Andy Shevchenko,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男), linux-gpio

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
  2020-04-15 16:14         ` Rob Herring
@ 2020-04-20  7:21           ` Dongchun Zhu
  0 siblings, 0 replies; 4+ messages in thread
From: Dongchun Zhu @ 2020-04-20  7:21 UTC (permalink / raw)
  To: Rob Herring, Mauro Carvalho Chehab, Bartosz Golaszewski,
	Sakari Ailus
  Cc: Tomasz Figa, Mauro Carvalho Chehab, Bartosz Golaszewski,
	Sakari Ailus, broonie, Linus Walleij, Andy Shevchenko,
	Mark Rutland, Nicolas Boichat, Matthias Brugger, Cao Bing Bu,
	srv_heupstream, moderated list:ARM/Mediatek SoC support,
	list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg  Roedel <joro@8bytes.org>,,
	Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
	Shengnan Wang (王圣男), linux-gpio

Hello Rob,

Thanks for the review.

On Wed, 2020-04-15 at 11:14 -0500, Rob Herring wrote:
> 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.
> 

Okay.
Let's focus on the new DT binding in YAML for OV02A10.

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

Thanks for the reminder.
Now we could run dt_binding_check pass for the required
property:endpoint.

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

Could you have the permission to see the change:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2143922/3

This change is improved relative to earlier patchset.
But there are still some errors if we running dt_binding_check on the
change.
$make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
  CHKDT   Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
/proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: Additional properties are not allowed ('optional' was unexpected)
/proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: Additional properties are not allowed ('optional' was unexpected)
/proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: properties:ovti,mipi-tx-speed: {'description': 'Indication of MIPI transmission speed select.'} is not valid under any of the given schemas (Possible causes of the failure):
        /proj/mtk15013/new_dt_check/kernel_only_dev/kernel/mediatek/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml: properties:ovti,mipi-tx-speed: 'not' is a required property

make[2]: ***
[Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.example.dts]
Error 1
make[1]: *** [dt_binding_check] Error 2
make: *** [sub-make] Error 2

It seems that we shall not use "optional" to describe private properties
for DT bindings in YAML.
Rob, could you help provide one example to tell us how to define new
properties?
Thanks a lot.

> Rob


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-04-20  7:22 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20191211112849.16705-1-dongchun.zhu@mediatek.com>
     [not found] ` <20191211112849.16705-2-dongchun.zhu@mediatek.com>
     [not found]   ` <CAAFQd5AnWZqjQEVvw8gv7JzOBHxJvsOWaGrbY8CXQ_87ap-ahA@mail.gmail.com>
2020-04-08 12:49     ` [V6, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings Tomasz Figa
2020-04-09 13:03       ` Dongchun Zhu
2020-04-15 16:14         ` Rob Herring
2020-04-20  7:21           ` Dongchun Zhu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).