* [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high @ 2023-09-13 19:39 Fabio Estevam 2023-09-14 10:28 ` Sakari Ailus 2023-09-14 10:46 ` Krzysztof Kozlowski 0 siblings, 2 replies; 4+ messages in thread From: Fabio Estevam @ 2023-09-13 19:39 UTC (permalink / raw) To: sakari.ailus Cc: mchehab, robh+dt, krzysztof.kozlowski+dt, conor+dt, linux-media, devicetree, 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high 2023-09-13 19:39 [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high Fabio Estevam @ 2023-09-14 10:28 ` Sakari Ailus 2023-09-14 13:20 ` Jacopo Mondi 2023-09-14 10:46 ` Krzysztof Kozlowski 1 sibling, 1 reply; 4+ messages in thread From: Sakari Ailus @ 2023-09-14 10:28 UTC (permalink / raw) To: Fabio Estevam Cc: mchehab, robh+dt, krzysztof.kozlowski+dt, conor+dt, linux-media, devicetree, Fabio Estevam, jacopo.mondi 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high 2023-09-14 10:28 ` Sakari Ailus @ 2023-09-14 13:20 ` Jacopo Mondi 0 siblings, 0 replies; 4+ messages in thread From: Jacopo Mondi @ 2023-09-14 13:20 UTC (permalink / raw) To: Sakari Ailus Cc: Fabio Estevam, mchehab, robh+dt, krzysztof.kozlowski+dt, conor+dt, linux-media, devicetree, Fabio Estevam, 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high 2023-09-13 19:39 [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high Fabio Estevam 2023-09-14 10:28 ` Sakari Ailus @ 2023-09-14 10:46 ` Krzysztof Kozlowski 1 sibling, 0 replies; 4+ messages in thread From: Krzysztof Kozlowski @ 2023-09-14 10:46 UTC (permalink / raw) To: Fabio Estevam, sakari.ailus Cc: mchehab, robh+dt, krzysztof.kozlowski+dt, conor+dt, linux-media, devicetree, Fabio Estevam 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-14 13:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-13 19:39 [PATCH] media: dt-bindings: ovti,ov772x: Make powerdown-gpios active-high Fabio Estevam 2023-09-14 10:28 ` Sakari Ailus 2023-09-14 13:20 ` Jacopo Mondi 2023-09-14 10:46 ` Krzysztof Kozlowski
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).