On Sun, Dec 04, 2022 at 09:18:52AM +0100, Greg Kroah-Hartman wrote: > 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? Hi Greg, Thanks for reviewing. This device is quite strange -- it presents itself as a USB HID, but it provides both an I2C master and a UART. The existing driver supports only the I2C functionality currently. > > +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. Yes, that really is the data structure. Is there a better way to do this? > > --- 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. As far as I understand it, I don't think usb-serial is usable, due to the fact that this is already an HID driver. I'll change to use dynamic majors, unless there's a better option. > > + > > #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. It probably isn't. I'd taken another driver as an example when implementing this, and that's what it did. Should I instead set the port field to PORT_UNKNOWN and return NULL from uart_type? Cheers, Daniel -- Daniel Beer Firmware Engineer at Igor Institute daniel.beer@xxxxxxxxxxxxxxxxx or +64-27-420-8101 Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312
