| Message ID | 20190905141304.22005-1-alexandre.belloni@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: at91-pio4: implement .get_multiple and .set_multiple | expand |
On 05.09.2019 17:13, Alexandre Belloni wrote:
> + pr_err("ABE: %d %08x\n", bank, bits[word]);
Is this needed?
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
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.
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
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 > >
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, };
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(+)