[PATCH 0/3] of: Add support for named irq and reg

[PATCH 0/3] of: Add support for named irq and reg

[Benoit Cousson]

Hi Grant & Rob,

Following the previous patch submission [1], here is an updated series
that adds the support for both reg and irq names.
A small improvement is done as well on the property with multiple strings
helper function.

Please note that I've just realized that Russell's concern with the
/proc/iomem readability might not even be valid in the DT case.
I might be wrong, but it looks like devices created by of_device_alloc
does not seems to call the insert_resource. It means that the /proc/iomem
is mostly empty when the devices are created from device tree blob.
I'm not sure it was done on purpose, but even if this is not the case,
the fact is that this /proc/iomem entry does not seems to be used
extensively.
That does not mean it cannot be improved, but it should change the urgency
to fix that with regards to that series.

This series is based on 3.2-rc4 and is available here:
git://gitorious.org/omap-pm/linux.git for_3.3/resource-names

Comments are welcome.

Auto comment: I'm not super happy with these new properties name because it
is not really consistent. If you have any better idea, do not hesitate.

Thanks,
Benoit


[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg57881.html


Benoit Cousson (3):
  of/base: Take NULL string into account for property with multiple strings
  of/address: Add reg-names property to name an iomem resource
  of/irq: Add interrupts-names property to name an irq resource

 .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
 Documentation/devicetree/bindings/reg-names.txt    |   48 +++++++++++++++++++
 drivers/of/address.c                               |   16 ++++--
 drivers/of/base.c                                  |    8 +--
 drivers/of/irq.c                                   |   11 ++++-
 5 files changed, 122 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
 create mode 100644 Documentation/devicetree/bindings/reg-names.txt

[PATCH 0/3] of: Add support for named irq and reg

[Benoit Cousson]

Hi Grant & Rob,

Following the previous patch submission [1], here is an updated series
that adds the support for both reg and irq names.
A small improvement is done as well on the property with multiple strings
helper function.

Please note that I've just realized that Russell's concern with the
/proc/iomem readability might not even be valid in the DT case.
I might be wrong, but it looks like devices created by of_device_alloc
does not seems to call the insert_resource. It means that the /proc/iomem
is mostly empty when the devices are created from device tree blob.
I'm not sure it was done on purpose, but even if this is not the case,
the fact is that this /proc/iomem entry does not seems to be used
extensively.
That does not mean it cannot be improved, but it should change the urgency
to fix that with regards to that series.

This series is based on 3.2-rc4 and is available here:
git://gitorious.org/omap-pm/linux.git for_3.3/resource-names

Comments are welcome.

Auto comment: I'm not super happy with these new properties name because it
is not really consistent. If you have any better idea, do not hesitate.

Thanks,
Benoit


[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg57881.html


Benoit Cousson (3):
  of/base: Take NULL string into account for property with multiple strings
  of/address: Add reg-names property to name an iomem resource
  of/irq: Add interrupts-names property to name an irq resource

 .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
 Documentation/devicetree/bindings/reg-names.txt    |   48 +++++++++++++++++++
 drivers/of/address.c                               |   16 ++++--
 drivers/of/base.c                                  |    8 +--
 drivers/of/irq.c                                   |   11 ++++-
 5 files changed, 122 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
 create mode 100644 Documentation/devicetree/bindings/reg-names.txt

[PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Benoit Cousson]

The current implementation just ignore any NULL string inserted in a
multiple strings property.
In some cases we can have a property with a fix number of strings but
not necessarily used, like for example in a list of valid pinmux modes.

 prop = "uart_rx", "uart_tx", "", "", "safe_mode";

Do no skip NULL string and take them into account in
of_property_read_string_index and of_property_count_strings.

Reported-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 drivers/of/base.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9b6588e..b707243 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -752,7 +752,7 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
 
 	for (i = 0; total < prop->length; total += l, p += l) {
 		l = strlen(p) + 1;
-		if ((*p != 0) && (i++ == index)) {
+		if (i++ == index) {
 			*output = p;
 			return 0;
 		}
@@ -790,11 +790,9 @@ int of_property_count_strings(struct device_node *np, const char *propname)
 
 	p = prop->value;
 
-	for (i = 0; total < prop->length; total += l, p += l) {
+	for (i = 0; total < prop->length; total += l, p += l, i++)
 		l = strlen(p) + 1;
-		if (*p != 0)
-			i++;
-	}
+
 	return i;
 }
 EXPORT_SYMBOL_GPL(of_property_count_strings);
-- 
1.7.0.4

[PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Benoit Cousson]

The current implementation just ignore any NULL string inserted in a
multiple strings property.
In some cases we can have a property with a fix number of strings but
not necessarily used, like for example in a list of valid pinmux modes.

 prop = "uart_rx", "uart_tx", "", "", "safe_mode";

Do no skip NULL string and take them into account in
of_property_read_string_index and of_property_count_strings.

Reported-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 drivers/of/base.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9b6588e..b707243 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -752,7 +752,7 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
 
 	for (i = 0; total < prop->length; total += l, p += l) {
 		l = strlen(p) + 1;
-		if ((*p != 0) && (i++ == index)) {
+		if (i++ == index) {
 			*output = p;
 			return 0;
 		}
@@ -790,11 +790,9 @@ int of_property_count_strings(struct device_node *np, const char *propname)
 
 	p = prop->value;
 
-	for (i = 0; total < prop->length; total += l, p += l) {
+	for (i = 0; total < prop->length; total += l, p += l, i++)
 		l = strlen(p) + 1;
-		if (*p != 0)
-			i++;
-	}
+
 	return i;
 }
 EXPORT_SYMBOL_GPL(of_property_count_strings);
-- 
1.7.0.4

[PATCH 2/3] of/address: Add reg-names property to name an iomem resource

[Benoit Cousson]

In a HW system, resources in general have name to identify them.
The is the case as well for IOMEM resources filled by DT "reg" entries.
The current DT mechanism is relying on the "reg" order to identify
the proper resource. This is error prone and not the natural way to
retrieve an information in general.
Moreover, the Linux resource system does support a name and an API to
allow a driver to retrieve the resource using the name instead of an
index.

Add a reg-names property to allow the possiblity to provide a name
to any reg entries.
If the name is available, use it to name the resource, otherwise
keep the legacy device name.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 Documentation/devicetree/bindings/reg-names.txt |   48 +++++++++++++++++++++++
 drivers/of/address.c                            |   16 +++++--
 2 files changed, 59 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reg-names.txt

diff --git a/Documentation/devicetree/bindings/reg-names.txt b/Documentation/devicetree/bindings/reg-names.txt
new file mode 100644
index 0000000..5554065
--- /dev/null
+++ b/Documentation/devicetree/bindings/reg-names.txt
@@ -0,0 +1,48 @@
+reg-names property
+
+In a HW system, resources in general have name to identify them.
+The is the case as well for register entries.
+The current DT mechanism is relying on the "reg" order to identify
+the proper resource. The reg-names is adding the possiblity to
+provide a name to reg entries.
+
+Usage:
+
+This attribute must be used along with a regular reg entry. If not
+it will be simply ignored.
+The number of entry must match otherwise the default device name will
+be used to as the resource name.
+
+
+Example:
+
+
+l4-abe {
+	compatible = "simple-bus";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
+		 <1 0 0x49000000 0x00001000>; /* L3 path */
+	mcasp {
+		compatible = "ti,mcasp";
+		reg = <0 0x10 0x10>, <0 0x20 0x10>,
+		      <1 0x10 0x10>, <1 0x20 0x10>;
+		reg-names = "mpu", "dat",
+			    "dma", "dma_dat";
+	};
+
+	timer {
+		compatible = "ti,timer";
+		reg = <0 0x40 0x10>, <1 0x40 0x10>;
+		reg-names = "mpu", "dma";
+	};
+};
+
+
+usb {
+	compatible = "ti,usb-host";
+	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
+	      <0x4a064c00 0x200>;
+	reg-names = "config", "ohci", "ehci";
+};
+
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 72c33fb..66d96f1 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -14,7 +14,7 @@
 static struct of_bus *of_match_bus(struct device_node *np);
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
-				    struct resource *r);
+		const char *name, struct resource *r);
 
 /* Debug utility */
 #ifdef DEBUG
@@ -215,7 +215,7 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
 	addrp = of_get_pci_address(dev, bar, &size, &flags);
 	if (addrp == NULL)
 		return -EINVAL;
-	return __of_address_to_resource(dev, addrp, size, flags, r);
+	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
 }
 EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
 #endif /* CONFIG_PCI */
@@ -529,7 +529,7 @@ EXPORT_SYMBOL(of_get_address);
 
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
-				    struct resource *r)
+		const char *name, struct resource *r)
 {
 	u64 taddr;
 
@@ -551,7 +551,8 @@ static int __of_address_to_resource(struct device_node *dev,
 		r->end = taddr + size - 1;
 	}
 	r->flags = flags;
-	r->name = dev->full_name;
+	r->name = name ? name : dev->full_name;
+
 	return 0;
 }
 
@@ -569,11 +570,16 @@ int of_address_to_resource(struct device_node *dev, int index,
 	const __be32	*addrp;
 	u64		size;
 	unsigned int	flags;
+	const char	*name = NULL;
 
 	addrp = of_get_address(dev, index, &size, &flags);
 	if (addrp == NULL)
 		return -EINVAL;
-	return __of_address_to_resource(dev, addrp, size, flags, r);
+
+	/* Get optional "reg-names" property to add a name to a resource */
+	of_property_read_string_index(dev, "reg-names",	index, &name);
+
+	return __of_address_to_resource(dev, addrp, size, flags, name, r);
 }
 EXPORT_SYMBOL_GPL(of_address_to_resource);
 
-- 
1.7.0.4

[PATCH 2/3] of/address: Add reg-names property to name an iomem resource

[Benoit Cousson]

In a HW system, resources in general have name to identify them.
The is the case as well for IOMEM resources filled by DT "reg" entries.
The current DT mechanism is relying on the "reg" order to identify
the proper resource. This is error prone and not the natural way to
retrieve an information in general.
Moreover, the Linux resource system does support a name and an API to
allow a driver to retrieve the resource using the name instead of an
index.

Add a reg-names property to allow the possiblity to provide a name
to any reg entries.
If the name is available, use it to name the resource, otherwise
keep the legacy device name.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 Documentation/devicetree/bindings/reg-names.txt |   48 +++++++++++++++++++++++
 drivers/of/address.c                            |   16 +++++--
 2 files changed, 59 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reg-names.txt

diff --git a/Documentation/devicetree/bindings/reg-names.txt b/Documentation/devicetree/bindings/reg-names.txt
new file mode 100644
index 0000000..5554065
--- /dev/null
+++ b/Documentation/devicetree/bindings/reg-names.txt
@@ -0,0 +1,48 @@
+reg-names property
+
+In a HW system, resources in general have name to identify them.
+The is the case as well for register entries.
+The current DT mechanism is relying on the "reg" order to identify
+the proper resource. The reg-names is adding the possiblity to
+provide a name to reg entries.
+
+Usage:
+
+This attribute must be used along with a regular reg entry. If not
+it will be simply ignored.
+The number of entry must match otherwise the default device name will
+be used to as the resource name.
+
+
+Example:
+
+
+l4-abe {
+	compatible = "simple-bus";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
+		 <1 0 0x49000000 0x00001000>; /* L3 path */
+	mcasp {
+		compatible = "ti,mcasp";
+		reg = <0 0x10 0x10>, <0 0x20 0x10>,
+		      <1 0x10 0x10>, <1 0x20 0x10>;
+		reg-names = "mpu", "dat",
+			    "dma", "dma_dat";
+	};
+
+	timer {
+		compatible = "ti,timer";
+		reg = <0 0x40 0x10>, <1 0x40 0x10>;
+		reg-names = "mpu", "dma";
+	};
+};
+
+
+usb {
+	compatible = "ti,usb-host";
+	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
+	      <0x4a064c00 0x200>;
+	reg-names = "config", "ohci", "ehci";
+};
+
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 72c33fb..66d96f1 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -14,7 +14,7 @@
 static struct of_bus *of_match_bus(struct device_node *np);
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
-				    struct resource *r);
+		const char *name, struct resource *r);
 
 /* Debug utility */
 #ifdef DEBUG
@@ -215,7 +215,7 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
 	addrp = of_get_pci_address(dev, bar, &size, &flags);
 	if (addrp == NULL)
 		return -EINVAL;
-	return __of_address_to_resource(dev, addrp, size, flags, r);
+	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
 }
 EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
 #endif /* CONFIG_PCI */
@@ -529,7 +529,7 @@ EXPORT_SYMBOL(of_get_address);
 
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
-				    struct resource *r)
+		const char *name, struct resource *r)
 {
 	u64 taddr;
 
@@ -551,7 +551,8 @@ static int __of_address_to_resource(struct device_node *dev,
 		r->end = taddr + size - 1;
 	}
 	r->flags = flags;
-	r->name = dev->full_name;
+	r->name = name ? name : dev->full_name;
+
 	return 0;
 }
 
@@ -569,11 +570,16 @@ int of_address_to_resource(struct device_node *dev, int index,
 	const __be32	*addrp;
 	u64		size;
 	unsigned int	flags;
+	const char	*name = NULL;
 
 	addrp = of_get_address(dev, index, &size, &flags);
 	if (addrp == NULL)
 		return -EINVAL;
-	return __of_address_to_resource(dev, addrp, size, flags, r);
+
+	/* Get optional "reg-names" property to add a name to a resource */
+	of_property_read_string_index(dev, "reg-names",	index, &name);
+
+	return __of_address_to_resource(dev, addrp, size, flags, name, r);
 }
 EXPORT_SYMBOL_GPL(of_address_to_resource);
 
-- 
1.7.0.4

[PATCH 3/3] of/irq: Add interrupts-names property to name an irq resource

[Benoit Cousson]

In a HW system, resources in general have name to identify them.
The is the case as well for IORESOURCE_IRQ resources filled by DT
"interrupts" entries.
The current DT mechanism is relying on the "interrupts" order to identify
the proper resource. This is error prone and not the natural way to
retrieve an information in general.
Moreover, the resource does support a name and an API is available to
allow a driver to retrieve the resource using the name instead of an
index.

Add a interrupts-names property to allow the possiblity to provide a name
to any interrupts entries.
If the name is available, use it to name the resource, otherwise
keep the legacy device full name.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
 drivers/of/irq.c                                   |   11 ++++-
 2 files changed, 60 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt

diff --git a/Documentation/devicetree/bindings/interrupts-names.txt b/Documentation/devicetree/bindings/interrupts-names.txt
new file mode 100644
index 0000000..d9a796d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupts-names.txt
@@ -0,0 +1,50 @@
+interrupts-names property
+
+In a HW system, physical resources in general have name to identify them.
+The is the case as well for interrupt lines.
+The current DT mechanism is relying on the "interrupts" order to identify
+the proper resource. The interrupts-names is adding the possiblity to
+provide a name to interrupts entries.
+
+Usage:
+
+This attribute must be used along with a regular interrupts entry. If not
+it will be simply ignored.
+
+
+Example:
+
+
+l4-abe {
+	compatible = "simple-bus";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
+		 <1 0 0x49000000 0x00001000>; /* L3 path */
+	mcasp {
+		compatible = "ti,mcasp";
+		reg = <0 0x10 0x10>, <0 0x20 0x10>,
+		      <1 0x10 0x10>, <1 0x20 0x10>;
+		reg-names = "mpu", "dat",
+			    "dma", "dma_dat";
+		interrupts = <11>, <12>;
+		interrupts-names = "rx", "tx";
+	};
+
+	timer {
+		compatible = "ti,timer";
+		reg = <0 0x40 0x10>, <1 0x40 0x10>;
+		reg-names = "mpu", "dma";
+	};
+};
+
+
+usb {
+	compatible = "ti,usb-host";
+	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
+	      <0x4a064c00 0x200>;
+	reg-names = "config", "ohci", "ehci";
+	interrupts = <14>, <15>;
+	interrupts-names = "ohci", "ehci";
+};
+
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 19c0115..604b53f 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -346,9 +346,18 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 	/* Only dereference the resource if both the
 	 * resource and the irq are valid. */
 	if (r && irq != NO_IRQ) {
+		const char *name = NULL;
+
+		/*
+		 * Get optional "interrupts-names" property to add a name
+		 * to the resource.
+		 */
+		of_property_read_string_index(dev, "interrupts-names", index,
+					      &name);
+
 		r->start = r->end = irq;
 		r->flags = IORESOURCE_IRQ;
-		r->name = dev->full_name;
+		r->name = name ? name : dev->full_name;
 	}
 
 	return irq;
-- 
1.7.0.4

[PATCH 3/3] of/irq: Add interrupts-names property to name an irq resource

[Benoit Cousson]

In a HW system, resources in general have name to identify them.
The is the case as well for IORESOURCE_IRQ resources filled by DT
"interrupts" entries.
The current DT mechanism is relying on the "interrupts" order to identify
the proper resource. This is error prone and not the natural way to
retrieve an information in general.
Moreover, the resource does support a name and an API is available to
allow a driver to retrieve the resource using the name instead of an
index.

Add a interrupts-names property to allow the possiblity to provide a name
to any interrupts entries.
If the name is available, use it to name the resource, otherwise
keep the legacy device full name.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
---
 .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
 drivers/of/irq.c                                   |   11 ++++-
 2 files changed, 60 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt

diff --git a/Documentation/devicetree/bindings/interrupts-names.txt b/Documentation/devicetree/bindings/interrupts-names.txt
new file mode 100644
index 0000000..d9a796d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupts-names.txt
@@ -0,0 +1,50 @@
+interrupts-names property
+
+In a HW system, physical resources in general have name to identify them.
+The is the case as well for interrupt lines.
+The current DT mechanism is relying on the "interrupts" order to identify
+the proper resource. The interrupts-names is adding the possiblity to
+provide a name to interrupts entries.
+
+Usage:
+
+This attribute must be used along with a regular interrupts entry. If not
+it will be simply ignored.
+
+
+Example:
+
+
+l4-abe {
+	compatible = "simple-bus";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
+		 <1 0 0x49000000 0x00001000>; /* L3 path */
+	mcasp {
+		compatible = "ti,mcasp";
+		reg = <0 0x10 0x10>, <0 0x20 0x10>,
+		      <1 0x10 0x10>, <1 0x20 0x10>;
+		reg-names = "mpu", "dat",
+			    "dma", "dma_dat";
+		interrupts = <11>, <12>;
+		interrupts-names = "rx", "tx";
+	};
+
+	timer {
+		compatible = "ti,timer";
+		reg = <0 0x40 0x10>, <1 0x40 0x10>;
+		reg-names = "mpu", "dma";
+	};
+};
+
+
+usb {
+	compatible = "ti,usb-host";
+	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
+	      <0x4a064c00 0x200>;
+	reg-names = "config", "ohci", "ehci";
+	interrupts = <14>, <15>;
+	interrupts-names = "ohci", "ehci";
+};
+
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 19c0115..604b53f 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -346,9 +346,18 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 	/* Only dereference the resource if both the
 	 * resource and the irq are valid. */
 	if (r && irq != NO_IRQ) {
+		const char *name = NULL;
+
+		/*
+		 * Get optional "interrupts-names" property to add a name
+		 * to the resource.
+		 */
+		of_property_read_string_index(dev, "interrupts-names", index,
+					      &name);
+
 		r->start = r->end = irq;
 		r->flags = IORESOURCE_IRQ;
-		r->name = dev->full_name;
+		r->name = name ? name : dev->full_name;
 	}
 
 	return irq;
-- 
1.7.0.4

Re: [PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Tony Lindgren]

* Benoit Cousson <b-cousson@ti.com> [111205 05:53]:
> The current implementation just ignore any NULL string inserted in a
> multiple strings property.
> In some cases we can have a property with a fix number of strings but
> not necessarily used, like for example in a list of valid pinmux modes.
> 
>  prop = "uart_rx", "uart_tx", "", "", "safe_mode";
> 
> Do no skip NULL string and take them into account in
> of_property_read_string_index and of_property_count_strings.
> 
> Reported-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>

This fix should be merged before people start doing workarounds
in drivers for this:

Tested-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/of/base.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 9b6588e..b707243 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -752,7 +752,7 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
>  
>  	for (i = 0; total < prop->length; total += l, p += l) {
>  		l = strlen(p) + 1;
> -		if ((*p != 0) && (i++ == index)) {
> +		if (i++ == index) {
>  			*output = p;
>  			return 0;
>  		}
> @@ -790,11 +790,9 @@ int of_property_count_strings(struct device_node *np, const char *propname)
>  
>  	p = prop->value;
>  
> -	for (i = 0; total < prop->length; total += l, p += l) {
> +	for (i = 0; total < prop->length; total += l, p += l, i++)
>  		l = strlen(p) + 1;
> -		if (*p != 0)
> -			i++;
> -	}
> +
>  	return i;
>  }
>  EXPORT_SYMBOL_GPL(of_property_count_strings);
> -- 
> 1.7.0.4
> 

[PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Tony Lindgren]

* Benoit Cousson <b-cousson@ti.com> [111205 05:53]:
> The current implementation just ignore any NULL string inserted in a
> multiple strings property.
> In some cases we can have a property with a fix number of strings but
> not necessarily used, like for example in a list of valid pinmux modes.
> 
>  prop = "uart_rx", "uart_tx", "", "", "safe_mode";
> 
> Do no skip NULL string and take them into account in
> of_property_read_string_index and of_property_count_strings.
> 
> Reported-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>

This fix should be merged before people start doing workarounds
in drivers for this:

Tested-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/of/base.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 9b6588e..b707243 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -752,7 +752,7 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
>  
>  	for (i = 0; total < prop->length; total += l, p += l) {
>  		l = strlen(p) + 1;
> -		if ((*p != 0) && (i++ == index)) {
> +		if (i++ == index) {
>  			*output = p;
>  			return 0;
>  		}
> @@ -790,11 +790,9 @@ int of_property_count_strings(struct device_node *np, const char *propname)
>  
>  	p = prop->value;
>  
> -	for (i = 0; total < prop->length; total += l, p += l) {
> +	for (i = 0; total < prop->length; total += l, p += l, i++)
>  		l = strlen(p) + 1;
> -		if (*p != 0)
> -			i++;
> -	}
> +
>  	return i;
>  }
>  EXPORT_SYMBOL_GPL(of_property_count_strings);
> -- 
> 1.7.0.4
> 

Re: [PATCH 0/3] of: Add support for named irq and reg

[Cousson, Benoit]

Hi Grant and Rob,

Gentle ping: Do you have some comment on that series?

Thanks,
Benoit

On 12/5/2011 3:23 PM, Benoit Cousson wrote:
> Hi Grant&  Rob,
>
> Following the previous patch submission [1], here is an updated series
> that adds the support for both reg and irq names.
> A small improvement is done as well on the property with multiple strings
> helper function.
>
> Please note that I've just realized that Russell's concern with the
> /proc/iomem readability might not even be valid in the DT case.
> I might be wrong, but it looks like devices created by of_device_alloc
> does not seems to call the insert_resource. It means that the /proc/iomem
> is mostly empty when the devices are created from device tree blob.
> I'm not sure it was done on purpose, but even if this is not the case,
> the fact is that this /proc/iomem entry does not seems to be used
> extensively.
> That does not mean it cannot be improved, but it should change the urgency
> to fix that with regards to that series.
>
> This series is based on 3.2-rc4 and is available here:
> git://gitorious.org/omap-pm/linux.git for_3.3/resource-names
>
> Comments are welcome.
>
> Auto comment: I'm not super happy with these new properties name because it
> is not really consistent. If you have any better idea, do not hesitate.
>
> Thanks,
> Benoit
>
>
> [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg57881.html
>
>
> Benoit Cousson (3):
>    of/base: Take NULL string into account for property with multiple strings
>    of/address: Add reg-names property to name an iomem resource
>    of/irq: Add interrupts-names property to name an irq resource
>
>   .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
>   Documentation/devicetree/bindings/reg-names.txt    |   48 +++++++++++++++++++
>   drivers/of/address.c                               |   16 ++++--
>   drivers/of/base.c                                  |    8 +--
>   drivers/of/irq.c                                   |   11 ++++-
>   5 files changed, 122 insertions(+), 11 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
>   create mode 100644 Documentation/devicetree/bindings/reg-names.txt

[PATCH 0/3] of: Add support for named irq and reg

[Cousson, Benoit]

Hi Grant and Rob,

Gentle ping: Do you have some comment on that series?

Thanks,
Benoit

On 12/5/2011 3:23 PM, Benoit Cousson wrote:
> Hi Grant&  Rob,
>
> Following the previous patch submission [1], here is an updated series
> that adds the support for both reg and irq names.
> A small improvement is done as well on the property with multiple strings
> helper function.
>
> Please note that I've just realized that Russell's concern with the
> /proc/iomem readability might not even be valid in the DT case.
> I might be wrong, but it looks like devices created by of_device_alloc
> does not seems to call the insert_resource. It means that the /proc/iomem
> is mostly empty when the devices are created from device tree blob.
> I'm not sure it was done on purpose, but even if this is not the case,
> the fact is that this /proc/iomem entry does not seems to be used
> extensively.
> That does not mean it cannot be improved, but it should change the urgency
> to fix that with regards to that series.
>
> This series is based on 3.2-rc4 and is available here:
> git://gitorious.org/omap-pm/linux.git for_3.3/resource-names
>
> Comments are welcome.
>
> Auto comment: I'm not super happy with these new properties name because it
> is not really consistent. If you have any better idea, do not hesitate.
>
> Thanks,
> Benoit
>
>
> [1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg57881.html
>
>
> Benoit Cousson (3):
>    of/base: Take NULL string into account for property with multiple strings
>    of/address: Add reg-names property to name an iomem resource
>    of/irq: Add interrupts-names property to name an irq resource
>
>   .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
>   Documentation/devicetree/bindings/reg-names.txt    |   48 +++++++++++++++++++
>   drivers/of/address.c                               |   16 ++++--
>   drivers/of/base.c                                  |    8 +--
>   drivers/of/irq.c                                   |   11 ++++-
>   5 files changed, 122 insertions(+), 11 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
>   create mode 100644 Documentation/devicetree/bindings/reg-names.txt

Re: [PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Rob Herring]

On 12/05/2011 08:23 AM, Benoit Cousson wrote:
> The current implementation just ignore any NULL string inserted in a
> multiple strings property.
> In some cases we can have a property with a fix number of strings but
> not necessarily used, like for example in a list of valid pinmux modes.
> 
>  prop = "uart_rx", "uart_tx", "", "", "safe_mode";
> 
> Do no skip NULL string and take them into account in
> of_property_read_string_index and of_property_count_strings.
> 
> Reported-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>

Applied. The others need Grant's agreement.

Rob

> ---
>  drivers/of/base.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 9b6588e..b707243 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -752,7 +752,7 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
>  
>  	for (i = 0; total < prop->length; total += l, p += l) {
>  		l = strlen(p) + 1;
> -		if ((*p != 0) && (i++ == index)) {
> +		if (i++ == index) {
>  			*output = p;
>  			return 0;
>  		}
> @@ -790,11 +790,9 @@ int of_property_count_strings(struct device_node *np, const char *propname)
>  
>  	p = prop->value;
>  
> -	for (i = 0; total < prop->length; total += l, p += l) {
> +	for (i = 0; total < prop->length; total += l, p += l, i++)
>  		l = strlen(p) + 1;
> -		if (*p != 0)
> -			i++;
> -	}
> +
>  	return i;
>  }
>  EXPORT_SYMBOL_GPL(of_property_count_strings);

[PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Rob Herring]

On 12/05/2011 08:23 AM, Benoit Cousson wrote:
> The current implementation just ignore any NULL string inserted in a
> multiple strings property.
> In some cases we can have a property with a fix number of strings but
> not necessarily used, like for example in a list of valid pinmux modes.
> 
>  prop = "uart_rx", "uart_tx", "", "", "safe_mode";
> 
> Do no skip NULL string and take them into account in
> of_property_read_string_index and of_property_count_strings.
> 
> Reported-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>

Applied. The others need Grant's agreement.

Rob

> ---
>  drivers/of/base.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 9b6588e..b707243 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -752,7 +752,7 @@ int of_property_read_string_index(struct device_node *np, const char *propname,
>  
>  	for (i = 0; total < prop->length; total += l, p += l) {
>  		l = strlen(p) + 1;
> -		if ((*p != 0) && (i++ == index)) {
> +		if (i++ == index) {
>  			*output = p;
>  			return 0;
>  		}
> @@ -790,11 +790,9 @@ int of_property_count_strings(struct device_node *np, const char *propname)
>  
>  	p = prop->value;
>  
> -	for (i = 0; total < prop->length; total += l, p += l) {
> +	for (i = 0; total < prop->length; total += l, p += l, i++)
>  		l = strlen(p) + 1;
> -		if (*p != 0)
> -			i++;
> -	}
> +
>  	return i;
>  }
>  EXPORT_SYMBOL_GPL(of_property_count_strings);

Re: [PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Cousson, Benoit]

Hi Grant,

On 12/19/2011 3:02 PM, Rob Herring wrote:
> On 12/05/2011 08:23 AM, Benoit Cousson wrote:
>> The current implementation just ignore any NULL string inserted in a
>> multiple strings property.
>> In some cases we can have a property with a fix number of strings but
>> not necessarily used, like for example in a list of valid pinmux modes.
>>
>>   prop = "uart_rx", "uart_tx", "", "", "safe_mode";
>>
>> Do no skip NULL string and take them into account in
>> of_property_read_string_index and of_property_count_strings.
>>
>> Reported-by: Tony Lindgren<tony@atomide.com>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: Rob Herring<rob.herring@calxeda.com>
>
> Applied. The others need Grant's agreement.

New year gentle ping.

Thanks,
Benoit

[PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Cousson, Benoit]

Hi Grant,

On 12/19/2011 3:02 PM, Rob Herring wrote:
> On 12/05/2011 08:23 AM, Benoit Cousson wrote:
>> The current implementation just ignore any NULL string inserted in a
>> multiple strings property.
>> In some cases we can have a property with a fix number of strings but
>> not necessarily used, like for example in a list of valid pinmux modes.
>>
>>   prop = "uart_rx", "uart_tx", "", "", "safe_mode";
>>
>> Do no skip NULL string and take them into account in
>> of_property_read_string_index and of_property_count_strings.
>>
>> Reported-by: Tony Lindgren<tony@atomide.com>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: Rob Herring<rob.herring@calxeda.com>
>
> Applied. The others need Grant's agreement.

New year gentle ping.

Thanks,
Benoit

Re: [PATCH 3/3] of/irq: Add interrupts-names property to name an irq resource

[Grant Likely]

On Mon, Dec 05, 2011 at 03:23:56PM +0100, Benoit Cousson wrote:
> In a HW system, resources in general have name to identify them.
> The is the case as well for IORESOURCE_IRQ resources filled by DT
> "interrupts" entries.
> The current DT mechanism is relying on the "interrupts" order to identify
> the proper resource. This is error prone and not the natural way to
> retrieve an information in general.

I do not agree with this assessment.  interrupt property order has
worked quite well for a very long time.  There are some uses cases
that want to access it by-name, but I expect accessing by-index to
continue to be the preferred method.  I'm going to drop this paragraph
from the commit text.

> Moreover, the resource does support a name and an API is available to
> allow a driver to retrieve the resource using the name instead of an
> index.
> 
> Add a interrupts-names property to allow the possiblity to provide a name
> to any interrupts entries.
> If the name is available, use it to name the resource, otherwise
> keep the legacy device full name.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> ---
>  .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
>  drivers/of/irq.c                                   |   11 ++++-
>  2 files changed, 60 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupts-names.txt b/Documentation/devicetree/bindings/interrupts-names.txt
> new file mode 100644
> index 0000000..d9a796d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupts-names.txt
> @@ -0,0 +1,50 @@
> +interrupts-names property
> +
> +In a HW system, physical resources in general have name to identify them.
> +The is the case as well for interrupt lines.
> +The current DT mechanism is relying on the "interrupts" order to identify
> +the proper resource. The interrupts-names is adding the possiblity to
> +provide a name to interrupts entries.
> +
> +Usage:
> +
> +This attribute must be used along with a regular interrupts entry. If not
> +it will be simply ignored.

This documentation is pretty much identical to the reg-names property
description.  I'll merge the two into a single file.

> +
> +
> +Example:
> +
> +
> +l4-abe {
> +	compatible = "simple-bus";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
> +		 <1 0 0x49000000 0x00001000>; /* L3 path */
> +	mcasp {
> +		compatible = "ti,mcasp";
> +		reg = <0 0x10 0x10>, <0 0x20 0x10>,
> +		      <1 0x10 0x10>, <1 0x20 0x10>;
> +		reg-names = "mpu", "dat",
> +			    "dma", "dma_dat";
> +		interrupts = <11>, <12>;
> +		interrupts-names = "rx", "tx";

Nitpick: I'm going to change the property name to "interrupt-names"
(dropping the 's' from interrupts).  After playing with the clock
binding (clocks vs. clock-names) and looking at what could be done
with the gpio binding (gpios vs. gpio-names), I think it 'feels' more
consistent to drop the s.

Merged, thanks.

> +	};
> +
> +	timer {
> +		compatible = "ti,timer";
> +		reg = <0 0x40 0x10>, <1 0x40 0x10>;
> +		reg-names = "mpu", "dma";
> +	};
> +};
> +
> +
> +usb {
> +	compatible = "ti,usb-host";
> +	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
> +	      <0x4a064c00 0x200>;
> +	reg-names = "config", "ohci", "ehci";
> +	interrupts = <14>, <15>;
> +	interrupts-names = "ohci", "ehci";
> +};
> +
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 19c0115..604b53f 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -346,9 +346,18 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>  	/* Only dereference the resource if both the
>  	 * resource and the irq are valid. */
>  	if (r && irq != NO_IRQ) {
> +		const char *name = NULL;
> +
> +		/*
> +		 * Get optional "interrupts-names" property to add a name
> +		 * to the resource.
> +		 */
> +		of_property_read_string_index(dev, "interrupts-names", index,
> +					      &name);
> +
>  		r->start = r->end = irq;
>  		r->flags = IORESOURCE_IRQ;
> -		r->name = dev->full_name;
> +		r->name = name ? name : dev->full_name;
>  	}
>  
>  	return irq;
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] of/irq: Add interrupts-names property to name an irq resource

[Grant Likely]

On Mon, Dec 05, 2011 at 03:23:56PM +0100, Benoit Cousson wrote:
> In a HW system, resources in general have name to identify them.
> The is the case as well for IORESOURCE_IRQ resources filled by DT
> "interrupts" entries.
> The current DT mechanism is relying on the "interrupts" order to identify
> the proper resource. This is error prone and not the natural way to
> retrieve an information in general.

I do not agree with this assessment.  interrupt property order has
worked quite well for a very long time.  There are some uses cases
that want to access it by-name, but I expect accessing by-index to
continue to be the preferred method.  I'm going to drop this paragraph
from the commit text.

> Moreover, the resource does support a name and an API is available to
> allow a driver to retrieve the resource using the name instead of an
> index.
> 
> Add a interrupts-names property to allow the possiblity to provide a name
> to any interrupts entries.
> If the name is available, use it to name the resource, otherwise
> keep the legacy device full name.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Rob Herring <rob.herring@calxeda.com>
> ---
>  .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
>  drivers/of/irq.c                                   |   11 ++++-
>  2 files changed, 60 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupts-names.txt b/Documentation/devicetree/bindings/interrupts-names.txt
> new file mode 100644
> index 0000000..d9a796d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupts-names.txt
> @@ -0,0 +1,50 @@
> +interrupts-names property
> +
> +In a HW system, physical resources in general have name to identify them.
> +The is the case as well for interrupt lines.
> +The current DT mechanism is relying on the "interrupts" order to identify
> +the proper resource. The interrupts-names is adding the possiblity to
> +provide a name to interrupts entries.
> +
> +Usage:
> +
> +This attribute must be used along with a regular interrupts entry. If not
> +it will be simply ignored.

This documentation is pretty much identical to the reg-names property
description.  I'll merge the two into a single file.

> +
> +
> +Example:
> +
> +
> +l4-abe {
> +	compatible = "simple-bus";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	ranges = <0 0 0x48000000 0x00001000>, /* MPU path */
> +		 <1 0 0x49000000 0x00001000>; /* L3 path */
> +	mcasp {
> +		compatible = "ti,mcasp";
> +		reg = <0 0x10 0x10>, <0 0x20 0x10>,
> +		      <1 0x10 0x10>, <1 0x20 0x10>;
> +		reg-names = "mpu", "dat",
> +			    "dma", "dma_dat";
> +		interrupts = <11>, <12>;
> +		interrupts-names = "rx", "tx";

Nitpick: I'm going to change the property name to "interrupt-names"
(dropping the 's' from interrupts).  After playing with the clock
binding (clocks vs. clock-names) and looking at what could be done
with the gpio binding (gpios vs. gpio-names), I think it 'feels' more
consistent to drop the s.

Merged, thanks.

> +	};
> +
> +	timer {
> +		compatible = "ti,timer";
> +		reg = <0 0x40 0x10>, <1 0x40 0x10>;
> +		reg-names = "mpu", "dma";
> +	};
> +};
> +
> +
> +usb {
> +	compatible = "ti,usb-host";
> +	reg = <0x4a064000 0x800>, <0x4a064800 0x200>,
> +	      <0x4a064c00 0x200>;
> +	reg-names = "config", "ohci", "ehci";
> +	interrupts = <14>, <15>;
> +	interrupts-names = "ohci", "ehci";
> +};
> +
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 19c0115..604b53f 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -346,9 +346,18 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>  	/* Only dereference the resource if both the
>  	 * resource and the irq are valid. */
>  	if (r && irq != NO_IRQ) {
> +		const char *name = NULL;
> +
> +		/*
> +		 * Get optional "interrupts-names" property to add a name
> +		 * to the resource.
> +		 */
> +		of_property_read_string_index(dev, "interrupts-names", index,
> +					      &name);
> +
>  		r->start = r->end = irq;
>  		r->flags = IORESOURCE_IRQ;
> -		r->name = dev->full_name;
> +		r->name = name ? name : dev->full_name;
>  	}
>  
>  	return irq;
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Grant Likely]

On Tue, Jan 03, 2012 at 03:15:05PM +0100, Cousson, Benoit wrote:
> Hi Grant,
> 
> On 12/19/2011 3:02 PM, Rob Herring wrote:
> >On 12/05/2011 08:23 AM, Benoit Cousson wrote:
> >>The current implementation just ignore any NULL string inserted in a
> >>multiple strings property.
> >>In some cases we can have a property with a fix number of strings but
> >>not necessarily used, like for example in a list of valid pinmux modes.
> >>
> >>  prop = "uart_rx", "uart_tx", "", "", "safe_mode";
> >>
> >>Do no skip NULL string and take them into account in
> >>of_property_read_string_index and of_property_count_strings.
> >>
> >>Reported-by: Tony Lindgren<tony@atomide.com>
> >>Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> >>Cc: Grant Likely<grant.likely@secretlab.ca>
> >>Cc: Rob Herring<rob.herring@calxeda.com>
> >
> >Applied. The others need Grant's agreement.
> 
> New year gentle ping.

merged both; should appear in linux-next tomorrow.

g.

[PATCH 1/3] of/base: Take NULL string into account for property with multiple strings

[Grant Likely]

On Tue, Jan 03, 2012 at 03:15:05PM +0100, Cousson, Benoit wrote:
> Hi Grant,
> 
> On 12/19/2011 3:02 PM, Rob Herring wrote:
> >On 12/05/2011 08:23 AM, Benoit Cousson wrote:
> >>The current implementation just ignore any NULL string inserted in a
> >>multiple strings property.
> >>In some cases we can have a property with a fix number of strings but
> >>not necessarily used, like for example in a list of valid pinmux modes.
> >>
> >>  prop = "uart_rx", "uart_tx", "", "", "safe_mode";
> >>
> >>Do no skip NULL string and take them into account in
> >>of_property_read_string_index and of_property_count_strings.
> >>
> >>Reported-by: Tony Lindgren<tony@atomide.com>
> >>Signed-off-by: Benoit Cousson<b-cousson@ti.com>
> >>Cc: Grant Likely<grant.likely@secretlab.ca>
> >>Cc: Rob Herring<rob.herring@calxeda.com>
> >
> >Applied. The others need Grant's agreement.
> 
> New year gentle ping.

merged both; should appear in linux-next tomorrow.

g.

Re: [PATCH 3/3] of/irq: Add interrupts-names property to name an irq resource

[Cousson, Benoit]

Hi Grant,

On 1/4/2012 8:17 AM, Grant Likely wrote:
> On Mon, Dec 05, 2011 at 03:23:56PM +0100, Benoit Cousson wrote:
>> In a HW system, resources in general have name to identify them.
>> The is the case as well for IORESOURCE_IRQ resources filled by DT
>> "interrupts" entries.
>> The current DT mechanism is relying on the "interrupts" order to identify
>> the proper resource. This is error prone and not the natural way to
>> retrieve an information in general.
>
> I do not agree with this assessment.  interrupt property order has
> worked quite well for a very long time.  There are some uses cases
> that want to access it by-name, but I expect accessing by-index to
> continue to be the preferred method.  I'm going to drop this paragraph
> from the commit text.

OK, fair enough. My point was when the HW description is providing names 
to list several IRQ resources, giving a number instead is error prone. 
If, on the other side, the spec is providing something like IRQ_A, B or 
C, then yes, the by-index is the natural one.

Considering that most IPs will anyway only have one IRQ line, the 
by-index method will be the most widely used.

>> Moreover, the resource does support a name and an API is available to
>> allow a driver to retrieve the resource using the name instead of an
>> index.
>>
>> Add a interrupts-names property to allow the possiblity to provide a name
>> to any interrupts entries.
>> If the name is available, use it to name the resource, otherwise
>> keep the legacy device full name.
>>
>> Signed-off-by: Benoit Cousson<b-cousson-l0cyMroinI0@public.gmane.org>
>> Cc: Grant Likely<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> Cc: Rob Herring<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
>>   drivers/of/irq.c                                   |   11 ++++-
>>   2 files changed, 60 insertions(+), 1 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupts-names.txt b/Documentation/devicetree/bindings/interrupts-names.txt
>> new file mode 100644
>> index 0000000..d9a796d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupts-names.txt
>> @@ -0,0 +1,50 @@
>> +interrupts-names property
>> +
>> +In a HW system, physical resources in general have name to identify them.
>> +The is the case as well for interrupt lines.
>> +The current DT mechanism is relying on the "interrupts" order to identify
>> +the proper resource. The interrupts-names is adding the possiblity to
>> +provide a name to interrupts entries.
>> +
>> +Usage:
>> +
>> +This attribute must be used along with a regular interrupts entry. If not
>> +it will be simply ignored.
>
> This documentation is pretty much identical to the reg-names property
> description.  I'll merge the two into a single file.

OK.

>
>> +
>> +
>> +Example:
>> +
>> +
>> +l4-abe {
>> +	compatible = "simple-bus";
>> +	#address-cells =<2>;
>> +	#size-cells =<1>;
>> +	ranges =<0 0 0x48000000 0x00001000>, /* MPU path */
>> +		<1 0 0x49000000 0x00001000>; /* L3 path */
>> +	mcasp {
>> +		compatible = "ti,mcasp";
>> +		reg =<0 0x10 0x10>,<0 0x20 0x10>,
>> +		<1 0x10 0x10>,<1 0x20 0x10>;
>> +		reg-names = "mpu", "dat",
>> +			    "dma", "dma_dat";
>> +		interrupts =<11>,<12>;
>> +		interrupts-names = "rx", "tx";
>
> Nitpick: I'm going to change the property name to "interrupt-names"
> (dropping the 's' from interrupts).  After playing with the clock
> binding (clocks vs. clock-names) and looking at what could be done
> with the gpio binding (gpios vs. gpio-names), I think it 'feels' more
> consistent to drop the s.

Fully agree, I made a comment on that in the cover-letter because I was 
not fully happy with that either.

> Merged, thanks.

Cool, thanks.

Regards,
Benoit

[PATCH 3/3] of/irq: Add interrupts-names property to name an irq resource

[Cousson, Benoit]

Hi Grant,

On 1/4/2012 8:17 AM, Grant Likely wrote:
> On Mon, Dec 05, 2011 at 03:23:56PM +0100, Benoit Cousson wrote:
>> In a HW system, resources in general have name to identify them.
>> The is the case as well for IORESOURCE_IRQ resources filled by DT
>> "interrupts" entries.
>> The current DT mechanism is relying on the "interrupts" order to identify
>> the proper resource. This is error prone and not the natural way to
>> retrieve an information in general.
>
> I do not agree with this assessment.  interrupt property order has
> worked quite well for a very long time.  There are some uses cases
> that want to access it by-name, but I expect accessing by-index to
> continue to be the preferred method.  I'm going to drop this paragraph
> from the commit text.

OK, fair enough. My point was when the HW description is providing names 
to list several IRQ resources, giving a number instead is error prone. 
If, on the other side, the spec is providing something like IRQ_A, B or 
C, then yes, the by-index is the natural one.

Considering that most IPs will anyway only have one IRQ line, the 
by-index method will be the most widely used.

>> Moreover, the resource does support a name and an API is available to
>> allow a driver to retrieve the resource using the name instead of an
>> index.
>>
>> Add a interrupts-names property to allow the possiblity to provide a name
>> to any interrupts entries.
>> If the name is available, use it to name the resource, otherwise
>> keep the legacy device full name.
>>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: Rob Herring<rob.herring@calxeda.com>
>> ---
>>   .../devicetree/bindings/interrupts-names.txt       |   50 ++++++++++++++++++++
>>   drivers/of/irq.c                                   |   11 ++++-
>>   2 files changed, 60 insertions(+), 1 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/interrupts-names.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupts-names.txt b/Documentation/devicetree/bindings/interrupts-names.txt
>> new file mode 100644
>> index 0000000..d9a796d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupts-names.txt
>> @@ -0,0 +1,50 @@
>> +interrupts-names property
>> +
>> +In a HW system, physical resources in general have name to identify them.
>> +The is the case as well for interrupt lines.
>> +The current DT mechanism is relying on the "interrupts" order to identify
>> +the proper resource. The interrupts-names is adding the possiblity to
>> +provide a name to interrupts entries.
>> +
>> +Usage:
>> +
>> +This attribute must be used along with a regular interrupts entry. If not
>> +it will be simply ignored.
>
> This documentation is pretty much identical to the reg-names property
> description.  I'll merge the two into a single file.

OK.

>
>> +
>> +
>> +Example:
>> +
>> +
>> +l4-abe {
>> +	compatible = "simple-bus";
>> +	#address-cells =<2>;
>> +	#size-cells =<1>;
>> +	ranges =<0 0 0x48000000 0x00001000>, /* MPU path */
>> +		<1 0 0x49000000 0x00001000>; /* L3 path */
>> +	mcasp {
>> +		compatible = "ti,mcasp";
>> +		reg =<0 0x10 0x10>,<0 0x20 0x10>,
>> +		<1 0x10 0x10>,<1 0x20 0x10>;
>> +		reg-names = "mpu", "dat",
>> +			    "dma", "dma_dat";
>> +		interrupts =<11>,<12>;
>> +		interrupts-names = "rx", "tx";
>
> Nitpick: I'm going to change the property name to "interrupt-names"
> (dropping the 's' from interrupts).  After playing with the clock
> binding (clocks vs. clock-names) and looking at what could be done
> with the gpio binding (gpios vs. gpio-names), I think it 'feels' more
> consistent to drop the s.

Fully agree, I made a comment on that in the cover-letter because I was 
not fully happy with that either.

> Merged, thanks.

Cool, thanks.

Regards,
Benoit