pinctrl: at91-pio4: implement .get_multiple and .set_multiple
diff mbox series

Message ID 20190905141304.22005-1-alexandre.belloni@bootlin.com
State New
Headers show
Series
  • pinctrl: at91-pio4: implement .get_multiple and .set_multiple
Related show

Commit Message

Alexandre Belloni Sept. 5, 2019, 2:13 p.m. UTC
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 <alexandre.belloni@bootlin.com>
---
 drivers/pinctrl/pinctrl-at91-pio4.c | 60 +++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Claudiu Beznea Sept. 5, 2019, 2:33 p.m. UTC | #1
On 05.09.2019 17:13, Alexandre Belloni wrote:
> +		pr_err("ABE: %d %08x\n", bank, bits[word]);

Is this needed?
Linus Walleij Sept. 11, 2019, 12:27 a.m. UTC | #2
On Thu, Sep 5, 2019 at 3:13 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> 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 <alexandre.belloni@bootlin.com>

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
Alexandre Belloni Sept. 11, 2019, 9:11 a.m. UTC | #3
On 11/09/2019 01:27:10+0100, Linus Walleij wrote:
> On Thu, Sep 5, 2019 at 3:13 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> 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 <alexandre.belloni@bootlin.com>
> 
> 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.
Linus Walleij Sept. 12, 2019, 1:46 p.m. UTC | #4
On Wed, Sep 11, 2019 at 10:11 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> 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
Ludovic Desroches Sept. 14, 2019, 7:36 p.m. UTC | #5
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 <alexandre.belloni@bootlin.com>

Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

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

Patch
diff mbox series

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,
 };