[PATCH 0/6] of, of_gpio, of_spi: Fix and improve of_parse_phandle_with_args, of_gpio_named_count and of_spi_register_master

[PATCH 0/6] of, of_gpio, of_spi: Fix and improve of_parse_phandle_with_args, of_gpio_named_count and of_spi_register_master

[Andreas Larsson]

This patch series fixes a bug where of_gpio_named count relied upon a return
value that was no longer returned from of_parse_phandle_with_args and adds the
possibility for of_gpio_named_count to return error values.

In addition, for of_spi_register_master it fixes a bug, adds documentation,
adds fetching of gpio flags and initializes gpio values to be consistent with
return values from of_parse_phandle_with_args.

Tested on sparc. Build tested on x86, arm and ppc.

Andreas Larsson (6):
  of: Return -EEXIST from of_parse_phandle_with_args for holes in
    phandle list
  of: Return -ENXIO from of_parse_phandle_with_args for too large index
    and return errors from of_gpio_named_count
  of_spi: Initialize cs_gpios properly
  of_spi: Document cs_gpios and cs_gpio in kernel-doc
  of_spi: Add fetching of of_gpio flags to of_spi_register_master
  of_spi: Initialize cs_gpios and cs_gpio with -EEXIST

 Documentation/devicetree/bindings/spi/spi-bus.txt |    3 +-
 drivers/gpio/gpiolib-of.c                         |    8 +++--
 drivers/hwmon/gpio-fan.c                          |    2 +-
 drivers/of/base.c                                 |    9 +++--
 drivers/of/selftest.c                             |    2 +-
 drivers/spi/spi.c                                 |   40 ++++++++++++++++-----
 include/linux/spi/spi.h                           |   10 +++++
 7 files changed, 56 insertions(+), 18 deletions(-)

[PATCH 1/6] of: Return -EEXIST from of_parse_phandle_with_args for holes in phandle list

[Andreas Larsson]

Return value for an empty phandle was -EEXIST before commit 15c9a0ac, that
changed the return value in this case to -ENOENT. However, of_gpio_named_count
relies upon the return value to be -EEXIST and relies upon being able to
distinguish this case from the case of no list at all which also returns
-ENOENT.

Also change the of selftest to expect -EEXIST in this case.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---

I have not run-tested the selftest, not having appropriate hardware around for that.

 drivers/of/base.c     |    4 ++--
 drivers/of/selftest.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2390ddb..986afd7 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1083,11 +1083,11 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 		 * All of the error cases above bail out of the loop, so at
 		 * this point, the parsing is successful. If the requested
 		 * index matches, then fill the out_args structure and return,
-		 * or return -ENOENT for an empty entry.
+		 * or return -EEXIST for an empty entry.
 		 */
 		if (cur_index == index) {
 			if (!phandle)
-				return -ENOENT;
+				return -EEXIST;
 
 			if (out_args) {
 				int i;
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index f24ffd7..b1c2ae9 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -54,7 +54,7 @@ static void __init of_selftest_parse_phandle_with_args(void)
 			passed &= (args.args[1] == 0);
 			break;
 		case 2:
-			passed &= (rc == -ENOENT);
+			passed &= (rc == -EEXIST);
 			break;
 		case 3:
 			passed &= !rc;
-- 
1.7.0.4

[PATCH 2/6] of: Return -ENXIO from of_parse_phandle_with_args for too large index and return errors from of_gpio_named_count

[Andreas Larsson]

This lets of_gpio_named_count return an errno on errors by being able to
distinguish between reaching the end of the phandle list and getting some other
error from of_parse_phandle_with_args.

Return error from of_spi_register_master when there is an "cs-gpios" list for
which gp_gpio_named_count fails.

Adjust drivers/hwmon/gpio-fan.c to cope with error return.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---

Uses -ENXIO modelled after uses such as in platform_get_irq, but maybe some
other errno than ENXIO is more appropriate for this case.

 drivers/gpio/gpiolib-of.c |    8 +++++---
 drivers/hwmon/gpio-fan.c  |    2 +-
 drivers/of/base.c         |    5 ++++-
 drivers/spi/spi.c         |   11 +++++++----
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index d542a14..28f24a6 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -107,11 +107,10 @@ EXPORT_SYMBOL(of_get_named_gpio_flags);
  */
 unsigned int of_gpio_named_count(struct device_node *np, const char* propname)
 {
+	int ret;
 	unsigned int cnt = 0;
 
 	do {
-		int ret;
-
 		ret = of_parse_phandle_with_args(np, propname, "#gpio-cells",
 						 cnt, NULL);
 		/* A hole in the gpios = <> counts anyway. */
@@ -119,7 +118,10 @@ unsigned int of_gpio_named_count(struct device_node *np, const char* propname)
 			break;
 	} while (++cnt);
 
-	return cnt;
+	if (ret == -ENXIO)
+		return cnt;
+	else
+		return ret;
 }
 EXPORT_SYMBOL(of_gpio_named_count);
 
diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 4e04c12..9ccd92f 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -477,7 +477,7 @@ static int gpio_fan_get_of_pdata(struct device *dev,
 	pdata->speed = speed;
 
 	/* Alarm GPIO if one exists */
-	if (of_gpio_named_count(node, "alarm-gpios")) {
+	if (of_gpio_named_count(node, "alarm-gpios") > 0) {
 		struct gpio_fan_alarm *alarm;
 		int val;
 		enum of_gpio_flags flags;
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 986afd7..1f16629 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1110,7 +1110,10 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 	/* Loop exited without finding a valid entry; return an error */
 	if (node)
 		of_node_put(node);
-	return -EINVAL;
+	if (list == list_end)
+		return -ENXIO; /* Index beyond end of list */
+	else
+		return -EINVAL;
 }
 EXPORT_SYMBOL(of_parse_phandle_with_args);
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 19ee901..9c2acf1 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1059,7 +1059,7 @@ EXPORT_SYMBOL_GPL(spi_alloc_master);
 #ifdef CONFIG_OF
 static int of_spi_register_master(struct spi_master *master)
 {
-	u16 nb;
+	int nb;
 	int i, *cs;
 	struct device_node *np = master->dev.of_node;
 
@@ -1067,10 +1067,13 @@ static int of_spi_register_master(struct spi_master *master)
 		return 0;
 
 	nb = of_gpio_named_count(np, "cs-gpios");
-	master->num_chipselect = max(nb, master->num_chipselect);
-
-	if (nb < 1)
+	if (nb == 0 || nb == -ENOENT) /* No error if cs-gpios does not exist */
 		return 0;
+	else if (nb < 0)
+		return nb;
+
+	if (nb > master->num_chipselect)
+		master->num_chipselect = (u16)nb;
 
 	cs = devm_kzalloc(&master->dev,
 			  sizeof(int) * master->num_chipselect,
-- 
1.7.0.4

Re: [PATCH 2/6] of: Return -ENXIO from of_parse_phandle_with_args for too large index and return errors from of_gpio_named_count

[Andreas Larsson]

On 2012-12-27 13:10, Andreas Larsson wrote:
> This lets of_gpio_named_count return an errno on errors by being able to
> distinguish between reaching the end of the phandle list and getting some other
> error from of_parse_phandle_with_args.
>
> Return error from of_spi_register_master when there is an "cs-gpios" list for
> which gp_gpio_named_count fails.
>
> Adjust drivers/hwmon/gpio-fan.c to cope with error return.
>
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>

The callers of of_gpio_count of course also needs to handle errno 
returns. I'll take care of that in V2 of the patch-set. But I'll wait 
for more comments on the patch set before sending V2 unless someone insists.

Cheers,
Andreas

[PATCH 3/6] of_spi: Initialize cs_gpios properly

[Andreas Larsson]

Using memset does not set an array of integers properly

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/spi/spi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9c2acf1..a4baa0a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1083,7 +1083,8 @@ static int of_spi_register_master(struct spi_master *master)
 	if (!master->cs_gpios)
 		return -ENOMEM;
 
-	memset(cs, -EINVAL, master->num_chipselect);
+	for (i = 0; i < master->num_chipselect; i++)
+		cs[i] = -EINVAL;
 
 	for (i = 0; i < nb; i++)
 		cs[i] = of_get_named_gpio(np, "cs-gpios", i);
-- 
1.7.0.4

[PATCH 4/6] of_spi: Document cs_gpios and cs_gpio in kernel-doc

[Andreas Larsson]

This adds missing kernel-doc entries for cs_gpios in struct spi_master and
cs_gpio in struct spi_device.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 include/linux/spi/spi.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index f629189..88a669c 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -57,6 +57,7 @@ extern struct bus_type spi_bus_type;
  * @modalias: Name of the driver to use with this device, or an alias
  *	for that name.  This appears in the sysfs "modalias" attribute
  *	for driver coldplugging, and in uevents used for hotplugging
+ * @cs_gpio: Negative or gpio for chip select.
  *
  * A @spi_device is used to interchange data between an SPI slave
  * (usually a discrete chip) and CPU memory.
@@ -258,6 +259,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @unprepare_transfer_hardware: there are currently no more messages on the
  *	queue so the subsystem notifies the driver that it may relax the
  *	hardware by issuing this call
+ * @cs_gpios: possible array of negative values or gpios for chip selects
  *
  * Each SPI master controller can communicate with one or more @spi_device
  * children.  These make a small bus, sharing MOSI, MISO and SCK signals
-- 
1.7.0.4

[PATCH 5/6] of_spi: Add fetching of of_gpio flags to of_spi_register_master

[Andreas Larsson]

When using a gpio chip select with a OF_GPIO_ACTIVE_LOW flag, this needs to be
known to the controller driver.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 Documentation/devicetree/bindings/spi/spi-bus.txt |    3 +-
 drivers/spi/spi.c                                 |   24 ++++++++++++++++++--
 include/linux/spi/spi.h                           |    5 ++++
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 296015e..a4950a6 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -57,7 +57,8 @@ contain the following properties.
     		    3-wire mode.
 
 If a gpio chipselect is used for the SPI slave the gpio number will be passed
-via the cs_gpio
+via the cs_gpio and the corresponding of_gpio_flags will be passed via
+cs_gpio_flags.
 
 SPI example for an MPC5200 SPI bus:
 	spi@f00 {
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a4baa0a..6f1b717 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -385,8 +385,10 @@ int spi_add_device(struct spi_device *spi)
 		goto done;
 	}
 
-	if (master->cs_gpios)
+	if (master->cs_gpios) {
 		spi->cs_gpio = master->cs_gpios[spi->chip_select];
+		spi->cs_gpio_flags = master->cs_gpio_flags[spi->chip_select];
+	}
 
 	/* Drivers may modify this initial i/o setup, but will
 	 * normally rely on the device being setup.  Devices
@@ -1060,7 +1062,8 @@ EXPORT_SYMBOL_GPL(spi_alloc_master);
 static int of_spi_register_master(struct spi_master *master)
 {
 	int nb;
-	int i, *cs;
+	int ret, i, *cs;
+	enum of_gpio_flags *flags;
 	struct device_node *np = master->dev.of_node;
 
 	if (!np)
@@ -1083,13 +1086,28 @@ static int of_spi_register_master(struct spi_master *master)
 	if (!master->cs_gpios)
 		return -ENOMEM;
 
+	flags = devm_kzalloc(&master->dev,
+			     sizeof(*flags) * master->num_chipselect,
+			     GFP_KERNEL);
+	master->cs_gpio_flags = flags;
+
+	if (!master->cs_gpio_flags) {
+		ret = -ENOMEM;
+		goto err_alloc_flags;
+	}
+
 	for (i = 0; i < master->num_chipselect; i++)
 		cs[i] = -EINVAL;
 
 	for (i = 0; i < nb; i++)
-		cs[i] = of_get_named_gpio(np, "cs-gpios", i);
+		cs[i] = of_get_named_gpio_flags(np, "cs-gpios", i, &flags[i]);
 
 	return 0;
+
+err_alloc_flags:
+	devm_kfree(&master->dev, master->cs_gpios);
+	master->cs_gpios = NULL;
+	return ret;
 }
 #else
 static int of_spi_register_master(struct spi_master *master)
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 88a669c..96b1055 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -23,6 +23,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/slab.h>
 #include <linux/kthread.h>
+#include <linux/of_gpio.h>
 
 /*
  * INTERFACES between SPI master-side drivers and SPI infrastructure.
@@ -58,6 +59,7 @@ extern struct bus_type spi_bus_type;
  *	for that name.  This appears in the sysfs "modalias" attribute
  *	for driver coldplugging, and in uevents used for hotplugging
  * @cs_gpio: Negative or gpio for chip select.
+ * @cs_gpio_flags: of_gpio_flags corresponding to cs_gpio
  *
  * A @spi_device is used to interchange data between an SPI slave
  * (usually a discrete chip) and CPU memory.
@@ -92,6 +94,7 @@ struct spi_device {
 	void			*controller_data;
 	char			modalias[SPI_NAME_SIZE];
 	int			cs_gpio;	/* chip select gpio */
+	enum of_gpio_flags	cs_gpio_flags;	/* chip select of_gpio_flags */
 
 	/*
 	 * likely need more hooks for more protocol options affecting how
@@ -260,6 +263,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  *	queue so the subsystem notifies the driver that it may relax the
  *	hardware by issuing this call
  * @cs_gpios: possible array of negative values or gpios for chip selects
+ * @cs_gpio_flags: possible array of of_gpio_flags corresponding to cs_gpios
  *
  * Each SPI master controller can communicate with one or more @spi_device
  * children.  These make a small bus, sharing MOSI, MISO and SCK signals
@@ -367,6 +371,7 @@ struct spi_master {
 	int (*unprepare_transfer_hardware)(struct spi_master *master);
 	/* gpio chip select */
 	int			*cs_gpios;
+	enum of_gpio_flags	*cs_gpio_flags;
 };
 
 static inline void *spi_master_get_devdata(struct spi_master *master)
-- 
1.7.0.4

[PATCH 6/6] of_spi: Initialize cs_gpios and cs_gpio with -EEXIST

[Andreas Larsson]

Holes in the cs-gpios DT phandle list is supposed to mark that native
chipselects is to be used. The value returned from of_get_named_gpio_flags in
this case is -EEXIST. By initializing cs_gpios and cs_gpio with -EEXIST, this
and only this errno will indicate to a spi controller driver that a native
chipselect is to be used.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/spi/spi.c       |    4 ++--
 include/linux/spi/spi.h |    7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6f1b717..7494bad 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -334,7 +334,7 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
 	spi->dev.parent = &master->dev;
 	spi->dev.bus = &spi_bus_type;
 	spi->dev.release = spidev_release;
-	spi->cs_gpio = -EINVAL;
+	spi->cs_gpio = -EEXIST;
 	device_initialize(&spi->dev);
 	return spi;
 }
@@ -1097,7 +1097,7 @@ static int of_spi_register_master(struct spi_master *master)
 	}
 
 	for (i = 0; i < master->num_chipselect; i++)
-		cs[i] = -EINVAL;
+		cs[i] = -EEXIST;
 
 	for (i = 0; i < nb; i++)
 		cs[i] = of_get_named_gpio_flags(np, "cs-gpios", i, &flags[i]);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 96b1055..0701882 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -58,7 +58,8 @@ extern struct bus_type spi_bus_type;
  * @modalias: Name of the driver to use with this device, or an alias
  *	for that name.  This appears in the sysfs "modalias" attribute
  *	for driver coldplugging, and in uevents used for hotplugging
- * @cs_gpio: Negative or gpio for chip select.
+ * @cs_gpio: Negative or gpio for chip select.  Set to -EEXIST when chipselect
+ *	should be handled natively by the controller driver
  * @cs_gpio_flags: of_gpio_flags corresponding to cs_gpio
  *
  * A @spi_device is used to interchange data between an SPI slave
@@ -262,7 +263,9 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @unprepare_transfer_hardware: there are currently no more messages on the
  *	queue so the subsystem notifies the driver that it may relax the
  *	hardware by issuing this call
- * @cs_gpios: possible array of negative values or gpios for chip selects
+ * @cs_gpios: possible array of negative values or gpios for chip selects.  A
+ *	chipselect that should be handled natively by the controller driver is
+ *	set to -EEXIST.
  * @cs_gpio_flags: possible array of of_gpio_flags corresponding to cs_gpios
  *
  * Each SPI master controller can communicate with one or more @spi_device
-- 
1.7.0.4