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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On Mon, 12 Jun 2023, 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.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg KH]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg KH]

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 don't specify here what your tty device name is going to be, why?

It's not written anywhere, is that intentional?

Same for your tty major/minor, what numbers are you using that might
also be in use by a different device in the system?

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jacky Huang]

Dear Greg,


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.


Best regards,
Jacky Huang





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jacky Huang]

Dear Greg,


On 2023/6/13 下午 06:29, 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 don't specify here what your tty device name is going to be, why?
>
> It's not written anywhere, is that intentional?
>
> Same for your tty major/minor, what numbers are you using that might
> also be in use by a different device in the system?
>
> thanks,
>
> greg k-h

I will add description about the tty name to the log.
In practical testing, we specified in the u-boot parameters
to use ttyNVT0 for the console, and it worked fine.

Best regards,
Jacky Huang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg KH]

On Tue, Jun 13, 2023 at 07:03:11PM +0800, Jacky Huang wrote:
> Dear Greg,
> 
> 
> On 2023/6/13 下午 06:29, 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 don't specify here what your tty device name is going to be, why?
> > 
> > It's not written anywhere, is that intentional?
> > 
> > Same for your tty major/minor, what numbers are you using that might
> > also be in use by a different device in the system?
> > 
> > thanks,
> > 
> > greg k-h
> 
> I will add description about the tty name to the log.
> In practical testing, we specified in the u-boot parameters
> to use ttyNVT0 for the console, and it worked fine.

Where did you pick that name from?  Why can't you use the "default" uart
name instead?

I thought we had a list of tty names around somewhere, but I can't find
it right now...

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg KH]

On Tue, Jun 13, 2023 at 06:58:32PM +0800, Jacky Huang wrote:
> Dear Greg,
> 
> 
> 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?

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arnd Bergmann]

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.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jacky Huang]

Dear Greg,


On 2023/6/13 下午 10:48, Greg KH wrote:
> On Tue, Jun 13, 2023 at 07:03:11PM +0800, Jacky Huang wrote:
>> Dear Greg,
>>
>>
>> On 2023/6/13 下午 06:29, 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 don't specify here what your tty device name is going to be, why?
>>>
>>> It's not written anywhere, is that intentional?
>>>
>>> Same for your tty major/minor, what numbers are you using that might
>>> also be in use by a different device in the system?
>>>
>>> thanks,
>>>
>>> greg k-h
>> I will add description about the tty name to the log.
>> In practical testing, we specified in the u-boot parameters
>> to use ttyNVT0 for the console, and it worked fine.
> Where did you pick that name from?  Why can't you use the "default" uart
> name instead?
>
> I thought we had a list of tty names around somewhere, but I can't find
> it right now...
>
> thanks,
>
> greg k-h

Initially, we were using the well-known ttyS, but it is used by the 8250 
driver.
Since the MA35D1 UART is incompatible with the 8250 driver, Andr raised
concerns about using ttyS.

To differentiate this UART from the incompatible 8250, we defined ttyNVT.
This name is specified in the driver's UART name and console name
structure, and other serial drivers follow a similar approach. For example,
we can find names like ttySA, ttySAC, ttySC, ttySIF, ttySTM, ttySUP,
and so on.

If you believe that this UART driver can use ttyS, I am more than willing to
make the modification. After all, some applications and scripts default to
using ttyS, and using ttyNVT can indeed cause some inconvenience in
certain situations.

Best regards,
Jacky Huang



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jiri Slaby]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

On Tue, Jun 13, 2023 at 05:44:23PM +0200, 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 keep complaining about this, when I notice it.  Just use the "default"
port type in the serial driver and don't add a new type here and it
should just work, right?

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

Why do we need a special constant?

> - move all values used by the 8250 driver from serial_core.h
>   to serial.h, as this driver actually uses the constants.

Makes sense.

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

I think this is the best thing to do.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jacky Huang]

On 2023/6/15 下午 06:19, Greg Kroah-Hartman wrote:
> On Tue, Jun 13, 2023 at 05:44:23PM +0200, 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 keep complaining about this, when I notice it.  Just use the "default"
> port type in the serial driver and don't add a new type here and it
> should just work, right?
>
>> 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.
> Why do we need a special constant?
>
>> - move all values used by the 8250 driver from serial_core.h
>>    to serial.h, as this driver actually uses the constants.
> Makes sense.
>
>> - 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.
> I think this is the best thing to do.
>
> thanks,
>
> greg k-h

I will remove the definition of PORT_MA35 without modifying serial_core.h.

However, we still need to assign a value to port->type. So, can we follow
the approach used in the LiteUART driver and directly assign port->type = 1?


Best regards,
Jacky Huang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

On Thu, Jun 15, 2023 at 06:46:01PM +0800, Jacky Huang wrote:
> 
> 
> On 2023/6/15 下午 06:19, Greg Kroah-Hartman wrote:
> > On Tue, Jun 13, 2023 at 05:44:23PM +0200, 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 keep complaining about this, when I notice it.  Just use the "default"
> > port type in the serial driver and don't add a new type here and it
> > should just work, right?
> > 
> > > 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.
> > Why do we need a special constant?
> > 
> > > - move all values used by the 8250 driver from serial_core.h
> > >    to serial.h, as this driver actually uses the constants.
> > Makes sense.
> > 
> > > - 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.
> > I think this is the best thing to do.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I will remove the definition of PORT_MA35 without modifying serial_core.h.
> 
> However, we still need to assign a value to port->type. So, can we follow
> the approach used in the LiteUART driver and directly assign port->type = 1?

Works for me, thanks.

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arnd Bergmann]

On Thu, Jun 15, 2023, at 12:19, Greg Kroah-Hartman wrote:
> On Tue, Jun 13, 2023 at 05:44:23PM +0200, Arnd Bergmann wrote:
>> On Tue, Jun 13, 2023, at 16:49, Greg KH wrote:
>> 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 keep complaining about this, when I notice it.  Just use the "default"
> port type in the serial driver and don't add a new type here and it
> should just work, right?
>
>> 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.
>
> Why do we need a special constant?

The "default" value is 0, which translates to PORT_UNKNOWN, and the
serial core code prevents this from working. I think Jacky tried
to use this the last one or two times you commented on it, and
it did not work.

Setting it to a plain '1' as Jacky suggested in his reply is the
same as PORT_8250, which may or may not be a good choice here.

Since the number is exported to userspace in serial_struct,
it might be better to pick a new constant such as

#define PORT_SERIAL_GENERIC (-1)

in order to be less ambiguous. It's a signed integer, so -1
would work here this would clearly be a special value, or
another option might be to use 255 as something that is
slightly less special but still recognizable as something
that may have a special meaning.

      Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Greg Kroah-Hartman]

On Thu, Jun 15, 2023 at 04:01:55PM +0200, Arnd Bergmann wrote:
> On Thu, Jun 15, 2023, at 12:19, Greg Kroah-Hartman wrote:
> > On Tue, Jun 13, 2023 at 05:44:23PM +0200, Arnd Bergmann wrote:
> >> On Tue, Jun 13, 2023, at 16:49, Greg KH wrote:
> >> 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 keep complaining about this, when I notice it.  Just use the "default"
> > port type in the serial driver and don't add a new type here and it
> > should just work, right?
> >
> >> 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.
> >
> > Why do we need a special constant?
> 
> The "default" value is 0, which translates to PORT_UNKNOWN, and the
> serial core code prevents this from working. I think Jacky tried
> to use this the last one or two times you commented on it, and
> it did not work.

Ah, thanks, that makes sense.

> Setting it to a plain '1' as Jacky suggested in his reply is the
> same as PORT_8250, which may or may not be a good choice here.

Odds are it would be fine :)

> Since the number is exported to userspace in serial_struct,
> it might be better to pick a new constant such as
> 
> #define PORT_SERIAL_GENERIC (-1)
> 
> in order to be less ambiguous. It's a signed integer, so -1
> would work here this would clearly be a special value, or
> another option might be to use 255 as something that is
> slightly less special but still recognizable as something
> that may have a special meaning.

A new constant would be good, 255 is nice, and then we can move everyone
to use it unless they can specifically show a reason why it will not
work for them.

I think originally this was used to do device-specific ioctls, right?
That shouldn't be happening anymore, hopefully...

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arnd Bergmann]

On Thu, Jun 15, 2023, at 17:00, Greg Kroah-Hartman wrote:
> On Thu, Jun 15, 2023 at 04:01:55PM +0200, Arnd Bergmann wrote:
>> Since the number is exported to userspace in serial_struct,
>> it might be better to pick a new constant such as
>> 
>> #define PORT_SERIAL_GENERIC (-1)
>> 
>> in order to be less ambiguous. It's a signed integer, so -1
>> would work here this would clearly be a special value, or
>> another option might be to use 255 as something that is
>> slightly less special but still recognizable as something
>> that may have a special meaning.
>
> A new constant would be good, 255 is nice, and then we can move everyone
> to use it unless they can specifically show a reason why it will not
> work for them.
>
> I think originally this was used to do device-specific ioctls, right?
> That shouldn't be happening anymore, hopefully...

The only thing I could find is that you can use TIOCSSERIAL to set
the type between the supported types within a driver, which changes
the behavior in some cases, e.g. the exact size and layout of the
register file or its capabilities.

We may need a proper audit of TIOCSSERIAL anyway, I suspect there
are worse things you can do with other settings.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel