Hi, Greg KH : In patch v1 coding quite mess, it is almost 2000 LOCs. For down size code under 1000 LOCs, I decide to drop DMA function code after patch v3. I think that's the biggest difference compared with 8250. Without DMA function, like you said it looks like 8250 variant. I think I should put DMA function back in next submit. Another question for why I need PORT_SLUNPLUS ? I just check many other uart driver, almost all driver define their own PORT number. Actually, I didn't know about it. Maybe some device like bluetooth(use uart port) need autoconfig. Then it will call ioctl with TIOCSERCONFIG. I don't have tool for calling type/config/request/release/verify. Regards, Hammer Hsieh Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 於 2021年12月21日 週二 下午4:21寫道： > > On Tue, Dec 21, 2021 at 04:14:16PM +0800, hammer hsieh wrote: > > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 於 2021年12月20日 週一 下午11:51寫道： > > > > > > On Mon, Dec 13, 2021 at 03:10:07PM +0800, Hammer Hsieh wrote: > > > > +/* Register offsets */ > > > > +#define SUP_UART_DATA 0x00 > > > > +#define SUP_UART_LSR 0x04 > > > > +#define SUP_UART_MSR 0x08 > > > > +#define SUP_UART_LCR 0x0C > > > > +#define SUP_UART_MCR 0x10 > > > > +#define SUP_UART_DIV_L 0x14 > > > > +#define SUP_UART_DIV_H 0x18 > > > > +#define SUP_UART_ISC 0x1C > > > > +#define SUP_UART_TX_RESIDUE 0x20 > > > > +#define SUP_UART_RX_RESIDUE 0x24 > > > > + > > > > +/* Line Status Register bits */ > > > > +#define SUP_UART_LSR_TXE BIT(6) /* tx empty */ > > > > +#define SUP_UART_LSR_BC BIT(5) /* break condition status */ > > > > +#define SUP_UART_LSR_FE BIT(4) /* frame error status */ > > > > +#define SUP_UART_LSR_OE BIT(3) /* overrun error status */ > > > > +#define SUP_UART_LSR_PE BIT(2) /* parity error status */ > > > > +#define SUP_UART_LSR_RX BIT(1) /* 1: receive fifo not empty */ > > > > +#define SUP_UART_LSR_TX BIT(0) /* 1: transmit fifo is not full */ > > > > +#define SUP_UART_LSR_TX_NOT_FULL 1 > > > > +#define SUP_UART_LSR_BRK_ERROR_BITS GENMASK(5, 2) > > > > + > > > > +/* Line Control Register bits */ > > > > +#define SUP_UART_LCR_BC BIT(5) /* break condition select */ > > > > +#define SUP_UART_LCR_PR BIT(4) /* parity bit polarity select */ > > > > +#define SUP_UART_LCR_PE BIT(3) /* parity bit enable */ > > > > +#define SUP_UART_LCR_ST BIT(2) /* stop bits select */ > > > > +#define SUP_UART_LCR_WL5 0x00 /* word length 5 */ > > > > +#define SUP_UART_LCR_WL6 0x01 /* word length 6 */ > > > > +#define SUP_UART_LCR_WL7 0x02 /* word length 7 */ > > > > +#define SUP_UART_LCR_WL8 0x03 /* word length 8 (default) */ > > > > + > > > > +/* Modem Control Register bits */ > > > > +#define SUP_UART_MCR_LB BIT(4) /* Loopback mode */ > > > > +#define SUP_UART_MCR_RI BIT(3) /* ring indicator */ > > > > +#define SUP_UART_MCR_DCD BIT(2) /* data carrier detect */ > > > > +#define SUP_UART_MCR_RTS BIT(1) /* request to send */ > > > > +#define SUP_UART_MCR_DTS BIT(0) /* data terminal ready */ > > > > + > > > > +/* Interrupt Status/Control Register bits */ > > > > +#define SUP_UART_ISC_RXM BIT(5) /* RX interrupt enable */ > > > > +#define SUP_UART_ISC_TXM BIT(4) /* TX interrupt enable */ > > > > +#define SUP_UART_ISC_RX BIT(1) /* RX interrupt status */ > > > > +#define SUP_UART_ISC_TX BIT(0) /* TX interrupt status */ > > > > + > > > > +#define SUP_DUMMY_READ BIT(16) /* drop bytes received on a !CREAD port */ > > > > +#define SUP_UART_NR 5 > > > > > > Aren't most of these defines already in the kernel header files? Why > > > create them again? > > > > > > > If for reduce code. > > I can add #include<linux/serial_reg.h> > > And remove some overlap define name. > > > > #define SUP_UART_LCR_PR -> UART_LCR_EPAR > > #define SUP_UART_LCR_PE -> UART_LCR_PARITY > > #define SUP_UART_LCR_ST -> UART_LCR_STOP > > #define SUP_UART_LCR_WL5 -> UART_LCR_WLEN5 > > #define SUP_UART_LCR_WL6 -> UART_LCR_WLEN6 > > #define SUP_UART_LCR_WL7 -> UART_LCR_WLEN7 > > #define SUP_UART_LCR_WL8 -> UART_LCR_WLEN8 > > > > #define SUP_UART_MCR_LB -> UART_MCR_LOOP > > #define SUP_UART_MCR_RI -> UART_MCR_OUT2 ? > > #define SUP_UART_MCR_DCD -> UART_MCR_OUT1 ? > > #define SUP_UART_MCR_RTS -> UART_MCR_RTS > > #define SUP_UART_MCR_DTS -> UART_MCR_DTR > > > > But the rest define didn't match internal #include<linux/serial_reg.h> > > , those define still need to keep. > > Some use SUP_xxxx specific define. > > Some use internal #include<linux/serial_reg.h>, it is strange. > > Do not duplicate defines that we already have for the same hardware > type. > > And again, why is this not a normal serial driver for the existing UART > types as this hardware is obviously an 8250 variant? > > thanks, > > greg k-h
