Re: [Xen-devel] [PATCH v3 1/2] xen/arm: Add MVEBU UART driver for Marvell Armada 3700 SoC

[Julien Grall <julien.grall@xxxxxxx>]

(+Juergen)

Hi,

On 04/05/2018 11:16 AM, Amit Singh Tomar wrote:
> This patch adds driver for UART controller found on Armada 3700 SoC.
>
> There is no reference manuals available for 3700 SoC in public and it
> is derived by looking at Linux driver[1].
>
> [1]https://github.com/torvalds/linux/blob/master/drivers/tty/serial/mvebu-uart.c
>
> Signed-off-by: Amit Singh Tomar <amittomer25@xxxxxxxxx>
> ---
> Changes since v2:
>      * Addressed Andre's comments.
> Changes since v1:
>      * Addressed Wei Liu's comments
>      * Addressed Andre's comments.
> Changes since RFC:
>      * Addressed Julien's comments.
> ---
>   xen/drivers/char/Kconfig      |   8 ++
>   xen/drivers/char/Makefile     |   1 +
>   xen/drivers/char/mvebu-uart.c | 296 ++++++++++++++++++++++++++++++++++++++++++
As I said on the first version, I think this should fold under "ARM" as 
all the arch specific UART driver (PL011 & co).
I was expecting this patch to modify MAINTAINERS. I would be ok to 
commit like that but please send a patch to address this issue as soon 
as possible.
[...]

> +static void __init mvebu3700_uart_init_postirq(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    if ( uart->irq > 0 )
> +    {
> +        uart->irqaction.handler = mvebu3700_uart_interrupt;
> +        uart->irqaction.name    = "mvebu3700_uart";
> +        uart->irqaction.dev_id  = port;
> +    }
> +
> +    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
> +    {
> +        printk("Failed to allocated mvebu3700_uart IRQ %d\n", uart->irq);
> +        return;
> +    }
This code looks wrong. You only setup the action when uart->irq is > 0, 
but you unconditionally call setup_irq.
Other drivers are calling setup_irq(...) when uart->irq > 0.

However, this driver seems to assume the interrupt will always be there 
as you enable the interrupt below and the initialized would bail out if 
platform_irq() fails.
So I think you want to drop uart->irq.

[...]

> +static int __init mvebu3700_irq(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +
> +    return (uart->irq > 0) ? uart->irq : -1;
Same remark here.

[...]

> +static int mvebu3700_uart_tx_ready(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = mvebu3700_read(uart, UART_STATUS_REG);
> +
> +    if ( reg & STATUS_TXFIFO_EMP )
> +        return TX_FIFO_SIZE;
> +    if ( reg & STAT_TX_FIFO_FUL )
> +        return 0;
> +    if ( reg & STAT_TX_FIFO_HFL )
> +        return TX_FIFO_SIZE / 2;
> +
> +    /* if we reach here, we don't know the number of free char in FIFO
Coding style:

/*
 *
 */

The rest of the code looks good. Please resend the patch with that 
addressed.
Cheers,

--
Julien Grall

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