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

[Chen Gang <chengang@emindsoft.com.cn>]

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.