On Sat, Oct 22, 2022 at 11:19:20AM +1300, Daniel Beer wrote: > Based on an earlier patch submitted by Christina Quast: > > https://patches.linaro.org/project/linux-serial/patch/20220928192421.11908-1-contact@xxxxxxxxxxxxxxxxxx/ Please link to lore.kernel.org, we have no idea what will happen over time to other domains/links. > Simplified and reworked to use the UART API rather than the TTY layer > directly. Transmit, receive and baud rate changes are supported. Why use the uart layer? Did you just change how the existing driver works? > +struct ft260_input_report { > + u8 report; /* FT260_I2C_REPORT or FT260_UART_REPORT */ > u8 length; /* data payload length */ > - u8 data[2]; /* data payload */ > + u8 data[0]; /* data payload */ Please do not use [0], use [], people are working to replace all [0] instances in the kernel. > +struct ft260_configure_uart_request { > + u8 report; /* FT260_SYSTEM_SETTINGS */ > + u8 request; /* FT260_SET_UART_CONFIG */ > + u8 flow_ctrl; /* 0: OFF, 1: RTS_CTS, 2: DTR_DSR */ > + /* 3: XON_XOFF, 4: No flow ctrl */ > + __le32 baudrate; /* little endian, 9600 = 0x2580, 19200 = 0x4B00 */ The data structure in the device really looks like this? Unaligned accesses are odd. > +static void ft260_uart_set_termios(struct uart_port *port, > + struct ktermios *termios, > + const struct ktermios *old_termios) > +{ > + struct ft260_device *dev = container_of(port, struct ft260_device, port); > + struct hid_device *hdev = dev->hdev; > + unsigned int baud; > + struct ft260_configure_uart_request req; > + int ret; > + > + ft260_dbg("%s uart\n", __func__); Please just use ftrace, no need for any of these "I am here!" lines. Also dev_dbg() functions already have __func__ in them, no need to ever add them again. > --- a/include/uapi/linux/major.h > +++ b/include/uapi/linux/major.h > @@ -175,4 +175,6 @@ > #define BLOCK_EXT_MAJOR 259 > #define SCSI_OSD_MAJOR 260 /* open-osd's OSD scsi device */ > > +#define FT260_MAJOR 261 A whole new major for just a single tty port? Please no, use dynamic majors if you have to, or better yet, tie into the usb-serial implementation (this is a USB device, right?) and then you don't have to mess with this at all. > + > #endif > diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h > index 3ba34d8378bd..d9a7025f467e 100644 > --- a/include/uapi/linux/serial_core.h > +++ b/include/uapi/linux/serial_core.h > @@ -276,4 +276,7 @@ > /* Sunplus UART */ > #define PORT_SUNPLUS 123 > > +/* FT260 HID UART */ > +#define PORT_FT260 124 Why is this required? What userspace code needs this new id? I want to remove all of these ids, not add new ones. thanks, greg k-h
