diff mbox series

[v3] gpio-mmio: Use the new .get_multiple() callback

Message ID 20171020151030.25094-1-linus.walleij@linaro.org
State Accepted
Commit 80057cb417b2873cf645ac85568118c32f038f4c
Headers show
Series [v3] gpio-mmio: Use the new .get_multiple() callback | expand

Commit Message

Linus Walleij Oct. 20, 2017, 3:10 p.m. UTC 
It is possible to read all lines of a generic MMIO GPIO chip
with a single register read so support this if we are in
native endianness.

Add an especially quirky callback to read multiple lines for
the variants that require you to read values from the
output registers if and only if the line is set as output.
We managed to do that with a maximum of two register reads,
and just one read if the requested lines are all input or all
output.

Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v2->v3:
- Check the right big endianness flag: we want special handling
  for BE bitorder, not for BE byteorder.
- Implement a .get_multiple() solution for big-endian bit order
  if there is a single register to read all bits.
- Rebase on top of pin2mask() cleanups.
ChangeLog v1->v2:
- Only assign the functions for multiple get if we are in
  native endianness. Else each bit needs to be processed
  differently after read and it will be a mess. Likely
  the old get functions are better in this case.
- Directly write into *bits in both functions.
---
 drivers/gpio/gpio-mmio.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 3 deletions(-)

-- 
2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Lukas Wunner Jan. 15, 2018, 6:47 p.m. UTC | #1 
On Fri, Oct 20, 2017 at 05:10:30PM +0200, Linus Walleij wrote:
> It is possible to read all lines of a generic MMIO GPIO chip

> with a single register read so support this if we are in

> native endianness.

> 

> Add an especially quirky callback to read multiple lines for

> the variants that require you to read values from the

> output registers if and only if the line is set as output.

> We managed to do that with a maximum of two register reads,

> and just one read if the requested lines are all input or all

> output.

> 

> Cc: Anton Vorontsov <anton@enomsg.org>

> Cc: Lukas Wunner <lukas@wunner.de>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


I was cc'ed on this patch but unfortunately never got around to
reviewing it as I was swamped with other stuff.  Making up on that
now as the patch is causing a regression.


> +/*

> + * This assumes that the bits in the GPIO register are in native endianness.

> + * We only assign the function pointer if we have that.

> + */

> +static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,

> +				  unsigned long *bits)

> +{

> +	unsigned long get_mask = 0;

> +	unsigned long set_mask = 0;

> +	int bit = 0;

> +

> +	while ((bit = find_next_bit(mask, gc->ngpio, bit)) != gc->ngpio) {

> +		if (gc->bgpio_dir & BIT(bit))

> +			set_mask |= BIT(bit);

> +		else

> +			get_mask |= BIT(bit);

> +	}

> +

> +	if (set_mask)

> +		*bits |= gc->read_reg(gc->reg_set) & set_mask;

> +	if (get_mask)

> +		*bits |= gc->read_reg(gc->reg_dat) & get_mask;

> +

> +	return 0;

> +}


The while loop needs a "bit++;" at the end lest it becomes an
infinite loop.  That's because find_next_bit() starts searching
from "bit", not the bit after "bit".

Second, the while loop appears to be unnecessary, it seems it would
be possible to simply do this:

	set_mask = mask & gc->bgpio_dir;
	get_mask = mask & ~gc->bgpio_dir;

Third, I think we need to initialize *bits = 0 in case it contains
garbage.

Fourth, I notice bgpio_dir is an unsigned long.  Not a pointer to
an unsigned long, so not a bitmap.  Which means we cannot handle
gc->ngpio > sizeof(unsigned long).  Yet the driver doesn't check this
condition on ->probe and bail out if it's true.  Of course, once we
*do* support more than sizeof(unsigned long) pins, we need the while
loop again.

These issues are repeated in the remainder of the patch.

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner Jan. 15, 2018, 7:28 p.m. UTC | #2 
On Mon, Jan 15, 2018 at 07:47:28PM +0100, Lukas Wunner wrote:
> On Fri, Oct 20, 2017 at 05:10:30PM +0200, Linus Walleij wrote:

> Third, I think we need to initialize *bits = 0 in case it contains

> garbage.


Scratch that, it's the other way 'round, I remember now that in
max3191x_get_multiple() I deliberately only touched the bits in
*bits that are also set in *mask.

However there's another bug then:

bgpio_get_multiple() touches bits in *bits not set in *mask because
it outright assigns a value to *bits.  This should be something like:

        *bits &= ~*mask;
	*bits |= gc->read_reg(gc->reg_dat) & *mask;

And bgpio_get_set_multiple() only ORs bits to *bits but never clears it.
That's wrong, you need to AND with the inverse of *mask first, see above.
bgpio_get_multiple_be() has the same problem.

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 63bb48878de1..f9042bcc27a4 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -143,11 +143,78 @@  static int bgpio_get_set(struct gpio_chip *gc, unsigned int gpio)
 		return !!(gc->read_reg(gc->reg_dat) & pinmask);
 }
 
+/*
+ * This assumes that the bits in the GPIO register are in native endianness.
+ * We only assign the function pointer if we have that.
+ */
+static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				  unsigned long *bits)
+{
+	unsigned long get_mask = 0;
+	unsigned long set_mask = 0;
+	int bit = 0;
+
+	while ((bit = find_next_bit(mask, gc->ngpio, bit)) != gc->ngpio) {
+		if (gc->bgpio_dir & BIT(bit))
+			set_mask |= BIT(bit);
+		else
+			get_mask |= BIT(bit);
+	}
+
+	if (set_mask)
+		*bits |= gc->read_reg(gc->reg_set) & set_mask;
+	if (get_mask)
+		*bits |= gc->read_reg(gc->reg_dat) & get_mask;
+
+	return 0;
+}
+
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	return !!(gc->read_reg(gc->reg_dat) & bgpio_line2mask(gc, gpio));
 }
 
+/*
+ * This only works if the bits in the GPIO register are in native endianness.
+ * It is dirt simple and fast in this case. (Also the most common case.)
+ */
+static int bgpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+			      unsigned long *bits)
+{
+
+	*bits = gc->read_reg(gc->reg_dat) & *mask;
+	return 0;
+}
+
+/*
+ * With big endian mirrored bit order it becomes more tedious.
+ */
+static int bgpio_get_multiple_be(struct gpio_chip *gc, unsigned long *mask,
+				 unsigned long *bits)
+{
+	unsigned long readmask = 0;
+	unsigned long val;
+	int bit;
+
+	/* Create a mirrored mask */
+	bit = 0;
+	while ((bit = find_next_bit(mask, gc->ngpio, bit)) != gc->ngpio)
+		readmask |= bgpio_line2mask(gc, bit);
+
+	/* Read the register */
+	val = gc->read_reg(gc->reg_dat) & readmask;
+
+	/*
+	 * Mirror the result into the "bits" result, this will give line 0
+	 * in bit 0 ... line 31 in bit 31 for a 32bit register.
+	 */
+	bit = 0;
+	while ((bit = find_next_bit(&val, gc->ngpio, bit)) != gc->ngpio)
+		*bits |= bgpio_line2mask(gc, bit);
+
+	return 0;
+}
+
 static void bgpio_set_none(struct gpio_chip *gc, unsigned int gpio, int val)
 {
 }
@@ -455,10 +522,24 @@  static int bgpio_setup_io(struct gpio_chip *gc,
 	}
 
 	if (!(flags & BGPIOF_UNREADABLE_REG_SET) &&
-	    (flags & BGPIOF_READ_OUTPUT_REG_SET))
+	    (flags & BGPIOF_READ_OUTPUT_REG_SET)) {
 		gc->get = bgpio_get_set;
-	else
+		if (!gc->be_bits)
+			gc->get_multiple = bgpio_get_set_multiple;
+		/*
+		 * We deliberately avoid assigning the ->get_multiple() call
+		 * for big endian mirrored registers which are ALSO reflecting
+		 * their value in the set register when used as output. It is
+		 * simply too much complexity, let the GPIO core fall back to
+		 * reading each line individually in that fringe case.
+		 */
+	} else {
 		gc->get = bgpio_get;
+		if (gc->be_bits)
+			gc->get_multiple = bgpio_get_multiple_be;
+		else
+			gc->get_multiple = bgpio_get_multiple;
+	}
 
 	return 0;
 }
@@ -519,12 +600,12 @@  int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	gc->base = -1;
 	gc->ngpio = gc->bgpio_bits;
 	gc->request = bgpio_request;
+	gc->be_bits = !!(flags & BGPIOF_BIG_ENDIAN);
 
 	ret = bgpio_setup_io(gc, dat, set, clr, flags);
 	if (ret)
 		return ret;
 
-	gc->be_bits = !!(flags & BGPIOF_BIG_ENDIAN);
 	ret = bgpio_setup_accessors(dev, gc, flags & BGPIOF_BIG_ENDIAN_BYTE_ORDER);
 	if (ret)
 		return ret;