Re: [PATCH v2 8/9] serial: 8250: add acpi_match

[Greg Kroah-Hartman]

On Fri, Feb 12, 2016 at 08:43:39PM +0300, Aleksey Makarov wrote:
> From: Mark Salter <msalter@xxxxxxxxxx>
> 
> Add an implementation of acpi_match() to the 8250 driver.
> It allows console to check if it matches the console specified by
> ACPI SPCR table.
> 
> Signed-off-by: Mark Salter <msalter@xxxxxxxxxx>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@xxxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_core.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 7775221..059338a 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -40,6 +40,7 @@
>  #ifdef CONFIG_SPARC
>  #include <linux/sunserialcore.h>
>  #endif
> +#include <linux/acpi.h>
>  
>  #include <asm/irq.h>
>  
> @@ -669,12 +670,34 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>  	return -ENODEV;
>  }
>  
> +static int __init univ8250_console_acpi_match(struct console *co,
> +					      struct acpi_table_spcr *spcr)
> +{
> +	int index = co->index >= 0 ? co->index : 0;
> +	struct uart_port *port = &serial8250_ports[index].port;
> +
> +	if (spcr->interface_type != ACPI_DBG2_16550_SUBSET &&
> +	    spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE)
> +		return -ENODEV;
> +
> +	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> +	    spcr->serial_port.address == (u64)port->mapbase)
> +		return 0;
> +
> +	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
> +	    spcr->serial_port.address == (u64)port->iobase)
> +		return 0;
> +
> +	return -ENODEV;
> +}

Ick, I don't like this, you are doing almost the same thing that
univ8250_console_match does, yet you have to know the internals of the
acpi_table_spcr structure, which is insane (again, do you want to do
this for every firmware type?)

Isn't this something that resources are supposed to handle for us?  What
happened to getting this type of information from the firmware in a
non-firmware-specific way to pass to the drivers?  Please work on that
instead, then you don't have to modify each and every single driver you
want to work with your system, they will all "just work" automagically.

Please redo this series, as it is, I don't like it at all.

thanks,

greg k-h