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@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).
> 

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.