Re: [PATCH v2 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

[Philipp Zabel]

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