Re: [PATCH v4 16/28] tty: serial: owl: Implement console driver

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Jun 6, 2017 at 3:54 AM, Andreas Färber <afaerber@suse.de> wrote:
> Implement serial console driver to complement earlycon.
>
> Based on LeMaker linux-actions tree.

> +#define OWL_UART_CTL_DWLS_MASK         (0x3 << 0)

GENMASK() ?

> +#define OWL_UART_CTL_PRS_MASK          (0x7 << 4)

Ditto.

>  #define OWL_UART_STAT_TRFL_MASK                (0x1f << 11)

Ditto.

> +static struct owl_uart_port *owl_uart_ports[OWL_UART_PORT_NUM];

And this is needed for...?

> +static void owl_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +}

Do we need an empty stub?

> +static void owl_uart_send_chars(struct uart_port *port)
> +{

> +               xmit->tail = (xmit->tail + 1) & (SERIAL_XMIT_SIZE - 1);

% SERIAL_XMIT_SIZE shorter (I hope it's a power of 2), but it's up to you.

> +}

> +static irqreturn_t owl_uart_irq(int irq, void *dev_id)
> +{
> +       struct uart_port *port = (struct uart_port *)dev_id;

Useless casting.

> +       spin_lock(&port->lock);

spin_lock_irqsave() ?

Consider the kernel command option that switches all IRQ handlers to
be threaded.

> +}

> +static void owl_uart_change_baudrate(struct owl_uart_port *owl_port,
> +                                    unsigned long baud)
> +{
> +       clk_set_rate(owl_port->clk, baud * 8);
> +}

> +static void owl_uart_release_port(struct uart_port *port)
> +{
> +       struct platform_device *pdev = to_platform_device(port->dev);
> +       struct resource *res;
> +

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return;
> +
> +       if (port->flags & UPF_IOREMAP) {
> +               devm_release_mem_region(port->dev, port->mapbase,
> +                       resource_size(res));
> +               devm_iounmap(port->dev, port->membase);
> +               port->membase = NULL;
> +       }

Above is entirely redundant.

> +}
> +
> +static int owl_uart_request_port(struct uart_port *port)
> +{
> +       struct platform_device *pdev = to_platform_device(port->dev);
> +       struct resource *res;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENXIO;
> +
> +       if (!devm_request_mem_region(port->dev, port->mapbase,
> +                       resource_size(res), dev_name(port->dev)))
> +               return -EBUSY;
> +
> +       if (port->flags & UPF_IOREMAP) {
> +               port->membase = devm_ioremap_nocache(port->dev, port->mapbase,
> +                               resource_size(res));
> +               if (!port->membase)
> +                       return -EBUSY;
> +       }
> +
> +       return 0;
> +}

> +static void owl_uart_config_port(struct uart_port *port, int flags)
> +{

> +       if (flags & UART_CONFIG_TYPE) {

if (!(...))
 return;

?

> +               port->type = PORT_OWL;
> +               owl_uart_request_port(port);
> +       }
> +}


> +static int owl_uart_probe(struct platform_device *pdev)
> +{
> +       struct resource *res_mem, *res_irq;
> +       struct owl_uart_port *owl_port;
> +       int ret;
> +
> +       if (pdev->dev.of_node)
> +               pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
> +
> +       if (pdev->id < 0 || pdev->id >= OWL_UART_PORT_NUM) {
> +               dev_err(&pdev->dev, "id %d out of range\n", pdev->id);
> +               return -EINVAL;
> +       }
> +


> +       res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res_mem) {
> +               dev_err(&pdev->dev, "could not get mem\n");
> +               return -ENODEV;
> +       }

You can use

struct resource *mem;

mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
x = devm_ioremap_resource();
if (IS_ERR(x))
 return PTR_ERR(x);

and remote IOREMAP flag below.

> +       res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       if (!res_irq) {
> +               dev_err(&pdev->dev, "could not get irq\n");
> +               return -ENODEV;
> +       }

platform_get_irq()

> +       if (owl_uart_ports[pdev->id]) {
> +               dev_err(&pdev->dev, "port %d already allocated\n", pdev->id);
> +               return -EBUSY;
> +       }

I guess it's redundant. It should be conflicting by resource (for example, IRQ).

> +       owl_port->clk = clk_get(&pdev->dev, NULL);

devm_ ?

> +       owl_port->port.iotype = UPIO_MEM;
> +       owl_port->port.mapbase = res_mem->start;
> +       owl_port->port.irq = res_irq->start;


> +       owl_port->port.uartclk = clk_get_rate(owl_port->clk);

You need to check if it's 0 and use device property instead or bail out.

> +       owl_port->port.flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
> +       owl_port->port.x_char = 0;
> +       owl_port->port.fifosize = 16;
> +       owl_port->port.ops = &owl_uart_ops;
> +
> +       owl_uart_ports[pdev->id] = owl_port;

> +       platform_set_drvdata(pdev, owl_port);

It should be just before return 0, right?..

> +
> +       ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
> +       if (ret)
> +               owl_uart_ports[pdev->id] = NULL;

...and thus, taking into consideration redundancy of that global var:

      ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
      if (ret)
          retrun ret;

      platform_set_drvdata(pdev, owl_port);
      return 0;

> +       return ret;
> +}

> +
> +static int owl_uart_remove(struct platform_device *pdev)
> +{

> +       struct owl_uart_port *owl_port;
> +
> +       owl_port = platform_get_drvdata(pdev);

Do above in one line.

> +       uart_remove_one_port(&owl_uart_driver, &owl_port->port);

> +       owl_uart_ports[pdev->id] = NULL;

Redundant.

> +
> +       return 0;
> +}

> +/* Actions Semi Owl UART */
> +#define PORT_OWL       117

We can use holes for now IIUC.

See 36 or alike

-- 
With Best Regards,
Andy Shevchenko