diff mbox

[05/33] gpio: add generic single-register fixed-direction GPIO driver

Message ID E1beJjz-0000mA-0o@rmk-PC.armlinux.org.uk
State New
Headers show

Commit Message

Russell King (Oracle) Aug. 29, 2016, 10:24 a.m. UTC
Add a simple, generic, single register fixed-direction GPIO driver.
This is able to support a single register where a fixed number of
bits are used for input and a fixed number of bits used for output.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpio/Kconfig     |   6 ++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-reg.c  | 139 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/gpio-reg.h |  12 ++++
 4 files changed, 158 insertions(+)
 create mode 100644 drivers/gpio/gpio-reg.c
 create mode 100644 include/linux/gpio-reg.h

Comments

Robert Jarzmik Aug. 29, 2016, 7:39 p.m. UTC | #1
Hi Russell,

Russell King <rmk+kernel@armlinux.org.uk> writes:

> Add a simple, generic, single register fixed-direction GPIO driver.
> This is able to support a single register where a fixed number of
> bits are used for input and a fixed number of bits used for output.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpio/Kconfig     |   6 ++
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-reg.c  | 139 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/gpio-reg.h |  12 ++++
>  4 files changed, 158 insertions(+)
>  create mode 100644 drivers/gpio/gpio-reg.c
>  create mode 100644 include/linux/gpio-reg.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 98dd47a30fc7..49bd8b89712e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -365,6 +365,12 @@ config GPIO_RCAR
>  	help
>  	  Say yes here to support GPIO on Renesas R-Car SoCs.
>  
> +config GPIO_REG
> +	bool
So I suppose it is on purpose you forbid it to be a module. Is there a way to
write either in the commit message or in the Kconfig this purpose, so that
nobody on "purpose" changes this bool to tristate ?

> +	help
> +	  A 32-bit single register GPIO fixed in/out implementation.  This
> +	  can be used to represent any register as a set of GPIO signals.
Another question I was asking myself was how it differenciated itself from
gpio-mmio, ie. what brought the need for this driver that isn't available with
gpio-mmio ?

I seem to understand that this is mainly for platform code, hence the "builtin
only" necessity, and if I'm right a part of your cover letter could very well
fit within this patch's commit message.

> diff --git a/drivers/gpio/gpio-reg.c b/drivers/gpio/gpio-reg.c
> new file mode 100644
> index 000000000000..fc7e0a395f9f
> --- /dev/null
> +++ b/drivers/gpio/gpio-reg.c

...zip...
> +static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> +	unsigned long *bits)
> +{
> +	struct gpio_reg *r = to_gpio_reg(gc);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&r->lock, flags);
> +	r->out = (r->out & ~*mask) | *bits;
Shouldn't this be :
	r->out = (r->out & ~*mask) | (*bits & *mask);

> diff --git a/include/linux/gpio-reg.h b/include/linux/gpio-reg.h
> new file mode 100644
> index 000000000000..0352bec7319a
> --- /dev/null
> +++ b/include/linux/gpio-reg.h
> @@ -0,0 +1,12 @@
> +#ifndef GPIO_REG_H
> +#define GPIO_REG_H
> +
> +struct device;
> +
> +struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg,
> +	int base, int num, const char *label, u32 direction, u32 def_out,
> +	const char *const *names);
Maybe this one would deserve a doxygen comment ?

Cheers.
Russell King (Oracle) Aug. 29, 2016, 11:12 p.m. UTC | #2
On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote:
> Hi Russell,
> 
> Russell King <rmk+kernel@armlinux.org.uk> writes:
> 
> > Add a simple, generic, single register fixed-direction GPIO driver.
> > This is able to support a single register where a fixed number of
> > bits are used for input and a fixed number of bits used for output.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/gpio/Kconfig     |   6 ++
> >  drivers/gpio/Makefile    |   1 +
> >  drivers/gpio/gpio-reg.c  | 139 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/gpio-reg.h |  12 ++++
> >  4 files changed, 158 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-reg.c
> >  create mode 100644 include/linux/gpio-reg.h
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 98dd47a30fc7..49bd8b89712e 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -365,6 +365,12 @@ config GPIO_RCAR
> >  	help
> >  	  Say yes here to support GPIO on Renesas R-Car SoCs.
> >  
> > +config GPIO_REG
> > +	bool
> So I suppose it is on purpose you forbid it to be a module. Is there a way to
> write either in the commit message or in the Kconfig this purpose, so that
> nobody on "purpose" changes this bool to tristate ?

The only forbidding is that it's used from code early at boot, so it has
to be available to the kernel for some of these platforms to function.
Even if it's changed to tristate, the select from these platforms would
force it to be built-in.

> > +	help
> > +	  A 32-bit single register GPIO fixed in/out implementation.  This
> > +	  can be used to represent any register as a set of GPIO signals.
> Another question I was asking myself was how it differenciated itself from
> gpio-mmio, ie. what brought the need for this driver that isn't available with
> gpio-mmio ?

gpio-mmio doesn't allow the fixed direction - it assumes that you always
have some form of direction register.  It also doesn't do the double-read
that's necessary for some platforms to get the "current" state of inputs.
It also doesn't do the "use 32-bit accessors even for smaller numbers of
GPIOs".  Lastly, it assumes that the current output state can be read
from the registers - this is not always the case (and is not the case for
Assabet.)

> I seem to understand that this is mainly for platform code, hence the
> "builtin only" necessity, and if I'm right a part of your cover letter
> could very well fit within this patch's commit message.

Apart from the missing MODULE_* tags and symbol exports, there's nothing
whic prohibits it becoming a module.

> > +static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> > +	unsigned long *bits)
> > +{
> > +	struct gpio_reg *r = to_gpio_reg(gc);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&r->lock, flags);
> > +	r->out = (r->out & ~*mask) | *bits;
> Shouldn't this be :
> 	r->out = (r->out & ~*mask) | (*bits & *mask);

Possibly, but the latter is redundant when used through gpiolib.

> > diff --git a/include/linux/gpio-reg.h b/include/linux/gpio-reg.h
> > new file mode 100644
> > index 000000000000..0352bec7319a
> > --- /dev/null
> > +++ b/include/linux/gpio-reg.h
> > @@ -0,0 +1,12 @@
> > +#ifndef GPIO_REG_H
> > +#define GPIO_REG_H
> > +
> > +struct device;
> > +
> > +struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg,
> > +	int base, int num, const char *label, u32 direction, u32 def_out,
> > +	const char *const *names);
> Maybe this one would deserve a doxygen comment ?

Maybe, but I've been told not to put such comments in header files.
I've already spent the last two weeks on this stuff (at the expense
of reading mail), I've not got around to thinking about any kind of
documentation.
Alexander Shiyan Aug. 30, 2016, 6:08 a.m. UTC | #3
>Вторник, 30 августа 2016, 2:12 +03:00 от Russell King - ARM Linux <linux@armlinux.org.uk>:
>
>On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote:
>> Hi Russell,
>> 
>> Russell King < rmk+kernel@armlinux.org.uk > writes:
>> 
>> > Add a simple, generic, single register fixed-direction GPIO driver.
>> > This is able to support a single register where a fixed number of
>> > bits are used for input and a fixed number of bits used for output.
>> >
>> > Signed-off-by: Russell King < rmk+kernel@armlinux.org.uk >

There is a GPIO driver which already performs these tasks.
Plaease take a look on the gpio-74xx-mmio driver.

---
Russell King (Oracle) Aug. 30, 2016, 7:41 a.m. UTC | #4
On Tue, Aug 30, 2016 at 09:08:03AM +0300, Alexander Shiyan wrote:
> >Вторник, 30 августа 2016, 2:12 +03:00 от Russell King - ARM Linux <linux@armlinux.org.uk>:
> >
> >On Mon, Aug 29, 2016 at 09:39:54PM +0200, Robert Jarzmik wrote:
> >> Hi Russell,
> >> 
> >> Russell King < rmk+kernel@armlinux.org.uk > writes:
> >> 
> >> > Add a simple, generic, single register fixed-direction GPIO driver.
> >> > This is able to support a single register where a fixed number of
> >> > bits are used for input and a fixed number of bits used for output.
> >> >
> >> > Signed-off-by: Russell King < rmk+kernel@armlinux.org.uk >
> 
> There is a GPIO driver which already performs these tasks.
> Plaease take a look on the gpio-74xx-mmio driver.

I did, and no it doesn't, because:

1. It is either all-in or all-out, it doesn't support a mixture of
   fixed-directions for a single register.
2. It is DT-only, I need it for legacy platforms.
3. It uses the bgpio stuff, which is unsuitable for reasons already
   covered in a previous reply.

So, gpio-74xx-mmio is unsuitable.
Linus Walleij Aug. 30, 2016, 9:25 p.m. UTC | #5
On Mon, Aug 29, 2016 at 12:24 PM, Russell King
<rmk+kernel@armlinux.org.uk> wrote:

> Add a simple, generic, single register fixed-direction GPIO driver.
> This is able to support a single register where a fixed number of
> bits are used for input and a fixed number of bits used for output.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Clever, I like it!

>  include/linux/gpio-reg.h |  12 ++++

Can we put this in include/linux/gpio/gpio-reg.h?

I try to do some scopeing to <linux/gpio/*> there.

> +#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc)

You don't need that trickery anymore, just:

> +static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct gpio_reg *r = to_gpio_reg(gc);

struct gpio_reg *r = gpiochip_get_data(gc);

(applied everywhere)

> +       if (dev)
> +               ret = devm_gpiochip_add_data(dev, &r->gc, r);
> +       else
> +               ret = gpiochip_add_data(&r->gc, r);

Aha both device and device-less, I see.

Apart from that it looks nice, any other questionmarks were
fixed in the other replies.

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
Russell King (Oracle) Aug. 30, 2016, 9:42 p.m. UTC | #6
On Tue, Aug 30, 2016 at 11:25:19PM +0200, Linus Walleij wrote:
> On Mon, Aug 29, 2016 at 12:24 PM, Russell King
> <rmk+kernel@armlinux.org.uk> wrote:
> 
> > Add a simple, generic, single register fixed-direction GPIO driver.
> > This is able to support a single register where a fixed number of
> > bits are used for input and a fixed number of bits used for output.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Clever, I like it!
> 
> >  include/linux/gpio-reg.h |  12 ++++
> 
> Can we put this in include/linux/gpio/gpio-reg.h?
> 
> I try to do some scopeing to <linux/gpio/*> there.

Sure, I'll just have to hunt through all the patches to find an occurance
of the include to fix them all up...

> > +#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc)
> 
> You don't need that trickery anymore, just:
> 
> > +static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset)
> > +{
> > +       struct gpio_reg *r = to_gpio_reg(gc);
> 
> struct gpio_reg *r = gpiochip_get_data(gc);
> 
> (applied everywhere)

I prefer my method by a long shot - it's always going to work because
the gpiochip is embedded within the gpio_reg structure, and the compiler
is inteligent enough to keep a single pointer around.  With your
suggestion, the compiler has no idea that 'r' and 'gc' are actually
the same pointer, but a different type, and we end up having to carry
around identical pointers in two registers rather than just one.

It makes more sense to use gpiochip_get_data() if gpio_chip were a const
data structure that was never embedded, but the way *gpiochip_add*()
writes to the structure and the presence of members like "base" prevents
that.

> 
> > +       if (dev)
> > +               ret = devm_gpiochip_add_data(dev, &r->gc, r);
> > +       else
> > +               ret = gpiochip_add_data(&r->gc, r);
> 
> Aha both device and device-less, I see.

Yes, to avoid problems with the transition to it - the legacy APIs
(such as ASSABET_BCR_frob(), etc) can be called really early, so we
need the gpiochip available early as well so we can keep the legacy
APIs working until they can be killed off.  There's some corner
cases in the assabet code which make that difficult at the moment.

This whole patch set is still very much a work-in-progress - there's
more that needs doing, but I wanted to get _this_ out there so that
people can reviewing it, and hopefully get it queued for the next
merge window.

> Apart from that it looks nice, any other questionmarks were
> fixed in the other replies.
> 
> Yours,
> Linus Walleij
Linus Walleij Aug. 30, 2016, 9:47 p.m. UTC | #7
On Tue, Aug 30, 2016 at 11:42 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Aug 30, 2016 at 11:25:19PM +0200, Linus Walleij wrote:

>> > +#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc)
>>
>> You don't need that trickery anymore, just:
>>
>> > +static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset)
>> > +{
>> > +       struct gpio_reg *r = to_gpio_reg(gc);
>>
>> struct gpio_reg *r = gpiochip_get_data(gc);
>>
>> (applied everywhere)
>
> I prefer my method by a long shot

Sure it's no strong preference. Keep it like this if you like.
I'm very happy with the series either way!

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

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 98dd47a30fc7..49bd8b89712e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -365,6 +365,12 @@  config GPIO_RCAR
 	help
 	  Say yes here to support GPIO on Renesas R-Car SoCs.
 
+config GPIO_REG
+	bool
+	help
+	  A 32-bit single register GPIO fixed in/out implementation.  This
+	  can be used to represent any register as a set of GPIO signals.
+
 config GPIO_SPEAR_SPICS
 	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
 	depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 2a035ed8f168..3fc904fcc595 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -88,6 +88,7 @@  obj-$(CONFIG_GPIO_PXA)		+= gpio-pxa.o
 obj-$(CONFIG_GPIO_RC5T583)	+= gpio-rc5t583.o
 obj-$(CONFIG_GPIO_RDC321X)	+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_RCAR)		+= gpio-rcar.o
+obj-$(CONFIG_GPIO_REG)		+= gpio-reg.o
 obj-$(CONFIG_ARCH_SA1100)	+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
 obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
diff --git a/drivers/gpio/gpio-reg.c b/drivers/gpio/gpio-reg.c
new file mode 100644
index 000000000000..fc7e0a395f9f
--- /dev/null
+++ b/drivers/gpio/gpio-reg.c
@@ -0,0 +1,139 @@ 
+#include <linux/gpio/driver.h>
+#include <linux/gpio-reg.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+struct gpio_reg {
+	struct gpio_chip gc;
+	spinlock_t lock;
+	u32 direction;
+	u32 out;
+	void __iomem *reg;
+};
+
+#define to_gpio_reg(x) container_of(x, struct gpio_reg, gc)
+
+static int gpio_reg_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+
+	return r->direction & BIT(offset) ? 1 : 0;
+}
+
+static int gpio_reg_direction_output(struct gpio_chip *gc, unsigned offset,
+	int value)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+
+	if (r->direction & BIT(offset))
+		return -ENOTSUPP;
+
+	gc->set(gc, offset, value);
+	return 0;
+}
+
+static int gpio_reg_direction_input(struct gpio_chip *gc, unsigned offset)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+
+	return r->direction & BIT(offset) ? 0 : -ENOTSUPP;
+}
+
+static void gpio_reg_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+	unsigned long flags;
+	u32 val, mask = BIT(offset);
+
+	spin_lock_irqsave(&r->lock, flags);
+	val = r->out;
+	if (value)
+		val |= mask;
+	else
+		val &= ~mask;
+	r->out = val;
+	writel_relaxed(val, r->reg);
+	spin_unlock_irqrestore(&r->lock, flags);
+}
+
+static int gpio_reg_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+	u32 val, mask = BIT(offset);
+
+	if (r->direction & mask) {
+		/*
+		 * double-read the value, some registers latch after the
+		 * first read.
+		 */
+		readl_relaxed(r->reg);
+		val = readl_relaxed(r->reg);
+	} else {
+		val = r->out;
+	}
+	return !!(val & mask);
+}
+
+static void gpio_reg_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+	unsigned long *bits)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&r->lock, flags);
+	r->out = (r->out & ~*mask) | *bits;
+	writel_relaxed(r->out, r->reg);
+	spin_unlock_irqrestore(&r->lock, flags);
+}
+
+struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg,
+	int base, int num, const char *label, u32 direction, u32 def_out,
+	const char *const *names)
+{
+	struct gpio_reg *r;
+	int ret;
+
+	if (dev)
+		r = devm_kzalloc(dev, sizeof(*r), GFP_KERNEL);
+	else
+		r = kzalloc(sizeof(*r), GFP_KERNEL);
+
+	if (!r)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock_init(&r->lock);
+
+	r->gc.label = label;
+	r->gc.get_direction = gpio_reg_get_direction;
+	r->gc.direction_input = gpio_reg_direction_input;
+	r->gc.direction_output = gpio_reg_direction_output;
+	r->gc.set = gpio_reg_set;
+	r->gc.get = gpio_reg_get;
+	r->gc.set_multiple = gpio_reg_set_multiple;
+	r->gc.base = base;
+	r->gc.ngpio = num;
+	r->gc.names = names;
+	r->direction = direction;
+	r->out = def_out;
+	r->reg = reg;
+
+	if (dev)
+		ret = devm_gpiochip_add_data(dev, &r->gc, r);
+	else
+		ret = gpiochip_add_data(&r->gc, r);
+
+	return ret ? ERR_PTR(ret) : &r->gc;
+}
+
+int gpio_reg_resume(struct gpio_chip *gc)
+{
+	struct gpio_reg *r = to_gpio_reg(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&r->lock, flags);
+	writel_relaxed(r->out, r->reg);
+	spin_unlock_irqrestore(&r->lock, flags);
+
+	return 0;
+}
diff --git a/include/linux/gpio-reg.h b/include/linux/gpio-reg.h
new file mode 100644
index 000000000000..0352bec7319a
--- /dev/null
+++ b/include/linux/gpio-reg.h
@@ -0,0 +1,12 @@ 
+#ifndef GPIO_REG_H
+#define GPIO_REG_H
+
+struct device;
+
+struct gpio_chip *gpio_reg_init(struct device *dev, void __iomem *reg,
+	int base, int num, const char *label, u32 direction, u32 def_out,
+	const char *const *names);
+
+int gpio_reg_resume(struct gpio_chip *gc);
+
+#endif