[U-Boot] [PATCH 05/14] serial: 16550: Add port type as driver data

[Simon Glass]

Hi Marek,

On 29 November 2016 at 20:04, Marek Vasut <marex at denx.de> wrote:
> On 11/30/2016 03:26 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 29 November 2016 at 18:27, Marek Vasut <marex at denx.de> wrote:
>>> On 11/30/2016 01:32 AM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 27 November 2016 at 10:07, Marek Vasut <marex at denx.de> wrote:
>>>>> On 11/27/2016 06:03 PM, Simon Glass wrote:
>>>>>> Hi Marex,
>>>>>>
>>>>>> On 25 November 2016 at 15:32, Marek Vasut <marex at 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 <marex at denx.de>
>>>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>>>> Cc: Simon Glass <sjg at 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...

Regards,
Simon