[PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high

[PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

The powerdown-gpios description mentions:

"Reference to the GPIO connected to the PWDN pin which is active high."

Improve the example by making powerdown-gpios active-high for consistency.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
index 5d24edba8f99..5aec65b053af 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
@@ -114,7 +114,7 @@ examples:
             compatible = "ovti,ov7725";
             reg = <0x21>;
             reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
-            powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
+            powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_HIGH>;
             clocks = <&xclk>;
 
             port {
-- 
2.34.1

Re: [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high

[Sakari Ailus]

Hi Fabio,

On Wed, Sep 13, 2023 at 04:39:32PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> The powerdown-gpios description mentions:
> 
> "Reference to the GPIO connected to the PWDN pin which is active high."
> 
> Improve the example by making powerdown-gpios active-high for consistency.
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
>  Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
> index 5d24edba8f99..5aec65b053af 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
> @@ -114,7 +114,7 @@ examples:
>              compatible = "ovti,ov7725";
>              reg = <0x21>;
>              reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
> -            powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
> +            powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_HIGH>;
>              clocks = <&xclk>;
>  
>              port {

Looking at the driver code, it seems the powerdown GPIO is set to state 1
when the device is powered on and to 0 when it's powered down. This looks
like a driver bug.

But what happens if you fix something like this after five years in
existence? Maybe just leave it as-is, and document it??? Then again,
there's a single Renesas board that appears to have such a device, added
two and half years ago.

Also cc Jacopo.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high

[Krzysztof Kozlowski]

On 13/09/2023 21:39, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> The powerdown-gpios description mentions:
> 
> "Reference to the GPIO connected to the PWDN pin which is active high."

The binding description or device datasheet? If the first, what
guarantees you have that person writing binding understood the
difference between signal level and OS abstraction logical high/low?


>              port {

Best regards,
Krzysztof

Re: [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high

[Jacopo Mondi]

Hi Sakari, Fabio

On Thu, Sep 14, 2023 at 10:28:58AM +0000, Sakari Ailus wrote:
> Hi Fabio,
>
> On Wed, Sep 13, 2023 at 04:39:32PM -0300, Fabio Estevam wrote:
> > From: Fabio Estevam <festevam@denx.de>
> >
> > The powerdown-gpios description mentions:
> >
> > "Reference to the GPIO connected to the PWDN pin which is active high."

From datasheet:

        Power down mode selection:
        0: Normal mode
        1: Power down mode

> >
> > Improve the example by making powerdown-gpios active-high for consistency.
> >
> > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
> > index 5d24edba8f99..5aec65b053af 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml
> > @@ -114,7 +114,7 @@ examples:
> >              compatible = "ovti,ov7725";
> >              reg = <0x21>;
> >              reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
> > -            powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
> > +            powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_HIGH>;
> >              clocks = <&xclk>;
> >
> >              port {
>
> Looking at the driver code, it seems the powerdown GPIO is set to state 1
> when the device is powered on and to 0 when it's powered down. This looks
> like a driver bug.
>


It is. As you can see I ported the driver from the old soc-camera
version and in 762c28121d7c ("media: i2c: ov772x: Remove soc_camera
dependencies") I defintely introduced this. I'll here play the card "I
was young in 2018".

This is also probably wrong

	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "powerdown",
					     GPIOD_OUT_LOW);

As it sets the chip in power-up mode during probe() (this should be
safe to change, but there's no way I can test it unfortunately)

> But what happens if you fix something like this after five years in
> existence? Maybe just leave it as-is, and document it??? Then again,

As the rule "old dtbs are supposed to work with new versions of a
driver", "fixing" the driver would defintely break them.

I would add a comment in the .yaml file and in the driver.
As I introduced this, I can do that if Fabio doesn't.

> there's a single Renesas board that appears to have such a device, added
> two and half years ago.

yeah, that stuff is dead, but we can't tell how many users of this
driver are there in the wild..

>
> Also cc Jacopo.

Thanks, I'm listead as maintainer for this driver for odd-fixes.
Please use get_maintainer.pl

>
> --
> Regards,
>
> Sakari Ailus