On Sun, Jan 05, 2025 at 09:44:52PM +0100, Benjamin Larsson wrote:
> Hi.
>
> On 05/01/2025 16:59, Greg KH wrote:
> >
> > On Sun, Jan 05, 2025 at 02:11:47PM +0100, Benjamin Larsson wrote:
> > > Support for Airoha AN7581 SoC UART and HSUART baud rate
> > > calculation routine.
> > >
> > > Signed-off-by: Benjamin Larsson <benjamin.larsson@xxxxxxxxxx>
> > > ---
> > > drivers/tty/serial/8250/8250_airoha.c | 85 +++++++++++++++++++++++++++
> > > drivers/tty/serial/8250/8250_of.c | 2 +
> > > drivers/tty/serial/8250/8250_port.c | 26 ++++++++
> > > drivers/tty/serial/8250/Kconfig | 10 ++++
> > > drivers/tty/serial/8250/Makefile | 1 +
> > > include/linux/serial_8250.h | 1 +
> > > include/uapi/linux/serial_core.h | 6 ++
> > > include/uapi/linux/serial_reg.h | 9 +++
> > > 8 files changed, 140 insertions(+)
> > > create mode 100644 drivers/tty/serial/8250/8250_airoha.c
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_airoha.c b/drivers/tty/serial/8250/8250_airoha.c
> > > new file mode 100644
> > > index 000000000000..c57789dcc174
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_airoha.c
> > > @@ -0,0 +1,85 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> >
> > Do you really mean "+" here? Sorry, I have to ask.
> >
>
> I just try to follow the style of 8250_port.c. Whatever is fine by me.
That's your call, please talk about it with your company lawyers.
> > > /* Uart divisor latch read */
> > > @@ -2847,6 +2863,16 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
> > >
> > > serial8250_set_divisor(port, baud, quot, frac);
> > >
> > > +
> > > + /*
> > > + * Airoha SoCs have custom registers for baud rate settings
> > > + */
> > > +#ifdef CONFIG_SERIAL_8250_AIROHA
> >
> > Please don't put #ifdef in .c files, are you sure this is needed this
> > way? Why not do it the other way around like other uart types do it?
>
> There are other instances of ifdef in the code (#ifdef
> CONFIG_SERIAL_8250_RSA). Thus I used ifdefs.
Don't follow bad practices of others please.
> Should I use IS_REACHABLE or IS_ENABLED instead?
Neither, put it in a .h file properly instead.
> > > --- a/include/linux/serial_8250.h
> > > +++ b/include/linux/serial_8250.h
> > > @@ -195,6 +195,7 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl);
> > > void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
> > > unsigned int quot);
> > > int fsl8250_handle_irq(struct uart_port *port);
> > > +int airoha8250_set_baud_rate(struct uart_port *port, unsigned int baud, unsigned int hs);
> >
> > Why is this here?
>
> I use the same place as fsl8250_handle_irq() uses. Should I place it
> somewhere else ?
Yes please, put it in your own .h file which you will need anyway.
> > > int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
> > > u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr);
> > > void serial8250_read_char(struct uart_8250_port *up, u16 lsr);
> > > diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> > > index 9c007a106330..c71fb338accb 100644
> > > --- a/include/uapi/linux/serial_core.h
> > > +++ b/include/uapi/linux/serial_core.h
> > > @@ -231,6 +231,12 @@
> > > /* Sunplus UART */
> > > #define PORT_SUNPLUS 123
> > >
> > > +/* Airoha UART */
> > > +#define PORT_AIROHA 124
> > > +
> > > +/* Airoha HSUART */
> > > +#define PORT_AIROHA_HS 125
> >
> > Do you REALLY need these port definitions in userspace? If so, what is
> > going to use them there?
> >
>
> Testing another PORT define gives this result:
>
> grep -ri PORT_MTK_BTIF *
>
> drivers/tty/serial/8250/8250_of.c: .data = (void *)PORT_MTK_BTIF, },
> drivers/tty/serial/8250/8250_port.c: [PORT_MTK_BTIF] = {
> include/uapi/linux/serial_core.h:#define PORT_MTK_BTIF 117
>
> Per my understanding this is how the current code is designed to work.
That's a very old pattern, I'm asking you if you need this new number in
userspace, which is what you are doing here. I'd prefer not to add new
values here as they are a pain to manage and we can never change them if
added.
> > > +
> > > /* Generic type identifier for ports which type is not important to userspace. */
> > > #define PORT_GENERIC (-1)
> > >
> > > diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
> > > index 9c987b04e2d0..72a71e171602 100644
> > > --- a/include/uapi/linux/serial_reg.h
> > > +++ b/include/uapi/linux/serial_reg.h
> > > @@ -383,5 +383,14 @@
> > > #define UART_ALTR_EN_TXFIFO_LW 0x01 /* Enable the TX FIFO Low Watermark */
> > > #define UART_ALTR_TX_LOW 0x41 /* Tx FIFO Low Watermark */
> > >
> > > +/*
> > > + * These are definitions for the Airoha UART
> > > + * Normalized because of 32 bit registers.
> > > + */
> > > +#define UART_BRDL 0
> > > +#define UART_BRDH 1
> > > +#define UART_XINCLKDR 10
> > > +#define UART_XYD 11
> >
> > Why does the define not have your uart type in it?
>
> I will address this in v2.
>
> > And what is
> > userspace going to do with these values? Why do they need to be in this
> > file?
>
> Per my understanding I am following the intended design.
What design? Again what needs this in userspace to require it in this
.h file? Why not just put them in your own private .h file as they are
only used by your module and nothing else, right?
thanks,
greg k-h
