| Message ID | 20231218093153.192268-1-contact@christina-quast.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,RESEND] hid-ft260: Add serial driver | expand |
On Thu, Dec 28, 2023 at 12:50:08PM +0100, Christina Quast wrote: > Hi everyone! > > On 12/18/23 10:31, Christina Quast wrote: > > Adds the serial driver for FT260 USB HID devices, providing direct and > > simplified access to UART functionality without the need for FT260 HID > > report format knowledge. > > > > This chip implements an UART and I2C interface, but only the latter was > > previously supported with a kernel driver. For the UART interface, only > > FTDI example code using hidraw from userspace was available. > > > > This commit adds a serial interface /dev/ttyFTx (FT as in FT260), which > > implements tty serial driver ops, facilitating baudrate configuration, > > data transmission and reception, termios settings. > > > > Signed-off-by: Daniel Beer <daniel.beer@igorinstitute.com> > > Signed-off-by: Christina Quast <contact@christina-quast.de> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Reviewed-by: David Lamparter <equinox@diac24.net> > > Is this already queued for 6.8? > > Cheers and happy new year! > > Christina > Thanks for this work! I am adding Jiri Kosina, the maintainer of the HID subsystem. The FTDI FT260 chip implements three functionalities: USB to I2C, UART, and GPIO bridges through two USB HID class interfaces. I use the https://github.com/MichaelZaidman/hid-ft260 repository for FT260 driver development. The I2C support has been upstreamed since 5.13. The code is mature enough and is I2C performance-tuned. The initial GPIO support was developed and committed into the mainline https://patchwork.kernel.org/project/linux-input/cover/20230211115752.26276-1-michael.zaidman@gmail.com/. Two versions of the initial UART support were around for some time, and now they are unified in this commit. I am going to test it with I2C traffic to exclude a suspect of possible impact on the I2C performance and will provide feedback in a week or two. --Michael > > --- > > > > V1 -> V2: Adressed review comments, added power saving mode quirk > > V2 -> V3: Added return 0 in ft260_i2c_probe function > > V3 -> V4: > > - Adressed review comments > > - Added get_icount > > - Fixed tty port lifetime bug > > > > drivers/hid/hid-ft260.c | 833 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 781 insertions(+), 52 deletions(-)
On Tue, Jan 16, 2024 at 11:34:32PM +0200, Michael Zaidman wrote: > > +/* The FT260 has a "power saving mode" that causes the device to switch > > + * to a 30 kHz oscillator if there's no activity for 5 seconds. > > + * Unfortunately this mode can only be disabled by reprogramming > > + * internal fuses, which requires an additional programming voltage. > > + * > > + * One effect of this mode is to cause data loss on a fast UART that > > + * transmits after being idle for longer than 5 seconds. We work around > > + * this by sending a dummy report at least once per 4 seconds if the > > + * UART is in use. > > + */ > > For I2C, we addressed a similar issue in > https://lore.kernel.org/all/20221105211151.7094-8-michael.zaidman@gmail.com/ > commit. But we did it per IO synchronously when the distance between this and > the previous IO exceeded 5 seconds. In this way, the chip can still sleep > between the IOs. On the contrary, the suggested workaround prevents the chip > from entering the power saving mode during active TTY sessions regardless of > the traffic intensity on the UART bus. > > I cannot reproduce the issue with 1K Tx bursts at 921600 baud rate sent every > 10 seconds with the disabled chip wakeup workaround. > > Can you guide me on how to reproduce the data loss you observed? Hi Michael, This was my comment originally. It's been a long time (at least a year), but from memory I had an FT260 attached to a UART console on an MCU dev kit, which would print messages at 115200. If the MCU sat idle for more than 5 seconds and then printed a message, the first few characters of the line would be missing in picocom. If the MCU kept busy, printing more frequently than once every 5 seconds, the problem did not occur. Cheers, Daniel
On Wed, Jan 17, 2024 at 10:43:28PM +0200, Michael Zaidman wrote: > On Wed, Jan 17, 2024 at 10:43:31AM +1300, Daniel Beer wrote: > > On Tue, Jan 16, 2024 at 11:34:32PM +0200, Michael Zaidman wrote: > > > > +/* The FT260 has a "power saving mode" that causes the device to switch > > > > + * to a 30 kHz oscillator if there's no activity for 5 seconds. > > > > + * Unfortunately this mode can only be disabled by reprogramming > > > > + * internal fuses, which requires an additional programming voltage. > > > > + * > > > > + * One effect of this mode is to cause data loss on a fast UART that > > > > + * transmits after being idle for longer than 5 seconds. We work around > > > > + * this by sending a dummy report at least once per 4 seconds if the > > > > + * UART is in use. > > > > + */ > > > > > > For I2C, we addressed a similar issue in > > > https://lore.kernel.org/all/20221105211151.7094-8-michael.zaidman@gmail.com/ > > Link to the correct patch > https://lore.kernel.org/all/20221105211151.7094-11-michael.zaidman@gmail.com/ > > > > commit. But we did it per IO synchronously when the distance between this and > > > the previous IO exceeded 5 seconds. In this way, the chip can still sleep > > > between the IOs. On the contrary, the suggested workaround prevents the chip > > > from entering the power saving mode during active TTY sessions regardless of > > > the traffic intensity on the UART bus. > > > > > > I cannot reproduce the issue with 1K Tx bursts at 921600 baud rate sent every > > > 10 seconds with the disabled chip wakeup workaround. > > > > > > Can you guide me on how to reproduce the data loss you observed? > > > > Hi Michael, > > > > This was my comment originally. It's been a long time (at least a year), > > but from memory I had an FT260 attached to a UART console on an MCU dev > > kit, which would print messages at 115200. > > > > If the MCU sat idle for more than 5 seconds and then printed a message, > > the first few characters of the line would be missing in picocom. If the > > MCU kept busy, printing more frequently than once every 5 seconds, the > > problem did not occur. > > > > Cheers, > > Daniel > > > > Hi Daniel, > > Thanks for the clarification. It was not clear from the issue description > in the commit whether it happens on the ft260 Tx or Rx line, and I assumed > it is Tx. Also, the periodic dummy report workaround is not active in the > submitted patch. I reproduced the issue on the Rx line. And confirm the > workaround works as expected when enabled. > > May I suggest modifying the description to clarify that the data loss > happens on the Rx line and state that the current dummy report period is > 4.8 seconds? > > Also, please enable the reschedule_work flag in the ft260_uart_probe > routine to activate the periodic dummy reports. > > --Michael > Hi Daniel and Christina, My concern with the implemented workaround is that sending a dummy report every 4.8 seconds eliminates the chip's power save mode functionality. I did more testing and figured out that the baud rates 4800 and below work fine with power saving mode and do not require this workaround. So, I modified the code, making it dependent on the baud rate in this commit: https://github.com/MichaelZaidman/hid-ft260/commit/b5a2ad68c7cebbaaba0aa1675ae376f2895e19aa Another improvement is not to activate the wakeup workaround if power saving mode is not enabled in EEPROM. https://github.com/MichaelZaidman/hid-ft260/commit/0a41f3f3a4c664edc3bb90718807f2e62fe6d375 For UART testing, I sent files via picocom opened on ttyUSB0 (ch340 USB dongle device) connected to ft260 UART, receiving and displaying the trafic in screen manager utility. --Michael
On Sat, Jan 20, 2024 at 08:41:28PM +0200, Michael Zaidman wrote: > Hi Daniel and Christina, > > My concern with the implemented workaround is that sending a dummy report > every 4.8 seconds eliminates the chip's power save mode functionality. > > I did more testing and figured out that the baud rates 4800 and below work > fine with power saving mode and do not require this workaround. > > So, I modified the code, making it dependent on the baud rate in this commit: > https://github.com/MichaelZaidman/hid-ft260/commit/b5a2ad68c7cebbaaba0aa1675ae376f2895e19aa > > > Another improvement is not to activate the wakeup workaround if power > saving mode is not enabled in EEPROM. > https://github.com/MichaelZaidman/hid-ft260/commit/0a41f3f3a4c664edc3bb90718807f2e62fe6d375 > > For UART testing, I sent files via picocom opened on ttyUSB0 (ch340 USB > dongle device) connected to ft260 UART, receiving and displaying the > trafic in screen manager utility. > > --Michael Hi Michael, That sounds like a good idea to me. Are you planning to submit your improved version from here? I don't have any FT260 hardware with me any more, so there's unfortunately not much I can do now to help, aside from answering questions. Cheers, Daniel
On Sun, Jan 21, 2024 at 11:13:36AM +1300, Daniel Beer wrote: > On Sat, Jan 20, 2024 at 08:41:28PM +0200, Michael Zaidman wrote: > > Hi Daniel and Christina, > > > > My concern with the implemented workaround is that sending a dummy report > > every 4.8 seconds eliminates the chip's power save mode functionality. > > > > I did more testing and figured out that the baud rates 4800 and below work > > fine with power saving mode and do not require this workaround. > > > > So, I modified the code, making it dependent on the baud rate in this commit: > > https://github.com/MichaelZaidman/hid-ft260/commit/b5a2ad68c7cebbaaba0aa1675ae376f2895e19aa > > > > > > Another improvement is not to activate the wakeup workaround if power > > saving mode is not enabled in EEPROM. > > https://github.com/MichaelZaidman/hid-ft260/commit/0a41f3f3a4c664edc3bb90718807f2e62fe6d375 > > > > For UART testing, I sent files via picocom opened on ttyUSB0 (ch340 USB > > dongle device) connected to ft260 UART, receiving and displaying the > > trafic in screen manager utility. > > > > --Michael > > Hi Michael, > > That sounds like a good idea to me. Are you planning to submit your > improved version from here? > > I don't have any FT260 hardware with me any more, so there's > unfortunately not much I can do now to help, aside from answering > questions. > > Cheers, > Daniel > Yeah, I mean to do more testing, and then I will submit a new patch set based on your work. --Michael
On Sun, Jan 21, 2024 at 11:12:54AM +0200, Michael Zaidman wrote: > On Sun, Jan 21, 2024 at 11:13:36AM +1300, Daniel Beer wrote: > > On Sat, Jan 20, 2024 at 08:41:28PM +0200, Michael Zaidman wrote: > > > Hi Daniel and Christina, > > > > > > My concern with the implemented workaround is that sending a dummy report > > > every 4.8 seconds eliminates the chip's power save mode functionality. > > > > > > I did more testing and figured out that the baud rates 4800 and below work > > > fine with power saving mode and do not require this workaround. > > > > > > So, I modified the code, making it dependent on the baud rate in this commit: > > > https://github.com/MichaelZaidman/hid-ft260/commit/b5a2ad68c7cebbaaba0aa1675ae376f2895e19aa > > > > > > > > > Another improvement is not to activate the wakeup workaround if power > > > saving mode is not enabled in EEPROM. > > > https://github.com/MichaelZaidman/hid-ft260/commit/0a41f3f3a4c664edc3bb90718807f2e62fe6d375 > > > > > > For UART testing, I sent files via picocom opened on ttyUSB0 (ch340 USB > > > dongle device) connected to ft260 UART, receiving and displaying the > > > trafic in screen manager utility. > > > > > > --Michael > > > > Hi Michael, > > > > That sounds like a good idea to me. Are you planning to submit your > > improved version from here? > > > > I don't have any FT260 hardware with me any more, so there's > > unfortunately not much I can do now to help, aside from answering > > questions. > > > > Cheers, > > Daniel > > > > Yeah, I mean to do more testing, and then I will submit a new patch set > based on your work. > > --Michael > Hi Daniel, I pushed changes into https://github.com/MichaelZaidman/hid-ft260/tree/uart branch and would appreciate your review and feedback before preparing the cumulative patch set. I addressed the first FIXME. Please correct me if I did not get right what you meant. I am still doubting regarding the second FIXME. I see only three tty drivers are using the kfifo_avail and kfifo_len to retrieve the write room and number of chars in the buffer, and neither use locking. What was your concern? Thanks, Michael
On Wed, Jan 31, 2024 at 07:28:25PM +1300, Daniel Beer wrote: > On Mon, Jan 29, 2024 at 12:07:48AM +0200, Michael Zaidman wrote: > > I pushed changes into https://github.com/MichaelZaidman/hid-ft260/tree/uart > > branch and would appreciate your review and feedback before preparing the > > cumulative patch set. > > > > I addressed the first FIXME. Please correct me if I did not get right what > > you meant. > > > > I am still doubting regarding the second FIXME. I see only three tty drivers > > are using the kfifo_avail and kfifo_len to retrieve the write room and number > > of chars in the buffer, and neither use locking. What was your concern? > > I don't see anything obviously wrong, but I'm probably not the best > person to be reviewing this -- Christina has spent the most time on it > recently, and I think the FIXME comments are hers. > Thanks, Daniel, for your feedback. I see, it's my mistake. I should have reached out to both of you. I added multiple fine-grained commits to simplify the review. Christina, can you clarify the FIXME issue and review the changes? Thanks, Michael
Hi Michael! Thanks for your work! I will have a look at your latest code and answer your latest mail a bit later. On 1/16/24 22:34, Michael Zaidman wrote: > On Mon, Dec 18, 2023 at 10:31:53AM +0100, Christina Quast wrote: >> Adds the serial driver for FT260 USB HID devices, providing direct and >> simplified access to UART functionality without the need for FT260 HID >> report format knowledge. >> >> This chip implements an UART and I2C interface, but only the latter was >> previously supported with a kernel driver. For the UART interface, only >> FTDI example code using hidraw from userspace was available. >> >> This commit adds a serial interface /dev/ttyFTx (FT as in FT260), which >> implements tty serial driver ops, facilitating baudrate configuration, >> data transmission and reception, termios settings. >> >> Signed-off-by: Daniel Beer <daniel.beer@igorinstitute.com> >> Signed-off-by: Christina Quast <contact@christina-quast.de> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Reviewed-by: David Lamparter <equinox@diac24.net> >> --- >> >> V1 -> V2: Adressed review comments, added power saving mode quirk >> V2 -> V3: Added return 0 in ft260_i2c_probe function >> V3 -> V4: >> - Adressed review comments >> - Added get_icount >> - Fixed tty port lifetime bug >> > I applied the patch to the latest mainline driver code > and got two compilation issues, which I fixed in the > https://github.com/MichaelZaidman/hid-ft260/tree/uart > > incompatible-pointer-types error: > > You are using: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 > CC [M] /home/michael/sw/hid-ft260/hid-ft260.o > /home/michael/sw/hid-ft260/hid-ft260.c:1426:35: error: initialization of ‘int (*)(struct tty_struct *, > const unsigned char *, int)’ from incompatible pointer type ‘ssize_t (*)(struct tty_struct *, > const unsigned char *, size_t)’ {aka ‘long int (*)(struct tty_struct *, const unsigned char *, > long unsigned int)’} [-Werror=incompatible-pointer-types] > 1426 | .write = ft260_uart_write, > | ^~~~~~~~~~~~~~~~ > /home/michael/sw/hid-ft260/hid-ft260.c:1426:35: note: (near initialization for ‘ft260_uart_ops.write’) > cc1: some warnings being treated as errors > make[3]: *** [scripts/Makefile.build:251: /home/michael/sw/hid-ft260/hid-ft260.o] Error 1 > make[2]: *** [/usr/src/linux-headers-6.5.0-14-generic/Makefile:2037: /home/michael/sw/hid-ft260] Error 2 > make[1]: *** [Makefile:234: __sub-make] Error 2 > make[1]: Leaving directory '/usr/src/linux-headers-6.5.0-14-generic' > make: *** [Makefile:6: all] Error 2 > > > And Wformat warning: > > You are using: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 > CC [M] /home/michael/sw/hid-ft260/hid-ft260.o > In file included from ./include/linux/kernel.h:30, > from ./arch/x86/include/asm/percpu.h:27, > from ./arch/x86/include/asm/preempt.h:6, > from ./include/linux/preempt.h:79, > from ./include/linux/spinlock.h:56, > from ./include/linux/mmzone.h:8, > from ./include/linux/gfp.h:7, > from ./include/linux/slab.h:16, > from ./include/linux/hid.h:19, > from ./include/uapi/linux/hidraw.h:19, > from ./include/linux/hidraw.h:8, > from /home/michael/sw/hid-ft260/hid-ft260.c:12: > /home/michael/sw/hid-ft260/hid-ft260.c: In function ‘ft260_uart_write’: > ./include/linux/kern_levels.h:5:25: warning: format ‘%ld’ expects argument of type ‘long int’, > but argument 3 has type ‘int’ [-Wformat=] > 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ > | ^~~~~~ > ./include/linux/printk.h:427:25: note: in definition of macro ‘printk_index_wrap’ > 427 | _p_func(_fmt, ##__VA_ARGS__); \ > | ^~~~ > ./include/linux/printk.h:528:9: note: in expansion of macro ‘printk’ > 528 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) > | ^~~~~~ > ./include/linux/kern_levels.h:14:25: note: in expansion of macro ‘KERN_SOH’ > 14 | #define KERN_INFO KERN_SOH "6" /* informational */ > | ^~~~~~~~ > ./include/linux/printk.h:528:16: note: in expansion of macro ‘KERN_INFO’ > 528 | printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) > | ^~~~~~~~~ > /home/michael/sw/hid-ft260/hid-ft260.c:38:25: note: in expansion of macro ‘pr_info’ > 38 | pr_info("%s: " format, __func__, ##arg); \ > | ^~~~~~~ > /home/michael/sw/hid-ft260/hid-ft260.c:1231:9: note: in expansion of macro ‘ft260_dbg’ > 1231 | ft260_dbg("count: %ld, len: %d", count, len); > | ^~~~~~~~~ > >> drivers/hid/hid-ft260.c | 833 +++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 781 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c >> index 333341e80b0e..cc2cce3698e3 100644 >> --- a/drivers/hid/hid-ft260.c >> +++ b/drivers/hid/hid-ft260.c >> @@ -13,6 +13,16 @@ >> #include <linux/i2c.h> >> #include <linux/module.h> >> #include <linux/usb.h> >> +#include <linux/serial.h> >> +#include <linux/serial_core.h> >> +#include <linux/kfifo.h> >> +#include <linux/tty_flip.h> >> +#include <linux/minmax.h> >> +#include <asm/unaligned.h> /* Needed for cpu_to_le16, le16_to_cpu */ >> + >> +#define UART_COUNT_MAX 4 /* Number of UARTs this driver can handle */ >> +#define FIFO_SIZE 256 >> +#define TTY_WAKEUP_WATERMARK (FIFO_SIZE / 2) >> >> #ifdef DEBUG >> static int ft260_debug = 1; >> @@ -30,6 +40,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages"); >> >> #define FT260_REPORT_MAX_LENGTH (64) >> #define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4) >> +#define FT260_UART_DATA_REPORT_ID(len) (FT260_UART_REPORT_MIN + (len - 1) / 4) >> >> #define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */ >> >> @@ -43,7 +54,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages"); >> * or optoe limit the i2c reads to 128 bytes. To not block other drivers out >> * of I2C for potentially troublesome amounts of time, we select the maximum >> * read payload length to be 180 bytes. >> -*/ >> + */ >> #define FT260_RD_DATA_MAX (180) >> #define FT260_WR_DATA_MAX (60) >> >> @@ -81,7 +92,8 @@ enum { >> FT260_UART_INTERRUPT_STATUS = 0xB1, >> FT260_UART_STATUS = 0xE0, >> FT260_UART_RI_DCD_STATUS = 0xE1, >> - FT260_UART_REPORT = 0xF0, >> + FT260_UART_REPORT_MIN = 0xF0, >> + FT260_UART_REPORT_MAX = 0xFE, >> }; >> >> /* Feature Out */ >> @@ -132,6 +144,13 @@ enum { >> FT260_FLAG_START_STOP_REPEATED = 0x07, >> }; >> >> +/* Return values for ft260_get_interface_type func */ >> +enum { >> + FT260_IFACE_NONE, >> + FT260_IFACE_I2C, >> + FT260_IFACE_UART >> +}; >> + >> #define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | report_id) >> >> /* Feature In reports */ >> @@ -220,12 +239,59 @@ struct ft260_i2c_read_request_report { >> __le16 length; /* data payload length */ >> } __packed; >> >> -struct ft260_i2c_input_report { >> - u8 report; /* FT260_I2C_REPORT */ >> +struct ft260_input_report { >> + u8 report; /* FT260_I2C_REPORT or FT260_UART_REPORT */ >> u8 length; /* data payload length */ >> u8 data[2]; /* data payload */ >> } __packed; >> >> +/* UART reports */ >> +struct ft260_uart_write_request_report { >> + u8 report; /* FT260_UART_REPORT */ >> + u8 length; /* data payload length */ >> + u8 data[] __counted_by(length); /* variable data payload */ >> +} __packed; >> + >> +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 */ >> + /* The baudrate field is unaligned: */ >> + __le32 baudrate; /* little endian, 9600 = 0x2580, 19200 = 0x4B00 */ >> + u8 data_bit; /* 7 or 8 */ >> + u8 parity; /* 0: no parity, 1: odd, 2: even, 3: high, 4: low */ >> + u8 stop_bit; /* 0: one stop bit, 2: 2 stop bits */ >> + u8 breaking; /* 0: no break */ >> +} __packed; >> + >> +/* UART interface configuration */ >> +enum { >> + FT260_CFG_FLOW_CTRL_OFF = 0x00, >> + FT260_CFG_FLOW_CTRL_RTS_CTS = 0x01, >> + FT260_CFG_FLOW_CTRL_DTR_DSR = 0x02, >> + FT260_CFG_FLOW_CTRL_XON_XOFF = 0x03, >> + FT260_CFG_FLOW_CTRL_NONE = 0x04, >> + >> + FT260_CFG_DATA_BITS_7 = 0x07, >> + FT260_CFG_DATA_BITS_8 = 0x08, >> + >> + FT260_CFG_PAR_NO = 0x00, >> + FT260_CFG_PAR_ODD = 0x01, >> + FT260_CFG_PAR_EVEN = 0x02, >> + FT260_CFG_PAR_HIGH = 0x03, >> + FT260_CFG_PAR_LOW = 0x04, >> + >> + FT260_CFG_STOP_ONE_BIT = 0x00, >> + FT260_CFG_STOP_TWO_BIT = 0x02, >> + >> + FT260_CFG_BREAKING_NO = 0x00, >> + FT260_CFG_BEAKING_YES = 0x01, >> + >> + FT260_CFG_BAUD_MIN = 1200, >> + FT260_CFG_BAUD_MAX = 12000000, >> +}; >> + >> static const struct hid_device_id ft260_devices[] = { >> { HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY, >> USB_DEVICE_ID_FT260) }, >> @@ -236,6 +302,23 @@ MODULE_DEVICE_TABLE(hid, ft260_devices); >> struct ft260_device { >> struct i2c_adapter adap; >> struct hid_device *hdev; >> + >> + bool ft260_is_serial; >> + struct list_head device_list; >> + >> + /* tty_port lifetime is equal to device lifetime */ >> + struct tty_port port; >> + unsigned int index; >> + struct kfifo xmit_fifo; >> + /* write_lock: lock to serialize access to xmit fifo */ >> + spinlock_t write_lock; >> + struct uart_icount icount; >> + >> + struct timer_list wakeup_timer; >> + struct work_struct wakeup_work; >> + bool reschedule_work; >> + >> + >> struct completion wait; >> struct mutex lock; >> u8 write_buf[FT260_REPORT_MAX_LENGTH]; >> @@ -377,7 +460,7 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev, >> >> ret = ft260_hid_output_report(hdev, data, len); >> if (ret < 0) { >> - hid_err(hdev, "%s: failed to start transfer, ret %d\n", >> + hid_dbg(hdev, "%s: failed to start transfer, ret %d\n", >> __func__, ret); >> ft260_i2c_reset(hdev); >> return ret; >> @@ -592,7 +675,7 @@ static int ft260_i2c_write_read(struct ft260_device *dev, struct i2c_msg *msgs) >> else >> read_off = *msgs[0].buf; >> >> - pr_info("%s: off %#x rlen %d wlen %d\n", __func__, >> + ft260_dbg("%s: off %#x rlen %d wlen %d\n", __func__, >> read_off, rd_len, wr_len); >> } >> >> @@ -782,7 +865,7 @@ static int ft260_get_system_config(struct hid_device *hdev, >> return 0; >> } >> >> -static int ft260_is_interface_enabled(struct hid_device *hdev) >> +static int ft260_get_interface_type(struct hid_device *hdev, struct ft260_device *dev) >> { >> struct ft260_get_system_status_report cfg; >> struct usb_interface *usbif = to_usb_interface(hdev->dev.parent); >> @@ -799,21 +882,27 @@ static int ft260_is_interface_enabled(struct hid_device *hdev) >> ft260_dbg("i2c_enable: 0x%02x\n", cfg.i2c_enable); >> ft260_dbg("uart_mode: 0x%02x\n", cfg.uart_mode); >> >> + dev->ft260_is_serial = false; >> + >> switch (cfg.chip_mode) { >> case FT260_MODE_ALL: >> case FT260_MODE_BOTH: >> - if (interface == 1) >> - hid_info(hdev, "uart interface is not supported\n"); >> - else >> - ret = 1; >> + if (interface == 1) { >> + ret = FT260_IFACE_UART; >> + dev->ft260_is_serial = true; >> + } else { >> + ret = FT260_IFACE_I2C; >> + } >> break; >> case FT260_MODE_UART: >> - hid_info(hdev, "uart interface is not supported\n"); >> + ret = FT260_IFACE_UART; >> + dev->ft260_is_serial = true; >> break; >> case FT260_MODE_I2C: >> - ret = 1; >> + ret = FT260_IFACE_I2C; >> break; >> } >> + >> return ret; >> } >> >> @@ -957,51 +1046,486 @@ static const struct attribute_group ft260_attr_group = { >> } >> }; >> >> -static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) >> +/*** >> + * START Serial dev part >> + */ >> +static DEFINE_MUTEX(ft260_uart_list_lock); >> +static LIST_HEAD(ft260_uart_device_list); >> + >> +static struct ft260_device *ft260_dev_by_index(int index) >> { >> + struct ft260_device *port; >> + >> + list_for_each_entry(port, &ft260_uart_device_list, device_list) { >> + if (index == port->index) >> + return port; >> + } >> + return NULL; >> +} >> + >> +static int ft260_uart_add_port(struct ft260_device *port) >> +{ >> + int index = 0, ret = 0; >> struct ft260_device *dev; >> - struct ft260_get_chip_version_report version; >> - int ret; >> >> - if (!hid_is_usb(hdev)) >> - return -EINVAL; >> - >> - dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); >> - if (!dev) >> + spin_lock_init(&port->write_lock); >> + if (kfifo_alloc(&port->xmit_fifo, FIFO_SIZE, GFP_KERNEL)) >> return -ENOMEM; >> >> - ret = hid_parse(hdev); >> - if (ret) { >> - hid_err(hdev, "failed to parse HID\n"); >> - return ret; >> + mutex_lock(&ft260_uart_list_lock); >> + list_for_each_entry(dev, &ft260_uart_device_list, device_list) { >> + if (dev->index != index) >> + break; >> + index++; >> } >> >> - ret = hid_hw_start(hdev, 0); >> - if (ret) { >> - hid_err(hdev, "failed to start HID HW\n"); >> - return ret; >> + port->index = index; >> + list_add(&port->device_list, &ft260_uart_device_list); >> + mutex_unlock(&ft260_uart_list_lock); >> + >> + return ret; >> +} >> + >> +static void ft260_uart_port_put(struct ft260_device *port) >> +{ >> + tty_port_put(&port->port); >> +} >> + >> +static void ft260_uart_port_remove(struct ft260_device *port) >> +{ >> + timer_delete_sync(&port->wakeup_timer); >> + >> + mutex_lock(&ft260_uart_list_lock); >> + list_del(&port->device_list); >> + mutex_unlock(&ft260_uart_list_lock); >> + >> + spin_lock(&port->write_lock); >> + kfifo_free(&port->xmit_fifo); >> + spin_unlock(&port->write_lock); >> + >> + mutex_lock(&port->port.mutex); >> + port->reschedule_work = false; >> + tty_port_tty_hangup(&port->port, false); >> + mutex_unlock(&port->port.mutex); >> + >> + ft260_uart_port_put(port); >> +} >> + >> +static struct ft260_device *ft260_uart_port_get(unsigned int index) >> +{ >> + struct ft260_device *port; >> + >> + if (index >= UART_COUNT_MAX) >> + return NULL; >> + >> + mutex_lock(&ft260_uart_list_lock); >> + port = ft260_dev_by_index(index); >> + if (port) >> + tty_port_get(&port->port); >> + mutex_unlock(&ft260_uart_list_lock); >> + >> + return port; >> +} >> + >> +static int ft260_uart_open(struct tty_struct *tty, struct file *filp) >> +{ >> + int ret; >> + struct ft260_device *port = tty->driver_data; >> + >> + ret = tty_port_open(&port->port, tty, filp); >> + >> + return ret; >> +} >> + >> +static void ft260_uart_close(struct tty_struct *tty, struct file *filp) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + tty_port_close(&port->port, tty, filp); >> +} >> + >> +static void ft260_uart_hangup(struct tty_struct *tty) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + tty_port_hangup(&port->port); >> +} >> + >> +static int ft260_uart_transmit_chars(struct ft260_device *port) >> +{ >> + struct hid_device *hdev = port->hdev; >> + struct kfifo *xmit = &port->xmit_fifo; >> + struct tty_struct *tty; >> + struct ft260_uart_write_request_report *rep; >> + int len, data_len, ret = 0; >> + >> + tty = tty_port_tty_get(&port->port); >> + >> + data_len = kfifo_len(xmit); >> + if (!tty || !data_len) { >> + ret = -EINVAL; >> + goto tty_out; >> } >> >> - ret = hid_hw_open(hdev); >> - if (ret) { >> - hid_err(hdev, "failed to open HID HW\n"); >> - goto err_hid_stop; >> + rep = (struct ft260_uart_write_request_report *)port->write_buf; > The UART code uses the write_buf unsafely, compromising the data integrity > of both I2C and UART channels. > > The I2C channel uses the write_buf to send the HID reports. It uses mutex > to make it atomically. For UART to use this buffer, it should grab the > same mutex first. But then it will degrade the performance of both > channels. The better approach is to have a separate Tx buffer for UART. > > I fixed it in ft260 repo and briefly tested the data integrity simultaneously > writing via I2C and UART channels. > https://github.com/MichaelZaidman/hid-ft260/commit/4a8430cbf2c509932141dd3868ce0133821c3b2f Good idea! > >> + >> + do { >> + len = min(data_len, FT260_WR_DATA_MAX); >> + >> + rep->report = FT260_UART_DATA_REPORT_ID(len); >> + rep->length = len; >> + >> + len = kfifo_out_locked(xmit, rep->data, len, &port->write_lock); > The kfifo_out_locked is allias for kfifo_out_spinlocked and will be > removed in the future. It's better to use kfifo_out_spinlocked. > > The same for kfifo_in_locked. > >> + >> + ret = ft260_hid_output_report(hdev, (u8 *)rep, len + sizeof(*rep)); >> + if (ret < 0) { >> + hid_err(hdev, "Failed to start transfer, ret %d\n", ret); >> + goto tty_out; >> + } >> + >> + data_len -= len; >> + port->icount.tx += len; >> + } while (data_len > 0); >> + >> + len = kfifo_len(xmit); >> + if ((FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK) >> + tty_wakeup(tty); >> + >> + ret = 0; >> + >> +tty_out: >> + tty_kref_put(tty); >> + return ret; >> +} >> + >> +static int ft260_uart_receive_chars(struct ft260_device *port, >> + u8 *data, u8 length) >> +{ >> + struct hid_device *hdev = port->hdev; >> + int ret = 0; >> + >> + if (length > FT260_RD_DATA_MAX) { >> + hid_err(hdev, "Received too much data (%d)\n", length); >> + return -EBADR; >> } > Remove conditional expression, which will never be true here since > it is already filtered in the ft260_raw_event procedure. > >> >> - ret = ft260_hid_feature_report_get(hdev, FT260_CHIP_VERSION, >> - (u8 *)&version, sizeof(version)); >> + ret = tty_insert_flip_string(&port->port, data, length); >> + if (ret != length) >> + hid_err(hdev, "%d char not inserted to flip buffer\n", length - ret); > I would use hid_dbg verbosity here since it can happen and is a matter of overload. > >> + port->icount.rx += ret; >> + >> + if (ret) >> + tty_flip_buffer_push(&port->port); >> + >> + return ret; >> +} >> + >> +static ssize_t ft260_uart_write(struct tty_struct *tty, const unsigned char *buf, >> + size_t count) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + struct hid_device *hdev = port->hdev; >> + int len, ret; >> + >> + len = kfifo_in_locked(&port->xmit_fifo, buf, count, &port->write_lock); >> + ft260_dbg("count: %ld, len: %d", count, len); >> + >> + ret = ft260_uart_transmit_chars(port); >> if (ret < 0) { >> - hid_err(hdev, "failed to retrieve chip version\n"); >> - goto err_hid_close; >> + hid_dbg(hdev, "Failed to transmit chars: %d\n", ret); >> + return 0; >> } >> >> - hid_info(hdev, "chip code: %02x%02x %02x%02x\n", >> - version.chip_code[0], version.chip_code[1], >> - version.chip_code[2], version.chip_code[3]); >> + ret = kfifo_len(&port->xmit_fifo); >> + if (ret > 0) { >> + hid_dbg(hdev, "Failed to all kfifo data bytes\n"); > Fix typo "Failed to write all kfifo data bytes" > >> + ft260_dbg("return: %d", len - ret); >> + return len - ret; >> + } >> + >> + return len; >> +} >> + >> +static unsigned int ft260_uart_write_room(struct tty_struct *tty) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + return FIFO_SIZE - kfifo_len(&port->xmit_fifo); >> +} >> + >> +static unsigned int ft260_uart_chars_in_buffer(struct tty_struct *tty) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + return kfifo_len(&port->xmit_fifo); >> +} >> + >> +static int ft260_uart_change_speed(struct ft260_device *port, >> + struct ktermios *termios, >> + struct ktermios *old) >> +{ >> + struct hid_device *hdev = port->hdev; >> + unsigned int baud; >> + struct ft260_configure_uart_request req; >> + int ret; >> + >> + memset(&req, 0, sizeof(req)); >> + >> + req.report = FT260_SYSTEM_SETTINGS; >> + req.request = FT260_SET_UART_CONFIG; >> + >> + switch (termios->c_cflag & CSIZE) { >> + case CS7: >> + req.data_bit = FT260_CFG_DATA_BITS_7; >> + break; >> + case CS5: >> + case CS6: >> + hid_err(hdev, "Invalid data bit size, setting to default (8 bit)\n"); >> + req.data_bit = FT260_CFG_DATA_BITS_8; >> + termios->c_cflag &= ~CSIZE; >> + termios->c_cflag |= CS8; >> + break; >> + default: >> + case CS8: >> + req.data_bit = FT260_CFG_DATA_BITS_8; >> + break; >> + } >> + >> + req.stop_bit = (termios->c_cflag & CSTOPB) ? >> + FT260_CFG_STOP_TWO_BIT : FT260_CFG_STOP_ONE_BIT; >> + >> + if (termios->c_cflag & PARENB) { >> + req.parity = (termios->c_cflag & PARODD) ? >> + FT260_CFG_PAR_ODD : FT260_CFG_PAR_EVEN; >> + } else { >> + req.parity = FT260_CFG_PAR_NO; >> + } >> + >> + baud = tty_termios_baud_rate(termios); >> + if (baud == 0 || baud < FT260_CFG_BAUD_MIN || baud > FT260_CFG_BAUD_MAX) { >> + struct tty_struct *tty = tty_port_tty_get(&port->port); >> + >> + hid_err(hdev, "Invalid baud rate %d\n", baud); >> + baud = 9600; >> + tty_encode_baud_rate(tty, baud, baud); >> + tty_kref_put(tty); >> + } >> + put_unaligned_le32(cpu_to_le32(baud), &req.baudrate); >> + >> + if (termios->c_cflag & CRTSCTS) >> + req.flow_ctrl = FT260_CFG_FLOW_CTRL_RTS_CTS; >> + else >> + req.flow_ctrl = FT260_CFG_FLOW_CTRL_OFF; >> + >> + ft260_dbg("Configured termios: flow control: %d, baudrate: %d, ", >> + req.flow_ctrl, baud); >> + ft260_dbg("data_bit: %d, parity: %d, stop_bit: %d, breaking: %d\n", >> + req.data_bit, req.parity, >> + req.stop_bit, req.breaking); >> + >> + req.flow_ctrl = FT260_CFG_FLOW_CTRL_NONE; >> + req.breaking = FT260_CFG_BREAKING_NO; >> + >> + ret = ft260_hid_feature_report_set(hdev, (u8 *)&req, sizeof(req)); >> + if (ret < 0) >> + hid_err(hdev, "ft260_hid_feature_report_set failed: %d\n", ret); >> + >> + return ret; >> +} >> + >> +static int ft260_uart_get_icount(struct tty_struct *tty, >> + struct serial_icounter_struct *icount) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + memcpy(icount, &port->icount, sizeof(struct uart_icount)); >> + >> + return 0; >> +} >> + >> +static void ft260_uart_set_termios(struct tty_struct *tty, >> + const struct ktermios *old_termios) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + ft260_uart_change_speed(port, &tty->termios, NULL); >> +} >> + >> +static int ft260_uart_install(struct tty_driver *driver, struct tty_struct *tty) >> +{ >> + int idx = tty->index; >> + struct ft260_device *port = ft260_uart_port_get(idx); >> + int ret = tty_standard_install(driver, tty); >> + >> + if (ret == 0) >> + /* This is the ref ft260_uart_port get provided */ >> + tty->driver_data = port; >> + else >> + ft260_uart_port_put(port); >> + >> + return ret; >> +} >> + >> +static void ft260_uart_cleanup(struct tty_struct *tty) >> +{ >> + struct ft260_device *port = tty->driver_data; >> + >> + tty->driver_data = NULL; /* Bug trap */ >> + ft260_uart_port_put(port); >> +} >> + >> +static int ft260_uart_proc_show(struct seq_file *m, void *v) >> +{ >> + int i; >> + >> + seq_printf(m, "ft260 info:1.0 driver%s%s revision:%s\n", >> + "", "", ""); >> + for (i = 0; i < UART_COUNT_MAX; i++) { >> + struct ft260_device *port = ft260_uart_port_get(i); >> + >> + if (port) { >> + seq_printf(m, "%d: uart:FT260", i); >> + if (capable(CAP_SYS_ADMIN)) { >> + seq_printf(m, " tx:%d rx:%d", >> + port->icount.tx, port->icount.rx); >> + if (port->icount.frame) >> + seq_printf(m, " fe:%d", >> + port->icount.frame); >> + if (port->icount.parity) >> + seq_printf(m, " pe:%d", >> + port->icount.parity); >> + if (port->icount.brk) >> + seq_printf(m, " brk:%d", >> + port->icount.brk); >> + if (port->icount.overrun) >> + seq_printf(m, " oe:%d", >> + port->icount.overrun); >> + if (port->icount.cts) >> + seq_printf(m, " cts:%d", >> + port->icount.cts); >> + if (port->icount.dsr) >> + seq_printf(m, " dsr:%d", >> + port->icount.dsr); >> + if (port->icount.rng) >> + seq_printf(m, " rng:%d", >> + port->icount.rng); >> + if (port->icount.dcd) >> + seq_printf(m, " dcd:%d", >> + port->icount.dcd); >> + } >> + ft260_uart_port_put(port); >> + seq_putc(m, '\n'); >> + } >> + } >> + return 0; >> +} >> + >> +static const struct tty_operations ft260_uart_ops = { >> + .open = ft260_uart_open, >> + .close = ft260_uart_close, >> + .write = ft260_uart_write, >> + .write_room = ft260_uart_write_room, >> + .chars_in_buffer = ft260_uart_chars_in_buffer, >> + .set_termios = ft260_uart_set_termios, >> + .hangup = ft260_uart_hangup, >> + .install = ft260_uart_install, >> + .cleanup = ft260_uart_cleanup, >> + .proc_show = ft260_uart_proc_show, >> + .get_icount = ft260_uart_get_icount, >> +}; >> + >> +/* The FT260 has a "power saving mode" that causes the device to switch >> + * to a 30 kHz oscillator if there's no activity for 5 seconds. >> + * Unfortunately this mode can only be disabled by reprogramming >> + * internal fuses, which requires an additional programming voltage. >> + * >> + * One effect of this mode is to cause data loss on a fast UART that >> + * transmits after being idle for longer than 5 seconds. We work around >> + * this by sending a dummy report at least once per 4 seconds if the >> + * UART is in use. >> + */ > For I2C, we addressed a similar issue in > https://lore.kernel.org/all/20221105211151.7094-8-michael.zaidman@gmail.com/ > commit. But we did it per IO synchronously when the distance between this and > the previous IO exceeded 5 seconds. In this way, the chip can still sleep > between the IOs. On the contrary, the suggested workaround prevents the chip > from entering the power saving mode during active TTY sessions regardless of > the traffic intensity on the UART bus. > > I cannot reproduce the issue with 1K Tx bursts at 921600 baud rate sent every > 10 seconds with the disabled chip wakeup workaround. > > Can you guide me on how to reproduce the data loss you observed? > >> +static void ft260_uart_start_wakeup(struct timer_list *t) >> +{ >> + struct ft260_device *dev = >> + container_of(t, struct ft260_device, wakeup_timer); >> + >> + if (dev->reschedule_work) { >> + schedule_work(&dev->wakeup_work); >> + mod_timer(&dev->wakeup_timer, jiffies + >> + msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS)); >> + } >> +} >> + >> +static void ft260_uart_do_wakeup(struct work_struct *work) >> +{ >> + struct ft260_device *dev = >> + container_of(work, struct ft260_device, wakeup_work); >> + struct ft260_get_chip_version_report version; >> + int ret; >> + >> + if (dev->reschedule_work) { >> + ret = ft260_hid_feature_report_get(dev->hdev, FT260_CHIP_VERSION, >> + (u8 *)&version, sizeof(version)); >> + if (ret < 0) >> + hid_err(dev->hdev, >> + "%s: failed to start transfer, ret %d\n", >> + __func__, ret); >> + } >> +} >> + >> +static void ft260_uart_shutdown(struct tty_port *tport) >> +{ >> + struct ft260_device *port = >> + container_of(tport, struct ft260_device, port); >> + >> + port->reschedule_work = false; >> +} >> + >> +static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty) >> +{ >> + struct ft260_device *port = >> + container_of(tport, struct ft260_device, port); >> + >> + /* >> + * Set the TTY IO error marker - we will only clear this >> + * once we have successfully opened the port. >> + */ >> + set_bit(TTY_IO_ERROR, &tty->flags); >> + >> + spin_lock(&port->write_lock); >> + kfifo_reset(&port->xmit_fifo); >> + spin_unlock(&port->write_lock); >> + >> + ft260_uart_change_speed(port, &tty->termios, NULL); >> + clear_bit(TTY_IO_ERROR, &tty->flags); >> + >> + if (port->reschedule_work) { >> + mod_timer(&port->wakeup_timer, jiffies + >> + msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS)); >> + } >> + >> + return 0; >> +} >> + >> +static void ft260_uart_port_destroy(struct tty_port *tport) >> +{ >> + struct ft260_device *port = >> + container_of(tport, struct ft260_device, port); >> + >> + kfree(port); >> +} >> + >> +static const struct tty_port_operations ft260_uart_port_ops = { >> + .shutdown = ft260_uart_shutdown, >> + .activate = ft260_uart_activate, >> + .destruct = ft260_uart_port_destroy, >> +}; >> + >> +static struct tty_driver *ft260_tty_driver; >> >> - ret = ft260_is_interface_enabled(hdev); >> - if (ret <= 0) >> - goto err_hid_close; >> +static int ft260_i2c_probe(struct hid_device *hdev, struct ft260_device *dev) >> +{ >> + int ret; >> >> hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", >> hdev->version >> 8, hdev->version & 0xff, hdev->name, >> @@ -1028,7 +1552,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) >> ret = i2c_add_adapter(&dev->adap); >> if (ret) { >> hid_err(hdev, "failed to add i2c adapter\n"); >> - goto err_hid_close; >> + return ret; >> } >> >> ret = sysfs_create_group(&hdev->dev.kobj, &ft260_attr_group); >> @@ -1036,15 +1560,149 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) >> hid_err(hdev, "failed to create sysfs attrs\n"); >> goto err_i2c_free; >> } >> - >> return 0; >> >> err_i2c_free: >> i2c_del_adapter(&dev->adap); >> + return ret; >> +} >> + >> +static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev) >> +{ >> + struct ft260_configure_uart_request req; >> + int ret; >> + struct device *devt; >> + >> + INIT_WORK(&dev->wakeup_work, ft260_uart_do_wakeup); >> + // FIXME: Do I need that if I have cancel_work_sync? >> + // FIXME: are all kfifo access secured by lock? with irq or not? > Should we keep these FIXME? >> + dev->reschedule_work = false; > The dummy report is never sent since the reschedule_work flag is set to > false here. Neither wakeup_work nor wakeup_timer are activated. Please > elaborate. > >> + /* Work not started at this point */ >> + timer_setup(&dev->wakeup_timer, ft260_uart_start_wakeup, 0); >> + >> + tty_port_init(&dev->port); >> + dev->port.ops = &ft260_uart_port_ops; >> + >> + ret = ft260_uart_add_port(dev); >> + if (ret) { >> + hid_err(hdev, "failed to add port\n"); >> + return ret; >> + } >> + devt = tty_port_register_device_attr(&dev->port, >> + ft260_tty_driver, >> + dev->index, &hdev->dev, >> + dev, NULL); >> + if (IS_ERR(devt)) { >> + hid_err(hdev, "failed to register tty port\n"); >> + ret = PTR_ERR(devt); >> + goto err_register_tty; >> + } >> + hid_info(hdev, "Registering device /dev/%s%d\n", >> + ft260_tty_driver->name, dev->index); >> + >> + /* Send Feature Report to Configure FT260 as UART 9600-8-N-1 */ >> + req.report = FT260_SYSTEM_SETTINGS; >> + req.request = FT260_SET_UART_CONFIG; >> + req.flow_ctrl = FT260_CFG_FLOW_CTRL_NONE; >> + put_unaligned_le32(cpu_to_le32(9600), &req.baudrate); >> + req.data_bit = FT260_CFG_DATA_BITS_8; >> + req.parity = FT260_CFG_PAR_NO; >> + req.stop_bit = FT260_CFG_STOP_ONE_BIT; >> + req.breaking = FT260_CFG_BREAKING_NO; >> + >> + ret = ft260_hid_feature_report_set(hdev, (u8 *)&req, sizeof(req)); >> + if (ret < 0) { >> + hid_err(hdev, "ft260_hid_feature_report_set failed: %d\n", >> + ret); >> + goto err_hid_report; >> + } >> + >> + return 0; >> + >> +err_hid_report: >> + tty_port_unregister_device(&dev->port, ft260_tty_driver, dev->index); >> +err_register_tty: >> + ft260_uart_port_remove(dev); >> + return ret; >> +} >> + >> +static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) >> +{ >> + struct ft260_device *dev; >> + struct ft260_get_chip_version_report version; >> + int ret; >> + >> + if (!hid_is_usb(hdev)) >> + return -EINVAL; >> + >> + /* We cannot used devm_kzalloc here, because port has to survive until >> + * destroy function call >> + */ > Please fix the comment coding style. > >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> + if (!dev) { >> + ret = -ENOMEM; >> + goto alloc_fail; >> + } >> + hid_set_drvdata(hdev, dev); >> + >> + ret = hid_parse(hdev); >> + if (ret) { >> + hid_err(hdev, "failed to parse HID\n"); >> + goto hid_fail; >> + } >> + >> + ret = hid_hw_start(hdev, 0); >> + if (ret) { >> + hid_err(hdev, "failed to start HID HW\n"); >> + goto hid_fail; >> + } >> + >> + ret = hid_hw_open(hdev); >> + if (ret) { >> + hid_err(hdev, "failed to open HID HW\n"); >> + goto err_hid_stop; >> + } >> + >> + ret = ft260_hid_feature_report_get(hdev, FT260_CHIP_VERSION, >> + (u8 *)&version, sizeof(version)); >> + if (ret < 0) { >> + hid_err(hdev, "failed to retrieve chip version\n"); >> + goto err_hid_close; >> + } >> + >> + hid_info(hdev, "chip code: %02x%02x %02x%02x\n", >> + version.chip_code[0], version.chip_code[1], >> + version.chip_code[2], version.chip_code[3]); >> + >> + ret = ft260_get_interface_type(hdev, dev); >> + if (ret <= FT260_IFACE_NONE) >> + goto err_hid_close; >> + >> + hid_set_drvdata(hdev, dev); >> + dev->hdev = hdev; >> + >> + mutex_init(&dev->lock); >> + init_completion(&dev->wait); >> + >> + if (!dev->ft260_is_serial) { >> + ret = ft260_i2c_probe(hdev, dev); >> + if (ret) >> + goto err_hid_close; >> + } else { >> + ret = ft260_uart_probe(hdev, dev); >> + if (ret) >> + goto err_hid_close; >> + } >> + >> + return 0; >> + >> err_hid_close: >> hid_hw_close(hdev); >> err_hid_stop: >> hid_hw_stop(hdev); >> +hid_fail: >> + kfree(dev); >> +alloc_fail: >> return ret; >> } >> >> @@ -1055,8 +1713,18 @@ static void ft260_remove(struct hid_device *hdev) >> if (!dev) >> return; >> >> - sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group); >> - i2c_del_adapter(&dev->adap); >> + if (dev->ft260_is_serial) { >> + // FIXME: >> + cancel_work_sync(&dev->wakeup_work); >> + tty_port_unregister_device(&dev->port, ft260_tty_driver, >> + dev->index); >> + ft260_uart_port_remove(dev); >> + /* dev still needed, so we will free it in _destroy func */ >> + } else { >> + sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group); >> + i2c_del_adapter(&dev->adap); >> + kfree(dev); >> + } >> >> hid_hw_close(hdev); >> hid_hw_stop(hdev); >> @@ -1066,7 +1734,7 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report, >> u8 *data, int size) >> { >> struct ft260_device *dev = hid_get_drvdata(hdev); >> - struct ft260_i2c_input_report *xfer = (void *)data; >> + struct ft260_input_report *xfer = (void *)data; >> >> if (xfer->report >= FT260_I2C_REPORT_MIN && >> xfer->report <= FT260_I2C_REPORT_MAX) { >> @@ -1087,9 +1755,15 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report, >> if (dev->read_idx == dev->read_len) >> complete(&dev->wait); >> >> - } else { >> - hid_err(hdev, "unhandled report %#02x\n", xfer->report); > This change introduced i2c regression. > It caused wrong printout per every i2c hid input report: > > [21912.237393] ft260 0003:0403:6030.000C: unhandled report 0xde > [21912.247405] ft260 0003:0403:6030.000C: unhandled report 0xde > [21912.249403] ft260 0003:0403:6030.000C: unhandled report 0xd1 > [21912.275399] ft260 0003:0403:6030.000C: unhandled report 0xde > [21912.285404] ft260 0003:0403:6030.000C: unhandled report 0xde > [21912.287403] ft260 0003:0403:6030.000C: unhandled report 0xd1 > > I fixed it in the > https://github.com/MichaelZaidman/hid-ft260/commit/9a8e145735f822da1d9beb78b8e866533d409a93 > >> + } else if (xfer->length > FT260_RD_DATA_MAX) { > Did you observe the case when the HID input report exceeded 60 bytes? I did not, but I did not perform edge case testing with a malicious device or so. It's better not to have buffer overflows. > >> + hid_err(hdev, "Received data too long (%d)\n", xfer->length); >> + return -EBADR; >> + } else if (xfer->report >= FT260_UART_REPORT_MIN && >> + xfer->report <= FT260_UART_REPORT_MAX) { >> + return ft260_uart_receive_chars(dev, xfer->data, xfer->length); >> } >> + hid_err(hdev, "unhandled report %#02x\n", xfer->report); >> + >> return 0; >> } >> >> @@ -1101,7 +1775,62 @@ static struct hid_driver ft260_driver = { >> .raw_event = ft260_raw_event, >> }; >> >> -module_hid_driver(ft260_driver); >> -MODULE_DESCRIPTION("FTDI FT260 USB HID to I2C host bridge"); >> +static int __init ft260_driver_init(void) >> +{ >> + int ret; >> + >> + ft260_tty_driver = tty_alloc_driver(UART_COUNT_MAX, >> + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV); >> + if (IS_ERR(ft260_tty_driver)) { >> + pr_err("tty_alloc_driver failed: %d\n", >> + (int)PTR_ERR(ft260_tty_driver)); >> + return PTR_ERR(ft260_tty_driver); >> + } >> + >> + ft260_tty_driver->driver_name = "ft260_ser"; >> + ft260_tty_driver->name = "ttyFT"; >> + ft260_tty_driver->major = 0; >> + ft260_tty_driver->minor_start = 0; >> + ft260_tty_driver->type = TTY_DRIVER_TYPE_SERIAL; >> + ft260_tty_driver->subtype = SERIAL_TYPE_NORMAL; >> + ft260_tty_driver->init_termios = tty_std_termios; >> + ft260_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL; >> + ft260_tty_driver->init_termios.c_ispeed = 9600; >> + ft260_tty_driver->init_termios.c_ospeed = 9600; >> + tty_set_operations(ft260_tty_driver, &ft260_uart_ops); >> + >> + ret = tty_register_driver(ft260_tty_driver); >> + if (ret) { >> + pr_err("tty_register_driver failed: %d\n", ret); >> + goto err_reg_driver; >> + } >> + >> + ret = hid_register_driver(&(ft260_driver)); >> + if (ret) { >> + pr_err("hid_register_driver failed: %d\n", ret); >> + goto err_reg_hid; >> + } >> + >> + return 0; >> + >> +err_reg_hid: >> + tty_unregister_driver(ft260_tty_driver); >> +err_reg_driver: >> + tty_driver_kref_put(ft260_tty_driver); >> + >> + return ret; >> +} >> + >> +static void __exit ft260_driver_exit(void) >> +{ >> + hid_unregister_driver(&(ft260_driver)); >> + tty_unregister_driver(ft260_tty_driver); >> + tty_driver_kref_put(ft260_tty_driver); >> +} >> + >> +module_init(ft260_driver_init); >> +module_exit(ft260_driver_exit); >> + >> +MODULE_DESCRIPTION("FTDI FT260 USB HID to I2C host bridge and TTY driver"); >> MODULE_AUTHOR("Michael Zaidman <michael.zaidman@gmail.com>"); >> MODULE_LICENSE("GPL v2"); >> -- >> 2.42.0 >>
On Thu, Feb 01, 2024 at 02:03:25PM +0100, Christina Quast wrote: > Hi Michael! > > The FIXMEs should have been removed before submitting, sorry for that. They were just a reminder to myself to check the kfifo spinlocks. > > The code looks good to me! What's the next steps? Should I include your uart related changes to the patch and resend it to the mailng list? > Hi Christina, Thanks for pushing the UART support forward. It's much appreciated. Unfortunately, your latest patchset (V4) had compilation issues, broke the ft260 I2C driver, and lost the data on the RX line at higher than 4800 baud rates. To shorten the cycle, it will be better that I submit the code from my repository. Otherwise, I will need to review the new patch set and retest everything again. Also, I want to cover more corner cases, and then I will submit the patchset with your and Daniel's credentials. Jiri, there is a pending patchset related to the GPIO support - https://www.spinics.net/lists/linux-input/msg83937.html The ft260 chip has multifunctional pins, and their configuration affects the UART operation. The GPIO patchset adds this part. I am considering rebasing it above the UART code discussed in this email thread. I wonder whether resubmitting the new patch set that includes UART and multifunctional pins (GPIO) support will be a better approach. What do you think? Thanks, Michael > > > -------- Original Message -------- > From: Michael Zaidman <michael.zaidman@gmail.com> > Sent: Wed Jan 31 16:48:55 GMT+01:00 2024 > To: Daniel Beer <daniel.beer@igorinstitute.com>, Christina Quast <contact@christina-quast.de> > Cc: linux-serial@vger.kernel.org, ilpo.jarvinen@linux.intel.com, johan@kernel.org, gregkh@linuxfoundation.org, David Lamparter <equinox@diac24.net>, Jiri Kosina <jikos@kernel.org>, Michael Zaidman <michael.zaidman@gmail.com> > Subject: Re: [PATCH v4 RESEND] hid-ft260: Add serial driver > > On Wed, Jan 31, 2024 at 07:28:25PM +1300, Daniel Beer wrote: > > On Mon, Jan 29, 2024 at 12:07:48AM +0200, Michael Zaidman wrote: > > > I pushed changes into https://github.com/MichaelZaidman/hid-ft260/tree/uart > > > branch and would appreciate your review and feedback before preparing the > > > cumulative patch set. > > > > > > I addressed the first FIXME. Please correct me if I did not get right what > > > you meant. > > > > > > I am still doubting regarding the second FIXME. I see only three tty drivers > > > are using the kfifo_avail and kfifo_len to retrieve the write room and number > > > of chars in the buffer, and neither use locking. What was your concern? > > > > I don't see anything obviously wrong, but I'm probably not the best > > person to be reviewing this -- Christina has spent the most time on it > > recently, and I think the FIXME comments are hers. > > > Thanks, Daniel, for your feedback. > I see, it's my mistake. I should have reached out to both of you. > > I added multiple fine-grained commits to simplify the review. > Christina, can you clarify the FIXME issue and review the changes? > > Thanks, > Michael >
On Fri, Feb 02, 2024 at 11:04:08AM +0200, Michael Zaidman wrote: > On Thu, Feb 01, 2024 at 02:03:25PM +0100, Christina Quast wrote: > > Hi Michael! > > > > The FIXMEs should have been removed before submitting, sorry for that. They were just a reminder to myself to check the kfifo spinlocks. > > > > The code looks good to me! What's the next steps? Should I include your uart related changes to the patch and resend it to the mailng list? > > > Hi Christina, I did more testing and published the changes here https://lore.kernel.org/all/20240210215147.77629-1-michael.zaidman@gmail.com/ Please review and meld them into the v5 patch. Thanks, Michael
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c index 333341e80b0e..cc2cce3698e3 100644 --- a/drivers/hid/hid-ft260.c +++ b/drivers/hid/hid-ft260.c @@ -13,6 +13,16 @@ #include <linux/i2c.h> #include <linux/module.h> #include <linux/usb.h> +#include <linux/serial.h> +#include <linux/serial_core.h> +#include <linux/kfifo.h> +#include <linux/tty_flip.h> +#include <linux/minmax.h> +#include <asm/unaligned.h> /* Needed for cpu_to_le16, le16_to_cpu */ + +#define UART_COUNT_MAX 4 /* Number of UARTs this driver can handle */ +#define FIFO_SIZE 256 +#define TTY_WAKEUP_WATERMARK (FIFO_SIZE / 2) #ifdef DEBUG static int ft260_debug = 1; @@ -30,6 +40,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages"); #define FT260_REPORT_MAX_LENGTH (64) #define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4) +#define FT260_UART_DATA_REPORT_ID(len) (FT260_UART_REPORT_MIN + (len - 1) / 4) #define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */ @@ -43,7 +54,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages"); * or optoe limit the i2c reads to 128 bytes. To not block other drivers out * of I2C for potentially troublesome amounts of time, we select the maximum * read payload length to be 180 bytes. -*/ + */ #define FT260_RD_DATA_MAX (180) #define FT260_WR_DATA_MAX (60) @@ -81,7 +92,8 @@ enum { FT260_UART_INTERRUPT_STATUS = 0xB1, FT260_UART_STATUS = 0xE0, FT260_UART_RI_DCD_STATUS = 0xE1, - FT260_UART_REPORT = 0xF0, + FT260_UART_REPORT_MIN = 0xF0, + FT260_UART_REPORT_MAX = 0xFE, }; /* Feature Out */ @@ -132,6 +144,13 @@ enum { FT260_FLAG_START_STOP_REPEATED = 0x07, }; +/* Return values for ft260_get_interface_type func */ +enum { + FT260_IFACE_NONE, + FT260_IFACE_I2C, + FT260_IFACE_UART +}; + #define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | report_id) /* Feature In reports */ @@ -220,12 +239,59 @@ struct ft260_i2c_read_request_report { __le16 length; /* data payload length */ } __packed; -struct ft260_i2c_input_report { - u8 report; /* FT260_I2C_REPORT */ +struct ft260_input_report { + u8 report; /* FT260_I2C_REPORT or FT260_UART_REPORT */ u8 length; /* data payload length */ u8 data[2]; /* data payload */ } __packed; +/* UART reports */ +struct ft260_uart_write_request_report { + u8 report; /* FT260_UART_REPORT */ + u8 length; /* data payload length */ + u8 data[] __counted_by(length); /* variable data payload */ +} __packed; + +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 */ + /* The baudrate field is unaligned: */ + __le32 baudrate; /* little endian, 9600 = 0x2580, 19200 = 0x4B00 */ + u8 data_bit; /* 7 or 8 */ + u8 parity; /* 0: no parity, 1: odd, 2: even, 3: high, 4: low */ + u8 stop_bit; /* 0: one stop bit, 2: 2 stop bits */ + u8 breaking; /* 0: no break */ +} __packed; + +/* UART interface configuration */ +enum { + FT260_CFG_FLOW_CTRL_OFF = 0x00, + FT260_CFG_FLOW_CTRL_RTS_CTS = 0x01, + FT260_CFG_FLOW_CTRL_DTR_DSR = 0x02, + FT260_CFG_FLOW_CTRL_XON_XOFF = 0x03, + FT260_CFG_FLOW_CTRL_NONE = 0x04, + + FT260_CFG_DATA_BITS_7 = 0x07, + FT260_CFG_DATA_BITS_8 = 0x08, + + FT260_CFG_PAR_NO = 0x00, + FT260_CFG_PAR_ODD = 0x01, + FT260_CFG_PAR_EVEN = 0x02, + FT260_CFG_PAR_HIGH = 0x03, + FT260_CFG_PAR_LOW = 0x04, + + FT260_CFG_STOP_ONE_BIT = 0x00, + FT260_CFG_STOP_TWO_BIT = 0x02, + + FT260_CFG_BREAKING_NO = 0x00, + FT260_CFG_BEAKING_YES = 0x01, + + FT260_CFG_BAUD_MIN = 1200, + FT260_CFG_BAUD_MAX = 12000000, +}; + static const struct hid_device_id ft260_devices[] = { { HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY, USB_DEVICE_ID_FT260) }, @@ -236,6 +302,23 @@ MODULE_DEVICE_TABLE(hid, ft260_devices); struct ft260_device { struct i2c_adapter adap; struct hid_device *hdev; + + bool ft260_is_serial; + struct list_head device_list; + + /* tty_port lifetime is equal to device lifetime */ + struct tty_port port; + unsigned int index; + struct kfifo xmit_fifo; + /* write_lock: lock to serialize access to xmit fifo */ + spinlock_t write_lock; + struct uart_icount icount; + + struct timer_list wakeup_timer; + struct work_struct wakeup_work; + bool reschedule_work; + + struct completion wait; struct mutex lock; u8 write_buf[FT260_REPORT_MAX_LENGTH]; @@ -377,7 +460,7 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev, ret = ft260_hid_output_report(hdev, data, len); if (ret < 0) { - hid_err(hdev, "%s: failed to start transfer, ret %d\n", + hid_dbg(hdev, "%s: failed to start transfer, ret %d\n", __func__, ret); ft260_i2c_reset(hdev); return ret; @@ -592,7 +675,7 @@ static int ft260_i2c_write_read(struct ft260_device *dev, struct i2c_msg *msgs) else read_off = *msgs[0].buf; - pr_info("%s: off %#x rlen %d wlen %d\n", __func__, + ft260_dbg("%s: off %#x rlen %d wlen %d\n", __func__, read_off, rd_len, wr_len); } @@ -782,7 +865,7 @@ static int ft260_get_system_config(struct hid_device *hdev, return 0; } -static int ft260_is_interface_enabled(struct hid_device *hdev) +static int ft260_get_interface_type(struct hid_device *hdev, struct ft260_device *dev) { struct ft260_get_system_status_report cfg; struct usb_interface *usbif = to_usb_interface(hdev->dev.parent); @@ -799,21 +882,27 @@ static int ft260_is_interface_enabled(struct hid_device *hdev) ft260_dbg("i2c_enable: 0x%02x\n", cfg.i2c_enable); ft260_dbg("uart_mode: 0x%02x\n", cfg.uart_mode); + dev->ft260_is_serial = false; + switch (cfg.chip_mode) { case FT260_MODE_ALL: case FT260_MODE_BOTH: - if (interface == 1) - hid_info(hdev, "uart interface is not supported\n"); - else - ret = 1; + if (interface == 1) { + ret = FT260_IFACE_UART; + dev->ft260_is_serial = true; + } else { + ret = FT260_IFACE_I2C; + } break; case FT260_MODE_UART: - hid_info(hdev, "uart interface is not supported\n"); + ret = FT260_IFACE_UART; + dev->ft260_is_serial = true; break; case FT260_MODE_I2C: - ret = 1; + ret = FT260_IFACE_I2C; break; } + return ret; } @@ -957,51 +1046,486 @@ static const struct attribute_group ft260_attr_group = { } }; -static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) +/*** + * START Serial dev part + */ +static DEFINE_MUTEX(ft260_uart_list_lock); +static LIST_HEAD(ft260_uart_device_list); + +static struct ft260_device *ft260_dev_by_index(int index) { + struct ft260_device *port; + + list_for_each_entry(port, &ft260_uart_device_list, device_list) { + if (index == port->index) + return port; + } + return NULL; +} + +static int ft260_uart_add_port(struct ft260_device *port) +{ + int index = 0, ret = 0; struct ft260_device *dev; - struct ft260_get_chip_version_report version; - int ret; - if (!hid_is_usb(hdev)) - return -EINVAL; - - dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); - if (!dev) + spin_lock_init(&port->write_lock); + if (kfifo_alloc(&port->xmit_fifo, FIFO_SIZE, GFP_KERNEL)) return -ENOMEM; - ret = hid_parse(hdev); - if (ret) { - hid_err(hdev, "failed to parse HID\n"); - return ret; + mutex_lock(&ft260_uart_list_lock); + list_for_each_entry(dev, &ft260_uart_device_list, device_list) { + if (dev->index != index) + break; + index++; } - ret = hid_hw_start(hdev, 0); - if (ret) { - hid_err(hdev, "failed to start HID HW\n"); - return ret; + port->index = index; + list_add(&port->device_list, &ft260_uart_device_list); + mutex_unlock(&ft260_uart_list_lock); + + return ret; +} + +static void ft260_uart_port_put(struct ft260_device *port) +{ + tty_port_put(&port->port); +} + +static void ft260_uart_port_remove(struct ft260_device *port) +{ + timer_delete_sync(&port->wakeup_timer); + + mutex_lock(&ft260_uart_list_lock); + list_del(&port->device_list); + mutex_unlock(&ft260_uart_list_lock); + + spin_lock(&port->write_lock); + kfifo_free(&port->xmit_fifo); + spin_unlock(&port->write_lock); + + mutex_lock(&port->port.mutex); + port->reschedule_work = false; + tty_port_tty_hangup(&port->port, false); + mutex_unlock(&port->port.mutex); + + ft260_uart_port_put(port); +} + +static struct ft260_device *ft260_uart_port_get(unsigned int index) +{ + struct ft260_device *port; + + if (index >= UART_COUNT_MAX) + return NULL; + + mutex_lock(&ft260_uart_list_lock); + port = ft260_dev_by_index(index); + if (port) + tty_port_get(&port->port); + mutex_unlock(&ft260_uart_list_lock); + + return port; +} + +static int ft260_uart_open(struct tty_struct *tty, struct file *filp) +{ + int ret; + struct ft260_device *port = tty->driver_data; + + ret = tty_port_open(&port->port, tty, filp); + + return ret; +} + +static void ft260_uart_close(struct tty_struct *tty, struct file *filp) +{ + struct ft260_device *port = tty->driver_data; + + tty_port_close(&port->port, tty, filp); +} + +static void ft260_uart_hangup(struct tty_struct *tty) +{ + struct ft260_device *port = tty->driver_data; + + tty_port_hangup(&port->port); +} + +static int ft260_uart_transmit_chars(struct ft260_device *port) +{ + struct hid_device *hdev = port->hdev; + struct kfifo *xmit = &port->xmit_fifo; + struct tty_struct *tty; + struct ft260_uart_write_request_report *rep; + int len, data_len, ret = 0; + + tty = tty_port_tty_get(&port->port); + + data_len = kfifo_len(xmit); + if (!tty || !data_len) { + ret = -EINVAL; + goto tty_out; } - ret = hid_hw_open(hdev); - if (ret) { - hid_err(hdev, "failed to open HID HW\n"); - goto err_hid_stop; + rep = (struct ft260_uart_write_request_report *)port->write_buf; + + do { + len = min(data_len, FT260_WR_DATA_MAX); + + rep->report = FT260_UART_DATA_REPORT_ID(len); + rep->length = len; + + len = kfifo_out_locked(xmit, rep->data, len, &port->write_lock); + + ret = ft260_hid_output_report(hdev, (u8 *)rep, len + sizeof(*rep)); + if (ret < 0) { + hid_err(hdev, "Failed to start transfer, ret %d\n", ret); + goto tty_out; + } + + data_len -= len; + port->icount.tx += len; + } while (data_len > 0); + + len = kfifo_len(xmit); + if ((FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK) + tty_wakeup(tty); + + ret = 0; + +tty_out: + tty_kref_put(tty); + return ret; +} + +static int ft260_uart_receive_chars(struct ft260_device *port, + u8 *data, u8 length) +{ + struct hid_device *hdev = port->hdev; + int ret = 0; + + if (length > FT260_RD_DATA_MAX) { + hid_err(hdev, "Received too much data (%d)\n", length); + return -EBADR; } - ret = ft260_hid_feature_report_get(hdev, FT260_CHIP_VERSION, - (u8 *)&version, sizeof(version)); + ret = tty_insert_flip_string(&port->port, data, length); + if (ret != length) + hid_err(hdev, "%d char not inserted to flip buffer\n", length - ret); + port->icount.rx += ret; + + if (ret) + tty_flip_buffer_push(&port->port); + + return ret; +} + +static ssize_t ft260_uart_write(struct tty_struct *tty, const unsigned char *buf, + size_t count) +{ + struct ft260_device *port = tty->driver_data; + struct hid_device *hdev = port->hdev; + int len, ret; + + len = kfifo_in_locked(&port->xmit_fifo, buf, count, &port->write_lock); + ft260_dbg("count: %ld, len: %d", count, len); + + ret = ft260_uart_transmit_chars(port); if (ret < 0) { - hid_err(hdev, "failed to retrieve chip version\n"); - goto err_hid_close; + hid_dbg(hdev, "Failed to transmit chars: %d\n", ret); + return 0; } - hid_info(hdev, "chip code: %02x%02x %02x%02x\n", - version.chip_code[0], version.chip_code[1], - version.chip_code[2], version.chip_code[3]); + ret = kfifo_len(&port->xmit_fifo); + if (ret > 0) { + hid_dbg(hdev, "Failed to all kfifo data bytes\n"); + ft260_dbg("return: %d", len - ret); + return len - ret; + } + + return len; +} + +static unsigned int ft260_uart_write_room(struct tty_struct *tty) +{ + struct ft260_device *port = tty->driver_data; + + return FIFO_SIZE - kfifo_len(&port->xmit_fifo); +} + +static unsigned int ft260_uart_chars_in_buffer(struct tty_struct *tty) +{ + struct ft260_device *port = tty->driver_data; + + return kfifo_len(&port->xmit_fifo); +} + +static int ft260_uart_change_speed(struct ft260_device *port, + struct ktermios *termios, + struct ktermios *old) +{ + struct hid_device *hdev = port->hdev; + unsigned int baud; + struct ft260_configure_uart_request req; + int ret; + + memset(&req, 0, sizeof(req)); + + req.report = FT260_SYSTEM_SETTINGS; + req.request = FT260_SET_UART_CONFIG; + + switch (termios->c_cflag & CSIZE) { + case CS7: + req.data_bit = FT260_CFG_DATA_BITS_7; + break; + case CS5: + case CS6: + hid_err(hdev, "Invalid data bit size, setting to default (8 bit)\n"); + req.data_bit = FT260_CFG_DATA_BITS_8; + termios->c_cflag &= ~CSIZE; + termios->c_cflag |= CS8; + break; + default: + case CS8: + req.data_bit = FT260_CFG_DATA_BITS_8; + break; + } + + req.stop_bit = (termios->c_cflag & CSTOPB) ? + FT260_CFG_STOP_TWO_BIT : FT260_CFG_STOP_ONE_BIT; + + if (termios->c_cflag & PARENB) { + req.parity = (termios->c_cflag & PARODD) ? + FT260_CFG_PAR_ODD : FT260_CFG_PAR_EVEN; + } else { + req.parity = FT260_CFG_PAR_NO; + } + + baud = tty_termios_baud_rate(termios); + if (baud == 0 || baud < FT260_CFG_BAUD_MIN || baud > FT260_CFG_BAUD_MAX) { + struct tty_struct *tty = tty_port_tty_get(&port->port); + + hid_err(hdev, "Invalid baud rate %d\n", baud); + baud = 9600; + tty_encode_baud_rate(tty, baud, baud); + tty_kref_put(tty); + } + put_unaligned_le32(cpu_to_le32(baud), &req.baudrate); + + if (termios->c_cflag & CRTSCTS) + req.flow_ctrl = FT260_CFG_FLOW_CTRL_RTS_CTS; + else + req.flow_ctrl = FT260_CFG_FLOW_CTRL_OFF; + + ft260_dbg("Configured termios: flow control: %d, baudrate: %d, ", + req.flow_ctrl, baud); + ft260_dbg("data_bit: %d, parity: %d, stop_bit: %d, breaking: %d\n", + req.data_bit, req.parity, + req.stop_bit, req.breaking); + + req.flow_ctrl = FT260_CFG_FLOW_CTRL_NONE; + req.breaking = FT260_CFG_BREAKING_NO; + + ret = ft260_hid_feature_report_set(hdev, (u8 *)&req, sizeof(req)); + if (ret < 0) + hid_err(hdev, "ft260_hid_feature_report_set failed: %d\n", ret); + + return ret; +} + +static int ft260_uart_get_icount(struct tty_struct *tty, + struct serial_icounter_struct *icount) +{ + struct ft260_device *port = tty->driver_data; + + memcpy(icount, &port->icount, sizeof(struct uart_icount)); + + return 0; +} + +static void ft260_uart_set_termios(struct tty_struct *tty, + const struct ktermios *old_termios) +{ + struct ft260_device *port = tty->driver_data; + + ft260_uart_change_speed(port, &tty->termios, NULL); +} + +static int ft260_uart_install(struct tty_driver *driver, struct tty_struct *tty) +{ + int idx = tty->index; + struct ft260_device *port = ft260_uart_port_get(idx); + int ret = tty_standard_install(driver, tty); + + if (ret == 0) + /* This is the ref ft260_uart_port get provided */ + tty->driver_data = port; + else + ft260_uart_port_put(port); + + return ret; +} + +static void ft260_uart_cleanup(struct tty_struct *tty) +{ + struct ft260_device *port = tty->driver_data; + + tty->driver_data = NULL; /* Bug trap */ + ft260_uart_port_put(port); +} + +static int ft260_uart_proc_show(struct seq_file *m, void *v) +{ + int i; + + seq_printf(m, "ft260 info:1.0 driver%s%s revision:%s\n", + "", "", ""); + for (i = 0; i < UART_COUNT_MAX; i++) { + struct ft260_device *port = ft260_uart_port_get(i); + + if (port) { + seq_printf(m, "%d: uart:FT260", i); + if (capable(CAP_SYS_ADMIN)) { + seq_printf(m, " tx:%d rx:%d", + port->icount.tx, port->icount.rx); + if (port->icount.frame) + seq_printf(m, " fe:%d", + port->icount.frame); + if (port->icount.parity) + seq_printf(m, " pe:%d", + port->icount.parity); + if (port->icount.brk) + seq_printf(m, " brk:%d", + port->icount.brk); + if (port->icount.overrun) + seq_printf(m, " oe:%d", + port->icount.overrun); + if (port->icount.cts) + seq_printf(m, " cts:%d", + port->icount.cts); + if (port->icount.dsr) + seq_printf(m, " dsr:%d", + port->icount.dsr); + if (port->icount.rng) + seq_printf(m, " rng:%d", + port->icount.rng); + if (port->icount.dcd) + seq_printf(m, " dcd:%d", + port->icount.dcd); + } + ft260_uart_port_put(port); + seq_putc(m, '\n'); + } + } + return 0; +} + +static const struct tty_operations ft260_uart_ops = { + .open = ft260_uart_open, + .close = ft260_uart_close, + .write = ft260_uart_write, + .write_room = ft260_uart_write_room, + .chars_in_buffer = ft260_uart_chars_in_buffer, + .set_termios = ft260_uart_set_termios, + .hangup = ft260_uart_hangup, + .install = ft260_uart_install, + .cleanup = ft260_uart_cleanup, + .proc_show = ft260_uart_proc_show, + .get_icount = ft260_uart_get_icount, +}; + +/* The FT260 has a "power saving mode" that causes the device to switch + * to a 30 kHz oscillator if there's no activity for 5 seconds. + * Unfortunately this mode can only be disabled by reprogramming + * internal fuses, which requires an additional programming voltage. + * + * One effect of this mode is to cause data loss on a fast UART that + * transmits after being idle for longer than 5 seconds. We work around + * this by sending a dummy report at least once per 4 seconds if the + * UART is in use. + */ +static void ft260_uart_start_wakeup(struct timer_list *t) +{ + struct ft260_device *dev = + container_of(t, struct ft260_device, wakeup_timer); + + if (dev->reschedule_work) { + schedule_work(&dev->wakeup_work); + mod_timer(&dev->wakeup_timer, jiffies + + msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS)); + } +} + +static void ft260_uart_do_wakeup(struct work_struct *work) +{ + struct ft260_device *dev = + container_of(work, struct ft260_device, wakeup_work); + struct ft260_get_chip_version_report version; + int ret; + + if (dev->reschedule_work) { + ret = ft260_hid_feature_report_get(dev->hdev, FT260_CHIP_VERSION, + (u8 *)&version, sizeof(version)); + if (ret < 0) + hid_err(dev->hdev, + "%s: failed to start transfer, ret %d\n", + __func__, ret); + } +} + +static void ft260_uart_shutdown(struct tty_port *tport) +{ + struct ft260_device *port = + container_of(tport, struct ft260_device, port); + + port->reschedule_work = false; +} + +static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty) +{ + struct ft260_device *port = + container_of(tport, struct ft260_device, port); + + /* + * Set the TTY IO error marker - we will only clear this + * once we have successfully opened the port. + */ + set_bit(TTY_IO_ERROR, &tty->flags); + + spin_lock(&port->write_lock); + kfifo_reset(&port->xmit_fifo); + spin_unlock(&port->write_lock); + + ft260_uart_change_speed(port, &tty->termios, NULL); + clear_bit(TTY_IO_ERROR, &tty->flags); + + if (port->reschedule_work) { + mod_timer(&port->wakeup_timer, jiffies + + msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS)); + } + + return 0; +} + +static void ft260_uart_port_destroy(struct tty_port *tport) +{ + struct ft260_device *port = + container_of(tport, struct ft260_device, port); + + kfree(port); +} + +static const struct tty_port_operations ft260_uart_port_ops = { + .shutdown = ft260_uart_shutdown, + .activate = ft260_uart_activate, + .destruct = ft260_uart_port_destroy, +}; + +static struct tty_driver *ft260_tty_driver; - ret = ft260_is_interface_enabled(hdev); - if (ret <= 0) - goto err_hid_close; +static int ft260_i2c_probe(struct hid_device *hdev, struct ft260_device *dev) +{ + int ret; hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8, hdev->version & 0xff, hdev->name, @@ -1028,7 +1552,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) ret = i2c_add_adapter(&dev->adap); if (ret) { hid_err(hdev, "failed to add i2c adapter\n"); - goto err_hid_close; + return ret; } ret = sysfs_create_group(&hdev->dev.kobj, &ft260_attr_group); @@ -1036,15 +1560,149 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) hid_err(hdev, "failed to create sysfs attrs\n"); goto err_i2c_free; } - return 0; err_i2c_free: i2c_del_adapter(&dev->adap); + return ret; +} + +static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev) +{ + struct ft260_configure_uart_request req; + int ret; + struct device *devt; + + INIT_WORK(&dev->wakeup_work, ft260_uart_do_wakeup); + // FIXME: Do I need that if I have cancel_work_sync? + // FIXME: are all kfifo access secured by lock? with irq or not? + dev->reschedule_work = false; + /* Work not started at this point */ + timer_setup(&dev->wakeup_timer, ft260_uart_start_wakeup, 0); + + tty_port_init(&dev->port); + dev->port.ops = &ft260_uart_port_ops; + + ret = ft260_uart_add_port(dev); + if (ret) { + hid_err(hdev, "failed to add port\n"); + return ret; + } + devt = tty_port_register_device_attr(&dev->port, + ft260_tty_driver, + dev->index, &hdev->dev, + dev, NULL); + if (IS_ERR(devt)) { + hid_err(hdev, "failed to register tty port\n"); + ret = PTR_ERR(devt); + goto err_register_tty; + } + hid_info(hdev, "Registering device /dev/%s%d\n", + ft260_tty_driver->name, dev->index); + + /* Send Feature Report to Configure FT260 as UART 9600-8-N-1 */ + req.report = FT260_SYSTEM_SETTINGS; + req.request = FT260_SET_UART_CONFIG; + req.flow_ctrl = FT260_CFG_FLOW_CTRL_NONE; + put_unaligned_le32(cpu_to_le32(9600), &req.baudrate); + req.data_bit = FT260_CFG_DATA_BITS_8; + req.parity = FT260_CFG_PAR_NO; + req.stop_bit = FT260_CFG_STOP_ONE_BIT; + req.breaking = FT260_CFG_BREAKING_NO; + + ret = ft260_hid_feature_report_set(hdev, (u8 *)&req, sizeof(req)); + if (ret < 0) { + hid_err(hdev, "ft260_hid_feature_report_set failed: %d\n", + ret); + goto err_hid_report; + } + + return 0; + +err_hid_report: + tty_port_unregister_device(&dev->port, ft260_tty_driver, dev->index); +err_register_tty: + ft260_uart_port_remove(dev); + return ret; +} + +static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id) +{ + struct ft260_device *dev; + struct ft260_get_chip_version_report version; + int ret; + + if (!hid_is_usb(hdev)) + return -EINVAL; + + /* We cannot used devm_kzalloc here, because port has to survive until + * destroy function call + */ + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) { + ret = -ENOMEM; + goto alloc_fail; + } + hid_set_drvdata(hdev, dev); + + ret = hid_parse(hdev); + if (ret) { + hid_err(hdev, "failed to parse HID\n"); + goto hid_fail; + } + + ret = hid_hw_start(hdev, 0); + if (ret) { + hid_err(hdev, "failed to start HID HW\n"); + goto hid_fail; + } + + ret = hid_hw_open(hdev); + if (ret) { + hid_err(hdev, "failed to open HID HW\n"); + goto err_hid_stop; + } + + ret = ft260_hid_feature_report_get(hdev, FT260_CHIP_VERSION, + (u8 *)&version, sizeof(version)); + if (ret < 0) { + hid_err(hdev, "failed to retrieve chip version\n"); + goto err_hid_close; + } + + hid_info(hdev, "chip code: %02x%02x %02x%02x\n", + version.chip_code[0], version.chip_code[1], + version.chip_code[2], version.chip_code[3]); + + ret = ft260_get_interface_type(hdev, dev); + if (ret <= FT260_IFACE_NONE) + goto err_hid_close; + + hid_set_drvdata(hdev, dev); + dev->hdev = hdev; + + mutex_init(&dev->lock); + init_completion(&dev->wait); + + if (!dev->ft260_is_serial) { + ret = ft260_i2c_probe(hdev, dev); + if (ret) + goto err_hid_close; + } else { + ret = ft260_uart_probe(hdev, dev); + if (ret) + goto err_hid_close; + } + + return 0; + err_hid_close: hid_hw_close(hdev); err_hid_stop: hid_hw_stop(hdev); +hid_fail: + kfree(dev); +alloc_fail: return ret; } @@ -1055,8 +1713,18 @@ static void ft260_remove(struct hid_device *hdev) if (!dev) return; - sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group); - i2c_del_adapter(&dev->adap); + if (dev->ft260_is_serial) { + // FIXME: + cancel_work_sync(&dev->wakeup_work); + tty_port_unregister_device(&dev->port, ft260_tty_driver, + dev->index); + ft260_uart_port_remove(dev); + /* dev still needed, so we will free it in _destroy func */ + } else { + sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group); + i2c_del_adapter(&dev->adap); + kfree(dev); + } hid_hw_close(hdev); hid_hw_stop(hdev); @@ -1066,7 +1734,7 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data, int size) { struct ft260_device *dev = hid_get_drvdata(hdev); - struct ft260_i2c_input_report *xfer = (void *)data; + struct ft260_input_report *xfer = (void *)data; if (xfer->report >= FT260_I2C_REPORT_MIN && xfer->report <= FT260_I2C_REPORT_MAX) { @@ -1087,9 +1755,15 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report, if (dev->read_idx == dev->read_len) complete(&dev->wait); - } else { - hid_err(hdev, "unhandled report %#02x\n", xfer->report); + } else if (xfer->length > FT260_RD_DATA_MAX) { + hid_err(hdev, "Received data too long (%d)\n", xfer->length); + return -EBADR; + } else if (xfer->report >= FT260_UART_REPORT_MIN && + xfer->report <= FT260_UART_REPORT_MAX) { + return ft260_uart_receive_chars(dev, xfer->data, xfer->length); } + hid_err(hdev, "unhandled report %#02x\n", xfer->report); + return 0; } @@ -1101,7 +1775,62 @@ static struct hid_driver ft260_driver = { .raw_event = ft260_raw_event, }; -module_hid_driver(ft260_driver); -MODULE_DESCRIPTION("FTDI FT260 USB HID to I2C host bridge"); +static int __init ft260_driver_init(void) +{ + int ret; + + ft260_tty_driver = tty_alloc_driver(UART_COUNT_MAX, + TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV); + if (IS_ERR(ft260_tty_driver)) { + pr_err("tty_alloc_driver failed: %d\n", + (int)PTR_ERR(ft260_tty_driver)); + return PTR_ERR(ft260_tty_driver); + } + + ft260_tty_driver->driver_name = "ft260_ser"; + ft260_tty_driver->name = "ttyFT"; + ft260_tty_driver->major = 0; + ft260_tty_driver->minor_start = 0; + ft260_tty_driver->type = TTY_DRIVER_TYPE_SERIAL; + ft260_tty_driver->subtype = SERIAL_TYPE_NORMAL; + ft260_tty_driver->init_termios = tty_std_termios; + ft260_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL; + ft260_tty_driver->init_termios.c_ispeed = 9600; + ft260_tty_driver->init_termios.c_ospeed = 9600; + tty_set_operations(ft260_tty_driver, &ft260_uart_ops); + + ret = tty_register_driver(ft260_tty_driver); + if (ret) { + pr_err("tty_register_driver failed: %d\n", ret); + goto err_reg_driver; + } + + ret = hid_register_driver(&(ft260_driver)); + if (ret) { + pr_err("hid_register_driver failed: %d\n", ret); + goto err_reg_hid; + } + + return 0; + +err_reg_hid: + tty_unregister_driver(ft260_tty_driver); +err_reg_driver: + tty_driver_kref_put(ft260_tty_driver); + + return ret; +} + +static void __exit ft260_driver_exit(void) +{ + hid_unregister_driver(&(ft260_driver)); + tty_unregister_driver(ft260_tty_driver); + tty_driver_kref_put(ft260_tty_driver); +} + +module_init(ft260_driver_init); +module_exit(ft260_driver_exit); + +MODULE_DESCRIPTION("FTDI FT260 USB HID to I2C host bridge and TTY driver"); MODULE_AUTHOR("Michael Zaidman <michael.zaidman@gmail.com>"); MODULE_LICENSE("GPL v2");