linux-kernel - Re: [PATCH v9 1/3] gpio: exar: add gpio for exar cards

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] [day] [month] [year] [list]

Message-ID: <20170119132201.GA29998@kroah.com>
Date:   Thu, 19 Jan 2017 14:22:01 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        Jiri Slaby <jslaby@...e.com>,
        One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v9 1/3] gpio: exar: add gpio for exar cards

On Sun, Jan 15, 2017 at 04:57:32PM +0200, Andy Shevchenko wrote:
> On Sun, Jan 15, 2017 at 12:50 AM, Sudip Mukherjee
> <sudipm.mukherjee@...il.com> wrote:
> > From: Sudip Mukherjee <sudip.mukherjee@...ethink.co.uk>
> >
> > Exar XR17V352/354/358 chips have 16 multi-purpose inputs/outputs which
> > can be controlled using gpio interface.
> >
> > Add the gpio specific code.
> 
> Thanks for an updated code.
> Briefly looking into first two patches v10 would be needed.
> 
> Here one important line is missed, nevertheless I'll go to comment
> some minor stuff as well.
> Overall looks good!
> 
> > name[16] should be enough. Maximum of 12 char will be used.
> 
> Better to use enough space, otherwise there is a room for a bug.
> So, you need to understand that kernel might crash in such cases,
> though it's minor for now.
> 
> > +++ b/drivers/gpio/gpio-exar.c
> > @@ -0,0 +1,199 @@
> 
> > +static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
> > +                       unsigned int offset)
> > +{
> > +       struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> > +       int temp;
> > +
> > +       mutex_lock(&exar_gpio->lock);
> > +       temp = readb(exar_gpio->regs + reg);
> > +       temp &= ~BIT(offset);
> 
> > +       if (val)
> > +               temp |= BIT(offset);
> 
> You would do this in one line, but I dunno which style Linus prefers more
> 
> temp |= val ? BIT(...) : 0;

Use the if () format, it's easier to read and understand, which is more
important overall.

thanks,

greg k-h