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

[Jiri Slaby <jirislaby@kernel.org>]

On 14. 01. 25, 4:33, Yenchia Chen wrote:
> Add ACPI support to 8250_mtk driver. This makes it possible to
> use uart with edk2 UEFI firmware.

Could you mention what hardware this is in particular?

> Signed-off-by: Yenchia Chen <yenchia.chen@mediatek.com>
> ---
>   drivers/tty/serial/8250/8250_mtk.c | 31 ++++++++++++++++++++++++------
>   1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index b44de2ed7413..a4f1c30f77b0 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -19,6 +19,7 @@
>   #include <linux/dma-mapping.h>
>   #include <linux/tty.h>
>   #include <linux/tty_flip.h>
> +#include <linux/acpi.h>

Sort this properly.

>   #include "8250.h"
>   
> @@ -519,6 +520,7 @@ static int mtk8250_probe(struct platform_device *pdev)
>   	struct mtk8250_data *data;
>   	struct resource *regs;
>   	int irq, err;
> +	acpi_handle hdl = ACPI_HANDLE(&pdev->dev);

'hdl' sounds very weird and is not used in the tree for acpi_handles. 
I'd use 'acpi_handle' or 'acpi_dev_handle' in this case.

>   	irq = platform_get_irq(pdev, 0);
>   	if (irq < 0)
> @@ -545,8 +547,12 @@ static int mtk8250_probe(struct platform_device *pdev)
>   		err = mtk8250_probe_of(pdev, &uart.port, data);
>   		if (err)
>   			return err;
> -	} else
> -		return -ENODEV;
> +	} else {
> +		if (!hdl) {

so this should be:
   } else if () {

> +			dev_err(&pdev->dev, "no device\n");

Why this?

> +			return -ENODEV;

As this is self explanatory already, right?

> +		}
> +	}
>   
>   	spin_lock_init(&uart.port.lock);
>   	uart.port.mapbase = regs->start;
> @@ -560,16 +566,22 @@ static int mtk8250_probe(struct platform_device *pdev)
>   	uart.port.private_data = data;
>   	uart.port.shutdown = mtk8250_shutdown;
>   	uart.port.startup = mtk8250_startup;
> -	uart.port.set_termios = mtk8250_set_termios;
> -	uart.port.uartclk = clk_get_rate(data->uart_clk);
> +	if (hdl) {
> +		uart.port.uartclk = 26000000;

This is a magic constant. Define a macro for this. Hint: 26 * HZ_PER_MHZ.

Is it not/cannot it be part of the acpi table? What does MTKI0511 look like?

> +	} else {
> +		uart.port.set_termios = mtk8250_set_termios;
> +		uart.port.uartclk = clk_get_rate(data->uart_clk);
> +	}
>   #ifdef CONFIG_SERIAL_8250_DMA
>   	if (data->dma)
>   		uart.dma = data->dma;
>   #endif
>   
> -	/* Disable Rate Fix function */
> -	writel(0x0, uart.port.membase +
> +	if (!hdl) {
> +		/* Disable Rate Fix function */

Why is this only for non-ACPI devices?

> +		writel(0x0, uart.port.membase +
>   			(MTK_UART_RATE_FIX << uart.port.regshift));
> +	}
>   
>   	platform_set_drvdata(pdev, data);
>   
> @@ -647,11 +659,18 @@ static const struct of_device_id mtk8250_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, mtk8250_of_match);
>   
> +static const struct acpi_device_id mtk8250_acpi_match[] = {
> +	{ "MTKI0511", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, mtk8250_acpi_match);
> +
>   static struct platform_driver mtk8250_platform_driver = {
>   	.driver = {
>   		.name		= "mt6577-uart",
>   		.pm		= &mtk8250_pm_ops,
>   		.of_match_table	= mtk8250_of_match,
> +		.acpi_match_table = ACPI_PTR(mtk8250_acpi_match),
>   	},
>   	.probe			= mtk8250_probe,
>   	.remove			= mtk8250_remove,

thanks,
-- 
js
suse labs