Re: [PATCH v8 07/12] gpio: palmas: add in GPIO support for palmascharger

[Ian Lartey]

On 13/03/13 14:15, Linus Walleij wrote:
> Sorry for slow replies :-(
>
> On Thu, Mar 7, 2013 at 2:17 PM, Ian Lartey <ian@xxxxxxxxxxxxxxx> wrote:
>
> > +static int palmas_gpio_read(struct palmas *palmas, unsigned int reg,
> > +               int gpio, unsigned int *dest)
>
> I don't like "int gpio" here, please use "int offset".
>
> This is not a global GPIO number, it is an offset in the local gpio_chip,
> right?

Correct.
> > +{
> > +       /* registers for second bank are identical and offset by 0x9 */
> > +       if (gpio > 7)
> > +               reg += PALMAS_GPIO_DATA_IN2;
> > +
> > +       return palmas_read(palmas, PALMAS_GPIO_BASE, reg, dest);
> > +}
> > +
> > +static int palmas_gpio_write(struct palmas *palmas, unsigned int reg,
> > +               int gpio, unsigned int data)
> > +{
> > +       /* registers for second bank are identical and offset by 0x9 */
> > +       if (gpio > 7)
> > +               reg += PALMAS_GPIO_DATA_IN2;
> > +
> > +       return palmas_write(palmas, PALMAS_GPIO_BASE, reg, data);
> > +}
> > +
> > +static int palmas_gpio_update_bits(struct palmas *palmas, unsigned int reg,
> > +               int gpio, unsigned int mask, unsigned int data)
> > +{
> > +       /* registers for second bank are identical and offset by 0x9 */
> > +       if (gpio > 7)
> > +               reg += PALMAS_GPIO_DATA_IN2;
> > +
> > +       return palmas_update_bits(palmas, PALMAS_GPIO_BASE, reg, mask, data);
> > +}
>
>
> > -       ret = palmas_read(palmas, PALMAS_GPIO_BASE, PALMAS_GPIO_DATA_IN, &val);
> > +       ret = palmas_gpio_read(palmas, PALMAS_GPIO_DATA_IN, offset, &val);
>
> (Obviously you are passing the offset.)
>
> > +               ret = palmas_gpio_write(palmas, PALMAS_GPIO_SET_DATA_OUT,
> > +                               offset, BIT(offset % 8));
> (..)
> > +               ret = palmas_gpio_write(palmas, PALMAS_GPIO_CLEAR_DATA_OUT,
> > +                               offset, BIT(offset % 8));
> (...)
> > +       ret = palmas_gpio_update_bits(palmas, PALMAS_GPIO_DATA_DIR,
> > +                       offset, 1 << (offset % 8), 1 << (offset % 8));
>
> Why aren't you using the BIT() macro here too?
>
> Further: if these functions are always used like this, with offset
> and some BIT() or (1 << (offset % 8)) why don't you move that
> latter part into the static helper and just pass offset into the
> helper?

Good points  - I missed the other bit shifts.


> Yours,
> Linus Walleij


Thanks

Ian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/