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

[Simon Glass]

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.

Regards,
Simon