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

[Daniel Beer <daniel.beer@igorinstitute.com>]

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@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?

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@igorinstitute.com or +64-27-420-8101
Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312