Re: [Xen-devel] [PATCH] ns16550: Add ACPI support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2020 04:40, Wei Xu wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>  DT_DEVICE_END
>  
>  #endif /* HAS_DEVICE_TREE */
> +
> +#ifdef CONFIG_ACPI
> +#include <xen/acpi.h>
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct acpi_table_spcr *spcr = NULL;

The initializer isn't strictly needed, is it?

> +    acpi_status status;
> +    struct ns16550 *uart;
> +
> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +                            (struct acpi_table_header **)&spcr);
> +
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk("ns16550: Failed to get SPCR table\n");
> +        return -EINVAL;
> +    }
> +
> +    uart = &ns16550_com[0];

You want to justify the choice of what (on x86 at least= we'd call
com1 in the patch description. Also this could be the initializer
of the variable.

> +    ns16550_init_common(uart);
> +
> +    uart->baud      = BAUD_AUTO;

There's a baud_rate field in the structure. If there's a reason
to ignore it, please add a comment.

There's also an interface_type field - can you really ignore it?

> +    uart->data_bits = 8;
> +    uart->parity    = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;

There's also a flow_control field, which I think needs checking
that it matches ns16550_setup_preirq() comment:

    /* No flow ctrl: DTR and RTS are both wedged high to keep remote happy. */

Similarly any other fields you don't evaluate at all and which
aren't explained by the spec as possible to be ignored (and the
situation matching the use case, like you not caring about PCI
aspects here) need reasoning about in the description or a code
comment.

> +    uart->io_base = spcr->serial_port.address;

The field (or perhaps the whole spcr->serial_port) being zero looks
to have special meaning.

> +    uart->io_size = 8;
> +    uart->reg_shift = spcr->serial_port.bit_offset;

spcr->serial_port has other fields which I don't think you should
ignore.

> +    uart->reg_width = 1;

Please use consistent placement of = : Either all of them are
aligned, or all of them are preceded by a single space only.

> +    /* trigger/polarity information is not available in spcr */
> +    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    uart->irq = spcr->interrupt;
> +
> +    uart->vuart.base_addr = uart->io_base;
> +    uart->vuart.size = uart->io_size;
> +    uart->vuart.data_off = UART_THR << uart->reg_shift;
> +    uart->vuart.status_off = UART_LSR << uart->reg_shift;
> +    uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;

Style-wise this block should then match whatever the other
block above looks.

> +    /*  Register with generic serial driver. */
> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +
> +    return 0;
> +}
> +
> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
> +    .class_type = ACPI_DBG2_16550_COMPATIBLE,
> +    .init = ns16550_acpi_uart_init,
> +ACPI_DEVICE_END

I don't expect this to build on x86.

Finally, please follow patch submission guidelines: Patches ought to
be sent _to_ the list and maintainers be _cc_-ed.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel