Re: [PATCHv3 1/3] gpio: sch: Consolidate similar algorithms

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@xxxxxxxxx> wrote:
> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@xxxxxxxxx>
> ---
>  drivers/gpio/gpio-sch.c |   95 ++++++++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 99720c8..6e89be9 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -63,94 +63,105 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio)
>         return gpio % 8;
>  }
>
> -static void sch_gpio_enable(struct sch_gpio *sch, unsigned gpio)
> +static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio,
> +                                 unsigned reg)
>  {
>         unsigned short offset, bit;
>         u8 enable;
>
>         spin_lock(&sch->lock);

...

>
> -       offset = sch_gpio_offset(sch, gpio, GEN);
> +       offset = sch_gpio_offset(sch, gpio, reg);
>         bit = sch_gpio_bit(sch, gpio);
>
>         enable = inb(sch->iobase + offset);

I see identical blocks of code in sch_gpio_register_clear(),
sch_gpio_reg_get() and sch_gpio_reg_set() (and possibly other
functions?). Maybe this could be factorized into an inline function
that would return the "enable" variable?

Also, "enable" looks like the wrong name here. The exact same result
is later called "disable" and "curr_dirs" later.

> -       if (!(enable & (1 << bit)))
> -               outb(enable | (1 << bit), sch->iobase + offset);
> +       if (!(enable & BIT(bit)))
> +               outb(enable | BIT(bit), sch->iobase + offset);
>
>         spin_unlock(&sch->lock);
>  }
>
> -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
> +static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio,
> +                                   unsigned reg)
>  {
> -       struct sch_gpio *sch = to_sch_gpio(gc);
> -       u8 curr_dirs;
>         unsigned short offset, bit;
> +       u8 disable;
>
>         spin_lock(&sch->lock);
>
> -       offset = sch_gpio_offset(sch, gpio_num, GIO);
> -       bit = sch_gpio_bit(sch, gpio_num);
> -
> -       curr_dirs = inb(sch->iobase + offset);
> +       offset = sch_gpio_offset(sch, gpio, reg);
> +       bit = sch_gpio_bit(sch, gpio);
>
> -       if (!(curr_dirs & (1 << bit)))
> -               outb(curr_dirs | (1 << bit), sch->iobase + offset);
> +       disable = inb(sch->iobase + offset);
> +       if (disable & BIT(bit))
> +               outb(disable & ~BIT(bit), sch->iobase + offset);
>
>         spin_unlock(&sch->lock);
> -       return 0;
>  }
>
> -static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
> +static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> -       int res;
>         unsigned short offset, bit;
> +       u8 curr_dirs;
>
> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> -       bit = sch_gpio_bit(sch, gpio_num);
> +       offset = sch_gpio_offset(sch, gpio, reg);
> +       bit = sch_gpio_bit(sch, gpio);
>
> -       res = !!(inb(sch->iobase + offset) & (1 << bit));
> +       curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
>
> -       return res;
> +       return curr_dirs;
>  }
>
> -static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
> +static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg,
> +                            int val)
>  {
>         struct sch_gpio *sch = to_sch_gpio(gc);
> -       u8 curr_vals;
>         unsigned short offset, bit;
> +       u8 curr_dirs;
>
> -       spin_lock(&sch->lock);
> -
> -       offset = sch_gpio_offset(sch, gpio_num, GLV);
> -       bit = sch_gpio_bit(sch, gpio_num);
> +       offset = sch_gpio_offset(sch, gpio, reg);
> +       bit = sch_gpio_bit(sch, gpio);
>
> -       curr_vals = inb(sch->iobase + offset);
> +       curr_dirs = inb(sch->iobase + offset);
>
>         if (val)
> -               outb(curr_vals | (1 << bit), sch->iobase + offset);
> +               outb(curr_dirs | BIT(bit), sch->iobase + offset);
>         else
> -               outb((curr_vals & ~(1 << bit)), sch->iobase + offset);
> +               outb((curr_dirs & ~BIT(bit)), sch->iobase + offset);
> +}

Mmmm this really looks like sch_gpio_register_set() and
sch_gpio_register_clear() could just call sch_gpio_reg_set() right
after acquiring the spinlock. Also the names are very similar and it
is not clear why you need two different functions here. Couldn't you
call sch_gpio_reg_set(gc, gpio, reg, 1) in place of
sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for
sch_gpio_register_clear()? You may need to acquire the spinlock before
some call sites, but that's preferable to having very similar
functions bearing a very similar name IMHO.

>
> +static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
> +{
> +       struct sch_gpio *sch = to_sch_gpio(gc);
> +
> +       spin_lock(&sch->lock);
> +       sch_gpio_register_set(sch, gpio_num, GIO);

So here you are acquiring sch->lock, then calling
sch_gpio_register_set() which will try to acquire the same spinlock
first thing. Wouldn't that deadlock? Have you tested changing the
direction of a GPIO? This again speaks in favor or reducing the number
of similar functions in this file.

Unless I misunderstood something there are still some issues that make
this file harder to understand than it should, and which can sometimes
bork the system altogether. It is a good idea to cleanup this file,
but please try to go one step further - this should simplify locking
and ultimately get rid of the locking issues.

And hopefully I will also take less time to review v4. :P
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html