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: <5719D00F.3020202@st.com>
Date:	Fri, 22 Apr 2016 09:17:35 +0200
From:	Patrice Chotard <patrice.chotard@...com>
To:	Linus Walleij <linus.walleij@...aro.org>
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 04/20/2016 04:53 PM, Linus Walleij wrote:
> 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?

Ok i will rework all these points

Thanks

Patrice

>
> Yours,
> Linus Walleij