linux-kernel - Re: [PATCHv4 1/3] gpio: sch: Consolidate similar algorithms

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]

Date:	Thu, 4 Dec 2014 17:43:36 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	GPIO Subsystem Mailing List <linux-gpio@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Mika Westerberg <mika.westerberg@...el.com>,
	Denis Turischev <denis.turischev@...pulab.co.il>
Subject: Re: [PATCHv4 1/3] gpio: sch: Consolidate similar algorithms

2014-11-26 13:47 GMT+09:00 Chang Rebecca Swee Fun
<rebecca.swee.fun.chang@...el.com>:
> Consolidating similar algorithms into common functions to make
> GPIO SCH simpler and manageable.
>
> Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
> ---
>  drivers/gpio/gpio-sch.c |   81 ++++++++++++++++-------------------------------
>  1 file changed, 28 insertions(+), 53 deletions(-)

Your v3 had this:

 drivers/gpio/gpio-sch.c |   95 ++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

Just looking at these two lines we can already tell this v4 is going
to be *much* better. :)

>
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 99720c8..7967f14 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -63,75 +63,59 @@ 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 int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg)
>  {
> +       struct sch_gpio *sch = to_sch_gpio(gc);
>         unsigned short offset, bit;
> -       u8 enable;
> -
> -       spin_lock(&sch->lock);
> +       u8 curr_dirs;

This name sounds weird in this function. It has been copied from the
direction setting functions, but this function is much more generic.
So maybe come with a more generic name as well?

>
> -       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);
> -       if (!(enable & (1 << bit)))
> -               outb(enable | (1 << bit), sch->iobase + offset);
> +       curr_dirs = !!(inb(sch->iobase + offset) & BIT(bit));
>
> -       spin_unlock(&sch->lock);
> +       return curr_dirs;
>  }
>
> -static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned  gpio_num)
> +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_dirs;
>         unsigned short offset, bit;
> +       u8 curr_dirs;

Same here.

Meh, I guess that's my only complain... This really makes the driver
easier to understand, so after fixing this minor issue and rebasing on
Linus' devel branch, feel free to add my

Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/