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

[Greg Kroah-Hartman]

On Sun, Dec 04, 2022 at 10:12:47PM +1300, Daniel Beer wrote:
> 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.

Lots of devices are a "fake HID" device as other operating systems make
it easy to write userspace drivers that way.  Linux included.  What
userspace programs are going to want to interact with this device and
what api are they going to use?

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

No, just checking, it's just a rough thing for processors to handle at
times, but if that's how the device is designed, nothing you can do
about it.

And as this is a structure that comes from the device, you should be
using __u8 and friends.

> > > --- 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.

That should not be a restriction at all.  You are adding a tty device to
this driver, no reason you can't interact with usb-serial instead.  That
way you share the correct userspace tty name and major/minor numbers and
all userspace tools should "just work" as they know that name and how to
interact with it already.

Try doing that instead of your own "raw" tty device please.

> > > +
> > >  #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?

No, use the usb-serial api instead please.

thanks,

greg k-h