[PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Timur Tabi]

Add two functions: of_find_next_property() returns the next in a list of
properties for a given node.  It's handy when you want a list of properties
for a given node, but you don't know what the properties are called.
of_get_aliased_node() looks up the aliases in the "/aliases" node and returns
the index of the property that points to a given node.  That is, if you have
an alias "serial2" that points to a serial port node, of_get_aliased_index()
returns "2".

Signed-off-by: Timur Tabi <timur@freescale.com>
---

This patch is for 2.6.27.  It applies on top of Kumar's powerpc-next branch.

 arch/powerpc/kernel/prom_parse.c |   40 ++++++++++++++++++++++++++++++++++++++
 drivers/of/base.c                |   26 ++++++++++++++++++++++++
 include/asm-powerpc/prom.h       |   17 ++++++++++++++++
 include/linux/of.h               |    3 ++
 4 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/prom_parse.c b/arch/powerpc/kernel/prom_parse.c
index 90eb3a3..a09635e 100644
--- a/arch/powerpc/kernel/prom_parse.c
+++ b/arch/powerpc/kernel/prom_parse.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/ioport.h>
 #include <linux/etherdevice.h>
+#include <linux/ctype.h>
 #include <asm/prom.h>
 #include <asm/pci-bridge.h>
 
@@ -1079,3 +1080,42 @@ void __iomem *of_iomap(struct device_node *np, int index)
 	return ioremap(res.start, 1 + res.end - res.start);
 }
 EXPORT_SYMBOL(of_iomap);
+
+int of_get_aliased_index(struct device_node *np)
+{
+	struct device_node *aliases = of_find_node_by_path("/aliases");
+	struct property *pp = NULL;
+	int index = -ENODEV;
+
+	if (!np || !aliases)
+		return -ENODEV;
+
+	while ((pp = of_find_next_property(aliases, pp)) != NULL) {
+		if (of_find_node_by_path(pp->value) == np) {
+			const char *p = pp->name;
+			unsigned long result;
+
+			while (isalpha(*p))
+				p++;
+
+			if (!*p) {
+				index = 0;
+				break;
+			}
+
+			if (strict_strtoul(p, 10, &result)) {
+				index = -EINVAL;
+				break;
+			}
+
+			index = (int) result;
+			break;
+		}
+	}
+
+	of_node_put(aliases);
+
+	return index;
+}
+EXPORT_SYMBOL(of_get_aliased_index);
+
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 23ffb7c..5a644d9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -385,3 +385,29 @@ struct device_node *of_find_matching_node(struct device_node *from,
 	return np;
 }
 EXPORT_SYMBOL(of_find_matching_node);
+
+/**
+ * Return the first or next property within a node.
+ * @np: the node
+ * @prev: if NULL, return 1st prop, otherwise return the prop after 'prev'
+ *
+ * This function is used to get a list of properties within a node.  It's
+ * also useful for when you don't know the name of the propety you want to
+ * find.
+ */
+struct property *of_find_next_property(const struct device_node *np,
+	struct property *prev)
+{
+	struct property *next;
+
+	if (!np)
+		return NULL;
+
+	read_lock(&devtree_lock);
+	next = prev ? prev->next : np->properties;
+	read_unlock(&devtree_lock);
+
+	return next;
+}
+EXPORT_SYMBOL(of_find_next_property);
+
diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
index 78b7b0d..dddf814 100644
--- a/include/asm-powerpc/prom.h
+++ b/include/asm-powerpc/prom.h
@@ -330,6 +330,23 @@ extern int of_irq_to_resource(struct device_node *dev, int index,
  */
 extern void __iomem *of_iomap(struct device_node *device, int index);
 
+/**
+ * Return the index of the alias for the given node
+ * @np: the device node for which an index is sought
+ *
+ * This function scans that /aliases node for an alias that points to the
+ * given node.  It then parses the property to obtain the index (the number
+ * at the end of the property name).
+ *
+ * This allows device drivers to use the /aliases node to enumerate their
+ * devices, without needing a new property like "cell-index" or "device-id".
+ *
+ * We assume the alias property is of the format "xxxn", where "xxx" is a
+ * string of only alpha characters, and "n" is a number greater than or
+ * equal to 0. If "n" doesn't exist, we assume that the index is zero.
+ */
+int of_get_aliased_index(struct device_node *np);
+
 /*
  * NB:  This is here while we transition from using asm/prom.h
  * to linux/of.h
diff --git a/include/linux/of.h b/include/linux/of.h
index 59a61bd..98e90d4 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -71,4 +71,7 @@ extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(
 	const struct of_device_id *matches, const struct device_node *node);
 
+struct property *of_find_next_property(const struct device_node *np,
+	struct property *prev);
+
 #endif /* _LINUX_OF_H */
-- 
1.5.5

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Segher Boessenkool]

> of_get_aliased_node() looks up the aliases in the "/aliases" node and 
> returns
> the index of the property that points to a given node.  That is, if 
> you have
> an alias "serial2" that points to a serial port node, 
> of_get_aliased_index()
> returns "2".

It returns 2 for both "i2c-1" and "i2c-2".

Also, alias names do not have any significance in general, they are
just handy shortcut names for humans to use; it would be better not
to overload this.  What do you want to use this for?


Segher

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Timur Tabi]

Segher Boessenkool wrote:

> It returns 2 for both "i2c-1" and "i2c-2".

Well, I'm assuming that the alias property names will follow the current
convention of xxxxnn where xxxx is a name and nn is a number.  No dashes or
other punctuation.

> Also, alias names do not have any significance in general, they are
> just handy shortcut names for humans to use; it would be better not
> to overload this.  What do you want to use this for?

As an alternative to cell-index or device-id for enumerating devices.  The
consensus from the '"cell-index" vs. "index" vs. no index in I2C device nodes'
thread is that aliases are to be used to enumerate devices.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Segher Boessenkool]

>> It returns 2 for both "i2c-1" and "i2c-2".
>
> Well, I'm assuming that the alias property names will follow the 
> current
> convention of xxxxnn where xxxx is a name and nn is a number.  No 
> dashes or
> other punctuation.

Well, yes, your suggested code doesn't allow punctuation either; but
that wasn't my point, it doesn't allow numbers in names.  Why don't
you just parse a number from the end?

>> Also, alias names do not have any significance in general, they are
>> just handy shortcut names for humans to use; it would be better not
>> to overload this.  What do you want to use this for?
>
> As an alternative to cell-index or device-id for enumerating devices.  
> The
> consensus from the '"cell-index" vs. "index" vs. no index in I2C 
> device nodes'
> thread is that aliases are to be used to enumerate devices.

Erm, no.  That wasn't the consensus as I remember it; besides, it's
not a good plan.

Pretty much all busses can be enumerated without anything like this.
There was consensus on this.

Overloading "cell-index" is a bad plan.  There was consensus on this,
as well.

The only thing a platform should ever use aliases for is if it needs
to (for whatever purpose) find a specific device, that it cannot
identify otherwise (via "reg", ...).  And then that platform code
should look up the device by the alias, not look up the alias by the
device -- there is no 1-1 mapping from device to alias!


Segher

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Josh Boyer]

On Wed, 2008-06-25 at 15:05 -0500, Timur Tabi wrote:
> +/**
> + * Return the first or next property within a node.
> + * @np: the node
> + * @prev: if NULL, return 1st prop, otherwise return the prop after 'prev'
> + *
> + * This function is used to get a list of properties within a node.  It's
> + * also useful for when you don't know the name of the propety you want to
> + * find.

How is this useful when you don't know the name of the property you want
to find?  I don't understand that at all.

I'm not really sure what good having a list of the properties within a
node is either, outside of using it for debugging.

josh

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Benjamin Herrenschmidt]

> +struct property *of_find_next_property(const struct device_node *np,
> +	struct property *prev)
> +{
> +	struct property *next;
> +
> +	if (!np)
> +		return NULL;
> +
> +	read_lock(&devtree_lock);
> +	next = prev ? prev->next : np->properties;
> +	read_unlock(&devtree_lock);
> +
> +	return next;
> +}

Unfortunately, this isn't race free vs. removal or addition of
properties. This is fine with the intended usage (ie. retreiving
aliases) but at least that should be explained in a comment.

Cheers,
Ben.

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Timur Tabi]

Segher Boessenkool wrote:

> Well, yes, your suggested code doesn't allow punctuation either; but
> that wasn't my point, it doesn't allow numbers in names.  Why don't
> you just parse a number from the end?

Oh, you mean start at the end and go backwards?  Hmmm, I guess I could do that.

> The only thing a platform should ever use aliases for is if it needs
> to (for whatever purpose) find a specific device, that it cannot
> identify otherwise (via "reg", ...).  And then that platform code
> should look up the device by the alias, not look up the alias by the
> device -- there is no 1-1 mapping from device to alias!

Hmmm, I hadn't through about that.  I guess this patch isn't such a great idea
after all.  I rescind it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Stefan Roese]

On Thursday 26 June 2008, Timur Tabi wrote:
> > The only thing a platform should ever use aliases for is if it needs
> > to (for whatever purpose) find a specific device, that it cannot
> > identify otherwise (via "reg", ...).  And then that platform code
> > should look up the device by the alias, not look up the alias by the
> > device -- there is no 1-1 mapping from device to alias!
>
> Hmmm, I hadn't through about that.  I guess this patch isn't such a great
> idea after all.  I rescind it.

Too bad. So now we're back to where we started with the discussion 
about "cell-index" vs. "index" vs. no index on I2C device nodes. :-(

Best regards,
Stefan

Re: [PATCH] powerpc: add of_find_next_property and of_get_aliased_index

[Timur Tabi]

Stefan Roese wrote:
> On Thursday 26 June 2008, Timur Tabi wrote:
>>> The only thing a platform should ever use aliases for is if it needs
>>> to (for whatever purpose) find a specific device, that it cannot
>>> identify otherwise (via "reg", ...).  And then that platform code
>>> should look up the device by the alias, not look up the alias by the
>>> device -- there is no 1-1 mapping from device to alias!
>> Hmmm, I hadn't through about that.  I guess this patch isn't such a great
>> idea after all.  I rescind it.
> 
> Too bad. So now we're back to where we started with the discussion 
> about "cell-index" vs. "index" vs. no index on I2C device nodes. :-(

Well, there's a lot of disagreement on this subject.  Not only do we not agree
on a method of enumerating devices, a lot of people have a problem with the
concept of enumerating them in the first place!

This whole thing started with a problem I had in ASoC V2: identifying an I2C
device by name and number.  Scott W. pointed out that all I need in my "fabric
driver" is a pointer to the i2c_adapter structure that the I2C driver was using.
 If we create a link from the I2C device node to its matching i2c_adapter
structure, then I won't care what the adapter/bus number is.  Unfortunately, it
appears the current I2C code in fsl_soc.c can't handle that, but of_i2c.c can.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: add of_find_next_property andof_get_aliased_index

[Sean MacLennan]

On Thu, 26 Jun 2008 10:55:14 -0500
"Timur Tabi" <timur@freescale.com> wrote:

> Well, there's a lot of disagreement on this subject.  Not only do we
> not agree on a method of enumerating devices, a lot of people have a
> problem with the concept of enumerating them in the first place!

An interesting point is that I enforced an index in the i2c-ibm_iic
driver with no disagreement at all ;)

Cheers,
   Sean

Re: [PATCH] powerpc: add of_find_next_property andof_get_aliased_index

[Timur Tabi]

Sean MacLennan wrote:
> On Thu, 26 Jun 2008 10:55:14 -0500
> "Timur Tabi" <timur@freescale.com> wrote:
> 
>> Well, there's a lot of disagreement on this subject.  Not only do we
>> not agree on a method of enumerating devices, a lot of people have a
>> problem with the concept of enumerating them in the first place!
> 
> An interesting point is that I enforced an index in the i2c-ibm_iic
> driver with no disagreement at all ;)

I added it to the fsl_soc as well without any fuss, but I may need to withdraw
it.  Technically, the I2C nodes in our device trees should not have a cell-index
property, because there are no shared registers.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: add of_find_next_property andof_get_aliased_index

[Stefan Roese]

On Thursday 26 June 2008, Sean MacLennan wrote:
> > Well, there's a lot of disagreement on this subject.  Not only do we
> > not agree on a method of enumerating devices, a lot of people have a
> > problem with the concept of enumerating them in the first place!
>
> An interesting point is that I enforced an index in the i2c-ibm_iic
> driver with no disagreement at all ;)

You have been lucky I suppose. :)

I could easily just have used this existing "index" property for the other 4xx 
boards, but expected NAK's for this. That and because FSL uses "cell-index" 
is why I asked prior to sending patches.

Now I have no idea how to support I2C on the other 4xx boards. Perhaps Josh 
could advise how this should be done?

Best regards,
Stefan

Re: [PATCH] powerpc: add of_find_next_property andof_get_aliased_index

[David Gibson]

On Thu, Jun 26, 2008 at 08:41:12PM +0200, Stefan Roese wrote:
> On Thursday 26 June 2008, Sean MacLennan wrote:
> > > Well, there's a lot of disagreement on this subject.  Not only do we
> > > not agree on a method of enumerating devices, a lot of people have a
> > > problem with the concept of enumerating them in the first place!
> >
> > An interesting point is that I enforced an index in the i2c-ibm_iic
> > driver with no disagreement at all ;)
> 
> You have been lucky I suppose. :)

Ah... that's because IIC does have a correct reason to have cell-index
- like most of the 4xx devices, we may need the index for programming
the CPM power control retisters - and we didn't notice you were also
using for the incorrect purpose of supplying an index to the i2c
layer.

Please note that cell-index *does* *not* *work* for the global index
if there are multiple SoC-like units in the system.  For its correct
purpose of indexing the CPM registers, cell-index must be local to the
SoC, for the global index it must not.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] powerpc: add of_find_next_property andof_get_aliased_index

[Josh Boyer]

On Thu, 26 Jun 2008 20:41:12 +0200
Stefan Roese <sr@denx.de> wrote:

> On Thursday 26 June 2008, Sean MacLennan wrote:
> > > Well, there's a lot of disagreement on this subject.  Not only do we
> > > not agree on a method of enumerating devices, a lot of people have a
> > > problem with the concept of enumerating them in the first place!
> >
> > An interesting point is that I enforced an index in the i2c-ibm_iic
> > driver with no disagreement at all ;)
> 
> You have been lucky I suppose. :)
> 
> I could easily just have used this existing "index" property for the other 4xx 
> boards, but expected NAK's for this. That and because FSL uses "cell-index" 
> is why I asked prior to sending patches.
> 
> Now I have no idea how to support I2C on the other 4xx boards. Perhaps Josh 
> could advise how this should be done?

As David said elsewhere, cell-index is fine for figuring out how to
access the CPM registers, etc.  But it's not good for enumerating
across the whole system.

For I2C specifically, I think Sean already has a patch to switch the
4xx driver to not use the numbered functions, which eliminates the need
for the enumeration all together.  That seems like the right approach
to me.

josh

Re: [PATCH] powerpc: add of_find_next_property andof_get_aliased_index

[Stefan Roese]

On Thursday 03 July 2008, Josh Boyer wrote:
> > Now I have no idea how to support I2C on the other 4xx boards. Perhaps
> > Josh could advise how this should be done?
>
> As David said elsewhere, cell-index is fine for figuring out how to
> access the CPM registers, etc.  But it's not good for enumerating
> across the whole system.
>
> For I2C specifically, I think Sean already has a patch to switch the
> 4xx driver to not use the numbered functions, which eliminates the need
> for the enumeration all together.  That seems like the right approach
> to me.

Yes. I'm waiting for Sean's next patch version... :)

Best regards,
Stefan