linux-kernel - Re: [PATCH v2] serial: 8250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z5OtFqtf4dpvvc9v@smile.fi.intel.com>
Date: Fri, 24 Jan 2025 17:09:10 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Yenchia Chen <yenchia.chen@...iatek.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org,
	matthias.bgg@...il.com, angelogioacchino.delregno@...labora.com,
	yj.chiang@...iatek.com, tbergstrom@...dia.com,
	treapking@...omium.org, u.kleine-koenig@...libre.com,
	yujiaoliang@...o.com, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2] serial: 8250_mtk: Add ACPI support

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