Messages in this thread | | | Date | Wed, 24 May 2023 16:34:24 +0800 | Subject | Re: [PATCH v11 10/10] tty: serial: Add Nuvoton ma35d1 serial driver support | From | Jacky Huang <> |
| |
Dear Jiri,
Thanks for your advice.
On 2023/5/24 下午 03:42, Jiri Slaby wrote: > On 16. 05. 23, 9:52, Jacky Huang wrote: >> +static void ma35d1serial_config_port(struct uart_port *port, int flags) >> +{ >> + /* >> + * Driver core for serial ports forces a non-zero value for port >> type. >> + * Write an arbitrary value here to accommodate the serial core >> driver, >> + * as ID part of UAPI is redundant. >> + */ >> + port->type = 1; > > So this 1 translates to PORT_8250. Why not to use it directly? Or > something more saner like PORT_16550A? > It's not actually 8250 or 16550A. Can we add the following definition to 'include/uapi/linux/serial_core.h' and use PORT_MA35 instead?
#define PORT_MA35 124
>> +} >> + >> +static int ma35d1serial_verify_port(struct uart_port *port, struct >> serial_struct *ser) >> +{ >> + if (port->type != PORT_UNKNOWN && ser->type != 1) >> + return -EINVAL; >> + >> + return 0; >> +} > ... >> +static int __init ma35d1serial_console_setup(struct console *co, >> char *options) >> +{ >> + struct device_node *np = ma35d1serial_uart_nodes[co->index]; >> + struct uart_ma35d1_port *p = &ma35d1serial_ports[co->index]; >> + u32 val32[4]; >> + struct uart_port *port; >> + int baud = 115200; >> + int bits = 8; >> + int parity = 'n'; >> + int flow = 'n'; >> + >> + /* >> + * Check whether an invalid uart number has been specified, and > > You dereferenced ma35d1serial_uart_nodes already. Doesn't > console=ttyNVT1000 (or something like that) crash the system? >
I will add the following check before np = "ma35d1serial_uart_nodes[co->index]".
if (co->index < 0 || co->index >= MA35_UART_NR) return -EINVAL;
>> + * if so, search for the first available port that does have >> + * console support. > > The code below doesn't match this comment.
Yes, I will remove the above comment.
> >> + */ >> + if ((co->index < 0) || (co->index >= MA35_UART_NR)) { >> + pr_debug("Console Port%x out of range\n", co->index); >> + return -EINVAL; >> + } >> + >> + if (of_property_read_u32_array(np, "reg", val32, 4) != 0) > > Shouldn't that 4 be ARRAY_SIZE(val32) instead? >
Will be fixed.
>> + return -EINVAL; > > One \n here please. >
Okay, I will add it.
>> + p->port.iobase = val32[1]; >> + p->port.membase = ioremap(p->port.iobase, MA35_UART_REG_SIZE); > > What if this fails? >
I will add a check for the return value.
>> + p->port.ops = &ma35d1serial_ops; >> + p->port.line = 0; >> + p->port.uartclk = MA35_UART_CONSOLE_CLK; >> + >> + port = &ma35d1serial_ports[co->index].port; > > Isn't this: > port = &p->port; > ? > > Either use port on all above lines or drop the "port" variable > completely and use "p->port" below instead. >
I will remove port variable and use p->port only.
>> + >> + if (options) >> + uart_parse_options(options, &baud, &parity, &bits, &flow); >> + >> + return uart_set_options(port, co, baud, parity, bits, flow); >> +} >> + >> +static struct console ma35d1serial_console = { >> + .name = "ttyNVT", >> + .write = ma35d1serial_console_write, >> + .device = uart_console_device, >> + .setup = ma35d1serial_console_setup, >> + .flags = CON_PRINTBUFFER | CON_ENABLED, >> + .index = -1, >> + .data = &ma35d1serial_reg, > > I don't see console->data used anywhere in the driver? >
I will remove it.
>> +}; > ... >> +static int ma35d1serial_probe(struct platform_device *pdev) >> +{ >> + struct resource *res_mem; >> + struct uart_ma35d1_port *up; >> + int ret = 0; >> + >> + if (pdev->dev.of_node) { >> + ret = of_alias_get_id(pdev->dev.of_node, "serial"); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to get alias/pdev id, errno >> %d\n", ret); >> + return ret; >> + } >> + } >> + up = &ma35d1serial_ports[ret]; >> + up->port.line = ret; >> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res_mem) >> + return -ENODEV; >> + >> + up->port.iobase = res_mem->start; >> + up->port.membase = ioremap(up->port.iobase, MA35_UART_REG_SIZE); > > Check this too. >
Okay, sure.
>> + up->port.ops = &ma35d1serial_ops; >> + >> + spin_lock_init(&up->port.lock); >> + >> + up->clk = of_clk_get(pdev->dev.of_node, 0); >> + if (IS_ERR(up->clk)) { >> + ret = PTR_ERR(up->clk); >> + dev_err(&pdev->dev, "failed to get core clk: %d\n", ret); >> + goto err_iounmap; >> + } >> + >> + ret = clk_prepare_enable(up->clk); >> + if (ret) >> + goto err_iounmap; >> + >> + if (up->port.line != 0) >> + up->port.uartclk = clk_get_rate(up->clk); >> + >> + ret = platform_get_irq(pdev, 0); >> + if (ret < 0) >> + goto err_clk_disable; >> + >> + up->port.irq = ret; >> + up->port.dev = &pdev->dev; >> + up->port.flags = UPF_BOOT_AUTOCONF; >> + >> + platform_set_drvdata(pdev, up); >> + >> + ret = uart_add_one_port(&ma35d1serial_reg, &up->port); >> + if (ret < 0) >> + goto err_free_irq; >> + >> + return 0; >> + >> +err_free_irq: >> + free_irq(up->port.irq, &up->port); >> + >> +err_clk_disable: >> + clk_disable_unprepare(up->clk); >> + >> +err_iounmap: >> + iounmap(up->port.membase); >> + return ret; >> +} > > thanks,
Best regards, Jacky Huang
| |