On 11/30/2016 04:10 AM, Simon Glass wrote: > Hi Marek, > > On 29 November 2016 at 20:04, Marek Vasut <ma...@denx.de> wrote: >> On 11/30/2016 03:26 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 29 November 2016 at 18:27, Marek Vasut <ma...@denx.de> wrote: >>>> On 11/30/2016 01:32 AM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 27 November 2016 at 10:07, Marek Vasut <ma...@denx.de> wrote: >>>>>> On 11/27/2016 06:03 PM, Simon Glass wrote: >>>>>>> Hi Marex, >>>>>>> >>>>>>> On 25 November 2016 at 15:32, Marek Vasut <ma...@denx.de> wrote: >>>>>>>> Add driver data to each compatible string to identify the type of >>>>>>>> the port. Since all the ports in the driver are entirely compatible >>>>>>>> with 16550 for now, all are marked with PORT_NS16550. But, there >>>>>>>> are ports which have specific quirks, like the JZ4780 UART, which >>>>>>>> do not have any DT property to denote the quirks. Instead, Linux >>>>>>>> uses the compatible string to discern such ports and enable the >>>>>>>> necessary quirks. >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut <ma...@denx.de> >>>>>>>> Cc: Tom Rini <tr...@konsulko.com> >>>>>>>> Cc: Simon Glass <s...@chromium.org> >>>>>>>> --- >>>>>>>> drivers/serial/ns16550.c | 26 ++++++++++++++++---------- >>>>>>>> 1 file changed, 16 insertions(+), 10 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >>>>>>>> index 3c9f3b0..3130a1d 100644 >>>>>>>> --- a/drivers/serial/ns16550.c >>>>>>>> +++ b/drivers/serial/ns16550.c >>>>>>>> @@ -360,6 +360,12 @@ int ns16550_serial_probe(struct udevice *dev) >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> +#if CONFIG_IS_ENABLED(OF_CONTROL) >>>>>>>> +enum { >>>>>>>> + PORT_NS16550 = 0, >>>>>>>> +}; >>>>>>>> +#endif >>>>>>>> + >>>>>>>> #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) >>>>>>>> int ns16550_serial_ofdata_to_platdata(struct udevice *dev) >>>>>>>> { >>>>>>>> @@ -453,16 +459,16 @@ const struct dm_serial_ops ns16550_serial_ops = { >>>>>>>> * compatible string to your dts. >>>>>>>> */ >>>>>>>> static const struct udevice_id ns16550_serial_ids[] = { >>>>>>>> - { .compatible = "ns16550" }, >>>>>>>> - { .compatible = "ns16550a" }, >>>>>>>> - { .compatible = "nvidia,tegra20-uart" }, >>>>>>>> - { .compatible = "snps,dw-apb-uart" }, >>>>>>>> - { .compatible = "ti,omap2-uart" }, >>>>>>>> - { .compatible = "ti,omap3-uart" }, >>>>>>>> - { .compatible = "ti,omap4-uart" }, >>>>>>>> - { .compatible = "ti,am3352-uart" }, >>>>>>>> - { .compatible = "ti,am4372-uart" }, >>>>>>>> - { .compatible = "ti,dra742-uart" }, >>>>>>>> + { .compatible = "ns16550", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ns16550a", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "nvidia,tegra20-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "snps,dw-apb-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ti,omap2-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ti,omap3-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ti,omap4-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ti,am3352-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ti,am4372-uart", .data = PORT_NS16550 }, >>>>>>>> + { .compatible = "ti,dra742-uart", .data = PORT_NS16550 }, >>>>>>> >>>>>>> But can we have 0 as the default so we don't need these values? >>>>>> >>>>>> PORT_NS16550 is zero anyway, it's just explicitly spelled here. >>>>> >>>>> One way might be to use PORT_DEFAULT, set it to 0 and omit it here. It >>>>> does seem odd to have PORT_NS16550 in the ns16550 driver. >>>> >>>> I'd rather be explicit in case we grow this list, it also doesn't hurt >>>> anything since it is implicitly set to zero. Or is this something more >>>> than a matter of preference here ? >>> >>> Well, at least rename the PORT value to DEFAULT or something like that >>> if you want to be explicit. >> >> Why DEFAULT ? It's NS16550 compatible port and the others are not >> entirely NS16550 compatible, so I think NS16550 is exactly how it should >> be named. There never was any DEFAULT label on these chips, >> but the original 16550 UART chip had 16550 written on it. > > Because the driver is called ns16550. So one can assume that this is > the base case...
Until someone refactors it or splits it. I wouldn't go with that assumption. I really dislike having some default here. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot