[U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback

[Simon Glass]

Hi Andy,

On 15 November 2018 at 11:51, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> On Thu, Nov 15, 2018 at 9:46 PM Simon Glass <sjg at chromium.org> wrote:
>> On 15 November 2018 at 09:58, Andy Shevchenko
>> <andriy.shevchenko at linux.intel.com> wrote:
>> >
>> > New callback will give a necessary information to fill up ACPI SPCR table,
>> > for example. Maybe used later for other purposes.
>
>> Seems useful to me.
>
> Thanks. What do you think about introducing ->getconfig() at some point?

Let's do it.

>
>> Please add a test to test/dm/serial.c
>
> Will do.
>
>> > +int serial_getinfo(struct serial_device_info *info)
>>
>> This should use driver model, so:
>>
>> int serial_getinfo(struct udevice *dev, struct serial_device_info *info)
>
> Oh, sure!
>
>> > +/* REVISIT: ACPI GAS specification implied */
>> > +struct serial_device_info {
>> > +       unsigned int baudrate;
>> > +       u8      addr_space;     /* 0 - MMIO, 1 - IO */
>>
>> Please make this an enum
>>
>> > +       u8      reg_width;
>> > +       u8      reg_offset;
>> > +       u8      reg_shift;
>> > +       u64     addr;
>>
>> ulong
>>
>> Needs a struct comment as I don't know what most of these do.
>>
>> What about parity, number of bits, etc?
>
> What about splitting up some parameters structure with U-Boot defined semantics?
> In the acpi_create_spcr() we can convert it to what ACPI expects w/o
> polluting U-Boot generic code.
>
> That's why it has "REVISIT" tag, I would like to hear proposals how
> these data structures should look like. Also I have no clue about
> non-16550 drivers.

Well so long as you document the struct members I think this is fine.
But I think you should add serial-format info (the 8n1 / 7e1 business)
to this.

Regards,
Simon