linux-kernel - Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support

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]

Message-ID: <CACRpkdaLhgR8PcKuR1svU=iAEUq=-USN1zktvdor128kEO=G9A@mail.gmail.com>
Date:	Wed, 20 Apr 2016 16:53:45 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Patrice CHOTARD <patrice.chotard@...com>
Cc:	Lee Jones <lee.jones@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Maxime Coquelin <maxime.coquelin@...com>,
	amelie.delaunay@...com, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <kernel@...gutronix.de>,
	Dinh Nguyen <dinguyen@...nsource.altera.com>,
	Viresh Kumar <vireshk@...nel.org>,
	Shiraz Hashim <shiraz.linux.kernel@...il.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support

On Tue, Apr 19, 2016 at 2:18 PM,  <patrice.chotard@...com> wrote:

> From: Patrice Chotard <patrice.chotard@...com>
>
> The particularities of this variant are:
> - GPIO_XXX_LSB and GPIO_XXX_MSB memory locations are inverted compared
>   to other variants.
> - There is no Edge detection, Rising Edge and Falling Edge registers.
> - IRQ flags are cleared when read, no need to write in Status register.
>
> Signed-off-by: Amelie DELAUNAY <amelie.delaunay@...com>
> Signed-off-by: Patrice Chotard <patrice.chotard@...com>

> -       u8 reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);
> +       u8 reg;
>         u8 mask = 1 << (offset % 8);
>         int ret;
>
> +       if (stmpe->partnum == STMPE1600)
> +               reg = stmpe->regs[STMPE_IDX_GPMR_LSB] + (offset / 8);
> +       else
> +               reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);

This construct is a bit hard to grasp.

Can we think of something more intuitive? Maybe using more
code lines but easier to understand.

Subtracting the offset is just totally unintuitive in the first place,
the STMPE1600 arrangement is much more intuitive.

I would prefer if we address the LSB+MSB register explicitly
instead of adding or subtracting 1 to the LSB register to get
to the MSB register.

> +       if (stmpe->partnum == STMPE1600)
> +               reg = stmpe->regs[which] + (offset / 8);
> +       else
> +               reg = stmpe->regs[which] - (offset / 8);

Same.

> +       if (stmpe->partnum == STMPE1600)
> +               reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
> +       else
> +               reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);

Same.

> +       if (stmpe->partnum == STMPE1600)
> +               reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
> +       else
> +               reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);

Same.

> +                               stmpe_reg_write(stmpe,
> +                                               stmpe->regs[regmap[i]] + j,
> +                                               new);
> +                       else
> +                               stmpe_reg_write(stmpe,
> +                                               stmpe->regs[regmap[i]] - j,
> +                                               new);

This is also unintuitively backwards.

> +       if (stmpe->partnum == STMPE1600)
> +               dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
> +       else
> +               dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);

Same.

> +       if (stmpe->partnum == STMPE1600)
> +               statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_LSB];
> +       else
> +               statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_MSB];

And this kind of points at the problem.

Can we write this in some way that make it super-clear which register
we're using and why?

Yours,
Linus Walleij