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@xxxxxxxxx> wrote:
> > From: Sudip Mukherjee <sudip.mukherjee@xxxxxxxxxxxxxxx>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
