Re: [PATCH v14 1/1] tty: serial: Add Nuvoton ma35d1 serial driver support

[Jiri Slaby <jirislaby@kernel.org>]

On 13. 06. 23, 17:44, Arnd Bergmann wrote:
> On Tue, Jun 13, 2023, at 16:49, Greg KH wrote:
>> On Tue, Jun 13, 2023 at 06:58:32PM +0800, Jacky Huang wrote:
>>>
>>> On 2023/6/13 下午 06:28, Greg KH wrote:
>>>> On Mon, Jun 12, 2023 at 02:53:55AM +0000, Jacky Huang wrote:
>>>>> From: Jacky Huang <ychuang3@nuvoton.com>
>>>>>
>>>>> This adds UART and console driver for Nuvoton ma35d1 Soc.
>>>>> It supports full-duplex communication, FIFO control, and
>>>>> hardware flow control.
>>>> You get a full 72 columns for your changelog :)
>>>>
>>>>> --- a/include/uapi/linux/serial_core.h
>>>>> +++ b/include/uapi/linux/serial_core.h
>>>>> @@ -279,4 +279,7 @@
>>>>>    /* Sunplus UART */
>>>>>    #define PORT_SUNPLUS	123
>>>>> +/* Nuvoton MA35 SoC */
>>>>> +#define PORT_MA35	124
>>>>> +
>>>> Why is this change needed?  What userspace code is going to rely on it?
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>
>>> Because the serial driver requires a port->type, and almost all serial
>>> drivers defined their port type here. We follow the practice of most serial
>>> drivers here.
>>> If we don't do it this way, we would have to directly assign a value to
>>> port->type. However, such modifications were questioned in the past,
>>> which is why we changed it back to defining the port type in serial_core.h.
>>
>> I really really want to get rid of this list, as it's a UAPI that no one
>> uses.  So please don't use it, it doesn't help anything, and while the
>> serial driver might require it, it doesn't actually do anything with
>> that field, right?  So why don't we just set all of the values to the
>> same one?
> 
> I don't see how Jacky can come up with a patch to do this correctly
> without more specific guidance to what exactly you are looking for,
> after the last 123 people that added support for a new port got
> that merged.
> 
> I checked debian codesearch and found only three obscure packages that
> accidentally include this header instead of including linux/serial.h,
> a couple of lists of all kernel headers, and none that include it on
> purpose. I agree that this header should really not exist in uapi,
> but the question is what exactly to do about it.
> 
> Possible changes would be:
> 
> - add a special value PORT_* constant other than PORT_UNKNOWN that
>    can be used by serial drivers instead of a unique value, and
>    ensure that the serial core can handle drivers using it.
> 
> - move all values used by the 8250 driver from serial_core.h
>    to serial.h, as this driver actually uses the constants.
> 
> - Move the remaining contents of uapi/linux/serial.h into the
>    non-uapi version.
> 
> - Change all drivers that only reference a single PORT_*
>    value to use the generic one.

Hmm, we are looping :).

https://lore.kernel.org/all/75375f8d-e157-a364-3da5-9c8d5b832927@kernel.org/

regards,
-- 
js
suse labs