Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Joel Stanley]

Hi Andy,

Thanks for the review. I've incorporatd most of your comments in a v2
that I'll send out once I've given it a spin on hardware.

On Sun, Apr 2, 2017 at 10:37 PM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>> +static ssize_t ast_vuart_show_addr(struct device *dev,
>> +               struct device_attribute *attr, char *buf)
>> +{
>> +       struct ast_vuart *vuart = dev_get_drvdata(dev);
>> +       u16 addr;
>> +
>
>> +       addr = (readb(vuart->regs + AST_VUART_ADDRH) << 8) |
>> +               (readb(vuart->regs + AST_VUART_ADDRL));
>
> It looks like you have register shift 2 bits and byte accessors. We
> have some helpers for that (serial_in() / serial_out() or alike).

Thanks for the pointer. I took a look at this. It looks like I need to
define my own accessor?

I don't think it's worth it for the one read and one write we have in
this driver.

>
>> +
>> +       return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
>> +}
>> +

>> +static int ast_vuart_probe(struct platform_device *pdev)
>> +{
>> +       struct uart_8250_port port;
>> +       struct resource resource;
>> +       struct ast_vuart *vuart;
>> +       struct device_node *np;
>> +       u32 clk, prop;
>> +       int rc;
>> +
>
>> +       np = pdev->dev.of_node;
>
> And if np == NULL?

The driver will fail to probe due to the of_property_read_u32 calls
returning an error.

>> +       /* If current-speed was set, then try not to change it. */
>> +       if (of_property_read_u32(np, "current-speed", &prop) == 0)
>> +               port.port.custom_divisor = clk / (16 * prop);
>> +
>> +       /* Check for shifted address mapping */
>> +       if (of_property_read_u32(np, "reg-offset", &prop) == 0)
>> +               port.port.mapbase += prop;
>> +
>> +       /* Check for registers offset within the devices address range */
>> +       if (of_property_read_u32(np, "reg-shift", &prop) == 0)
>> +               port.port.regshift = prop;
>> +
>> +       /* Check for fifo size */
>> +       if (of_property_read_u32(np, "fifo-size", &prop) == 0)
>> +               port.port.fifosize = prop;
>
> Perhaps you need other way around, check for error and supply a
> default in such case.

We leave port.fifosize unmodified (set to zero) if there is no valid
device tree property. As the property is optional, it's not an error
if it's not present.

>> +
>> +       /* Check for a fixed line number */
>> +       rc = of_alias_get_id(np, "serial");
>> +       if (rc >= 0)
>> +               port.port.line = rc;
>> +
>> +       port.port.irq = irq_of_parse_and_map(np, 0);
>
>> +       port.port.irqflags = IRQF_SHARED;
>
> This is set by core. You already supplied correct flag for that below.

By setting UPF_SHARE_IRQ the core does correctly requset_irq with
IRQF_SHARED set. However, it does not store this in in port->irqflags,
so other tests in eg. serial8250_do_startup that test for IRQF_SHARED
will fail. This is a bug that we hit the other day.

Would you like a patch to the core that either tests for
UPF_SHARE_IRQ, or set IRQF_SHARED early on?

>
>> +       port.port.iotype = UPIO_MEM;
>> +       if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
>
> You hide an error code from of_property_read_u32() here. Why?

The property is optional, so if it doesn't exist we want to continue
without error.

We return EINVAL if the property is invalid, as the device tree code
will give us ENODATA or EOVERFLOW, which I don't think is informative
for a driver to return.

> And if there is an error you are continuing with what? 0?

we continue with port.port.iotype = UPIO_MEM from above.

>> +               switch (prop) {
>> +               case 1:
>> +                       port.port.iotype = UPIO_MEM;
>> +                       break;
>> +               case 4:
>> +                       port.port.iotype = of_device_is_big_endian(np) ?
>> +                                      UPIO_MEM32BE : UPIO_MEM32;
>> +                       break;
>> +               default:
>> +                       dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
>> +                                prop);
>> +                       rc = -EINVAL;
>> +                       goto err_clk_disable;
>> +               }
>> +       }

Cheers,

Joel