linux-kernel - Re: [PATCH v3 1/2] mfd: arizona: Update reset pin to use GPIOD

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: <20180307145559.y4tyyzd2pbqrk4yb@localhost.localdomain>
Date:   Wed, 7 Mar 2018 14:55:59 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Fabio Estevam <festevam@...il.com>
CC:     Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        "Mark Rutland" <mark.rutland@....com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        <patches@...nsource.cirrus.com>
Subject: Re: [PATCH v3 1/2] mfd: arizona: Update reset pin to use GPIOD

On Wed, Mar 07, 2018 at 11:24:10AM -0300, Fabio Estevam wrote:
> >  static inline void arizona_enable_reset(struct arizona *arizona)
> >  {
> >         if (arizona->pdata.reset)
> > -               gpio_set_value_cansleep(arizona->pdata.reset, 0);
> > +               gpiod_set_value_cansleep(arizona->pdata.reset, 0);
> 
> This should be:
> 
> gpiod_set_value_cansleep(arizona->pdata.reset, 1);
> 
> as here you want to put the reset GPIO into its active state.
> 
> Assuming that in the dts this GPIO is defined as GPIO_ACTIVE_LOW, then
> the command should put it to logic level 0 as done originally.
> 
> >  static void arizona_disable_reset(struct arizona *arizona)
> > @@ -295,7 +295,7 @@ static void arizona_disable_reset(struct arizona *arizona)
> >                         break;
> >                 }
> >
> > -               gpio_set_value_cansleep(arizona->pdata.reset, 1);
> > +               gpiod_set_value_cansleep(arizona->pdata.reset, 1);
> 
> This should be:
> 
> gpiod_set_value_cansleep(arizona->pdata.reset, 0);
> 
> as here you want to put the reset GPIO into its inactive state.

Hmm... this raises a rather good point that I hadn't considered.
This driver is used in a lot of shipping devices and the old
style GPIO calls didn't take the activeness of the GPIO into
account just blindly setting the value you asked for. However the
new style calls do take this into account.

The trouble is I guess we don't know whether most users bothered
to set GPIO_ACTIVE_LOW or not. So it is very hard to say here if
we are about to breaking a lot of existing device trees here.

I guess probably the safest approach is to use
gpiod_set_raw_value_cansleep here?

Thanks,
Charles