2016-03-14 15:19:25

by Geert Uytterhoeven

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

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

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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


2016-03-15 04:04:37

by Phil Reid

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

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 <[email protected]>

Reviewed-by: Phil Reid <[email protected]>


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

2016-03-22 10:54:18

by Linus Walleij

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

On Mon, Mar 14, 2016 at 4:19 PM, Geert Uytterhoeven
<[email protected]> wrote:

> This allows to set multiple outputs using a single SPI transfer.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>

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

Yours,
Linus Walleij

2016-03-22 10:57:00

by Linus Walleij

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

On Tue, Mar 15, 2016 at 5:04 AM, Phil Reid <[email protected]> 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