Re: [Xen-devel] [PATCH v2] ns16550: Add ACPI support for ARM only

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2020 04:44, Wei Xu wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1620,6 +1620,66 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>   DT_DEVICE_END
>   
>   #endif /* HAS_DEVICE_TREE */
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)

Blank line above here please.

> +#include <xen/acpi.h>
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct acpi_table_spcr *spcr;
> +    acpi_status status;
> +
> +    /* Same as the DT part.

Comment style (again below). Also there shouldn't be a blank line
until after _all_ declarations.

> +     * Only support one UART on ARM which happen to be ns16550_com[0].
> +     */
> +    struct ns16550 *uart = &ns16550_com[0];
> +
> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +                            (struct acpi_table_header **)&spcr);

Please avoid casts like this. Use more type-safe constructs like
container_of() instead.

> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk("ns16550: Failed to get SPCR table\n");

Is such a message warranted? I.e. wouldn't it trigger on all
systems not having the table, which is hardly what you/we want?

> +        return -EINVAL;

Also, is it really an error if there's no such table?

> +    }
> +
> +    ns16550_init_common(uart);
> +
> +    /* The baud rate is pre-configured by the firmware.
> +     * And currently the ACPI part is only targeting ARM so some fields
> +     * like PCI, flow control and so on we do not care yet are ignored.
> +     */

I'm no convinced though you can ignore some other fields. In
particular on v1 I recall pointing out that the GAS structure
has more fields you should look at. (Overall I'm not happy
with "and so on" here - please list all fields you mean to
ignore so that reviewers as well as future readers can judge
whether this is appropriate.)

> +    uart->baud = BAUD_AUTO;
> +    uart->data_bits = 8;
> +    uart->parity = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;
> +    uart->io_base = spcr->serial_port.address;
> +    uart->io_size = 8;
> +    uart->reg_shift = spcr->serial_port.bit_offset;
> +    uart->reg_width = 1;
> +
> +    /* The 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;
> +
> +    /*  Register with generic serial driver. */

Stray double blanks at the beginning of the comment.

> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);

I guess it's fine this way, but with "uart = &ns16550_com[0]" above
the construct looks more complicated than it needs to look.

Jan

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