Re: [PATCH v2 9/9] media: i2c: tw9910: Remove soc_camera dependencies

[jacopo mondi <jacopo@jmondi.org>]

Hi Fabio,

On Wed, Jan 03, 2018 at 02:41:20PM -0200, Fabio Estevam wrote:
> Hi Jacopo,
>
> On Thu, Dec 28, 2017 at 12:01 PM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
>
> > +       if (priv->rstb_gpio) {
> > +               gpiod_set_value(priv->rstb_gpio, 0);
> > +               usleep_range(500, 1000);
> > +               gpiod_set_value(priv->rstb_gpio, 1);
> > +               usleep_range(500, 1000);
>
> This seems to be inverted.
>
> Consider you have an active low GPIO reset.
>
> In order to reset it:
>
> Put the GPIO to logic level 0
> Wait some time
> Put the GPIO to logic level 1
>
> gpiod_set_value(priv->rstb_gpio, 1), means the GPIO in the active
> state (0 in this example).
>
> , so this should be:
>
> gpiod_set_value(priv->rstb_gpio, 1);
> usleep_range(500, 1000);
> gpiod_set_value(priv->rstb_gpio, 0);

That would be true if I would have declared the GPIO to be ACTIVE_LOW.
See patch [5/9] in this series and search for "rstb". The GPIO (which
is shared between two devices) is said to be active high...

Are you maybe suggesting I should declare it ACTIVE_LOW in board setup
code and invert the set_value() order in the driver as you proposed?
Don't you think it would be more natural for the driver to follow the
current sequence, as the reset GPIO is effectively active low (so
setting it to 0 put the video decoder in reset state and setting it to
1 wakes the video decoder up)?

I can maybe agree with you for the PDN GPIO instead. That's said to be
active high, so setting it to 1 disables the video decoder, and yes,
it feels unnatural in the driver's power up routines to set PDN GPIO
to 0 and set it to 1 when putting the device to sleep...

Thanks
   j