* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-03-20 19:12 ` Alexander Stein
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-03-20 19:12 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Linus Walleij
Cc: Alexander Stein, linux-arm-kernel, linux-gpio
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)
+ 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,
--
2.3.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-03-20 19:12 ` Alexander Stein
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-03-20 19:12 UTC (permalink / raw)
To: linux-arm-kernel
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)
+ 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,
--
2.3.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] pinctrl: at91: Add missing include
2015-03-20 19:12 ` Alexander Stein
@ 2015-03-20 19:12 ` Alexander Stein
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-03-20 19:12 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Linus Walleij
Cc: Alexander Stein, linux-arm-kernel, linux-gpio
This fixes the sparse warning:
drivers/pinctrl/pinctrl-at91.c:1556:6: warning: symbol
'at91_pinctrl_gpio_suspend' was not declared. Should it be static?
drivers/pinctrl/pinctrl-at91.c:1580:6: warning: symbol
'at91_pinctrl_gpio_resume' was not declared. Should it be static?
Signed-off-by: Alexander Stein <alexanders83@web.de>
---
drivers/pinctrl/pinctrl-at91.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index a882523..02f6477 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -25,6 +25,8 @@
/* Since we request GPIOs from ourself */
#include <linux/pinctrl/consumer.h>
+#include <mach/hardware.h>
+
#include "pinctrl-at91.h"
#include "core.h"
--
2.3.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] pinctrl: at91: Add missing include
@ 2015-03-20 19:12 ` Alexander Stein
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-03-20 19:12 UTC (permalink / raw)
To: linux-arm-kernel
This fixes the sparse warning:
drivers/pinctrl/pinctrl-at91.c:1556:6: warning: symbol
'at91_pinctrl_gpio_suspend' was not declared. Should it be static?
drivers/pinctrl/pinctrl-at91.c:1580:6: warning: symbol
'at91_pinctrl_gpio_resume' was not declared. Should it be static?
Signed-off-by: Alexander Stein <alexanders83@web.de>
---
drivers/pinctrl/pinctrl-at91.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index a882523..02f6477 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -25,6 +25,8 @@
/* Since we request GPIOs from ourself */
#include <linux/pinctrl/consumer.h>
+#include <mach/hardware.h>
+
#include "pinctrl-at91.h"
#include "core.h"
--
2.3.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
2015-03-20 19:12 ` Alexander Stein
@ 2015-03-27 9:07 ` Linus Walleij
-1 siblings, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2015-03-27 9:07 UTC (permalink / raw)
To: Alexander Stein, Jean-Christophe Plagniol-Villard
Cc: linux-arm-kernel, linux-gpio
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-03-27 9:07 ` Linus Walleij
0 siblings, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2015-03-27 9:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
2015-03-20 19:12 ` Alexander Stein
@ 2015-03-27 10:11 ` Ludovic Desroches
-1 siblings, 0 replies; 18+ messages in thread
From: Ludovic Desroches @ 2015-03-27 10:11 UTC (permalink / raw)
To: Alexander Stein
Cc: Jean-Christophe Plagniol-Villard, Linus Walleij,
linux-arm-kernel, linux-gpio
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-03-27 10:11 ` Ludovic Desroches
0 siblings, 0 replies; 18+ messages in thread
From: Ludovic Desroches @ 2015-03-27 10:11 UTC (permalink / raw)
To: linux-arm-kernel
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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Re: [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
2015-03-27 10:11 ` Ludovic Desroches
@ 2015-03-27 11:54 ` Alexander Stein
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-03-27 11:54 UTC (permalink / raw)
To: Ludovic Desroches
Cc: Jean-Christophe Plagniol-Villard, Linus Walleij,
linux-arm-kernel, linux-gpio
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-03-27 11:54 ` Alexander Stein
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-03-27 11:54 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
2015-03-27 9:07 ` Linus Walleij
@ 2015-04-01 7:45 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 18+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2015-04-01 7:45 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexander Stein, linux-gpio, Jean-Christophe PLAGNIOL-VILLARD,
linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-04-01 7:45 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 18+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2015-04-01 7:45 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] pinctrl: at91: Add missing include
2015-03-20 19:12 ` Alexander Stein
@ 2015-04-02 8:19 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 18+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2015-04-02 8:19 UTC (permalink / raw)
To: Alexander Stein; +Cc: linux-gpio, Linus Walleij, linux-arm-kernel
On 20:12 Fri 20 Mar , Alexander Stein wrote:
> This fixes the sparse warning:
> drivers/pinctrl/pinctrl-at91.c:1556:6: warning: symbol
> 'at91_pinctrl_gpio_suspend' was not declared. Should it be static?
> drivers/pinctrl/pinctrl-at91.c:1580:6: warning: symbol
> 'at91_pinctrl_gpio_resume' was not declared. Should it be static?
>
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
> drivers/pinctrl/pinctrl-at91.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index a882523..02f6477 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -25,6 +25,8 @@
> /* Since we request GPIOs from ourself */
> #include <linux/pinctrl/consumer.h>
>
> +#include <mach/hardware.h>
NACK this will break multi arch support
Best Regards,
J.
> +
> #include "pinctrl-at91.h"
> #include "core.h"
>
> --
> 2.3.3
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] pinctrl: at91: Add missing include
@ 2015-04-02 8:19 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 18+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2015-04-02 8:19 UTC (permalink / raw)
To: linux-arm-kernel
On 20:12 Fri 20 Mar , Alexander Stein wrote:
> This fixes the sparse warning:
> drivers/pinctrl/pinctrl-at91.c:1556:6: warning: symbol
> 'at91_pinctrl_gpio_suspend' was not declared. Should it be static?
> drivers/pinctrl/pinctrl-at91.c:1580:6: warning: symbol
> 'at91_pinctrl_gpio_resume' was not declared. Should it be static?
>
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
> drivers/pinctrl/pinctrl-at91.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index a882523..02f6477 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -25,6 +25,8 @@
> /* Since we request GPIOs from ourself */
> #include <linux/pinctrl/consumer.h>
>
> +#include <mach/hardware.h>
NACK this will break multi arch support
Best Regards,
J.
> +
> #include "pinctrl-at91.h"
> #include "core.h"
>
> --
> 2.3.3
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
2015-03-20 19:12 ` Alexander Stein
@ 2015-04-02 8:30 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 18+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2015-04-02 8:30 UTC (permalink / raw)
To: Alexander Stein; +Cc: Linus Walleij, linux-arm-kernel, linux-gpio
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature
@ 2015-04-02 8:30 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 18+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2015-04-02 8:30 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Re: [PATCH 2/2] pinctrl: at91: Add missing include
2015-04-02 8:19 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2015-04-02 9:39 ` Alexander Stein
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-04-02 9:39 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: Linus Walleij, linux-arm-kernel, linux-gpio
On Thursday 02 April 2015, 10:19:41 wrote Jean-Christophe PLAGNIOL-VILLARD:
> On 20:12 Fri 20 Mar , Alexander Stein wrote:
> > This fixes the sparse warning:
> > drivers/pinctrl/pinctrl-at91.c:1556:6: warning: symbol
> > 'at91_pinctrl_gpio_suspend' was not declared. Should it be static?
> > drivers/pinctrl/pinctrl-at91.c:1580:6: warning: symbol
> > 'at91_pinctrl_gpio_resume' was not declared. Should it be static?
> >
> > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > ---
> > drivers/pinctrl/pinctrl-at91.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> > index a882523..02f6477 100644
> > --- a/drivers/pinctrl/pinctrl-at91.c
> > +++ b/drivers/pinctrl/pinctrl-at91.c
> > @@ -25,6 +25,8 @@
> > /* Since we request GPIOs from ourself */
> > #include <linux/pinctrl/consumer.h>
> >
> > +#include <mach/hardware.h>
> NACK this will break multi arch support
Mh, i see. So I guess this sparse warning will stay there then.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] pinctrl: at91: Add missing include
@ 2015-04-02 9:39 ` Alexander Stein
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Stein @ 2015-04-02 9:39 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 02 April 2015, 10:19:41 wrote Jean-Christophe PLAGNIOL-VILLARD:
> On 20:12 Fri 20 Mar , Alexander Stein wrote:
> > This fixes the sparse warning:
> > drivers/pinctrl/pinctrl-at91.c:1556:6: warning: symbol
> > 'at91_pinctrl_gpio_suspend' was not declared. Should it be static?
> > drivers/pinctrl/pinctrl-at91.c:1580:6: warning: symbol
> > 'at91_pinctrl_gpio_resume' was not declared. Should it be static?
> >
> > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > ---
> > drivers/pinctrl/pinctrl-at91.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> > index a882523..02f6477 100644
> > --- a/drivers/pinctrl/pinctrl-at91.c
> > +++ b/drivers/pinctrl/pinctrl-at91.c
> > @@ -25,6 +25,8 @@
> > /* Since we request GPIOs from ourself */
> > #include <linux/pinctrl/consumer.h>
> >
> > +#include <mach/hardware.h>
> NACK this will break multi arch support
Mh, i see. So I guess this sparse warning will stay there then.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-04-02 9:40 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-20 19:12 [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature Alexander Stein
2015-03-20 19:12 ` Alexander Stein
2015-03-20 19:12 ` [PATCH 2/2] pinctrl: at91: Add missing include Alexander Stein
2015-03-20 19:12 ` Alexander Stein
2015-04-02 8:19 ` Jean-Christophe PLAGNIOL-VILLARD
2015-04-02 8:19 ` Jean-Christophe PLAGNIOL-VILLARD
2015-04-02 9:39 ` Alexander Stein
2015-04-02 9:39 ` Alexander Stein
2015-03-27 9:07 ` [PATCH 1/2] pinctrl: at91: Add set_multiple GPIO chip feature Linus Walleij
2015-03-27 9:07 ` Linus Walleij
2015-04-01 7:45 ` Jean-Christophe PLAGNIOL-VILLARD
2015-04-01 7:45 ` Jean-Christophe PLAGNIOL-VILLARD
2015-03-27 10:11 ` Ludovic Desroches
2015-03-27 10:11 ` Ludovic Desroches
2015-03-27 11:54 ` Alexander Stein
2015-03-27 11:54 ` Alexander Stein
2015-04-02 8:30 ` Jean-Christophe PLAGNIOL-VILLARD
2015-04-02 8:30 ` Jean-Christophe PLAGNIOL-VILLARD
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.