Modifications on top of "[PATCH v4 RESEND] hid-ft260: Add serial driver"
https://lore.kernel.org/all/[email protected]/
They are mostly the fixes to the original v4 patch and should be melded into
the v5 patch rather than upstreamed as separate patches.
Michael Zaidman (19):
hid-ft260: fix incompatible-pointer-types error
hid-ft260: fix Wformat warning
hid-ft260: fix i2c driver regression in ft260_raw_event
hid-ft260: remove dead code in ft260_uart_receive_chars
hid-ft260: fix unprotected write_buf concurrent access
hid-ft260: uart: enable wakeup workaround
hid-ft260: depend wakeup workaround activation on uart baud rate
hid-ft260: depend wakeup workaround activation on eeprom config
hid-ft260: uart: wakeup device early to not lose rx data
hid-ft260: uart: do not configure baud rate twice
hid-ft260: uart: do not disable wakeup workaround twice
hid-ft260: uart: use kfifo_avail for fifo write room check
hid-ft260: improve usb interface type detection logic
hid-ft260: uart: cleanup and refactoring
hid-ft260: uart: remove FIXME for wake-up workaround
hid-ft260: uart: suppress unhandled report 0xb1 dmesg
hid-ft260: uart: arm wake-up timer unconditionally on tty session start
hid-ft260: uart: fix rx data loss after device reopening
hid-ft260: uart: improve write performance
drivers/hid/hid-ft260.c | 330 ++++++++++++++++++++++------------------
1 file changed, 185 insertions(+), 145 deletions(-)
--
2.40.1
Fixed compilation error introduced by "hid-ft260: Add serial driver" patch
https://lore.kernel.org/all/[email protected]/
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
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index cc2cce3698e3..7273d401337a 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1220,8 +1220,8 @@ static int ft260_uart_receive_chars(struct ft260_device *port,
return ret;
}
-static ssize_t ft260_uart_write(struct tty_struct *tty, const unsigned char *buf,
- size_t count)
+static int ft260_uart_write(struct tty_struct *tty, const unsigned char *buf,
+ int count)
{
struct ft260_device *port = tty->driver_data;
struct hid_device *hdev = port->hdev;
--
2.40.1
Fixed compilation warning introduced by "hid-ft260: Add serial driver" patch
https://lore.kernel.org/all/[email protected]/
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);
| ^~~~~~~~~
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 7273d401337a..a67c625c9165 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1228,7 +1228,7 @@ static int ft260_uart_write(struct tty_struct *tty, const unsigned char *buf,
int len, ret;
len = kfifo_in_locked(&port->xmit_fifo, buf, count, &port->write_lock);
- ft260_dbg("count: %ld, len: %d", count, len);
+ ft260_dbg("count: %d, len: %d", count, len);
ret = ft260_uart_transmit_chars(port);
if (ret < 0) {
--
2.40.1
Fix i2c regression in ft260_raw_event introduced by
"[PATCH v4 RESEND] hid-ft260: Add serial driver" patch:
https://lore.kernel.org/all/[email protected]/
It caused wrong printout per every 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
[21912.315406] ft260 0003:0403:6030.000C: unhandled report 0xde
[21912.326390] ft260 0003:0403:6030.000C: unhandled report 0xde
[21912.327402] ft260 0003:0403:6030.000C: unhandled report 0xd1
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index a67c625c9165..a348f11600c6 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1755,6 +1755,8 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
if (dev->read_idx == dev->read_len)
complete(&dev->wait);
+ return 0;
+
} else if (xfer->length > FT260_RD_DATA_MAX) {
hid_err(hdev, "Received data too long (%d)\n", xfer->length);
return -EBADR;
--
2.40.1
Remove conditional expression, which will never be true here since
it is already filtered in the ft260_raw_event procedure.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index a348f11600c6..77638cae595e 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1202,16 +1202,11 @@ 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;
- }
+ int ret;
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);
+ hid_dbg(hdev, "%d char not inserted to flip buffer\n", length - ret);
port->icount.rx += ret;
if (ret)
--
2.40.1
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 and briefly tested the data integrity simultaneously writing
via I2C and UART channels.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 77638cae595e..3d6beac0b8b6 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -38,9 +38,12 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
pr_info("%s: " format, __func__, ##arg); \
} while (0)
-#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_REPORT_MAX_LEN (64)
+#define FT260_DATA_REPORT_ID(min, len) (min + (len - 1) / 4)
+#define FT260_I2C_DATA_REPORT_ID(len) \
+ FT260_DATA_REPORT_ID(FT260_I2C_REPORT_MIN, len)
+#define FT260_UART_DATA_REPORT_ID(len) \
+ FT260_DATA_REPORT_ID(FT260_UART_REPORT_MIN, len)
#define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */
@@ -56,7 +59,8 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
* read payload length to be 180 bytes.
*/
#define FT260_RD_DATA_MAX (180)
-#define FT260_WR_DATA_MAX (60)
+#define FT260_WR_I2C_DATA_MAX (60)
+#define FT260_WR_UART_DATA_MAX (62)
/*
* Device interface configuration.
@@ -229,7 +233,7 @@ struct ft260_i2c_write_request_report {
u8 address; /* 7-bit I2C address */
u8 flag; /* I2C transaction condition */
u8 length; /* data payload length */
- u8 data[FT260_WR_DATA_MAX]; /* data payload */
+ u8 data[FT260_WR_I2C_DATA_MAX]; /* data payload */
} __packed;
struct ft260_i2c_read_request_report {
@@ -249,7 +253,7 @@ struct ft260_input_report {
struct ft260_uart_write_request_report {
u8 report; /* FT260_UART_REPORT */
u8 length; /* data payload length */
- u8 data[] __counted_by(length); /* variable data payload */
+ u8 data[FT260_WR_UART_DATA_MAX]; /* data payload */
} __packed;
struct ft260_configure_uart_request {
@@ -318,10 +322,10 @@ struct ft260_device {
struct work_struct wakeup_work;
bool reschedule_work;
-
struct completion wait;
struct mutex lock;
- u8 write_buf[FT260_REPORT_MAX_LENGTH];
+ u8 i2c_wr_buf[FT260_REPORT_MAX_LEN];
+ u8 uart_wr_buf[FT260_REPORT_MAX_LEN];
unsigned long need_wakeup_at;
u8 *read_buf;
u16 read_idx;
@@ -503,7 +507,7 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
int ret, wr_len, idx = 0;
struct hid_device *hdev = dev->hdev;
struct ft260_i2c_write_request_report *rep =
- (struct ft260_i2c_write_request_report *)dev->write_buf;
+ (struct ft260_i2c_write_request_report *)dev->i2c_wr_buf;
if (len < 1)
return -EINVAL;
@@ -511,12 +515,12 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
rep->flag = FT260_FLAG_START;
do {
- if (len <= FT260_WR_DATA_MAX) {
+ if (len <= FT260_WR_I2C_DATA_MAX) {
wr_len = len;
if (flag == FT260_FLAG_START_STOP)
rep->flag |= FT260_FLAG_STOP;
} else {
- wr_len = FT260_WR_DATA_MAX;
+ wr_len = FT260_WR_I2C_DATA_MAX;
}
rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
@@ -552,7 +556,7 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
int len = 4;
struct ft260_i2c_write_request_report *rep =
- (struct ft260_i2c_write_request_report *)dev->write_buf;
+ (struct ft260_i2c_write_request_report *)dev->i2c_wr_buf;
if (data_len >= sizeof(rep->data))
return -EINVAL;
@@ -1167,10 +1171,10 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
goto tty_out;
}
- rep = (struct ft260_uart_write_request_report *)port->write_buf;
+ rep = (struct ft260_uart_write_request_report *)port->uart_wr_buf;
do {
- len = min(data_len, FT260_WR_DATA_MAX);
+ len = min(data_len, FT260_WR_UART_DATA_MAX);
rep->report = FT260_UART_DATA_REPORT_ID(len);
rep->length = len;
--
2.40.1
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, or external eeprom if exists.
One effect of this mode is to cause data loss on an Rx line at baud
rates higher than 4800 after being idle for longer than 5 seconds.
We work around this by sending a dummy report at least once per 4.8
seconds if the UART is in use. But it is not acctivated in the
"[PATCH v4 RESEND] hid-ft260: Add serial driver"
https://lore.kernel.org/all/[email protected]/
It causes data loss on the Rx line at the driver's default 9600 baud rate.
Enable periodic dummy report to prevent data loss on Rx line upon exiting
from power saving mode.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 3d6beac0b8b6..19599e64c6be 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1575,7 +1575,7 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
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;
+ dev->reschedule_work = true;
/* Work not started at this point */
timer_setup(&dev->wakeup_timer, ft260_uart_start_wakeup, 0);
--
2.40.1
The ft260 chip enters power save mode after 5 seconds of being in idle
state. It causes the data loss on the Rx line. The implemented workaround
sends a dummy report every 4.8 seconds, preventing the chip from entering
the power save mode. But the baud rates below 4800 work fine without a
wakeup workaround.
This commit makes the wakeup workaround activation dependent on the UART
rate, allowing entering the power saving mode for the UART rates below
4800 bauds.
[11238.542841] ft260_uart_wakeup_workaraund_enable: Activate wakeup workaround
[11238.542843] ft260_uart_change_speed: Configured termios: flow control: 0, baudrate: 9600,
[11238.542845] ft260_uart_change_speed: data_bit: 8, parity: 0, stop_bit: 0, breaking: 0
[11238.543671] ft260_uart_wakeup_workaraund_enable: Deactivate wakeup workaround
[11238.543683] ft260_uart_change_speed: Configured termios: flow control: 0, baudrate: 4800,
[11238.543684] ft260_uart_change_speed: data_bit: 8, parity: 0, stop_bit: 0, breaking: 0
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 48 +++++++++++++++++++++++++++++++----------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 19599e64c6be..b24998092d22 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -296,6 +296,8 @@ enum {
FT260_CFG_BAUD_MAX = 12000000,
};
+#define FT260_UART_EN_PW_SAVE_BAUD 4800
+
static const struct hid_device_id ft260_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY,
USB_DEVICE_ID_FT260) },
@@ -1056,6 +1058,15 @@ static const struct attribute_group ft260_attr_group = {
static DEFINE_MUTEX(ft260_uart_list_lock);
static LIST_HEAD(ft260_uart_device_list);
+static void ft260_uart_wakeup(struct ft260_device *dev);
+
+static void ft260_uart_wakeup_workaraund_enable(struct ft260_device *port,
+ bool enable)
+{
+ port->reschedule_work = enable;
+ ft260_dbg("%s wakeup workaround", enable ? "Activate" : "Deactivate");
+}
+
static struct ft260_device *ft260_dev_by_index(int index)
{
struct ft260_device *port;
@@ -1108,7 +1119,7 @@ static void ft260_uart_port_remove(struct ft260_device *port)
spin_unlock(&port->write_lock);
mutex_lock(&port->port.mutex);
- port->reschedule_work = false;
+ ft260_uart_wakeup_workaraund_enable(port, false);
tty_port_tty_hangup(&port->port, false);
mutex_unlock(&port->port.mutex);
@@ -1266,8 +1277,11 @@ static int ft260_uart_change_speed(struct ft260_device *port,
struct hid_device *hdev = port->hdev;
unsigned int baud;
struct ft260_configure_uart_request req;
+ bool wakeup_workaraund = false;
int ret;
+ ft260_uart_wakeup(port);
+
memset(&req, 0, sizeof(req));
req.report = FT260_SYSTEM_SETTINGS;
@@ -1309,6 +1323,12 @@ static int ft260_uart_change_speed(struct ft260_device *port,
tty_encode_baud_rate(tty, baud, baud);
tty_kref_put(tty);
}
+
+ if (baud > FT260_UART_EN_PW_SAVE_BAUD)
+ wakeup_workaraund = true;
+
+ ft260_uart_wakeup_workaraund_enable(port, wakeup_workaraund);
+
put_unaligned_le32(cpu_to_le32(baud), &req.baudrate);
if (termios->c_cflag & CRTSCTS)
@@ -1435,13 +1455,13 @@ static const struct tty_operations ft260_uart_ops = {
/* 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
+ * 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.
+ * One effect of this mode is to cause data loss on an Rx line at baud
+ * rates higher than 4800 after being idle for longer than 5 seconds.
+ * We work around this by sending a dummy report at least once per 4.8
+ * seconds if the UART is in use.
*/
static void ft260_uart_start_wakeup(struct timer_list *t)
{
@@ -1455,10 +1475,8 @@ static void ft260_uart_start_wakeup(struct timer_list *t)
}
}
-static void ft260_uart_do_wakeup(struct work_struct *work)
+static void ft260_uart_wakeup(struct ft260_device *dev)
{
- struct ft260_device *dev =
- container_of(work, struct ft260_device, wakeup_work);
struct ft260_get_chip_version_report version;
int ret;
@@ -1472,12 +1490,20 @@ static void ft260_uart_do_wakeup(struct work_struct *work)
}
}
+static void ft260_uart_do_wakeup(struct work_struct *work)
+{
+ struct ft260_device *dev =
+ container_of(work, struct ft260_device, wakeup_work);
+
+ ft260_uart_wakeup(dev);
+}
+
static void ft260_uart_shutdown(struct tty_port *tport)
{
struct ft260_device *port =
container_of(tport, struct ft260_device, port);
- port->reschedule_work = false;
+ ft260_uart_wakeup_workaraund_enable(port, false);
}
static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
@@ -1575,7 +1601,7 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
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 = true;
+ ft260_uart_wakeup_workaraund_enable(dev, true);
/* Work not started at this point */
timer_setup(&dev->wakeup_timer, ft260_uart_start_wakeup, 0);
--
2.40.1
Waking up the ft260 device from power saving mode earlier reduces the
probability of incoming data loss.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index ccd20f590720..6266e4f1100d 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1285,8 +1285,6 @@ static int ft260_uart_change_speed(struct ft260_device *port,
bool wakeup_workaraund = false;
int ret;
- ft260_uart_wakeup(port);
-
memset(&req, 0, sizeof(req));
req.report = FT260_SYSTEM_SETTINGS;
@@ -1529,6 +1527,9 @@ static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
ft260_uart_change_speed(port, &tty->termios, NULL);
clear_bit(TTY_IO_ERROR, &tty->flags);
+ /* Wake up the chip as early as possible to not miss incoming data */
+ ft260_uart_wakeup(port);
+
if (port->reschedule_work) {
mod_timer(&port->wakeup_timer, jiffies +
msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
--
2.40.1
Do not activate the 4.8-second wakeup workaround if power saving mode is
disabled in EEPROM.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index b24998092d22..ccd20f590720 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -323,6 +323,7 @@ struct ft260_device {
struct timer_list wakeup_timer;
struct work_struct wakeup_work;
bool reschedule_work;
+ bool power_saving_en;
struct completion wait;
struct mutex lock;
@@ -889,6 +890,7 @@ static int ft260_get_interface_type(struct hid_device *hdev, struct ft260_device
ft260_dbg("uart_mode: 0x%02x\n", cfg.uart_mode);
dev->ft260_is_serial = false;
+ dev->power_saving_en = cfg.power_saving_en;
switch (cfg.chip_mode) {
case FT260_MODE_ALL:
@@ -1063,8 +1065,11 @@ static void ft260_uart_wakeup(struct ft260_device *dev);
static void ft260_uart_wakeup_workaraund_enable(struct ft260_device *port,
bool enable)
{
- port->reschedule_work = enable;
- ft260_dbg("%s wakeup workaround", enable ? "Activate" : "Deactivate");
+ if (port->power_saving_en) {
+ port->reschedule_work = enable;
+ ft260_dbg("%s wakeup workaround",
+ enable ? "Activate" : "Deactivate");
+ }
}
static struct ft260_device *ft260_dev_by_index(int index)
--
2.40.1
The wakeup workaround is terminated twice: first in the
ft260_uart_port_shutdown and then, in the ft260_uart_port_remove routine.
We do not need to do it in the ft260_uart_port_remove routine since
it's always called when the wakeup mechanism is inactive:
1. Upon the ft260_uart_probe failure.
2. Upon the device/driver removal.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 63839f02e9b5..f8d4bf7e6c4f 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1124,7 +1124,6 @@ static void ft260_uart_port_remove(struct ft260_device *port)
spin_unlock(&port->write_lock);
mutex_lock(&port->port.mutex);
- ft260_uart_wakeup_workaraund_enable(port, false);
tty_port_tty_hangup(&port->port, false);
mutex_unlock(&port->port.mutex);
--
2.40.1
This commit simplifies the ft260_get_interface_type routine by
replacing the ft260_is_serial with iface_type and making use of
its return value as it's in the mainline ft260 driver code.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 3d1a9ec88cb9..9ecd91d173d2 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -309,7 +309,7 @@ struct ft260_device {
struct i2c_adapter adap;
struct hid_device *hdev;
- bool ft260_is_serial;
+ int iface_type;
struct list_head device_list;
/* tty_port lifetime is equal to device lifetime */
@@ -889,28 +889,25 @@ static int ft260_get_interface_type(struct hid_device *hdev, struct ft260_device
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;
dev->power_saving_en = cfg.power_saving_en;
switch (cfg.chip_mode) {
case FT260_MODE_ALL:
case FT260_MODE_BOTH:
- if (interface == 1) {
+ if (interface == 1)
ret = FT260_IFACE_UART;
- dev->ft260_is_serial = true;
- } else {
+ else
ret = FT260_IFACE_I2C;
- }
break;
case FT260_MODE_UART:
ret = FT260_IFACE_UART;
- dev->ft260_is_serial = true;
break;
case FT260_MODE_I2C:
ret = FT260_IFACE_I2C;
break;
}
+ dev->iface_type = ret;
return ret;
}
@@ -1713,15 +1710,12 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
mutex_init(&dev->lock);
init_completion(&dev->wait);
- if (!dev->ft260_is_serial) {
+ if (ret == FT260_IFACE_I2C)
ret = ft260_i2c_probe(hdev, dev);
- if (ret)
- goto err_hid_close;
- } else {
+ else
ret = ft260_uart_probe(hdev, dev);
- if (ret)
- goto err_hid_close;
- }
+ if (ret)
+ goto err_hid_close;
return 0;
@@ -1742,7 +1736,7 @@ static void ft260_remove(struct hid_device *hdev)
if (!dev)
return;
- if (dev->ft260_is_serial) {
+ if (dev->iface_type == FT260_IFACE_UART) {
// FIXME:
cancel_work_sync(&dev->wakeup_work);
tty_port_unregister_device(&dev->port, ft260_tty_driver,
--
2.40.1
Replace uart fifo write room calculation with the kfifo_avail kernel API.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index f8d4bf7e6c4f..3d1a9ec88cb9 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1264,7 +1264,7 @@ 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);
+ return kfifo_avail(&port->xmit_fifo);
}
static unsigned int ft260_uart_chars_in_buffer(struct tty_struct *tty)
--
2.40.1
- Cleanup printouts and comments.
- Refactor to adjust to the module's style.
- Replace the kfifo_in_locked/kfifo_out_locked with
kfifo_in_spinlocked/kfifo_out_spinlocked since the former may be
dropped in the future.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 148 ++++++++++++++++++----------------------
1 file changed, 66 insertions(+), 82 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 9ecd91d173d2..1c113f735524 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * hid-ft260.c - FTDI FT260 USB HID to I2C host bridge
+ * FTDI FT260 USB HID to I2C/UART host bridge
*
* Copyright (c) 2021, Michael Zaidman <[email protected]>
*
@@ -18,11 +18,7 @@
#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)
+#include <asm/unaligned.h>
#ifdef DEBUG
static int ft260_debug = 1;
@@ -148,7 +144,7 @@ enum {
FT260_FLAG_START_STOP_REPEATED = 0x07,
};
-/* Return values for ft260_get_interface_type func */
+/* USB interface type values */
enum {
FT260_IFACE_NONE,
FT260_IFACE_I2C,
@@ -250,18 +246,19 @@ struct ft260_input_report {
} __packed;
/* UART reports */
+
struct ft260_uart_write_request_report {
u8 report; /* FT260_UART_REPORT */
u8 length; /* data payload length */
u8 data[FT260_WR_UART_DATA_MAX]; /* data payload */
} __packed;
-struct ft260_configure_uart_request {
+struct ft260_configure_uart_request_report {
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: */
+ /* 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 */
@@ -296,7 +293,11 @@ enum {
FT260_CFG_BAUD_MAX = 12000000,
};
-#define FT260_UART_EN_PW_SAVE_BAUD 4800
+#define FT260_UART_EN_PW_SAVE_BAUD (4800)
+
+#define UART_COUNT_MAX (4) /* Number of supported UARTs */
+#define XMIT_FIFO_SIZE (256)
+#define TTY_WAKEUP_WATERMARK (XMIT_FIFO_SIZE / 2)
static const struct hid_device_id ft260_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY,
@@ -308,23 +309,18 @@ MODULE_DEVICE_TABLE(hid, ft260_devices);
struct ft260_device {
struct i2c_adapter adap;
struct hid_device *hdev;
-
int iface_type;
struct list_head device_list;
-
- /* tty_port lifetime is equal to device lifetime */
struct tty_port port;
+ /* tty port index */
unsigned int index;
struct kfifo xmit_fifo;
- /* write_lock: lock to serialize access to xmit fifo */
- spinlock_t write_lock;
+ spinlock_t xmit_fifo_lock;
struct uart_icount icount;
-
struct timer_list wakeup_timer;
struct work_struct wakeup_work;
bool reschedule_work;
bool power_saving_en;
-
struct completion wait;
struct mutex lock;
u8 i2c_wr_buf[FT260_REPORT_MAX_LEN];
@@ -337,8 +333,7 @@ struct ft260_device {
};
static int ft260_hid_feature_report_get(struct hid_device *hdev,
- unsigned char report_id, u8 *data,
- size_t len)
+ u8 report_id, u8 *data, size_t len)
{
u8 *buf;
int ret;
@@ -467,8 +462,6 @@ static int ft260_hid_output_report_check_status(struct ft260_device *dev,
ret = ft260_hid_output_report(hdev, data, len);
if (ret < 0) {
- hid_dbg(hdev, "%s: failed to start transfer, ret %d\n",
- __func__, ret);
ft260_i2c_reset(hdev);
return ret;
}
@@ -579,6 +572,8 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
rep->report, addr, cmd, rep->length, len);
ret = ft260_hid_output_report_check_status(dev, (u8 *)rep, len);
+ if (ret < 0)
+ hid_err(dev->hdev, "%s: failed with %d\n", __func__, ret);
return ret;
}
@@ -682,8 +677,7 @@ static int ft260_i2c_write_read(struct ft260_device *dev, struct i2c_msg *msgs)
else
read_off = *msgs[0].buf;
- ft260_dbg("%s: off %#x rlen %d wlen %d\n", __func__,
- read_off, rd_len, wr_len);
+ ft260_dbg("off %#x rlen %d wlen %d\n", read_off, rd_len, wr_len);
}
ret = ft260_i2c_write(dev, addr, msgs[0].buf, wr_len,
@@ -1051,9 +1045,6 @@ static const struct attribute_group ft260_attr_group = {
}
};
-/***
- * START Serial dev part
- */
static DEFINE_MUTEX(ft260_uart_list_lock);
static LIST_HEAD(ft260_uart_device_list);
@@ -1065,7 +1056,7 @@ static void ft260_uart_wakeup_workaraund_enable(struct ft260_device *port,
if (port->power_saving_en) {
port->reschedule_work = enable;
ft260_dbg("%s wakeup workaround",
- enable ? "Activate" : "Deactivate");
+ enable ? "activate" : "deactivate");
}
}
@@ -1085,8 +1076,8 @@ static int ft260_uart_add_port(struct ft260_device *port)
int index = 0, ret = 0;
struct ft260_device *dev;
- spin_lock_init(&port->write_lock);
- if (kfifo_alloc(&port->xmit_fifo, FIFO_SIZE, GFP_KERNEL))
+ spin_lock_init(&port->xmit_fifo_lock);
+ if (kfifo_alloc(&port->xmit_fifo, XMIT_FIFO_SIZE, GFP_KERNEL))
return -ENOMEM;
mutex_lock(&ft260_uart_list_lock);
@@ -1116,9 +1107,9 @@ static void ft260_uart_port_remove(struct ft260_device *port)
list_del(&port->device_list);
mutex_unlock(&ft260_uart_list_lock);
- spin_lock(&port->write_lock);
+ spin_lock(&port->xmit_fifo_lock);
kfifo_free(&port->xmit_fifo);
- spin_unlock(&port->write_lock);
+ spin_unlock(&port->xmit_fifo_lock);
mutex_lock(&port->port.mutex);
tty_port_tty_hangup(&port->port, false);
@@ -1127,7 +1118,7 @@ static void ft260_uart_port_remove(struct ft260_device *port)
ft260_uart_port_put(port);
}
-static struct ft260_device *ft260_uart_port_get(unsigned int index)
+static struct ft260_device *ft260_uart_port_get(int index)
{
struct ft260_device *port;
@@ -1191,20 +1182,18 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
rep->report = FT260_UART_DATA_REPORT_ID(len);
rep->length = len;
- len = kfifo_out_locked(xmit, rep->data, len, &port->write_lock);
+ len = kfifo_out_spinlocked(xmit, rep->data, len, &port->xmit_fifo_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);
+ if (ret < 0)
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)
+ if ((XMIT_FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK)
tty_wakeup(tty);
ret = 0;
@@ -1214,15 +1203,14 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
return ret;
}
-static int ft260_uart_receive_chars(struct ft260_device *port,
- u8 *data, u8 length)
+static int ft260_uart_receive_chars(struct ft260_device *port, u8 *data, u8 length)
{
- struct hid_device *hdev = port->hdev;
int ret;
ret = tty_insert_flip_string(&port->port, data, length);
if (ret != length)
- hid_dbg(hdev, "%d char not inserted to flip buffer\n", length - ret);
+ ft260_dbg("%d char not inserted to flip buf\n", length - ret);
+
port->icount.rx += ret;
if (ret)
@@ -1231,27 +1219,25 @@ static int ft260_uart_receive_chars(struct ft260_device *port,
return ret;
}
-static int ft260_uart_write(struct tty_struct *tty, const unsigned char *buf,
- int count)
+static int ft260_uart_write(struct tty_struct *tty, const u8 *buf, int cnt)
{
struct ft260_device *port = tty->driver_data;
- struct hid_device *hdev = port->hdev;
- int len, ret;
+ int len, ret, diff;
- len = kfifo_in_locked(&port->xmit_fifo, buf, count, &port->write_lock);
- ft260_dbg("count: %d, len: %d", count, len);
+ len = kfifo_in_spinlocked(&port->xmit_fifo, buf, cnt, &port->xmit_fifo_lock);
+ ft260_dbg("count: %d, len: %d", cnt, len);
ret = ft260_uart_transmit_chars(port);
if (ret < 0) {
- hid_dbg(hdev, "Failed to transmit chars: %d\n", ret);
+ ft260_dbg("failed to transmit %d\n", ret);
return 0;
}
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;
+ diff = len - ret;
+ ft260_dbg("failed to send %d out of %d bytes\n", diff, len);
+ return diff;
}
return len;
@@ -1277,7 +1263,7 @@ static int ft260_uart_change_speed(struct ft260_device *port,
{
struct hid_device *hdev = port->hdev;
unsigned int baud;
- struct ft260_configure_uart_request req;
+ struct ft260_configure_uart_request_report req;
bool wakeup_workaraund = false;
int ret;
@@ -1292,7 +1278,7 @@ static int ft260_uart_change_speed(struct ft260_device *port,
break;
case CS5:
case CS6:
- hid_err(hdev, "Invalid data bit size, setting to default (8 bit)\n");
+ hid_err(hdev, "invalid data bit size, setting a default\n");
req.data_bit = FT260_CFG_DATA_BITS_8;
termios->c_cflag &= ~CSIZE;
termios->c_cflag |= CS8;
@@ -1317,7 +1303,7 @@ static int ft260_uart_change_speed(struct ft260_device *port,
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);
+ hid_err(hdev, "invalid baud rate %d\n", baud);
baud = 9600;
tty_encode_baud_rate(tty, baud, baud);
tty_kref_put(tty);
@@ -1335,7 +1321,7 @@ static int ft260_uart_change_speed(struct ft260_device *port,
else
req.flow_ctrl = FT260_CFG_FLOW_CTRL_OFF;
- ft260_dbg("Configured termios: flow control: %d, baudrate: %d, ",
+ 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,
@@ -1346,7 +1332,7 @@ static int ft260_uart_change_speed(struct ft260_device *port,
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);
+ hid_err(hdev, "failed to change termios: %d\n", ret);
return ret;
}
@@ -1396,8 +1382,8 @@ 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",
- "", "", "");
+ 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);
@@ -1452,7 +1438,8 @@ static const struct tty_operations ft260_uart_ops = {
.get_icount = ft260_uart_get_icount,
};
-/* The FT260 has a "power saving mode" that causes the device to switch
+/*
+ * 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.
@@ -1476,16 +1463,14 @@ static void ft260_uart_start_wakeup(struct timer_list *t)
static void ft260_uart_wakeup(struct ft260_device *dev)
{
- struct ft260_get_chip_version_report version;
+ struct ft260_get_chip_version_report ver;
int ret;
if (dev->reschedule_work) {
ret = ft260_hid_feature_report_get(dev->hdev, FT260_CHIP_VERSION,
- (u8 *)&version, sizeof(version));
+ (u8 *)&ver, sizeof(ver));
if (ret < 0)
- hid_err(dev->hdev,
- "%s: failed to start transfer, ret %d\n",
- __func__, ret);
+ hid_err(dev->hdev, "%s: failed with %d\n", __func__, ret);
}
}
@@ -1497,7 +1482,7 @@ static void ft260_uart_do_wakeup(struct work_struct *work)
ft260_uart_wakeup(dev);
}
-static void ft260_uart_shutdown(struct tty_port *tport)
+static void ft260_uart_port_shutdown(struct tty_port *tport)
{
struct ft260_device *port =
container_of(tport, struct ft260_device, port);
@@ -1505,7 +1490,7 @@ static void ft260_uart_shutdown(struct tty_port *tport)
ft260_uart_wakeup_workaraund_enable(port, false);
}
-static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
+static int ft260_uart_port_activate(struct tty_port *tport, struct tty_struct *tty)
{
struct ft260_device *port =
container_of(tport, struct ft260_device, port);
@@ -1516,9 +1501,9 @@ static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
*/
set_bit(TTY_IO_ERROR, &tty->flags);
- spin_lock(&port->write_lock);
+ spin_lock(&port->xmit_fifo_lock);
kfifo_reset(&port->xmit_fifo);
- spin_unlock(&port->write_lock);
+ spin_unlock(&port->xmit_fifo_lock);
clear_bit(TTY_IO_ERROR, &tty->flags);
@@ -1542,8 +1527,8 @@ static void ft260_uart_port_destroy(struct tty_port *tport)
}
static const struct tty_port_operations ft260_uart_port_ops = {
- .shutdown = ft260_uart_shutdown,
- .activate = ft260_uart_activate,
+ .shutdown = ft260_uart_port_shutdown,
+ .activate = ft260_uart_port_activate,
.destruct = ft260_uart_port_destroy,
};
@@ -1595,7 +1580,7 @@ static int ft260_i2c_probe(struct hid_device *hdev, struct ft260_device *dev)
static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
{
- struct ft260_configure_uart_request req;
+ struct ft260_configure_uart_request_report req;
int ret;
struct device *devt;
@@ -1623,10 +1608,10 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
ret = PTR_ERR(devt);
goto err_register_tty;
}
- hid_info(hdev, "Registering device /dev/%s%d\n",
+ 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 */
+ /* Configure UART to 9600n8 */
req.report = FT260_SYSTEM_SETTINGS;
req.request = FT260_SET_UART_CONFIG;
req.flow_ctrl = FT260_CFG_FLOW_CTRL_NONE;
@@ -1638,8 +1623,7 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
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);
+ hid_err(hdev, "failed to configure uart: %d\n", ret);
goto err_hid_report;
}
@@ -1660,9 +1644,9 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (!hid_is_usb(hdev))
return -EINVAL;
-
- /* We cannot used devm_kzalloc here, because port has to survive until
- * destroy function call
+ /*
+ * We cannot use devm_kzalloc here because the port has to survive
+ * until destroy function call.
*/
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev) {
@@ -1742,7 +1726,7 @@ static void ft260_remove(struct hid_device *hdev)
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 */
+ /* dev is 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);
@@ -1781,7 +1765,7 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
return 0;
} else if (xfer->length > FT260_RD_DATA_MAX) {
- hid_err(hdev, "Received data too long (%d)\n", xfer->length);
+ 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) {
@@ -1830,7 +1814,7 @@ static int __init ft260_driver_init(void)
goto err_reg_driver;
}
- ret = hid_register_driver(&(ft260_driver));
+ ret = hid_register_driver(&ft260_driver);
if (ret) {
pr_err("hid_register_driver failed: %d\n", ret);
goto err_reg_hid;
@@ -1848,7 +1832,7 @@ static int __init ft260_driver_init(void)
static void __exit ft260_driver_exit(void)
{
- hid_unregister_driver(&(ft260_driver));
+ hid_unregister_driver(&ft260_driver);
tty_unregister_driver(ft260_tty_driver);
tty_driver_kref_put(ft260_tty_driver);
}
--
2.40.1
Suppress the "unhandled report 0xb1" error since it's related to the UART
DCD/RI function of the multifunctional GPIO pins status, which we do not
use for serial console. The configuration of these pins is a part
of the GPIO patch set.
[ 5453.117113] ft260 0003:0403:6030.0008: unhandled report 0xb1
[ 6641.582307] ft260 0003:0403:6030.0008: unhandled report 0xb1
[13418.439085] ft260 0003:0403:6030.0008: unhandled report 0xb1
[14110.820786] ft260 0003:0403:6030.0008: unhandled report 0xb1
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 52ccee83250e..d7eb00aeb669 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1768,6 +1768,8 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
} else if (xfer->report >= FT260_UART_REPORT_MIN &&
xfer->report <= FT260_UART_REPORT_MAX) {
return ft260_uart_receive_chars(dev, xfer->data, xfer->length);
+ } else if (xfer->report == FT260_UART_INTERRUPT_STATUS) {
+ return 0;
}
hid_err(hdev, "unhandled report %#02x\n", xfer->report);
--
2.40.1
The port setting may remain intact after session termination. Then, when
reopening the port without configuring the port setting, the wakeup
mechanism is not activated, and UART Rx loses the data at speeds greater
than 4800 bauds. To fix it, retrieve the baud rate from the device in the
ft260_uart_port_activate to reactivate the wakeup workaround if needed.
Example:
1. Configure the baud rate to 115200:
$ sudo picocom -f n -p n -d 8 -b 115200 /dev/ttyFT0
2. Quit the picocom without resetting port setting by entering:
C-a and C-q
It deactivates the wakeup workaround:
[31677.005464] ft260_uart_close:
[31677.005466] ft260_uart_chars_in_buffer:
[31677.005467] ft260_uart_port_shutdown:
[31677.005468] ft260_uart_wakeup_workaraund_enable: deactivate wakeup workaround
[31677.005476] ft260_uart_cleanup:
[31677.005477] ft260_uart_port_put:
3. Reopen the ttyFT0 port, but do not configure it. The fix retrieves the port
baud rate from the device, compares it against the threshold, and activates
the wakeup mechanism when needed.
$ sudo bash -c "cat < /dev/ttyFT0"
[31693.304991] ft260_uart_port_get:
[31693.304995] ft260_uart_install:
[31693.305000] ft260_uart_open:
[31693.305001] ft260_uart_port_activate:
[31693.309842] ft260_uart_wakeup_workaraund_enable: activate wakeup workaround
[31693.309847] ft260_uart_port_activate: configurd baudrate = 115200
[31698.132650] ft260_uart_start_wakeup:
[31698.132809] ft260_uart_do_wakeup:
[31698.132817] ft260_uart_wakeup:
[31702.996905] ft260_uart_start_wakeup:
[31702.996916] ft260_uart_do_wakeup:
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 56 ++++++++++++++++++++++++++++++++++-------
1 file changed, 47 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 7f3ef4f20075..6b172bfa4f98 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -90,7 +90,7 @@ enum {
FT260_I2C_REPORT_MAX = 0xDE,
FT260_GPIO = 0xB0,
FT260_UART_INTERRUPT_STATUS = 0xB1,
- FT260_UART_STATUS = 0xE0,
+ FT260_UART_SETTINGS = 0xE0,
FT260_UART_RI_DCD_STATUS = 0xE1,
FT260_UART_REPORT_MIN = 0xF0,
FT260_UART_REPORT_MAX = 0xFE,
@@ -190,6 +190,18 @@ struct ft260_get_i2c_status_report {
u8 reserved;
} __packed;
+struct ft260_get_uart_settings_report {
+ u8 report; /* FT260_UART_SETTINGS */
+ u8 flow_ctrl; /* 0 - OFF; 1 - RTS_CTS, 2 - DTR_DSR, */
+ /* 3 - XON_XOFF, 4 - No flow control */
+ /* 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;
+
/* Feature Out reports */
struct ft260_set_system_clock_report {
@@ -1050,6 +1062,21 @@ static LIST_HEAD(ft260_uart_device_list);
static void ft260_uart_wakeup(struct ft260_device *dev);
+static int ft260_get_uart_settings(struct hid_device *hdev,
+ struct ft260_get_uart_settings_report *cfg)
+{
+ int ret;
+ int len = sizeof(struct ft260_get_uart_settings_report);
+
+ ret = ft260_hid_feature_report_get(hdev, FT260_UART_SETTINGS,
+ (u8 *)cfg, len);
+ if (ret < 0) {
+ hid_err(hdev, "failed to retrieve uart settings\n");
+ return ret;
+ }
+ return 0;
+}
+
static void ft260_uart_wakeup_workaraund_enable(struct ft260_device *port,
bool enable)
{
@@ -1492,13 +1519,11 @@ static void ft260_uart_port_shutdown(struct tty_port *tport)
static int ft260_uart_port_activate(struct tty_port *tport, struct tty_struct *tty)
{
- struct ft260_device *port =
- container_of(tport, struct ft260_device, port);
+ int ret;
+ int baudrate;
+ struct ft260_get_uart_settings_report cfg;
+ 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->xmit_fifo_lock);
@@ -1507,8 +1532,21 @@ static int ft260_uart_port_activate(struct tty_port *tport, struct tty_struct *t
clear_bit(TTY_IO_ERROR, &tty->flags);
- /* Wake up the chip as early as possible to not miss incoming data */
- ft260_uart_wakeup(port);
+ /*
+ * The port setting may remain intact after session termination.
+ * Then, when reopening the port without configuring the port
+ * setting, we need to retrieve the baud rate from the device to
+ * reactivate the wakeup workaround if needed.
+ */
+ ret = ft260_get_uart_settings(port->hdev, &cfg);
+ if (ret)
+ return ret;
+
+ baudrate = get_unaligned_le32(&cfg.baudrate);
+ if (baudrate > FT260_UART_EN_PW_SAVE_BAUD)
+ ft260_uart_wakeup_workaraund_enable(port, true);
+
+ ft260_dbg("configurd baudrate = %d", baudrate);
mod_timer(&port->wakeup_timer, jiffies +
msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
--
2.40.1
Tx performance with the current buffer size of 256 bytes is lower when
the data length exceeds the xmit buf size.
[134331.147978] ft260_uart_write: count: 288, len: 256
[134331.157945] ft260_uart_write: count: 32, len: 32
[134331.159977] ft260_uart_write: count: 288, len: 256
[134331.169990] ft260_uart_write: count: 32, len: 32
1. Increase the xmit buffer size to page size as used in the serial core
and other tty drivers.
2. Remove the xmit buffer fulness against the watermark checking and the
tty_wakeup calling in the ft260_uart_transmit_chars routine. This code is
taken from other drivers, but other drivers may call the routine from the
interrupt context. In our case, this condition is always True since xmit
buffer filling and emptying are serialized and done synchronously.
Tested with picocom ASCII file transfer by 288-byte chunks at 921600
bauds rate with above 20% performance improvement.
Before:
2821.7 Kbytes transferred at 47367 CPS... Done.
After:
2821.7 Kbytes transferred at 57788 CPS... Done.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 6b172bfa4f98..1188b8e09938 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -308,8 +308,7 @@ enum {
#define FT260_UART_EN_PW_SAVE_BAUD (4800)
#define UART_COUNT_MAX (4) /* Number of supported UARTs */
-#define XMIT_FIFO_SIZE (256)
-#define TTY_WAKEUP_WATERMARK (XMIT_FIFO_SIZE / 2)
+#define XMIT_FIFO_SIZE (PAGE_SIZE)
static const struct hid_device_id ft260_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY,
@@ -1211,7 +1210,7 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
len = kfifo_out_spinlocked(xmit, rep->data, len, &port->xmit_fifo_lock);
- ret = ft260_hid_output_report(hdev, (u8 *)rep, len + sizeof(*rep));
+ ret = ft260_hid_output_report(hdev, (u8 *)rep, len + 2);
if (ret < 0)
goto tty_out;
@@ -1219,10 +1218,6 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
port->icount.tx += len;
} while (data_len > 0);
- len = kfifo_len(xmit);
- if ((XMIT_FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK)
- tty_wakeup(tty);
-
ret = 0;
tty_out:
@@ -1546,7 +1541,7 @@ static int ft260_uart_port_activate(struct tty_port *tport, struct tty_struct *t
if (baudrate > FT260_UART_EN_PW_SAVE_BAUD)
ft260_uart_wakeup_workaraund_enable(port, true);
- ft260_dbg("configurd baudrate = %d", baudrate);
+ ft260_dbg("configured baudrate = %d", baudrate);
mod_timer(&port->wakeup_timer, jiffies +
msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
--
2.40.1
The uart speed settings are configured twice per session:
the old settings - by the ft260_uart_port_activate() and the new -
by the ft260_uart_set_termios() routine. We do not need to configure
the old settings per tty session since they have already been sent
to the device in the probe and passed to the tty via termios in the
ft260_driver_init.
Removed their coonfiguration from the ft260_uart_port_activate routine.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 6266e4f1100d..63839f02e9b5 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1524,7 +1524,6 @@ static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
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);
/* Wake up the chip as early as possible to not miss incoming data */
--
2.40.1
The FIXME is related to the wake-up workaround cancelation - do we
need the reschedule_work flag alongside the cancel_work_sync usage?
The teardown sequence is as described below:
Upon tty session termination:
ft260_uart_port_shutdown
ft260_uart_wakeup_workaraund_enable: deactivate wakeup workaround
t260_uart_cleanup
ft260_uart_port_put
On rmmod:
ft260_remove
cancel_work_sync
ft260_uart_port_remove
timer_delete_sync
ft260_uart_port_put
ft260_uart_port_destroy
The ft260_uart_start_wakeup timer_work can occur after the cancel_work_sync
returns, rescheduling the wakeup_work again. The reschedule_work flag set
earlier at the ft260_uart_port_shutdown time prevents it.
The alternative could be performing the timer_delete_sync first, preventing
the wakeup_timer and wakeup_work from rescheduling, and then canceling the
last wakeup_work by calling the cancel_work_sync. However, we still need the
reschedule_work flag to enable or bypass the power saving mode according to
the requested baud rate.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 1c113f735524..52ccee83250e 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1585,7 +1585,6 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
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?
ft260_uart_wakeup_workaraund_enable(dev, true);
/* Work not started at this point */
@@ -1721,7 +1720,6 @@ static void ft260_remove(struct hid_device *hdev)
return;
if (dev->iface_type == FT260_IFACE_UART) {
- // FIXME:
cancel_work_sync(&dev->wakeup_work);
tty_port_unregister_device(&dev->port, ft260_tty_driver,
dev->index);
--
2.40.1
The previous session deactivated the wake-up workaround. The next session calls
ft260_uart_port_activate while the wake-up workaround flag is yet deactivated.
It is enabled later, conditionally, in the ft260_uart_change_speed. Thus, the
timer is never armed again, and the device enters power-saving mode and misses
the incoming data.
Arming the wake-up timer unconditionally on the tty session start resolved the
issue.
Signed-off-by: Michael Zaidman <[email protected]>
---
drivers/hid/hid-ft260.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index d7eb00aeb669..7f3ef4f20075 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -1510,10 +1510,8 @@ static int ft260_uart_port_activate(struct tty_port *tport, struct tty_struct *t
/* Wake up the chip as early as possible to not miss incoming data */
ft260_uart_wakeup(port);
- if (port->reschedule_work) {
- mod_timer(&port->wakeup_timer, jiffies +
- msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
- }
+ mod_timer(&port->wakeup_timer, jiffies +
+ msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
return 0;
}
--
2.40.1
On Sat, 10 Feb 2024, Michael Zaidman wrote:
> Modifications on top of "[PATCH v4 RESEND] hid-ft260: Add serial driver"
> https://lore.kernel.org/all/[email protected]/
>
> They are mostly the fixes to the original v4 patch and should be melded into
> the v5 patch rather than upstreamed as separate patches.
Agreed; I am not acting upon this series now then, and will wait for v5
with these folded in.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi Jiri!
I send Patch v5, feel free to have a look!
Best regards,
Christina
On 2/13/24 11:20, Jiri Kosina wrote:
> On Sat, 10 Feb 2024, Michael Zaidman wrote:
>
>> Modifications on top of "[PATCH v4 RESEND] hid-ft260: Add serial driver"
>> https://lore.kernel.org/all/[email protected]/
>>
>> They are mostly the fixes to the original v4 patch and should be melded into
>> the v5 patch rather than upstreamed as separate patches.
> Agreed; I am not acting upon this series now then, and will wait for v5
> with these folded in.
>
> Thanks,
>
On Sat, Feb 10, 2024 at 11:51:47PM +0200, Michael Zaidman wrote:
> Tx performance with the current buffer size of 256 bytes is lower when
> the data length exceeds the xmit buf size.
>
> [134331.147978] ft260_uart_write: count: 288, len: 256
> [134331.157945] ft260_uart_write: count: 32, len: 32
> [134331.159977] ft260_uart_write: count: 288, len: 256
> [134331.169990] ft260_uart_write: count: 32, len: 32
>
> 1. Increase the xmit buffer size to page size as used in the serial core
> and other tty drivers.
>
> 2. Remove the xmit buffer fulness against the watermark checking and the
> tty_wakeup calling in the ft260_uart_transmit_chars routine. This code is
> taken from other drivers, but other drivers may call the routine from the
> interrupt context. In our case, this condition is always True since xmit
> buffer filling and emptying are serialized and done synchronously.
>
> Tested with picocom ASCII file transfer by 288-byte chunks at 921600
> bauds rate with above 20% performance improvement.
>
> Before:
> 2821.7 Kbytes transferred at 47367 CPS... Done.
>
> After:
> 2821.7 Kbytes transferred at 57788 CPS... Done.
>
Besides the performance improvement, it fixes the bug of outputting
characters to the local terminal when local echo is disabled and
printing every character twice with local echo enabled.
> Signed-off-by: Michael Zaidman <[email protected]>
> ---
> drivers/hid/hid-ft260.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 6b172bfa4f98..1188b8e09938 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -308,8 +308,7 @@ enum {
> #define FT260_UART_EN_PW_SAVE_BAUD (4800)
>
> #define UART_COUNT_MAX (4) /* Number of supported UARTs */
> -#define XMIT_FIFO_SIZE (256)
> -#define TTY_WAKEUP_WATERMARK (XMIT_FIFO_SIZE / 2)
> +#define XMIT_FIFO_SIZE (PAGE_SIZE)
>
> static const struct hid_device_id ft260_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_FUTURE_TECHNOLOGY,
> @@ -1211,7 +1210,7 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
>
> len = kfifo_out_spinlocked(xmit, rep->data, len, &port->xmit_fifo_lock);
>
> - ret = ft260_hid_output_report(hdev, (u8 *)rep, len + sizeof(*rep));
> + ret = ft260_hid_output_report(hdev, (u8 *)rep, len + 2);
> if (ret < 0)
> goto tty_out;
>
> @@ -1219,10 +1218,6 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
> port->icount.tx += len;
> } while (data_len > 0);
>
> - len = kfifo_len(xmit);
> - if ((XMIT_FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK)
> - tty_wakeup(tty);
> -
> ret = 0;
>
> tty_out: