Message ID | E1beJjz-0000mA-0o@rmk-PC.armlinux.org.uk |
---|---|
State | New |
Headers | show |
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.
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.
>Вторник, 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. ---
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.
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
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
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 --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
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