Hi, On Wed, 2021-11-10 at 15:51 +0800, Hammer Hsieh wrote: [...] > +struct sunplus_uart_port { > + char name[16]; > + struct uart_port uport; > + struct sunplus_dma_info *uartdma_rx; > + struct sunplus_dma_info *uartdma_tx; > + struct clk *clk; > + struct reset_control *rstc; > + unsigned int pllsys_rate; > + struct gpio_desc *rts_gpio; > + struct hrtimer rts_check_tx_empty; > + struct hrtimer rts_delay_before_send; > + struct hrtimer rts_delay_after_send; > +}; > +struct sunplus_uart_port sunplus_uart_ports[UART_NR]; Does this have to be a global array? I would expect these to be allocated in the probe function, one at a time. [...] > +static int sunplus_uart_probe(struct platform_device *pdev) > +{ > + struct resource *res_mem; > + struct uart_port *port; > + struct clk *clk, *pllsys; > + unsigned int pllsys_rate; > + int ret, irq; > + int idx_offset, idx; > + int idx_which_uart; > + char peri_name[16]; > + > + if (pdev->dev.of_node) { > + pdev->id = of_alias_get_id(pdev->dev.of_node, "serial"); > + if (pdev->id < 0) > + pdev->id = of_alias_get_id(pdev->dev.of_node, "uart"); > + } > + > + idx_offset = -1; > + > + if (IS_UARTDMARX_ID(pdev->id)) > + idx_offset = 0; > + else if (IS_UARTDMATX_ID(pdev->id)) > + idx_offset = UART_DMARX_NR; > + > + /* init txdma or rxdma */ > + if (idx_offset >= 0) { > + clk = devm_clk_get(&pdev->dev, NULL); Should this be requested by name? Looking at the binding, this could be UADMA or HWUA? > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + return ret; I suggest to move this down after all required resources are available. Otherwise you'll have to either disable the clock in the error paths, or you are left with a running clock if anything below fails. > + if (idx_offset == 0) > + idx = idx_offset + pdev->id - ID_BASE_DMARX; > + else > + idx = idx_offset + pdev->id - ID_BASE_DMATX; > + > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res_mem) > + return -ENODEV; > + > + sprintf(peri_name, "PERI%d", (idx & 0x01)); > + > + clk = devm_clk_get(&pdev->dev, peri_name); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + ret = clk_prepare_enable(clk); Same as above. > + if (ret) > + return ret; > + > + sunplus_uartdma[idx].addr_phy = > + (unsigned long)(res_mem->start); > + sunplus_uartdma[idx].membase = > + devm_ioremap_resource(&pdev->dev, res_mem); > + > + if (IS_ERR(sunplus_uartdma[idx].membase)) > + return PTR_ERR(sunplus_uartdma[idx].membase); > + > + if (IS_UARTDMARX_ID(pdev->id)) { > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return -ENODEV; > + > + sunplus_uartdma[idx].irq = irq; > + } else { > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res_mem) > + return -ENODEV; > + > + sunplus_uartdma[idx].gdma_membase = > + devm_ioremap_resource(&pdev->dev, res_mem); > + > + if (IS_ERR(sunplus_uartdma[idx].gdma_membase)) > + return -EINVAL; > + } > + > + if (of_property_read_u32(pdev->dev.of_node, "which-uart", &idx_which_uart) != 0) { > + dev_err(&pdev->dev, "\"which-uart\" is not assigned."); > + return -EINVAL; > + } > + > + if (idx_which_uart >= UART_NR) { > + dev_err(&pdev->dev, "\"which-uart\" is not valid."); > + return -EINVAL; > + } > + > + sunplus_uartdma[idx].which_uart = idx_which_uart; > + > + return 0; > + } else if (pdev->id < 0 || pdev->id >= UART_NR) > + return -EINVAL; > + > + /* init uart */ > + port = &sunplus_uart_ports[pdev->id].uport; > + if (port->membase) > + return -EBUSY; > + > + memset(port, 0, sizeof(*port)); > + > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res_mem) > + return -ENODEV; > + > + port->dev = &pdev->dev; > + port->mapbase = res_mem->start; > + > + port->membase = devm_ioremap_resource(&pdev->dev, res_mem); > + if (IS_ERR(port->membase)) > + return PTR_ERR(port->membase); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return -ENODEV; > + > + uart_get_rs485_mode(port); > + sunplus_uart_ports[pdev->id].rts_gpio = > + devm_gpiod_get(&pdev->dev, "rts", GPIOD_OUT_LOW); > + port->rs485_config = sunplus_uart_config_rs485; > + sunplus_uart_ports[pdev->id].rts_check_tx_empty.function = NULL; > + sunplus_uart_ports[pdev->id].rts_delay_before_send.function = NULL; > + sunplus_uart_ports[pdev->id].rts_delay_after_send.function = NULL; > + if (port->rs485.flags & SER_RS485_ENABLED) > + sunplus_uart_rs485_on(port); > + > + sunplus_uart_ports[pdev->id].clk = devm_clk_get(&pdev->dev, NULL); Here the same nameless clock as in the loop above is requested again. Should this be UADMA or HWUA? > + if (IS_ERR(sunplus_uart_ports[pdev->id].clk)) > + return PTR_ERR(sunplus_uart_ports[pdev->id].clk); > + > + ret = clk_prepare_enable(sunplus_uart_ports[pdev->id].clk); > + if (ret) > + return ret; Same comment as above. Better to request the reset control before enabling the clock, for example. > + > + sunplus_uart_ports[pdev->id].rstc = > + devm_reset_control_get(&pdev->dev, NULL); Please use devm_reset_control_get_exclusive() instead. > + > + if (IS_ERR(sunplus_uart_ports[pdev->id].rstc)) > + return PTR_ERR(sunplus_uart_ports[pdev->id].rstc); > + > + ret = reset_control_deassert(sunplus_uart_ports[pdev->id].rstc); > + if (ret) > + return ret; > + > + clk = sunplus_uart_ports[pdev->id].clk; > + if (IS_ERR(clk)) This can't ever be true, the code above already returned in this case. [...] > +static int sunplus_uart_remove(struct platform_device *pdev) > +{ > +#ifdef CONFIG_PM_RUNTIME_UART > + if (pdev->id != 0) { > + pm_runtime_disable(&pdev->dev); > + pm_runtime_set_suspended(&pdev->dev); > + } > +#endif > + uart_remove_one_port(&sunplus_uart_driver, > + &sunplus_uart_ports[pdev->id].uport); > + > + if (pdev->id < UART_NR) { > + clk_disable_unprepare(sunplus_uart_ports[pdev->id].clk); > + reset_control_assert(sunplus_uart_ports[pdev->id].rstc); > + } What about the PERI clocks? This seems to leave them enabled. regards Philipp