Date:   Tue, 13 Jun 2023 17:44:23 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Jacky Huang" <ychuang570808@...il.com>
Cc:     "Rob Herring" <robh+dt@...nel.org>,
        krzysztof.kozlowski+dt@...aro.org, "Lee Jones" <lee@...nel.org>,
        "Michael Turquette" <mturquette@...libre.com>,
        "Stephen Boyd" <sboyd@...nel.org>,
        "Philipp Zabel" <p.zabel@...gutronix.de>,
        "Jiri Slaby" <jirislaby@...nel.org>,
        "Tomer Maimon" <tmaimon77@...il.com>,
        "Catalin Marinas" <catalin.marinas@....com>,
        "Will Deacon" <will@...nel.org>, devicetree@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-serial@...r.kernel.org,
        soc@...nel.org, schung@...oton.com, mjchen@...oton.com,
        "Jacky Huang" <ychuang3@...oton.com>
Subject: Re: [PATCH v14 1/1] tty: serial: Add Nuvoton ma35d1 serial driver support

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

       Arnd

Powered by blists - more mailing lists