Implement .get_multiple and .set_multiple to allow reading or setting
multiple pins simultaneously. Pins in the same bank will all be switched at
the same time, improving synchronization and performances.
Signed-off-by: Alexandre Belloni <[email protected]>
---
drivers/pinctrl/pinctrl-at91-pio4.c | 60 +++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index d6de4d360cd4..488a302a60d4 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -328,6 +328,35 @@ static int atmel_gpio_get(struct gpio_chip *chip, unsigned offset)
return !!(reg & BIT(pin->line));
}
+static int atmel_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
+ unsigned int bank;
+
+ bitmap_zero(bits, atmel_pioctrl->npins);
+
+ for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
+ unsigned int word = bank;
+ unsigned int offset = 0;
+ unsigned int reg;
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+ word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
+ offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
+#endif
+ if (!mask[word])
+ continue;
+
+ reg = atmel_gpio_read(atmel_pioctrl, bank, ATMEL_PIO_PDSR);
+ bits[word] |= mask[word] & (reg << offset);
+
+ pr_err("ABE: %d %08x\n", bank, bits[word]);
+ }
+
+ return 0;
+}
+
static int atmel_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
int value)
{
@@ -358,11 +387,42 @@ static void atmel_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
BIT(pin->line));
}
+static void atmel_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
+ unsigned int bank;
+
+ for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
+ unsigned int bitmask;
+ unsigned int word = bank;
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+ word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
+#endif
+ if (!mask[word])
+ continue;
+
+ bitmask = mask[word] & bits[word];
+ atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_SODR, bitmask);
+
+ bitmask = mask[word] & ~bits[word];
+ atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_CODR, bitmask);
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+ mask[word] >>= ATMEL_PIO_NPINS_PER_BANK;
+ bits[word] >>= ATMEL_PIO_NPINS_PER_BANK;
+#endif
+ }
+}
+
static struct gpio_chip atmel_gpio_chip = {
.direction_input = atmel_gpio_direction_input,
.get = atmel_gpio_get,
+ .get_multiple = atmel_gpio_get_multiple,
.direction_output = atmel_gpio_direction_output,
.set = atmel_gpio_set,
+ .set_multiple = atmel_gpio_set_multiple,
.to_irq = atmel_gpio_to_irq,
.base = 0,
};
--
2.21.0
On Thu, Sep 5, 2019 at 3:13 PM Alexandre Belloni
<[email protected]> wrote:
>
> Implement .get_multiple and .set_multiple to allow reading or setting
> multiple pins simultaneously. Pins in the same bank will all be switched at
> the same time, improving synchronization and performances.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Good initiative!
> + for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {> + unsigned int word = bank;
> + unsigned int offset = 0;
> + unsigned int reg;
> +
> +#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
Should it not be > rather than != ?
> + word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
> + offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
> +#endif
This doesn't look good for multiplatform kernels.
We need to get rid of any compiletime constants like this.
Not your fault I suppose it is already there, but this really need
to be fixed. Any ideas?
Yours,
Linus Walleij
On 11/09/2019 01:27:10+0100, Linus Walleij wrote:
> On Thu, Sep 5, 2019 at 3:13 PM Alexandre Belloni
> <[email protected]> wrote:
> >
> > Implement .get_multiple and .set_multiple to allow reading or setting
> > multiple pins simultaneously. Pins in the same bank will all be switched at
> > the same time, improving synchronization and performances.
> >
> > Signed-off-by: Alexandre Belloni <[email protected]>
>
> Good initiative!
>
> > + for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {> + unsigned int word = bank;
> > + unsigned int offset = 0;
> > + unsigned int reg;
> > +
> > +#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
>
> Should it not be > rather than != ?
>
Realistically, the only case that could happen would be
ATMEL_PIO_NPINS_PER_BANK == 32 and BITS_PER_LONG ==64. so I would go for
ATMEL_PIO_NPINS_PER_BANK < BITS_PER_LONG
> > + word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
> > + offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
> > +#endif
>
> This doesn't look good for multiplatform kernels.
>
I don't think we have multiplatform kernels that run both in 32 and 64
bits. I don't believe ATMEL_PIO_NPINS_PER_BANK will ever change, it has
been 32 on all the atmel SoCs since 2001.
> We need to get rid of any compiletime constants like this.
>
> Not your fault I suppose it is already there, but this really need
> to be fixed. Any ideas?
>
I can go for a variable instead of a constant but the fact is that there
is currently no 64bit SoC with that IP. I added the compile time check
just in case a 64 bit SoC appears with that IP one day.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Sep 11, 2019 at 10:11 AM Alexandre Belloni
<[email protected]> wrote:
> On 11/09/2019 01:27:10+0100, Linus Walleij wrote:
> > > +#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
> >
> > Should it not be > rather than != ?
>
> Realistically, the only case that could happen would be
> ATMEL_PIO_NPINS_PER_BANK == 32 and BITS_PER_LONG ==64. so I would go for
> ATMEL_PIO_NPINS_PER_BANK < BITS_PER_LONG
OK I see.
> > > + word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
> > > + offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
> > > +#endif
> >
> > This doesn't look good for multiplatform kernels.
>
> I don't think we have multiplatform kernels that run both in 32 and 64
> bits. I don't believe ATMEL_PIO_NPINS_PER_BANK will ever change, it has
> been 32 on all the atmel SoCs since 2001.
So there is a bit missing from the commit message: the info that
the same driver is being used on 32 and 64 bit builds, and that is
the reason we allow compile-time ifdef things.
Can you add this to the commit message, or maybe
inline in the code, or both?
It confused me so it will confuse others.
Yours,
Linus Walleij
On Thu, Sep 05, 2019 at 04:13:04PM +0200, Alexandre Belloni wrote:
>
> Implement .get_multiple and .set_multiple to allow reading or setting
> multiple pins simultaneously. Pins in the same bank will all be switched at
> the same time, improving synchronization and performances.
>
> Signed-off-by: Alexandre Belloni <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>
Thanks for this improvement. You can keep my ack for v3 as the changes
should be the commit message only. I'll be off for three weeks.
Regards
Ludovic
> ---
> drivers/pinctrl/pinctrl-at91-pio4.c | 60 +++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
> index d6de4d360cd4..488a302a60d4 100644
> --- a/drivers/pinctrl/pinctrl-at91-pio4.c
> +++ b/drivers/pinctrl/pinctrl-at91-pio4.c
> @@ -328,6 +328,35 @@ static int atmel_gpio_get(struct gpio_chip *chip, unsigned offset)
> return !!(reg & BIT(pin->line));
> }
>
> +static int atmel_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
> + unsigned int bank;
> +
> + bitmap_zero(bits, atmel_pioctrl->npins);
> +
> + for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
> + unsigned int word = bank;
> + unsigned int offset = 0;
> + unsigned int reg;
> +
> +#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
> + word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
> + offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
> +#endif
> + if (!mask[word])
> + continue;
> +
> + reg = atmel_gpio_read(atmel_pioctrl, bank, ATMEL_PIO_PDSR);
> + bits[word] |= mask[word] & (reg << offset);
> +
> + pr_err("ABE: %d %08x\n", bank, bits[word]);
> + }
> +
> + return 0;
> +}
> +
> static int atmel_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
> int value)
> {
> @@ -358,11 +387,42 @@ static void atmel_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
> BIT(pin->line));
> }
>
> +static void atmel_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
> + unsigned int bank;
> +
> + for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
> + unsigned int bitmask;
> + unsigned int word = bank;
> +
> +#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
> + word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
> +#endif
> + if (!mask[word])
> + continue;
> +
> + bitmask = mask[word] & bits[word];
> + atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_SODR, bitmask);
> +
> + bitmask = mask[word] & ~bits[word];
> + atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_CODR, bitmask);
> +
> +#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
> + mask[word] >>= ATMEL_PIO_NPINS_PER_BANK;
> + bits[word] >>= ATMEL_PIO_NPINS_PER_BANK;
> +#endif
> + }
> +}
> +
> static struct gpio_chip atmel_gpio_chip = {
> .direction_input = atmel_gpio_direction_input,
> .get = atmel_gpio_get,
> + .get_multiple = atmel_gpio_get_multiple,
> .direction_output = atmel_gpio_direction_output,
> .set = atmel_gpio_set,
> + .set_multiple = atmel_gpio_set_multiple,
> .to_irq = atmel_gpio_to_irq,
> .base = 0,
> };
> --
> 2.21.0
>
>