Messages in this thread | | | Date | Sun, 4 Dec 2022 09:18:52 +0100 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] hid-ft260: add UART support. |
| |
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@christina-quast.de/
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
| |