On Tue, Jun 6, 2017 at 3:54 AM, Andreas F?rber [off-list ref] 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