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
