| Message ID | 20230913193932.1947918-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high | expand |
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.
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
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
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 {