Hello. Grant Likely wrote:
On Thu, Mar 20, 2008 at 8:43 AM, John Linn <[EMAIL PROTECTED]> wrote:
The Xilinx 16550 uart core is not a standard 16550, because it uses word-based addressing rather than byte-based addressing. As a result, it is not compatible with the open firmware 'ns16550' compatible binding. This code introduces new bindings, which pass the correct register base and regshift properties to the uart driver to enable this core to be used. Doing this cleanly required some refactoring of the existing code.
Personally, I'm not fond of this approach. There is already some traction to using the reg-shift property to specify spacing, and I think it would be appropriate to also define a reg-offset property to handle the +3 offset and then let the xilinx 16550 nodes use those.
That's making things only worse than the mere "reg-shift" idea. I think that both are totally wrong. Everything about the programming interface should be said in the "compatible" and possibly "model" properties. of_serial driver should recognize them and pass the necessary details to 8250.c. As for me, I'm strongly against plaguing the device tree with the *Linux driver implementation specifics* (despite I was trying this with MTD -- there it seemed somewhat more grounded :-).
More comments below.
Signed-off-by: Stephen Neuendorffer <[EMAIL PROTECTED]> Signed-off-by: John Linn <[EMAIL PROTECTED]>
diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c index 2efb892..910c94f 100644 --- a/drivers/serial/of_serial.c +++ b/drivers/serial/of_serial.c
[...]
return 0; } @@ -81,8 +82,9 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev, if (info == NULL) return -ENOMEM; - port_type = (unsigned long)id->data; - ret = of_platform_serial_setup(ofdev, port_type, &port); + memcpy(info, id->data, sizeof(struct of_serial_info)); + port_type = info->type; + ret = of_platform_serial_setup(ofdev, info, &port); if (ret) goto out; @@ -100,7 +102,6 @@ static int __devinit of_platform_serial_probe(struct of_device *ofdev, if (ret < 0) goto out; - info->type = port_type; info->line = ret; ofdev->dev.driver_data = info; return 0; @@ -128,15 +129,33 @@ static int of_platform_serial_remove(struct of_device *ofdev) return 0; } +static struct of_serial_info __devinitdata ns8250_info = { .type = PORT_8250 }; +static struct of_serial_info __devinitdata ns16450_info = { .type = PORT_16450 }; +static struct of_serial_info __devinitdata ns16550_info = { .type = PORT_16550 }; +static struct of_serial_info __devinitdata ns16750_info = { .type = PORT_16750 }; +static struct of_serial_info __devinitdata xilinx_16550_info = { + .type = PORT_16550, + .regshift = 2, + .regoffset = 3,
I see that the data is already encoded in the driver itself, so I agree with the patch.
+}; +static struct of_serial_info __devinitdata unknown_info = { .type = PORT_UNKNOWN };
In support of my argument; the fact that you need a table of data says to me that this data should really be encoded in the device tree. :-)
Not at all.
Cheers, g.
WBR, Sergei _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
