On Tue, Jun 6, 2017 at 3:54 AM, Andreas Färber <afaerber@xxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
