linux-kernel - Re: [PATCH v2 9/9] media: i2c: tw9910: Remove soc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180103171347.GF9493@w540>
Date:   Wed, 3 Jan 2018 18:13:47 +0100
From:   jacopo mondi <jacopo@...ndi.org>
To:     Fabio Estevam <festevam@...il.com>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Magnus Damm <magnus.damm@...il.com>, geert@...der.be,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-renesas-soc@...r.kernel.org,
        linux-media <linux-media@...r.kernel.org>,
        linux-sh@...r.kernel.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 9/9] media: i2c: tw9910: Remove soc_camera dependencies

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@...ndi.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