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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut