lkml.org 
[lkml]   [2017]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4 16/28] tty: serial: owl: Implement console driver
    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

    \
     
     \ /
      Last update: 2017-06-12 01:07    [W:4.032 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site