[PATCH] gpio: 74x164: Implement gpiochip.set_multiple()

[PATCH] gpio: 74x164: Implement gpiochip.set_multiple()

[Geert Uytterhoeven]

This allows to set multiple outputs using a single SPI transfer.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/gpio/gpio-74x164.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index c81224ff2dca988b..62291a81c97f7140 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -75,6 +75,29 @@ static void gen_74x164_set_value(struct gpio_chip *gc,
 	mutex_unlock(&chip->lock);
 }
 
+static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct gen_74x164_chip *chip = gpiochip_get_data(gc);
+	unsigned int i, idx, shift;
+	u8 bank, bankmask;
+
+	mutex_lock(&chip->lock);
+	for (i = 0, bank = chip->registers - 1; i < chip->registers;
+	     i++, bank--) {
+		idx = i / sizeof(*mask);
+		shift = i % sizeof(*mask) * BITS_PER_BYTE;
+		bankmask = mask[idx] >> shift;
+		if (!bankmask)
+			continue;
+
+		chip->buffer[bank] &= ~bankmask;
+		chip->buffer[bank] |= bankmask & (bits[idx] >> shift);
+	}
+	__gen_74x164_write_config(chip);
+	mutex_unlock(&chip->lock);
+}
+
 static int gen_74x164_direction_output(struct gpio_chip *gc,
 		unsigned offset, int val)
 {
@@ -114,6 +137,7 @@ static int gen_74x164_probe(struct spi_device *spi)
 	chip->gpio_chip.direction_output = gen_74x164_direction_output;
 	chip->gpio_chip.get = gen_74x164_get_value;
 	chip->gpio_chip.set = gen_74x164_set_value;
+	chip->gpio_chip.set_multiple = gen_74x164_set_multiple;
 	chip->gpio_chip.base = -1;
 
 	chip->registers = nregs;
-- 
1.9.1

Re: [PATCH] gpio: 74x164: Implement gpiochip.set_multiple()

[Phil Reid]

On 14/03/2016 11:19 PM, Geert Uytterhoeven wrote:
> This allows to set multiple outputs using a single SPI transfer.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Phil Reid <preid@electromag.com.au>


I do have a general question about GPIO drivers.
pca953x does not update the cached data unless the write operation
was successful. Which I folowed with the implement of set_multiple.
However a number of other drivers update regardless.
eg chip->buffer is updated even if the write_config fails.

What is the preferred approach?

> ---
>   drivers/gpio/gpio-74x164.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
> index c81224ff2dca988b..62291a81c97f7140 100644
> --- a/drivers/gpio/gpio-74x164.c
> +++ b/drivers/gpio/gpio-74x164.c
> @@ -75,6 +75,29 @@ static void gen_74x164_set_value(struct gpio_chip *gc,
>   	mutex_unlock(&chip->lock);
>   }
>
> +static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> +				    unsigned long *bits)
> +{
> +	struct gen_74x164_chip *chip = gpiochip_get_data(gc);
> +	unsigned int i, idx, shift;
> +	u8 bank, bankmask;
> +
> +	mutex_lock(&chip->lock);
> +	for (i = 0, bank = chip->registers - 1; i < chip->registers;
> +	     i++, bank--) {
> +		idx = i / sizeof(*mask);
> +		shift = i % sizeof(*mask) * BITS_PER_BYTE;
> +		bankmask = mask[idx] >> shift;
> +		if (!bankmask)
> +			continue;
> +
> +		chip->buffer[bank] &= ~bankmask;
> +		chip->buffer[bank] |= bankmask & (bits[idx] >> shift);
> +	}
> +	__gen_74x164_write_config(chip);
> +	mutex_unlock(&chip->lock);
> +}
> +
>   static int gen_74x164_direction_output(struct gpio_chip *gc,
>   		unsigned offset, int val)
>   {
> @@ -114,6 +137,7 @@ static int gen_74x164_probe(struct spi_device *spi)
>   	chip->gpio_chip.direction_output = gen_74x164_direction_output;
>   	chip->gpio_chip.get = gen_74x164_get_value;
>   	chip->gpio_chip.set = gen_74x164_set_value;
> +	chip->gpio_chip.set_multiple = gen_74x164_set_multiple;
>   	chip->gpio_chip.base = -1;
>
>   	chip->registers = nregs;
>


-- 
Regards
Phil Reid

Re: [PATCH] gpio: 74x164: Implement gpiochip.set_multiple()

[Linus Walleij]

On Tue, Mar 15, 2016 at 5:04 AM, Phil Reid <preid@electromag.com.au> wrote:

> pca953x does not update the cached data unless the write operation
> was successful. Which I folowed with the implement of set_multiple.
> However a number of other drivers update regardless.
> eg chip->buffer is updated even if the write_config fails.
>
> What is the preferred approach?

Obviously the other drivers are buggy and need to be fixed.
It can't be too many since the number of drivers with failable
write operations aren's so many. (And I guess they seldom fail
so it's not a big real world problem.) But it should be fixed.

Add Axel Lin to the thread, he's awesome at finding semantic
bugs.

Patches accepted :)

Yours,
Linus Walleij

Re: [PATCH] gpio: 74x164: Implement gpiochip.set_multiple()

[Linus Walleij]

On Mon, Mar 14, 2016 at 4:19 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> This allows to set multiple outputs using a single SPI transfer.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Patch applied for v4.7 with Phil's Review tag.

Yours,
Linus Walleij