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@xxxxxxxxxxxxxxxx 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