Re: [PATCH v2] serial: 8250_mtk: Add ACPI support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jan 24, 2025 at 11:18:32AM +0800, Yenchia Chen wrote:
> Add ACPI support to 8250_mtk driver. This makes it possible to
> use UART on ARM-based desktops with EDK2 UEFI firmware.

...

> +#include <linux/acpi.h>

This will not be needed (see below)

>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -15,6 +16,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/serial_8250.h>
>  #include <linux/serial_reg.h>

> +#include <linux/units.h>

This might not be needed either (see below)

>  #include <linux/console.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/tty.h>

You probably want to sort header inclusions first for easier maintenance and
feature additions like this one.

Can you add that one?

...

> +	} else if (!acpi_dev_handle) {

> +	}

This should look like

	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
	...
	if (is_of_node(fwnode)) {
		...parse OF...
	} else if (!fwnode) {
		return -ENODEV;
	}

...

>  	uart.port.uartclk = clk_get_rate(data->uart_clk);
> +	if (!uart.port.uartclk)
> +		uart.port.uartclk = 26 * HZ_PER_MHZ;

Why doesn't your table provide the clock-frequency property?

What I expect to see is the use of uart_read_port_properties().

...

>  #ifdef CONFIG_SERIAL_8250_DMA
>  	if (data->dma)
>  		uart.dma = data->dma;
>  #endif

Btw, how do you handle DMA in ACPI case?

...

> -	/* Disable Rate Fix function */
> -	writel(0x0, uart.port.membase +
> +	if (!acpi_dev_handle) {
> +		/* Disable Rate Fix function */
> +		writel(0x0, uart.port.membase +
>  			(MTK_UART_RATE_FIX << uart.port.regshift));
> +	}

	if (is_of_node()) {
		...
	}

...

> +static const struct acpi_device_id mtk8250_acpi_match[] = {
> +	{ "MTKI0511", 0 },

No ', 0' part, please.

> +	{},

No comma in the terminator line.

> +};

-- 
With Best Regards,
Andy Shevchenko