From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Wed, 30 Nov 2016 04:16:02 +0100 Subject: [U-Boot] [PATCH 05/14] serial: 16550: Add port type as driver data In-Reply-To: References: <20161125223235.3434-1-marex@denx.de> <20161125223235.3434-5-marex@denx.de> <026837e5-b6da-3b05-bf38-5fbecf8b3ca3@denx.de> Message-ID: <06a9a5cc-33be-6cc2-c539-78cf7b98b24b@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 11/30/2016 04:10 AM, Simon Glass wrote: > Hi Marek, > > On 29 November 2016 at 20:04, Marek Vasut wrote: >> On 11/30/2016 03:26 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 29 November 2016 at 18:27, Marek Vasut wrote: >>>> On 11/30/2016 01:32 AM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 27 November 2016 at 10:07, Marek Vasut wrote: >>>>>> On 11/27/2016 06:03 PM, Simon Glass wrote: >>>>>>> Hi Marex, >>>>>>> >>>>>>> On 25 November 2016 at 15:32, Marek Vasut 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 >>>>>>>> Cc: Tom Rini >>>>>>>> Cc: Simon Glass >>>>>>>> --- >>>>>>>> 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