[PATCH] gpio: add ETRAXFS GPIO driver

[PATCH] gpio: add ETRAXFS GPIO driver

[Rabin Vincent]

Add a GPIO driver for the General I/O block on Axis ETRAX FS SoCs.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 .../devicetree/bindings/gpio/gpio-etraxfs.txt      |  21 +++
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-etraxfs.c                        | 181 +++++++++++++++++++++
 4 files changed, 211 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-etraxfs.txt
 create mode 100644 drivers/gpio/gpio-etraxfs.c

diff --git a/Documentation/devicetree/bindings/gpio/gpio-etraxfs.txt b/Documentation/devicetree/bindings/gpio/gpio-etraxfs.txt
new file mode 100644
index 0000000..dad24ab
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-etraxfs.txt
@@ -0,0 +1,21 @@
+Axis ETRAX FS General I/O controller bindings
+
+Required properties:
+
+- compatible:
+  - "axis,etraxfs-gio"
+- reg: Physical base address and length of the controller's registers.
+- #gpio-cells: Should be 3
+  - The first cell is the port number (hex).
+  - The seconds cell is the gpio offset number.
+  - The third cell is reserved and is currently unused.
+- gpio-controller: Marks the device node as a GPIO controller.
+
+Example:
+
+	gio: gpio@b001a000 {
+		compatible = "axis,etraxfs-gio";
+		reg = <0xb001a000 0x1000>;
+		gpio-controller;
+		#gpio-cells = <3>;
+	};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index caefe80..fe0b9da 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -159,6 +159,14 @@ config GPIO_EP93XX
 	depends on ARCH_EP93XX
 	select GPIO_GENERIC
 
+config GPIO_ETRAXFS
+	bool "Axis ETRAX FS General I/O"
+	depends on CRIS || COMPILE_TEST
+	depends on OF
+	select GPIO_GENERIC
+	help
+	  Say yes here to support the GPIO controller on Axis ETRAX FS SoCs.
+
 config GPIO_F7188X
 	tristate "F71869, F71869A, F71882FG and F71889F GPIO support"
 	depends on X86
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index f71bb97..4f816ec 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_GPIO_DLN2)		+= gpio-dln2.o
 obj-$(CONFIG_GPIO_DWAPB)	+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
+obj-$(CONFIG_GPIO_ETRAXFS)	+= gpio-etraxfs.o
 obj-$(CONFIG_GPIO_F7188X)	+= gpio-f7188x.o
 obj-$(CONFIG_GPIO_GE_FPGA)	+= gpio-ge.o
 obj-$(CONFIG_GPIO_GRGPIO)	+= gpio-grgpio.o
diff --git a/drivers/gpio/gpio-etraxfs.c b/drivers/gpio/gpio-etraxfs.c
new file mode 100644
index 0000000..93ec548
--- /dev/null
+++ b/drivers/gpio/gpio-etraxfs.c
@@ -0,0 +1,181 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/basic_mmio_gpio.h>
+
+#define ETRAX_FS_rw_pa_dout	0
+#define ETRAX_FS_r_pa_din	4
+#define ETRAX_FS_rw_pa_oe	8
+#define ETRAX_FS_rw_intr_cfg	12
+#define ETRAX_FS_rw_intr_mask	16
+#define ETRAX_FS_rw_ack_intr	20
+#define ETRAX_FS_r_intr		24
+#define ETRAX_FS_rw_pb_dout	32
+#define ETRAX_FS_r_pb_din	36
+#define ETRAX_FS_rw_pb_oe	40
+#define ETRAX_FS_rw_pc_dout	48
+#define ETRAX_FS_r_pc_din	52
+#define ETRAX_FS_rw_pc_oe	56
+#define ETRAX_FS_rw_pd_dout	64
+#define ETRAX_FS_r_pd_din	68
+#define ETRAX_FS_rw_pd_oe	72
+#define ETRAX_FS_rw_pe_dout	80
+#define ETRAX_FS_r_pe_din	84
+#define ETRAX_FS_rw_pe_oe	88
+
+struct etraxfs_gpio_port {
+	const char *label;
+	unsigned int oe;
+	unsigned int dout;
+	unsigned int din;
+	unsigned int ngpio;
+};
+
+struct etraxfs_gpio_info {
+	int num_ports;
+	const struct etraxfs_gpio_port *ports;
+};
+
+static const struct etraxfs_gpio_port etraxfs_gpio_etraxfs_ports[] = {
+	{
+		.label	= "A",
+		.ngpio	= 8,
+		.oe	= ETRAX_FS_rw_pa_oe,
+		.dout	= ETRAX_FS_rw_pa_dout,
+		.din	= ETRAX_FS_r_pa_din,
+	},
+	{
+		.label	= "B",
+		.ngpio	= 18,
+		.oe	= ETRAX_FS_rw_pb_oe,
+		.dout	= ETRAX_FS_rw_pb_dout,
+		.din	= ETRAX_FS_r_pb_din,
+	},
+	{
+		.label	= "C",
+		.ngpio	= 18,
+		.oe	= ETRAX_FS_rw_pc_oe,
+		.dout	= ETRAX_FS_rw_pc_dout,
+		.din	= ETRAX_FS_r_pc_din,
+	},
+	{
+		.label	= "D",
+		.ngpio	= 18,
+		.oe	= ETRAX_FS_rw_pd_oe,
+		.dout	= ETRAX_FS_rw_pd_dout,
+		.din	= ETRAX_FS_r_pd_din,
+	},
+	{
+		.label	= "E",
+		.ngpio	= 18,
+		.oe	= ETRAX_FS_rw_pe_oe,
+		.dout	= ETRAX_FS_rw_pe_dout,
+		.din	= ETRAX_FS_r_pe_din,
+	},
+};
+
+static const struct etraxfs_gpio_info etraxfs_gpio_etraxfs = {
+	.num_ports = ARRAY_SIZE(etraxfs_gpio_etraxfs_ports),
+	.ports = etraxfs_gpio_etraxfs_ports,
+};
+
+static int etraxfs_gpio_of_xlate(struct gpio_chip *gc,
+			       const struct of_phandle_args *gpiospec,
+			       u32 *flags)
+{
+	/*
+	 * Port numbers are A to E, and the properties are integers, so we
+	 * specify them as 0xA - 0xE.
+	 */
+	if (gc->label[0] - 'A' + 0xA != gpiospec->args[0])
+		return -EINVAL;
+
+	if (gpiospec->args[1] >= gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	return gpiospec->args[1];
+}
+
+static const struct of_device_id etraxfs_gpio_of_table[] = {
+	{
+		.compatible = "axis,etraxfs-gio",
+		.data = &etraxfs_gpio_etraxfs,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, etraxfs_gpio_of_table);
+
+static int etraxfs_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct etraxfs_gpio_info *info;
+	const struct of_device_id *match;
+	struct bgpio_chip *chips;
+	struct resource *res;
+	void __iomem *regs;
+	int ret;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	regs = devm_ioremap_resource(dev, res);
+	if (!regs)
+		return -ENOMEM;
+
+	match = of_match_node(etraxfs_gpio_of_table, dev->of_node);
+	if (!match)
+		return -EINVAL;
+
+	info = match->data;
+
+	chips = devm_kzalloc(dev, sizeof(*chips) * info->num_ports, GFP_KERNEL);
+	if (!chips)
+		return -ENOMEM;
+
+	for (i = 0; i < info->num_ports; i++) {
+		struct bgpio_chip *bgc = &chips[i];
+		const struct etraxfs_gpio_port *port = &info->ports[i];
+
+		ret = bgpio_init(bgc, dev, 4,
+				 regs + port->din,	/* dat */
+				 regs + port->dout,	/* set */
+				 NULL,			/* clr */
+				 regs + port->oe,	/* dirout */
+				 NULL,			/* dirin */
+				 BGPIOF_UNREADABLE_REG_SET);
+		if (ret)
+			return ret;
+
+		bgc->gc.ngpio = port->ngpio;
+		bgc->gc.label = port->label;
+
+		bgc->gc.of_node = dev->of_node;
+		bgc->gc.of_gpio_n_cells = 3;
+		bgc->gc.of_xlate = etraxfs_gpio_of_xlate;
+
+		ret = gpiochip_add(&bgc->gc);
+		if (ret)
+			dev_err(dev, "Unable to register port %s\n",
+				bgc->gc.label);
+	}
+
+	return 0;
+}
+
+static struct platform_driver etraxfs_gpio_driver = {
+	.driver = {
+		.name		= "etraxfs-gpio",
+		.of_match_table = of_match_ptr(etraxfs_gpio_of_table),
+	},
+	.probe	= etraxfs_gpio_probe,
+};
+
+module_platform_driver(etraxfs_gpio_driver);
+
+MODULE_DESCRIPTION("ETRAX FS GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Paul Bolle]

On Sat, 2015-05-16 at 00:27 +0200, Rabin Vincent wrote:
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
 
> +config GPIO_ETRAXFS
> +	bool "Axis ETRAX FS General I/O"
> +	depends on CRIS || COMPILE_TEST
> +	depends on OF
> +	select GPIO_GENERIC
> +	help
> +	  Say yes here to support the GPIO controller on Axis ETRAX FS SoCs.

> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile

> +obj-$(CONFIG_GPIO_ETRAXFS)	+= gpio-etraxfs.o

GPIO_ETRAXFS is a bool symbol, so gpio-etraxfs.o can only be built-in,
right?

> --- /dev/null
> +++ b/drivers/gpio/gpio-etraxfs.c

> +MODULE_DEVICE_TABLE(of, etraxfs_gpio_of_table);

> +module_platform_driver(etraxfs_gpio_driver);

(A patch was submitted that would allow built-in only code to use
builtin_platform_driver(), see https://lkml.org/lkml/2015/5/10/125 .) 

> +MODULE_DESCRIPTION("ETRAX FS GPIO driver");
> +MODULE_LICENSE("GPL");

But the code this patch adds contains a bit of module specific
boilerplate. Was it perhaps your intention to make GPIO_ETRAXFS
tristate?


Paul Bolle

Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Rabin Vincent]

On Sat, May 16, 2015 at 03:59:27PM +0200, Paul Bolle wrote:
> On Sat, 2015-05-16 at 00:27 +0200, Rabin Vincent wrote:
> > +obj-$(CONFIG_GPIO_ETRAXFS)	+= gpio-etraxfs.o
> 
> GPIO_ETRAXFS is a bool symbol, so gpio-etraxfs.o can only be built-in,
> right?

Right.

> > --- /dev/null
> > +++ b/drivers/gpio/gpio-etraxfs.c
> 
> > +MODULE_DEVICE_TABLE(of, etraxfs_gpio_of_table);
> 
> > +module_platform_driver(etraxfs_gpio_driver);
> 
> (A patch was submitted that would allow built-in only code to use
> builtin_platform_driver(), see https://lkml.org/lkml/2015/5/10/125 .) 

Thanks, good to know.

> 
> > +MODULE_DESCRIPTION("ETRAX FS GPIO driver");
> > +MODULE_LICENSE("GPL");
> 
> But the code this patch adds contains a bit of module specific
> boilerplate. Was it perhaps your intention to make GPIO_ETRAXFS
> tristate?

No, the intention is to have it boolean as it is now.  I'll remove the
unnecessary lines.

Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Linus Walleij]

On Sat, May 16, 2015 at 12:27 AM, Rabin Vincent <rabin@rab.in> wrote:

> Add a GPIO driver for the General I/O block on Axis ETRAX FS SoCs.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Nice!

(...)
> +++ b/Documentation/devicetree/bindings/gpio/gpio-etraxfs.txt
> @@ -0,0 +1,21 @@
> +Axis ETRAX FS General I/O controller bindings
> +
> +Required properties:
> +
> +- compatible:
> +  - "axis,etraxfs-gio"
> +- reg: Physical base address and length of the controller's registers.
> +- #gpio-cells: Should be 3
> +  - The first cell is the port number (hex).
> +  - The seconds cell is the gpio offset number.
> +  - The third cell is reserved and is currently unused.
> +- gpio-controller: Marks the device node as a GPIO controller.
> +
> +Example:
> +
> +       gio: gpio@b001a000 {
> +               compatible = "axis,etraxfs-gio";
> +               reg = <0xb001a000 0x1000>;
> +               gpio-controller;
> +               #gpio-cells = <3>;
> +       };

Three cells is rather unusual, is it the best arrangement?

Usually it's just offset+flags (your flags are ununsed I see).
And then you could divide offset by num gpios per bank
(I guess 32) in the driver to get bank number.

The other obvious question is whether you considered the
design pattern of using one DT node per bank, so you
have offset 0..31 (I guess) on each device, simplifying things
with two cells.

The latter design pattern is usually recommended unless
there is a "strong" coupling between the banks, such as
if they all share the same IRQ line so they need the
same interrupt handler, or share other common registers.

> +config GPIO_ETRAXFS
> +       bool "Axis ETRAX FS General I/O"
> +       depends on CRIS || COMPILE_TEST
> +       depends on OF
> +       select GPIO_GENERIC

Very nice to have generic for this.

> +struct etraxfs_gpio_port {
> +       const char *label;
> +       unsigned int oe;
> +       unsigned int dout;
> +       unsigned int din;

consider using u32 for these.

> +       unsigned int ngpio;
> +};
> +
> +struct etraxfs_gpio_info {
> +       int num_ports;

unsigned?

> +       const struct etraxfs_gpio_port *ports;
> +};

Yours,
Linus Walleij

Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Rabin Vincent]

On Tue, May 19, 2015 at 11:39:01AM +0200, Linus Walleij wrote:
> On Sat, May 16, 2015 at 12:27 AM, Rabin Vincent <rabin-66gdRtMMWGc@public.gmane.org> wrote:
> > +Axis ETRAX FS General I/O controller bindings
> > +
> > +Required properties:
> > +
> > +- compatible:
> > +  - "axis,etraxfs-gio"
> > +- reg: Physical base address and length of the controller's registers.
> > +- #gpio-cells: Should be 3
> > +  - The first cell is the port number (hex).
> > +  - The seconds cell is the gpio offset number.
> > +  - The third cell is reserved and is currently unused.
> > +- gpio-controller: Marks the device node as a GPIO controller.
> > +
> > +Example:
> > +
> > +       gio: gpio@b001a000 {
> > +               compatible = "axis,etraxfs-gio";
> > +               reg = <0xb001a000 0x1000>;
> > +               gpio-controller;
> > +               #gpio-cells = <3>;
> > +       };
> 
> Three cells is rather unusual, is it the best arrangement?
> 
> Usually it's just offset+flags (your flags are ununsed I see).
> And then you could divide offset by num gpios per bank
> (I guess 32) in the driver to get bank number.

At least to me, this:

+       i2c {
+               compatible = "i2c-gpio";
+               gpios = <&gio 0xD 5 0>, <&gio 0xD 6 0>;
+               i2c-gpio,delay-us = <2>;

which immediately shows that it's port D pins 5 and 6 which are being used, and
which matches the naming in the schematics and the chip documentation is
clearly preferable to this:

+               gpios = <&gio 101 0>, <&gio 102 0>;

which uses made up numbers with no relation to any documentation and which
probably requires the use of a calculator to determine if the correct
pins are being used.

(btw, the ports have varying numbers of GPIOs and none of them have 32).

> 
> The other obvious question is whether you considered the
> design pattern of using one DT node per bank, so you
> have offset 0..31 (I guess) on each device, simplifying things
> with two cells.

Yes, but I did it the way I did for the reasons below.

> The latter design pattern is usually recommended unless
> there is a "strong" coupling between the banks, such as
> if they all share the same IRQ line so they need the
> same interrupt handler, or share other common registers.

The binding in the patch matches the hardware.  The hardware is
described as one IP with several ports and not several instances of the
same IP.  The registers are also just 3 per port in the same region.
Creating one instance of the device for handling each port, seems
like useless overhead at best and, because it doesn't even match how the
hardware looks like, quite wrong anyway.

Only port A has interrupt support; this is not implemented in the
current driver.

BTW, the documentation for the chip is available here (GIO starts at
page 647 and its registers at page 895):
http://www.axis.com/files/manuals/etrax_fs_des_ref-070821.pdf

> > +struct etraxfs_gpio_port {
> > +       const char *label;
> > +       unsigned int oe;
> > +       unsigned int dout;
> > +       unsigned int din;
> 
> consider using u32 for these.

Why?  These are just offsets to the base address so there's no reason
they _have_ to be 32 bits so u32 seems semantically wrong.

> > +       unsigned int ngpio;
> > +};
> > +
> > +struct etraxfs_gpio_info {
> > +       int num_ports;
> 
> unsigned?

OK.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Linus Walleij]

On Thu, May 21, 2015 at 8:48 PM, Rabin Vincent <rabin@rab.in> wrote:
> On Tue, May 19, 2015 at 11:39:01AM +0200, Linus Walleij wrote:

>> Three cells is rather unusual, is it the best arrangement?
>>
>> Usually it's just offset+flags (your flags are ununsed I see).
>> And then you could divide offset by num gpios per bank
>> (I guess 32) in the driver to get bank number.
>
> At least to me, this:
>
> +       i2c {
> +               compatible = "i2c-gpio";
> +               gpios = <&gio 0xD 5 0>, <&gio 0xD 6 0>;
> +               i2c-gpio,delay-us = <2>;
>
> which immediately shows that it's port D pins 5 and 6 which are being used, and
> which matches the naming in the schematics and the chip documentation is
> clearly preferable to this:
>
> +               gpios = <&gio 101 0>, <&gio 102 0>;
>
> which uses made up numbers with no relation to any documentation and which
> probably requires the use of a calculator to determine if the correct
> pins are being used.

Sure I buy this ... just need to push back some to not get too
many deviant DT bindings :/

There is also the code such as of_gpio_simple_xlate()
that can't be reused for this, so thus it needs its own xlate
function and adds some complexity to the code.
But the convention in of_gpio_simple_xlate() is that
cell 0 is offset, and cell 1 is flags, so what about
moving the bank number to the last argument so you
can still use of_gpio_simple_xlate()?

Like so:
gpios = <&gio 5 0 0xD>, <&gio 6 0 0xD>;

I.e. extra cells go at the end. I can see you have a check
hack to see it is hitting a valid GPIO chip by using the label,
but that doesn't seem totally necessary.

Maybe we should even document this as a preferred binding
for "one IP with many banks inside it" use cases so we can
use that going forward?

> (btw, the ports have varying numbers of GPIOs and none of them have 32).

How typical.

> The binding in the patch matches the hardware.  The hardware is
> described as one IP with several ports and not several instances of the
> same IP.  The registers are also just 3 per port in the same region.
> Creating one instance of the device for handling each port, seems
> like useless overhead at best and, because it doesn't even match how the
> hardware looks like, quite wrong anyway.

OK.

> Only port A has interrupt support; this is not implemented in the
> current driver.

OK.

> BTW, the documentation for the chip is available here (GIO starts at
> page 647 and its registers at page 895):
> http://www.axis.com/files/manuals/etrax_fs_des_ref-070821.pdf

Ah I see it has pin multiplexing in front of the GPIO controller
block too. The driver may need  pinctrl_request_gpio()/
pinctrl_free_gpio() etc the day there is a standard pin control
driver in the back end of it. But no hurry with that.

>> > +struct etraxfs_gpio_port {
>> > +       const char *label;
>> > +       unsigned int oe;
>> > +       unsigned int dout;
>> > +       unsigned int din;
>>
>> consider using u32 for these.
>
> Why?  These are just offsets to the base address so there's no reason
> they _have_ to be 32 bits so u32 seems semantically wrong.

Ah I thought it was register shadows or something,
offsets are OK. Sorry.

Yours,
Linus Walleij

Re: [PATCH] gpio: add ETRAXFS GPIO driver

[Rabin Vincent]

On Mon, Jun 01, 2015 at 03:45:35PM +0200, Linus Walleij wrote:
> There is also the code such as of_gpio_simple_xlate()
> that can't be reused for this, so thus it needs its own xlate
> function and adds some complexity to the code.
> But the convention in of_gpio_simple_xlate() is that
> cell 0 is offset, and cell 1 is flags, so what about
> moving the bank number to the last argument so you
> can still use of_gpio_simple_xlate()?
> 
> Like so:
> gpios = <&gio 5 0 0xD>, <&gio 6 0 0xD>;
> 
> I.e. extra cells go at the end. I can see you have a check
> hack to see it is hitting a valid GPIO chip by using the label,
> but that doesn't seem totally necessary.

That code is quite necessary, because that is what makes us bind to the
correct gpio chip.  The wrong gpio chips' xlate() return an error code
and the code in of_gpiochip_find_and_xlate() makes sure we look through
the rest of the chips in the OF node until we find the right one.

But the xlate function can change the order as you suggest and use
of_gpio_simple_xlate() as a helper.  I'll make it do that.