Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383465

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

From Linus Walleij <linus.walleij@linaro.org>
Newsgroups linux.kernel
Subject Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support
Date 2016-04-20 17:00 +0200
Message-ID <rq1LI-13P-3@gated-at.bofh.it> (permalink)
References <rpCNk-6J2-3@gated-at.bofh.it> <rpCX0-6Oi-9@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

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?

Yours,
Linus Walleij

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/8] STMPE fixes/rework and add STMPE1600 support <patrice.chotard@st.com> - 2016-04-19 14:30 +0200
  [PATCH 7/8] gpio: stmpe: Add STMPE1600 support <patrice.chotard@st.com> - 2016-04-19 14:30 +0200
    Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support Linus Walleij <linus.walleij@linaro.org> - 2016-04-20 17:00 +0200
      Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support Patrice Chotard <patrice.chotard@st.com> - 2016-04-22 09:20 +0200
  [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection <patrice.chotard@st.com> - 2016-04-19 14:30 +0200
    Re: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge  detection kbuild test robot <lkp@intel.com> - 2016-04-19 14:40 +0200
    Re: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection Linus Walleij <linus.walleij@linaro.org> - 2016-04-20 16:40 +0200
      Re: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge  detection Patrice Chotard <patrice.chotard@st.com> - 2016-04-21 15:50 +0200
  [PATCH 8/8] gpio: stmpe: configure GPIO as output by default <patrice.chotard@st.com> - 2016-04-19 14:30 +0200
    Re: [PATCH 8/8] gpio: stmpe: configure GPIO as output by default Linus Walleij <linus.walleij@linaro.org> - 2016-04-20 17:00 +0200
  [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant <patrice.chotard@st.com> - 2016-04-19 14:30 +0200
    Re: [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant Linus Walleij <linus.walleij@linaro.org> - 2016-04-20 16:40 +0200
    Re: [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant Lee Jones <lee.jones@linaro.org> - 2016-04-26 10:20 +0200
  Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Thierry Reding <thierry.reding@gmail.com> - 2016-04-19 14:50 +0200
    Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Marcel Ziswiler <marcel.ziswiler@toradex.com> - 2016-04-20 20:00 +0200
  Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Stephen Warren <swarren@wwwdotorg.org> - 2016-04-19 18:00 +0200
    Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Patrice Chotard <patrice.chotard@st.com> - 2016-04-20 09:50 +0200
      Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Stephen Warren <swarren@wwwdotorg.org> - 2016-04-20 18:10 +0200
        Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Marcel Ziswiler <marcel.ziswiler@toradex.com> - 2016-04-21 05:20 +0200
    Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support Linus Walleij <linus.walleij@linaro.org> - 2016-04-20 16:30 +0200

csiph-web