Hi Ricardo,
Apologies for the delay. Comments below.
On 04/07/2016 06:02 AM, Ricardo Ribalda Delgado wrote:
> The 8250_fintek driver advertises as the PNP0501 driver; however this
> conflicts with the standard 16550A uart PNP0501. The conflict causes
> the 8250_fintek driver to load with _every_ PNP0501, but never probe,
> and causing the entire 8250 driver stack to unload if the 8250_fintek
> driver is unloaded (modprobe doesn't know that 8250_pnp rather than
> 8250_fintek claimed the resource).
>
> This patch merges the Fintek driver into 8250_base. On autoconfig_16550
> the device is probed to verify if it is a FINTEK device or not. Only the
> address ranges that belong to the ACPI device PNP0C02 are probed. If the
> vendor and product matches, the port_type is changed to PORT_F81216A.
I wouldn't use a new port type:
1. All the 8250 port types are used (see include/uapi/linux/serial_core.h)
The include/uapi/linux/serial.h is older and the serial_core.h uapi
header supercedes it for port types.
2. The port won't be recognized elsewhere as a 16550A (which the Fintek
part is compatible with).
Do you _need_ the port type for anything? It doesn't look it to me, but
I might have missed something.
> This custom probing can be disabled completely via configuration. When a
> Fintek device is not probed it will behave as a standard 16550A device,
> with no RS485 capabilities.
>
> Reported-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
> ---
>
> This patches includes the comments from Peter for patch:
>
> serial: 8250: Integrate Fintek into 8250_pnp
>
>
> drivers/tty/serial/8250/8250.h | 6 ++
> drivers/tty/serial/8250/8250_fintek.c | 190 ++++++++++++++--------------------
> drivers/tty/serial/8250/8250_port.c | 14 +++
> drivers/tty/serial/8250/Kconfig | 22 ++--
> drivers/tty/serial/8250/Makefile | 2 +-
> include/uapi/linux/serial.h | 1 +
> 6 files changed, 116 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 047a7ba6796a..52b5700a6c59 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -151,6 +151,12 @@ static inline int serial8250_pnp_init(void) { return 0; }
> static inline void serial8250_pnp_exit(void) { }
> #endif
>
> +#ifdef CONFIG_SERIAL_8250_FINTEK
> +int fintek_8250_probe(struct uart_8250_port *uart);
> +#else
> +static inline int fintek_8250_probe(struct uart_8250_port *uart) { return 0; }
> +#endif
> +
> #ifdef CONFIG_ARCH_OMAP1
> static inline int is_omap1_8250(struct uart_8250_port *pt)
> {
> diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
> index 89474399ab89..2096d87b6de9 100644
> --- a/drivers/tty/serial/8250/8250_fintek.c
> +++ b/drivers/tty/serial/8250/8250_fintek.c
> @@ -1,9 +1,7 @@
> /*
> * Probe for F81216A LPC to 4 UART
> *
> - * Based on drivers/tty/serial/8250_pnp.c, by Russell King, et al
> - *
> - * Copyright (C) 2014 Ricardo Ribalda, Qtechnology A/S
> + * Copyright (C) 2014-2016 Ricardo Ribalda, Qtechnology A/S
> *
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -15,6 +13,7 @@
> #include <linux/pnp.h>
> #include <linux/kernel.h>
> #include <linux/serial_core.h>
> +#include <linux/sfi_acpi.h>
> #include "8250.h"
>
> #define ADDR_PORT 0
> @@ -38,30 +37,44 @@
> #define RXW4C_IRA BIT(3)
> #define TXW4C_IRA BIT(2)
>
> -#define DRIVER_NAME "8250_fintek"
> -
> struct fintek_8250 {
> u16 base_port;
> u8 index;
> u8 key;
> - long line;
> };
>
> +struct fintek_8250_probe {
> + struct fintek_8250 pdata;
> + u16 io_address;
> +};
> +
> +static void _fintek_8250_enter_key(u16 base_port, u8 key)
> +{
> +
> + outb(key, base_port + ADDR_PORT);
> + outb(key, base_port + ADDR_PORT);
> +}
> +
> static int fintek_8250_enter_key(u16 base_port, u8 key)
> {
>
> - if (!request_muxed_region(base_port, 2, DRIVER_NAME))
> + if (!request_muxed_region(base_port, 2, "8250_fintek"))
> return -EBUSY;
>
> - outb(key, base_port + ADDR_PORT);
> - outb(key, base_port + ADDR_PORT);
> + _fintek_8250_enter_key(base_port, key);
> +
> return 0;
> }
>
> +static void _fintek_8250_exit_key(u16 base_port)
> +{
> + outb(EXIT_KEY, base_port + ADDR_PORT);
> +}
> +
> static void fintek_8250_exit_key(u16 base_port)
> {
>
> - outb(EXIT_KEY, base_port + ADDR_PORT);
> + return _fintek_8250_exit_key(base_port);
> release_region(base_port + ADDR_PORT, 2);
> }
>
> @@ -138,21 +151,40 @@ static int fintek_8250_rs485_config(struct uart_port *port,
> return 0;
> }
>
> -static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
> +static acpi_status check_fintek_resource(struct acpi_resource *res, void *data)
> {
> static const u16 addr[] = {0x4e, 0x2e};
> static const u8 keys[] = {0x77, 0xa0, 0x87, 0x67};
> + struct fintek_8250_probe *probe_data = data;
> int i, j, k;
>
> + if (res->type != ACPI_RESOURCE_TYPE_IO || res->length < 2)
> + return AE_OK;
> +
> +
> for (i = 0; i < ARRAY_SIZE(addr); i++) {
> + struct resource mem =
> + DEFINE_RES_IO(res->data.io.minimum, 2);
> +
> + if (addr[i] != res->data.io.minimum)
> + continue;
> +
> + /*
> + * Avoid probing on ioports requested by other
> + * devices.
> + * request_muxed_region() cannot be used here
> + * because this function is called from a non
> + * sleeping context.
> + */
> + if (!request_resource(&ioport_resource, &mem))
> + continue;
> +
> for (j = 0; j < ARRAY_SIZE(keys); j++) {
>
> - if (fintek_8250_enter_key(addr[i], keys[j]))
> - continue;
> - if (fintek_8250_check_id(addr[i])) {
> - fintek_8250_exit_key(addr[i]);
> + _fintek_8250_enter_key(addr[i], keys[j]);
> +
> + if (fintek_8250_check_id(addr[i]))
> continue;
> - }
>
> for (k = 0; k < 4; k++) {
> u16 aux;
> @@ -164,119 +196,57 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
> aux = inb(addr[i] + DATA_PORT);
> outb(IO_ADDR2, addr[i] + ADDR_PORT);
> aux |= inb(addr[i] + DATA_PORT) << 8;
> - if (aux != io_address)
> + if (aux != probe_data->io_address)
> continue;
>
> - fintek_8250_exit_key(addr[i]);
> - *key = keys[j];
> - *index = k;
> - return addr[i];
> + _fintek_8250_exit_key(addr[i]);
> + probe_data->pdata.key = keys[j];
> + probe_data->pdata.base_port = addr[i];
> + probe_data->pdata.index = k;
> +
> + return AE_CTRL_TERMINATE;
> }
> - fintek_8250_exit_key(addr[i]);
> + _fintek_8250_exit_key(addr[i]);
> }
> }
>
> - return -ENODEV;
> + return AE_OK;
> +
> }
>
> -static int
> -fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
> +static acpi_status find_fintek_resource(acpi_handle handle, u32 lvl,
> + void *context, void **rv)
> {
> - struct uart_8250_port uart;
> - struct fintek_8250 *pdata;
> - int base_port;
> - u8 key;
> - u8 index;
> + struct fintek_8250_probe *probe_data = context;
>
> - if (!pnp_port_valid(dev, 0))
> - return -ENODEV;
> -
> - base_port = fintek_8250_base_port(pnp_port_start(dev, 0), &key, &index);
> - if (base_port < 0)
> - return -ENODEV;
> -
> - memset(&uart, 0, sizeof(uart));
> -
> - pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
> - if (!pdata)
> - return -ENOMEM;
> - uart.port.private_data = pdata;
> + acpi_walk_resources(handle, METHOD_NAME__CRS,
> + check_fintek_resource, probe_data);
>
> - if (!pnp_irq_valid(dev, 0))
> - return -ENODEV;
> - uart.port.irq = pnp_irq(dev, 0);
> - uart.port.iobase = pnp_port_start(dev, 0);
> - uart.port.iotype = UPIO_PORT;
> - uart.port.rs485_config = fintek_8250_rs485_config;
> -
> - uart.port.flags |= UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
> - if (pnp_irq_flags(dev, 0) & IORESOURCE_IRQ_SHAREABLE)
> - uart.port.flags |= UPF_SHARE_IRQ;
> - uart.port.uartclk = 1843200;
> - uart.port.dev = &dev->dev;
> -
> - pdata->key = key;
> - pdata->base_port = base_port;
> - pdata->index = index;
> - pdata->line = serial8250_register_8250_port(&uart);
> - if (pdata->line < 0)
> - return -ENODEV;
> + if (probe_data->pdata.base_port)
> + return AE_CTRL_TERMINATE;
>
> - pnp_set_drvdata(dev, pdata);
> - return 0;
> + return AE_OK;
> }
>
> -static void fintek_8250_remove(struct pnp_dev *dev)
> -{
> - struct fintek_8250 *pdata = pnp_get_drvdata(dev);
>
> - if (pdata)
> - serial8250_unregister_port(pdata->line);
> -}
> -
> -#ifdef CONFIG_PM
> -static int fintek_8250_suspend(struct pnp_dev *dev, pm_message_t state)
> +int fintek_8250_probe(struct uart_8250_port *uart)
> {
> - struct fintek_8250 *pdata = pnp_get_drvdata(dev);
> + struct fintek_8250 *pdata;
> + struct fintek_8250_probe probe_data;
>
> - if (!pdata)
> + probe_data.io_address = uart->port.iobase;
> + probe_data.pdata.base_port = 0;
> + acpi_get_devices("PNP0C02", find_fintek_resource, &probe_data, NULL);
For the moment, I'd like to drop the ACPI scanning; just do the probing
on the known super i/o ports (0x2e, 0x4e) [of course, after getting the
mux region].
> + if (!probe_data.pdata.base_port)
> return -ENODEV;
> - serial8250_suspend_port(pdata->line);
> - return 0;
> -}
> -
> -static int fintek_8250_resume(struct pnp_dev *dev)
> -{
> - struct fintek_8250 *pdata = pnp_get_drvdata(dev);
>
> + pdata = devm_kzalloc(uart->port.dev, sizeof(*pdata), GFP_ATOMIC);
> if (!pdata)
> - return -ENODEV;
> - serial8250_resume_port(pdata->line);
> - return 0;
> -}
> -#else
> -#define fintek_8250_suspend NULL
> -#define fintek_8250_resume NULL
> -#endif /* CONFIG_PM */
> -
> -static const struct pnp_device_id fintek_dev_table[] = {
> - /* Qtechnology Panel PC / IO1000 */
> - { "PNP0501"},
> - {}
> -};
> + return -ENOMEM;
>
> -MODULE_DEVICE_TABLE(pnp, fintek_dev_table);
> + memcpy(pdata, &probe_data.pdata, sizeof(probe_data.pdata));
> + uart->port.rs485_config = fintek_8250_rs485_config;
> + uart->port.private_data = pdata;
>
> -static struct pnp_driver fintek_8250_driver = {
> - .name = DRIVER_NAME,
> - .probe = fintek_8250_probe,
> - .remove = fintek_8250_remove,
> - .suspend = fintek_8250_suspend,
> - .resume = fintek_8250_resume,
> - .id_table = fintek_dev_table,
> -};
> -
> -module_pnp_driver(fintek_8250_driver);
> -MODULE_DESCRIPTION("Fintek F812164 module");
> -MODULE_AUTHOR("Ricardo Ribalda <ricardo.ribalda@xxxxxxxxx>");
> -MODULE_LICENSE("GPL");
> + return 0;
> +}
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e213da01a3d7..f29b52cead71 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -272,6 +272,14 @@ static const struct serial8250_config uart_config[] = {
> .rxtrig_bytes = {1, 4, 8, 14},
> .flags = UART_CAP_FIFO,
> },
> + [PORT_F81216A] = {
> + .name = "Fintek F81216A",
> + .fifo_size = 16,
> + .tx_loadsz = 16,
> + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> + .rxtrig_bytes = {1, 4, 8, 14},
> + .flags = UART_CAP_FIFO,
> + },
> };
>
> /* Uart divisor latch read */
> @@ -1151,6 +1159,12 @@ static void autoconfig_16550a(struct uart_8250_port *up)
> up->port.type = PORT_U6_16550A;
> up->capabilities |= UART_CAP_AFE;
> }
> +
> + /*
> + * Check if the device is a Fintek F81216A
> + */
> + if (fintek_8250_probe(up) == 0)
> + up->port.type = PORT_F81216A;
Please move this probe up one in the call chain:
@@ -1329,6 +1323,13 @@ static void autoconfig(struct uart_8250_port *up)
out_lock:
spin_unlock_irqrestore(&port->lock, flags);
+
+ /*
+ * Check if the device is a Fintek F81216A
+ */
+ if (port->type == PORT_16550A)
+ fintek_8250_probe(up);
+
if (up->capabilities != old_capabilities) {
pr_warn("ttyS%d: detected caps %08x should be %08x\n",
serial_index(port), old_capabilities,
This will also let you use sleeping functions (request_muxed_region(),
kzalloc(GPF_KERNEL), etc.)
> }
>
> /*
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 64742a086ae3..0b2855e0d9b7 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -57,6 +57,20 @@ config SERIAL_8250_PNP
> This builds standard PNP serial support. You may be able to
> disable this feature if you only need legacy serial support.
>
> +config SERIAL_8250_FINTEK
> + bool "Support for Fintek F81216A LPC to 4 UART RS485 API"
> + depends on SERIAL_8250 && PNPACPI
> + ---help---
> + Selecting this option will add support for the RS485 capabilities
> + of the Fintek F81216A LPC to 4 UART.
> +
> + If this option is not selected the device will be configured as a
> + standard 16550A serial port.
> +
> + Probing relies on an ACPI table with one or more PNP0C02 devices.
> +
> + If unsure, say N.
> +
> config SERIAL_8250_CONSOLE
> bool "Console on 8250/16550 and compatible serial port"
> depends on SERIAL_8250=y
> @@ -359,14 +373,6 @@ config SERIAL_8250_OMAP_TTYO_FIXUP
> not booting kernel because the serial console remains silent in case
> they forgot to update the command line.
>
> -config SERIAL_8250_FINTEK
> - tristate "Support for Fintek F81216A LPC to 4 UART"
> - depends on SERIAL_8250 && PNP
> - help
> - Selecting this option will add support for the Fintek F81216A
> - LPC to 4 UART. This device has some RS485 functionality not available
> - through the PNP driver. If unsure, say N.
> -
> config SERIAL_8250_LPC18XX
> tristate "NXP LPC18xx/43xx serial port support"
> depends on SERIAL_8250 && OF && (ARCH_LPC18XX || COMPILE_TEST)
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index c9a2d6ed87e9..367d403d28d5 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250.o 8250_base.o
> 8250-$(CONFIG_SERIAL_8250_PNP) += 8250_pnp.o
> 8250_base-y := 8250_port.o
> 8250_base-$(CONFIG_SERIAL_8250_DMA) += 8250_dma.o
> +8250_base-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
> obj-$(CONFIG_SERIAL_8250_GSC) += 8250_gsc.o
> obj-$(CONFIG_SERIAL_8250_PCI) += 8250_pci.o
> obj-$(CONFIG_SERIAL_8250_HP300) += 8250_hp300.o
> @@ -23,7 +24,6 @@ obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
> obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
> obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
> obj-$(CONFIG_SERIAL_8250_OMAP) += 8250_omap.o
> -obj-$(CONFIG_SERIAL_8250_FINTEK) += 8250_fintek.o
> obj-$(CONFIG_SERIAL_8250_LPC18XX) += 8250_lpc18xx.o
> obj-$(CONFIG_SERIAL_8250_MT6577) += 8250_mtk.o
> obj-$(CONFIG_SERIAL_8250_UNIPHIER) += 8250_uniphier.o
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 5d59c3ebf459..cf76df415d7d 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -61,6 +61,7 @@ struct serial_struct {
> #define PORT_16850 12
> #define PORT_RSA 13 /* RSA-DV II/S card */
> #define PORT_MAX 13
> +#define PORT_F81216A 14
>
> #define SERIAL_IO_PORT 0
> #define SERIAL_IO_HUB6 1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
