[PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver

[PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver

[Nandor Han]

The patchset will add a driver to support basic functionality for
XRA1403 device. Features supported:
	- configure gpin as input/out
	- get/set gpio status

Documentation: A gpio-xra1403.txt file was added to document the DTS
	bindings related to driver.

Testing:

1. XRA1403 connected to iMX53 MCU
2. Export gpio from userspace
3. Verify that corresponding gpio directories are created
   in `/sys/class/gpio/gpioXX`
4. Export gpios from first and second bank as output
5. Set the output gpio pin to high/low and verify with the 
   oscilloscope that gpio status is according with the configured
   value.

Nandor Han (3):
  gpio - Add EXAR XRA1403 SPI GPIO expander driver
  doc,dts - add XRA1403 DTS binding documentation
  Add XRA1403 support to MAINTAINERS file

 .../devicetree/bindings/gpio/gpio-xra1403.txt      |  37 +++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 MAINTAINERS                                        |   8 +
 drivers/gpio/Kconfig                               |   5 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-xra1403.c                        | 252 +++++++++++++++++++++
 6 files changed, 304 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
 create mode 100644 drivers/gpio/gpio-xra1403.c

-- 
2.10.1

[PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

[Nandor Han]

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>
---
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/gpio/Kconfig                               |   5 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-xra1403.c                        | 252 +++++++++++++++++++++
 4 files changed, 259 insertions(+)
 create mode 100644 drivers/gpio/gpio-xra1403.c

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 0ad67d5..7ca9d41 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -103,6 +103,7 @@ ettus	NI Ettus Research
 eukrea  Eukréa Electromatique
 everest	Everest Semiconductor Co. Ltd.
 everspin	Everspin Technologies, Inc.
+exar	Exar Corporation
 excito	Excito
 ezchip	EZchip Semiconductor
 faraday	Faraday Technology Corporation
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d6e3cfd..3a6c9a3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1208,6 +1208,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 bd995dc..8f50844 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -139,6 +139,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..1b7138a8
--- /dev/null
+++ b/drivers/gpio/gpio-xra1403.c
@@ -0,0 +1,252 @@
+/*
+ * 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>
+
+/* XRA1403 registers */
+#define XRA_GSR  0x00 /* GPIO State */
+#define XRA_OCR  0x02 /* Output Control */
+#define XRA_GCR  0x06 /* GPIO Configuration */
+
+/* SPI headers */
+#define XRA_READ 0x80 /* read bit of the SPI command byte */
+
+struct xra1403 {
+	struct mutex      lock;
+	struct gpio_chip  chip;
+	struct spi_device *spi;
+};
+
+static int xra1403_get_byte(struct xra1403 *xra, unsigned int addr)
+{
+	return spi_w8r8(xra->spi, XRA_READ | (addr << 1));
+}
+
+static int xra1403_get_bit(struct xra1403 *xra, unsigned int addr,
+			   unsigned int bit)
+{
+	int ret;
+
+	ret = xra1403_get_byte(xra, addr + (bit > 7));
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & BIT(bit % 8));
+}
+
+static int xra1403_set_bit(struct xra1403 *xra, unsigned int addr,
+			   unsigned int bit, int value)
+{
+	int ret;
+	u8 mask;
+	u8 tx[2];
+
+	addr += bit > 7;
+
+	mutex_lock(&xra->lock);
+
+	ret = xra1403_get_byte(xra, addr);
+	if (ret < 0)
+		goto out_unlock;
+
+	mask = BIT(bit % 8);
+	if (value)
+		value = ret | mask;
+	else
+		value = ret & ~mask;
+
+	if (value != ret) {
+		tx[0] = addr << 1;
+		tx[1] = value;
+		ret = spi_write(xra->spi, tx, sizeof(tx));
+	} else {
+		ret = 0;
+	}
+
+out_unlock:
+	mutex_unlock(&xra->lock);
+
+	return ret;
+}
+
+static int xra1403_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	return xra1403_set_bit(gpiochip_get_data(chip), XRA_GCR, offset, 1);
+}
+
+static int xra1403_direction_output(struct gpio_chip *chip, unsigned int offset,
+				    int value)
+{
+	int ret;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+
+	ret = xra1403_set_bit(xra, XRA_OCR, offset, value);
+	if (ret)
+		return ret;
+
+	ret = xra1403_set_bit(xra, XRA_GCR, offset, 0);
+
+	return ret;
+}
+
+static int xra1403_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return xra1403_get_bit(gpiochip_get_data(chip), XRA_GSR, offset);
+}
+
+static void xra1403_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	xra1403_set_bit(gpiochip_get_data(chip), XRA_OCR, offset, value);
+}
+
+#ifdef CONFIG_DEBUG_FS
+#define XRA_REGS 0x16
+static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	int reg;
+	struct xra1403 *xra = gpiochip_get_data(chip);
+	int value[XRA_REGS];
+	int i;
+	unsigned int gcr;
+	unsigned int gsr;
+
+	seq_puts(s, "xra reg:");
+	for (reg = 0; reg < XRA_REGS; reg++)
+		seq_printf(s, " %2.2x", reg);
+	seq_puts(s, "\n  value:");
+	for (reg = 0; reg < XRA_REGS; reg++) {
+		value[reg] = xra1403_get_byte(xra, 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;
+
+	xra = devm_kzalloc(&spi->dev, sizeof(*xra), GFP_KERNEL);
+	if (!xra)
+		return -ENOMEM;
+
+	/* bring the chip out of reset */
+	reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset_gpio))
+		dev_warn(&spi->dev, "could not get reset-gpios\n");
+	else if (reset_gpio)
+		gpiod_put(reset_gpio);
+
+	mutex_init(&xra->lock);
+
+	xra->chip.direction_input = xra1403_direction_input;
+	xra->chip.direction_output = xra1403_direction_output;
+	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->spi = spi;
+	spi_set_drvdata(spi, xra);
+
+	return gpiochip_add_data(&xra->chip, xra);
+}
+
+static int xra1403_remove(struct spi_device *spi)
+{
+	struct xra1403 *xra = spi_get_drvdata(spi);
+
+	gpiochip_remove(&xra->chip);
+
+	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,
+	.remove   = xra1403_remove,
+	.id_table = xra1403_ids,
+	.driver   = {
+		.name           = "xra1403",
+		.of_match_table = of_match_ptr(xra1403_spi_of_match),
+	},
+};
+
+static int __init xra1403_init(void)
+{
+	return spi_register_driver(&xra1403_driver);
+}
+
+/*
+ * register after spi postcore initcall and before
+ * subsys initcalls that may rely on these GPIOs
+ */
+subsys_initcall(xra1403_init);
+
+static void __exit xra1403_exit(void)
+{
+	spi_unregister_driver(&xra1403_driver);
+}
+module_exit(xra1403_exit);
+
+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");
-- 
2.10.1

Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

[Linus Walleij]

On Mon, Mar 27, 2017 at 8:23 AM, 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.

Use the gpio tools in tools/gpio, use the characcter device.
Do not use sysfs. Change this to reference the tools.

> 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>
(...)
> +exar   Exar Corporation

Send this as a separate patch to the DT bindings maintainer
(Rob Herring.)

> +static int xra1403_get_byte(struct xra1403 *xra, unsigned int addr)
> +{
> +       return spi_w8r8(xra->spi, XRA_READ | (addr << 1));
> +}
> +
> +static int xra1403_get_bit(struct xra1403 *xra, unsigned int addr,
> +                          unsigned int bit)
> +{
> +       int ret;
> +
> +       ret = xra1403_get_byte(xra, addr + (bit > 7));
> +       if (ret < 0)
> +               return ret;
> +
> +       return !!(ret & BIT(bit % 8));
> +}

This looks like it can use regmap-spi right off, do you agree?

git grep devm_regmap_init_spi
should give you some examples of how to use it.

If it's not off-the shelf regmap drivers like
drivers/iio/pressure/mpl115_spi.c
give examples of how to make more elaborate custom
SPI transfers with regmap.

> +static int xra1403_set_bit(struct xra1403 *xra, unsigned int addr,
> +                          unsigned int bit, int value)
> +{
> +       int ret;
> +       u8 mask;
> +       u8 tx[2];
> +
> +       addr += bit > 7;
> +
> +       mutex_lock(&xra->lock);
> +
> +       ret = xra1403_get_byte(xra, addr);
> +       if (ret < 0)
> +               goto out_unlock;
> +
> +       mask = BIT(bit % 8);
> +       if (value)
> +               value = ret | mask;
> +       else
> +               value = ret & ~mask;
> +
> +       if (value != ret) {
> +               tx[0] = addr << 1;
> +               tx[1] = value;
> +               ret = spi_write(xra->spi, tx, sizeof(tx));
> +       } else {
> +               ret = 0;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&xra->lock);
> +
> +       return ret;
> +}

Classical mask-and-set implementation right?
With regmap this becomes simply regmap_update_bits(map, addr, mask, set)

> +static int xra1403_probe(struct spi_device *spi)
> +{
> +       struct xra1403 *xra;
> +       struct gpio_desc *reset_gpio;
> +
> +       xra = devm_kzalloc(&spi->dev, sizeof(*xra), GFP_KERNEL);
> +       if (!xra)
> +               return -ENOMEM;
> +
> +       /* bring the chip out of reset */
> +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(reset_gpio))
> +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> +       else if (reset_gpio)
> +               gpiod_put(reset_gpio);

I don't think you should put it, other than in the remove()
function and in that case you need to have it in the
state container.

> +       mutex_init(&xra->lock);
> +
> +       xra->chip.direction_input = xra1403_direction_input;
> +       xra->chip.direction_output = xra1403_direction_output;

Please implement .get_direction(). This is very nice to have.

> +static int xra1403_remove(struct spi_device *spi)
> +{
> +       struct xra1403 *xra = spi_get_drvdata(spi);
> +
> +       gpiochip_remove(&xra->chip);

Use devm_gpiochip_add_data() and this remove is not
needed at all.

> +static int __init xra1403_init(void)
> +{
> +       return spi_register_driver(&xra1403_driver);
> +}
> +
> +/*
> + * register after spi postcore initcall and before
> + * subsys initcalls that may rely on these GPIOs
> + */
> +subsys_initcall(xra1403_init);
> +
> +static void __exit xra1403_exit(void)
> +{
> +       spi_unregister_driver(&xra1403_driver);
> +}
> +module_exit(xra1403_exit);

This seems like tricksy. Just module_spi_driver()
should be fine don't you think?

Yours,
Linus Walleij

[PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 29 March 2017 05:07
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: 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 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
> 
> On Mon, Mar 27, 2017 at 8:23 AM, 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.
> 
> Use the gpio tools in tools/gpio, use the characcter device.
> Do not use sysfs. Change this to reference the tools.
> 
> > 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>
> (...)
> > +exar   Exar Corporation
> 
> Send this as a separate patch to the DT bindings maintainer
> (Rob Herring.)
> 

OK. I will create a separate patch with this one. 
I guess is not an issue to send all the patches to Rob as well.

<snip>

> > +
> > +       ret = xra1403_get_byte(xra, addr + (bit > 7));
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return !!(ret & BIT(bit % 8));
> > +}
> 
> This looks like it can use regmap-spi right off, do you agree?
> 

Yes. Using regmap-spi will definitely improve the code readability and reduce boilerplate.
Done.

> git grep devm_regmap_init_spi
> should give you some examples of how to use it.
> 
> If it's not off-the shelf regmap drivers like
> drivers/iio/pressure/mpl115_spi.c
> give examples of how to make more elaborate custom
> SPI transfers with regmap.
> 

Thanks, I did check other drivers as examples. 
Not that I needed for this driver, but ...mpl115_spi.c doesn't seem to
use regmap (checked on next-20170327)

<snip>

> > +
> > +       if (value != ret) {
> > +               tx[0] = addr << 1;
> > +               tx[1] = value;
> > +               ret = spi_write(xra->spi, tx, sizeof(tx));
> > +       } else {
> > +               ret = 0;
> > +       }
> > +
> > +out_unlock:
> > +       mutex_unlock(&xra->lock);
> > +
> > +       return ret;
> > +}
> 
> Classical mask-and-set implementation right?
> With regmap this becomes simply regmap_update_bits(map, addr, mask, set)
> 

True. :)

<snip>

> > +       /* bring the chip out of reset */
> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> > +       if (IS_ERR(reset_gpio))
> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> > +       else if (reset_gpio)
> > +               gpiod_put(reset_gpio);
> 
> I don't think you should put it, other than in the remove()
> function and in that case you need to have it in the
> state container.

Can you please be more explicit here.

Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
I don't see how this could be done in remove()  :)

> 
> > +       mutex_init(&xra->lock);
> > +
> > +       xra->chip.direction_input = xra1403_direction_input;
> > +       xra->chip.direction_output = xra1403_direction_output;
> 
> Please implement .get_direction(). This is very nice to have.
> 

Done

> > +static int xra1403_remove(struct spi_device *spi)
> > +{
> > +       struct xra1403 *xra = spi_get_drvdata(spi);
> > +
> > +       gpiochip_remove(&xra->chip);
> 
> Use devm_gpiochip_add_data() and this remove is not
> needed at all.
> 

True. Done

<snip>

> > +subsys_initcall(xra1403_init);
> > +
> > +static void __exit xra1403_exit(void)
> > +{
> > +       spi_unregister_driver(&xra1403_driver);
> > +}
> > +module_exit(xra1403_exit);
> 
> This seems like tricksy. Just module_spi_driver()
> should be fine don't you think?

Yeah. TBH I don't have a strong reason why module_spi_driver init level shouldn't be enough. 
Done.

Regards,
    Nandor

Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

[Linus Walleij]

On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
<nandor.han@ge.com> wrote:
> [Me]
>> > +       /* bring the chip out of reset */
>> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
>> > +       if (IS_ERR(reset_gpio))
>> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
>> > +       else if (reset_gpio)
>> > +               gpiod_put(reset_gpio);
>>
>> I don't think you should put it, other than in the remove()
>> function and in that case you need to have it in the
>> state container.
>
> Can you please be more explicit here.
>
> Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> I don't see how this could be done in remove()  :)

If you issue gpiod_put() you release the GPIO hande so something else
can go in and grab the GPIO and assert the reset.

This is not what you want to make possible: you want to hold this gpiod handle
as long as the driver is running. devm_gpiod_get_optional() will do the
trick if you don't want to put the handle under explicit control.

Yours,
Linus Walleij

[PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 07 April 2017 13:07
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: 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 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
> 
> On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
> <nandor.han@ge.com> wrote:
> > [Me]
> >> > +       /* bring the chip out of reset */
> >> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> >> > +       if (IS_ERR(reset_gpio))
> >> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> >> > +       else if (reset_gpio)
> >> > +               gpiod_put(reset_gpio);
> >>
> >> I don't think you should put it, other than in the remove()
> >> function and in that case you need to have it in the
> >> state container.
> >
> > Can you please be more explicit here.
> >
> > Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> > I don't see how this could be done in remove()  :)
> 
> If you issue gpiod_put() you release the GPIO hande so something else
> can go in and grab the GPIO and assert the reset.
> 
> This is not what you want to make possible: you want to hold this gpiod handle
> as long as the driver is running. devm_gpiod_get_optional() will do the
> trick if you don't want to put the handle under explicit control.
> 

That was my first intention to release the reset line in case somebody else wants to control it. I did it
like that because usually reset line controls multiple devices and probably some upper layer wants to
control that. 

After your comment I did some analysing and I will follow your advice and change the reset line handling. 
Once the GPIO is provided to the driver the driver will own it and bring out the device from reset. In case not
provided the reset line is somebody else responsibility. 

This way we are able to cover multiple use-cases. 

Thanks Linus,
Nandy

> Yours,
> Linus Walleij

Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

[Rob Herring]

On Mon, Mar 27, 2017 at 09:23:00AM +0300, Nandor Han 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>
> ---
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +

This should go with the binding patch or by itself.

>  drivers/gpio/Kconfig                               |   5 +
>  drivers/gpio/Makefile                              |   1 +
>  drivers/gpio/gpio-xra1403.c                        | 252 +++++++++++++++++++++
>  4 files changed, 259 insertions(+)
>  create mode 100644 drivers/gpio/gpio-xra1403.c

[PATCH 2/3] doc,dts - add XRA1403 DTS binding documentation

[Nandor Han]

Add the XRA1403 DTS binding documentation.

Signed-off-by: Nandor Han <nandor.han@ge.com>
---
 .../devicetree/bindings/gpio/gpio-xra1403.txt      | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
new file mode 100644
index 0000000..ccf5337
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
@@ -0,0 +1,37 @@
+GPIO Driver for XRA1403 16-BIT GPIO Expander With Reset Input from EXAR
+
+The XRA1403 is an 16-bit GPIO expander with an SPI interface. Features available:
+	- Individually programmable inputs:
+		- Internal pull-up resistors
+		- Polarity inversion
+		- Individual interrupt enable
+		- Rising edge and/or Falling edge interrupt
+		- Input filter
+	- Individually programmable outputs
+		- Output Level Control
+		- Output Three-State Control
+
+Properties
+----------
+Check documentation for SPI and GPIO controllers regarding properties needed to configure the node.
+
+	- compatible = "exar,xra1403".
+	- reg = SPI id of the device.
+	- gpio-controller: mark the node as gpio.
+
+Optional properties:
+-------------------
+	- reset-gpios: in case available used to control the device reset line.
+
+Example
+--------
+
+	gpioxra0: gpio@2 {
+		compatible = "exar,xra1403";
+		reg = <2>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+		spi-max-frequency = <1000000>;
+		status = "okay";
+	};
-- 
2.10.1

Re: [PATCH 2/3] doc,dts - add XRA1403 DTS binding documentation

[Linus Walleij]

On Mon, Mar 27, 2017 at 8:23 AM, Nandor Han <nandor.han@ge.com> wrote:

> Add the XRA1403 DTS binding documentation.
>
> Signed-off-by: Nandor Han <nandor.han@ge.com>

There is no big problem with this but:

> +The XRA1403 is an 16-bit GPIO expander with an SPI interface. Features available:
> +       - Individually programmable inputs:
> +               - Internal pull-up resistors
> +               - Polarity inversion
> +               - Individual interrupt enable
> +               - Rising edge and/or Falling edge interrupt
> +               - Input filter

Since you mention that it has interrupts maybe you want to add bindings
for the cascaded interrupt and the interrupt-controller; keyword etc
already now.

We just document what the hardware can do, we don't have to do all
of it in the first Linux driver submission.

Yours,
Linus Walleij

Re: [PATCH 2/3] doc,dts - add XRA1403 DTS binding documentation

[Rob Herring]

On Mon, Mar 27, 2017 at 09:23:01AM +0300, Nandor Han wrote:
> Add the XRA1403 DTS binding documentation.

"dt-bindings: gpio: ..." for the subject prefix please.

> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> ---
>  .../devicetree/bindings/gpio/gpio-xra1403.txt      | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
> new file mode 100644
> index 0000000..ccf5337
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
> @@ -0,0 +1,37 @@
> +GPIO Driver for XRA1403 16-BIT GPIO Expander With Reset Input from EXAR
> +
> +The XRA1403 is an 16-bit GPIO expander with an SPI interface. Features available:
> +	- Individually programmable inputs:
> +		- Internal pull-up resistors
> +		- Polarity inversion
> +		- Individual interrupt enable
> +		- Rising edge and/or Falling edge interrupt
> +		- Input filter
> +	- Individually programmable outputs
> +		- Output Level Control
> +		- Output Three-State Control
> +
> +Properties
> +----------
> +Check documentation for SPI and GPIO controllers regarding properties needed to configure the node.
> +
> +	- compatible = "exar,xra1403".
> +	- reg = SPI id of the device.
> +	- gpio-controller: mark the node as gpio.

#gpio-cells?

> +
> +Optional properties:
> +-------------------
> +	- reset-gpios: in case available used to control the device reset line.
> +
> +Example
> +--------
> +
> +	gpioxra0: gpio@2 {
> +		compatible = "exar,xra1403";
> +		reg = <2>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
> +		spi-max-frequency = <1000000>;
> +		status = "okay";

Don't show status in examples.

> +	};
> -- 
> 2.10.1
> 

[PATCH 3/3] Add XRA1403 support to MAINTAINERS file

[Nandor Han]

Add the XRA1403 support to MAINTAINERS list.

Signed-off-by: Nandor Han <nandor.han@ge.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 58b3a22..539c88c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13903,6 +13903,14 @@ L:	linux-kernel@vger.kernel.org
 S:	Supported
 F:	drivers/char/xillybus/
 
+XRA1403 GPIO EXPANDER
+M:	Nandor Han <nandor.han@ge.com>
+M:	Semi Malinen <semi.malinen@ge.com>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	drivers/gpio/gpio-xra1403.c
+F:	Documentation/devicetree/bindings/gpio/gpio-xra1403.txt
+
 XTENSA XTFPGA PLATFORM SUPPORT
 M:	Max Filippov <jcmvbkbc@gmail.com>
 L:	linux-xtensa@linux-xtensa.org
-- 
2.10.1

Re: [PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver

[Linus Walleij]

On Mon, Mar 27, 2017 at 8:22 AM, Nandor Han <nandor.han@ge.com> wrote:

> Testing:
>
> 1. XRA1403 connected to iMX53 MCU
> 2. Export gpio from userspace
> 3. Verify that corresponding gpio directories are created
>    in `/sys/class/gpio/gpioXX`
> 4. Export gpios from first and second bank as output
> 5. Set the output gpio pin to high/low and verify with the
>    oscilloscope that gpio status is according with the configured
>    value.

Do *NOT* use the sysfs for testing GPIO.
This is being phased out.

Use the tools in tools/gpio/* so that you exercise the
character device instead of the old deprecated ABI.

Yours,
Linus Walleij

[PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 29 March 2017 04:51
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: 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
> Subject: EXT: Re: [PATCH 0/3] XRA1403,gpio - add XRA1403 gpio expander driver
> 

<snip>

> Do *NOT* use the sysfs for testing GPIO.
> This is being phased out.
> 
> Use the tools in tools/gpio/* so that you exercise the
> character device instead of the old deprecated ABI.
> 

Thanks Linus, really good and  helpful comments.
I agree that make more sense to test with tools/gpio/*.

> Yours,
> Linus Walleij