| Message ID | 1426878721-2618-1-git-send-email-alexanders83@web.de |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 20, 2015 at 8:12 PM, Alexander Stein <alexanders83@web.de> wrote: > This adds the callback for set_multiple. > As this controller has a separate set and clear register, we can't write > directly to PIO_ODSR as this would required a cached variable and would > race with at91_gpio_set. > So build masks for the PIO_SODR and PIO_CODR registers and write them > together. > > Signed-off-by: Alexander Stein <alexanders83@web.de> > --- > This was tested by using an own test driver which uses > gpiod_set_array_cansleep to set multiple GPIOs at once. I remember J-C brought this up ages ago, so I hope he will be delighted to see we can now, FINALLY, do this! J-C can I have your review on this? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alexander, On Fri, Mar 20, 2015 at 08:12:00PM +0100, Alexander Stein wrote: > This adds the callback for set_multiple. > As this controller has a separate set and clear register, we can't write > directly to PIO_ODSR as this would required a cached variable and would > race with at91_gpio_set. > So build masks for the PIO_SODR and PIO_CODR registers and write them > together. Sure seems safer and easier to use SODR and CODR. > > Signed-off-by: Alexander Stein <alexanders83@web.de> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> A question below. > --- > This was tested by using an own test driver which uses > gpiod_set_array_cansleep to set multiple GPIOs at once. > > drivers/pinctrl/pinctrl-at91.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index f4cd0b9..a882523 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -1330,6 +1330,33 @@ static void at91_gpio_set(struct gpio_chip *chip, unsigned offset, > writel_relaxed(mask, pio + (val ? PIO_SODR : PIO_CODR)); > } > > +static void at91_gpio_set_multiple(struct gpio_chip *chip, > + unsigned long *mask, unsigned long *bits) > +{ > + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); > + void __iomem *pio = at91_gpio->regbase; > + unsigned long set_mask; > + unsigned long clear_mask; > + size_t i; > + > + set_mask = 0; > + clear_mask = 0; > + > + for (i = 0; i < chip->ngpio; i++) { > + if (*mask == 0) > + break; > + if (__test_and_clear_bit(i, mask)) { For my knowledge, why do you need to clear the mask? > + if (test_bit(i, bits)) > + set_mask |= BIT(i); > + else > + clear_mask |= BIT(i); > + } > + } > + > + writel_relaxed(set_mask, pio + PIO_SODR); > + writel_relaxed(clear_mask, pio + PIO_CODR); > +} > + > static int at91_gpio_direction_output(struct gpio_chip *chip, unsigned offset, > int val) > { > @@ -1692,6 +1719,7 @@ static struct gpio_chip at91_gpio_template = { > .get = at91_gpio_get, > .direction_output = at91_gpio_direction_output, > .set = at91_gpio_set, > + .set_multiple = at91_gpio_set_multiple, > .dbg_show = at91_gpio_dbg_show, > .can_sleep = false, > .ngpio = MAX_NB_GPIO_PER_BANK, > -- > 2.3.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Ludovic, On Friday 27 March 2015, 11:11:52 wrote Ludovic Desroches: > On Fri, Mar 20, 2015 at 08:12:00PM +0100, Alexander Stein wrote: > > This adds the callback for set_multiple. > > As this controller has a separate set and clear register, we can't write > > directly to PIO_ODSR as this would required a cached variable and would > > race with at91_gpio_set. > > So build masks for the PIO_SODR and PIO_CODR registers and write them > > together. > > Sure seems safer and easier to use SODR and CODR. > > > > > Signed-off-by: Alexander Stein <alexanders83@web.de> > Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> > > A question below. > > > --- > > This was tested by using an own test driver which uses > > gpiod_set_array_cansleep to set multiple GPIOs at once. > > > > drivers/pinctrl/pinctrl-at91.c | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > > index f4cd0b9..a882523 100644 > > --- a/drivers/pinctrl/pinctrl-at91.c > > +++ b/drivers/pinctrl/pinctrl-at91.c > > @@ -1330,6 +1330,33 @@ static void at91_gpio_set(struct gpio_chip *chip, unsigned offset, > > writel_relaxed(mask, pio + (val ? PIO_SODR : PIO_CODR)); > > } > > > > +static void at91_gpio_set_multiple(struct gpio_chip *chip, > > + unsigned long *mask, unsigned long *bits) > > +{ > > + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); > > + void __iomem *pio = at91_gpio->regbase; > > + unsigned long set_mask; > > + unsigned long clear_mask; > > + size_t i; > > + > > + set_mask = 0; > > + clear_mask = 0; > > + > > + for (i = 0; i < chip->ngpio; i++) { > > + if (*mask == 0) > > + break; > > + if (__test_and_clear_bit(i, mask)) { > > For my knowledge, why do you need to clear the mask? I tried to do the same as mpc8xxx_gpio_set_multiple. I think the reason is that an empty mask will quit that loop potentially earlier. Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
J > On Mar 27, 2015, at 5:07 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, Mar 20, 2015 at 8:12 PM, Alexander Stein <alexanders83@web.de> wrote: > >> This adds the callback for set_multiple. >> As this controller has a separate set and clear register, we can't write >> directly to PIO_ODSR as this would required a cached variable and would >> race with at91_gpio_set. >> So build masks for the PIO_SODR and PIO_CODR registers and write them >> together. >> >> Signed-off-by: Alexander Stein <alexanders83@web.de> >> --- >> This was tested by using an own test driver which uses >> gpiod_set_array_cansleep to set multiple GPIOs at once. > > I remember J-C brought this up ages ago, so I hope he will be delighted > to see we can now, FINALLY, do this! > > J-C can I have your review on this? I will check it tonight Best Regards, J. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20:12 Fri 20 Mar , Alexander Stein wrote: > This adds the callback for set_multiple. > As this controller has a separate set and clear register, we can't write > directly to PIO_ODSR as this would required a cached variable and would > race with at91_gpio_set. > So build masks for the PIO_SODR and PIO_CODR registers and write them > together. > > Signed-off-by: Alexander Stein <alexanders83@web.de> > --- > This was tested by using an own test driver which uses > gpiod_set_array_cansleep to set multiple GPIOs at once. > > drivers/pinctrl/pinctrl-at91.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c > index f4cd0b9..a882523 100644 > --- a/drivers/pinctrl/pinctrl-at91.c > +++ b/drivers/pinctrl/pinctrl-at91.c > @@ -1330,6 +1330,33 @@ static void at91_gpio_set(struct gpio_chip *chip, unsigned offset, > writel_relaxed(mask, pio + (val ? PIO_SODR : PIO_CODR)); > } > > +static void at91_gpio_set_multiple(struct gpio_chip *chip, > + unsigned long *mask, unsigned long *bits) > +{ > + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); > + void __iomem *pio = at91_gpio->regbase; > + unsigned long set_mask; > + unsigned long clear_mask; > + size_t i; > + > + set_mask = 0; > + clear_mask = 0; > + > + for (i = 0; i < chip->ngpio; i++) { > + if (*mask == 0) so why do you loop? > + break; > + if (__test_and_clear_bit(i, mask)) { > + if (test_bit(i, bits)) > + set_mask |= BIT(i); > + else > + clear_mask |= BIT(i); > + } > + } just use mask to invert the mask for the CODR #define BITS_MASK(bits) (((bits) == 32) ? ~0U : (BIT(bits) - 1)) uint32_t set_mask = *mask & BITS_MASK(chip->ngpio);; uint32_t clear_mask = ~set_mask; writel_relaxed(set_mask, pio + PIO_SODR); writel_relaxed(clear_mask, pio + PIO_CODR); Best Regards, J. > + > + writel_relaxed(set_mask, pio + PIO_SODR); > + writel_relaxed(clear_mask, pio + PIO_CODR); > +} > + > static int at91_gpio_direction_output(struct gpio_chip *chip, unsigned offset, > int val) > { > @@ -1692,6 +1719,7 @@ static struct gpio_chip at91_gpio_template = { > .get = at91_gpio_get, > .direction_output = at91_gpio_direction_output, > .set = at91_gpio_set, > + .set_multiple = at91_gpio_set_multiple, > .dbg_show = at91_gpio_dbg_show, > .can_sleep = false, > .ngpio = MAX_NB_GPIO_PER_BANK, > -- > 2.3.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c index f4cd0b9..a882523 100644 --- a/drivers/pinctrl/pinctrl-at91.c +++ b/drivers/pinctrl/pinctrl-at91.c @@ -1330,6 +1330,33 @@ static void at91_gpio_set(struct gpio_chip *chip, unsigned offset, writel_relaxed(mask, pio + (val ? PIO_SODR : PIO_CODR)); } +static void at91_gpio_set_multiple(struct gpio_chip *chip, + unsigned long *mask, unsigned long *bits) +{ + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); + void __iomem *pio = at91_gpio->regbase; + unsigned long set_mask; + unsigned long clear_mask; + size_t i; + + set_mask = 0; + clear_mask = 0; + + for (i = 0; i < chip->ngpio; i++) { + if (*mask == 0) + break; + if (__test_and_clear_bit(i, mask)) { + if (test_bit(i, bits)) + set_mask |= BIT(i); + else + clear_mask |= BIT(i); + } + } + + writel_relaxed(set_mask, pio + PIO_SODR); + writel_relaxed(clear_mask, pio + PIO_CODR); +} + static int at91_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int val) { @@ -1692,6 +1719,7 @@ static struct gpio_chip at91_gpio_template = { .get = at91_gpio_get, .direction_output = at91_gpio_direction_output, .set = at91_gpio_set, + .set_multiple = at91_gpio_set_multiple, .dbg_show = at91_gpio_dbg_show, .can_sleep = false, .ngpio = MAX_NB_GPIO_PER_BANK,
This adds the callback for set_multiple. As this controller has a separate set and clear register, we can't write directly to PIO_ODSR as this would required a cached variable and would race with at91_gpio_set. So build masks for the PIO_SODR and PIO_CODR registers and write them together. Signed-off-by: Alexander Stein <alexanders83@web.de> --- This was tested by using an own test driver which uses gpiod_set_array_cansleep to set multiple GPIOs at once. drivers/pinctrl/pinctrl-at91.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)