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
