* [PATCH v2 09/49] Input: atmel_mxt_ts - implement T15 Key Array support
From: Jiada Wang @ 2019-08-27 6:26 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062714.20266-1-jiada_wang@mentor.com>
From: Nick Dyer <nick.dyer@itdev.co.uk>
There is a key array object in many maXTouch chips which allows some X/Y
lines to be used as a key array. This patch maps them to a series of keys
which may be configured in a platform data array.
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
(cherry picked from ndyer/linux/for-upstream commit 15bb074b5abf3a101f7b79544213f1c110ea4cab)
[gdavis: Resolve forward port conflicts due to applying upstream
commit 96a938aa214e ("Input: atmel_mxt_ts - remove platform
data support").]
Signed-off-by: George G. Davis <george_davis@mentor.com>
[jiada: Fix compilation warning]
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 85 ++++++++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 68c8237f7932..1d738c488bdd 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -314,6 +314,9 @@ struct mxt_data {
struct mxt_dbg dbg;
struct gpio_desc *reset_gpio;
bool use_retrigen_workaround;
+ unsigned long t15_keystatus;
+ int t15_num_keys;
+ const unsigned int *t15_keymap;
/* Cached parameters from object table */
u16 T5_address;
@@ -324,6 +327,8 @@ struct mxt_data {
u16 T71_address;
u8 T9_reportid_min;
u8 T9_reportid_max;
+ u8 T15_reportid_min;
+ u8 T15_reportid_max;
u16 T18_address;
u8 T19_reportid;
u8 T42_reportid_min;
@@ -987,6 +992,38 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
data->update_input = true;
}
+static void mxt_proc_t15_messages(struct mxt_data *data, u8 *msg)
+{
+ struct input_dev *input_dev = data->input_dev;
+ struct device *dev = &data->client->dev;
+ int key;
+ bool curr_state, new_state;
+ bool sync = false;
+ unsigned long keystates = le32_to_cpu((__force __le32)msg[2]);
+
+ for (key = 0; key < data->t15_num_keys; key++) {
+ curr_state = test_bit(key, &data->t15_keystatus);
+ new_state = test_bit(key, &keystates);
+
+ if (!curr_state && new_state) {
+ dev_dbg(dev, "T15 key press: %u\n", key);
+ __set_bit(key, &data->t15_keystatus);
+ input_event(input_dev, EV_KEY,
+ data->t15_keymap[key], 1);
+ sync = true;
+ } else if (curr_state && !new_state) {
+ dev_dbg(dev, "T15 key release: %u\n", key);
+ __clear_bit(key, &data->t15_keystatus);
+ input_event(input_dev, EV_KEY,
+ data->t15_keymap[key], 0);
+ sync = true;
+ }
+ }
+
+ if (sync)
+ input_sync(input_dev);
+}
+
static void mxt_proc_t42_messages(struct mxt_data *data, u8 *msg)
{
struct device *dev = &data->client->dev;
@@ -1045,6 +1082,9 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
} else if (report_id == data->T19_reportid) {
mxt_input_button(data, message);
data->update_input = true;
+ } else if (report_id >= data->T15_reportid_min
+ && report_id <= data->T15_reportid_max) {
+ mxt_proc_t15_messages(data, message);
} else {
mxt_dump_message(data, message);
}
@@ -1706,6 +1746,8 @@ static void mxt_free_object_table(struct mxt_data *data)
data->T71_address = 0;
data->T9_reportid_min = 0;
data->T9_reportid_max = 0;
+ data->T15_reportid_min = 0;
+ data->T15_reportid_max = 0;
data->T18_address = 0;
data->T19_reportid = 0;
data->T42_reportid_min = 0;
@@ -1784,6 +1826,10 @@ static int mxt_parse_object_table(struct mxt_data *data,
object->num_report_ids - 1;
data->num_touchids = object->num_report_ids;
break;
+ case MXT_TOUCH_KEYARRAY_T15:
+ data->T15_reportid_min = min_id;
+ data->T15_reportid_max = max_id;
+ break;
case MXT_SPT_COMMSCONFIG_T18:
data->T18_address = object->start_address;
break;
@@ -2077,6 +2123,7 @@ static int mxt_initialize_input_device(struct mxt_data *data)
int error;
unsigned int num_mt_slots;
unsigned int mt_flags = 0;
+ int i;
switch (data->multitouch) {
case MXT_TOUCH_MULTI_T9:
@@ -2190,6 +2237,15 @@ static int mxt_initialize_input_device(struct mxt_data *data)
0, 255, 0, 0);
}
+ /* For T15 Key Array */
+ if (data->T15_reportid_min) {
+ data->t15_keystatus = 0;
+
+ for (i = 0; i < data->t15_num_keys; i++)
+ input_set_capability(input_dev, EV_KEY,
+ data->t15_keymap[i]);
+ }
+
input_set_drvdata(input_dev, data);
error = input_register_device(input_dev);
@@ -3148,8 +3204,10 @@ static void mxt_input_close(struct input_dev *dev)
static int mxt_parse_device_properties(struct mxt_data *data)
{
static const char keymap_property[] = "linux,gpio-keymap";
+ static const char buttons_property[] = "atmel,key-buttons";
struct device *dev = &data->client->dev;
u32 *keymap;
+ u32 *buttonmap;
int n_keys;
int error;
@@ -3180,6 +3238,33 @@ static int mxt_parse_device_properties(struct mxt_data *data)
data->t19_num_keys = n_keys;
}
+ if (device_property_present(dev, buttons_property)) {
+ n_keys = device_property_read_u32_array(dev, buttons_property,
+ NULL, 0);
+ if (n_keys <= 0) {
+ error = n_keys < 0 ? n_keys : -EINVAL;
+ dev_err(dev, "invalid/malformed '%s' property: %d\n",
+ buttons_property, error);
+ return error;
+ }
+
+ buttonmap = devm_kmalloc_array(dev, n_keys, sizeof(*buttonmap),
+ GFP_KERNEL);
+ if (!buttonmap)
+ return -ENOMEM;
+
+ error = device_property_read_u32_array(dev, buttons_property,
+ buttonmap, n_keys);
+ if (error) {
+ dev_err(dev, "failed to parse '%s' property: %d\n",
+ buttons_property, error);
+ return error;
+ }
+
+ data->t15_keymap = buttonmap;
+ data->t15_num_keys = n_keys;
+ }
+
return 0;
}
--
2.19.2
^ permalink raw reply related
* [PATCH v2 08/49] Input: atmel_mxt_ts - implement T9 vector/orientation support
From: Jiada Wang @ 2019-08-27 6:26 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062714.20266-1-jiada_wang@mentor.com>
From: Nick Dyer <nick.dyer@itdev.co.uk>
The atmel touch messages contain orientation information as a byte in a
packed format which can be passed straight on to Android if the input
device configuration is correct.
This requires vector reports to be enabled in maXTouch config (zero
DISVECT bit 3 in T9 CTRL field)
Android converts the format in InputReader.cpp, search for
ORIENTATION_CALIBRATION_VECTOR.
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
(cherry picked from ndyer/linux/for-upstream commit a6f0ee919d2631678169b23fb18f55b6dbabcd4c)
Signed-off-by: George G. Davis <george_davis@mentor.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 19fa3e58269a..68c8237f7932 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -817,6 +817,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
int y;
int area;
int amplitude;
+ u8 vector;
id = message[0] - data->T9_reportid_min;
status = message[1];
@@ -831,9 +832,10 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
area = message[5];
amplitude = message[6];
+ vector = message[7];
dev_dbg(dev,
- "[%u] %c%c%c%c%c%c%c%c x: %5u y: %5u area: %3u amp: %3u\n",
+ "[%u] %c%c%c%c%c%c%c%c x: %5u y: %5u area: %3u amp: %3u vector: %02X\n",
id,
(status & MXT_T9_DETECT) ? 'D' : '.',
(status & MXT_T9_PRESS) ? 'P' : '.',
@@ -843,7 +845,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
(status & MXT_T9_AMP) ? 'A' : '.',
(status & MXT_T9_SUPPRESS) ? 'S' : '.',
(status & MXT_T9_UNGRIP) ? 'U' : '.',
- x, y, area, amplitude);
+ x, y, area, amplitude, vector);
input_mt_slot(input_dev, id);
@@ -868,6 +870,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
+ input_report_abs(input_dev, ABS_MT_ORIENTATION, vector);
} else {
/* Touch no longer active, close out slot */
input_mt_report_slot_inactive(input_dev);
@@ -2180,8 +2183,9 @@ static int mxt_initialize_input_device(struct mxt_data *data)
0, 255, 0, 0);
}
- if (data->multitouch == MXT_TOUCH_MULTITOUCHSCREEN_T100 &&
- data->t100_aux_vect) {
+ if (data->multitouch == MXT_TOUCH_MULTI_T9 ||
+ (data->multitouch == MXT_TOUCH_MULTITOUCHSCREEN_T100 &&
+ data->t100_aux_vect)) {
input_set_abs_params(input_dev, ABS_MT_ORIENTATION,
0, 255, 0, 0);
}
--
2.19.2
^ permalink raw reply related
* [PATCH v2 07/49] Input: atmel_mxt_ts - output status from T42 Touch Suppression
From: Jiada Wang @ 2019-08-27 6:26 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062714.20266-1-jiada_wang@mentor.com>
From: Nick Dyer <nick.dyer@itdev.co.uk>
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
(cherry picked from ndyer/linux/for-upstream commit ab95b5a309999d2c098daaa9f88d9fcfae7eb516)
Signed-off-by: George G. Davis <george_davis@mentor.com>
[jiada: Replace dev_info() with dev_dbg()]
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 25 ++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 351347e2eced..19fa3e58269a 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -155,6 +155,9 @@ struct t37_debug {
#define MXT_RESET_VALUE 0x01
#define MXT_BACKUP_VALUE 0x55
+/* Define for MXT_PROCI_TOUCHSUPPRESSION_T42 */
+#define MXT_T42_MSG_TCHSUP BIT(0)
+
/* T100 Multiple Touch Touchscreen */
#define MXT_T100_CTRL 0
#define MXT_T100_CFG1 1
@@ -323,6 +326,8 @@ struct mxt_data {
u8 T9_reportid_max;
u16 T18_address;
u8 T19_reportid;
+ u8 T42_reportid_min;
+ u8 T42_reportid_max;
u16 T44_address;
u8 T48_reportid;
u8 T100_reportid_min;
@@ -979,6 +984,17 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
data->update_input = true;
}
+static void mxt_proc_t42_messages(struct mxt_data *data, u8 *msg)
+{
+ struct device *dev = &data->client->dev;
+ u8 status = msg[1];
+
+ if (status & MXT_T42_MSG_TCHSUP)
+ dev_dbg(dev, "T42 suppress\n");
+ else
+ dev_dbg(dev, "T42 normal\n");
+}
+
static int mxt_proc_t48_messages(struct mxt_data *data, u8 *msg)
{
struct device *dev = &data->client->dev;
@@ -1006,6 +1022,9 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
if (report_id == data->T6_reportid) {
mxt_proc_t6_messages(data, message);
+ } else if (report_id >= data->T42_reportid_min
+ && report_id <= data->T42_reportid_max) {
+ mxt_proc_t42_messages(data, message);
} else if (report_id == data->T48_reportid) {
mxt_proc_t48_messages(data, message);
} else if (!data->input_dev) {
@@ -1686,6 +1705,8 @@ static void mxt_free_object_table(struct mxt_data *data)
data->T9_reportid_max = 0;
data->T18_address = 0;
data->T19_reportid = 0;
+ data->T42_reportid_min = 0;
+ data->T42_reportid_max = 0;
data->T44_address = 0;
data->T48_reportid = 0;
data->T100_reportid_min = 0;
@@ -1763,6 +1784,10 @@ static int mxt_parse_object_table(struct mxt_data *data,
case MXT_SPT_COMMSCONFIG_T18:
data->T18_address = object->start_address;
break;
+ case MXT_PROCI_TOUCHSUPPRESSION_T42:
+ data->T42_reportid_min = min_id;
+ data->T42_reportid_max = max_id;
+ break;
case MXT_SPT_MESSAGECOUNT_T44:
data->T44_address = object->start_address;
break;
--
2.19.2
^ permalink raw reply related
* [PATCH v2 06/49] Input: atmel_mxt_ts - output status from T48 Noise Supression
From: Jiada Wang @ 2019-08-27 6:26 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062714.20266-1-jiada_wang@mentor.com>
From: Nick Dyer <nick.dyer@itdev.co.uk>
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
(cherry picked from ndyer/linux/for-upstream commit 2895a6ff150a49f27a02938f8d262be238b296d8)
Signed-off-by: George G. Davis <george_davis@mentor.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 25 ++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 45bab5253775..351347e2eced 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -324,6 +324,7 @@ struct mxt_data {
u16 T18_address;
u8 T19_reportid;
u16 T44_address;
+ u8 T48_reportid;
u8 T100_reportid_min;
u8 T100_reportid_max;
@@ -978,6 +979,24 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
data->update_input = true;
}
+static int mxt_proc_t48_messages(struct mxt_data *data, u8 *msg)
+{
+ struct device *dev = &data->client->dev;
+ u8 status, state;
+
+ status = msg[1];
+ state = msg[4];
+
+ dev_dbg(dev, "T48 state %d status %02X %s%s%s%s%s\n", state, status,
+ status & 0x01 ? "FREQCHG " : "",
+ status & 0x02 ? "APXCHG " : "",
+ status & 0x04 ? "ALGOERR " : "",
+ status & 0x10 ? "STATCHG " : "",
+ status & 0x20 ? "NLVLCHG " : "");
+
+ return 0;
+}
+
static int mxt_proc_message(struct mxt_data *data, u8 *message)
{
u8 report_id = message[0];
@@ -987,6 +1006,8 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
if (report_id == data->T6_reportid) {
mxt_proc_t6_messages(data, message);
+ } else if (report_id == data->T48_reportid) {
+ mxt_proc_t48_messages(data, message);
} else if (!data->input_dev) {
/*
* Do not report events if input device
@@ -1666,6 +1687,7 @@ static void mxt_free_object_table(struct mxt_data *data)
data->T18_address = 0;
data->T19_reportid = 0;
data->T44_address = 0;
+ data->T48_reportid = 0;
data->T100_reportid_min = 0;
data->T100_reportid_max = 0;
data->max_reportid = 0;
@@ -1747,6 +1769,9 @@ static int mxt_parse_object_table(struct mxt_data *data,
case MXT_SPT_GPIOPWM_T19:
data->T19_reportid = min_id;
break;
+ case MXT_PROCG_NOISESUPPRESSION_T48:
+ data->T48_reportid = min_id;
+ break;
case MXT_TOUCH_MULTITOUCHSCREEN_T100:
data->multitouch = MXT_TOUCH_MULTITOUCHSCREEN_T100;
data->T100_reportid_min = min_id;
--
2.19.2
^ permalink raw reply related
* [PATCH v2 05/49] Input: atmel_mxt_ts - split large i2c transfers into blocks
From: Jiada Wang @ 2019-08-27 6:26 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
From: Nick Dyer <nick.dyer@itdev.co.uk>
On some firmware variants, the size of the info block exceeds what can
be read in a single transfer.
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
(cherry picked from ndyer/linux/for-upstream commit 74c4f5277cfa403d43fafc404119dc57a08677db)
[gdavis: Forward port and fix conflicts due to v4.14.51 commit
960fe000b1d3 ("Input: atmel_mxt_ts - fix the firmware
update").]
Signed-off-by: George G. Davis <george_davis@mentor.com>
[jiada: Change mxt_read_blks() to __mxt_read_reg(), original __mxt_read_reg() to
__mxt_read_chunk()]
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 28 +++++++++++++++++++++---
1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 35cbe60094ab..45bab5253775 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -40,7 +40,7 @@
#define MXT_OBJECT_START 0x07
#define MXT_OBJECT_SIZE 6
#define MXT_INFO_CHECKSUM_SIZE 3
-#define MXT_MAX_BLOCK_WRITE 256
+#define MXT_MAX_BLOCK_WRITE 255
/* Object types */
#define MXT_DEBUG_DIAGNOSTIC_T37 37
@@ -624,8 +624,8 @@ static int mxt_send_bootloader_cmd(struct mxt_data *data, bool unlock)
return 0;
}
-static int __mxt_read_reg(struct i2c_client *client,
- u16 reg, u16 len, void *val)
+static int __mxt_read_chunk(struct i2c_client *client,
+ u16 reg, u16 len, void *val)
{
struct i2c_msg xfer[2];
u8 buf[2];
@@ -659,6 +659,28 @@ static int __mxt_read_reg(struct i2c_client *client,
return ret;
}
+static int __mxt_read_reg(struct i2c_client *client,
+ u16 reg, u16 len, void *buf)
+{
+ u16 offset = 0;
+ int error;
+ u16 size;
+
+ while (offset < len) {
+ size = min(MXT_MAX_BLOCK_WRITE, len - offset);
+
+ error = __mxt_read_chunk(client,
+ reg + offset,
+ size, buf + offset);
+ if (error)
+ return error;
+
+ offset += size;
+ }
+
+ return 0;
+}
+
static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
const void *val)
{
--
2.19.2
^ permalink raw reply related
* [PATCH v2 04/49] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary
From: Jiada Wang @ 2019-08-27 6:25 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062607.19839-1-jiada_wang@mentor.com>
From: Nick Dyer <nick.dyer@itdev.co.uk>
The workaround of reading all messages until an invalid is received is a
way of forcing the CHG line high, which means that when using
edge-triggered interrupts the interrupt can be acquired.
With level-triggered interrupts the workaround is unnecessary.
Also, most recent maXTouch chips have a feature called RETRIGEN which, when
enabled, reasserts the interrupt line every cycle if there are messages
waiting. This also makes the workaround unnecessary.
Note: the RETRIGEN feature is only in some firmware versions/chips, it's
not valid simply to enable the bit.
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
(cherry picked from ndyer/linux/for-upstream commit 1ae4e8281e491b22442cd5acdfca1862555f8ecb)
[gdavis: Fix conflicts due to v4.6-rc7 commit eb43335c4095 ("Input:
atmel_mxt_ts - use mxt_acquire_irq in mxt_soft_reset").]
Signed-off-by: George G. Davis <george_davis@mentor.com>
[jiada: reset use_retrigen_workaround at beginning of mxt_check_retrigen()]
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 51 ++++++++++++++++++++++--
1 file changed, 48 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 17263c260124..35cbe60094ab 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -20,6 +20,7 @@
#include <linux/i2c.h>
#include <linux/input/mt.h>
#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/of.h>
#include <linux/property.h>
#include <linux/slab.h>
@@ -129,6 +130,7 @@ struct t9_range {
/* MXT_SPT_COMMSCONFIG_T18 */
#define MXT_COMMS_CTRL 0
#define MXT_COMMS_CMD 1
+#define MXT_COMMS_RETRIGEN BIT(6)
/* MXT_DEBUG_DIAGNOSTIC_T37 */
#define MXT_DIAGNOSTIC_PAGEUP 0x01
@@ -308,6 +310,7 @@ struct mxt_data {
struct t7_config t7_cfg;
struct mxt_dbg dbg;
struct gpio_desc *reset_gpio;
+ bool use_retrigen_workaround;
/* Cached parameters from object table */
u16 T5_address;
@@ -318,6 +321,7 @@ struct mxt_data {
u16 T71_address;
u8 T9_reportid_min;
u8 T9_reportid_max;
+ u16 T18_address;
u8 T19_reportid;
u16 T44_address;
u8 T100_reportid_min;
@@ -1190,9 +1194,11 @@ static int mxt_acquire_irq(struct mxt_data *data)
enable_irq(data->irq);
- error = mxt_process_messages_until_invalid(data);
- if (error)
- return error;
+ if (data->use_retrigen_workaround) {
+ error = mxt_process_messages_until_invalid(data);
+ if (error)
+ return error;
+ }
return 0;
}
@@ -1282,6 +1288,33 @@ static u32 mxt_calculate_crc(u8 *base, off_t start_off, off_t end_off)
return crc;
}
+static int mxt_check_retrigen(struct mxt_data *data)
+{
+ struct i2c_client *client = data->client;
+ int error;
+ int val;
+
+ data->use_retrigen_workaround = false;
+
+ if (irq_get_trigger_type(data->irq) & IRQF_TRIGGER_LOW)
+ return 0;
+
+ if (data->T18_address) {
+ error = __mxt_read_reg(client,
+ data->T18_address + MXT_COMMS_CTRL,
+ 1, &val);
+ if (error)
+ return error;
+
+ if (val & MXT_COMMS_RETRIGEN)
+ return 0;
+ }
+
+ dev_warn(&client->dev, "Enabling RETRIGEN workaround\n");
+ data->use_retrigen_workaround = true;
+ return 0;
+}
+
static int mxt_prepare_cfg_mem(struct mxt_data *data, struct mxt_cfg *cfg)
{
struct device *dev = &data->client->dev;
@@ -1561,6 +1594,10 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
mxt_update_crc(data, MXT_COMMAND_BACKUPNV, MXT_BACKUP_VALUE);
+ ret = mxt_check_retrigen(data);
+ if (ret)
+ goto release_mem;
+
ret = mxt_soft_reset(data);
if (ret)
goto release_mem;
@@ -1604,6 +1641,7 @@ static void mxt_free_object_table(struct mxt_data *data)
data->T71_address = 0;
data->T9_reportid_min = 0;
data->T9_reportid_max = 0;
+ data->T18_address = 0;
data->T19_reportid = 0;
data->T44_address = 0;
data->T100_reportid_min = 0;
@@ -1678,6 +1716,9 @@ static int mxt_parse_object_table(struct mxt_data *data,
object->num_report_ids - 1;
data->num_touchids = object->num_report_ids;
break;
+ case MXT_SPT_COMMSCONFIG_T18:
+ data->T18_address = object->start_address;
+ break;
case MXT_SPT_MESSAGECOUNT_T44:
data->T44_address = object->start_address;
break;
@@ -2141,6 +2182,10 @@ static int mxt_initialize(struct mxt_data *data)
msleep(MXT_FW_RESET_TIME);
}
+ error = mxt_check_retrigen(data);
+ if (error)
+ return error;
+
error = mxt_acquire_irq(data);
if (error)
return error;
--
2.19.2
^ permalink raw reply related
* [PATCH v2 03/49] Input: atmel_mxt_ts - rework sysfs init/remove
From: Jiada Wang @ 2019-08-27 6:25 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062607.19839-1-jiada_wang@mentor.com>
From: Nick Dyer <nick.dyer@itdev.co.uk>
An error in the sysfs init may otherwise interfere with the async return
from the firmware loader
Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
(cherry picked from ndyer/linux/for-upstream commit 3114584ae77c2b03b6dad87174f010d002e9c05d)
[gdavis: Forward port and fixup conflicts. Also fixed sysfs leaks in
both the mxt_initialize() and mxt_probe() error return cases.]
Signed-off-by: George G. Davis <george_davis@mentor.com>
[jiada: keep call mxt_initialize() before sysfs creation]
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 64 +++++++++++++++++++-----
1 file changed, 52 insertions(+), 12 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 573b94a049b2..17263c260124 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -2086,10 +2086,14 @@ static int mxt_initialize_input_device(struct mxt_data *data)
return 0;
err_free_mem:
+ data->input_dev = NULL;
input_free_device(input_dev);
return error;
}
+static int mxt_sysfs_init(struct mxt_data *data);
+static void mxt_sysfs_remove(struct mxt_data *data);
+
static int mxt_configure_objects(struct mxt_data *data,
const struct firmware *cfg);
@@ -2141,16 +2145,24 @@ static int mxt_initialize(struct mxt_data *data)
if (error)
return error;
+ error = mxt_sysfs_init(data);
+ if (error)
+ return error;
+
error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
&client->dev, GFP_KERNEL, data,
mxt_config_cb);
if (error) {
dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
error);
- return error;
+ goto err_free_sysfs;
}
return 0;
+
+err_free_sysfs:
+ mxt_sysfs_remove(data);
+ return error;
}
static int mxt_set_t7_power_cfg(struct mxt_data *data, u8 sleep)
@@ -2803,6 +2815,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
if (ret)
goto release_firmware;
+ mxt_sysfs_remove(data);
mxt_free_input_device(data);
mxt_free_object_table(data);
} else {
@@ -2909,16 +2922,25 @@ static ssize_t mxt_update_fw_store(struct device *dev,
return count;
}
+static DEVICE_ATTR(update_fw, S_IWUSR, NULL, mxt_update_fw_store);
+
+static struct attribute *mxt_fw_attrs[] = {
+ &dev_attr_update_fw.attr,
+ NULL
+};
+
+static const struct attribute_group mxt_fw_attr_group = {
+ .attrs = mxt_fw_attrs,
+};
+
static DEVICE_ATTR(fw_version, S_IRUGO, mxt_fw_version_show, NULL);
static DEVICE_ATTR(hw_version, S_IRUGO, mxt_hw_version_show, NULL);
static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL);
-static DEVICE_ATTR(update_fw, S_IWUSR, NULL, mxt_update_fw_store);
static struct attribute *mxt_attrs[] = {
&dev_attr_fw_version.attr,
&dev_attr_hw_version.attr,
&dev_attr_object.attr,
- &dev_attr_update_fw.attr,
NULL
};
@@ -2926,6 +2948,28 @@ static const struct attribute_group mxt_attr_group = {
.attrs = mxt_attrs,
};
+static int mxt_sysfs_init(struct mxt_data *data)
+{
+ struct i2c_client *client = data->client;
+ int error;
+
+ error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
+ if (error) {
+ dev_err(&client->dev, "Failure %d creating sysfs group\n",
+ error);
+ return error;
+ }
+
+ return 0;
+}
+
+static void mxt_sysfs_remove(struct mxt_data *data)
+{
+ struct i2c_client *client = data->client;
+
+ sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
+}
+
static void mxt_start(struct mxt_data *data)
{
switch (data->suspend_mode) {
@@ -3113,19 +3157,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (error)
return error;
- error = sysfs_create_group(&client->dev.kobj, &mxt_attr_group);
+ error = sysfs_create_group(&client->dev.kobj, &mxt_fw_attr_group);
if (error) {
- dev_err(&client->dev, "Failure %d creating sysfs group\n",
+ dev_err(&client->dev, "Failure %d creating fw sysfs group\n",
error);
- goto err_free_object;
+ return error;
}
return 0;
-
-err_free_object:
- mxt_free_input_device(data);
- mxt_free_object_table(data);
- return error;
}
static int mxt_remove(struct i2c_client *client)
@@ -3133,7 +3172,8 @@ static int mxt_remove(struct i2c_client *client)
struct mxt_data *data = i2c_get_clientdata(client);
disable_irq(data->irq);
- sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
+ sysfs_remove_group(&client->dev.kobj, &mxt_fw_attr_group);
+ mxt_sysfs_remove(data);
mxt_free_input_device(data);
mxt_free_object_table(data);
--
2.19.2
^ permalink raw reply related
* [PATCH v2 02/49] Input: introduce input_mt_report_slot_inactive
From: Jiada Wang @ 2019-08-27 6:25 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062607.19839-1-jiada_wang@mentor.com>
input_mt_report_slot_state() ignores the tool when the slot is closed.
which has caused a bit of confusion.
This patch introduces input_mt_report_slot_inactive() to report slot
inactive state.
replaces all input_mt_report_slot_state() with
input_mt_report_slot_inactive() in case of close of slot.
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/hid/hid-alps.c | 3 +--
drivers/hid/hid-multitouch.c | 6 ++----
drivers/input/input-mt.c | 2 +-
drivers/input/misc/xen-kbdfront.c | 2 +-
drivers/input/mouse/elan_i2c_core.c | 2 +-
drivers/input/touchscreen/atmel_mxt_ts.c | 7 +++----
drivers/input/touchscreen/cyttsp4_core.c | 5 ++---
drivers/input/touchscreen/cyttsp_core.c | 2 +-
drivers/input/touchscreen/melfas_mip4.c | 4 ++--
drivers/input/touchscreen/mms114.c | 2 +-
drivers/input/touchscreen/raspberrypi-ts.c | 2 +-
drivers/input/touchscreen/stmfts.c | 2 +-
include/linux/input/mt.h | 5 +++++
13 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index ae79a7c66737..36ca1d815d53 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -387,8 +387,7 @@ static int u1_raw_event(struct alps_dev *hdata, u8 *data, int size)
input_report_abs(hdata->input,
ABS_MT_PRESSURE, z);
} else {
- input_mt_report_slot_state(hdata->input,
- MT_TOOL_FINGER, 0);
+ input_mt_report_slot_inactive(hdata->input);
}
}
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 672396ed413e..b2221f2030c1 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -880,7 +880,7 @@ static void mt_release_pending_palms(struct mt_device *td,
clear_bit(slotnum, app->pending_palm_slots);
input_mt_slot(input, slotnum);
- input_mt_report_slot_state(input, MT_TOOL_PALM, false);
+ input_mt_report_slot_inactive(input);
need_sync = true;
}
@@ -1620,9 +1620,7 @@ static void mt_release_contacts(struct hid_device *hid)
if (mt) {
for (i = 0; i < mt->num_slots; i++) {
input_mt_slot(input_dev, i);
- input_mt_report_slot_state(input_dev,
- MT_TOOL_FINGER,
- false);
+ input_mt_report_slot_inactive(input_dev);
}
input_mt_sync_frame(input_dev);
input_sync(input_dev);
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index a81e14148407..7626fe5bfe44 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -145,7 +145,7 @@ bool input_mt_report_slot_state(struct input_dev *dev,
slot->frame = mt->frame;
if (!active) {
- input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
+ input_mt_report_slot_inactive(dev);
return false;
}
diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index 24bc5c5d876f..a1bba722b234 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -146,7 +146,7 @@ static void xenkbd_handle_mt_event(struct xenkbd_info *info,
break;
case XENKBD_MT_EV_UP:
- input_mt_report_slot_state(info->mtouch, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(info->mtouch);
break;
case XENKBD_MT_EV_SYN:
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index d9b103a81a79..b72358d4b35c 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -938,7 +938,7 @@ static void elan_report_contact(struct elan_tp_data *data,
input_report_abs(input, ABS_MT_TOUCH_MINOR, minor);
} else {
input_mt_slot(input, contact_num);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(input);
}
}
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 4a5f482cf1af..573b94a049b2 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -822,8 +822,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
* have happened.
*/
if (status & MXT_T9_RELEASE) {
- input_mt_report_slot_state(input_dev,
- MT_TOOL_FINGER, 0);
+ input_mt_report_slot_inactive(input_dev);
mxt_input_sync(data);
}
@@ -839,7 +838,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
} else {
/* Touch no longer active, close out slot */
- input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
+ input_mt_report_slot_inactive(input_dev);
}
data->update_input = true;
@@ -947,7 +946,7 @@ static void mxt_proc_t100_message(struct mxt_data *data, u8 *message)
dev_dbg(dev, "[%u] release\n", id);
/* close out slot */
- input_mt_report_slot_state(input_dev, 0, 0);
+ input_mt_report_slot_inactive(input_dev);
}
data->update_input = true;
diff --git a/drivers/input/touchscreen/cyttsp4_core.c b/drivers/input/touchscreen/cyttsp4_core.c
index 4b22d49a0f49..a3a85e2348a2 100644
--- a/drivers/input/touchscreen/cyttsp4_core.c
+++ b/drivers/input/touchscreen/cyttsp4_core.c
@@ -744,8 +744,7 @@ static void cyttsp4_report_slot_liftoff(struct cyttsp4_mt_data *md,
for (t = 0; t < max_slots; t++) {
input_mt_slot(md->input, t);
- input_mt_report_slot_state(md->input,
- MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(md->input);
}
}
@@ -845,7 +844,7 @@ static void cyttsp4_final_sync(struct input_dev *input, int max_slots, int *ids)
if (ids[t])
continue;
input_mt_slot(input, t);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(input);
}
input_sync(input);
diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index 3f5d463dbeed..697aa2c158f7 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -340,7 +340,7 @@ static void cyttsp_report_tchdata(struct cyttsp *ts)
continue;
input_mt_slot(input, i);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(input);
}
input_sync(input);
diff --git a/drivers/input/touchscreen/melfas_mip4.c b/drivers/input/touchscreen/melfas_mip4.c
index 247c3aaba2d8..f67efdd040b2 100644
--- a/drivers/input/touchscreen/melfas_mip4.c
+++ b/drivers/input/touchscreen/melfas_mip4.c
@@ -391,7 +391,7 @@ static void mip4_clear_input(struct mip4_ts *ts)
/* Screen */
for (i = 0; i < MIP4_MAX_FINGERS; i++) {
input_mt_slot(ts->input, i);
- input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, 0);
+ input_mt_report_slot_inactive(ts->input);
}
/* Keys */
@@ -534,7 +534,7 @@ static void mip4_report_touch(struct mip4_ts *ts, u8 *packet)
} else {
/* Release event */
input_mt_slot(ts->input, id);
- input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, 0);
+ input_mt_report_slot_inactive(ts->input);
}
input_mt_sync_frame(ts->input);
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index ea46da04e517..d4e088364bed 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -550,7 +550,7 @@ static int __maybe_unused mms114_suspend(struct device *dev)
/* Release all touch */
for (id = 0; id < MMS114_MAX_TOUCH; id++) {
input_mt_slot(input_dev, id);
- input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(input_dev);
}
input_mt_report_pointer_emulation(input_dev, true);
diff --git a/drivers/input/touchscreen/raspberrypi-ts.c b/drivers/input/touchscreen/raspberrypi-ts.c
index 69881265d121..147ea4f8f87b 100644
--- a/drivers/input/touchscreen/raspberrypi-ts.c
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -102,7 +102,7 @@ static void rpi_ts_poll(struct input_polled_dev *dev)
released_ids = ts->known_ids & ~modified_ids;
for_each_set_bit(i, &released_ids, RPI_TS_MAX_SUPPORTED_POINTS) {
input_mt_slot(input, i);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, 0);
+ input_mt_report_slot_inactive(input);
modified_ids &= ~(BIT(i));
}
ts->known_ids = modified_ids;
diff --git a/drivers/input/touchscreen/stmfts.c b/drivers/input/touchscreen/stmfts.c
index b6f95f20f924..b54cc64e4ea6 100644
--- a/drivers/input/touchscreen/stmfts.c
+++ b/drivers/input/touchscreen/stmfts.c
@@ -198,7 +198,7 @@ static void stmfts_report_contact_release(struct stmfts_data *sdata,
u8 slot_id = (event[0] & STMFTS_MASK_TOUCH_ID) >> 4;
input_mt_slot(sdata->input, slot_id);
- input_mt_report_slot_state(sdata->input, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(sdata->input);
input_sync(sdata->input);
}
diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
index 9e409bb13642..ab9a34c312f9 100644
--- a/include/linux/input/mt.h
+++ b/include/linux/input/mt.h
@@ -97,6 +97,11 @@ static inline bool input_is_mt_axis(int axis)
return axis == ABS_MT_SLOT || input_is_mt_value(axis);
}
+static inline void input_mt_report_slot_inactive(struct input_dev *dev)
+{
+ input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
+
+}
bool input_mt_report_slot_state(struct input_dev *dev,
unsigned int tool_type, bool active);
--
2.19.2
^ permalink raw reply related
* [PATCH v2 01/49] Input: switch to use return value of input_mt_report_slot_state
From: Jiada Wang @ 2019-08-27 6:25 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
In-Reply-To: <20190827062607.19839-1-jiada_wang@mentor.com>
Currently several places are calling input_mt_report_slot_state()
like following:
input_mt_report_slot_state(dev, tool_type, active)
if (active) {
...
< report events for active slot >
}
when 'active' is false, input_mt_report_slot_state() will always
return false, so instead of checking 'active', switch to use
return value of input_mt_report_slot_state()
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
drivers/hid/hid-asus.c | 3 +--
drivers/hid/hid-elan.c | 3 +--
drivers/hid/hid-logitech-hidpp.c | 5 ++---
drivers/hid/hid-magicmouse.c | 3 +--
drivers/hid/hid-multitouch.c | 3 +--
drivers/hid/hid-sony.c | 8 ++++----
drivers/hid/wacom_wac.c | 15 +++++----------
drivers/input/mouse/elantech.c | 5 ++---
drivers/input/mouse/focaltech.c | 3 +--
drivers/input/mouse/sentelic.c | 3 +--
drivers/input/mouse/synaptics.c | 3 +--
drivers/input/rmi4/rmi_2d_sensor.c | 6 ++----
drivers/input/touchscreen/chipone_icn8505.c | 4 ++--
drivers/input/touchscreen/egalax_ts.c | 3 +--
drivers/input/touchscreen/hideep.c | 7 +++----
drivers/input/touchscreen/ili210x.c | 3 +--
drivers/input/touchscreen/mms114.c | 4 ++--
drivers/input/touchscreen/penmount.c | 5 ++---
drivers/input/touchscreen/raydium_i2c_ts.c | 4 ++--
drivers/input/touchscreen/sis_i2c.c | 5 ++---
drivers/input/touchscreen/surface3_spi.c | 4 ++--
drivers/input/touchscreen/wacom_w8001.c | 3 +--
drivers/input/touchscreen/zforce_ts.c | 6 ++----
23 files changed, 42 insertions(+), 66 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 8063b1d567b1..55f1004f0e58 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -240,9 +240,8 @@ static int asus_report_input(struct asus_drvdata *drvdat, u8 *data, int size)
MT_TOOL_PALM : MT_TOOL_FINGER;
input_mt_slot(drvdat->input, i);
- input_mt_report_slot_state(drvdat->input, toolType, down);
- if (down) {
+ if (input_mt_report_slot_state(drvdat->input, toolType, down)) {
asus_report_contact_down(drvdat, toolType, contactData);
contactData += drvdat->tp->contact_size;
}
diff --git a/drivers/hid/hid-elan.c b/drivers/hid/hid-elan.c
index 45c4f888b7c4..84a317c6feb6 100644
--- a/drivers/hid/hid-elan.c
+++ b/drivers/hid/hid-elan.c
@@ -216,8 +216,7 @@ static void elan_report_mt_slot(struct elan_drvdata *drvdata, u8 *data,
bool active = !!data;
input_mt_slot(input, slot_num);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
- if (active) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, active)) {
x = ((data[0] & 0xF0) << 4) | data[1];
y = drvdata->max_y -
(((data[0] & 0x07) << 8) | data[2]);
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 21268c9fa71a..a7b552a797f1 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2295,9 +2295,8 @@ static void wtp_touch_event(struct hidpp_device *hidpp,
slot = input_mt_get_slot_by_key(hidpp->input, touch_report->finger_id);
input_mt_slot(hidpp->input, slot);
- input_mt_report_slot_state(hidpp->input, MT_TOOL_FINGER,
- touch_report->contact_status);
- if (touch_report->contact_status) {
+ if (input_mt_report_slot_state(hidpp->input, MT_TOOL_FINGER,
+ touch_report->contact_status)) {
input_event(hidpp->input, EV_ABS, ABS_MT_POSITION_X,
touch_report->x);
input_event(hidpp->input, EV_ABS, ABS_MT_POSITION_Y,
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 34138667f8af..5f9f6426d7b8 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -282,10 +282,9 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
msc->ntouches++;
input_mt_slot(input, id);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, down);
/* Generate the input events for this touch. */
- if (down) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, down)) {
input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major << 2);
input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor << 2);
input_report_abs(input, ABS_MT_ORIENTATION, -orientation);
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index b603c14d043b..672396ed413e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1020,8 +1020,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
}
input_mt_slot(input, slotnum);
- input_mt_report_slot_state(input, tool, active);
- if (active) {
+ if (input_mt_report_slot_state(input, tool, active)) {
/* this finger is in proximity of the sensor */
int wide = (*slot->w > *slot->h);
int major = max(*slot->w, *slot->h);
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 49dd2d905c7f..8ba21584a84a 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1092,9 +1092,9 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
active = !(rd[offset] >> 7);
input_mt_slot(sc->touchpad, n);
- input_mt_report_slot_state(sc->touchpad, MT_TOOL_FINGER, active);
- if (active) {
+ if (input_mt_report_slot_state(sc->touchpad,
+ MT_TOOL_FINGER, active)) {
input_report_abs(sc->touchpad, ABS_MT_POSITION_X, x);
input_report_abs(sc->touchpad, ABS_MT_POSITION_Y, y);
}
@@ -1146,9 +1146,9 @@ static void nsg_mrxu_parse_report(struct sony_sc *sc, u8 *rd, int size)
y = ((rd[offset+1] & 0xF0) >> 4) | (rd[offset+2] << 4);
input_mt_slot(sc->touchpad, n);
- input_mt_report_slot_state(sc->touchpad, MT_TOOL_FINGER, active & 0x03);
- if (active & 0x03) {
+ if (input_mt_report_slot_state(sc->touchpad,
+ MT_TOOL_FINGER, active & 0x03)) {
contactx = rd[offset+3] & 0x0F;
contacty = rd[offset+3] >> 4;
input_report_abs(sc->touchpad, ABS_MT_TOUCH_MAJOR,
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 7a8ddc999a8e..c49b261753f0 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1504,9 +1504,8 @@ static int wacom_24hdt_irq(struct wacom_wac *wacom)
if (slot < 0)
continue;
input_mt_slot(input, slot);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, touch)) {
int t_x = get_unaligned_le16(&data[offset + 2]);
int t_y = get_unaligned_le16(&data[offset + 4 + y_offset]);
@@ -1570,8 +1569,7 @@ static int wacom_mt_touch(struct wacom_wac *wacom)
continue;
input_mt_slot(input, slot);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, touch)) {
int x = get_unaligned_le16(&data[offset + x_offset + 7]);
int y = get_unaligned_le16(&data[offset + x_offset + 9]);
input_report_abs(input, ABS_MT_POSITION_X, x);
@@ -1599,8 +1597,7 @@ static int wacom_tpc_mt_touch(struct wacom_wac *wacom)
bool touch = p && report_touch_events(wacom);
input_mt_slot(input, i);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, touch)) {
int x = le16_to_cpup((__le16 *)&data[i * 2 + 2]) & 0x7fff;
int y = le16_to_cpup((__le16 *)&data[i * 2 + 6]) & 0x7fff;
@@ -2842,8 +2839,7 @@ static int wacom_bpt_touch(struct wacom_wac *wacom)
&& (data[offset + 3] & 0x80);
input_mt_slot(input, i);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, touch)) {
int x = get_unaligned_be16(&data[offset + 3]) & 0x7ff;
int y = get_unaligned_be16(&data[offset + 5]) & 0x7ff;
if (features->quirks & WACOM_QUIRK_BBTOUCH_LOWRES) {
@@ -2879,9 +2875,8 @@ static void wacom_bpt3_touch_msg(struct wacom_wac *wacom, unsigned char *data)
touch = touch && report_touch_events(wacom);
input_mt_slot(input, slot);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (touch) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER, touch)) {
int x = (data[2] << 4) | (data[4] >> 4);
int y = (data[3] << 4) | (data[4] & 0x0f);
int width, height;
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index 2d8434b7b623..da06490bb914 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -343,8 +343,7 @@ static void elantech_set_slot(struct input_dev *dev, int slot, bool active,
unsigned int x, unsigned int y)
{
input_mt_slot(dev, slot);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
- if (active) {
+ if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, active)) {
input_report_abs(dev, ABS_MT_POSITION_X, x);
input_report_abs(dev, ABS_MT_POSITION_Y, y);
}
@@ -608,7 +607,7 @@ static void process_packet_status_v4(struct psmouse *psmouse)
for (i = 0; i < ETP_MAX_FINGERS; i++) {
if ((fingers & (1 << i)) == 0) {
input_mt_slot(dev, i);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, false);
+ input_mt_report_slot_inactive(dev);
}
}
diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index 6fd5fff0cbff..fd627fab16c0 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -124,8 +124,7 @@ static void focaltech_report_state(struct psmouse *psmouse)
bool active = finger->active && finger->valid;
input_mt_slot(dev, i);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
- if (active) {
+ if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, active)) {
unsigned int clamped_x, clamped_y;
/*
* The touchpad might report invalid data, so we clamp
diff --git a/drivers/input/mouse/sentelic.c b/drivers/input/mouse/sentelic.c
index e99d9bf1a267..1a8a2e19c514 100644
--- a/drivers/input/mouse/sentelic.c
+++ b/drivers/input/mouse/sentelic.c
@@ -683,8 +683,7 @@ static void fsp_set_slot(struct input_dev *dev, int slot, bool active,
unsigned int x, unsigned int y)
{
input_mt_slot(dev, slot);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
- if (active) {
+ if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, active)) {
input_report_abs(dev, ABS_MT_POSITION_X, x);
input_report_abs(dev, ABS_MT_POSITION_Y, y);
}
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index b1956ed4c0dd..6969b37ba731 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -882,8 +882,7 @@ static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
bool active, int x, int y)
{
input_mt_slot(dev, slot);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
- if (active) {
+ if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, active)) {
input_report_abs(dev, ABS_MT_POSITION_X, x);
input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(y));
}
diff --git a/drivers/input/rmi4/rmi_2d_sensor.c b/drivers/input/rmi4/rmi_2d_sensor.c
index ea549efe4bc4..c0f7be7c3737 100644
--- a/drivers/input/rmi4/rmi_2d_sensor.c
+++ b/drivers/input/rmi4/rmi_2d_sensor.c
@@ -76,10 +76,8 @@ void rmi_2d_sensor_abs_report(struct rmi_2d_sensor *sensor,
else
input_mt_slot(input, slot);
- input_mt_report_slot_state(input, obj->mt_tool,
- obj->type != RMI_2D_OBJECT_NONE);
-
- if (obj->type != RMI_2D_OBJECT_NONE) {
+ if (input_mt_report_slot_state(input, obj->mt_tool,
+ obj->type != RMI_2D_OBJECT_NONE)) {
obj->x = sensor->tracking_pos[slot].x;
obj->y = sensor->tracking_pos[slot].y;
diff --git a/drivers/input/touchscreen/chipone_icn8505.c b/drivers/input/touchscreen/chipone_icn8505.c
index c768186ce856..9880101cd14b 100644
--- a/drivers/input/touchscreen/chipone_icn8505.c
+++ b/drivers/input/touchscreen/chipone_icn8505.c
@@ -344,8 +344,8 @@ static irqreturn_t icn8505_irq(int irq, void *dev_id)
bool act = icn8505_touch_active(touch->event);
input_mt_slot(icn8505->input, touch->slot);
- input_mt_report_slot_state(icn8505->input, MT_TOOL_FINGER, act);
- if (!act)
+ if (!input_mt_report_slot_state(icn8505->input,
+ MT_TOOL_FINGER, act))
continue;
touchscreen_report_pos(icn8505->input, &icn8505->prop,
diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
index 83ac8c128192..c0774c29e496 100644
--- a/drivers/input/touchscreen/egalax_ts.c
+++ b/drivers/input/touchscreen/egalax_ts.c
@@ -99,12 +99,11 @@ static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id)
}
input_mt_slot(input_dev, id);
- input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, down);
dev_dbg(&client->dev, "%s id:%d x:%d y:%d z:%d",
down ? "down" : "up", id, x, y, z);
- if (down) {
+ if (input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, down)) {
input_report_abs(input_dev, ABS_MT_POSITION_X, x);
input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
input_report_abs(input_dev, ABS_MT_PRESSURE, z);
diff --git a/drivers/input/touchscreen/hideep.c b/drivers/input/touchscreen/hideep.c
index 84fbbf415c43..6f4c96f45a02 100644
--- a/drivers/input/touchscreen/hideep.c
+++ b/drivers/input/touchscreen/hideep.c
@@ -693,10 +693,9 @@ static void hideep_report_slot(struct input_dev *input,
const struct hideep_event *event)
{
input_mt_slot(input, event->index & 0x0f);
- input_mt_report_slot_state(input,
- __GET_MT_TOOL_TYPE(event->type),
- !(event->flag & HIDEEP_MT_RELEASED));
- if (!(event->flag & HIDEEP_MT_RELEASED)) {
+ if (input_mt_report_slot_state(input,
+ __GET_MT_TOOL_TYPE(event->type),
+ !(event->flag & HIDEEP_MT_RELEASED))) {
input_report_abs(input, ABS_MT_POSITION_X,
le16_to_cpup(&event->x));
input_report_abs(input, ABS_MT_POSITION_Y,
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index e9006407c9bc..2a5698e129d8 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -154,8 +154,7 @@ static bool ili210x_report_events(struct ili210x *priv, u8 *touchdata)
}
input_mt_slot(input, i);
- input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
- if (!touch)
+ if (!input_mt_report_slot_state(input, MT_TOOL_FINGER, touch))
continue;
touchscreen_report_pos(input, &priv->prop, x, y,
true);
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index a5ab774da4cc..ea46da04e517 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -178,9 +178,9 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
x, y, touch->width, touch->strength);
input_mt_slot(input_dev, id);
- input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, touch->pressed);
- if (touch->pressed) {
+ if (input_mt_report_slot_state(input_dev,
+ MT_TOOL_FINGER, touch->pressed)) {
touchscreen_report_pos(input_dev, &data->props, x, y, true);
input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, touch->width);
input_report_abs(input_dev, ABS_MT_PRESSURE, touch->strength);
diff --git a/drivers/input/touchscreen/penmount.c b/drivers/input/touchscreen/penmount.c
index 12abb3b36128..2721a608d7fc 100644
--- a/drivers/input/touchscreen/penmount.c
+++ b/drivers/input/touchscreen/penmount.c
@@ -69,9 +69,8 @@ static void pm_mtevent(struct pm *pm, struct input_dev *input)
for (i = 0; i < pm->maxcontacts; ++i) {
input_mt_slot(input, i);
- input_mt_report_slot_state(input, MT_TOOL_FINGER,
- pm->slots[i].active);
- if (pm->slots[i].active) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER,
+ pm->slots[i].active)) {
input_event(input, EV_ABS, ABS_MT_POSITION_X, pm->slots[i].x);
input_event(input, EV_ABS, ABS_MT_POSITION_Y, pm->slots[i].y);
}
diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
index 6ed9f22e6401..d9f253a82eba 100644
--- a/drivers/input/touchscreen/raydium_i2c_ts.c
+++ b/drivers/input/touchscreen/raydium_i2c_ts.c
@@ -801,9 +801,9 @@ static void raydium_mt_event(struct raydium_data *ts)
u8 wx, wy;
input_mt_slot(ts->input, i);
- input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, state);
- if (!state)
+ if (!input_mt_report_slot_state(ts->input,
+ MT_TOOL_FINGER, state))
continue;
input_report_abs(ts->input, ABS_MT_POSITION_X,
diff --git a/drivers/input/touchscreen/sis_i2c.c b/drivers/input/touchscreen/sis_i2c.c
index 6274555f1673..55857328bbc7 100644
--- a/drivers/input/touchscreen/sis_i2c.c
+++ b/drivers/input/touchscreen/sis_i2c.c
@@ -189,10 +189,9 @@ static int sis_ts_report_contact(struct sis_ts_data *ts, const u8 *data, u8 id)
return -ENOENT;
input_mt_slot(input, slot);
- input_mt_report_slot_state(input, MT_TOOL_FINGER,
- status == SIS_STATUS_DOWN);
- if (status == SIS_STATUS_DOWN) {
+ if (input_mt_report_slot_state(input, MT_TOOL_FINGER,
+ status == SIS_STATUS_DOWN)) {
pressure = height = width = 1;
if (id != SIS_ALL_IN_ONE_PACKAGE) {
if (SIS_PKT_HAS_AREA(id)) {
diff --git a/drivers/input/touchscreen/surface3_spi.c b/drivers/input/touchscreen/surface3_spi.c
index ce4828b1415a..6981790dd541 100644
--- a/drivers/input/touchscreen/surface3_spi.c
+++ b/drivers/input/touchscreen/surface3_spi.c
@@ -75,8 +75,8 @@ static void surface3_spi_report_touch(struct surface3_ts_data *ts_data,
return;
input_mt_slot(ts_data->input_dev, slot);
- input_mt_report_slot_state(ts_data->input_dev, MT_TOOL_FINGER, st);
- if (st) {
+ if (input_mt_report_slot_state(ts_data->input_dev,
+ MT_TOOL_FINGER, st)) {
input_report_abs(ts_data->input_dev,
ABS_MT_POSITION_X,
get_unaligned_le16(&finger->x));
diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 3715d1eace92..cf2f233cdbfb 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -155,8 +155,7 @@ static void parse_multi_touch(struct w8001 *w8001)
bool touch = data[0] & (1 << i);
input_mt_slot(dev, i);
- input_mt_report_slot_state(dev, MT_TOOL_FINGER, touch);
- if (touch) {
+ if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, touch)) {
x = (data[6 * i + 1] << 7) | data[6 * i + 2];
y = (data[6 * i + 3] << 7) | data[6 * i + 4];
/* data[5,6] and [11,12] is finger capacity */
diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index 5230519b0f74..37ae4599c0db 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c
@@ -386,10 +386,8 @@ static int zforce_touch_event(struct zforce_ts *ts, u8 *payload)
/* the zforce id starts with "1", so needs to be decreased */
input_mt_slot(ts->input, point.id - 1);
- input_mt_report_slot_state(ts->input, MT_TOOL_FINGER,
- point.state != STATE_UP);
-
- if (point.state != STATE_UP) {
+ if (input_mt_report_slot_state(ts->input, MT_TOOL_FINGER,
+ point.state != STATE_UP)) {
input_report_abs(ts->input, ABS_MT_POSITION_X,
point.coord_x);
input_report_abs(ts->input, ABS_MT_POSITION_Y,
--
2.19.2
^ permalink raw reply related
* [PATCH v2 00/49] atmel_mxt_ts misc
From: Jiada Wang @ 2019-08-27 6:25 UTC (permalink / raw)
To: nick, dmitry.torokhov; +Cc: linux-input, linux-kernel, jiada_wang, george_davis
This patch-set forward ports Nick Dyer's work in ndyer/linux github repository
as long as some other features and fixes
Balasubramani Vivekanandan (2):
Input: atmel_mxt_ts: Limit the max bytes transferred in an i2c
transaction
Input: atmel_mxt_ts: use gpiod_set_value_cansleep for reset pin
Dean Jenkins (1):
Input: atmel_mxt_ts: return error from
mxt_process_messages_until_invalid()
Deepak Das (6):
Input: Atmel: improve error handling in mxt_start()
Input: Atmel: improve error handling in mxt_initialize()
Input: Atmel: improve error handling in mxt_update_cfg()
Input: Atmel: Improve error handling in mxt_initialize_input_device()
Input: Atmel: handle ReportID "0x00" while processing T5 messages
Input: Atmel: use T44 object to process T5 messages
George G. Davis (1):
input: atmel_mxt_ts: export GPIO reset line via sysfs
Jiada Wang (4):
Input: switch to use return value of input_mt_report_slot_state
Input: introduce input_mt_report_slot_inactive
Input: atmel_mxt_ts - eliminate data->raw_info_block
Input: atmel_mxt_ts - Fix compilation warning
Karl Tsou (1):
Input: atmel_mxt_ts - add debug for T92 gesture and T93 touch seq msgs
Kautuk Consul (2):
Input: atmel_mxt_ts - Change call-points of mxt_free_* functions
Input: atmel_mxt_ts - rely on calculated_crc rather than file
config_crc
Naveen Chakka (2):
input: touchscreen: atmel_mxt_ts: Added sysfs entry for touchscreen
status
input: atmel_mxt_ts: added sysfs interface to update atmel T38 data
Nick Dyer (26):
Input: atmel_mxt_ts - rework sysfs init/remove
Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when
necessary
Input: atmel_mxt_ts - split large i2c transfers into blocks
Input: atmel_mxt_ts - output status from T48 Noise Supression
Input: atmel_mxt_ts - output status from T42 Touch Suppression
Input: atmel_mxt_ts - implement T9 vector/orientation support
Input: atmel_mxt_ts - implement T15 Key Array support
Input: atmel_mxt_ts - handle reports from T47 Stylus object
Input: atmel_mxt_ts - implement support for T107 active stylus
Input: atmel_mxt_ts - release touch state during suspend
Input: atmel_mxt_ts - add regulator control support
Input: atmel_mxt_ts - report failures in suspend/resume
Input: atmel_mxt_ts - allow specification of firmware file name
Input: atmel_mxt_ts - handle cfg filename via pdata/sysfs
Input: atmel_mxt_ts - allow input name to be specified in platform
data
Input: atmel_mxt_ts - refactor firmware flash to extract context into
struct
Input: atmel_mxt_ts - refactor code to enter bootloader into separate
func
Input: atmel_mxt_ts - combine bootloader version query with probe
Input: atmel_mxt_ts - improve bootloader state machine handling
Input: atmel_mxt_ts - rename bl_completion to chg_completion
Input: atmel_mxt_ts - make bootloader interrupt driven
Input: atmel_mxt_ts - delay enabling IRQ when not using regulators
Input: atmel_mxt_ts - implement I2C retries
Input: atmel_mxt_ts - orientation is not present in hover
Input: atmel_mxt_ts - implement debug output for messages
Input: atmel_mxt_ts - implement improved debug message interface
Nikhil Ravindran (1):
Input: atmel_mxt_ts: Add support for run self-test routine.
Sanjeev Chugh (1):
Input: atmel_mxt_ts: Implement synchronization during various
operation
karl tsou (1):
Input: atmel_mxt_ts - add config checksum attribute to sysfs
keerthikumarp (1):
input: atmel_mxt_ts: Add Missing Delay for reset handling of Atmel
touch panel controller in detachable displays.
---
v2:
Following commit in v1 patchset has been split into two commits
Input: introduce input_mt_report_slot_inactive
Following commits have been updated compared to v1 patchset
Input: atmel_mxt_ts - split large i2c transfers into blocks
Input: atmel_mxt_ts - output status from T42 Touch Suppression
Following commits in v1 patchset have been squashed
Input: touchscreen: Atmel: Add device tree support for T15 key array objects
Input: atmel_mxt_ts - check data->input_dev is not null in mxt_input_sync()
Input: atmel_mxt_ts - check firmware format before entering bootloader
Input: atmel_mxt_ts: update stale use_retrigen_workaround flag
input: atmel_mxt_ts: move bootloader probe from mxt_initialize()
input: Atmel: limit the max bytes transferred while reading T5 messages
Input: atmel_mxt_ts: Use msecs_to_jiffies() instead of HZ
Input: atmel_mxt_ts: Use complete when in_bootloader true
Input: atmel_mxt_ts: Prevent crash due to freeing of input device
input: atmel_mxt_ts: Add NULL check for sysfs attribute debug_msg_attr
Following commits in v1 patchset have been dropped:
Input: atmel_mxt_ts - configure and use gpios as real gpios
Input: touchscreen: Atmel: Enable IRQ_DISABLE_UNLAZY flag for interrupt
Input: atmel_mxt_ts - add memory access interface via sysfs
Input: atmel_mxt_ts: Remove sysfs attributes during driver detach
Input: atmel_mxt_ts: Avoid race condition in freeing of input device
v1: initial version
---
.../bindings/input/atmel,maxtouch.txt | 14 +
MAINTAINERS | 1 +
drivers/hid/hid-alps.c | 3 +-
drivers/hid/hid-asus.c | 3 +-
drivers/hid/hid-elan.c | 3 +-
drivers/hid/hid-logitech-hidpp.c | 5 +-
drivers/hid/hid-magicmouse.c | 3 +-
drivers/hid/hid-multitouch.c | 9 +-
drivers/hid/hid-sony.c | 8 +-
drivers/hid/wacom_wac.c | 15 +-
drivers/input/input-mt.c | 2 +-
drivers/input/misc/xen-kbdfront.c | 2 +-
drivers/input/mouse/elan_i2c_core.c | 2 +-
drivers/input/mouse/elantech.c | 5 +-
drivers/input/mouse/focaltech.c | 3 +-
drivers/input/mouse/sentelic.c | 3 +-
drivers/input/mouse/synaptics.c | 3 +-
drivers/input/rmi4/rmi_2d_sensor.c | 6 +-
drivers/input/touchscreen/atmel_mxt_ts.c | 2269 ++++++++++++++---
drivers/input/touchscreen/chipone_icn8505.c | 4 +-
drivers/input/touchscreen/cyttsp4_core.c | 5 +-
drivers/input/touchscreen/cyttsp_core.c | 2 +-
drivers/input/touchscreen/egalax_ts.c | 3 +-
drivers/input/touchscreen/hideep.c | 7 +-
drivers/input/touchscreen/ili210x.c | 3 +-
drivers/input/touchscreen/melfas_mip4.c | 4 +-
drivers/input/touchscreen/mms114.c | 6 +-
drivers/input/touchscreen/penmount.c | 5 +-
drivers/input/touchscreen/raspberrypi-ts.c | 2 +-
drivers/input/touchscreen/raydium_i2c_ts.c | 4 +-
drivers/input/touchscreen/sis_i2c.c | 5 +-
drivers/input/touchscreen/stmfts.c | 2 +-
drivers/input/touchscreen/surface3_spi.c | 4 +-
drivers/input/touchscreen/wacom_w8001.c | 3 +-
drivers/input/touchscreen/zforce_ts.c | 6 +-
include/dt-bindings/input/atmel_mxt_ts.h | 22 +
include/linux/input/mt.h | 5 +
37 files changed, 2026 insertions(+), 425 deletions(-)
create mode 100644 include/dt-bindings/input/atmel_mxt_ts.h
--
2.19.2
^ permalink raw reply
* RE: [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q
From: Robin Gong @ 2019-08-27 6:17 UTC (permalink / raw)
To: Robin van der Gracht
Cc: Mark Rutland, devicetree@vger.kernel.org, Shawn Guo, Sascha Hauer,
Dmitry Torokhov, linux-kernel@vger.kernel.org, Rob Herring,
dl-linux-imx, Pengutronix Kernel Team,
linux-input@vger.kernel.org, Adam Ford, Fabio Estevam,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190823123002.10448-1-robin@protonic.nl>
On Fri, Aug 23, 2019 at 02:30:02PM +0200, Robin van der Gracht wrote:>
> The older generation i.MX6 processors send a powerdown request interrupt
> if the powerkey is released before a hard shutdown (5 second press). This
> should allow software to bring down the SoC safely.
>
> For this driver to work as a regular powerkey with the older SoCs, we need to
> send a keypress AND release when we get the powerdown request interrupt.
Please clarify here more clearly that because there is NO press interrupt triggered
but only release interrupt on elder i.mx6 processors and that HW issue fixed from
i.mx6sx.
>
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
> arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
> arch/arm/boot/dts/imx6sll.dtsi | 2 +-
> arch/arm/boot/dts/imx6sx.dtsi | 2 +-
> arch/arm/boot/dts/imx6ul.dtsi | 2 +-
> arch/arm/boot/dts/imx7s.dtsi | 2 +-
As Shawn talked, please keep the original "fsl,sec-v4.0-pwrkey", just add
'imx6qdl-snvs-pwrkey' for elder i.mx6 processor i.mx6q/dl/sl, thus no need
to touch other newer processor's dts.
>
> static void imx_imx_snvs_check_for_events(struct timer_list *t) @@ -67,13
> +85,23 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void
> *dev_id) {
> struct platform_device *pdev = dev_id;
> struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> + struct input_dev *input = pdata->input;
> u32 lp_status;
>
> - pm_wakeup_event(pdata->input->dev.parent, 0);
> + pm_wakeup_event(input->dev.parent, 0);
>
> regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> - if (lp_status & SNVS_LPSR_SPO)
> - mod_timer(&pdata->check_timer, jiffies +
> msecs_to_jiffies(DEBOUNCE_TIME));
> + if (lp_status & SNVS_LPSR_SPO) {
> + if (pdata->hwtype == IMX6QDL_SNVS) {
> + input_report_key(input, pdata->keycode, 1);
> + input_report_key(input, pdata->keycode, 0);
> + input_sync(input);
> + pm_relax(input->dev.parent);
Could you move the above input event report steps into imx_imx_snvs_check_for_events()
as before? That make code better to understand and less operation in ISR.
> + } else {
> + mod_timer(&pdata->check_timer,
> + jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> + }
> + }
>
> /* clear SPO status */
> regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO); @@
> -88,11 +116,24 @@ static void imx_snvs_pwrkey_act(void *pdata)
> del_timer_sync(&pd->check_timer);
> }
>
> +static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> + {
> + .compatible = "fsl,imx6sx-sec-v4.0-pwrkey",
> + .data = &imx_snvs_devtype[IMX6SX_SNVS],
> + }, {
> + .compatible = "fsl,imx6qdl-sec-v4.0-pwrkey",
> + .data = &imx_snvs_devtype[IMX6QDL_SNVS],
No ' IMX6QDL_SNVS ' defined in your patch or am I missing?
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix crash when initial logi_dj_recv_query_paired_devices fails
From: Hans de Goede @ 2019-08-26 11:19 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, open list:HID CORE LAYER, 3.8+
In-Reply-To: <CAO-hwJ+O34__f2CFH7-kBQc_95ur1A_yyBe7PXDmGAsQA8ZR7w@mail.gmail.com>
Hi,
On 26-08-19 11:41, Benjamin Tissoires wrote:
> On Mon, Aug 26, 2019 at 11:04 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 26-08-19 09:46, Benjamin Tissoires wrote:
>>> Hi Hans,
>>>
>>> On Sun, Aug 25, 2019 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Before this commit dj_probe would exit with an error if the initial
>>>> logi_dj_recv_query_paired_devices fails. The initial call may fail
>>>> when the receiver is connected through a kvm and the focus is away.
>>>>
>>>> When the call fails this causes 2 problems:
>>>>
>>>> 1) dj_probe calls logi_dj_recv_query_paired_devices after calling
>>>> hid_device_io_start() so a HID report may have been received in between
>>>> and our delayedwork_callback may be running. It seems that the initial
>>>> logi_dj_recv_query_paired_devices failure happening with some KVMs triggers
>>>> this exact scenario, causing the work-queue to run on free-ed memory,
>>>> leading to:
>>>>
>>>> BUG: unable to handle page fault for address: 0000000000001e88
>>>> #PF: supervisor read access in kernel mode
>>>> #PF: error_code(0x0000) - not-present page
>>>> PGD 0 P4D 0
>>>> Oops: 0000 [#1] SMP PTI
>>>> CPU: 3 PID: 257 Comm: kworker/3:3 Tainted: G OE 5.3.0-rc5+ #100
>>>> Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016
>>>> Workqueue: events 0xffffffffc02ba200
>>>> RIP: 0010:0xffffffffc02ba1bd
>>>> Code: e8 e8 13 00 d8 48 89 c5 48 85 c0 74 4c 48 8b 7b 10 48 89 ea b9 07 00 00 00 41 b9 09 00 00 00 41 b8 01 00 00 00 be 10 00 00 00 <48> 8b 87 88 1e 00 00 48 8b 40 40 e8 b3 6b b4 d8 48 89 ef 41 89 c4
>>>> RSP: 0018:ffffb760c046bdb8 EFLAGS: 00010286
>>>> RAX: ffff935038ea4550 RBX: ffff935046778000 RCX: 0000000000000007
>>>> RDX: ffff935038ea4550 RSI: 0000000000000010 RDI: 0000000000000000
>>>> RBP: ffff935038ea4550 R08: 0000000000000001 R09: 0000000000000009
>>>> R10: 000000000000e011 R11: 0000000000000001 R12: ffff9350467780e8
>>>> R13: ffff935046778000 R14: 0000000000000000 R15: ffff935046778070
>>>> FS: 0000000000000000(0000) GS:ffff935054e00000(0000) knlGS:0000000000000000
>>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> CR2: 0000000000001e88 CR3: 000000075a612002 CR4: 00000000003606e0
>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>> Call Trace:
>>>> 0xffffffffc02ba2f7
>>>> ? process_one_work+0x1b1/0x560
>>>> process_one_work+0x234/0x560
>>>> worker_thread+0x50/0x3b0
>>>> kthread+0x10a/0x140
>>>> ? process_one_work+0x560/0x560
>>>> ? kthread_park+0x80/0x80
>>>> ret_from_fork+0x3a/0x50
>>>> Modules linked in: vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) bnep vfat fat btusb btrtl btbcm btintel bluetooth intel_rapl_msr ecdh_generic rfkill ecc snd_usb_audio snd_usbmidi_lib intel_rapl_common snd_rawmidi mc x86_pkg_temp_thermal intel_powerclamp coretemp iTCO_wdt iTCO_vendor_support mei_wdt mei_hdcp ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_generic crc32_pclmul snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio ghash_clmulni_intel intel_cstate snd_hda_intel snd_hda_codec intel_uncore snd_hda_core snd_hwdep intel_rapl_perf snd_seq snd_seq_device snd_pcm snd_timer intel_wmi_thunderbolt snd e1000e soundcore mxm_wmi i2c_i801 bfq mei_me mei intel_pch_thermal parport_pc parport acpi_pad binfmt_misc hid_lg_g15(E) hid_logitech_dj(E) i915 cr
c32c_intel i2c_algo_bit drm_kms_helper nvme nvme_core drm wmi video uas usb_storage i2c_dev
>>>> CR2: 0000000000001e88
>>>> ---[ end trace 1d3f8afdcfcbd842 ]---
>>>>
>>>> 2) Even if we were to fix 1. by making sure the work is stopped before
>>>> failing probe, failing probe is the wrong thing to do, we have
>>>> logi_dj_recv_queue_unknown_work to deal with the initial
>>>> logi_dj_recv_query_paired_devices failure.
>>>>
>>>> Rather then error-ing out of the probe, causing the receiver to not work at
>>>> all we should rely on this, so that the attached devices will get properly
>>>> enumerated once the KVM focus is switched back.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 74808f9115ce ("HID: logitech-dj: add support for non unifying receivers")
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>
>>> Patch looks good to me, but doesn't logi_dj_recv_switch_to_dj_mode()
>>> has the same potential issue?
>>
>> logi_dj_recv_query_paired_devices() solicits data being send from the
>> device, so we do: hid_device_io_start(hdev); just before calling it.
>
> Right, so the patch is just about fixing the workqueue item (and a
> little bit of the device too)
>
> [/me tries to understand the KVM implications]
>
>>
>> logi_dj_recv_switch_to_dj_mode() is done before the hid_device_io_start()
>> so it cannot cause the work to get queued.
>>
>> Also logi_dj_recv_switch_to_dj_mode() failing is something we cannot
>> recover from.
>
> There is one thing I do not get.
> When the KVM hadn't the focus on the device, how can it not forward
> reports when you can actually call logi_dj_recv_switch_to_dj_mode()?
> Does it present the device to all the hosts connected to it and just
> filter the input reports to the "focused" one, or is it something
> different?
I honestly do not know, I have tested with this exact same kvm before and
then the logi_dj_recv_query_paired_devices() call worked, but the
input-reports send in response to it where forwarded to the wrong machine.
And later when focus got restored the unknown handler would redo
logi_dj_recv_query_paired_devices() and all was well.
For some reason, yesterday when I hit this bug and it crashes my workstation
as well as another machine attached to the KVM also running 5.3-rc5, I am now
getting -32 (EPIPE / USB endpoint stalled) errors from the
logi_dj_recv_switch_to_dj_mode() call. I think it may have something TODO
with which of the 2 USB ports on the KVM for mouse and kbd the dongle is in,
but I have not confirmed this yet.
Also I did do a firmware update of the KVM a while ago, I would like to think
that that was done before my previous round of testing. But I'm not sure.
TL;DR: The logi_dj_recv_query_paired_devices() call failing with EPIPE
when the focus is switched away is new and I do not know why this is
happening, but it does crash my machine(s) without the fix.
logi_dj_recv_switch_to_dj_mode() OTOH seems to not care where the focus is...
> And in the case logi_dj_recv_switch_to_dj_mode() failed, on a kvm it
> should not have much implications, because as long as one host
> converts the receiver to the DJ mode and the receiver keeps being
> powered on, it won't change back to the non DJ mode...
True.
> Anyway, I have queued the patch locally for testing, and will push it soon.
Thanks.
Regards,
Hans
>>>> drivers/hid/hid-logitech-dj.c | 10 +++++-----
>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>>>> index cc47f948c1d0..7badbaa18878 100644
>>>> --- a/drivers/hid/hid-logitech-dj.c
>>>> +++ b/drivers/hid/hid-logitech-dj.c
>>>> @@ -1734,14 +1734,14 @@ static int logi_dj_probe(struct hid_device *hdev,
>>>> if (retval < 0) {
>>>> hid_err(hdev, "%s: logi_dj_recv_query_paired_devices error:%d\n",
>>>> __func__, retval);
>>>> - goto logi_dj_recv_query_paired_devices_failed;
>>>> + /*
>>>> + * This can happen with a KVM, let the probe succeed,
>>>> + * logi_dj_recv_queue_unknown_work will retry later.
>>>> + */
>>>> }
>>>> }
>>>>
>>>> - return retval;
>>>> -
>>>> -logi_dj_recv_query_paired_devices_failed:
>>>> - hid_hw_close(hdev);
>>>> + return 0;
>>>>
>>>> llopen_failed:
>>>> switch_to_dj_mode_fail:
>>>> --
>>>> 2.23.0
>>>>
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix crash when initial logi_dj_recv_query_paired_devices fails
From: Benjamin Tissoires @ 2019-08-26 10:01 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, open list:HID CORE LAYER, 3.8+
In-Reply-To: <CAO-hwJ+O34__f2CFH7-kBQc_95ur1A_yyBe7PXDmGAsQA8ZR7w@mail.gmail.com>
On Mon, Aug 26, 2019 at 11:41 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Mon, Aug 26, 2019 at 11:04 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 26-08-19 09:46, Benjamin Tissoires wrote:
> > > Hi Hans,
> > >
> > > On Sun, Aug 25, 2019 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > >>
> > >> Before this commit dj_probe would exit with an error if the initial
> > >> logi_dj_recv_query_paired_devices fails. The initial call may fail
> > >> when the receiver is connected through a kvm and the focus is away.
> > >>
> > >> When the call fails this causes 2 problems:
> > >>
> > >> 1) dj_probe calls logi_dj_recv_query_paired_devices after calling
> > >> hid_device_io_start() so a HID report may have been received in between
> > >> and our delayedwork_callback may be running. It seems that the initial
> > >> logi_dj_recv_query_paired_devices failure happening with some KVMs triggers
> > >> this exact scenario, causing the work-queue to run on free-ed memory,
> > >> leading to:
> > >>
> > >> BUG: unable to handle page fault for address: 0000000000001e88
> > >> #PF: supervisor read access in kernel mode
> > >> #PF: error_code(0x0000) - not-present page
> > >> PGD 0 P4D 0
> > >> Oops: 0000 [#1] SMP PTI
> > >> CPU: 3 PID: 257 Comm: kworker/3:3 Tainted: G OE 5.3.0-rc5+ #100
> > >> Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016
> > >> Workqueue: events 0xffffffffc02ba200
> > >> RIP: 0010:0xffffffffc02ba1bd
> > >> Code: e8 e8 13 00 d8 48 89 c5 48 85 c0 74 4c 48 8b 7b 10 48 89 ea b9 07 00 00 00 41 b9 09 00 00 00 41 b8 01 00 00 00 be 10 00 00 00 <48> 8b 87 88 1e 00 00 48 8b 40 40 e8 b3 6b b4 d8 48 89 ef 41 89 c4
> > >> RSP: 0018:ffffb760c046bdb8 EFLAGS: 00010286
> > >> RAX: ffff935038ea4550 RBX: ffff935046778000 RCX: 0000000000000007
> > >> RDX: ffff935038ea4550 RSI: 0000000000000010 RDI: 0000000000000000
> > >> RBP: ffff935038ea4550 R08: 0000000000000001 R09: 0000000000000009
> > >> R10: 000000000000e011 R11: 0000000000000001 R12: ffff9350467780e8
> > >> R13: ffff935046778000 R14: 0000000000000000 R15: ffff935046778070
> > >> FS: 0000000000000000(0000) GS:ffff935054e00000(0000) knlGS:0000000000000000
> > >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >> CR2: 0000000000001e88 CR3: 000000075a612002 CR4: 00000000003606e0
> > >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > >> Call Trace:
> > >> 0xffffffffc02ba2f7
> > >> ? process_one_work+0x1b1/0x560
> > >> process_one_work+0x234/0x560
> > >> worker_thread+0x50/0x3b0
> > >> kthread+0x10a/0x140
> > >> ? process_one_work+0x560/0x560
> > >> ? kthread_park+0x80/0x80
> > >> ret_from_fork+0x3a/0x50
> > >> Modules linked in: vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) bnep vfat fat btusb btrtl btbcm btintel bluetooth intel_rapl_msr ecdh_generic rfkill ecc snd_usb_audio snd_usbmidi_lib intel_rapl_common snd_rawmidi mc x86_pkg_temp_thermal intel_powerclamp coretemp iTCO_wdt iTCO_vendor_support mei_wdt mei_hdcp ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_generic crc32_pclmul snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio ghash_clmulni_intel intel_cstate snd_hda_intel snd_hda_codec intel_uncore snd_hda_core snd_hwdep intel_rapl_perf snd_seq snd_seq_device snd_pcm snd_timer intel_wmi_thunderbolt snd e1000e soundcore mxm_wmi i2c_i801 bfq mei_me mei intel_pch_thermal parport_pc parport acpi_pad binfmt_misc hid_lg_g15(E) hid_logitech_dj(E) i915 c
rc32c_intel i2c_algo_bit drm_kms_helper nvme nvme_core drm wmi video uas usb_storage i2c_dev
> > >> CR2: 0000000000001e88
> > >> ---[ end trace 1d3f8afdcfcbd842 ]---
> > >>
> > >> 2) Even if we were to fix 1. by making sure the work is stopped before
> > >> failing probe, failing probe is the wrong thing to do, we have
> > >> logi_dj_recv_queue_unknown_work to deal with the initial
> > >> logi_dj_recv_query_paired_devices failure.
> > >>
> > >> Rather then error-ing out of the probe, causing the receiver to not work at
> > >> all we should rely on this, so that the attached devices will get properly
> > >> enumerated once the KVM focus is switched back.
> > >>
> > >> Cc: stable@vger.kernel.org
> > >> Fixes: 74808f9115ce ("HID: logitech-dj: add support for non unifying receivers")
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> ---
> > >
> > > Patch looks good to me, but doesn't logi_dj_recv_switch_to_dj_mode()
> > > has the same potential issue?
> >
> > logi_dj_recv_query_paired_devices() solicits data being send from the
> > device, so we do: hid_device_io_start(hdev); just before calling it.
>
> Right, so the patch is just about fixing the workqueue item (and a
> little bit of the device too)
>
> [/me tries to understand the KVM implications]
>
> >
> > logi_dj_recv_switch_to_dj_mode() is done before the hid_device_io_start()
> > so it cannot cause the work to get queued.
> >
> > Also logi_dj_recv_switch_to_dj_mode() failing is something we cannot
> > recover from.
>
> There is one thing I do not get.
> When the KVM hadn't the focus on the device, how can it not forward
> reports when you can actually call logi_dj_recv_switch_to_dj_mode()?
> Does it present the device to all the hosts connected to it and just
> filter the input reports to the "focused" one, or is it something
> different?
>
> And in the case logi_dj_recv_switch_to_dj_mode() failed, on a kvm it
> should not have much implications, because as long as one host
> converts the receiver to the DJ mode and the receiver keeps being
> powered on, it won't change back to the non DJ mode...
>
> Anyway, I have queued the patch locally for testing, and will push it soon.
FWIW, patch is now applied in f-5.3/upstream/fixes
But I still would like to have your input on my KVM questions above :)
Cheers,
Benjamin
>
> Cheers,
> Benjamin
>
> > >> drivers/hid/hid-logitech-dj.c | 10 +++++-----
> > >> 1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > >> index cc47f948c1d0..7badbaa18878 100644
> > >> --- a/drivers/hid/hid-logitech-dj.c
> > >> +++ b/drivers/hid/hid-logitech-dj.c
> > >> @@ -1734,14 +1734,14 @@ static int logi_dj_probe(struct hid_device *hdev,
> > >> if (retval < 0) {
> > >> hid_err(hdev, "%s: logi_dj_recv_query_paired_devices error:%d\n",
> > >> __func__, retval);
> > >> - goto logi_dj_recv_query_paired_devices_failed;
> > >> + /*
> > >> + * This can happen with a KVM, let the probe succeed,
> > >> + * logi_dj_recv_queue_unknown_work will retry later.
> > >> + */
> > >> }
> > >> }
> > >>
> > >> - return retval;
> > >> -
> > >> -logi_dj_recv_query_paired_devices_failed:
> > >> - hid_hw_close(hdev);
> > >> + return 0;
> > >>
> > >> llopen_failed:
> > >> switch_to_dj_mode_fail:
> > >> --
> > >> 2.23.0
> > >>
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix crash when initial logi_dj_recv_query_paired_devices fails
From: Benjamin Tissoires @ 2019-08-26 9:41 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, open list:HID CORE LAYER, 3.8+
In-Reply-To: <d5ff19d7-4a0c-6c38-2d97-fcc33a8cdedb@redhat.com>
On Mon, Aug 26, 2019 at 11:04 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 26-08-19 09:46, Benjamin Tissoires wrote:
> > Hi Hans,
> >
> > On Sun, Aug 25, 2019 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Before this commit dj_probe would exit with an error if the initial
> >> logi_dj_recv_query_paired_devices fails. The initial call may fail
> >> when the receiver is connected through a kvm and the focus is away.
> >>
> >> When the call fails this causes 2 problems:
> >>
> >> 1) dj_probe calls logi_dj_recv_query_paired_devices after calling
> >> hid_device_io_start() so a HID report may have been received in between
> >> and our delayedwork_callback may be running. It seems that the initial
> >> logi_dj_recv_query_paired_devices failure happening with some KVMs triggers
> >> this exact scenario, causing the work-queue to run on free-ed memory,
> >> leading to:
> >>
> >> BUG: unable to handle page fault for address: 0000000000001e88
> >> #PF: supervisor read access in kernel mode
> >> #PF: error_code(0x0000) - not-present page
> >> PGD 0 P4D 0
> >> Oops: 0000 [#1] SMP PTI
> >> CPU: 3 PID: 257 Comm: kworker/3:3 Tainted: G OE 5.3.0-rc5+ #100
> >> Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016
> >> Workqueue: events 0xffffffffc02ba200
> >> RIP: 0010:0xffffffffc02ba1bd
> >> Code: e8 e8 13 00 d8 48 89 c5 48 85 c0 74 4c 48 8b 7b 10 48 89 ea b9 07 00 00 00 41 b9 09 00 00 00 41 b8 01 00 00 00 be 10 00 00 00 <48> 8b 87 88 1e 00 00 48 8b 40 40 e8 b3 6b b4 d8 48 89 ef 41 89 c4
> >> RSP: 0018:ffffb760c046bdb8 EFLAGS: 00010286
> >> RAX: ffff935038ea4550 RBX: ffff935046778000 RCX: 0000000000000007
> >> RDX: ffff935038ea4550 RSI: 0000000000000010 RDI: 0000000000000000
> >> RBP: ffff935038ea4550 R08: 0000000000000001 R09: 0000000000000009
> >> R10: 000000000000e011 R11: 0000000000000001 R12: ffff9350467780e8
> >> R13: ffff935046778000 R14: 0000000000000000 R15: ffff935046778070
> >> FS: 0000000000000000(0000) GS:ffff935054e00000(0000) knlGS:0000000000000000
> >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> CR2: 0000000000001e88 CR3: 000000075a612002 CR4: 00000000003606e0
> >> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >> Call Trace:
> >> 0xffffffffc02ba2f7
> >> ? process_one_work+0x1b1/0x560
> >> process_one_work+0x234/0x560
> >> worker_thread+0x50/0x3b0
> >> kthread+0x10a/0x140
> >> ? process_one_work+0x560/0x560
> >> ? kthread_park+0x80/0x80
> >> ret_from_fork+0x3a/0x50
> >> Modules linked in: vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) bnep vfat fat btusb btrtl btbcm btintel bluetooth intel_rapl_msr ecdh_generic rfkill ecc snd_usb_audio snd_usbmidi_lib intel_rapl_common snd_rawmidi mc x86_pkg_temp_thermal intel_powerclamp coretemp iTCO_wdt iTCO_vendor_support mei_wdt mei_hdcp ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_generic crc32_pclmul snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio ghash_clmulni_intel intel_cstate snd_hda_intel snd_hda_codec intel_uncore snd_hda_core snd_hwdep intel_rapl_perf snd_seq snd_seq_device snd_pcm snd_timer intel_wmi_thunderbolt snd e1000e soundcore mxm_wmi i2c_i801 bfq mei_me mei intel_pch_thermal parport_pc parport acpi_pad binfmt_misc hid_lg_g15(E) hid_logitech_dj(E) i915 crc
32c_intel i2c_algo_bit drm_kms_helper nvme nvme_core drm wmi video uas usb_storage i2c_dev
> >> CR2: 0000000000001e88
> >> ---[ end trace 1d3f8afdcfcbd842 ]---
> >>
> >> 2) Even if we were to fix 1. by making sure the work is stopped before
> >> failing probe, failing probe is the wrong thing to do, we have
> >> logi_dj_recv_queue_unknown_work to deal with the initial
> >> logi_dj_recv_query_paired_devices failure.
> >>
> >> Rather then error-ing out of the probe, causing the receiver to not work at
> >> all we should rely on this, so that the attached devices will get properly
> >> enumerated once the KVM focus is switched back.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 74808f9115ce ("HID: logitech-dj: add support for non unifying receivers")
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >
> > Patch looks good to me, but doesn't logi_dj_recv_switch_to_dj_mode()
> > has the same potential issue?
>
> logi_dj_recv_query_paired_devices() solicits data being send from the
> device, so we do: hid_device_io_start(hdev); just before calling it.
Right, so the patch is just about fixing the workqueue item (and a
little bit of the device too)
[/me tries to understand the KVM implications]
>
> logi_dj_recv_switch_to_dj_mode() is done before the hid_device_io_start()
> so it cannot cause the work to get queued.
>
> Also logi_dj_recv_switch_to_dj_mode() failing is something we cannot
> recover from.
There is one thing I do not get.
When the KVM hadn't the focus on the device, how can it not forward
reports when you can actually call logi_dj_recv_switch_to_dj_mode()?
Does it present the device to all the hosts connected to it and just
filter the input reports to the "focused" one, or is it something
different?
And in the case logi_dj_recv_switch_to_dj_mode() failed, on a kvm it
should not have much implications, because as long as one host
converts the receiver to the DJ mode and the receiver keeps being
powered on, it won't change back to the non DJ mode...
Anyway, I have queued the patch locally for testing, and will push it soon.
Cheers,
Benjamin
> >> drivers/hid/hid-logitech-dj.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> >> index cc47f948c1d0..7badbaa18878 100644
> >> --- a/drivers/hid/hid-logitech-dj.c
> >> +++ b/drivers/hid/hid-logitech-dj.c
> >> @@ -1734,14 +1734,14 @@ static int logi_dj_probe(struct hid_device *hdev,
> >> if (retval < 0) {
> >> hid_err(hdev, "%s: logi_dj_recv_query_paired_devices error:%d\n",
> >> __func__, retval);
> >> - goto logi_dj_recv_query_paired_devices_failed;
> >> + /*
> >> + * This can happen with a KVM, let the probe succeed,
> >> + * logi_dj_recv_queue_unknown_work will retry later.
> >> + */
> >> }
> >> }
> >>
> >> - return retval;
> >> -
> >> -logi_dj_recv_query_paired_devices_failed:
> >> - hid_hw_close(hdev);
> >> + return 0;
> >>
> >> llopen_failed:
> >> switch_to_dj_mode_fail:
> >> --
> >> 2.23.0
> >>
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix crash when initial logi_dj_recv_query_paired_devices fails
From: Hans de Goede @ 2019-08-26 9:03 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, open list:HID CORE LAYER, 3.8+
In-Reply-To: <CAO-hwJ+AiViJg34dNKz05HfvnPVqigD7ZLyJpfsviBH8Rs0L2g@mail.gmail.com>
Hi,
On 26-08-19 09:46, Benjamin Tissoires wrote:
> Hi Hans,
>
> On Sun, Aug 25, 2019 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Before this commit dj_probe would exit with an error if the initial
>> logi_dj_recv_query_paired_devices fails. The initial call may fail
>> when the receiver is connected through a kvm and the focus is away.
>>
>> When the call fails this causes 2 problems:
>>
>> 1) dj_probe calls logi_dj_recv_query_paired_devices after calling
>> hid_device_io_start() so a HID report may have been received in between
>> and our delayedwork_callback may be running. It seems that the initial
>> logi_dj_recv_query_paired_devices failure happening with some KVMs triggers
>> this exact scenario, causing the work-queue to run on free-ed memory,
>> leading to:
>>
>> BUG: unable to handle page fault for address: 0000000000001e88
>> #PF: supervisor read access in kernel mode
>> #PF: error_code(0x0000) - not-present page
>> PGD 0 P4D 0
>> Oops: 0000 [#1] SMP PTI
>> CPU: 3 PID: 257 Comm: kworker/3:3 Tainted: G OE 5.3.0-rc5+ #100
>> Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016
>> Workqueue: events 0xffffffffc02ba200
>> RIP: 0010:0xffffffffc02ba1bd
>> Code: e8 e8 13 00 d8 48 89 c5 48 85 c0 74 4c 48 8b 7b 10 48 89 ea b9 07 00 00 00 41 b9 09 00 00 00 41 b8 01 00 00 00 be 10 00 00 00 <48> 8b 87 88 1e 00 00 48 8b 40 40 e8 b3 6b b4 d8 48 89 ef 41 89 c4
>> RSP: 0018:ffffb760c046bdb8 EFLAGS: 00010286
>> RAX: ffff935038ea4550 RBX: ffff935046778000 RCX: 0000000000000007
>> RDX: ffff935038ea4550 RSI: 0000000000000010 RDI: 0000000000000000
>> RBP: ffff935038ea4550 R08: 0000000000000001 R09: 0000000000000009
>> R10: 000000000000e011 R11: 0000000000000001 R12: ffff9350467780e8
>> R13: ffff935046778000 R14: 0000000000000000 R15: ffff935046778070
>> FS: 0000000000000000(0000) GS:ffff935054e00000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000001e88 CR3: 000000075a612002 CR4: 00000000003606e0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>> 0xffffffffc02ba2f7
>> ? process_one_work+0x1b1/0x560
>> process_one_work+0x234/0x560
>> worker_thread+0x50/0x3b0
>> kthread+0x10a/0x140
>> ? process_one_work+0x560/0x560
>> ? kthread_park+0x80/0x80
>> ret_from_fork+0x3a/0x50
>> Modules linked in: vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) bnep vfat fat btusb btrtl btbcm btintel bluetooth intel_rapl_msr ecdh_generic rfkill ecc snd_usb_audio snd_usbmidi_lib intel_rapl_common snd_rawmidi mc x86_pkg_temp_thermal intel_powerclamp coretemp iTCO_wdt iTCO_vendor_support mei_wdt mei_hdcp ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_generic crc32_pclmul snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio ghash_clmulni_intel intel_cstate snd_hda_intel snd_hda_codec intel_uncore snd_hda_core snd_hwdep intel_rapl_perf snd_seq snd_seq_device snd_pcm snd_timer intel_wmi_thunderbolt snd e1000e soundcore mxm_wmi i2c_i801 bfq mei_me mei intel_pch_thermal parport_pc parport acpi_pad binfmt_misc hid_lg_g15(E) hid_logitech_dj(E) i915 crc32
c_intel i2c_algo_bit drm_kms_helper nvme nvme_core drm wmi video uas usb_storage i2c_dev
>> CR2: 0000000000001e88
>> ---[ end trace 1d3f8afdcfcbd842 ]---
>>
>> 2) Even if we were to fix 1. by making sure the work is stopped before
>> failing probe, failing probe is the wrong thing to do, we have
>> logi_dj_recv_queue_unknown_work to deal with the initial
>> logi_dj_recv_query_paired_devices failure.
>>
>> Rather then error-ing out of the probe, causing the receiver to not work at
>> all we should rely on this, so that the attached devices will get properly
>> enumerated once the KVM focus is switched back.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 74808f9115ce ("HID: logitech-dj: add support for non unifying receivers")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>
> Patch looks good to me, but doesn't logi_dj_recv_switch_to_dj_mode()
> has the same potential issue?
logi_dj_recv_query_paired_devices() solicits data being send from the
device, so we do: hid_device_io_start(hdev); just before calling it.
logi_dj_recv_switch_to_dj_mode() is done before the hid_device_io_start()
so it cannot cause the work to get queued.
Also logi_dj_recv_switch_to_dj_mode() failing is something we cannot
recover from.
Regards,
Hans
>
> Cheers,
> Benjamin
>
>> drivers/hid/hid-logitech-dj.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index cc47f948c1d0..7badbaa18878 100644
>> --- a/drivers/hid/hid-logitech-dj.c
>> +++ b/drivers/hid/hid-logitech-dj.c
>> @@ -1734,14 +1734,14 @@ static int logi_dj_probe(struct hid_device *hdev,
>> if (retval < 0) {
>> hid_err(hdev, "%s: logi_dj_recv_query_paired_devices error:%d\n",
>> __func__, retval);
>> - goto logi_dj_recv_query_paired_devices_failed;
>> + /*
>> + * This can happen with a KVM, let the probe succeed,
>> + * logi_dj_recv_queue_unknown_work will retry later.
>> + */
>> }
>> }
>>
>> - return retval;
>> -
>> -logi_dj_recv_query_paired_devices_failed:
>> - hid_hw_close(hdev);
>> + return 0;
>>
>> llopen_failed:
>> switch_to_dj_mode_fail:
>> --
>> 2.23.0
>>
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix crash when initial logi_dj_recv_query_paired_devices fails
From: Benjamin Tissoires @ 2019-08-26 7:46 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, open list:HID CORE LAYER, 3.8+
In-Reply-To: <20190825153542.79245-1-hdegoede@redhat.com>
Hi Hans,
On Sun, Aug 25, 2019 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Before this commit dj_probe would exit with an error if the initial
> logi_dj_recv_query_paired_devices fails. The initial call may fail
> when the receiver is connected through a kvm and the focus is away.
>
> When the call fails this causes 2 problems:
>
> 1) dj_probe calls logi_dj_recv_query_paired_devices after calling
> hid_device_io_start() so a HID report may have been received in between
> and our delayedwork_callback may be running. It seems that the initial
> logi_dj_recv_query_paired_devices failure happening with some KVMs triggers
> this exact scenario, causing the work-queue to run on free-ed memory,
> leading to:
>
> BUG: unable to handle page fault for address: 0000000000001e88
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 3 PID: 257 Comm: kworker/3:3 Tainted: G OE 5.3.0-rc5+ #100
> Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016
> Workqueue: events 0xffffffffc02ba200
> RIP: 0010:0xffffffffc02ba1bd
> Code: e8 e8 13 00 d8 48 89 c5 48 85 c0 74 4c 48 8b 7b 10 48 89 ea b9 07 00 00 00 41 b9 09 00 00 00 41 b8 01 00 00 00 be 10 00 00 00 <48> 8b 87 88 1e 00 00 48 8b 40 40 e8 b3 6b b4 d8 48 89 ef 41 89 c4
> RSP: 0018:ffffb760c046bdb8 EFLAGS: 00010286
> RAX: ffff935038ea4550 RBX: ffff935046778000 RCX: 0000000000000007
> RDX: ffff935038ea4550 RSI: 0000000000000010 RDI: 0000000000000000
> RBP: ffff935038ea4550 R08: 0000000000000001 R09: 0000000000000009
> R10: 000000000000e011 R11: 0000000000000001 R12: ffff9350467780e8
> R13: ffff935046778000 R14: 0000000000000000 R15: ffff935046778070
> FS: 0000000000000000(0000) GS:ffff935054e00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000001e88 CR3: 000000075a612002 CR4: 00000000003606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> 0xffffffffc02ba2f7
> ? process_one_work+0x1b1/0x560
> process_one_work+0x234/0x560
> worker_thread+0x50/0x3b0
> kthread+0x10a/0x140
> ? process_one_work+0x560/0x560
> ? kthread_park+0x80/0x80
> ret_from_fork+0x3a/0x50
> Modules linked in: vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) bnep vfat fat btusb btrtl btbcm btintel bluetooth intel_rapl_msr ecdh_generic rfkill ecc snd_usb_audio snd_usbmidi_lib intel_rapl_common snd_rawmidi mc x86_pkg_temp_thermal intel_powerclamp coretemp iTCO_wdt iTCO_vendor_support mei_wdt mei_hdcp ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_generic crc32_pclmul snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio ghash_clmulni_intel intel_cstate snd_hda_intel snd_hda_codec intel_uncore snd_hda_core snd_hwdep intel_rapl_perf snd_seq snd_seq_device snd_pcm snd_timer intel_wmi_thunderbolt snd e1000e soundcore mxm_wmi i2c_i801 bfq mei_me mei intel_pch_thermal parport_pc parport acpi_pad binfmt_misc hid_lg_g15(E) hid_logitech_dj(E) i915 crc32c_
intel i2c_algo_bit drm_kms_helper nvme nvme_core drm wmi video uas usb_storage i2c_dev
> CR2: 0000000000001e88
> ---[ end trace 1d3f8afdcfcbd842 ]---
>
> 2) Even if we were to fix 1. by making sure the work is stopped before
> failing probe, failing probe is the wrong thing to do, we have
> logi_dj_recv_queue_unknown_work to deal with the initial
> logi_dj_recv_query_paired_devices failure.
>
> Rather then error-ing out of the probe, causing the receiver to not work at
> all we should rely on this, so that the attached devices will get properly
> enumerated once the KVM focus is switched back.
>
> Cc: stable@vger.kernel.org
> Fixes: 74808f9115ce ("HID: logitech-dj: add support for non unifying receivers")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
Patch looks good to me, but doesn't logi_dj_recv_switch_to_dj_mode()
has the same potential issue?
Cheers,
Benjamin
> drivers/hid/hid-logitech-dj.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index cc47f948c1d0..7badbaa18878 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1734,14 +1734,14 @@ static int logi_dj_probe(struct hid_device *hdev,
> if (retval < 0) {
> hid_err(hdev, "%s: logi_dj_recv_query_paired_devices error:%d\n",
> __func__, retval);
> - goto logi_dj_recv_query_paired_devices_failed;
> + /*
> + * This can happen with a KVM, let the probe succeed,
> + * logi_dj_recv_queue_unknown_work will retry later.
> + */
> }
> }
>
> - return retval;
> -
> -logi_dj_recv_query_paired_devices_failed:
> - hid_hw_close(hdev);
> + return 0;
>
> llopen_failed:
> switch_to_dj_mode_fail:
> --
> 2.23.0
>
^ permalink raw reply
* Re: Re: [PATCH v2] HID: input: fix a4tech horizontal wheel custom usage
From: Heorhi Valakhanovich @ 2019-08-25 20:50 UTC (permalink / raw)
To: Jiri Kosina
Cc: Nicolas Saenz Julienne, Benjamin Tissoires, dmitry.torokhov,
wbauer, linux-input, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.1908051437490.5899@cbobk.fhfr.pm>
On 8/5/19 3:38 PM, Jiri Kosina wrote:
> On Thu, 1 Aug 2019, Nicolas Saenz Julienne wrote:
>
>>> Some a4tech mice use the 'GenericDesktop.00b8' usage to inform whether
>>> the previous wheel report was horizontal or vertical. Before
>>> c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this
>>> usage was being mapped to 'Relative.Misc'. After the patch it's simply
>>> ignored (usage->type == 0 & usage->code == 0). Which ultimately makes
>>> hid-a4tech ignore the WHEEL/HWHEEL selection event, as it has no
>>> usage->type.
>>>
>>> We shouldn't rely on a mapping for that usage as it's nonstandard and
>>> doesn't really map to an input event. So we bypass the mapping and make
>>> sure the custom event handling properly handles both reports.
>>>
>>> Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key")
>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> ---
>>
>> It would be nice for this patch not to get lost. It fixes issues both repoted
>> on opensuse and fedora.
>
> Sorry for the delay. I've now queued the patch. Thanks for fixing this,
>
Any plans to apply this patch on top of 5.2.* and 5.1.* ?
^ permalink raw reply
* [PATCH] HID: logitech-dj: Fix crash when initial logi_dj_recv_query_paired_devices fails
From: Hans de Goede @ 2019-08-25 15:35 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: Hans de Goede, linux-input, stable
Before this commit dj_probe would exit with an error if the initial
logi_dj_recv_query_paired_devices fails. The initial call may fail
when the receiver is connected through a kvm and the focus is away.
When the call fails this causes 2 problems:
1) dj_probe calls logi_dj_recv_query_paired_devices after calling
hid_device_io_start() so a HID report may have been received in between
and our delayedwork_callback may be running. It seems that the initial
logi_dj_recv_query_paired_devices failure happening with some KVMs triggers
this exact scenario, causing the work-queue to run on free-ed memory,
leading to:
BUG: unable to handle page fault for address: 0000000000001e88
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 257 Comm: kworker/3:3 Tainted: G OE 5.3.0-rc5+ #100
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./B150M Pro4S/D3, BIOS P7.10 12/06/2016
Workqueue: events 0xffffffffc02ba200
RIP: 0010:0xffffffffc02ba1bd
Code: e8 e8 13 00 d8 48 89 c5 48 85 c0 74 4c 48 8b 7b 10 48 89 ea b9 07 00 00 00 41 b9 09 00 00 00 41 b8 01 00 00 00 be 10 00 00 00 <48> 8b 87 88 1e 00 00 48 8b 40 40 e8 b3 6b b4 d8 48 89 ef 41 89 c4
RSP: 0018:ffffb760c046bdb8 EFLAGS: 00010286
RAX: ffff935038ea4550 RBX: ffff935046778000 RCX: 0000000000000007
RDX: ffff935038ea4550 RSI: 0000000000000010 RDI: 0000000000000000
RBP: ffff935038ea4550 R08: 0000000000000001 R09: 0000000000000009
R10: 000000000000e011 R11: 0000000000000001 R12: ffff9350467780e8
R13: ffff935046778000 R14: 0000000000000000 R15: ffff935046778070
FS: 0000000000000000(0000) GS:ffff935054e00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000001e88 CR3: 000000075a612002 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
0xffffffffc02ba2f7
? process_one_work+0x1b1/0x560
process_one_work+0x234/0x560
worker_thread+0x50/0x3b0
kthread+0x10a/0x140
? process_one_work+0x560/0x560
? kthread_park+0x80/0x80
ret_from_fork+0x3a/0x50
Modules linked in: vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) bnep vfat fat btusb btrtl btbcm btintel bluetooth intel_rapl_msr ecdh_generic rfkill ecc snd_usb_audio snd_usbmidi_lib intel_rapl_common snd_rawmidi mc x86_pkg_temp_thermal intel_powerclamp coretemp iTCO_wdt iTCO_vendor_support mei_wdt mei_hdcp ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_generic crc32_pclmul snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio ghash_clmulni_intel intel_cstate snd_hda_intel snd_hda_codec intel_uncore snd_hda_core snd_hwdep intel_rapl_perf snd_seq snd_seq_device snd_pcm snd_timer intel_wmi_thunderbolt snd e1000e soundcore mxm_wmi i2c_i801 bfq mei_me mei intel_pch_thermal parport_pc parport acpi_pad binfmt_misc hid_lg_g15(E) hid_logitech_dj(E) i915 crc32c_in
tel i2c_algo_bit drm_kms_helper nvme nvme_core drm wmi video uas usb_storage i2c_dev
CR2: 0000000000001e88
---[ end trace 1d3f8afdcfcbd842 ]---
2) Even if we were to fix 1. by making sure the work is stopped before
failing probe, failing probe is the wrong thing to do, we have
logi_dj_recv_queue_unknown_work to deal with the initial
logi_dj_recv_query_paired_devices failure.
Rather then error-ing out of the probe, causing the receiver to not work at
all we should rely on this, so that the attached devices will get properly
enumerated once the KVM focus is switched back.
Cc: stable@vger.kernel.org
Fixes: 74808f9115ce ("HID: logitech-dj: add support for non unifying receivers")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index cc47f948c1d0..7badbaa18878 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1734,14 +1734,14 @@ static int logi_dj_probe(struct hid_device *hdev,
if (retval < 0) {
hid_err(hdev, "%s: logi_dj_recv_query_paired_devices error:%d\n",
__func__, retval);
- goto logi_dj_recv_query_paired_devices_failed;
+ /*
+ * This can happen with a KVM, let the probe succeed,
+ * logi_dj_recv_queue_unknown_work will retry later.
+ */
}
}
- return retval;
-
-logi_dj_recv_query_paired_devices_failed:
- hid_hw_close(hdev);
+ return 0;
llopen_failed:
switch_to_dj_mode_fail:
--
2.23.0
^ permalink raw reply related
* Re: [PATCH] input: keyboard: snvs_pwrkey: Send press and release event for i.MX6 S,DL and Q
From: Shawn Guo @ 2019-08-25 7:10 UTC (permalink / raw)
To: Robin van der Gracht
Cc: Rob Herring, Mark Rutland, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, NXP Linux Team, Dmitry Torokhov, devicetree,
linux-arm-kernel, linux-kernel, linux-input, Robin Gong,
Adam Ford
In-Reply-To: <20190823123002.10448-1-robin@protonic.nl>
On Fri, Aug 23, 2019 at 02:30:02PM +0200, Robin van der Gracht wrote:
> The older generation i.MX6 processors send a powerdown request interrupt if
> the powerkey is released before a hard shutdown (5 second press). This should
> allow software to bring down the SoC safely.
>
> For this driver to work as a regular powerkey with the older SoCs, we need to
> send a keypress AND release when we get the powerdown request interrupt.
>
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
> arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
> arch/arm/boot/dts/imx6sll.dtsi | 2 +-
> arch/arm/boot/dts/imx6sx.dtsi | 2 +-
> arch/arm/boot/dts/imx6ul.dtsi | 2 +-
> arch/arm/boot/dts/imx7s.dtsi | 2 +-
Please have dts changes in a separate patch.
> drivers/input/keyboard/Kconfig | 2 +-
> drivers/input/keyboard/snvs_pwrkey.c | 59 +++++++++++++++++++++++-----
> 7 files changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index b3a77bcf00d51..c10d12658743c 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -836,7 +836,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6qdl-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6sll.dtsi b/arch/arm/boot/dts/imx6sll.dtsi
> index 1b4899f0fcded..91c7d5bdcc359 100644
> --- a/arch/arm/boot/dts/imx6sll.dtsi
> +++ b/arch/arm/boot/dts/imx6sll.dtsi
> @@ -571,7 +571,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
> index b16a123990a26..b6736db65350f 100644
> --- a/arch/arm/boot/dts/imx6sx.dtsi
> +++ b/arch/arm/boot/dts/imx6sx.dtsi
> @@ -733,7 +733,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
> index a7f6d1d58e20d..d4678c52b55db 100644
> --- a/arch/arm/boot/dts/imx6ul.dtsi
> +++ b/arch/arm/boot/dts/imx6ul.dtsi
> @@ -644,7 +644,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 106711d2c01b0..bb68c23beb199 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -604,7 +604,7 @@
> };
>
> snvs_pwrkey: snvs-powerkey {
> - compatible = "fsl,sec-v4.0-pwrkey";
> + compatible = "fsl,imx6sx-sec-v4.0-pwrkey";
> regmap = <&snvs>;
> interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> linux,keycode = <KEY_POWER>;
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 7c4f19dab34fd..937e58da5ce17 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -436,7 +436,7 @@ config KEYBOARD_SNVS_PWRKEY
> depends on OF
> help
> This is the snvs powerkey driver for the Freescale i.MX application
> - processors that are newer than i.MX6 SX.
> + processors.
>
> To compile this driver as a module, choose M here; the
> module will be called snvs_pwrkey.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 5342d8d45f811..c321e5f2d1087 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -29,6 +29,11 @@
> #define DEBOUNCE_TIME 30
> #define REPEAT_INTERVAL 60
>
> +enum imx_snvs_hwtype {
> + IMX6SX_SNVS, /* i.MX6 SoloX and newer */
> + IMX6QDL_SNVS, /* i.MX6 Solo, DualLite adn Quad */
> +};
> +
> struct pwrkey_drv_data {
> struct regmap *snvs;
> int irq;
> @@ -37,6 +42,19 @@ struct pwrkey_drv_data {
> int wakeup;
> struct timer_list check_timer;
> struct input_dev *input;
> + enum imx_snvs_hwtype hwtype;
> +};
> +
> +static const struct platform_device_id imx_snvs_devtype[] = {
> + {
> + .name = "imx6sx-snvs-pwrkey",
> + .driver_data = IMX6SX_SNVS,
> + }, {
> + .name = "imx6qdl-snvs-pwrkey",
> + .driver_data = IMX6QDL_SNVS,
> + }, {
> + /* sentinel */
> + }
> };
>
> static void imx_imx_snvs_check_for_events(struct timer_list *t)
> @@ -67,13 +85,23 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
> {
> struct platform_device *pdev = dev_id;
> struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> + struct input_dev *input = pdata->input;
> u32 lp_status;
>
> - pm_wakeup_event(pdata->input->dev.parent, 0);
> + pm_wakeup_event(input->dev.parent, 0);
>
> regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> - if (lp_status & SNVS_LPSR_SPO)
> - mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> + if (lp_status & SNVS_LPSR_SPO) {
> + if (pdata->hwtype == IMX6QDL_SNVS) {
> + input_report_key(input, pdata->keycode, 1);
> + input_report_key(input, pdata->keycode, 0);
> + input_sync(input);
> + pm_relax(input->dev.parent);
> + } else {
> + mod_timer(&pdata->check_timer,
> + jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> + }
> + }
>
> /* clear SPO status */
> regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> @@ -88,11 +116,24 @@ static void imx_snvs_pwrkey_act(void *pdata)
> del_timer_sync(&pd->check_timer);
> }
>
> +static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> + {
> + .compatible = "fsl,imx6sx-sec-v4.0-pwrkey",
The new compatible should be documented.
> + .data = &imx_snvs_devtype[IMX6SX_SNVS],
> + }, {
> + .compatible = "fsl,imx6qdl-sec-v4.0-pwrkey",
> + .data = &imx_snvs_devtype[IMX6QDL_SNVS],
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> +
> static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> {
> struct pwrkey_drv_data *pdata = NULL;
> struct input_dev *input = NULL;
> struct device_node *np;
> + const struct of_device_id *of_id;
> int error;
>
> /* Get SNVS register Page */
> @@ -100,6 +141,11 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> if (!np)
> return -ENODEV;
>
> + of_id = of_match_node(imx_snvs_pwrkey_ids, np);
> + if (!of_id)
> + return -ENODEV;
> + pdev->id_entry = of_id->data;
> +
> pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> return -ENOMEM;
> @@ -116,6 +162,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> }
>
> pdata->wakeup = of_property_read_bool(np, "wakeup-source");
> + pdata->hwtype = pdev->id_entry->driver_data;
>
> pdata->irq = platform_get_irq(pdev, 0);
> if (pdata->irq < 0) {
> @@ -175,12 +222,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct of_device_id imx_snvs_pwrkey_ids[] = {
> - { .compatible = "fsl,sec-v4.0-pwrkey" },
The compatible should be kept for not breaking existing DTBs.
Shawn
> - { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
> -
> static struct platform_driver imx_snvs_pwrkey_driver = {
> .driver = {
> .name = "snvs_pwrkey",
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Kai-Heng Feng @ 2019-08-24 16:28 UTC (permalink / raw)
To: Alan Stern
Cc: Oliver Neukum, jikos, benjamin.tissoires, linux-input,
linux-kernel, linux-usb
In-Reply-To: <Pine.LNX.4.44L0.1908221043080.1311-100000@iolanthe.rowland.org>
at 22:49, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 22 Aug 2019, Kai-Heng Feng wrote:
>
>> at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
>>
>>> Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
>>>> Hi Oliver,
>>>>
>>>> at 17:45, Oliver Neukum <oneukum@suse.com> wrote:
>>>>
>>>>> Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
>>>>>> The optical sensor of the mouse gets turned off when it's runtime
>>>>>> suspended, so moving the mouse can't wake the mouse up, despite that
>>>>>> USB remote wakeup is successfully set.
>>>>>>
>>>>>> Introduce a new quirk to prevent the mouse from getting runtime
>>>>>> suspended.
>>>>>
>>>>> Hi,
>>>>>
>>>>> I am afraid this is a bad approach in principle. The device
>>>>> behaves according to spec.
>>>>
>>>> Can you please point out which spec it is? Is it USB 2.0 spec?
>>>
>>> Well, sort of. The USB spec merely states how to enter and exit
>>> a suspended state and that device state must not be lost.
>>> It does not tell you what a suspended device must be able to do.
>>
>> But shouldn’t remote wakeup signaling wakes the device up and let it exit
>> suspend state?
>> Or it’s okay to let the device be suspended when remote wakeup is needed
>> but broken?
>>
>>>>> And it behaves like most hardware.
>>>>
>>>> So seems like most hardware are broken.
>>>> Maybe a more appropriate solution is to disable RPM for all USB mice.
>>>
>>> That is a decision a distro certainly can make. However, the kernel
>>> does not, by default, call usb_enable_autosuspend() for HID devices
>>> for this very reason. It is enabled by default only for hubs,
>>> BT dongles and UVC cameras (and some minor devices)
>>>
>>> In other words, if on your system it is on, you need to look
>>> at udev, not the kernel.
>>
>> So if a device is broken when “power/control” is flipped by user, we
>> should
>> deal it at userspace? That doesn’t sound right to me.
>>
>>>>> If you do not want runtime PM for such devices, do not switch
>>>>> it on.
>>>>
>>>> A device should work regardless of runtime PM status.
>>>
>>> Well, no. Runtime PM is a trade off. You lose something if you use
>>> it. If it worked just as well as full power, you would never use
>>> full power, would you?
>>
>> I am not asking the suspended state to work as full power, but to
>> prevent a
>> device enters suspend state because of broken remote wakeup.
>>
>>> Whether the loss of functionality or performance is worth the energy
>>> savings is a policy decision. Hence it belongs into udev.
>>> Ideally the kernel would tell user space what will work in a
>>> suspended state. Unfortunately HID does not provide support for that.
>>
>> I really don’t think “loss of functionally” belongs to policy decision.
>> But
>> that’s just my opinion.
>>
>>> This is a deficiency of user space. The kernel has an ioctl()
>>> to let user space tell it, whether a device is fully needed.
>>> X does not use them.
>>
>> Ok, I’ll take a look at other device drivers that use it.
>>
>>>>> The refcounting needs to be done correctly.
>>>>
>>>> Will do.
>>>
>>> Well, I am afraid your patch breaks it and if you do not break
>>> it, the patch is reduced to nothing.
>>
>> Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
>> the refcount?
>>
>>>>> This patch does something that udev should do and in a
>>>>> questionable manner.
>>>>
>>>> IMO if the device doesn’t support runtime suspend, then it needs to be
>>>> disabled in kernel but not workaround in userspace.
>>>
>>> You switch it on from user space. Of course the kernel default
>>> must be safe, as you said. It already is.
>>
>> I’d also like to hear maintainers' opinion on this issue.
>
> I agree with Oliver. There is no formal requirement on what actions
> should cause a mouse to generate a remote wakeup request. Some mice
> will do it when they are moved and some mice won't.
>
> If you don't like the way a particular mouse behaves then you should
> not allow it to go into runtime suspend. By default, the kernel
> prevents _all_ USB mice from being runtime suspended; the only way a
> mouse can be suspended is if some userspace program tells the kernel to
> allow it.
>
> It might be a udev script which does this, or a powertop setting, or
> something else. Regardless, what the kernel does is correct.
> Furthermore, the kernel has to accomodate users who don't mind pressing
> a mouse button to wake up their mice. For their sake, the kernel
> should not forbid a mouse from ever going into runtime suspend merely
> because it won't generate a wakeup request when it is moved.
True, if some users don’t mind clicking mouse button before using it then
we need to keep the current behavior.
Kai-Heng
>
> Alan Stern
^ permalink raw reply
* Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
From: Kai-Heng Feng @ 2019-08-24 16:22 UTC (permalink / raw)
To: Oliver Neukum
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, linux-usb
In-Reply-To: <1566481032.8347.44.camel@suse.com>
at 21:37, Oliver Neukum <oneukum@suse.com> wrote:
> Am Donnerstag, den 22.08.2019, 21:23 +0800 schrieb Kai-Heng Feng:
>> at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
>>> Well, sort of. The USB spec merely states how to enter and exit
>>> a suspended state and that device state must not be lost.
>>> It does not tell you what a suspended device must be able to do.
>>
>> But shouldn’t remote wakeup signaling wakes the device up and let it exit
>> suspend state?
>
> Yes. Have you tested using a button? If they indeed do not work, then
> the device lies about supporting remote wakeup. That would warrant a
> quirk, but for remote wakeup.
Button click can wake the mouse up but not movement.
>
>> Or it’s okay to let the device be suspended when remote wakeup is needed
>> but broken?
>
> Again, the HID spec does not specify what should trigger a remote
> wakeup. Limiting this to mouse buttons but not movements is
> inconvinient, but not buggy.
Ok, I still find the behavior really surprising.
>
> This is indeed what Windows does. The device is suspended when the
> screen saver switches on. That we do not do that is a deficiency
> of X.
> To use runtime PM regularly you need an .ini file
Thanks for the explanation. I guess we can mimic the behavior in systemd or
upower.
>
>
>>> In other words, if on your system it is on, you need to look
>>> at udev, not the kernel.
>>
>> So if a device is broken when “power/control” is flipped by user, we
>> should
>> deal it at userspace? That doesn’t sound right to me.
>
> If it is broken, as in crashing we could talk about it. If it merely
> does not do what you want, then, yes, that is for user space to deal
> with.
Ok, I’ll take a look at userspace then.
>
>>> Well, no. Runtime PM is a trade off. You lose something if you use
>>> it. If it worked just as well as full power, you would never use
>>> full power, would you?
>>
>> I am not asking the suspended state to work as full power, but to
>> prevent a
>> device enters suspend state because of broken remote wakeup.
>
> What then would be the difference between suspended and active? A small
> delay in data transfer?
Non-operational but with wakeup capability and vise versa.
>
>>> Whether the loss of functionality or performance is worth the energy
>>> savings is a policy decision. Hence it belongs into udev.
>>> Ideally the kernel would tell user space what will work in a
>>> suspended state. Unfortunately HID does not provide support for that.
>>
>> I really don’t think “loss of functionally” belongs to policy decision.
>> But
>> that’s just my opinion.
>
> That is just what we do if, for example, you choose between the configs
> of a USB device or when you use authorization.
>
>> Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
>> the refcount?
>
> No, the refcount is good. If remote wakeup is totally broken, you need
> to introduce a quirk that will prevent the kernel from believing the
> device when it claims to support it.
Ok. I’ll see if it’s possible to mimic other OS under current Linux Desktop.
Kai-Heng
>
> Regards
> Oliver
^ permalink raw reply
* [PATCH v2] HID: fix error message in hid_open_report()
From: Michał Mirosław @ 2019-08-23 19:15 UTC (permalink / raw)
To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel
On HID report descriptor parsing error the code displays bogus
pointer instead of error offset (subtracts start=NULL from end).
Make the message more useful by displaying correct error offset
and include total buffer size for reference.
This was carried over from ancient times - "Fixed" commit just
promoted the message from DEBUG to ERROR.
Cc: stable@vger.kernel.org
Fixes: 8c3d52fc393b ("HID: make parser more verbose about parsing errors by default")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: fixed printf() warning spotted by Jiri Kosina <jikos@kernel.org>
* against v5.2.9
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/hid/hid-core.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 210b81a56e1a..3af76624e4aa 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1139,6 +1139,7 @@ int hid_open_report(struct hid_device *device)
__u8 *start;
__u8 *buf;
__u8 *end;
+ __u8 *next;
int ret;
static int (*dispatch_type[])(struct hid_parser *parser,
struct hid_item *item) = {
@@ -1192,7 +1193,8 @@ int hid_open_report(struct hid_device *device)
device->collection_size = HID_DEFAULT_NUM_COLLECTIONS;
ret = -EINVAL;
- while ((start = fetch_item(start, end, &item)) != NULL) {
+ while ((next = fetch_item(start, end, &item)) != NULL) {
+ start = next;
if (item.format != HID_ITEM_FORMAT_SHORT) {
hid_err(device, "unexpected long global item\n");
@@ -1230,7 +1232,8 @@ int hid_open_report(struct hid_device *device)
}
}
- hid_err(device, "item fetching failed at offset %d\n", (int)(end - start));
+ hid_err(device, "item fetching failed at offset %u/%u\n",
+ size - (unsigned int)(end - start), size);
err:
kfree(parser->collection_stack);
alloc_err:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v4] hid-logitech-hidpp: read battery voltage from newer devices
From: Benjamin Tissoires @ 2019-08-23 16:15 UTC (permalink / raw)
To: Pedro Vanzella
Cc: open list:HID CORE LAYER, Filipe Laíns, Jiri Kosina, lkml
In-Reply-To: <20190823154952.7525-1-pedro@pedrovanzella.com>
Hi Pedro,
On Fri, Aug 23, 2019 at 5:51 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>
> Newer Logitech mice report their battery voltage through feature 0x1001
> instead of the battery levels through feature 0x1000.
>
> When the device is brought up and we try to query the battery, figure
> out if it supports the old or the new feature. If it supports the new
> feature, record the feature index and read the battery voltage and
> its status.
>
> If everything went correctly, record the fact that we're capable
> of querying battery voltage.
>
> Note that the protocol only gives us the current voltage in millivolts.
>
> Like we do for other ways of interacting with the battery for other
> devices, refresh the battery status and notify the power supply
> subsystem of the changes in voltage and status.
>
> Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
I still have comments, please see inline:
> ---
> drivers/hid/hid-logitech-hidpp.c | 140 ++++++++++++++++++++++++++++++-
> 1 file changed, 138 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 0179f7ed77e5..5c9c3133d2ae 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -87,6 +87,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> #define HIDPP_CAPABILITY_HIDPP20_BATTERY BIT(1)
> #define HIDPP_CAPABILITY_BATTERY_MILEAGE BIT(2)
> #define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS BIT(3)
> +#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4)
>
> /*
> * There are two hidpp protocols in use, the first version hidpp10 is known
> @@ -135,12 +136,14 @@ struct hidpp_report {
> struct hidpp_battery {
> u8 feature_index;
> u8 solar_feature_index;
> + u8 voltage_feature_index;
> struct power_supply_desc desc;
> struct power_supply *ps;
> char name[64];
> int status;
> int capacity;
> int level;
> + int voltage; /* in millivolts */
> bool online;
> };
>
> @@ -1219,6 +1222,122 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
> return 0;
> }
>
> +/* -------------------------------------------------------------------------- */
> +/* 0x1001: Battery voltage */
> +/* -------------------------------------------------------------------------- */
> +
> +#define HIDPP_PAGE_BATTERY_VOLTAGE 0x1001
> +
> +#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00
> +
> +static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage)
> +{
> + int status;
> +
> + switch (data[2]) {
> + case 0x00: /* discharging */
> + status = POWER_SUPPLY_STATUS_DISCHARGING;
> + break;
> + case 0x10: /* wireless charging */
> + case 0x80: /* charging */
> + status = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> + case 0x81: /* fully charged */
> + status = POWER_SUPPLY_STATUS_FULL;
> + break;
> + default:
> + status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + }
> +
> + *voltage = (data[0] << 8) + data[1];
If I am not wrong, you can use get_unaligned_be16 here instead
> +
> + return status;
> +}
> +
> +static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
> + u8 feature_index,
> + int *status, int *voltage)
> +{
> + struct hidpp_report response;
> + int ret;
> + u8 *params = (u8 *)response.fap.params;
> +
> + ret = hidpp_send_fap_command_sync(hidpp, feature_index,
> + CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE,
> + NULL, 0, &response);
> +
> + if (ret > 0) {
> + hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
> + __func__, ret);
> + return -EPROTO;
> + }
> + if (ret)
> + return ret;
> +
> + hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_VOLTAGE;
> +
> + *status = hidpp20_battery_map_status_voltage(params, voltage);
> +
> + return 0;
> +}
> +
> +static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
> +{
> + u8 feature_type;
> + int ret;
> + int status, voltage;
> +
> + if (hidpp->battery.voltage_feature_index == 0xff) {
> + ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_BATTERY_VOLTAGE,
> + &hidpp->battery.voltage_feature_index,
> + &feature_type);
> + if (ret)
> + return ret;
> + }
> +
> + ret = hidpp20_battery_get_battery_voltage(hidpp,
> + hidpp->battery.voltage_feature_index,
> + &status, &voltage);
> +
> + if (ret)
> + return ret;
> +
> + hidpp->battery.status = status;
> + hidpp->battery.voltage = voltage;
> + hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> + return 0;
> +}
> +
> +static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
> + u8 *data, int size)
> +{
> + struct hidpp_report *report = (struct hidpp_report *)data;
> + int status, voltage;
> + bool changed;
> +
> + if (report->fap.feature_index != hidpp->battery.voltage_feature_index ||
> + report->fap.funcindex_clientid !=
> + EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
this should probably be defined in 0x1001 as
EVENT_BATTERY_VOLTAGE_STATUS_BROADCAST
> + return 0;
> +
> + status = hidpp20_battery_map_status_voltage(report->fap.params,
> + &voltage);
> +
> + hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> + changed = voltage != hidpp->battery.voltage ||
> + status != hidpp->battery.status;
> +
> + if (changed) {
There is no point in using an extra `changed` variable here, just
unwind the boolean operation above here.
> + hidpp->battery.voltage = voltage;
> + hidpp->battery.status = status;
> + if (hidpp->battery.ps)
> + power_supply_changed(hidpp->battery.ps);
> + }
> + return 0;
> +}
> +
> static enum power_supply_property hidpp_battery_props[] = {
> POWER_SUPPLY_PROP_ONLINE,
> POWER_SUPPLY_PROP_STATUS,
> @@ -1228,6 +1347,7 @@ static enum power_supply_property hidpp_battery_props[] = {
> POWER_SUPPLY_PROP_SERIAL_NUMBER,
> 0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */
> 0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */
> + 0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */
> };
>
> static int hidpp_battery_get_property(struct power_supply *psy,
> @@ -1265,6 +1385,9 @@ static int hidpp_battery_get_property(struct power_supply *psy,
> case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> val->strval = hidpp->hid_dev->uniq;
> break;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = hidpp->battery.voltage;
> + break;
> default:
> ret = -EINVAL;
> break;
> @@ -3083,6 +3206,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
> ret = hidpp_solar_battery_event(hidpp, data, size);
> if (ret != 0)
> return ret;
> + ret = hidpp20_battery_voltage_event(hidpp, data, size);
> + if (ret != 0)
> + return ret;
> }
>
> if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
> @@ -3204,12 +3330,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
>
> hidpp->battery.feature_index = 0xff;
> hidpp->battery.solar_feature_index = 0xff;
> + hidpp->battery.voltage_feature_index = 0xff;
>
> if (hidpp->protocol_major >= 2) {
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
> ret = hidpp_solar_request_battery_event(hidpp);
> - else
> + else {
> ret = hidpp20_query_battery_info(hidpp);
> + if (ret)
> + ret = hidpp20_query_battery_voltage_info(hidpp);
> + }
we should probably do the other way around: first test
hidpp20_query_battery_voltage_info and then go for the generic one, so
that the error returned will be the one from the most common feature,
and you will keep existing behaviour.
>
> if (ret)
> return ret;
> @@ -3234,7 +3364,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
> if (!battery_props)
> return -ENOMEM;
>
> - num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2;
> + num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
>
> if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE)
> battery_props[num_battery_props++] =
> @@ -3244,6 +3374,10 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
> battery_props[num_battery_props++] =
> POWER_SUPPLY_PROP_CAPACITY_LEVEL;
>
> + if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
> + battery_props[num_battery_props++] =
> + POWER_SUPPLY_PROP_VOLTAGE_NOW;
> +
> battery = &hidpp->battery;
>
> n = atomic_inc_return(&battery_no) - 1;
> @@ -3408,6 +3542,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> hidpp10_query_battery_status(hidpp);
> } else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
> hidpp20_query_battery_info(hidpp);
> + if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
> + hidpp20_query_battery_voltage_info(hidpp);
I'd put the test above `hidpp20_query_battery_info(hidpp);` and would
add an else statement to fetch the battery info
> }
> if (hidpp->battery.ps)
> power_supply_changed(hidpp->battery.ps);
> --
> 2.23.0
>
Cheers,
Benjamin
^ permalink raw reply
* [PATCH v4] hid-logitech-hidpp: read battery voltage from newer devices
From: Pedro Vanzella @ 2019-08-23 15:49 UTC (permalink / raw)
To: linux-input
Cc: lains, Pedro Vanzella, Jiri Kosina, Benjamin Tissoires,
linux-kernel
Newer Logitech mice report their battery voltage through feature 0x1001
instead of the battery levels through feature 0x1000.
When the device is brought up and we try to query the battery, figure
out if it supports the old or the new feature. If it supports the new
feature, record the feature index and read the battery voltage and
its status.
If everything went correctly, record the fact that we're capable
of querying battery voltage.
Note that the protocol only gives us the current voltage in millivolts.
Like we do for other ways of interacting with the battery for other
devices, refresh the battery status and notify the power supply
subsystem of the changes in voltage and status.
Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
drivers/hid/hid-logitech-hidpp.c | 140 ++++++++++++++++++++++++++++++-
1 file changed, 138 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..5c9c3133d2ae 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -87,6 +87,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_HIDPP20_BATTERY BIT(1)
#define HIDPP_CAPABILITY_BATTERY_MILEAGE BIT(2)
#define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS BIT(3)
+#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4)
/*
* There are two hidpp protocols in use, the first version hidpp10 is known
@@ -135,12 +136,14 @@ struct hidpp_report {
struct hidpp_battery {
u8 feature_index;
u8 solar_feature_index;
+ u8 voltage_feature_index;
struct power_supply_desc desc;
struct power_supply *ps;
char name[64];
int status;
int capacity;
int level;
+ int voltage; /* in millivolts */
bool online;
};
@@ -1219,6 +1222,122 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp,
return 0;
}
+/* -------------------------------------------------------------------------- */
+/* 0x1001: Battery voltage */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_BATTERY_VOLTAGE 0x1001
+
+#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00
+
+static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage)
+{
+ int status;
+
+ switch (data[2]) {
+ case 0x00: /* discharging */
+ status = POWER_SUPPLY_STATUS_DISCHARGING;
+ break;
+ case 0x10: /* wireless charging */
+ case 0x80: /* charging */
+ status = POWER_SUPPLY_STATUS_CHARGING;
+ break;
+ case 0x81: /* fully charged */
+ status = POWER_SUPPLY_STATUS_FULL;
+ break;
+ default:
+ status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ }
+
+ *voltage = (data[0] << 8) + data[1];
+
+ return status;
+}
+
+static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
+ u8 feature_index,
+ int *status, int *voltage)
+{
+ struct hidpp_report response;
+ int ret;
+ u8 *params = (u8 *)response.fap.params;
+
+ ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+ CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE,
+ NULL, 0, &response);
+
+ if (ret > 0) {
+ hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return -EPROTO;
+ }
+ if (ret)
+ return ret;
+
+ hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_VOLTAGE;
+
+ *status = hidpp20_battery_map_status_voltage(params, voltage);
+
+ return 0;
+}
+
+static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
+{
+ u8 feature_type;
+ int ret;
+ int status, voltage;
+
+ if (hidpp->battery.voltage_feature_index == 0xff) {
+ ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_BATTERY_VOLTAGE,
+ &hidpp->battery.voltage_feature_index,
+ &feature_type);
+ if (ret)
+ return ret;
+ }
+
+ ret = hidpp20_battery_get_battery_voltage(hidpp,
+ hidpp->battery.voltage_feature_index,
+ &status, &voltage);
+
+ if (ret)
+ return ret;
+
+ hidpp->battery.status = status;
+ hidpp->battery.voltage = voltage;
+ hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+ return 0;
+}
+
+static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
+ u8 *data, int size)
+{
+ struct hidpp_report *report = (struct hidpp_report *)data;
+ int status, voltage;
+ bool changed;
+
+ if (report->fap.feature_index != hidpp->battery.voltage_feature_index ||
+ report->fap.funcindex_clientid !=
+ EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
+ return 0;
+
+ status = hidpp20_battery_map_status_voltage(report->fap.params,
+ &voltage);
+
+ hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+ changed = voltage != hidpp->battery.voltage ||
+ status != hidpp->battery.status;
+
+ if (changed) {
+ hidpp->battery.voltage = voltage;
+ hidpp->battery.status = status;
+ if (hidpp->battery.ps)
+ power_supply_changed(hidpp->battery.ps);
+ }
+ return 0;
+}
+
static enum power_supply_property hidpp_battery_props[] = {
POWER_SUPPLY_PROP_ONLINE,
POWER_SUPPLY_PROP_STATUS,
@@ -1228,6 +1347,7 @@ static enum power_supply_property hidpp_battery_props[] = {
POWER_SUPPLY_PROP_SERIAL_NUMBER,
0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */
0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */
+ 0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */
};
static int hidpp_battery_get_property(struct power_supply *psy,
@@ -1265,6 +1385,9 @@ static int hidpp_battery_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_SERIAL_NUMBER:
val->strval = hidpp->hid_dev->uniq;
break;
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ val->intval = hidpp->battery.voltage;
+ break;
default:
ret = -EINVAL;
break;
@@ -3083,6 +3206,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
ret = hidpp_solar_battery_event(hidpp, data, size);
if (ret != 0)
return ret;
+ ret = hidpp20_battery_voltage_event(hidpp, data, size);
+ if (ret != 0)
+ return ret;
}
if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
@@ -3204,12 +3330,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
hidpp->battery.feature_index = 0xff;
hidpp->battery.solar_feature_index = 0xff;
+ hidpp->battery.voltage_feature_index = 0xff;
if (hidpp->protocol_major >= 2) {
if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
ret = hidpp_solar_request_battery_event(hidpp);
- else
+ else {
ret = hidpp20_query_battery_info(hidpp);
+ if (ret)
+ ret = hidpp20_query_battery_voltage_info(hidpp);
+ }
if (ret)
return ret;
@@ -3234,7 +3364,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
if (!battery_props)
return -ENOMEM;
- num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2;
+ num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE)
battery_props[num_battery_props++] =
@@ -3244,6 +3374,10 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
battery_props[num_battery_props++] =
POWER_SUPPLY_PROP_CAPACITY_LEVEL;
+ if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+ battery_props[num_battery_props++] =
+ POWER_SUPPLY_PROP_VOLTAGE_NOW;
+
battery = &hidpp->battery;
n = atomic_inc_return(&battery_no) - 1;
@@ -3408,6 +3542,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
hidpp10_query_battery_status(hidpp);
} else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) {
hidpp20_query_battery_info(hidpp);
+ if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+ hidpp20_query_battery_voltage_info(hidpp);
}
if (hidpp->battery.ps)
power_supply_changed(hidpp->battery.ps);
--
2.23.0
^ permalink raw reply related
* Re: [Resubmit] Read battery voltage from Logitech Gaming mice
From: Pedro Vanzella @ 2019-08-23 15:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: open list:HID CORE LAYER, Filipe Laíns, Jiri Kosina, lkml
In-Reply-To: <CAO-hwJ+=dAyFnUfiPSmiGpzYTj=9BPDdeKQOY7UoCOvwQ5CH7Q@mail.gmail.com>
On 8/23/19 10:32 AM, Benjamin Tissoires wrote:
> On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>>
>> Hi Benjamin,
>>
>> On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
>>> Hi Pedro,
>>>
>>> On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>>>>
>>>> Resumitting this after having rebased it against the latest changes.
>>>
>>> thanks for resubmitting. Sorry I wasn't able to provide feedback on
>>> the last revision
>>>
>>
>> No worries, I know how these things are.
>>
>>>>
>>>> The gaming line of Logitech devices doesn't use the old hidpp20
>>>> feature for battery level reporting. Instead, they report the
>>>> current voltage of the battery, in millivolts.
>>>>
>>>> This patch set handles this case by adding a quirk to the
>>>> devices we know to have this new feature, in both wired
>>>> and wireless mode.
>>>
>>> So the quirk is in the end a bad idea after all. I had some chats with
>>> Filipe that made me realize this.
>>
>> I actually resubmitted by Filipe's request, since the patches weren't
>> applying cleanly anymore. The idea was to apply these patches and in the
>> future refactor the code to use the feature discovery routines.
>>
>>> Re-reading our previous exchanges also made me understood why I wasn't
>>> happy with the initial submission: for every request the code was
>>> checking both features 0x1000 and 0x1001 when we can remember this
>>> once and for all during hidpp_initialize_battery().
>>
>> Yeah I wasn't too happy about this either, but since there was nothing
>> prohibiting some device to have both features enabled, I thought this
>> wasn't too horrible.
>
> I honestly don't think we will find one device that has both features
> enabled. It doesn't make sense as the "new" feature allows for fine
> tuning in the software, which would be moot if you also enable the
> percentage.
>
>>
>>>
>>> So I think we should remove the useless quirk in the end (bad idea
>>> from me, I concede), and instead during hidpp_initialize_battery() set
>>> the correct HIDPP_CAPABILITY_*.
>>> Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
>>> should rely on the 0x0001 feature to know which feature is available,
>>> but this should be implementation detail.
>>
>> I like the idea of calling 0x0001 once on device boot much better. I
>> think we could easily just fetch the location for the features we know
>> about and want to deal with once only. This also makes sure we support
>> every single device that supports this feature, so that is a huge plus.
>>
>> In fact, I think we should _not_ call 0x0001 on battery init, but only
>> call battery init _after_ we called 0x0001 and discovered either 0x1000
>> or 0x1001 (or the solar battery feature, or any other one that might
>> crop up in the feature).
>
> ack for that
>
>>
>>>
>>>>
>>>> This version of the patch set is better split, as well as adding the
>>>> quirk to make sure we don't needlessly probe every device connected.
>>>
>>> It is for sure easy to review, but doesn't make much sense in the end.
>>> I think we should squash all the patches together as you are just
>>> adding one feature in the driver, and it is a little bit disturbing to
>>> first add the quirk that has no use, then set up the structs when they
>>> are not used, and so on, so forth.
>>
>> You're right. My first instinct was to send a single patch. As much as I
>> tested this, I always feel like breaking the patch up post-facto will
>> break a git bisect in the future and everyone will hate me :P
>
> as long as the patches are compiling and are not breaking, git bisect
> will not be a problem. However, we might end up with the last one,
> which is not very explicit in what it does as it just enables the
> features implemented previously.
>
>>
>> So we (you, me and Filipe) should probably come up with an action plan
>> here. The way I see it there are two issues here: one is adding this
>> feature, and the other is refactoring to use feature discovery for all
>> features. There are advantages and disadvantages to doing one or another
>> first and we might want to discuss that.
>>
>> By merging this first (probably after I resubmit it as a single squashed
>> patch) we get to test it a bit better and have a usable feature sooner.
>> Plenty of people have been requesting this and there is plenty of stuff
>> that can be built on top of it, but only once this is actually merged I
>> think.
>>
>> On the other hand, by first refactoring the rest of the code to use
>> 0x0001 we avoid some rework on this patch. It should be minor, as most
>> functions here do all the heavy lifting after the initial feature
>> discovery, and are thus mostly independent from how that is done.
>>
>> I'm happy either way, so just let me know what you guys decide.
>
> I think we should merge your v3 squashed series with a slight
> autodetection during battery init, like the method you used in the v1.
> This would remove the quirk, but keep the straightforward commands
> when addressing battery data.
Alright, I rebased against for-5.4/logitech to make sure, squashed
everything and restored the detection code from v1, removing the quirk.
Tested and it works on both wired and wireless on my G900.
>
> Relying on 0x0001 should be done separately and can come in in a later
> patch IMO (unless you plan to work on it, in which case you can send
> both at once).
0x0001 is quite the task and I think Filipe already has a good plan to
tackle it, so I'll leave that for him.
>
> The problem I have with quirks, and that I explained to Filipe on IRC
> is that this is kernel ABI. Even if there is a very low chance we have
> someone using this, re-using the same drv_data bit in the future might
> break someone's device.
>
>>
>> If you guys (or anyone else reading this on the public list, really) has
>> any input - naming things being notoriosly hard, I'm actually happy with
>> nitpicking - I'd appreciate it. On that note, come to think of it, I'm
>> not 100% sure reporting the voltage in milivolts is the standard way. I
>> looked through the docs, but found no solid guideline. It was either
>> that or a float, so I think I made the right call here, but still.
>
> I am not sure either. Adding Bastien as he has a lot more experience in upower.
>
> But I am under the impression that the kernel part is more "try to
> deal with whatever the hardware provides, and deal with it in user
> space".
>
I'll submit v4 as a single patch in the next couple of minutes.
- Pedro
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox