[v2,2/4] gpio - Add EXAR XRA1403 SPI GPIO expander driver

Submitted by Nandor Han on April 13, 2017, 10:27 a.m.

Details

Message ID f28eef264102786357ac5ed7205813619c508fb4.1492077070.git.nandor.han@ge.com
State New
Headers show

Commit Message

Nandor Han April 13, 2017, 10:27 a.m.
This is a simple driver that provides a /sys/class/gpio
interface for controlling and configuring the GPIO lines.
It does not provide support for chip select or interrupts.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Semi Malinen <semi.malinen@ge.com>
---
 drivers/gpio/Kconfig        |   5 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-xra1403.c | 236 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 242 insertions(+)
 create mode 100644 drivers/gpio/gpio-xra1403.c

Comments

kbuild test robot April 13, 2017, 4:22 p.m.
Hi Nandor,

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.11-rc6 next-20170413]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nandor-Han/XRA1403-gpio-add-XRA1403-gpio-expander-driver/20170413-215739
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: frv-allyesconfig (attached as .config)
compiler: frv-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=frv 

All errors (new ones prefixed by >>):

   drivers//gpio/gpio-xra1403.c: In function 'xra1403_dbg_show':
>> drivers//gpio/gpio-xra1403.c:133:2: error: implicit declaration of function 'seq_puts' [-Werror=implicit-function-declaration]
     seq_puts(s, "xra reg:");
     ^~~~~~~~
>> drivers//gpio/gpio-xra1403.c:135:3: error: implicit declaration of function 'seq_printf' [-Werror=implicit-function-declaration]
      seq_printf(s, " %2.2x", reg);
      ^~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/seq_puts +133 drivers//gpio/gpio-xra1403.c

   127		struct xra1403 *xra = gpiochip_get_data(chip);
   128		int value[xra1403_regmap_cfg.max_register];
   129		int i;
   130		unsigned int gcr;
   131		unsigned int gsr;
   132	
 > 133		seq_puts(s, "xra reg:");
   134		for (reg = 0; reg <= xra1403_regmap_cfg.max_register; reg++)
 > 135			seq_printf(s, " %2.2x", reg);
   136		seq_puts(s, "\n  value:");
   137		for (reg = 0; reg < xra1403_regmap_cfg.max_register; reg++) {
   138			regmap_read(xra->regmap, reg, &value[reg]);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Linus Walleij April 24, 2017, 1:47 p.m.
On Thu, Apr 13, 2017 at 12:27 PM, Nandor Han <nandor.han@ge.com> wrote:

> This is a simple driver that provides a /sys/class/gpio
> interface for controlling and configuring the GPIO lines.
> It does not provide support for chip select or interrupts.
>
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Semi Malinen <semi.malinen@ge.com>

I almost want to make the driver depend on !GPIO_SYSFS because
of this commit message.

DO NOT USE OR ENCOURAGE THE USE OF THE GPIO SYSFS
INTERFACE.

Use the character device.

Use the example in tools/gpio/* as a guideline and testbed.

Use libgpiod as a rich userspace.

And the commit message should state that this is a driver
for such and such Exar hardware instead.

Thanks.

> +#include <linux/bitops.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>

You are missing
#include <linux/seq_file.h>

and that is why the build robot is complaining.

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
Nandor Han April 24, 2017, 1:53 p.m.
> -----Original Message-----

> From: Linus Walleij [mailto:linus.walleij@linaro.org]

> Sent: 24 April 2017 16:47

> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>

> Cc: Greg KH <gregkh@linuxfoundation.org>; David S. Miller <davem@davemloft.net>; Geert Uytterhoeven <geert@linux-

> m68k.org>; Mauro Carvalho Chehab <mchehab@kernel.org>; Daniel Vetter <daniel.vetter@ffwll.ch>; Alexandre Courbot

> <gnurou@gmail.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; linux-

> gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Malinen, Semi (GE Healthcare)

> <semi.malinen@ge.com>

> Subject: EXT: Re: [PATCH v2 2/4] gpio - Add EXAR XRA1403 SPI GPIO expander driver

> 

> On Thu, Apr 13, 2017 at 12:27 PM, Nandor Han <nandor.han@ge.com> wrote:

> 

> > This is a simple driver that provides a /sys/class/gpio

> > interface for controlling and configuring the GPIO lines.

> > It does not provide support for chip select or interrupts.

> >

> > Signed-off-by: Nandor Han <nandor.han@ge.com>

> > Signed-off-by: Semi Malinen <semi.malinen@ge.com>

> 

> I almost want to make the driver depend on !GPIO_SYSFS because

> of this commit message.

> 

> DO NOT USE OR ENCOURAGE THE USE OF THE GPIO SYSFS

> INTERFACE.

> 


Ahh.. I forgot to change this commit message. Sorry I will change it ASAP.

> Use the character device.

> 

> Use the example in tools/gpio/* as a guideline and testbed.

> 

> Use libgpiod as a rich userspace.

> 

> And the commit message should state that this is a driver

> for such and such Exar hardware instead.

> 

> Thanks.

> 

> > +#include <linux/bitops.h>

> > +#include <linux/gpio/driver.h>

> > +#include <linux/kernel.h>

> > +#include <linux/module.h>

> > +#include <linux/mutex.h>

> > +#include <linux/of_device.h>

> > +#include <linux/of_gpio.h>

> > +#include <linux/spi/spi.h>

> > +#include <linux/regmap.h>

> 

> You are missing

> #include <linux/seq_file.h>

> 

> and that is why the build robot is complaining.

> 


I've noticed that. I will change it in the next rev.

> Yours,

> Linus Walleij


Thanks,
   Nandy
Benjamin Henrion April 25, 2017, 7:07 a.m.
On Mon, Apr 24, 2017 at 3:47 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Apr 13, 2017 at 12:27 PM, Nandor Han <nandor.han@ge.com> wrote:
>
>> This is a simple driver that provides a /sys/class/gpio
>> interface for controlling and configuring the GPIO lines.
>> It does not provide support for chip select or interrupts.
>>
>> Signed-off-by: Nandor Han <nandor.han@ge.com>
>> Signed-off-by: Semi Malinen <semi.malinen@ge.com>
>
> I almost want to make the driver depend on !GPIO_SYSFS because
> of this commit message.
>
> DO NOT USE OR ENCOURAGE THE USE OF THE GPIO SYSFS
> INTERFACE.
>
> Use the character device.

I doubt you will be able to convince the majority of people toggling
GPIOs via a simple shell script to switch to write a complex C
program. Not to mention cross compilation and the libraries
dependencies here.

Is there some good cli tools to access the new char device? If they
are shipped with most distros, that would reduce the pain.

Best,

--
Benjamin Henrion <bhenrion at ffii.org>
FFII Brussels - +32-484-566109 - +32-2-3500762
"In July 2005, after several failed attempts to legalise software
patents in Europe, the patent establishment changed its strategy.
Instead of explicitly seeking to sanction the patentability of
software, they are now seeking to create a central European patent
court, which would establish and enforce patentability rules in their
favor, without any possibility of correction by competing courts or
democratically elected legislators."
--
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
Geert Uytterhoeven April 25, 2017, 7:15 a.m.
Hi Benjamin,

On Tue, Apr 25, 2017 at 9:07 AM, Benjamin Henrion <zoobab@gmail.com> wrote:
> On Mon, Apr 24, 2017 at 3:47 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Apr 13, 2017 at 12:27 PM, Nandor Han <nandor.han@ge.com> wrote:
>>
>>> This is a simple driver that provides a /sys/class/gpio
>>> interface for controlling and configuring the GPIO lines.
>>> It does not provide support for chip select or interrupts.
>>>
>>> Signed-off-by: Nandor Han <nandor.han@ge.com>
>>> Signed-off-by: Semi Malinen <semi.malinen@ge.com>
>>
>> I almost want to make the driver depend on !GPIO_SYSFS because
>> of this commit message.
>>
>> DO NOT USE OR ENCOURAGE THE USE OF THE GPIO SYSFS
>> INTERFACE.
>>
>> Use the character device.
>
> I doubt you will be able to convince the majority of people toggling
> GPIOs via a simple shell script to switch to write a complex C
> program. Not to mention cross compilation and the libraries
> dependencies here.
>
> Is there some good cli tools to access the new char device? If they
> are shipped with most distros, that would reduce the pain.

https://github.com/brgl/libgpiod

A bit early to expect it to be shipped with all distros, though.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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
Linus Walleij April 26, 2017, 2:42 p.m.
On Tue, Apr 25, 2017 at 9:07 AM, Benjamin Henrion <zoobab@gmail.com> wrote:

> I doubt you will be able to convince the majority of people toggling
> GPIOs via a simple shell script to switch to write a complex C
> program. Not to mention cross compilation and the libraries
> dependencies here.

I do not need to convince anyone, I'm not into politics.

The way to attract users to the character device is by offering better
features... so we smack in the following goodies:

- Need a userspace ABI? No more need to select CONFIG_GPIO_SYSFS!
  The chardev is always there for any gpio chip in newer kernels!
  Board vendors would have to actively delete core code to disable it!

- Need open drain? The chardev will support that, the sysfs will never.

- Need to set/get multiple lines with a single context switch? Chardev
  does this. Also the set operation will turn into a single register write
  if your driver implements .set_multiple()

- All future needs: line biasing? Schmitt triggers? Drive strengths?
  All that will use the character device, and the sysfs ABI will never
  support any of it.

> Is there some good cli tools to access the new char device? If they
> are shipped with most distros, that would reduce the pain.

I have those that come with the kernel:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/gpio

Then libgpiod as mentioned:
https://github.com/brgl/libgpiod/tree/master/src/tools

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
Andy Shevchenko April 26, 2017, 5:50 p.m.
On Tue, Apr 25, 2017 at 10:15 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

>> Is there some good cli tools to access the new char device? If they
>> are shipped with most distros, that would reduce the pain.
>
> https://github.com/brgl/libgpiod
>
> A bit early to expect it to be shipped with all distros, though.

Buildroot has it.
For kernel development it is quite enough.

Patch hide | download patch | download mbox

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 63ceed2..53cbe9b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1214,6 +1214,11 @@  config GPIO_PISOSR
 	  GPIO driver for SPI compatible parallel-in/serial-out shift
 	  registers. These are input only devices.
 
+config GPIO_XRA1403
+	tristate "EXAR XRA1403 16-bit GPIO expander"
+	help
+	  GPIO driver for EXAR XRA1403 16-bit SPI-based GPIO expander.
+
 endmenu
 
 menu "SPI or I2C GPIO expanders"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 095598e..76dc3d7 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -140,6 +140,7 @@  obj-$(CONFIG_GPIO_XGENE)	+= gpio-xgene.o
 obj-$(CONFIG_GPIO_XGENE_SB)	+= gpio-xgene-sb.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
 obj-$(CONFIG_GPIO_XLP)		+= gpio-xlp.o
+obj-$(CONFIG_GPIO_XRA1403)	+= gpio-xra1403.o
 obj-$(CONFIG_GPIO_XTENSA)	+= gpio-xtensa.o
 obj-$(CONFIG_GPIO_ZEVIO)	+= gpio-zevio.o
 obj-$(CONFIG_GPIO_ZYNQ)		+= gpio-zynq.o
diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c
new file mode 100644
index 0000000..e6966d9
--- /dev/null
+++ b/drivers/gpio/gpio-xra1403.c
@@ -0,0 +1,236 @@ 
+/*
+ * GPIO driver for EXAR XRA1403 16-bit GPIO expander
+ *
+ * Copyright (c) 2017, General Electric Company
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/regmap.h>
+
+/* XRA1403 registers */
+#define XRA_GSR   0x00 /* GPIO State */
+#define XRA_OCR   0x02 /* Output Control */
+#define XRA_PIR   0x04 /* Input Polarity Inversion */
+#define XRA_GCR   0x06 /* GPIO Configuration */
+#define XRA_PUR   0x08 /* Input Internal Pull-up Resistor Enable/Disable */
+#define XRA_IER   0x0A /* Input Interrupt Enable */
+#define XRA_TSCR  0x0C /* Output Three-State Control */
+#define XRA_ISR   0x0E /* Input Interrupt Status */
+#define XRA_REIR  0x10 /* Input Rising Edge Interrupt Enable */
+#define XRA_FEIR  0x12 /* Input Falling Edge Interrupt Enable */
+#define XRA_IFR   0x14 /* Input Filter Enable/Disable */
+
+struct xra1403 {
+	struct gpio_chip  chip;
+	struct regmap     *regmap;
+};
+
+static const struct regmap_config xra1403_regmap_cfg = {
+		.reg_bits = 7,
+		.pad_bits = 1,
+		.val_bits = 8,
+
+		.max_register = XRA_IFR | 0x01,
+};
+
+static unsigned int to_reg(unsigned int reg, unsigned int offset)
+{
+	return reg + (offset > 7);
+}
+
+static int xra1403_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct xra1403 *xra = gpiochip_get_data(chip);
+
+	return regmap_update_bits(xra->regmap, to_reg(XRA_GCR, offset),
+			BIT(offset % 8), BIT(offset % 8));
+}
+
+static int xra1403_direction_output(struct gpio_chip *chip, unsigned int offset,
+				    int value)
+{
+	int ret;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+
+	ret = regmap_update_bits(xra->regmap, to_reg(XRA_GCR, offset),
+			BIT(offset % 8), 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(xra->regmap, to_reg(XRA_OCR, offset),
+			BIT(offset % 8), value ? BIT(offset % 8) : 0);
+
+	return ret;
+}
+
+static int xra1403_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret;
+	unsigned int val;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+
+	ret = regmap_read(xra->regmap, to_reg(XRA_GCR, offset), &val);
+	if (ret)
+		return ret;
+
+	return !!(val & BIT(offset % 8));
+}
+
+static int xra1403_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret;
+	unsigned int val;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+
+	ret = regmap_read(xra->regmap, to_reg(XRA_GSR, offset), &val);
+	if (ret)
+		return ret;
+
+	return !!(val & BIT(offset % 8));
+}
+
+static void xra1403_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	int ret;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+
+	ret = regmap_update_bits(xra->regmap, to_reg(XRA_OCR, offset),
+			BIT(offset % 8), value ? BIT(offset % 8) : 0);
+	if (ret)
+		dev_err(chip->parent, "Failed to set pin: %d, ret: %d\n",
+				offset, ret);
+}
+
+#ifdef CONFIG_DEBUG_FS
+static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	int reg;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+	int value[xra1403_regmap_cfg.max_register];
+	int i;
+	unsigned int gcr;
+	unsigned int gsr;
+
+	seq_puts(s, "xra reg:");
+	for (reg = 0; reg <= xra1403_regmap_cfg.max_register; reg++)
+		seq_printf(s, " %2.2x", reg);
+	seq_puts(s, "\n  value:");
+	for (reg = 0; reg < xra1403_regmap_cfg.max_register; reg++) {
+		regmap_read(xra->regmap, reg, &value[reg]);
+		seq_printf(s, " %2.2x", value[reg]);
+	}
+	seq_puts(s, "\n");
+
+	gcr = value[XRA_GCR + 1] << 8 | value[XRA_GCR];
+	gsr = value[XRA_GSR + 1] << 8 | value[XRA_GSR];
+	for (i = 0; i < chip->ngpio; i++) {
+		const char *label = gpiochip_is_requested(chip, i);
+
+		if (!label)
+			continue;
+
+		seq_printf(s, " gpio-%-3d (%-12s) %s %s\n",
+			   chip->base + i, label,
+			   (gcr & BIT(i)) ? "in" : "out",
+			   (gsr & BIT(i)) ? "hi" : "lo");
+	}
+}
+#else
+#define xra1403_dbg_show NULL
+#endif
+
+static int xra1403_probe(struct spi_device *spi)
+{
+	struct xra1403 *xra;
+	struct gpio_desc *reset_gpio;
+	int ret;
+
+	xra = devm_kzalloc(&spi->dev, sizeof(*xra), GFP_KERNEL);
+	if (!xra)
+		return -ENOMEM;
+
+	/* bring the chip out of reset if reset pin is provided*/
+	reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset_gpio))
+		dev_warn(&spi->dev, "Could not get reset-gpios\n");
+
+	xra->chip.direction_input = xra1403_direction_input;
+	xra->chip.direction_output = xra1403_direction_output;
+	xra->chip.get_direction = xra1403_get_direction;
+	xra->chip.get = xra1403_get;
+	xra->chip.set = xra1403_set;
+
+	xra->chip.dbg_show = xra1403_dbg_show;
+
+	xra->chip.ngpio = 16;
+	xra->chip.label = "xra1403";
+
+	xra->chip.base = -1;
+	xra->chip.can_sleep = true;
+	xra->chip.parent = &spi->dev;
+	xra->chip.owner = THIS_MODULE;
+
+	xra->regmap = devm_regmap_init_spi(spi, &xra1403_regmap_cfg);
+	if (IS_ERR(xra->regmap)) {
+		ret = PTR_ERR(xra->regmap);
+		dev_err(&spi->dev, "Failed to allocate regmap: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&spi->dev, &xra->chip, xra);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Unable to register gpiochip\n");
+		return ret;
+	}
+
+	spi_set_drvdata(spi, xra);
+
+	return 0;
+}
+
+static const struct spi_device_id xra1403_ids[] = {
+	{ "xra1403" },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, xra1403_ids);
+
+static const struct of_device_id xra1403_spi_of_match[] = {
+	{ .compatible = "exar,xra1403" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xra1403_spi_of_match);
+
+static struct spi_driver xra1403_driver = {
+	.probe    = xra1403_probe,
+	.id_table = xra1403_ids,
+	.driver   = {
+		.name           = "xra1403",
+		.of_match_table = of_match_ptr(xra1403_spi_of_match),
+	},
+};
+
+module_spi_driver(xra1403_driver);
+
+MODULE_AUTHOR("Nandor Han <nandor.han@ge.com>");
+MODULE_AUTHOR("Semi Malinen <semi.malinen@ge.com>");
+MODULE_DESCRIPTION("GPIO expander driver for EXAR XRA1403");
+MODULE_LICENSE("GPL v2");