Re: [PATCH] hid-ft260: add UART support.

From: Daniel Beer
Date: Sun Dec 04 2022 - 04:14:48 EST


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