Commit 07c7b6a5 authored by Linus Walleij's avatar Linus Walleij
Browse files

gpio: mmio: Also read bits that are zero 



The code for .get_multiple() has bugs:

1. The simple .get_multiple() just reads a register, masks it
and sets the return value. This is not correct: we only want to
assign values (whether 0 or 1) to the bits that are set in the
mask. Fix this by using &= ~mask to clear all bits in the mask
and then |= val & mask to set the corresponding bits from the
read.

2. The bgpio_get_multiple_be() call has a similar problem: it
uses the |= operator to set the bits, so only the bits in the
mask are affected, but it misses to clear all returned bits
from the mask initially, so some bits will be returned
erroneously set to 1.

3. The bgpio_get_set_multiple() again fails to clear the bits
from the mask.

4. find_next_bit() wasn't handled correctly, use a totally
different approach for one function and change the other
function to follow the design pattern of assigning the first
bit to -1, then use bit + 1 in the for loop and < num_iterations
as break condition.

Fixes: 80057cb4 ("gpio-mmio: Use the new .get_multiple() callback")
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Reported-by: default avatarClemens Gruber <clemens.gruber@pqgruber.com>
Tested-by: default avatarClemens Gruber <clemens.gruber@pqgruber.com>
Reported-by: default avatarLukas Wunner <lukas@wunner.de>
Signed-off-by: default avatarLinus Walleij <linus.walleij@linaro.org>
parent a8750ddc

drivers/gpio/gpio-mmio.c

+16 −14
Original line number Diff line number Diff line
@@ -152,14 +152,13 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask, 
{

	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);

	}

	/* Make sure we first clear any bits that are zero when we read the register */

	*bits &= ~*mask;



	/* Exploit the fact that we know which directions are set */

	set_mask = *mask & gc->bgpio_dir;

	get_mask = *mask & ~gc->bgpio_dir;



	if (set_mask)

		*bits |= gc->read_reg(gc->reg_set) & set_mask;
@@ -176,13 +175,13 @@ static int bgpio_get(struct gpio_chip *gc, unsigned int 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;

	/* Make sure we first clear any bits that are zero when we read the register */

	*bits &= ~*mask;

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

	return 0;

}
@@ -196,9 +195,12 @@ static int bgpio_get_multiple_be(struct gpio_chip *gc, unsigned long *mask, 
	unsigned long val;

	int bit;



	/* Make sure we first clear any bits that are zero when we read the register */

	*bits &= ~*mask;



	/* Create a mirrored mask */

	bit = 0;

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

	bit = -1;

	while ((bit = find_next_bit(mask, gc->ngpio, bit + 1)) < gc->ngpio)

		readmask |= bgpio_line2mask(gc, bit);



	/* Read the register */
@@ -208,8 +210,8 @@ static int bgpio_get_multiple_be(struct gpio_chip *gc, unsigned long *mask, 
	 * 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)

	bit = -1;

	while ((bit = find_next_bit(&val, gc->ngpio, bit + 1)) < gc->ngpio)

		*bits |= bgpio_line2mask(gc, bit);



	return 0;

CRA Git | Maintained and supported by SUSTech CRA and CCSE