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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux