From: patrice.chotard@st•com (Patrice Chotard)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support
Date: Fri, 22 Apr 2016 09:17:35 +0200 [thread overview]
Message-ID: <5719D00F.3020202@st.com> (raw)
In-Reply-To: <CACRpkdaLhgR8PcKuR1svU=iAEUq=-USN1zktvdor128kEO=G9A@mail.gmail.com>
On 04/20/2016 04:53 PM, Linus Walleij wrote:
> On Tue, Apr 19, 2016 at 2:18 PM, <patrice.chotard@st•com> wrote:
>
>> From: Patrice Chotard <patrice.chotard@st•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@st•com>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st•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
next prev parent reply other threads:[~2016-04-22 7:17 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-19 12:18 [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support patrice.chotard at st.com
2016-04-19 12:18 ` [PATCH 1/8] mfd: stmpe: Add STMPE_IDX_SYS_CTRL/2 enum patrice.chotard at st.com
2016-04-20 14:32 ` Linus Walleij
2016-04-26 8:19 ` Lee Jones
2016-04-19 12:18 ` [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant patrice.chotard at st.com
2016-04-20 14:34 ` Linus Walleij
2016-04-26 8:18 ` Lee Jones
2016-04-19 12:18 ` [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection patrice.chotard at st.com
2016-04-19 12:38 ` kbuild test robot
2016-04-20 14:37 ` Linus Walleij
2016-04-21 13:48 ` Patrice Chotard
2016-04-19 12:18 ` [PATCH 4/8] gpio: stmpe: write int status register only when needed patrice.chotard at st.com
2016-04-20 14:38 ` Linus Walleij
2016-04-19 12:18 ` [PATCH 5/8] Documentation: dt: add stmpe1600 compatible string to stmpe mfd patrice.chotard at st.com
2016-04-20 14:39 ` Linus Walleij
2016-04-19 12:18 ` [PATCH 6/8] mfd: Add STMPE1600 support patrice.chotard at st.com
2016-04-20 14:43 ` Linus Walleij
2016-04-21 13:51 ` Patrice Chotard
2016-04-19 12:18 ` [PATCH 7/8] gpio: stmpe: " patrice.chotard at st.com
2016-04-20 14:53 ` Linus Walleij
2016-04-22 7:17 ` Patrice Chotard [this message]
2016-04-19 12:18 ` [PATCH 8/8] gpio: stmpe: configure GPIO as output by default patrice.chotard at st.com
2016-04-20 14:56 ` Linus Walleij
2016-04-19 12:41 ` [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Thierry Reding
2016-04-20 16:15 ` Marcel Ziswiler
2016-04-19 15:53 ` Stephen Warren
2016-04-20 7:40 ` Patrice Chotard
2016-04-20 16:02 ` Stephen Warren
2016-04-21 2:39 ` Marcel Ziswiler
2016-04-20 14:25 ` Linus Walleij
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5719D00F.3020202@st.com \
--to=patrice.chotard@st$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox