On Tue, Jun 21, 2011 at 01:13:50PM -0600, Grant Likely wrote:

On Tue, Jun 21, 2011 at 7:55 AM, Shawn Guo [off-list ref] wrote:

quoted

Hi Grant,

I just gave a try to use aliases node for identify the device index.
Please take a look and let me know if it's what you expect.

Thanks Shawn.  This is definitely on track.  Comments below...

quoted

On Sun, Jun 19, 2011 at 03:30:02PM +0800, Shawn Guo wrote:
[...]

quoted

quoted

quoted

+#ifdef CONFIG_OF
+static int serial_imx_probe_dt(struct imx_port *sport,
+ ? ? ? ? struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ const __be32 *line;
+
+ if (!node)
+ ? ? ? ? return -ENODEV;
+
+ line = of_get_property(node, "id", NULL);
+ if (!line)
+ ? ? ? ? return -ENODEV;
+
+ sport->port.line = be32_to_cpup(line) - 1;

Hmmm, I really would like to be rid of this. ?Instead, if uarts must
be enumerated, the driver should look for a /aliases/uart* property
that matches the of_node. ?Doing it that way is already established in
the OpenFirmware documentation, and it ensures there are no overlaps
in the global namespace.

I just gave one more try to avoid using 'aliases', and you gave a
'no' again. ?Now, I know how hard you are on this. ?Okay, I start
thinking about your suggestion seriously :)

quoted

We do need some infrastructure to make that easier though. ?Would you
have time to help put that together?

Ok, I will give it a try.

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
index da0381a..f4a5c3c 100644
--- a/arch/arm/boot/dts/imx51-babbage.dts
+++ b/arch/arm/boot/dts/imx51-babbage.dts

@@ -18,6 +18,12 @@

? ? ? ?compatible = "fsl,imx51-babbage", "fsl,imx51";
? ? ? ?interrupt-parent = <&tzic>;

+ ? ? ? aliases {
+ ? ? ? ? ? ? ? serial0 = &uart0;
+ ? ? ? ? ? ? ? serial1 = &uart1;
+ ? ? ? ? ? ? ? serial2 = &uart2;
+ ? ? ? };
+

Hmmm.  David Gibson had tossed out an idea of automatically generating
aliases from labels.  It may be time to revisit that idea.

David, perhaps using this format for a label should turn it into an
alias (prefix label with an asterisk):

        *thealias: i2c at 12340000 { /*...*/ };

.... although that approach gets *really* hairy when considering that
different boards will want different enumeration.  How would one
override an automatic alias defined by an included .dts file?

Another dependency the patch has to wait for?  Or we can go ahead and
utilize the facility later when it gets ready?

quoted

? ? ? ?chosen {
? ? ? ? ? ? ? ?bootargs = "console=ttymxc0,115200 root=/dev/mmcblk0p3 rootwait";
? ? ? ?};

@@ -47,29 +53,29 @@

? ? ? ? ? ? ? ? ? ? ? ?reg = <0x70000000 0x40000>;
? ? ? ? ? ? ? ? ? ? ? ?ranges;

- ? ? ? ? ? ? ? ? ? ? ? uart at 7000c000 {
+ ? ? ? ? ? ? ? ? ? ? ? uart2: uart at 7000c000 {
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?compatible = "fsl,imx51-uart", "fsl,imx21-uart";
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?reg = <0x7000c000 0x4000>;
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?interrupts = <33>;
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?id = <3>;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? fsl,has-rts-cts;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? fsl,uart-has-rtscts;
? ? ? ? ? ? ? ? ? ? ? ?};
? ? ? ? ? ? ? ?};

- ? ? ? ? ? ? ? uart at 73fbc000 {
+ ? ? ? ? ? ? ? uart0: uart at 73fbc000 {
? ? ? ? ? ? ? ? ? ? ? ?compatible = "fsl,imx51-uart", "fsl,imx21-uart";
? ? ? ? ? ? ? ? ? ? ? ?reg = <0x73fbc000 0x4000>;
? ? ? ? ? ? ? ? ? ? ? ?interrupts = <31>;
? ? ? ? ? ? ? ? ? ? ? ?id = <1>;
- ? ? ? ? ? ? ? ? ? ? ? fsl,has-rts-cts;
+ ? ? ? ? ? ? ? ? ? ? ? fsl,uart-has-rtscts;
? ? ? ? ? ? ? ?};

- ? ? ? ? ? ? ? uart at 73fc0000 {
+ ? ? ? ? ? ? ? uart1: uart at 73fc0000 {
? ? ? ? ? ? ? ? ? ? ? ?compatible = "fsl,imx51-uart", "fsl,imx21-uart";
? ? ? ? ? ? ? ? ? ? ? ?reg = <0x73fc0000 0x4000>;
? ? ? ? ? ? ? ? ? ? ? ?interrupts = <32>;
? ? ? ? ? ? ? ? ? ? ? ?id = <2>;
- ? ? ? ? ? ? ? ? ? ? ? fsl,has-rts-cts;
+ ? ? ? ? ? ? ? ? ? ? ? fsl,uart-has-rtscts;
? ? ? ? ? ? ? ?};
? ? ? ?};

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 632ebae..13df5d2 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c

@@ -737,6 +737,37 @@ err0:

?EXPORT_SYMBOL(of_parse_phandles_with_args);

?/**
+ * ? ? of_get_device_index - Get device index by looking up "aliases" node
+ * ? ? @np: ? ?Pointer to device node that asks for device index
+ * ? ? @name: ?The device alias without index number
+ *
+ * ? ? Returns the device index if find it, else returns -ENODEV.
+ */
+int of_get_device_index(struct device_node *np, const char *alias)
+{
+ ? ? ? struct device_node *aliases = of_find_node_by_name(NULL, "aliases");
+ ? ? ? struct property *prop;
+ ? ? ? char name[32];
+ ? ? ? int index = 0;
+
+ ? ? ? if (!aliases)
+ ? ? ? ? ? ? ? return -ENODEV;
+
+ ? ? ? while (1) {
+ ? ? ? ? ? ? ? snprintf(name, sizeof(name), "%s%d", alias, index);
+ ? ? ? ? ? ? ? prop = of_find_property(aliases, name, NULL);
+ ? ? ? ? ? ? ? if (!prop)
+ ? ? ? ? ? ? ? ? ? ? ? return -ENODEV;
+ ? ? ? ? ? ? ? if (np == of_find_node_by_path(prop->value))
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? index++;
+ ? ? ? }

Rather than parsing the alias strings everytime, it would probably be
better to preprocess all the properties in the aliases node and create
a lookup table of alias->node references that can be walked quickly
and trivially.

Ok, I'm thinking about it.  Will probably ask something on details
later.

Also, when obtaining an enumeration for a device, you'll need to be
careful about what number gets returned.  If the node doesn't match a
given alias, but aliases do exist for other devices of like type, then
you need to be careful not to assign a number already assigned to
another device via an alias (this of course assumes the driver
supports dynamics enumeration, which many drivers will).  It would be

Some words not finished?

\> +

quoted

+ ? ? ? return index;
+}
+EXPORT_SYMBOL(of_get_device_index);
+
+/**
?* prom_add_property - Add a property to a node
?*/
?int prom_add_property(struct device_node *np, struct property *prop)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index da436e0..852668f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c

@@ -1271,18 +1271,18 @@ static int serial_imx_probe_dt(struct imx_port *sport,

? ? ? ?struct device_node *node = pdev->dev.of_node;
? ? ? ?const struct of_device_id *of_id =
? ? ? ? ? ? ? ? ? ? ? ?of_match_device(imx_uart_dt_ids, &pdev->dev);
- ? ? ? const __be32 *line;
+ ? ? ? int line;

? ? ? ?if (!node)
? ? ? ? ? ? ? ?return -ENODEV;

- ? ? ? line = of_get_property(node, "id", NULL);
- ? ? ? if (!line)
+ ? ? ? line = of_get_device_index(node, "serial");
+ ? ? ? if (IS_ERR_VALUE(line))
? ? ? ? ? ? ? ?return -ENODEV;

Personally, it an alias isn't present, then I'd dynamically assign a port id.

We probably can not.  The driver works with being told the correct
port id which is defined by soc.  Instead of dynamically assigning
a port id, we have to tell the driver the exact hardware port id of
the device that is being probed.

-- 
Regards,
Shawn