[PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

Sorry for this patch being too late, which is for linux-next 20151127 (
about linux 4.4-rc2).  After 4 years, much things have been changed. But
I think it might be still valuable for some old versions. Welcome anyone
to refact this patch for their own.

Fintek serial ports can share irq, but they need be enabled firstly, so
enable or disable irq sharing based on isa or pci bus. From kconfig, it
can be configured.

For integrated 8250 drivers, kernel always calls pnp driver, which will
not use integrated fintek driver for ever. So let pnp driver try the
other drivers firstly (e.g. fintek), if fail, try pnp driver its own.

Cc: Lv Li-song <lvlisong@emindsoft.com.cn>
Signed-off-by: Chen Gang <chengang@emindsoft.com.cn>
---
 drivers/tty/serial/8250/8250.h        |   9 +++
 drivers/tty/serial/8250/8250_fintek.c | 130 +++++++++++++++++++++-------------
 drivers/tty/serial/8250/8250_pnp.c    |  13 +++-
 drivers/tty/serial/8250/Kconfig       |  26 +++++++
 4 files changed, 129 insertions(+), 49 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index d54dcd8..a6e3245 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -14,6 +14,7 @@
 #include <linux/serial_8250.h>
 #include <linux/serial_reg.h>
 #include <linux/dmaengine.h>
+#include <linux/pnp.h>
 
 struct uart_8250_dma {
 	int (*tx_dma)(struct uart_8250_port *p);
@@ -130,6 +131,9 @@ void serial8250_rpm_put(struct uart_8250_port *p);
 #endif
 
 #ifdef CONFIG_SERIAL_8250_PNP
+struct pnp_base_data {
+	struct pnp_driver *driver;
+};
 int serial8250_pnp_init(void);
 void serial8250_pnp_exit(void);
 #else
@@ -137,6 +141,11 @@ static inline int serial8250_pnp_init(void) { return 0; }
 static inline void serial8250_pnp_exit(void) { }
 #endif
 
+#ifdef CONFIG_SERIAL_8250_FINTEK
+extern int
+fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id);
+#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 8947439..50c4ed7e 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -38,9 +38,15 @@
 #define RXW4C_IRA BIT(3)
 #define TXW4C_IRA BIT(2)
 
+#define ICSR   0x70
+#define IRQ_SHARING_MOD 0x10
+#define PCI_IRQ_SHARING 0x00
+#define ISA_IRQ_SHARING 0x20
+
 #define DRIVER_NAME "8250_fintek"
 
 struct fintek_8250 {
+	struct pnp_base_data base; /* must be the first */
 	u16 base_port;
 	u8 index;
 	u8 key;
@@ -138,6 +144,31 @@ static int fintek_8250_rs485_config(struct uart_port *port,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
+
+static void set_icsr(u16 base_port, u8 index)
+{
+	uint8_t icsr = 0;
+
+	outb(LDN, base_port + ADDR_PORT);
+	outb(index, base_port + DATA_PORT);
+	outb(ICSR, base_port + ADDR_PORT);
+	icsr = inb(base_port + DATA_PORT);
+
+	if (icsr != 0xff) {
+		icsr |= IRQ_SHARING_MOD;
+#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING_ISA)
+		icsr |= ISA_IRQ_SHARING;
+#else
+		icsr |= PCI_IRQ_SHARING;
+#endif
+		outb(ICSR, base_port + ADDR_PORT);
+		outb(icsr, base_port + DATA_PORT);
+	}
+}
+
+#endif
+
 static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
 {
 	static const u16 addr[] = {0x4e, 0x2e};
@@ -166,7 +197,9 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
 				aux |= inb(addr[i] + DATA_PORT) << 8;
 				if (aux != io_address)
 					continue;
-
+#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
+				set_icsr(addr[i], k);
+#endif
 				fintek_8250_exit_key(addr[i]);
 				*key = keys[j];
 				*index = k;
@@ -179,53 +212,6 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
 	return -ENODEV;
 }
 
-static int
-fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
-{
-	struct uart_8250_port uart;
-	struct fintek_8250 *pdata;
-	int base_port;
-	u8 key;
-	u8 index;
-
-	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;
-
-	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;
-
-	pnp_set_drvdata(dev, pdata);
-	return 0;
-}
-
 static void fintek_8250_remove(struct pnp_dev *dev)
 {
 	struct fintek_8250 *pdata = pnp_get_drvdata(dev);
@@ -276,6 +262,54 @@ static struct pnp_driver fintek_8250_driver = {
 	.id_table	= fintek_dev_table,
 };
 
+int
+fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
+{
+	struct uart_8250_port uart;
+	struct fintek_8250 *pdata;
+	int base_port;
+	u8 key;
+	u8 index;
+
+	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;
+
+	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;
+	pdata->base.driver = &fintek_8250_driver;
+
+	pnp_set_drvdata(dev, pdata);
+	return 0;
+}
+
 module_pnp_driver(fintek_8250_driver);
 MODULE_DESCRIPTION("Fintek F812164 module");
 MODULE_AUTHOR("Ricardo Ribalda <ricardo.ribalda@gmail.com>");
diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
index 658b392..af43a4c 100644
--- a/drivers/tty/serial/8250/8250_pnp.c
+++ b/drivers/tty/serial/8250/8250_pnp.c
@@ -438,8 +438,13 @@ static int
 serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 {
 	struct uart_8250_port uart, *port;
-	int ret, line, flags = dev_id->driver_data;
+	int ret, line, flags;
 
+#if IS_BUILTIN(CONFIG_SERIAL_8250_FINTEK)
+	if (!fintek_8250_probe(dev, dev_id))
+		return 0;
+#endif
+	flags = dev_id->driver_data;
 	if (flags & UNKNOWN_DEV) {
 		ret = serial_pnp_guess_board(dev);
 		if (ret < 0)
@@ -494,6 +499,8 @@ static void serial_pnp_remove(struct pnp_dev *dev)
 {
 	long line = (long)pnp_get_drvdata(dev);
 
+	if (line > CONFIG_SERIAL_8250_NR_UARTS)
+		((struct pnp_base_data *)line)->driver->remove(dev);
 	dev->capabilities &= ~PNP_CONSOLE;
 	if (line)
 		serial8250_unregister_port(line - 1);
@@ -504,6 +511,8 @@ static int serial_pnp_suspend(struct pnp_dev *dev, pm_message_t state)
 {
 	long line = (long)pnp_get_drvdata(dev);
 
+	if (line > CONFIG_SERIAL_8250_NR_UARTS)
+		((struct pnp_base_data *)line)->driver->suspend(dev, state);
 	if (!line)
 		return -ENODEV;
 	serial8250_suspend_port(line - 1);
@@ -514,6 +523,8 @@ static int serial_pnp_resume(struct pnp_dev *dev)
 {
 	long line = (long)pnp_get_drvdata(dev);
 
+	if (line > CONFIG_SERIAL_8250_NR_UARTS)
+		((struct pnp_base_data *)line)->driver->resume(dev);
 	if (!line)
 		return -ENODEV;
 	serial8250_resume_port(line - 1);
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 6412f14..37d120b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -337,6 +337,32 @@ config SERIAL_8250_FINTEK
 	  LPC to 4 UART. This device has some RS485 functionality not available
 	  through the PNP driver. If unsure, say N.
 
+config SERIAL_8250_FINTEK_IRQ_SHARING
+	bool "Enable IRQ sharing for Fintek F81216A LPC to 4 UART"
+	depends on SERIAL_8250_FINTEK
+	help
+	  Selecting this option will enable IRQ sharing for the Fintek F81216A
+	  LPC to 4 UART.
+
+choice
+	prompt "IRQ sharing ways for Fintek F81216A LPC to 4 UART"
+	depends on SERIAL_8250_FINTEK_IRQ_SHARING
+	default SERIAL_8250_FINTEK_IRQ_SHARING_ISA
+
+config SERIAL_8250_FINTEK_IRQ_SHARING_ISA
+	bool "ISA bus IRQ sharing"
+	depends on SERIAL_8250_FINTEK_IRQ_SHARING
+	help
+	  Based on ISA bus.
+
+config SERIAL_8250_FINTEK_IRQ_SHARING_PCI
+	bool "PCI bus IRQ sharing"
+	depends on SERIAL_8250_FINTEK_IRQ_SHARING
+	help
+	  Based on PCI bus.
+
+endchoice
+
 config SERIAL_8250_LPC18XX
 	tristate "NXP LPC18xx/43xx serial port support"
 	depends on SERIAL_8250 && OF && (ARCH_LPC18XX || COMPILE_TEST)
-- 
1.9.1

Re: [PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[Andy Shevchenko]

On Fri, Dec 13, 2019 at 01:17:17PM +0800, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>

Thanks for the patch, my comments below.

> Sorry for this patch being too late, which is for linux-next 20151127 (
> about linux 4.4-rc2).  After 4 years, much things have been changed. But
> I think it might be still valuable for some old versions. Welcome anyone
> to refact this patch for their own.

This part should go after '---' line below.

> Fintek serial ports can share irq, but they need be enabled firstly, so
> enable or disable irq sharing based on isa or pci bus. From kconfig, it

irq -> IRQ
isa -> ISA
pci -> PCI
kconfig -> Kconfig

> can be configured.
> 
> For integrated 8250 drivers, kernel always calls pnp driver, which will
> not use integrated fintek driver for ever. So let pnp driver try the

fintek -> Fintek or Fintek 8250

> other drivers firstly (e.g. fintek), if fail, try pnp driver its own.

Ditto.

...

> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -14,6 +14,7 @@
>  #include <linux/serial_8250.h>
>  #include <linux/serial_reg.h>
>  #include <linux/dmaengine.h>

> +#include <linux/pnp.h>

The changes below doesn't require a header.
Pointers are known to the compiler. Names of data structures can be forward
declared. Moreover, these declarations may go inside respective #ifdef.

...

> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
> +
> +static void set_icsr(u16 base_port, u8 index)
> +{
> +	uint8_t icsr = 0;
> +
> +	outb(LDN, base_port + ADDR_PORT);
> +	outb(index, base_port + DATA_PORT);
> +	outb(ICSR, base_port + ADDR_PORT);
> +	icsr = inb(base_port + DATA_PORT);
> +

> +	if (icsr != 0xff) {

	if (icsr == 0xff)
		return;

?

> +		icsr |= IRQ_SHARING_MOD;
> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING_ISA)
> +		icsr |= ISA_IRQ_SHARING;
> +#else
> +		icsr |= PCI_IRQ_SHARING;
> +#endif
> +		outb(ICSR, base_port + ADDR_PORT);
> +		outb(icsr, base_port + DATA_PORT);
> +	}
> +}
> +
> +#endif

...

>  				aux |= inb(addr[i] + DATA_PORT) << 8;
>  				if (aux != io_address)
>  					continue;

> -

What the point?

> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
> +				set_icsr(addr[i], k);
> +#endif
>  				fintek_8250_exit_key(addr[i]);
>  				*key = keys[j];
>  				*index = k;
> @@ -179,53 +212,6 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
>  	return -ENODEV;
>  }
>  
> -static int
> -fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)

Why did you move this function?
It's now not only hard to follow what has been changed, and to review.

> --- a/drivers/tty/serial/8250/8250_pnp.c
> +++ b/drivers/tty/serial/8250/8250_pnp.c
> @@ -438,8 +438,13 @@ static int
>  serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>  {
>  	struct uart_8250_port uart, *port;
> -	int ret, line, flags = dev_id->driver_data;
> +	int ret, line, flags;
>  

> +#if IS_BUILTIN(CONFIG_SERIAL_8250_FINTEK)
> +	if (!fintek_8250_probe(dev, dev_id))
> +		return 0;
> +#endif
> +	flags = dev_id->driver_data;

Oh, I don't like this.
It needs a bit more refactoring done first.

The idea that we are not going to pollute generic driver(s) with quirks anymore
(only when it's really unavoidable).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[Chen Gang]

Thank you for your reply.

I guess, this patch has to be refactored to match the related linux
versions. And excuse me, my orignal hardware environments has been gone,
so I can not give the new refactored patch additional test.

It is necessary to continue discussing and reviewing this patch to let
it be known completely, but I guess I am not the suitable persion to
refactor the patch.

After finish discussing and reviewing, if anyone still wants me to
refactor the patch, please let me know, I shall try.

The contents below are my reply, pelease check, thanks.

On 2019/12/13 下午6:50, Andy Shevchenko wrote:
> On Fri, Dec 13, 2019 at 01:17:17PM +0800, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <chengang@emindsoft.com.cn>
>> Sorry for this patch being too late, which is for linux-next 20151127 (
>> about linux 4.4-rc2).  After 4 years, much things have been changed. But
>> I think it might be still valuable for some old versions. Welcome anyone
>> to refact this patch for their own.
> 
> This part should go after '---' line below.
> 
>> Fintek serial ports can share irq, but they need be enabled firstly, so
>> enable or disable irq sharing based on isa or pci bus. From kconfig, it
> 
> irq -> IRQ
> isa -> ISA
> pci -> PCI
> kconfig -> Kconfig
> 
>> can be configured.
>>
>> For integrated 8250 drivers, kernel always calls pnp driver, which will
>> not use integrated fintek driver for ever. So let pnp driver try the
> 
> fintek -> Fintek or Fintek 8250
> 
>> other drivers firstly (e.g. fintek), if fail, try pnp driver its own.
> 
> Ditto.
> 

OK, thanks.

> 
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -14,6 +14,7 @@
>>  #include <linux/serial_8250.h>
>>  #include <linux/serial_reg.h>
>>  #include <linux/dmaengine.h>
> 
>> +#include <linux/pnp.h>
> 
> The changes below doesn't require a header.
> Pointers are known to the compiler. Names of data structures can be forward
> declared. Moreover, these declarations may go inside respective #ifdef.
> 

OK. I guess, originally, I forgot to remove it.

> 
>> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
>> +
>> +static void set_icsr(u16 base_port, u8 index)
>> +{
>> +	uint8_t icsr = 0;
>> +
>> +	outb(LDN, base_port + ADDR_PORT);
>> +	outb(index, base_port + DATA_PORT);
>> +	outb(ICSR, base_port + ADDR_PORT);
>> +	icsr = inb(base_port + DATA_PORT);
>> +
>> +	if (icsr != 0xff) {
> 
> 	if (icsr == 0xff)
> 		return;
> 

It'll be OK.

> 
>> +		icsr |= IRQ_SHARING_MOD;
>> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING_ISA)
>> +		icsr |= ISA_IRQ_SHARING;
>> +#else
>> +		icsr |= PCI_IRQ_SHARING;
>> +#endif
>> +		outb(ICSR, base_port + ADDR_PORT);
>> +		outb(icsr, base_port + DATA_PORT);
>> +	}
>> +}
>> +
>> +#endif
> 
> ..
> 
>>  				aux |= inb(addr[i] + DATA_PORT) << 8;
>>  				if (aux != io_address)
>>  					continue;
> 
>> -
> 
> What the point?
> 
>> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
>> +				set_icsr(addr[i], k);
>> +#endif
>>  				fintek_8250_exit_key(addr[i]);
>>  				*key = keys[j];
>>  				*index = k;
>> @@ -179,53 +212,6 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
>>  	return -ENODEV;
>>  }
>>  

In my case at that time, for fintex irq sharing, it needed additional
initinalization, or it could not work well. I wrote the related code
based on the fintek data-sheet which was downloaded from internet.

>> -static int
>> -fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
> 
> Why did you move this function?
> It's now not only hard to follow what has been changed, and to review.
> 
>> --- a/drivers/tty/serial/8250/8250_pnp.c
>> +++ b/drivers/tty/serial/8250/8250_pnp.c
>> @@ -438,8 +438,13 @@ static int
>>  serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>>  {
>>  	struct uart_8250_port uart, *port;
>> -	int ret, line, flags = dev_id->driver_data;
>> +	int ret, line, flags;
>>  
> 

I thought locating the main probe function at the end of the source file
was better for normal code reading (maybe it need be a seperate patch).
But if we don't mind, we can still remain its orignal position.

>> +#if IS_BUILTIN(CONFIG_SERIAL_8250_FINTEK)
>> +	if (!fintek_8250_probe(dev, dev_id))
>> +		return 0;
>> +#endif
>> +	flags = dev_id->driver_data;
> 
> Oh, I don't like this.
> It needs a bit more refactoring done first.
> 
> The idea that we are not going to pollute generic driver(s) with quirks anymore
> (only when it's really unavoidable).
> 

At that time, for me, I could not get any new better ways in a short
time, and the issue had to be fixed in time, so the code was not good
engough.

At present, Linux version has been changed much, welcome any one to
refactor it for current linux version or another related old linux
versions if this patch is valuable more or less.


Thanks.

Re: [PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[Andy Shevchenko]

On Mon, Dec 16, 2019 at 10:27:23AM +0800, Chen Gang wrote:
> Thank you for your reply.
> 
> I guess, this patch has to be refactored to match the related linux
> versions. And excuse me, my orignal hardware environments has been gone,
> so I can not give the new refactored patch additional test.
> 
> It is necessary to continue discussing and reviewing this patch to let
> it be known completely, but I guess I am not the suitable persion to
> refactor the patch.

Yeah, you may refactor it, but please mention in the comment (the text going
after '---' line) that you are not able to test it. At least for maintainer it
may be a crucial point either to take your change or not.

> After finish discussing and reviewing, if anyone still wants me to
> refactor the patch, please let me know, I shall try.
> 
> The contents below are my reply, pelease check, thanks.

My reply below.

> On 2019/12/13 下午6:50, Andy Shevchenko wrote:
> > On Fri, Dec 13, 2019 at 01:17:17PM +0800, chengang@emindsoft.com.cn wrote:

> >>  				aux |= inb(addr[i] + DATA_PORT) << 8;
> >>  				if (aux != io_address)
> >>  					continue;
> > 
> >> -
> > 
> > What the point?

(1)

> >> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
> >> +				set_icsr(addr[i], k);
> >> +#endif
> >>  				fintek_8250_exit_key(addr[i]);
> >>  				*key = keys[j];
> >>  				*index = k;
> >> @@ -179,53 +212,6 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
> >>  	return -ENODEV;
> >>  }
> >>  
> 
> In my case at that time, for fintex irq sharing, it needed additional
> initinalization, or it could not work well. I wrote the related code
> based on the fintek data-sheet which was downloaded from internet.

I guess it's an answer to the (1). Though in (1) I simple meant the removal
of blank line (see, I emphasized the excerpt I'm commenting with blank lines
before and after).

> >> -static int
> >> -fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
> > 
> > Why did you move this function?
> > It's now not only hard to follow what has been changed, and to review.
> > 
> >> --- a/drivers/tty/serial/8250/8250_pnp.c
> >> +++ b/drivers/tty/serial/8250/8250_pnp.c
> >> @@ -438,8 +438,13 @@ static int
> >>  serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
> >>  {
> >>  	struct uart_8250_port uart, *port;
> >> -	int ret, line, flags = dev_id->driver_data;
> >> +	int ret, line, flags;
> >>  
> > 
> 
> I thought locating the main probe function at the end of the source file
> was better for normal code reading (maybe it need be a seperate patch).

Yes, it needs to be in a separated (preparatory) patch.

> But if we don't mind, we can still remain its orignal position.

I do mind, sorry. The rule of thumb is one logical change per patch.

> >> +#if IS_BUILTIN(CONFIG_SERIAL_8250_FINTEK)
> >> +	if (!fintek_8250_probe(dev, dev_id))
> >> +		return 0;
> >> +#endif
> >> +	flags = dev_id->driver_data;
> > 
> > Oh, I don't like this.
> > It needs a bit more refactoring done first.
> > 
> > The idea that we are not going to pollute generic driver(s) with quirks anymore
> > (only when it's really unavoidable).
> > 
> 
> At that time, for me, I could not get any new better ways in a short
> time, and the issue had to be fixed in time, so the code was not good
> engough.

It's not an excuse to put hacks in the code that will make maintenance hard.
The usual case is such situations is that author of the fix do:

- provide a fix (perhaps ugly one)
- refactor and clean up the code

So at the result we have keep maintainable piece in kernel.
This is by the way my main motivation to NAK this change.

> At present, Linux version has been changed much, welcome any one to
> refactor it for current linux version or another related old linux
> versions if this patch is valuable more or less.

Then it's no go for this patch, sorry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[Chen Gang]

On 2019/12/16 下午5:51, Andy Shevchenko wrote:
> On Mon, Dec 16, 2019 at 10:27:23AM +0800, Chen Gang wrote:
>> Thank you for your reply.
>>
>> I guess, this patch has to be refactored to match the related linux
>> versions. And excuse me, my orignal hardware environments has been gone,
>> so I can not give the new refactored patch additional test.
>>
>> It is necessary to continue discussing and reviewing this patch to let
>> it be known completely, but I guess I am not the suitable persion to
>> refactor the patch.
> 
> Yeah, you may refactor it, but please mention in the comment (the text going
> after '---' line) that you are not able to test it. At least for maintainer it
> may be a crucial point either to take your change or not.
> 

OK, I shall try to refactor the patch within this weekend in the latest
linux-next tree.

I should abey the GPL license, so it is my duty to send my modification
to upstream and try my best to let the patch OK. If the patch can not be
merged, I can understand (especially, the patch is too late).

>> On 2019/12/13 下午6:50, Andy Shevchenko wrote:
>>> On Fri, Dec 13, 2019 at 01:17:17PM +0800, chengang@emindsoft.com.cn wrote:
> 
>>>>  				aux |= inb(addr[i] + DATA_PORT) << 8;
>>>>  				if (aux != io_address)
>>>>  					continue;
>>>
>>>> -
>>>
>>> What the point?
> 
> (1)
> 
>>>> +#if IS_ENABLED(CONFIG_SERIAL_8250_FINTEK_IRQ_SHARING)
>>>> +				set_icsr(addr[i], k);
>>>> +#endif
>>>>  				fintek_8250_exit_key(addr[i]);
>>>>  				*key = keys[j];
>>>>  				*index = k;
>>>> @@ -179,53 +212,6 @@ static int fintek_8250_base_port(u16 io_address, u8 *key, u8 *index)
>>>>  	return -ENODEV;
>>>>  }
>>>>  
>>
>> In my case at that time, for fintex irq sharing, it needed additional
>> initinalization, or it could not work well. I wrote the related code
>> based on the fintek data-sheet which was downloaded from internet.
> 
> I guess it's an answer to the (1). Though in (1) I simple meant the removal
> of blank line (see, I emphasized the excerpt I'm commenting with blank lines
> before and after).
> 

Oh, sorry, I missunderstood. For me, reserving the original blank line
is OK.

>>>> -static int
>>>> -fintek_8250_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>>>
>>> Why did you move this function?
>>> It's now not only hard to follow what has been changed, and to review.
>>>
>>>> --- a/drivers/tty/serial/8250/8250_pnp.c
>>>> +++ b/drivers/tty/serial/8250/8250_pnp.c
>>>> @@ -438,8 +438,13 @@ static int
>>>>  serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>>>>  {
>>>>  	struct uart_8250_port uart, *port;
>>>> -	int ret, line, flags = dev_id->driver_data;
>>>> +	int ret, line, flags;
>>>>  
>>>
>>
>> I thought locating the main probe function at the end of the source file
>> was better for normal code reading (maybe it need be a seperate patch).
> 
> Yes, it needs to be in a separated (preparatory) patch.
> 
>> But if we don't mind, we can still remain its orignal position.
> 
> I do mind, sorry. The rule of thumb is one logical change per patch.
> 

OK, in the latest linux tree, if necessary, I will send 2 patches.

>>>> +#if IS_BUILTIN(CONFIG_SERIAL_8250_FINTEK)
>>>> +	if (!fintek_8250_probe(dev, dev_id))
>>>> +		return 0;
>>>> +#endif
>>>> +	flags = dev_id->driver_data;
>>>
>>> Oh, I don't like this.
>>> It needs a bit more refactoring done first.
>>>
>>> The idea that we are not going to pollute generic driver(s) with quirks anymore
>>> (only when it's really unavoidable).
>>>
>>
>> At that time, for me, I could not get any new better ways in a short
>> time, and the issue had to be fixed in time, so the code was not good
>> engough.
> 
> It's not an excuse to put hacks in the code that will make maintenance hard.
> The usual case is such situations is that author of the fix do:
> 
> - provide a fix (perhaps ugly one)
> - refactor and clean up the code
> 
> So at the result we have keep maintainable piece in kernel.
> This is by the way my main motivation to NAK this change.
> 
>> At present, Linux version has been changed much, welcome any one to
>> refactor it for current linux version or another related old linux
>> versions if this patch is valuable more or less.
> 
> Then it's no go for this patch, sorry.
> 

Yes, refactoring and cleaning up the code is the patch sender's
resposibility.

And thank you for reviewing the patch.

Re: [PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[Chen Gang]

After check the linux-next tree, the core content is already fixed by
the patch "4da22f1418cb serial: 8250_fintek: fix the mismatched IRQ
mode" (it was applied on 2016-05-27).

And it looks my original modification for 8250_pnp.c is unnecessary, I
guess originally I only wanted to make sure it should work well, but did
not cleanup the code.

So this patch is useless, sorry to bother you. And again, thank you for
reviewing the code.

Re: [PATCH] drivers: tty: serial: 8250: fintek: Can enable or disable irq sharing based on isa or pci bus

[Andy Shevchenko]

On Thu, Dec 19, 2019 at 05:35:59PM +0800, Chen Gang wrote:
> After check the linux-next tree, the core content is already fixed by
> the patch "4da22f1418cb serial: 8250_fintek: fix the mismatched IRQ
> mode" (it was applied on 2016-05-27).
> 
> And it looks my original modification for 8250_pnp.c is unnecessary, I
> guess originally I only wanted to make sure it should work well, but did
> not cleanup the code.

Good news!

> So this patch is useless, sorry to bother you. And again, thank you for
> reviewing the code.

You're welcome!

-- 
With Best Regards,
Andy Shevchenko