* Re: [PATCH] Input: i8042 - add quirk for Lenovo ThinkPad T14 Gen 1
From: Jonathan Denose @ 2024-01-10 23:42 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Jonathan Denose, Hans de Goede, Huacai Chen,
Mattijs Korpershoek, Takashi Iwai, Werner Sembach, linux-kernel
In-Reply-To: <ZZ2eduF_h7lcBrSL@google.com>
Dmitry,
Sorry I forgot to reply all, so I'm resending my other email.
On Tue, Jan 9, 2024 at 1:28 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Jonathan,
>
> On Mon, Nov 27, 2023 at 10:38:57AM -0600, Jonathan Denose wrote:
> > Hi Dmitry
> >
> > On Fri, Nov 24, 2023 at 10:45 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Jonathan,
> > >
> > > On Mon, Sep 25, 2023 at 04:33:20PM -0500, Jonathan Denose wrote:
> > > > The ThinkPad T14 Gen 1 touchpad works fine except that clicking
> > > > and dragging by tapping the touchpad or depressing the touchpad
> > > > do not work. Disabling PNP for controller setting discovery enables
> > > > click and drag without negatively impacting other touchpad features.
> > >
> > > I would like to understand more on how enabling PnP discovery for i8042
> > > affects the touchpad. Do you see it using different interrupt or IO
> > > ports? What protocol does the touchpad use with/without PnP? If the
> > > protocol is the same, do you see difference in the ranges (pressure,
> > > etc) reported by the device?
> > >
> > > Thanks.
> > >
> > > --
> > > Dmitry
> >
> > Without PnP discovery the touchpad is using the SynPS/2 protocol, with
> > PnP discovery, the touchpad is using the rmi4 protocol. Since the
> > protocols are different, so are the ranges but let me know if you
> > still want to see them.
>
> Thank you for this information. So it is not PnP discovery that appears
> harmful in your case, but rather that legacy PS/2 mode appears to be
> working better than RMI4 for the device in question.
>
> I will note that the original enablement of RMI4 for T14 was done by
> Hans in [1]. Later T14 with AMD were added to the list of devices that
> should use RMI4 [2], however this was reverted in [3].
>
> Could you please tell me what exact device you are dealing with? What's
> it ACPI ID?
>
> [1] https://lore.kernel.org/all/20201005114919.371592-1-hdegoede@redhat.com/
> [2] https://lore.kernel.org/r/20220318113949.32722-1-snafu109@gmail.com
> [3] https://lore.kernel.org/r/20220920193936.8709-1-markpearson@lenovo.com
>
> Thanks.
>
> --
> Dmitry
Thanks for your reply!
I'm not 100% sure which of these is the ACPI ID, but from `udevadm
info -e` there's:
N: Name="Synaptics TM3471-020"
P: Phys=rmi4-00/input0
^ permalink raw reply
* Re: [PATCH v12 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Dmitry Torokhov @ 2024-01-11 1:20 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Conor Dooley, Thomas Weißschuh,
linux-kernel, Krzysztof Kozlowski, Conor Dooley, Rob Herring,
Krzysztof Kozlowski, Jeff LaBundy, linux-kernel-mentees
In-Reply-To: <20240106015111.882325-2-anshulusr@gmail.com>
Hi Anshul,
On Sat, Jan 06, 2024 at 07:21:00AM +0530, Anshul Dalal wrote:
> +
> + for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK,
> + BITS_PER_TYPE(SEESAW_BUTTON_MASK)) {
This is not really safe as it might not be aligned properly, and we can
potentially try to peek beyond the data element (even though we limit
how many bits we consider valid). I changed SEESAW_BUTTON_MASK to be
unsigned long.
I also dropped bunch of unneeded casts and applied, thank you.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v12 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Dmitry Torokhov @ 2024-01-11 1:20 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Conor Dooley, Thomas Weißschuh,
linux-kernel, Krzysztof Kozlowski, Conor Dooley, Rob Herring,
Krzysztof Kozlowski, Jeff LaBundy, linux-kernel-mentees
In-Reply-To: <20240106015111.882325-1-anshulusr@gmail.com>
On Sat, Jan 06, 2024 at 07:20:59AM +0530, Anshul Dalal wrote:
> Adds bindings for the Adafruit Seesaw Gamepad.
>
> The gamepad functions as an i2c device with the default address of 0x50
> and has an IRQ pin that can be enabled in the driver to allow for a rising
> edge trigger on each button press or joystick movement.
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: adafruit-seesaw - only report buttons that changed state
From: Dmitry Torokhov @ 2024-01-11 7:13 UTC (permalink / raw)
To: Anshul Dalal; +Cc: Thomas Weißschuh, linux-input, linux-kernel
If a button has not changed its state when we poll the device the
driver does not need to report it. While duplicate events will be
filtered out by the input core anyway we can do it very cheaply
directly in the driver.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/joystick/adafruit-seesaw.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
index 1b9279f024cc..5c775ca886a5 100644
--- a/drivers/input/joystick/adafruit-seesaw.c
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -56,7 +56,7 @@
#define SEESAW_GAMEPAD_POLL_MIN 8
#define SEESAW_GAMEPAD_POLL_MAX 32
-static const unsigned long SEESAW_BUTTON_MASK =
+static const u32 SEESAW_BUTTON_MASK =
BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) | BIT(SEESAW_BUTTON_X) |
BIT(SEESAW_BUTTON_Y) | BIT(SEESAW_BUTTON_START) |
BIT(SEESAW_BUTTON_SELECT);
@@ -64,6 +64,7 @@ static const unsigned long SEESAW_BUTTON_MASK =
struct seesaw_gamepad {
struct input_dev *input_dev;
struct i2c_client *i2c_client;
+ u32 button_state;
};
struct seesaw_data {
@@ -178,10 +179,20 @@ static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
return 0;
}
+static int seesaw_open(struct input_dev *input)
+{
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+
+ private->button_state = 0;
+
+ return 0;
+}
+
static void seesaw_poll(struct input_dev *input)
{
struct seesaw_gamepad *private = input_get_drvdata(input);
struct seesaw_data data;
+ unsigned long changed;
int err, i;
err = seesaw_read_data(private->i2c_client, &data);
@@ -194,8 +205,11 @@ static void seesaw_poll(struct input_dev *input)
input_report_abs(input, ABS_X, data.x);
input_report_abs(input, ABS_Y, data.y);
- for_each_set_bit(i, &SEESAW_BUTTON_MASK,
- BITS_PER_TYPE(SEESAW_BUTTON_MASK)) {
+ data.button_state &= SEESAW_BUTTON_MASK;
+ changed = private->button_state ^ data.button_state;
+ private->button_state = data.button_state;
+
+ for_each_set_bit(i, &changed, fls(SEESAW_BUTTON_MASK)) {
if (!sparse_keymap_report_event(input, i,
data.button_state & BIT(i),
false))
@@ -253,6 +267,7 @@ static int seesaw_probe(struct i2c_client *client)
seesaw->input_dev->id.bustype = BUS_I2C;
seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
+ seesaw->input_dev->open = seesaw_open;
input_set_drvdata(seesaw->input_dev, seesaw);
input_set_abs_params(seesaw->input_dev, ABS_X,
0, SEESAW_JOYSTICK_MAX_AXIS,
--
2.43.0.275.g3460e3d667-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v14 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Dmitry Torokhov @ 2024-01-11 7:20 UTC (permalink / raw)
To: Neil Armstrong
Cc: linux-input, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20231221-topic-goodix-berlin-upstream-initial-v14-2-04459853b640@linaro.org>
Hi Neil,
On Thu, Dec 21, 2023 at 04:21:20PM +0100, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs.
Thank you very much for explaining how reading of additional contacts
and checksum works, it makes sense now.
I was a bit unhappy about number of times we copy/move the data over;
could you please try the patch below to see if the device still works
with it?
I also shortened some #defines and defines some additional structures.
Also as far as I can see not everything needs to be packed as the data
is naturally aligned on the word boundaries.
Thanks!
--
Dmitry
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
index 6aca57e6b5d6..d1f1c0474116 100644
--- a/drivers/input/touchscreen/goodix_berlin_core.c
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -39,19 +39,6 @@
#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
-#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
-#define GOODIX_BERLIN_STATUS_OFFSET 0
-#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
-
-#define GOODIX_BERLIN_BYTES_PER_POINT 8
-#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
-#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
-
-/* Read n finger events */
-#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
- (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
- GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
-
#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
@@ -71,6 +58,8 @@
#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+#define GOODIX_BERLIN_CHECKSUM_SIZE sizeof(u16)
+
struct goodix_berlin_fw_version {
u8 rom_pid[6];
u8 rom_vid[3];
@@ -81,7 +70,7 @@ struct goodix_berlin_fw_version {
u8 sensor_id;
u8 reserved[2];
__le16 checksum;
-} __packed;
+};
struct goodix_berlin_ic_info_version {
u8 info_customer_id;
@@ -147,13 +136,30 @@ struct goodix_berlin_ic_info_misc {
__le32 esd_addr;
} __packed;
-struct goodix_berlin_touch_data {
- u8 id;
- u8 unused;
+struct goodix_berlin_touch {
+ u8 status;
+ u8 reserved;
__le16 x;
__le16 y;
__le16 w;
-} __packed;
+};
+#define GOODIX_BERLIN_TOUCH_SIZE sizeof(struct goodix_berlin_touch)
+
+struct goodix_berlin_header {
+ u8 status;
+ u8 reserved1;
+ u8 request_type;
+ u8 reserved2[3];
+ __le16 checksum;
+};
+#define GOODIX_BERLIN_HEADER_SIZE sizeof(struct goodix_berlin_header)
+
+struct goodix_berlin_event {
+ struct goodix_berlin_header hdr;
+ /* The data below is u16/__le16 aligned */
+ u8 data[GOODIX_BERLIN_TOUCH_SIZE * GOODIX_BERLIN_MAX_TOUCH +
+ GOODIX_BERLIN_CHECKSUM_SIZE];
+};
struct goodix_berlin_core {
struct device *dev;
@@ -168,25 +174,25 @@ struct goodix_berlin_core {
/* Runtime parameters extracted from IC_INFO buffer */
u32 touch_data_addr;
+
+ struct goodix_berlin_event event;
};
static bool goodix_berlin_checksum_valid(const u8 *data, int size)
{
u32 cal_checksum = 0;
u16 r_checksum;
- u32 i;
+ int i;
- if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+ if (size < GOODIX_BERLIN_CHECKSUM_SIZE)
return false;
- for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+ for (i = 0; i < size - GOODIX_BERLIN_CHECKSUM_SIZE; i++)
cal_checksum += data[i];
- cal_checksum = FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK,
- cal_checksum);
r_checksum = get_unaligned_le16(&data[i]);
- return cal_checksum == r_checksum;
+ return (u16)cal_checksum == r_checksum;
}
static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
@@ -291,33 +297,30 @@ static void goodix_berlin_power_off(struct goodix_berlin_core *cd)
static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
{
- u8 buf[sizeof(struct goodix_berlin_fw_version)];
int error;
error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR,
- buf, sizeof(buf));
+ &cd->fw_version, sizeof(cd->fw_version));
if (error) {
dev_err(cd->dev, "error reading fw version, %d\n", error);
return error;
}
- if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+ if (!goodix_berlin_checksum_valid((u8 *)&cd->fw_version,
+ sizeof(cd->fw_version))) {
dev_err(cd->dev, "invalid fw version: checksum error\n");
return -EINVAL;
}
- memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
-
return 0;
}
/* Only extract necessary data for runtime */
-static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
- const u8 *data, u16 length)
+static int goodix_berlin_parse_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
{
- struct goodix_berlin_ic_info_misc misc;
+ struct goodix_berlin_ic_info_misc *misc;
unsigned int offset = 0;
- u8 param_num;
offset += sizeof(__le16); /* length */
offset += sizeof(struct goodix_berlin_ic_info_version);
@@ -325,49 +328,25 @@ static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
/* IC_INFO Parameters, variable width structure */
offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
-
- if (offset >= length)
- goto invalid_offset;
-
- param_num = data[offset++]; /* active_scan_rate_num */
-
- offset += param_num * sizeof(__le16);
-
- if (offset >= length)
- goto invalid_offset;
-
- param_num = data[offset++]; /* mutual_freq_num*/
-
- offset += param_num * sizeof(__le16);
-
- if (offset >= length)
- goto invalid_offset;
-
- param_num = data[offset++]; /* self_tx_freq_num */
-
- offset += param_num * sizeof(__le16);
-
- if (offset >= length)
- goto invalid_offset;
-
- param_num = data[offset++]; /* self_rx_freq_num */
-
- offset += param_num * sizeof(__le16);
-
if (offset >= length)
goto invalid_offset;
- param_num = data[offset++]; /* stylus_freq_num */
-
- offset += param_num * sizeof(__le16);
-
- if (offset + sizeof(misc) > length)
- goto invalid_offset;
-
- /* goodix_berlin_ic_info_misc */
- memcpy(&misc, &data[offset], sizeof(misc));
-
- cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+#define ADVANCE_LE16_PARAMS() \
+ do { \
+ u8 param_num = data[offset++]; \
+ offset += param_num * sizeof(__le16); \
+ if (offset >= length) \
+ goto invalid_offset; \
+ } while (0)
+ ADVANCE_LE16_PARAMS(); /* active_scan_rate_num */
+ ADVANCE_LE16_PARAMS(); /* mutual_freq_num*/
+ ADVANCE_LE16_PARAMS(); /* self_tx_freq_num */
+ ADVANCE_LE16_PARAMS(); /* self_rx_freq_num */
+ ADVANCE_LE16_PARAMS(); /* stylus_freq_num */
+#undef ADVANCE_LE16_PARAMS
+
+ misc = (struct goodix_berlin_ic_info_misc *)&data[offset];
+ cd->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
return 0;
@@ -419,7 +398,7 @@ static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
return -EINVAL;
}
- error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ error = goodix_berlin_parse_ic_info(cd, afe_data, length);
if (error)
return error;
@@ -432,20 +411,47 @@ static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
return 0;
}
-static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
- const void *buf, int touch_num)
+static int goodix_berlin_get_remaining_contacts(struct goodix_berlin_core *cd,
+ int n)
+{
+ size_t offset = 2 * GOODIX_BERLIN_TOUCH_SIZE +
+ GOODIX_BERLIN_CHECKSUM_SIZE;
+ u32 addr = cd->touch_data_addr + GOODIX_BERLIN_HEADER_SIZE + offset;
+ int error;
+
+ error = regmap_raw_read(cd->regmap, addr,
+ &cd->event.data[offset],
+ (n - 2) * GOODIX_BERLIN_TOUCH_SIZE);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+ error);
+ return error;
+ }
+
+ return 0;
+}
+
+static void goodix_berlin_report_state(struct goodix_berlin_core *cd, int n)
{
- const struct goodix_berlin_touch_data *touch_data = buf;
+ struct goodix_berlin_touch *touch_data =
+ (struct goodix_berlin_touch *)cd->event.data;
+ struct goodix_berlin_touch *t;
int i;
+ u8 type, id;
+
+ for (i = 0; i < n; i++) {
+ t = &touch_data[i];
- /* Report finger touches */
- for (i = 0; i < touch_num; i++) {
- unsigned int id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
- touch_data[i].id);
+ type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK, t->status);
+ if (type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ continue;
+ }
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, t->status);
if (id >= GOODIX_BERLIN_MAX_TOUCH) {
- dev_warn_ratelimited(cd->dev,
- "invalid finger id %d\n", id);
+ dev_warn_ratelimited(cd->dev, "invalid finger id %d\n", id);
continue;
}
@@ -453,69 +459,46 @@ static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
touchscreen_report_pos(cd->input_dev, &cd->props,
- __le16_to_cpu(touch_data[i].x),
- __le16_to_cpu(touch_data[i].y),
+ __le16_to_cpu(t->x), __le16_to_cpu(t->y),
true);
input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
- __le16_to_cpu(touch_data[i].w));
+ __le16_to_cpu(t->w));
}
input_mt_sync_frame(cd->input_dev);
input_sync(cd->input_dev);
}
-static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
- const void *pre_buf, u32 pre_buf_len)
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd)
{
- u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
- u8 point_type, touch_num;
+ u8 touch_num;
int error;
- /* copy pre-data to buffer */
- memcpy(buffer, pre_buf, pre_buf_len);
-
touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
- buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
-
+ cd->event.hdr.request_type);
if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
return;
}
- if (touch_num) {
- /* read more data if more than 2 touch events */
- if (unlikely(touch_num > 2)) {
- error = regmap_raw_read(cd->regmap,
- cd->touch_data_addr + pre_buf_len,
- &buffer[pre_buf_len],
- (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
- if (error) {
- dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
- error);
- return;
- }
- }
-
- point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
- buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
-
- if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
- point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
- dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ if (touch_num > 2) {
+ /* read additional contact data if more than 2 touch events */
+ error = goodix_berlin_get_remaining_contacts(cd, touch_num);
+ if (error)
return;
- }
+ }
- if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
- touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+ if (touch_num) {
+ int len = touch_num * GOODIX_BERLIN_TOUCH_SIZE +
+ GOODIX_BERLIN_CHECKSUM_SIZE;
+ if (!goodix_berlin_checksum_valid(cd->event.data, len)) {
dev_err(cd->dev, "touch data checksum error: %*ph\n",
- touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
- &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+ len, cd->event.data);
return;
}
}
- goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
- touch_num);
+ goodix_berlin_report_state(cd, touch_num);
}
static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
@@ -532,68 +515,72 @@ static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
static irqreturn_t goodix_berlin_irq(int irq, void *data)
{
struct goodix_berlin_core *cd = data;
- u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
- u8 event_status;
int error;
/*
* First, read buffer with space for 2 touch events:
- * - GOODIX_BERLIN_IRQ_EVENT_HEAD = 8 bytes
- * - GOODIX_BERLIN_BYTES_PER_POINT * 2 +
- * GOODIX_BERLIN_COOR_DATA_CHECKSUM = 18 bytes
+ * - GOODIX_BERLIN_HEADER_SIZE = 8 bytes
+ * - GOODIX_BERLIN_TOUCH_SIZE * 2 = 16 bytes
+ * - GOODIX_BERLIN_CHECKLSUM_SIZE = 2 bytes
* For a total of 26 bytes.
*
- * If only a single finger is reported, we will read 8 bytes more than needed:
- * - bytes 0-7: GOODIX_BERLIN_IRQ_EVENT_HEAD
+ * If only a single finger is reported, we will read 8 bytes more than
+ * needed:
+ * - bytes 0-7: Header (GOODIX_BERLIN_HEADER_SIZE)
* - bytes 8-15: Finger 0 Data
- * - bytes 16-17: GOODIX_BERLIN_COOR_DATA_CHECKSUM
+ * - bytes 24-25: Checksum
* - bytes 18-25: Unused 8 bytes
*
- * If 2 fingers are reported, we would have read the exact needed amount of
- * data and checkout would be at the end of the buffer:
- * - bytes 0-7: GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN
+ * If 2 fingers are reported, we would have read the exact needed
+ * amount of data and checksum would be at the end of the buffer:
+ * - bytes 0-7: Header (GOODIX_BERLIN_HEADER_SIZE)
* - bytes 8-15: Finger 0 Bytes 0-7
* - bytes 16-23: Finger 1 Bytes 0-7
- * - bytes 24-25: GOODIX_BERLIN_COOR_DATA_CHECKSUM
+ * - bytes 24-25: Checksum
*
- * If more than 2 fingers were reported, the "Checksum" bytes would in fact
- * contain part of the next finger data, and then we would complete the buffer
- * with the missing bytes, but by keeping the GOODIX_BERLIN_IRQ_READ_LEN(2)
- * size as base, it will still contain space for the final 2 bytes checksum:
- * - bytes 0-7: GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]
+ * If more than 2 fingers were reported, the "Checksum" bytes would
+ * in fact contain part of the next finger data, and then
+ * goodix_berlin_get_remaining_contacts() would complete the buffer
+ * with the missing bytes, including the trailing checksum.
+ * For example, if 3 fingers are reported, then we would do:
+ * Read 1:
+ * - bytes 0-7: Header (GOODIX_BERLIN_HEADER_SIZE)
* - bytes 8-15: Finger 0 Bytes 0-7
* - bytes 16-23: Finger 1 Bytes 0-7
* - bytes 24-25: Finger 2 Bytes 0-1
- * for example if 3 fingers are reported, (3 - 2) * 8 = 8 bytes would be read:
+ * Read 2 (with length of (3 - 2) * 8 = 8 bytes):
* - bytes 26-31: Finger 2 Bytes 2-7
- * - bytes 32-33: GOODIX_BERLIN_COOR_DATA_CHECKSUM
+ * - bytes 32-33: Checksum
*/
- error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
- GOODIX_BERLIN_IRQ_READ_LEN(2));
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr,
+ &cd->event,
+ GOODIX_BERLIN_HEADER_SIZE +
+ 2 * GOODIX_BERLIN_TOUCH_SIZE +
+ GOODIX_BERLIN_CHECKSUM_SIZE);
if (error) {
dev_warn_ratelimited(cd->dev,
"failed get event head data: %d\n", error);
- return IRQ_HANDLED;
+ goto out;
}
- if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
- return IRQ_HANDLED;
+ if (cd->event.hdr.status == 0)
+ goto out;
- if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+ if (!goodix_berlin_checksum_valid((u8 *)&cd->event.hdr,
+ GOODIX_BERLIN_HEADER_SIZE)) {
dev_warn_ratelimited(cd->dev,
"touch head checksum error: %*ph\n",
- GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
- return IRQ_HANDLED;
+ (int)GOODIX_BERLIN_HEADER_SIZE,
+ &cd->event.hdr);
+ // FIXME: should we clear the status?
+ goto out;
}
- event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
-
- if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
- goodix_berlin_touch_handler(cd, buf,
- GOODIX_BERLIN_IRQ_READ_LEN(2));
+ if (cd->event.hdr.status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd);
- if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
- switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+ if (cd->event.hdr.status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (cd->event.hdr.request_type) {
case GOODIX_BERLIN_REQUEST_CODE_RESET:
if (cd->reset_gpio)
goodix_berlin_request_handle_reset(cd);
@@ -601,13 +588,14 @@ static irqreturn_t goodix_berlin_irq(int irq, void *data)
default:
dev_warn(cd->dev, "unsupported request code 0x%x\n",
- buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+ cd->event.hdr.request_type);
}
}
/* Clear up status field */
regmap_write(cd->regmap, cd->touch_data_addr, 0);
+out:
return IRQ_HANDLED;
}
^ permalink raw reply related
* Re: [PATCH] vt: remove superfluous CONFIG_HW_CONSOLE
From: Dmitry Torokhov @ 2024-01-11 7:21 UTC (permalink / raw)
To: Lukas Bulwahn
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Geert Uytterhoeven,
Helge Deller, Andrew Morton, linux-input, linux-m68k, linux-fbdev,
dri-devel, kernel-janitors, linux-kernel
In-Reply-To: <20240108134102.601-1-lukas.bulwahn@gmail.com>
On Mon, Jan 08, 2024 at 02:41:02PM +0100, Lukas Bulwahn wrote:
> The config HW_CONSOLE is always identical to the config VT and is not
> visible in the kernel's build menuconfig. So, CONFIG_HW_CONSOLE is
> redundant.
>
> Replace all references to CONFIG_HW_CONSOLE with CONFIG_VT and remove
> CONFIG_HW_CONSOLE.
>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> I think this patch is best picked up by Greg rather than splitting it
> in smaller pieces for m68k, amiga keyboard, fbdev etc.
>
> Greg, if that is fine, could you pick this for the next merge window?
>
> I was also considering to rename config VT_HW_CONSOLE_BINDING to
> VT_CONSOLE_BINDING, as the dependency is on VT, not HW_CONSOLE, but
> at the moment, that seemed more churn than value of clarification.
>
> arch/m68k/amiga/config.c | 2 +-
> drivers/input/keyboard/amikbd.c | 6 +++---
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver
From: Dmitry Torokhov @ 2024-01-11 7:28 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Fred Treven, Ben Bright, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Simon Trimmer, Charles Keepax,
Richard Fitzgerald, Lee Jones, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, James Schulman, David Rhodes,
Alexandre Belloni, Peng Fan, Jeff LaBundy, Sebastian Reichel,
Jacky Bai, Weidong Wang, Arnd Bergmann, Herve Codina, Shuming Fan,
Shenghao Ding, Ryan Lee, Linus Walleij,
open list:CIRRUS LOGIC HAPTIC DRIVERS,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list,
open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
moderated list:CIRRUS LOGIC AUDIO CODEC DRIVERS
In-Reply-To: <42A07166-6569-4872-B5E0-6D71C6F3656D@cirrus.com>
On Wed, Jan 10, 2024 at 02:36:55PM +0000, James Ogletree wrote:
>
> > On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
> >> Hi Dmitry,
> >>
> >> Thank you for your excellent review. Just a few questions.
> >>
> >>> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>>
> >>> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
> >>>> +
> >>>> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
> >>>> + if (!info->add_effect.u.periodic.custom_data)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
> >>>> + effect->u.periodic.custom_data, sizeof(s16) * len)) {
> >>>> + info->add_error = -EFAULT;
> >>>> + goto out_free;
> >>>> + }
> >>>> +
> >>>> + queue_work(info->vibe_wq, &info->add_work);
> >>>> + flush_work(&info->add_work);
> >>>
> >>> I do not understand the need of scheduling a work here. You are
> >>> obviously in a sleeping context (otherwise you would not be able to
> >>> execute flush_work()) so you should be able to upload the effect right
> >>> here.
> >>
> >> Scheduling work here is to ensure its ordering with “playback" worker
> >> items, which themselves are called in atomic context and so need
> >> deferred work. I think this explains why we need a workqueue as well,
> >> but please correct me.
> >>
> >>>
> >>>> +
> >>>> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
> >>>> +{
> >>>> + struct vibra_info *info = input_get_drvdata(dev);
> >>>> +
> >>>> + if (val > 0) {
> >>>
> >>> value is supposed to signal how many times an effect should be repeated.
> >>> It looks like you are not handling this at all.
> >>
> >> For playbacks, we mandate that the input_event value field is set to either 1
> >
> > I am sorry, who is "we"?
>
> Just a royal “I”. Apologies, no claim to authority intended here. :)
>
> >
> >> or 0 to command either a start playback or stop playback respectively.
> >> Values other than that should be rejected, so in the next version I will fix this
> >> to explicitly check for 1 or 0.
> >
> > No, please implement the API properly.
>
> Ack.
>
> >
> >>
> >>>
> >>>> + info->start_effect = &dev->ff->effects[effect_id];
> >>>> + queue_work(info->vibe_wq, &info->vibe_start_work);
> >>>
> >>> The API allows playback of several effects at once, the way you have it
> >>> done here if multiple requests come at same time only one will be
> >>> handled.
> >>
> >> I think I may need some clarification on this point. Why would concurrent
> >> start/stop playback commands get dropped? It seems they would all be
> >> added to the workqueue and executed eventually.
> >
> > You only have one instance of vibe_start_work, as well as only one
> > "slot" to hold the effect you want to start. So if you issue 2 request
> > back to back to play effect 1 and 2 you are likely to end with
> > info->start_effect == 2 and that is what vibe_start_work handler will
> > observe, effectively dropping request to play effect 1 on the floor.
>
> Understood, ack.
>
> >
> >>
> >>>
> >>>> + } else {
> >>>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
> >>>
> >>> Which effect are you stopping? All of them? You need to stop a
> >>> particular one.
> >>
> >> Our implementation of “stop” stops all effects in flight which is intended.
> >> That is probably unusual so I will add a comment here in the next
> >> version.
> >
> > Again, please implement the driver properly, not define your own
> > carveouts for the expected behavior.
>
> Ack, and a clarification question: the device is not actually able to
> play multiple effects at once. In that case, does stopping a specific
> effect entail just cancelling an effect in the queue?
In this case I believe the device should declare maximum number of
effects as 1. Userspace is supposed to determine maximum number of
simultaneously playable effects by issuing EVIOCGEFFECTS ioctl on the
corresponding event device.
Thanks.
--
Dmitry
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS 52c4e5985a730796a3fa555b83b404708b960f9d
From: kernel test robot @ 2024-01-11 9:20 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 52c4e5985a730796a3fa555b83b404708b960f9d Input: driver for Adafruit Seesaw Gamepad
elapsed time: 1446m
configs tested: 193
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allmodconfig gcc
arc allnoconfig gcc
arc allyesconfig gcc
arc defconfig gcc
arc haps_hs_smp_defconfig gcc
arc randconfig-001-20240111 gcc
arc randconfig-002-20240111 gcc
arm allmodconfig gcc
arm allnoconfig gcc
arm allyesconfig gcc
arm aspeed_g4_defconfig clang
arm collie_defconfig clang
arm defconfig clang
arm multi_v4t_defconfig gcc
arm omap1_defconfig clang
arm randconfig-001-20240111 gcc
arm randconfig-002-20240111 gcc
arm randconfig-003-20240111 gcc
arm randconfig-004-20240111 gcc
arm s5pv210_defconfig clang
arm socfpga_defconfig clang
arm spitz_defconfig clang
arm64 allmodconfig clang
arm64 allnoconfig gcc
arm64 defconfig gcc
arm64 randconfig-001-20240111 gcc
arm64 randconfig-002-20240111 gcc
arm64 randconfig-003-20240111 gcc
arm64 randconfig-004-20240111 gcc
csky allmodconfig gcc
csky allnoconfig gcc
csky allyesconfig gcc
csky defconfig gcc
csky randconfig-001-20240111 gcc
csky randconfig-002-20240111 gcc
hexagon allmodconfig clang
hexagon allnoconfig clang
hexagon allyesconfig clang
hexagon defconfig clang
hexagon randconfig-001-20240111 clang
hexagon randconfig-002-20240111 clang
i386 allmodconfig clang
i386 allnoconfig clang
i386 allyesconfig clang
i386 buildonly-randconfig-001-20240111 gcc
i386 buildonly-randconfig-002-20240111 gcc
i386 buildonly-randconfig-003-20240111 gcc
i386 buildonly-randconfig-004-20240111 gcc
i386 buildonly-randconfig-005-20240111 gcc
i386 buildonly-randconfig-006-20240111 gcc
i386 defconfig gcc
i386 randconfig-001-20240111 gcc
i386 randconfig-002-20240111 gcc
i386 randconfig-003-20240111 gcc
i386 randconfig-004-20240111 gcc
i386 randconfig-005-20240111 gcc
i386 randconfig-006-20240111 gcc
i386 randconfig-011-20240111 clang
i386 randconfig-012-20240111 clang
i386 randconfig-013-20240111 clang
i386 randconfig-014-20240111 clang
i386 randconfig-015-20240111 clang
i386 randconfig-016-20240111 clang
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
loongarch randconfig-001-20240111 gcc
loongarch randconfig-002-20240111 gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k atari_defconfig gcc
m68k defconfig gcc
m68k m5249evb_defconfig gcc
m68k mac_defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allnoconfig clang
mips allyesconfig gcc
mips bcm47xx_defconfig gcc
mips cu1000-neo_defconfig clang
mips ip27_defconfig gcc
mips malta_defconfig clang
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
nios2 randconfig-001-20240111 gcc
nios2 randconfig-002-20240111 gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
openrisc or1klitex_defconfig gcc
openrisc simple_smp_defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-001-20240111 gcc
parisc randconfig-002-20240111 gcc
parisc64 defconfig gcc
powerpc acadia_defconfig clang
powerpc allmodconfig clang
powerpc allnoconfig gcc
powerpc allyesconfig clang
powerpc icon_defconfig clang
powerpc mpc832x_rdb_defconfig clang
powerpc mpc836x_rdk_defconfig clang
powerpc ps3_defconfig gcc
powerpc randconfig-001-20240111 gcc
powerpc randconfig-002-20240111 gcc
powerpc randconfig-003-20240111 gcc
powerpc redwood_defconfig gcc
powerpc64 randconfig-001-20240111 gcc
powerpc64 randconfig-002-20240111 gcc
powerpc64 randconfig-003-20240111 gcc
riscv allmodconfig gcc
riscv allnoconfig clang
riscv allyesconfig gcc
riscv defconfig gcc
riscv nommu_k210_defconfig gcc
riscv randconfig-001-20240111 gcc
riscv randconfig-002-20240111 gcc
riscv rv32_defconfig clang
s390 alldefconfig clang
s390 allmodconfig gcc
s390 allnoconfig gcc
s390 allyesconfig gcc
s390 defconfig gcc
s390 randconfig-001-20240111 clang
s390 randconfig-002-20240111 clang
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh defconfig gcc
sh edosk7760_defconfig gcc
sh magicpanelr2_defconfig gcc
sh randconfig-001-20240111 gcc
sh randconfig-002-20240111 gcc
sh rts7751r2d1_defconfig gcc
sh se7722_defconfig gcc
sh sh2007_defconfig gcc
sh sh7763rdp_defconfig gcc
sparc allmodconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
sparc64 randconfig-001-20240111 gcc
sparc64 randconfig-002-20240111 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um randconfig-001-20240111 gcc
um randconfig-002-20240111 gcc
um x86_64_defconfig gcc
x86_64 allnoconfig gcc
x86_64 allyesconfig clang
x86_64 buildonly-randconfig-001-20240111 gcc
x86_64 buildonly-randconfig-002-20240111 gcc
x86_64 buildonly-randconfig-003-20240111 gcc
x86_64 buildonly-randconfig-004-20240111 gcc
x86_64 buildonly-randconfig-005-20240111 gcc
x86_64 buildonly-randconfig-006-20240111 gcc
x86_64 defconfig gcc
x86_64 randconfig-001-20240111 clang
x86_64 randconfig-002-20240111 clang
x86_64 randconfig-003-20240111 clang
x86_64 randconfig-004-20240111 clang
x86_64 randconfig-005-20240111 clang
x86_64 randconfig-006-20240111 clang
x86_64 randconfig-011-20240111 gcc
x86_64 randconfig-012-20240111 gcc
x86_64 randconfig-013-20240111 gcc
x86_64 randconfig-014-20240111 gcc
x86_64 randconfig-015-20240111 gcc
x86_64 randconfig-016-20240111 gcc
x86_64 randconfig-071-20240111 gcc
x86_64 randconfig-072-20240111 gcc
x86_64 randconfig-073-20240111 gcc
x86_64 randconfig-074-20240111 gcc
x86_64 randconfig-075-20240111 gcc
x86_64 randconfig-076-20240111 gcc
x86_64 rhel-8.3-rust clang
xtensa allnoconfig gcc
xtensa randconfig-001-20240111 gcc
xtensa randconfig-002-20240111 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH] Input: i8042 - add quirk for Lenovo ThinkPad T14 Gen 1
From: Hans de Goede @ 2024-01-11 10:48 UTC (permalink / raw)
To: Jonathan Denose, Dmitry Torokhov
Cc: linux-input, Jonathan Denose, Huacai Chen, Mattijs Korpershoek,
Takashi Iwai, Werner Sembach, linux-kernel
In-Reply-To: <CALNJtpWr0h+r3=R2scxyCGzgbZ1C6FiYrCGWW1_aSVPBdmNc3Q@mail.gmail.com>
Hi Jonathan,
On 1/11/24 00:42, Jonathan Denose wrote:
> Dmitry,
>
> Sorry I forgot to reply all, so I'm resending my other email.
>
> On Tue, Jan 9, 2024 at 1:28 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> Hi Jonathan,
>>
>> On Mon, Nov 27, 2023 at 10:38:57AM -0600, Jonathan Denose wrote:
>>> Hi Dmitry
>>>
>>> On Fri, Nov 24, 2023 at 10:45 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> Hi Jonathan,
>>>>
>>>> On Mon, Sep 25, 2023 at 04:33:20PM -0500, Jonathan Denose wrote:
>>>>> The ThinkPad T14 Gen 1 touchpad works fine except that clicking
>>>>> and dragging by tapping the touchpad or depressing the touchpad
>>>>> do not work. Disabling PNP for controller setting discovery enables
>>>>> click and drag without negatively impacting other touchpad features.
>>>>
>>>> I would like to understand more on how enabling PnP discovery for i8042
>>>> affects the touchpad. Do you see it using different interrupt or IO
>>>> ports? What protocol does the touchpad use with/without PnP? If the
>>>> protocol is the same, do you see difference in the ranges (pressure,
>>>> etc) reported by the device?
>>>>
>>>> Thanks.
>>>>
>>>> --
>>>> Dmitry
>>>
>>> Without PnP discovery the touchpad is using the SynPS/2 protocol, with
>>> PnP discovery, the touchpad is using the rmi4 protocol. Since the
>>> protocols are different, so are the ranges but let me know if you
>>> still want to see them.
>>
>> Thank you for this information. So it is not PnP discovery that appears
>> harmful in your case, but rather that legacy PS/2 mode appears to be
>> working better than RMI4 for the device in question.
>>
>> I will note that the original enablement of RMI4 for T14 was done by
>> Hans in [1]. Later T14 with AMD were added to the list of devices that
>> should use RMI4 [2], however this was reverted in [3].
>>
>> Could you please tell me what exact device you are dealing with? What's
>> it ACPI ID?
>>
>> [1] https://lore.kernel.org/all/20201005114919.371592-1-hdegoede@redhat.com/
>> [2] https://lore.kernel.org/r/20220318113949.32722-1-snafu109@gmail.com
>> [3] https://lore.kernel.org/r/20220920193936.8709-1-markpearson@lenovo.com
>>
>> Thanks.
>>
>> --
>> Dmitry
>
> Thanks for your reply!
>
> I'm not 100% sure which of these is the ACPI ID, but from `udevadm
> info -e` there's:
> N: Name="Synaptics TM3471-020"
> P: Phys=rmi4-00/input0
To get the ACPI ID you need to run e.g. :``
cat /sys/bus/serio/devices/serio1/firmware_id
After reading the original bug report again I take back my
Reviewed-by and I'm tending towards a nack for this.
Jonathan upon re-reading things I think that your problem
is more a case of user space mis-configuration then
a kernel problem.
You mention both tap-n-drag not working as well as click+drag
not working.
tap-n-drag is purely done in userspace and typically only
works if tap-to-click is enabled in the touchpad configuration
of your desktop environment.
Click + drag requires you to use the bottom of the touchpad
(the only part which actually clicks) as if there still were
2 physical buttons there and then click the touchpad down
with 1 finger till it clicks and then drags with another
finger (you can click+drag with one finger but the force
required to keep the touchpad clicked down while dragging
makes this uncomfortable to do).
This will likely also only work if the mouse click emulation
mode is set to "area" and not "fingers" with "fingers" being
the default now. In GNOME you can configure
the "click emulation mode" in the "tweaks" tools under
"mouse & touchpad" (and tap to click is in the normal
settings menu / control panel).
If you have the click emulations set to fingers and
then do the click with 1 finger + drag with another
finger thing, I think the drag will turn into a
right button drag instead of a left button drag which
is likely why this is not working.
You can check which mode you are in by seeing how
you right click. If you right-click by pressing down
in the right bottom corner of the touchpad then
your userspace (libinput) config is set to areas,
if you can right click anywhere by pressing down
with 2 fingers at once then your click emulation
is in fingers mode and this is likely why click-n-drag
is not working.
I have just dug up my T14 gen1 (Intel) and updated it
to kernel 6.6.11 to rule out kernel regressions.
And both click-n-drag and tap-n-drag (double-tap then
drag) both work fine there with a touchpad with
an ACPI id of LEN2068 as shown by
cat /sys/bus/serio/devices/serio1/firmware_id
(with the Desktop Environment configured for bottom
area click emulation and tap-to-click enabled)
As for why changing things back to synps2 works,
I don't know. One guess is that you already configured
the touchpad behavior of your desktop environment to
your liking in the past and your desktop environment
has remembered this only for the input device-name
which is used in SynPS/2 mode and the different
input device-name in RMI4 mode in new (new-ish)
kernels causes the desktop environment to use
default settings which are typically "fingers"
click emulation and tap-to-click disabled.
This can e.g. also happen if you have moved your
disk (contents) over from an older machine. IIRC
the SynPS/2 driver always used the same input
device-name where as with RMI4 the name is tied
to the actual laptop model.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay handling
From: Jeff LaBundy @ 2024-01-11 13:55 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <12ac3718-2c69-4d11-8ea4-b555f2321232@wolfvision.net>
Hi Javier,
On Fri, Jan 05, 2024 at 08:21:25PM +0100, Javier Carrasco wrote:
> Hi Jeff,
>
> On 30.12.23 21:29, Jeff LaBundy wrote:
> > Having reviewed this version in detail, it's clear how the implementation
> > imposes this restriction. However, it's not clear why we have to have this
> > restriction straight out of the gate; it also breaks the "square doughnut"
> > example we discussed in v5, where a button resides inside a touch surface
> > which is split into four discrete rectangles that report X/Y coordinates
> > relative to the same origin.
> >
> > From my naive point of view, a driver should merely pass a contact's ID
> > (for HW that can track contacts), coordinates, and pressure. Your helper(s)
> > are then responisble for iterating over the list, determining the segment
> > in which the coordinates fall, and then reporting the event by way of
> > input_report_abs() or input_report_key() based on whether or not a keycode
> > is defined.
> >
> > I think the problem with that approach is that touchscreen drivers only
> > report coordinates when the pressure is nonzero. The process of dropping
> > a contact, i.e. button release for some segments, happens inside input-mt
> > by virtue of the driver calling input_mt_sync_frame().
> >
> > It makes sense now why you are duplicating the contact tracking to a degree
> > here. Therefore, it's starting to look more and more like the overlay segment
> > handling needs to move into the input-mt core, where much of the information
> > you need already exists.
> >
> > If we look at input_mt_report_pointer_emulation(), the concept of button
> > release is already happening; all we really want to do here is gently
> > expand the core to understand that some ranges of coordinates are simply
> > quantized to a keycode with binary pressure (i.e. press/release).
> >
> > In addition to removing duplicate code as well as the restriction of supporting
> > only one X/Y surface, moving overlay support into the input-mt core would
> > remove the need to modify each touchscreen driver one at a time with what
> > are largely the same nontrivial changes. If we think about it more, the
> > touchscreen controller itself is not changing, so the driver really shouldn't
> > have to change much either.
> >
> > Stated another way, I think it's a better design pattern if we let drivers
> > continue to do their job of merely lobbing hardware state to the input
> > subsytem via input_mt_slot(), touchscreen_report_pos() and input_mt_sync_frame(),
> > then leave it to the input subsystem alone to iterate over the list and
> > determine whether some coordinates must be handled differently.
> >
> > The main drawback to this approach is that the overlay buttons would need
> > to go back to being part of the touchscreen input device as in v1, as opposed
> > to giving the driver the flexibility of splitting the buttons and X/Y surfaces
> > into two separate input devices.
> >
> > When we first discussed this with Peter, we agreed that splitting them into two
> > input devices grants the most flexibility, in case user space opts to inhibit
> > one but not the other, etc. However since the buttons and X/Y surfaces are all
> > part of the same physical substrate, it seems the chances of user space being
> > interested in one but not the other are low.
> >
> > Furthermore, folding the buttons and X/Y surfaces back into the same input
> > device would remove the need for each touchscreen driver to preemptively
> > allocate a second input device, but then remove it later as in patch [4/4]
> > in case the helpers did not find any buttons.
> >
> > What are your thoughts on evolving the approach in this way? It's obviously
> > another big change and carries some risk to the core, so I'm curious to hear
> > Dmitry's and others' thoughts as well. I appreciate that you've been iterating
> > on this for some time, and good is not the enemy of great; therefore, maybe
> > a compromise is to move forward with the current approach in support of the
> > hardware you have today, then work it into the input-mt core over time. But
> > it would be nice to avoid ripping up participating touchscreen drivers twice.
> >
> > Thank you for your patience and continued effort. In the meantime, please note
> > some minor comments that are independent of this architectural decision.
> >
>
> Thanks again for your thorough reviews and proposals to improve the code.
>
> I am basically open to any solution that improves the quality of the
> feature. If I get you right, moving everything to the input-mt core
> would hide the overlay stuff from the device drivers (which sounds good)
> and a bit of code could be simplified by using the existing infrastructure.
Yes, that is the idea.
>
> On the other hand, adding this feature to the input-mt core right away
> increases the risk of breaking things that many users need. We are
> already using this feature in some prototypes since v1 without any issue
> so far, but it would be great if it could be tested under different
> circumstances (hardware, configurations, etc.) before it goes into the
> core, wouldn't it?
I agree with you. Thinking about this more, immediately introducing this
feature to the core is a relatively high risk that would be shared by all
users. I like your idea of introducing a preliminary version first before
making heavy-handed changes. That's the beauty of helper functions; they
only impact users who explicitly opt in.
>
> I would also like to know what more experienced people think about it,
> if we should go all out and add it to the input-mt core now or as you
> also suggested, move forward with the current approach and let it
> "mature" first. The cost of that would be modifying device driver code
> twice, but I suppose that not so many drivers will add this feature in
> the next kernel iterations... maybe you? it sounds like that you might
> have a use case for it :)
I don't have an immediate use case, but I've been looking at this from
the perspective of a future customer of it. Maybe the right path forward
is as follows:
1. Stick with the same general architecture of v6 and its "limitations",
which in practice are unlikely to be encountered. I imagine the overlay
layout you have been using would be the most common use case.
2. Make the handful of small changes that have been suggested thus far.
3. Consider updating patch [4/4] to combine the touchscreen and buttons
into the same input device as you had in v1. This sets a little simpler
precedent for the first user of these helpers. If later these helpers
do get absorbed into the core, thereby forcing a single input device,
the st1232 would continue to appear the same to user space.
Does this seem reasonable?
>
> >> +struct button {
> >> + u32 key;
> >> + bool pressed;
> >> + int slot;
> >> +};
> >> +
> >> +struct touch_overlay_segment {
> >> + struct list_head list;
> >> + u32 x_origin;
> >> + u32 y_origin;
> >> + u32 x_size;
> >> + u32 y_size;
> >> + struct button *btn;
> >
> > I think you can simply declare the struct itself here as opposed to a pointer to
> > one; this would avoid a second call to devm_kzalloc().
> >
>
> That was the other option I mentioned in my reply and I am fine with the
> modification you propose.
>
> >> + if (fwnode_property_present(segment_node, "linux,code")) {
> >
> > Instead of walking the device tree twice by calling fwnode_property_present()
> > followed by fwnode_property_read_u32(), you can simply check whether the
> > latter returns -EINVAL, which indicates the optional property was absent.
> >
>
> Ack.
>
> >> + segment->btn = devm_kzalloc(dev, sizeof(*segment->btn),
> >> + GFP_KERNEL);
> >> + if (!segment->btn)
> >> + return -ENOMEM;
> >> +
>
>
> >> + fwnode_for_each_child_node(overlay, fw_segment) {
> >> + segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
> >> + if (!segment) {
> >> + error = -ENOMEM;
> >> + goto put_overlay;
> >
> > For this and the below case where you exit the loop early in case of an
> > error, you must call fwnode_handle_put(fw_segment) manually. The reference
> > count is handled automatically only when the loop iterates and terminates
> > naturally.
> >
> > Since nothing else happens between the loop and the 'put_overlay' label,
> > you can also replace the goto with a break and remove the label altogether.
> >
>
> Ack.
>
> >
> > Kind regards,
> > Jeff LaBundy
>
>
> Thanks and best regards,
> Javier Carrasco
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH V2] Input: adc-joystick: Handle inverted axes
From: Chris Morgan @ 2024-01-11 22:03 UTC (permalink / raw)
To: linux-input
Cc: dmitry.torokhov, hdegoede, paul, peter.hutterer, svv, biswarupp,
contact, Chris Morgan
From: Chris Morgan <macromorgan@hotmail.com>
When one or more axes are inverted, (where min > max), normalize the
data so that min < max and invert the values reported to the input
stack.
This ensures we can continue defining the device correctly in the
device tree while not breaking downstream assumptions that min is
always less than max.
Changes since V1:
- Moved proposed helper for inversion from input stack to adc-joystick
driver.
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
drivers/input/joystick/adc-joystick.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..46197ebd3564 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -18,6 +18,7 @@ struct adc_joystick_axis {
s32 range[2];
s32 fuzz;
s32 flat;
+ bool inverted;
};
struct adc_joystick {
@@ -29,6 +30,14 @@ struct adc_joystick {
bool polled;
};
+static int adc_joystick_invert(struct input_dev *dev, unsigned int axis, int val)
+{
+ int min = dev->absinfo[axis].minimum;
+ int max = dev->absinfo[axis].maximum;
+
+ return (max + min) - val;
+}
+
static void adc_joystick_poll(struct input_dev *input)
{
struct adc_joystick *joy = input_get_drvdata(input);
@@ -38,6 +47,8 @@ static void adc_joystick_poll(struct input_dev *input)
ret = iio_read_channel_raw(&joy->chans[i], &val);
if (ret < 0)
return;
+ if (joy->axes[i].inverted)
+ val = adc_joystick_invert(input, i, val);
input_report_abs(input, joy->axes[i].code, val);
}
input_sync(input);
@@ -86,6 +97,8 @@ static int adc_joystick_handle(const void *data, void *private)
val = sign_extend32(val, msb);
else
val &= GENMASK(msb, 0);
+ if (joy->axes[i].inverted)
+ val = adc_joystick_invert(joy->input, i, val);
input_report_abs(joy->input, joy->axes[i].code, val);
}
@@ -168,11 +181,17 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
goto err_fwnode_put;
}
+ if (axes[i].range[0] > axes[i].range[1]) {
+ dev_dbg(dev, "abs-axis %d inverted\n", i);
+ axes[i].inverted = 1;
+ }
+
fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz);
fwnode_property_read_u32(child, "abs-flat", &axes[i].flat);
input_set_abs_params(joy->input, axes[i].code,
- axes[i].range[0], axes[i].range[1],
+ min_array(axes[i].range, 2),
+ max_array(axes[i].range, 2),
axes[i].fuzz, axes[i].flat);
input_set_capability(joy->input, EV_ABS, axes[i].code);
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 1/2] Input: uinput - Allow uinput_request_submit wait interrupting
From: Vi Pfau @ 2024-01-12 1:37 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <853baa9e-1c76-4e61-b1f6-a1155ccb5dd7@endrift.com>
Hello Dmitry,
It's been almost a month since I replied addressing your concerns on
this patch. Can you please comment?
On 12/14/23 19:04, Vicki Pfau wrote:
> Hi Dmitry
>
> On 12/8/23 19:24, Vicki Pfau wrote:
>> Hi Dmitry,
>>
>> On 12/8/23 11:32, Dmitry Torokhov wrote:
>>> Hi Vicki,
>>>
>>> On Wed, Dec 06, 2023 at 10:34:05PM -0800, Vicki Pfau wrote:
>>>> Currently, uinput_request_submit will only fail if the request wait times out.
>>>> However, in other places this wait is interruptable, and in this specific
>>>> location it can lead to issues, such as causing system suspend to hang until
>>>> the request times out.
>>>
>>> Could you please explain how a sleeping process can cause suspend to
>>> hang?
>>
>> While I'm not 100% sure how it happens, given I found this by reproducing it before I came up with a theory for why it happened, my guess is that as it's trying to suspend all of userspace programs, it suspends the process that owns the uinput handle, so it can't continue to service requests, while the other process hangs in the uninterruptable call, blocking suspend for 30 seconds until the call times out.
>>
>>>
>>>> Since the timeout is so long, this can cause the
>>>> appearance of a total system freeze. Making the wait interruptable resolves
>>>> this and possibly further issues.
>>>
>>> I think you are trying to find a justification too hard and it does not
>>> make sense, however I agree that allowing to kill the process issuing
>>> the request without waiting for the timeout to expire if the other side
>>> is stuck might be desirable.
>>
>> This isn't reaching. As I said above, I discovered the patched line of code *after* observing suspend hanging for 30 seconds while trying to reproduce another bug. I wrote this patch, retested, and found that it now suspended immediately, leading to a visible -ERESTARTSYS in strace on coming back from suspend.
>>
>> I can post the reproduction case somewhere, but the test program is only the evdev client end, with the uinput side being Steam, which I don't have source code for.
>>
>>>
>>> I think the best way to use wait_for_completion_killable_timeout()
>>> so that stray signals do not disturb userspace, but the processes can
>>> still be terminated.
>>
>> There's already a mutex_lock_interruptable in uinput_request_send that could cause this to fall back to userspace under similar circumstances. The only difference I can find, which is admittedly a bug in this patch now that I look at it again, is that uinput_dev_event would get called twice, leading to the request getting duplicated.
>
> After further investigation, it seems this would still be the case even if the request times out--an invalid request would get left in the buffer, which means that while this is a new way to trigger the issue, it's not actually a new issue.
>
> It seems to me that this driver could use a lot of love to get it into better shape, which I could work on, but I'm not actually sure where to begin. Especially if we don't want to break ABI.
>
>>
>> If there's a better way to handle the suspend case let me know, but this is not a hypothetical issue.
>>
>>>
>>> Thanks.
>>>
>>
>> Vicki
>
> Vici
Vicki
^ permalink raw reply
* [PATCH] Input: fix atomicity violation in gameport_run_poll_handler
From: Gui-Dong Han @ 2024-01-12 7:38 UTC (permalink / raw)
To: dmitry.torokhov, arnd, schnelle
Cc: linux-input, linux-kernel, baijiaju1990, Gui-Dong Han, stable
In gameport_run_poll_handler():
...
if (gameport->poll_cnt)
mod_timer(&gameport->poll_timer, jiffies + ...));
In gameport_stop_polling():
spin_lock(&gameport->timer_lock);
if (!--gameport->poll_cnt)
del_timer(&gameport->poll_timer);
spin_unlock(&gameport->timer_lock);
An atomicity violation occurs due to the concurrent execution of
gameport_run_poll_handler() and gameport_stop_polling(). The current check
for gameport->poll_cnt in gameport_run_poll_handler() is not effective
because poll_cnt can be decremented to 0 and del_timer can be called in
gameport_stop_polling() before mod_timer is called in
gameport_run_poll_handler(). This situation leads to the risk of calling
mod_timer for a timer that has already been deleted in
gameport_stop_polling(). Since calling mod_timer on a deleted timer
reactivates it, this atomicity violation could result in the timer being
activated while the poll_cnt value is 0.
This possible bug is found by an experimental static analysis tool
developed by our team, BassCheck[1]. This tool analyzes the locking APIs
to extract function pairs that can be concurrently executed, and then
analyzes the instructions in the paired functions to identify possible
concurrency bugs including data races and atomicity violations. The above
possible bug is reported when our tool analyzes the source code of
Linux 5.17.
To resolve this issue, it is suggested to add a spinlock pair in
gameport_run_poll_handler() to ensure atomicity. With this patch applied,
our tool no longer reports the bug, with the kernel configuration
allyesconfig for x86_64. Due to the absence of the requisite hardware, we
are unable to conduct runtime testing of the patch. Therefore, our
verification is solely based on code logic analysis.
[1] https://sites.google.com/view/basscheck/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
---
drivers/input/gameport/gameport.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/input/gameport/gameport.c b/drivers/input/gameport/gameport.c
index 34f416a3ebcb..12af46d3c059 100644
--- a/drivers/input/gameport/gameport.c
+++ b/drivers/input/gameport/gameport.c
@@ -202,8 +202,13 @@ static void gameport_run_poll_handler(struct timer_list *t)
struct gameport *gameport = from_timer(gameport, t, poll_timer);
gameport->poll_handler(gameport);
+
+ spin_lock(&gameport->timer_lock);
+
if (gameport->poll_cnt)
mod_timer(&gameport->poll_timer, jiffies + msecs_to_jiffies(gameport->poll_interval));
+
+ spin_unlock(&gameport->timer_lock);
}
/*
--
2.34.1
^ permalink raw reply related
* [PATCH 1/2] HID: hid-steam: remove pointless error message
From: Dan Carpenter @ 2024-01-12 14:34 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, linux-input, linux-kernel, kernel-janitors
This error message doesn't really add any information. If modprobe
fails then the user will already know what the error code is. In the
case of kmalloc() it's a style violation to print an error message for
that because kmalloc has it's own better error messages built in.
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/hid/hid-steam.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index b3c4e50e248a..59df6ead7b54 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1109,10 +1109,9 @@ static int steam_probe(struct hid_device *hdev,
return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
- if (!steam) {
- ret = -ENOMEM;
- goto steam_alloc_fail;
- }
+ if (!steam)
+ return -ENOMEM;
+
steam->hdev = hdev;
hid_set_drvdata(hdev, steam);
spin_lock_init(&steam->lock);
@@ -1179,9 +1178,6 @@ static int steam_probe(struct hid_device *hdev,
cancel_work_sync(&steam->work_connect);
cancel_delayed_work_sync(&steam->mode_switch);
cancel_work_sync(&steam->rumble_work);
-steam_alloc_fail:
- hid_err(hdev, "%s: failed with error %d\n",
- __func__, ret);
return ret;
}
--
2.43.0
^ permalink raw reply related
* [PATCH 2/2] HID: hid-steam: Fix cleanup in probe()
From: Dan Carpenter @ 2024-01-12 14:35 UTC (permalink / raw)
To: Vicki Pfau
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <305898fb-6bd4-4749-806c-05ec51bbeb80@moroto.mountain>
There are a number of issues in this code. First of all if
steam_create_client_hid() fails then it leads to an error pointer
dereference when we call hid_destroy_device(steam->client_hdev).
Also there are a number of leaks. hid_hw_stop() is not called if
hid_hw_open() fails for example. And it doesn't call steam_unregister()
or hid_hw_close().
Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
This is just from static analysis and code review. I haven't tested
it. I only included the fixes tag for the error pointer dereference.
drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 59df6ead7b54..b08a5ab58528 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
*/
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
if (ret)
- goto hid_hw_start_fail;
+ goto err_cancel_work;
ret = hid_hw_open(hdev);
if (ret) {
hid_err(hdev,
"%s:hid_hw_open\n",
__func__);
- goto hid_hw_open_fail;
+ goto err_hw_stop;
}
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
@@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
hid_err(hdev,
"%s:steam_register failed with error %d\n",
__func__, ret);
- goto input_register_fail;
+ goto err_hw_close;
}
}
steam->client_hdev = steam_create_client_hid(hdev);
if (IS_ERR(steam->client_hdev)) {
ret = PTR_ERR(steam->client_hdev);
- goto client_hdev_fail;
+ goto err_stream_unregister;
}
steam->client_hdev->driver_data = steam;
ret = hid_add_device(steam->client_hdev);
if (ret)
- goto client_hdev_add_fail;
+ goto err_destroy;
return 0;
-client_hdev_add_fail:
- hid_hw_stop(hdev);
-client_hdev_fail:
+err_destroy:
hid_destroy_device(steam->client_hdev);
-input_register_fail:
-hid_hw_open_fail:
-hid_hw_start_fail:
+err_stream_unregister:
+ if (steam->connected)
+ steam_unregister(steam);
+err_hw_close:
+ hid_hw_close(hdev);
+err_hw_stop:
+ hid_hw_stop(hdev);
+err_cancel_work:
cancel_work_sync(&steam->work_connect);
cancel_delayed_work_sync(&steam->mode_switch);
cancel_work_sync(&steam->rumble_work);
+
return ret;
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v14 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2024-01-12 15:08 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZZ-W0UPHOdpU-8el@google.com>
Hi Dmitry,
On 11/01/2024 08:20, Dmitry Torokhov wrote:
> Hi Neil,
>
> On Thu, Dec 21, 2023 at 04:21:20PM +0100, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>
> Thank you very much for explaining how reading of additional contacts
> and checksum works, it makes sense now.
>
> I was a bit unhappy about number of times we copy/move the data over;
> could you please try the patch below to see if the device still works
> with it?
Sure, I'll test it and report you.
>
> I also shortened some #defines and defines some additional structures.
> Also as far as I can see not everything needs to be packed as the data
> is naturally aligned on the word boundaries.
Great, thank!
Neil
>
> Thanks!
>
^ permalink raw reply
* Re: [PATCH] Input: i8042 - add quirk for Lenovo ThinkPad T14 Gen 1
From: Jonathan Denose @ 2024-01-12 15:10 UTC (permalink / raw)
To: Hans de Goede
Cc: Dmitry Torokhov, linux-input, Jonathan Denose, Huacai Chen,
Mattijs Korpershoek, Takashi Iwai, Werner Sembach, linux-kernel
In-Reply-To: <005a6d3c-ffba-45df-bdc0-cb2d32e6b676@redhat.com>
Hans,
On Thu, Jan 11, 2024 at 4:48 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Jonathan,
>
> On 1/11/24 00:42, Jonathan Denose wrote:
> > Dmitry,
> >
> > Sorry I forgot to reply all, so I'm resending my other email.
> >
> > On Tue, Jan 9, 2024 at 1:28 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >>
> >> Hi Jonathan,
> >>
> >> On Mon, Nov 27, 2023 at 10:38:57AM -0600, Jonathan Denose wrote:
> >>> Hi Dmitry
> >>>
> >>> On Fri, Nov 24, 2023 at 10:45 PM Dmitry Torokhov
> >>> <dmitry.torokhov@gmail.com> wrote:
> >>>>
> >>>> Hi Jonathan,
> >>>>
> >>>> On Mon, Sep 25, 2023 at 04:33:20PM -0500, Jonathan Denose wrote:
> >>>>> The ThinkPad T14 Gen 1 touchpad works fine except that clicking
> >>>>> and dragging by tapping the touchpad or depressing the touchpad
> >>>>> do not work. Disabling PNP for controller setting discovery enables
> >>>>> click and drag without negatively impacting other touchpad features.
> >>>>
> >>>> I would like to understand more on how enabling PnP discovery for i8042
> >>>> affects the touchpad. Do you see it using different interrupt or IO
> >>>> ports? What protocol does the touchpad use with/without PnP? If the
> >>>> protocol is the same, do you see difference in the ranges (pressure,
> >>>> etc) reported by the device?
> >>>>
> >>>> Thanks.
> >>>>
> >>>> --
> >>>> Dmitry
> >>>
> >>> Without PnP discovery the touchpad is using the SynPS/2 protocol, with
> >>> PnP discovery, the touchpad is using the rmi4 protocol. Since the
> >>> protocols are different, so are the ranges but let me know if you
> >>> still want to see them.
> >>
> >> Thank you for this information. So it is not PnP discovery that appears
> >> harmful in your case, but rather that legacy PS/2 mode appears to be
> >> working better than RMI4 for the device in question.
> >>
> >> I will note that the original enablement of RMI4 for T14 was done by
> >> Hans in [1]. Later T14 with AMD were added to the list of devices that
> >> should use RMI4 [2], however this was reverted in [3].
> >>
> >> Could you please tell me what exact device you are dealing with? What's
> >> it ACPI ID?
> >>
> >> [1] https://lore.kernel.org/all/20201005114919.371592-1-hdegoede@redhat.com/
> >> [2] https://lore.kernel.org/r/20220318113949.32722-1-snafu109@gmail.com
> >> [3] https://lore.kernel.org/r/20220920193936.8709-1-markpearson@lenovo.com
> >>
> >> Thanks.
> >>
> >> --
> >> Dmitry
> >
> > Thanks for your reply!
> >
> > I'm not 100% sure which of these is the ACPI ID, but from `udevadm
> > info -e` there's:
> > N: Name="Synaptics TM3471-020"
> > P: Phys=rmi4-00/input0
>
> To get the ACPI ID you need to run e.g. :``
>
> cat /sys/bus/serio/devices/serio1/firmware_id
>
> After reading the original bug report again I take back my
> Reviewed-by and I'm tending towards a nack for this.
>
> Jonathan upon re-reading things I think that your problem
> is more a case of user space mis-configuration then
> a kernel problem.
>
> You mention both tap-n-drag not working as well as click+drag
> not working.
>
> tap-n-drag is purely done in userspace and typically only
> works if tap-to-click is enabled in the touchpad configuration
> of your desktop environment.
>
> Click + drag requires you to use the bottom of the touchpad
> (the only part which actually clicks) as if there still were
> 2 physical buttons there and then click the touchpad down
> with 1 finger till it clicks and then drags with another
> finger (you can click+drag with one finger but the force
> required to keep the touchpad clicked down while dragging
> makes this uncomfortable to do).
>
> This will likely also only work if the mouse click emulation
> mode is set to "area" and not "fingers" with "fingers" being
> the default now. In GNOME you can configure
> the "click emulation mode" in the "tweaks" tools under
> "mouse & touchpad" (and tap to click is in the normal
> settings menu / control panel).
>
> If you have the click emulations set to fingers and
> then do the click with 1 finger + drag with another
> finger thing, I think the drag will turn into a
> right button drag instead of a left button drag which
> is likely why this is not working.
>
> You can check which mode you are in by seeing how
> you right click. If you right-click by pressing down
> in the right bottom corner of the touchpad then
> your userspace (libinput) config is set to areas,
> if you can right click anywhere by pressing down
> with 2 fingers at once then your click emulation
> is in fingers mode and this is likely why click-n-drag
> is not working.
>
> I have just dug up my T14 gen1 (Intel) and updated it
> to kernel 6.6.11 to rule out kernel regressions.
>
> And both click-n-drag and tap-n-drag (double-tap then
> drag) both work fine there with a touchpad with
> an ACPI id of LEN2068 as shown by
> cat /sys/bus/serio/devices/serio1/firmware_id
>
> (with the Desktop Environment configured for bottom
> area click emulation and tap-to-click enabled)
>
> As for why changing things back to synps2 works,
> I don't know. One guess is that you already configured
> the touchpad behavior of your desktop environment to
> your liking in the past and your desktop environment
> has remembered this only for the input device-name
> which is used in SynPS/2 mode and the different
> input device-name in RMI4 mode in new (new-ish)
> kernels causes the desktop environment to use
> default settings which are typically "fingers"
> click emulation and tap-to-click disabled.
>
> This can e.g. also happen if you have moved your
> disk (contents) over from an older machine. IIRC
> the SynPS/2 driver always used the same input
> device-name where as with RMI4 the name is tied
> to the actual laptop model.
>
> Regards,
>
> Hans
>
>
Thank you for your thorough reply. Based on what you've written, I
agree this sounds more like a user-space issue than a kernel issue. At
least that narrows it down for me, so I'll take a look at what could
be misconfigured in user-space.
Thanks so much for your help!
Jonathan
^ permalink raw reply
* Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver
From: James Ogletree @ 2024-01-12 15:41 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: James Ogletree, Fred Treven, Ben Bright, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Simon Trimmer, Charles Keepax,
Richard Fitzgerald, Lee Jones, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, James Schulman, David Rhodes,
Alexandre Belloni, Peng Fan, Jeff LaBundy, Sebastian Reichel,
Jacky Bai, Weidong Wang, Arnd Bergmann, Herve Codina, Shuming Fan,
Shenghao Ding, Ryan Lee, Linus Walleij,
open list:CIRRUS LOGIC HAPTIC DRIVERS,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list,
open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
moderated list:CIRRUS LOGIC AUDIO CODEC DRIVERS
In-Reply-To: <ZZ-YhtIulqrSFc3R@google.com>
> On Jan 11, 2024, at 1:28 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Jan 10, 2024 at 02:36:55PM +0000, James Ogletree wrote:
>>
>>> On Jan 9, 2024, at 4:31 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>
>>> On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
>>>>
>>>>
>>>>> On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>>>
>>>>> On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
>>>>>> + } else {
>>>>>> + queue_work(info->vibe_wq, &info->vibe_stop_work);
>>>>>
>>>>> Which effect are you stopping? All of them? You need to stop a
>>>>> particular one.
>>>>
>>>> Our implementation of “stop” stops all effects in flight which is intended.
>>>> That is probably unusual so I will add a comment here in the next
>>>> version.
>>>
>>> Again, please implement the driver properly, not define your own
>>> carveouts for the expected behavior.
>>
>> Ack, and a clarification question: the device is not actually able to
>> play multiple effects at once. In that case, does stopping a specific
>> effect entail just cancelling an effect in the queue?
>
> In this case I believe the device should declare maximum number of
> effects as 1. Userspace is supposed to determine maximum number of
> simultaneously playable effects by issuing EVIOCGEFFECTS ioctl on the
> corresponding event device.
Is it possible to specify the device’s maximum simultaneous effects
without also restricting the number of effects the user can upload? It
looks like both are tied to ff->max_effects.
Best,
James
^ permalink raw reply
* Re: [PATCH bpf-next v3 0/3] Annotate kfuncs in .BTF_ids section
From: Jiri Olsa @ 2024-01-12 16:20 UTC (permalink / raw)
To: Daniel Xu
Cc: linux-input, coreteam, linux-arm-kernel, linux-kernel,
netfilter-devel, linux-kselftest, linux-trace-kernel, fsverity,
bpf, netdev, linux-stm32, cgroups, alexei.starovoitov, olsajiri,
quentin, alan.maguire, memxor
In-Reply-To: <cover.1704565248.git.dxu@dxuuu.xyz>
On Sat, Jan 06, 2024 at 11:24:07AM -0700, Daniel Xu wrote:
> === Description ===
>
> This is a bpf-treewide change that annotates all kfuncs as such inside
> .BTF_ids. This annotation eventually allows us to automatically generate
> kfunc prototypes from bpftool.
>
> We store this metadata inside a yet-unused flags field inside struct
> btf_id_set8 (thanks Kumar!). pahole will be taught where to look.
>
> More details about the full chain of events are available in commit 3's
> description.
>
> The accompanying pahole changes (still needs some cleanup) can be viewed
> here on this "frozen" branch [0].
so the plan is to have bpftool support to generate header file
with detected kfuncs?
jirka
>
> [0]: https://github.com/danobi/pahole/tree/kfunc_btf-mailed
>
> === Changelog ===
>
> Changes from v2:
> * Only WARN() for vmlinux kfuncs
>
> Changes from v1:
> * Move WARN_ON() up a call level
> * Also return error when kfunc set is not properly tagged
> * Use BTF_KFUNCS_START/END instead of flags
> * Rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS
>
> Daniel Xu (3):
> bpf: btf: Support flags for BTF_SET8 sets
> bpf: btf: Add BTF_KFUNCS_START/END macro pair
> bpf: treewide: Annotate BPF kfuncs in BTF
>
> drivers/hid/bpf/hid_bpf_dispatch.c | 8 +++----
> fs/verity/measure.c | 4 ++--
> include/linux/btf_ids.h | 21 +++++++++++++++----
> kernel/bpf/btf.c | 8 +++++++
> kernel/bpf/cpumask.c | 4 ++--
> kernel/bpf/helpers.c | 8 +++----
> kernel/bpf/map_iter.c | 4 ++--
> kernel/cgroup/rstat.c | 4 ++--
> kernel/trace/bpf_trace.c | 8 +++----
> net/bpf/test_run.c | 8 +++----
> net/core/filter.c | 16 +++++++-------
> net/core/xdp.c | 4 ++--
> net/ipv4/bpf_tcp_ca.c | 4 ++--
> net/ipv4/fou_bpf.c | 4 ++--
> net/ipv4/tcp_bbr.c | 4 ++--
> net/ipv4/tcp_cubic.c | 4 ++--
> net/ipv4/tcp_dctcp.c | 4 ++--
> net/netfilter/nf_conntrack_bpf.c | 4 ++--
> net/netfilter/nf_nat_bpf.c | 4 ++--
> net/xfrm/xfrm_interface_bpf.c | 4 ++--
> net/xfrm/xfrm_state_bpf.c | 4 ++--
> .../selftests/bpf/bpf_testmod/bpf_testmod.c | 8 +++----
> 22 files changed, 81 insertions(+), 60 deletions(-)
>
> --
> 2.42.1
>
^ permalink raw reply
* Re: [PATCH bpf-next v3 0/3] Annotate kfuncs in .BTF_ids section
From: Daniel Xu @ 2024-01-12 20:03 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-input, coreteam, linux-arm-kernel, linux-kernel,
netfilter-devel, linux-kselftest, linux-trace-kernel, fsverity,
bpf, netdev, linux-stm32, cgroups, alexei.starovoitov, quentin,
alan.maguire, memxor
In-Reply-To: <ZaFm13GyXUukcnkm@krava>
On Fri, Jan 12, 2024 at 05:20:39PM +0100, Jiri Olsa wrote:
> On Sat, Jan 06, 2024 at 11:24:07AM -0700, Daniel Xu wrote:
> > === Description ===
> >
> > This is a bpf-treewide change that annotates all kfuncs as such inside
> > .BTF_ids. This annotation eventually allows us to automatically generate
> > kfunc prototypes from bpftool.
> >
> > We store this metadata inside a yet-unused flags field inside struct
> > btf_id_set8 (thanks Kumar!). pahole will be taught where to look.
> >
> > More details about the full chain of events are available in commit 3's
> > description.
> >
> > The accompanying pahole changes (still needs some cleanup) can be viewed
> > here on this "frozen" branch [0].
>
> so the plan is to have bpftool support to generate header file
> with detected kfuncs?
Yep, that's the major use case. But I see other use cases as well like
precision probing of kfuncs. Rather than guess and check which progs can
load (in the event of backwards incompatible kfunc changes), programs
can look at kfunc type signature thru BTF.
^ permalink raw reply
* Re: [PATCH 2/2] HID: hid-steam: Fix cleanup in probe()
From: Vicki Pfau @ 2024-01-13 2:11 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <1fd87904-dabf-4879-bb89-72d13ebfc91e@moroto.mountain>
I have applied this to our downstream and made sure it compiles and runs
without obvious issues.
Reviewed-by: Vicki Pfau <vi@endrift.com>
Thanks,
Vicki
On 1/12/24 06:35, Dan Carpenter wrote:
> There are a number of issues in this code. First of all if
> steam_create_client_hid() fails then it leads to an error pointer
> dereference when we call hid_destroy_device(steam->client_hdev).
>
> Also there are a number of leaks. hid_hw_stop() is not called if
> hid_hw_open() fails for example. And it doesn't call steam_unregister()
> or hid_hw_close().
>
> Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This is just from static analysis and code review. I haven't tested
> it. I only included the fixes tag for the error pointer dereference.
>
> drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 59df6ead7b54..b08a5ab58528 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
> */
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> if (ret)
> - goto hid_hw_start_fail;
> + goto err_cancel_work;
>
> ret = hid_hw_open(hdev);
> if (ret) {
> hid_err(hdev,
> "%s:hid_hw_open\n",
> __func__);
> - goto hid_hw_open_fail;
> + goto err_hw_stop;
> }
>
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> @@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
> hid_err(hdev,
> "%s:steam_register failed with error %d\n",
> __func__, ret);
> - goto input_register_fail;
> + goto err_hw_close;
> }
> }
>
> steam->client_hdev = steam_create_client_hid(hdev);
> if (IS_ERR(steam->client_hdev)) {
> ret = PTR_ERR(steam->client_hdev);
> - goto client_hdev_fail;
> + goto err_stream_unregister;
> }
> steam->client_hdev->driver_data = steam;
>
> ret = hid_add_device(steam->client_hdev);
> if (ret)
> - goto client_hdev_add_fail;
> + goto err_destroy;
>
> return 0;
>
> -client_hdev_add_fail:
> - hid_hw_stop(hdev);
> -client_hdev_fail:
> +err_destroy:
> hid_destroy_device(steam->client_hdev);
> -input_register_fail:
> -hid_hw_open_fail:
> -hid_hw_start_fail:
> +err_stream_unregister:
> + if (steam->connected)
> + steam_unregister(steam);
> +err_hw_close:
> + hid_hw_close(hdev);
> +err_hw_stop:
> + hid_hw_stop(hdev);
> +err_cancel_work:
> cancel_work_sync(&steam->work_connect);
> cancel_delayed_work_sync(&steam->mode_switch);
> cancel_work_sync(&steam->rumble_work);
> +
> return ret;
> }
>
^ permalink raw reply
* [PATCH v2 1/2] Input: leds - set default-trigger for mute
From: Bernhard Seibold @ 2024-01-13 10:37 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Hans de Goede,
Jamie Lentin, Bernhard Seibold
In-Reply-To: <ZZbxHpibdyNY_zUt@google.com>
Set the default-trigger for the mute led to audio-mute.
Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
drivers/input/input-leds.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index 0e935914bc3a..b16fc81940f5 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -18,6 +18,12 @@
#define VT_TRIGGER(_name) .trigger = NULL
#endif
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
+#define AUDIO_TRIGGER(_name) .trigger = _name
+#else
+#define AUDIO_TRIGGER(_name) .trigger = NULL
+#endif
+
static const struct {
const char *name;
const char *trigger;
@@ -29,7 +35,7 @@ static const struct {
[LED_KANA] = { "kana", VT_TRIGGER("kbd-kanalock") },
[LED_SLEEP] = { "sleep" } ,
[LED_SUSPEND] = { "suspend" },
- [LED_MUTE] = { "mute" },
+ [LED_MUTE] = { "mute", AUDIO_TRIGGER("audio-mute") },
[LED_MISC] = { "misc" },
[LED_MAIL] = { "mail" },
[LED_CHARGING] = { "charging" },
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/2] HID: input: add support for micmute LED
From: Bernhard Seibold @ 2024-01-13 10:37 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Hans de Goede,
Jamie Lentin, Bernhard Seibold
In-Reply-To: <20240113103743.97205-1-mail@bernhard-seibold.de>
Since LED support via input-leds is grandfathered, the new LED is added
directly in hid-input.
Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
drivers/hid/Kconfig | 11 +++++
drivers/hid/hid-input.c | 92 +++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 1 +
3 files changed, 104 insertions(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ce74af79657..71de0af8f460 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -42,6 +42,17 @@ config HID_BATTERY_STRENGTH
that support this feature) through power_supply class so that userspace
tools, such as upower, can display it.
+config HID_LEDS
+ bool "LED support for HID devices"
+ select LEDS_CLASS
+ default y
+ help
+ This option adds support for LEDs on HID devices. Currently, the
+ only supported LED is microphone mute. For all other LEDs,
+ enable CONFIG_INPUT_LEDS.
+
+ If unsure, say Y.
+
config HIDRAW
bool "/dev/hidraw raw HID device support"
help
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index c8b20d44b147..32d3e6a2ac44 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -16,6 +16,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/kernel.h>
+#include <linux/leds.h>
#include <linux/hid.h>
#include <linux/hid-debug.h>
@@ -104,6 +105,9 @@ static const struct usage_priority hidinput_usages_priorities[] = {
#define map_key_clear(c) hid_map_usage_clear(hidinput, usage, &bit, \
&max, EV_KEY, (c))
+#define setup_led(name, trigger) \
+ hidinput_setup_led(device, field, usage_index, name, trigger)
+
static bool match_scancode(struct hid_usage *usage,
unsigned int cur_idx, unsigned int scancode)
{
@@ -674,6 +678,88 @@ static bool hidinput_set_battery_charge_status(struct hid_device *dev,
}
#endif /* CONFIG_HID_BATTERY_STRENGTH */
+#ifdef CONFIG_HID_LEDS
+
+struct hid_led {
+ struct list_head list;
+ struct led_classdev cdev;
+ struct hid_field *field;
+ unsigned int offset;
+ char *name;
+};
+
+static int hidinput_led_brightness_set(struct led_classdev *cdev,
+ enum led_brightness value)
+{
+ struct device *dev = cdev->dev->parent;
+ struct hid_device *device = to_hid_device(dev);
+ struct hid_led *led = container_of(cdev, struct hid_led, cdev);
+
+ hid_set_field(led->field, led->offset, !!value);
+ schedule_work(&device->led_work);
+
+ return 0;
+}
+
+static void hidinput_setup_led(struct hid_device *device,
+ struct hid_field *field, unsigned int offset,
+ const char *name, const char *trigger)
+{
+ struct hid_led *led;
+ struct device *dev = &device->dev;
+ struct device *idev = &field->hidinput->input->dev;
+
+ led = kzalloc(sizeof(*led), GFP_KERNEL);
+ if (!led)
+ return;
+
+ led->name = kasprintf(GFP_KERNEL, "%s::%s", dev_name(idev), name);
+ if (!led->name) {
+ kfree(led);
+ return;
+ }
+
+ led->cdev.name = led->name;
+ led->cdev.default_trigger = trigger;
+ led->cdev.max_brightness = 1;
+ led->cdev.brightness_set_blocking = hidinput_led_brightness_set;
+ led->field = field;
+ led->offset = offset;
+
+ if (led_classdev_register(dev, &led->cdev)) {
+ kfree(name);
+ kfree(led);
+ return;
+ }
+
+ list_add_tail(&led->list, &device->leds);
+}
+
+static void hidinput_cleanup_leds(struct hid_device *device)
+{
+ struct hid_led *led, *tmp;
+
+ list_for_each_entry_safe(led, tmp, &device->leds, list) {
+ led_classdev_unregister(&led->cdev);
+ kfree(led->name);
+ kfree(led);
+ }
+}
+
+#else /* !CONFIG_HID_LEDS */
+
+static void hidinput_setup_led(struct hid_device *device,
+ struct hid_field *field, unsigned int offset,
+ const char *name, const char *trigger)
+{
+}
+
+static void hidinput_cleanup_leds(struct hid_device *device)
+{
+}
+
+#endif /* CONFIG_HID_LEDS */
+
static bool hidinput_field_in_collection(struct hid_device *device, struct hid_field *field,
unsigned int type, unsigned int usage)
{
@@ -935,6 +1021,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
case 0x19: map_led (LED_MAIL); break; /* "Message Waiting" */
case 0x4d: map_led (LED_CHARGING); break; /* "External Power Connected" */
+ case 0x21: /* "Microphone" */
+ setup_led("micmute", "audio-micmute");
+ break;
+
default: goto ignore;
}
break;
@@ -2282,6 +2372,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
int i, k;
INIT_LIST_HEAD(&hid->inputs);
+ INIT_LIST_HEAD(&hid->leds);
INIT_WORK(&hid->led_work, hidinput_led_worker);
hid->status &= ~HID_STAT_DUP_DETECTED;
@@ -2380,6 +2471,7 @@ void hidinput_disconnect(struct hid_device *hid)
{
struct hid_input *hidinput, *next;
+ hidinput_cleanup_leds(hid);
hidinput_cleanup_battery(hid);
list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index bf43f3ff6664..d7cea5476979 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -617,6 +617,7 @@ struct hid_device { /* device report descriptor */
unsigned country; /* HID country */
struct hid_report_enum report_enum[HID_REPORT_TYPES];
struct work_struct led_work; /* delayed LED worker */
+ struct list_head leds; /* List of associated LEDs */
struct semaphore driver_input_lock; /* protects the current driver */
struct device dev; /* device */
--
2.43.0
^ permalink raw reply related
* Re: [PATCH V2] Input: adc-joystick: Handle inverted axes
From: Artur Rojek @ 2024-01-13 11:28 UTC (permalink / raw)
To: Chris Morgan
Cc: linux-input, dmitry.torokhov, hdegoede, paul, peter.hutterer, svv,
biswarupp, Chris Morgan
In-Reply-To: <20240111220333.66060-1-macroalpha82@gmail.com>
Hi Chris,
some comments inline.
On 2024-01-11 23:03, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> When one or more axes are inverted, (where min > max), normalize the
> data so that min < max and invert the values reported to the input
> stack.
>
> This ensures we can continue defining the device correctly in the
> device tree while not breaking downstream assumptions that min is
> always less than max.
>
> Changes since V1:
> - Moved proposed helper for inversion from input stack to adc-joystick
> driver.
The changes summary should go after the "---" separator, otherwise it
ends up in the commit description.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
> drivers/input/joystick/adc-joystick.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/joystick/adc-joystick.c
> b/drivers/input/joystick/adc-joystick.c
> index c0deff5d4282..46197ebd3564 100644
> --- a/drivers/input/joystick/adc-joystick.c
> +++ b/drivers/input/joystick/adc-joystick.c
> @@ -18,6 +18,7 @@ struct adc_joystick_axis {
> s32 range[2];
> s32 fuzz;
> s32 flat;
> + bool inverted;
> };
>
> struct adc_joystick {
> @@ -29,6 +30,14 @@ struct adc_joystick {
> bool polled;
> };
>
> +static int adc_joystick_invert(struct input_dev *dev, unsigned int
> axis, int val)
As the parameter list exceeds 80 characters, can you move "int val" into
a second line?
> +{
> + int min = dev->absinfo[axis].minimum;
> + int max = dev->absinfo[axis].maximum;
> +
> + return (max + min) - val;
> +}
> +
> static void adc_joystick_poll(struct input_dev *input)
> {
> struct adc_joystick *joy = input_get_drvdata(input);
> @@ -38,6 +47,8 @@ static void adc_joystick_poll(struct input_dev
> *input)
> ret = iio_read_channel_raw(&joy->chans[i], &val);
> if (ret < 0)
> return;
> + if (joy->axes[i].inverted)
> + val = adc_joystick_invert(input, i, val);
> input_report_abs(input, joy->axes[i].code, val);
> }
> input_sync(input);
> @@ -86,6 +97,8 @@ static int adc_joystick_handle(const void *data,
> void *private)
> val = sign_extend32(val, msb);
> else
> val &= GENMASK(msb, 0);
> + if (joy->axes[i].inverted)
> + val = adc_joystick_invert(joy->input, i, val);
> input_report_abs(joy->input, joy->axes[i].code, val);
> }
>
> @@ -168,11 +181,17 @@ static int adc_joystick_set_axes(struct device
> *dev, struct adc_joystick *joy)
> goto err_fwnode_put;
> }
>
> + if (axes[i].range[0] > axes[i].range[1]) {
> + dev_dbg(dev, "abs-axis %d inverted\n", i);
There is no "abs-axis" property. Make it "Axis %d inverted\n" instead.
> + axes[i].inverted = 1;
Turn this into an explicit "true", as its a bool.
With all above nits fixed:
Acked-by: Artur Rojek <contact@artur-rojek.eu>
Cheers,
Artur
> + }
> +
> fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz);
> fwnode_property_read_u32(child, "abs-flat", &axes[i].flat);
>
> input_set_abs_params(joy->input, axes[i].code,
> - axes[i].range[0], axes[i].range[1],
> + min_array(axes[i].range, 2),
> + max_array(axes[i].range, 2),
> axes[i].fuzz, axes[i].flat);
> input_set_capability(joy->input, EV_ABS, axes[i].code);
> }
^ permalink raw reply
* Re: [PATCH bpf-next v3 0/3] Annotate kfuncs in .BTF_ids section
From: Jiri Olsa @ 2024-01-13 13:57 UTC (permalink / raw)
To: Daniel Xu
Cc: Jiri Olsa, linux-input, coreteam, linux-arm-kernel, linux-kernel,
netfilter-devel, linux-kselftest, linux-trace-kernel, fsverity,
bpf, netdev, linux-stm32, cgroups, alexei.starovoitov, quentin,
alan.maguire, memxor
In-Reply-To: <2dhmwvfnnqnlrui2qcr5fob54gdsuse5caievct42trvvia6qe@p24nymz3uttv>
On Fri, Jan 12, 2024 at 01:03:59PM -0700, Daniel Xu wrote:
> On Fri, Jan 12, 2024 at 05:20:39PM +0100, Jiri Olsa wrote:
> > On Sat, Jan 06, 2024 at 11:24:07AM -0700, Daniel Xu wrote:
> > > === Description ===
> > >
> > > This is a bpf-treewide change that annotates all kfuncs as such inside
> > > .BTF_ids. This annotation eventually allows us to automatically generate
> > > kfunc prototypes from bpftool.
> > >
> > > We store this metadata inside a yet-unused flags field inside struct
> > > btf_id_set8 (thanks Kumar!). pahole will be taught where to look.
> > >
> > > More details about the full chain of events are available in commit 3's
> > > description.
> > >
> > > The accompanying pahole changes (still needs some cleanup) can be viewed
> > > here on this "frozen" branch [0].
> >
> > so the plan is to have bpftool support to generate header file
> > with detected kfuncs?
>
> Yep, that's the major use case. But I see other use cases as well like
ok, any chance you could already include it in the patchset?
would be a great way to test this.. maybe we could change
selftests to use that
thanks,
jirka
> precision probing of kfuncs. Rather than guess and check which progs can
> load (in the event of backwards incompatible kfunc changes), programs
> can look at kfunc type signature thru BTF.
^ 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