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
