* [HID Patchsets for Samsung driver v4 0/6] Patchsets for Samsung driver
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <CGME20240125043646epcas5p1d599fecff0fc37926295fc5260a80682@epcas5p1.samsung.com>
Dear Jiri & Team,
I hope this email finds you well.
As per the review comments given by Mr. Jiri and Mr. Joe Perches in our last converstaion over mail.
We have incorporated the feedback on our driver. Please check this set of series and help us to improve samsung driver.
As of today, Opensource kernel Samsung driver only supports USB HID devices and do not have support for Bluetooth HID devices.
Samsung would like to improve the samsung driver and extend it's support for bluetooth devices as well.
Summary of changes in Samsung driver:
1. Add support for below bluetooth devices
Samsung wireless Keyboard
Samsung wireless GamePad
Samsung Wireless Action Mouse
Samsung Wireless Book Cover
Samsung Wireless Universal Keyboard
Samsung Wireless HOGP Keyboard
2. Add support for Special key processing on each of the above devices.
Patch Series Overview:
--------------------------------------
[Patch 1/6]
HID Samsung : Broaden device compatibility in samsung driver.
hid_is_usb() check being moved.
[Patch 2/6]
HID: Samsung : Fix the checkpatch complain.
Warning found by checkpatch.pl script.
[Patch 3/6]
HID: Samsung : Add Samsung wireless keyboard support.
[Patch 4/6]
HID: Samsung : Add Samsung wireless gamepad support.
[Patch 5/6]
HID: Samsung : Add Samsung wireless action mouse support.
[Patch 6/6]
HID: Samsung : Add Samsung wireless bookcover and universal keyboard support.
All these changes have been verified and tested thoroughly in android devices.
Please accept our changes.
Thanks for your time and consideration.
Best regards
Sandeep C S
Sandeep C S (6):
HID Samsung : Broaden device compatibility in samsung driver.
HID: Samsung : Fix the checkpatch complain. Rewritten code using
memcmp where applicable.
HID: Samsung : Add Samsung wireless keyboard support.
HID: Samsung : Add Samsung wireless gamepad support.
HID: Samsung : Add Samsung wireless action mouse support.
HID: Samsung : Add Samsung wireless bookcover and universal keyboard
support.
drivers/hid/hid-ids.h | 7 +
drivers/hid/hid-samsung.c | 437 ++++++++++++++++++++++++++++++++++----
2 files changed, 408 insertions(+), 36 deletions(-)
--
2.34.1
^ permalink raw reply
* [HID Patchsets for Samsung driver v4 1/6] HID Samsung : Broaden device compatibility in samsung driver.
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
The USB validation check has been moved wherever its required.
Earlier Samsung driver only handles USB HID devices and returns an error if it encounters a Bluetooth type of HID device.
This changes improves driver compatibility and extends its support for a wide range of devices.
Signed-off-by: Sandeep C S <sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
---
drivers/hid/hid-samsung.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index cf5992e97094..3a8756232731 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -128,7 +128,7 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
- if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product)
+ if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product && hid_is_usb(hdev))
rdesc = samsung_irda_report_fixup(hdev, rdesc, rsize);
return rdesc;
}
@@ -139,7 +139,7 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
{
int ret = 0;
- if (USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE == hdev->product)
+ if (USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE == hdev->product && hid_is_usb(hdev))
ret = samsung_kbd_mouse_input_mapping(hdev,
hi, field, usage, bit, max);
@@ -152,9 +152,6 @@ static int samsung_probe(struct hid_device *hdev,
int ret;
unsigned int cmask = HID_CONNECT_DEFAULT;
- if (!hid_is_usb(hdev))
- return -EINVAL;
-
ret = hid_parse(hdev);
if (ret) {
hid_err(hdev, "parse failed\n");
@@ -162,6 +159,10 @@ static int samsung_probe(struct hid_device *hdev,
}
if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product) {
+ if (!hid_is_usb(hdev)) {
+ ret = -EINVAL;
+ goto err_free;
+ }
if (hdev->rsize == 184) {
/* disable hidinput, force hiddev */
cmask = (cmask & ~HID_CONNECT_HIDINPUT) |
--
2.34.1
^ permalink raw reply related
* [HID Patchsets for Samsung driver v4 2/6] HID: Samsung : Fix the checkpatch complain. Rewritten code using memcmp where applicable.
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
Resolved warnings found by checkpatch.pl script.
Signed-off-by: Sandeep C S <sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
---
drivers/hid/hid-samsung.c | 80 +++++++++++++++++++++++----------------
1 file changed, 47 insertions(+), 33 deletions(-)
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index 3a8756232731..97d0bf7d4d7e 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -58,33 +58,25 @@ static inline void samsung_irda_dev_trace(struct hid_device *hdev,
static __u8 *samsung_irda_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
- if (*rsize == 184 && rdesc[175] == 0x25 && rdesc[176] == 0x40 &&
- rdesc[177] == 0x75 && rdesc[178] == 0x30 &&
- rdesc[179] == 0x95 && rdesc[180] == 0x01 &&
+ if (*rsize == 184 && !memcmp(&rdesc[175], "\x25\x40\x75\x30\x95\x01", 6) &&
rdesc[182] == 0x40) {
samsung_irda_dev_trace(hdev, 184);
rdesc[176] = 0xff;
rdesc[178] = 0x08;
rdesc[180] = 0x06;
rdesc[182] = 0x42;
- } else
- if (*rsize == 203 && rdesc[192] == 0x15 && rdesc[193] == 0x0 &&
- rdesc[194] == 0x25 && rdesc[195] == 0x12) {
+ } else if (*rsize == 203 && !memcmp(&rdesc[192], "\x15\x00\x25\x12", 4)) {
samsung_irda_dev_trace(hdev, 203);
- rdesc[193] = 0x1;
- rdesc[195] = 0xf;
- } else
- if (*rsize == 135 && rdesc[124] == 0x15 && rdesc[125] == 0x0 &&
- rdesc[126] == 0x25 && rdesc[127] == 0x11) {
+ rdesc[193] = 0x01;
+ rdesc[195] = 0x0f;
+ } else if (*rsize == 135 && !memcmp(&rdesc[124], "\x15\x00\x25\x11", 4)) {
samsung_irda_dev_trace(hdev, 135);
- rdesc[125] = 0x1;
- rdesc[127] = 0xe;
- } else
- if (*rsize == 171 && rdesc[160] == 0x15 && rdesc[161] == 0x0 &&
- rdesc[162] == 0x25 && rdesc[163] == 0x01) {
+ rdesc[125] = 0x01;
+ rdesc[127] = 0x0e;
+ } else if (*rsize == 171 && !memcmp(&rdesc[160], "\x15\x00\x25\x01", 4)) {
samsung_irda_dev_trace(hdev, 171);
- rdesc[161] = 0x1;
- rdesc[163] = 0x3;
+ rdesc[161] = 0x01;
+ rdesc[163] = 0x03;
}
return rdesc;
}
@@ -99,7 +91,7 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
unsigned short ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
- if (1 != ifnum || HID_UP_CONSUMER != (usage->hid & HID_USAGE_PAGE))
+ if (ifnum != 1 || HID_UP_CONSUMER != (usage->hid & HID_USAGE_PAGE))
return 0;
dbg_hid("samsung wireless keyboard/mouse input mapping event [0x%x]\n",
@@ -107,17 +99,39 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
switch (usage->hid & HID_USAGE) {
/* report 2 */
- case 0x183: samsung_kbd_mouse_map_key_clear(KEY_MEDIA); break;
- case 0x195: samsung_kbd_mouse_map_key_clear(KEY_EMAIL); break;
- case 0x196: samsung_kbd_mouse_map_key_clear(KEY_CALC); break;
- case 0x197: samsung_kbd_mouse_map_key_clear(KEY_COMPUTER); break;
- case 0x22b: samsung_kbd_mouse_map_key_clear(KEY_SEARCH); break;
- case 0x22c: samsung_kbd_mouse_map_key_clear(KEY_WWW); break;
- case 0x22d: samsung_kbd_mouse_map_key_clear(KEY_BACK); break;
- case 0x22e: samsung_kbd_mouse_map_key_clear(KEY_FORWARD); break;
- case 0x22f: samsung_kbd_mouse_map_key_clear(KEY_FAVORITES); break;
- case 0x230: samsung_kbd_mouse_map_key_clear(KEY_REFRESH); break;
- case 0x231: samsung_kbd_mouse_map_key_clear(KEY_STOP); break;
+ case 0x183:
+ samsung_kbd_mouse_map_key_clear(KEY_MEDIA);
+ break;
+ case 0x195:
+ samsung_kbd_mouse_map_key_clear(KEY_EMAIL);
+ break;
+ case 0x196:
+ samsung_kbd_mouse_map_key_clear(KEY_CALC);
+ break;
+ case 0x197:
+ samsung_kbd_mouse_map_key_clear(KEY_COMPUTER);
+ break;
+ case 0x22b:
+ samsung_kbd_mouse_map_key_clear(KEY_SEARCH);
+ break;
+ case 0x22c:
+ samsung_kbd_mouse_map_key_clear(KEY_WWW);
+ break;
+ case 0x22d:
+ samsung_kbd_mouse_map_key_clear(KEY_BACK);
+ break;
+ case 0x22e:
+ samsung_kbd_mouse_map_key_clear(KEY_FORWARD);
+ break;
+ case 0x22f:
+ samsung_kbd_mouse_map_key_clear(KEY_FAVORITES);
+ break;
+ case 0x230:
+ samsung_kbd_mouse_map_key_clear(KEY_REFRESH);
+ break;
+ case 0x231:
+ samsung_kbd_mouse_map_key_clear(KEY_STOP);
+ break;
default:
return 0;
}
@@ -128,7 +142,7 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
- if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product && hid_is_usb(hdev))
+ if (hdev->product == USB_DEVICE_ID_SAMSUNG_IR_REMOTE && hid_is_usb(hdev))
rdesc = samsung_irda_report_fixup(hdev, rdesc, rsize);
return rdesc;
}
@@ -139,7 +153,7 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
{
int ret = 0;
- if (USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE == hdev->product && hid_is_usb(hdev))
+ if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE && hid_is_usb(hdev))
ret = samsung_kbd_mouse_input_mapping(hdev,
hi, field, usage, bit, max);
@@ -158,7 +172,7 @@ static int samsung_probe(struct hid_device *hdev,
goto err_free;
}
- if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product) {
+ if (hdev->product == USB_DEVICE_ID_SAMSUNG_IR_REMOTE) {
if (!hid_is_usb(hdev)) {
ret = -EINVAL;
goto err_free;
--
2.34.1
^ permalink raw reply related
* [HID Patchsets for Samsung driver v4 3/6] HID: Samsung : Add Samsung wireless keyboard support.
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
Add Support for samsung wireless keyboard with input mapping events.
Device 7021 (Samsung wireless keyboard)
Signed-off-by: Sandeep C S <sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
---
drivers/hid/hid-ids.h | 2 +
drivers/hid/hid-samsung.c | 97 +++++++++++++++++++++++++++++++++++++++
2 files changed, 99 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index fb30e228d35f..d7572ab5ea6c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1143,8 +1143,10 @@
#define USB_DEVICE_ID_SAITEK_X65 0x0b6a
#define USB_VENDOR_ID_SAMSUNG 0x0419
+#define USB_VENDOR_ID_SAMSUNG_ELECTRONICS 0x04e8
#define USB_DEVICE_ID_SAMSUNG_IR_REMOTE 0x0001
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE 0x0600
+#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD 0x7021
#define USB_VENDOR_ID_SEMICO 0x1a2c
#define USB_DEVICE_ID_SEMICO_USB_KEYKOARD 0x0023
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index 97d0bf7d4d7e..aa8e7805be76 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -139,6 +139,99 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
return 1;
}
+static int samsung_kbd_input_mapping(struct hid_device *hdev,
+ struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
+{
+ if (!(HID_UP_CONSUMER == (usage->hid & HID_USAGE_PAGE) ||
+ HID_UP_KEYBOARD == (usage->hid & HID_USAGE_PAGE)))
+ return 0;
+
+ dbg_hid("samsung wireless keyboard input mapping event [0x%x]\n",
+ usage->hid & HID_USAGE);
+
+ if (HID_UP_KEYBOARD == (usage->hid & HID_USAGE_PAGE)) {
+ set_bit(EV_REP, hi->input->evbit);
+ switch (usage->hid & HID_USAGE) {
+ case 0x32:
+ samsung_kbd_mouse_map_key_clear(KEY_BACKSLASH);
+ break;
+ case 0x64:
+ samsung_kbd_mouse_map_key_clear(KEY_102ND);
+ break;
+ /* Only for BR keyboard */
+ case 0x87:
+ samsung_kbd_mouse_map_key_clear(KEY_RO);
+ break;
+ default:
+ return 0;
+ }
+ }
+
+ if (HID_UP_CONSUMER == (usage->hid & HID_USAGE_PAGE)) {
+ switch (usage->hid & HID_USAGE) {
+ /* report 2 */
+ /* MENU */
+ case 0x040:
+ samsung_kbd_mouse_map_key_clear(KEY_MENU);
+ break;
+ case 0x18a:
+ samsung_kbd_mouse_map_key_clear(KEY_MAIL);
+ break;
+ case 0x196:
+ samsung_kbd_mouse_map_key_clear(KEY_WWW);
+ break;
+ case 0x19e:
+ samsung_kbd_mouse_map_key_clear(KEY_SCREENLOCK);
+ break;
+ case 0x221:
+ samsung_kbd_mouse_map_key_clear(KEY_SEARCH);
+ break;
+ case 0x223:
+ samsung_kbd_mouse_map_key_clear(KEY_HOMEPAGE);
+ break;
+ /* Smtart Voice Key */
+ case 0x300:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY13);
+ break;
+ /* RECENTAPPS */
+ case 0x301:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY1);
+ break;
+ /* APPLICATION */
+ case 0x302:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY2);
+ break;
+ /* Voice search */
+ case 0x305:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY4);
+ break;
+ /* QPANEL on/off */
+ case 0x306:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY5);
+ break;
+ /* SIP on/off */
+ case 0x307:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY3);
+ break;
+ /* LANG */
+ case 0x308:
+ samsung_kbd_mouse_map_key_clear(KEY_LANGUAGE);
+ break;
+ case 0x30a:
+ samsung_kbd_mouse_map_key_clear(KEY_BRIGHTNESSDOWN);
+ break;
+ case 0x30b:
+ samsung_kbd_mouse_map_key_clear(KEY_BRIGHTNESSUP);
+ break;
+ default:
+ return 0;
+ }
+ }
+
+ return 1;
+}
+
static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
@@ -156,6 +249,9 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE && hid_is_usb(hdev))
ret = samsung_kbd_mouse_input_mapping(hdev,
hi, field, usage, bit, max);
+ else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD)
+ ret = samsung_kbd_input_mapping(hdev,
+ hi, field, usage, bit, max);
return ret;
}
@@ -198,6 +294,7 @@ static int samsung_probe(struct hid_device *hdev,
static const struct hid_device_id samsung_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SAMSUNG, USB_DEVICE_ID_SAMSUNG_IR_REMOTE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SAMSUNG, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD) },
{ }
};
MODULE_DEVICE_TABLE(hid, samsung_devices);
--
2.34.1
^ permalink raw reply related
* [HID Patchsets for Samsung driver v4 4/6] HID: Samsung : Add Samsung wireless gamepad support.
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
Add support for samsung wireless gamepad with input mapping events.
Device a000 (Samsung wireless gamepad)
Signed-off-by: Sandeep C S <sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-samsung.c | 95 +++++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index d7572ab5ea6c..8a106fd6b339 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1147,6 +1147,7 @@
#define USB_DEVICE_ID_SAMSUNG_IR_REMOTE 0x0001
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE 0x0600
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD 0x7021
+#define USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD 0xa000
#define USB_VENDOR_ID_SEMICO 0x1a2c
#define USB_DEVICE_ID_SEMICO_USB_KEYKOARD 0x0023
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index aa8e7805be76..217b30b71bb8 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -232,6 +232,97 @@ static int samsung_kbd_input_mapping(struct hid_device *hdev,
return 1;
}
+static int samsung_gamepad_input_mapping(struct hid_device *hdev,
+ struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
+{
+ if (!(HID_UP_BUTTON == (usage->hid & HID_USAGE_PAGE) ||
+ HID_UP_CONSUMER == (usage->hid & HID_USAGE_PAGE)))
+ return 0;
+
+ dbg_hid("samsung wireless gamepad input mapping event [0x%x], %ld, %ld, [0x%x]\n",
+ usage->hid & HID_USAGE, hi->input->evbit[0], hi->input->absbit[0], usage->hid & HID_USAGE_PAGE);
+
+ if (HID_UP_BUTTON == (usage->hid & HID_USAGE_PAGE)) {
+ switch (usage->hid & HID_USAGE) {
+ case 0x01:
+ samsung_kbd_mouse_map_key_clear(BTN_A);
+ break;
+ case 0x02:
+ samsung_kbd_mouse_map_key_clear(BTN_B);
+ break;
+ case 0x03:
+ samsung_kbd_mouse_map_key_clear(BTN_C);
+ break;
+ case 0x04:
+ samsung_kbd_mouse_map_key_clear(BTN_X);
+ break;
+ case 0x05:
+ samsung_kbd_mouse_map_key_clear(BTN_Y);
+ break;
+ case 0x06:
+ samsung_kbd_mouse_map_key_clear(BTN_Z);
+ break;
+ case 0x07:
+ samsung_kbd_mouse_map_key_clear(BTN_TL);
+ break;
+ case 0x08:
+ samsung_kbd_mouse_map_key_clear(BTN_TR);
+ break;
+ case 0x09:
+ samsung_kbd_mouse_map_key_clear(BTN_TL2);
+ break;
+ case 0x0a:
+ samsung_kbd_mouse_map_key_clear(BTN_TR2);
+ break;
+ case 0x0b:
+ samsung_kbd_mouse_map_key_clear(BTN_SELECT);
+ break;
+ case 0x0c:
+ samsung_kbd_mouse_map_key_clear(BTN_START);
+ break;
+ case 0x0d:
+ samsung_kbd_mouse_map_key_clear(BTN_MODE);
+ break;
+ case 0x0e:
+ samsung_kbd_mouse_map_key_clear(BTN_THUMBL);
+ break;
+ case 0x0f:
+ samsung_kbd_mouse_map_key_clear(BTN_THUMBR);
+ break;
+ case 0x10:
+ samsung_kbd_mouse_map_key_clear(0x13f);
+ break;
+ default:
+ return 0;
+ }
+ }
+
+ if (HID_UP_CONSUMER == (usage->hid & HID_USAGE_PAGE)) {
+ switch (usage->hid & HID_USAGE) {
+ case 0x040:
+ samsung_kbd_mouse_map_key_clear(KEY_MENU);
+ break;
+ case 0x223:
+ samsung_kbd_mouse_map_key_clear(KEY_HOMEPAGE);
+ break;
+ case 0x224:
+ samsung_kbd_mouse_map_key_clear(KEY_BACK);
+ break;
+
+ /* Screen Capture */
+ case 0x303:
+ samsung_kbd_mouse_map_key_clear(KEY_SYSRQ);
+ break;
+
+ default:
+ return 0;
+ }
+ }
+
+ return 1;
+}
+
static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
@@ -252,6 +343,9 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD)
ret = samsung_kbd_input_mapping(hdev,
hi, field, usage, bit, max);
+ else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD)
+ ret = samsung_gamepad_input_mapping(hdev,
+ hi, field, usage, bit, max);
return ret;
}
@@ -295,6 +389,7 @@ static const struct hid_device_id samsung_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SAMSUNG, USB_DEVICE_ID_SAMSUNG_IR_REMOTE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SAMSUNG, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD) },
{ }
};
MODULE_DEVICE_TABLE(hid, samsung_devices);
--
2.34.1
^ permalink raw reply related
* [HID Patchsets for Samsung driver v4 5/6] HID: Samsung : Add Samsung wireless action mouse support.
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
Add support for samsung wireless action mouse with input mapping events.
Device a004 (Samsung wireless action mouse)
Signed-off-by: Sandeep C S <sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-samsung.c | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 8a106fd6b339..06bf718961da 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1148,6 +1148,7 @@
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE 0x0600
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD 0x7021
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD 0xa000
+#define USB_DEVICE_ID_SAMSUNG_WIRELESS_ACTIONMOUSE 0xa004
#define USB_VENDOR_ID_SEMICO 0x1a2c
#define USB_DEVICE_ID_SEMICO_USB_KEYKOARD 0x0023
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index 217b30b71bb8..0de23a70dddb 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -323,6 +323,29 @@ static int samsung_gamepad_input_mapping(struct hid_device *hdev,
return 1;
}
+static int samsung_actionmouse_input_mapping(struct hid_device *hdev,
+ struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
+{
+
+ dbg_hid("samsung wireless actionmouse input mapping event [0x%x], [0x%x], %ld, %ld, [0x%x]\n",
+ usage->hid, usage->hid & HID_USAGE, hi->input->evbit[0], hi->input->absbit[0],
+ usage->hid & HID_USAGE_PAGE);
+
+ if (((usage->hid & HID_USAGE_PAGE) != HID_UP_CONSUMER) && ((usage->hid & HID_USAGE_PAGE) != HID_UP_BUTTON))
+ return 0;
+
+ switch (usage->hid & HID_USAGE) {
+ case 0x301:
+ samsung_kbd_mouse_map_key_clear(254);
+ break;
+ default:
+ return 0;
+ }
+
+ return 1;
+}
+
static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
@@ -346,6 +369,9 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD)
ret = samsung_gamepad_input_mapping(hdev,
hi, field, usage, bit, max);
+ else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_ACTIONMOUSE)
+ ret = samsung_actionmouse_input_mapping(hdev,
+ hi, field, usage, bit, max);
return ret;
}
@@ -390,6 +416,7 @@ static const struct hid_device_id samsung_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_SAMSUNG, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD_MOUSE) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_ACTIONMOUSE) },
{ }
};
MODULE_DEVICE_TABLE(hid, samsung_devices);
--
2.34.1
^ permalink raw reply related
* [HID Patchsets for Samsung driver v4 6/6] HID: Samsung : Add Samsung wireless bookcover and universal keyboard support.
From: Sandeep C S @ 2024-01-25 4:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: gaudium.lee, ih0923.kim, suhyun_.kim, jitender.s21, junwan.cho,
sandeep.cs, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
Add support for samsung wireless bookcover and universal keyboard with input mapping events.
Device a005 (Samsung wireless bookcover keyboard)
Device a006 (Samsung wireless universal keyboard)
Device a064 (Samsung wireless multi hogp keyboard)
Signed-off-by: Sandeep C S <sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
Signed-off-by: Gwangho Lee <gaudium.lee@samsung.com>
---
drivers/hid/hid-ids.h | 3 +
drivers/hid/hid-samsung.c | 131 ++++++++++++++++++++++++++++++++++++++
2 files changed, 134 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 06bf718961da..221bae808c25 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1149,6 +1149,9 @@
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD 0x7021
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD 0xa000
#define USB_DEVICE_ID_SAMSUNG_WIRELESS_ACTIONMOUSE 0xa004
+#define USB_DEVICE_ID_SAMSUNG_WIRELESS_BOOKCOVER 0xa005
+#define USB_DEVICE_ID_SAMSUNG_WIRELESS_UNIVERSAL_KBD 0xa006
+#define USB_DEVICE_ID_SAMSUNG_WIRELESS_MULTI_HOGP_KBD 0xa064
#define USB_VENDOR_ID_SEMICO 0x1a2c
#define USB_DEVICE_ID_SEMICO_USB_KEYKOARD 0x0023
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index 0de23a70dddb..08fb25b8459a 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -346,6 +346,129 @@ static int samsung_actionmouse_input_mapping(struct hid_device *hdev,
return 1;
}
+static int samsung_universal_kbd_input_mapping(struct hid_device *hdev,
+ struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
+{
+ if (!(HID_UP_CONSUMER == (usage->hid & HID_USAGE_PAGE) ||
+ HID_UP_KEYBOARD == (usage->hid & HID_USAGE_PAGE)))
+ return 0;
+
+ dbg_hid("samsung wireless keyboard input mapping event [0x%x]\n",
+ usage->hid & HID_USAGE);
+
+ if (HID_UP_KEYBOARD == (usage->hid & HID_USAGE_PAGE)) {
+ set_bit(EV_REP, hi->input->evbit);
+ switch (usage->hid & HID_USAGE) {
+ case 0x32:
+ samsung_kbd_mouse_map_key_clear(KEY_BACKSLASH);
+ break;
+ case 0x64:
+ samsung_kbd_mouse_map_key_clear(KEY_102ND);
+ break;
+ /* Only for BR keyboard */
+ case 0x87:
+ samsung_kbd_mouse_map_key_clear(KEY_RO);
+ break;
+ default:
+ return 0;
+ }
+ }
+
+ if (HID_UP_CONSUMER == (usage->hid & HID_USAGE_PAGE)) {
+ switch (usage->hid & HID_USAGE) {
+ /* report 2 */
+ /* MENU */
+ case 0x040:
+ samsung_kbd_mouse_map_key_clear(KEY_MENU);
+ break;
+ case 0x18a:
+ samsung_kbd_mouse_map_key_clear(KEY_MAIL);
+ break;
+ case 0x196:
+ samsung_kbd_mouse_map_key_clear(KEY_WWW);
+ break;
+ case 0x19e:
+ samsung_kbd_mouse_map_key_clear(KEY_SCREENLOCK);
+ break;
+ case 0x221:
+ samsung_kbd_mouse_map_key_clear(KEY_SEARCH);
+ break;
+ case 0x223:
+ samsung_kbd_mouse_map_key_clear(KEY_HOMEPAGE);
+ break;
+ /* RECENTAPPS */
+ case 0x301:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY1);
+ break;
+ /* APPLICATION */
+ case 0x302:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY2);
+ break;
+ /* Voice search */
+ case 0x305:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY4);
+ break;
+ /* QPANEL on/off */
+ case 0x306:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY5);
+ break;
+ /* SIP on/off */
+ case 0x307:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY3);
+ break;
+ /* LANG */
+ case 0x308:
+ samsung_kbd_mouse_map_key_clear(KEY_LANGUAGE);
+ break;
+ case 0x30a:
+ samsung_kbd_mouse_map_key_clear(KEY_BRIGHTNESSDOWN);
+ break;
+ case 0x070:
+ samsung_kbd_mouse_map_key_clear(KEY_BRIGHTNESSDOWN);
+ break;
+ case 0x30b:
+ samsung_kbd_mouse_map_key_clear(KEY_BRIGHTNESSUP);
+ break;
+ case 0x06f:
+ samsung_kbd_mouse_map_key_clear(KEY_BRIGHTNESSUP);
+ break;
+ /* S-Finder */
+ case 0x304:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY7);
+ break;
+ /* Screen Capture */
+ case 0x303:
+ samsung_kbd_mouse_map_key_clear(KEY_SYSRQ);
+ break;
+ /* Multi Window */
+ case 0x309:
+ samsung_kbd_mouse_map_key_clear(BTN_TRIGGER_HAPPY9);
+ break;
+ /* HotKey App 1 */
+ case 0x071:
+ samsung_kbd_mouse_map_key_clear(0x2f5);
+ break;
+ /* HotKey App 2 */
+ case 0x072:
+ samsung_kbd_mouse_map_key_clear(0x2f6);
+ break;
+ /* HotKey App 3 */
+ case 0x073:
+ samsung_kbd_mouse_map_key_clear(0x2f7);
+ break;
+ /* Dex */
+ case 0x06e:
+ samsung_kbd_mouse_map_key_clear(0x2bd);
+ break;
+ default:
+ return 0;
+ }
+ }
+
+ return 1;
+}
+
static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
{
@@ -372,6 +495,12 @@ static int samsung_input_mapping(struct hid_device *hdev, struct hid_input *hi,
else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_ACTIONMOUSE)
ret = samsung_actionmouse_input_mapping(hdev,
hi, field, usage, bit, max);
+ else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_UNIVERSAL_KBD)
+ ret = samsung_universal_kbd_input_mapping(hdev,
+ hi, field, usage, bit, max);
+ else if (hdev->product == USB_DEVICE_ID_SAMSUNG_WIRELESS_MULTI_HOGP_KBD)
+ ret = samsung_universal_kbd_input_mapping(hdev,
+ hi, field, usage, bit, max);
return ret;
}
@@ -417,6 +546,8 @@ static const struct hid_device_id samsung_devices[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_KBD) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_GAMEPAD) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_ACTIONMOUSE) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_UNIVERSAL_KBD) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SAMSUNG_ELECTRONICS, USB_DEVICE_ID_SAMSUNG_WIRELESS_MULTI_HOGP_KBD) },
{ }
};
MODULE_DEVICE_TABLE(hid, samsung_devices);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 1/4] iio: hid-sensor-als: Assign channels dynamically
From: Basavaraj Natikar @ 2024-01-25 5:51 UTC (permalink / raw)
To: Srinivas Pandruvada, jikos, jic23, lars, Basavaraj.Natikar
Cc: linux-input, linux-iio, linux-kernel
In-Reply-To: <20240109180007.3373784-2-srinivas.pandruvada@linux.intel.com>
Hi Srinivas,
On 1/9/2024 11:30 PM, Srinivas Pandruvada wrote:
> Instead of assuming that every channel defined statically by
> als_channels[] is present, assign dynamically based on presence of the
> respective usage id in the descriptor. This will allow to register ALS
> with limited channel support. Append the timestamp as the last channel.
>
> When not all usage ids are present, the scan index is adjusted to
> exclude unsupported channels.
>
> There is no intentional function changes done.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v3:
> Addressed comments from Jonthan:
> - Remove channel allocation and move to iio_priv()
> - Parse all usage IDs in a single loop and continue
> for failure. This way the temperature and chromaticity
> will not need any special processing to parse usage ids.
> - Don't leave empty channel indexes
>
> v2:
> New change
>
> drivers/iio/light/hid-sensor-als.c | 56 +++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 5cd27f04b45e..72a7c01c97f8 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -25,17 +25,26 @@ struct als_state {
> struct hid_sensor_hub_callbacks callbacks;
> struct hid_sensor_common common_attributes;
> struct hid_sensor_hub_attribute_info als[CHANNEL_SCAN_INDEX_MAX];
> + struct iio_chan_spec channels[CHANNEL_SCAN_INDEX_MAX + 1];
> struct {
> u32 illum[CHANNEL_SCAN_INDEX_MAX];
> + u32 scan_index[CHANNEL_SCAN_INDEX_MAX];
> u64 timestamp __aligned(8);
> } scan;
> int scale_pre_decml;
> int scale_post_decml;
> int scale_precision;
> int value_offset;
> + int num_channels;
> s64 timestamp;
> };
>
> +/* The order of usage ids must match scan index starting from CHANNEL_SCAN_INDEX_INTENSITY */
> +static const u32 als_usage_ids[] = {
> + HID_USAGE_SENSOR_LIGHT_ILLUM,
> + HID_USAGE_SENSOR_LIGHT_ILLUM,
> +};
> +
> static const u32 als_sensitivity_addresses[] = {
> HID_USAGE_SENSOR_DATA_LIGHT,
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> @@ -216,11 +225,14 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> struct als_state *als_state = iio_priv(indio_dev);
> int ret = -EINVAL;
> u32 sample_data = *(u32 *)raw_data;
> + int scan_index;
>
> switch (usage_id) {
> case HID_USAGE_SENSOR_LIGHT_ILLUM:
> - als_state->scan.illum[CHANNEL_SCAN_INDEX_INTENSITY] = sample_data;
> - als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] = sample_data;
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_INTENSITY];
> + als_state->scan.illum[scan_index] = sample_data;
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_ILLUM];
> + als_state->scan.illum[scan_index] = sample_data;
Could you please use original above change which works fine for some reason if
we use this change als_state->scan.scan_index we are getting garbage value.
> ret = 0;
> break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> @@ -237,27 +249,39 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> /* Parse report which is specific to an usage id*/
> static int als_parse_report(struct platform_device *pdev,
> struct hid_sensor_hub_device *hsdev,
> - struct iio_chan_spec *channels,
> unsigned usage_id,
> struct als_state *st)
> {
> - int ret;
> + struct iio_chan_spec *channels;
> + int ret, index = 0;
> int i;
>
> + channels = st->channels;
> +
> for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
Could you please change CHANNEL_SCAN_INDEX_ILLUM to CHANNEL_SCAN_INDEX_MAX
which helps remaining 2 patches in the series.
> ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> usage_id,
> - HID_USAGE_SENSOR_LIGHT_ILLUM,
> + als_usage_ids[i],
> &st->als[i]);
> if (ret < 0)
> - return ret;
> - als_adjust_channel_bit_mask(channels, i, st->als[i].size);
> + continue;
> +
> + channels[index] = als_channels[i];
> + st->scan.scan_index[i] = index;
> +
> + als_adjust_channel_bit_mask(channels, index, st->als[i].size);
> + ++index;
>
> dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> st->als[i].report_id);
> }
>
> + st->num_channels = index;
> + /* Return success even if one usage id is present */
> + if (index)
> + ret = 0;
> +
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st->als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st->scale_post_decml);
> @@ -293,15 +317,7 @@ static int hid_als_probe(struct platform_device *pdev)
> return ret;
> }
>
> - indio_dev->channels = devm_kmemdup(&pdev->dev, als_channels,
> - sizeof(als_channels), GFP_KERNEL);
> - if (!indio_dev->channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> - return -ENOMEM;
> - }
> -
> ret = als_parse_report(pdev, hsdev,
> - (struct iio_chan_spec *)indio_dev->channels,
> hsdev->usage,
> als_state);
> if (ret) {
> @@ -309,8 +325,14 @@ static int hid_als_probe(struct platform_device *pdev)
> return ret;
> }
>
> - indio_dev->num_channels =
> - ARRAY_SIZE(als_channels);
> + /* Add timestamp channel */
> + als_state->channels[als_state->num_channels] = als_channels[CHANNEL_SCAN_INDEX_TIMESTAMP];
> + als_state->channels[als_state->num_channels].scan_index = als_state->num_channels;
> +
> + /* +1 for adding timestamp channel */
> + indio_dev->num_channels = als_state->num_channels + 1;
> +
> + indio_dev->channels = als_state->channels;
> indio_dev->info = &als_info;
> indio_dev->name = name;
> indio_dev->modes = INDIO_DIRECT_MODE;
^ permalink raw reply
* Re: [PATCH v3 3/4] iio: hid-sensor-als: Add light color temperature support
From: Basavaraj Natikar @ 2024-01-25 5:55 UTC (permalink / raw)
To: Srinivas Pandruvada, jikos, jic23, lars, Basavaraj.Natikar
Cc: linux-input, linux-iio, linux-kernel
In-Reply-To: <20240109180007.3373784-4-srinivas.pandruvada@linux.intel.com>
Hi Srinivas,
On 1/9/2024 11:30 PM, Srinivas Pandruvada wrote:
> From: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>
> On some platforms, ambient color sensors also support light color
> temperature. Add support of light color temperature.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>
> I don't have a system to test this patch.
> Hi Basavraj,
> Please test.
After fixing both comments in patch 1 all works fine.
Thanks,
--
Basavaraj
>
> v3:
> Simplilified as no special processing is required in als_parse_report()
> v2:
> Original patch from Basavaraj Natikar <Basavaraj.Natikar@amd.com> is
> modified to prevent failure when the new usage id is not found in the
> descriptor.
>
> drivers/iio/light/hid-sensor-als.c | 22 ++++++++++++++++++++++
> include/linux/hid-sensor-ids.h | 1 +
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index a7bde6b68102..0d54eb59e47d 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -16,6 +16,7 @@
> enum {
> CHANNEL_SCAN_INDEX_INTENSITY,
> CHANNEL_SCAN_INDEX_ILLUM,
> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -43,6 +44,7 @@ struct als_state {
> static const u32 als_usage_ids[] = {
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> + HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE,
> };
>
> static const u32 als_sensitivity_addresses[] = {
> @@ -74,6 +76,16 @@ static const struct iio_chan_spec als_channels[] = {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_ILLUM,
> },
> + {
> + .type = IIO_COLORTEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + },
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> @@ -112,6 +124,11 @@ static int als_read_raw(struct iio_dev *indio_dev,
> min = als_state->als[chan->scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> + case CHANNEL_SCAN_INDEX_COLOR_TEMP:
> + report_id = als_state->als[chan->scan_index].report_id;
> + min = als_state->als[chan->scan_index].logical_minimum;
> + address = HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> + break;
> default:
> report_id = -1;
> break;
> @@ -235,6 +252,11 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> als_state->scan.illum[scan_index] = sample_data;
> ret = 0;
> break;
> + case HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE:
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_COLOR_TEMP];
> + als_state->scan.illum[scan_index] = sample_data;
> + ret = 0;
> + break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> als_state->timestamp = hid_sensor_convert_timestamp(&als_state->common_attributes,
> *(s64 *)raw_data);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index 13b1e65fbdcc..8af4fb3e0254 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -21,6 +21,7 @@
> #define HID_USAGE_SENSOR_ALS 0x200041
> #define HID_USAGE_SENSOR_DATA_LIGHT 0x2004d0
> #define HID_USAGE_SENSOR_LIGHT_ILLUM 0x2004d1
> +#define HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE 0x2004d2
>
> /* PROX (200011) */
> #define HID_USAGE_SENSOR_PROX 0x200011
^ permalink raw reply
* Re: [PATCH v3 4/4] iio: hid-sensor-als: Add light chromaticity support
From: Basavaraj Natikar @ 2024-01-25 5:56 UTC (permalink / raw)
To: Srinivas Pandruvada, jikos, jic23, lars, Basavaraj.Natikar
Cc: linux-input, linux-iio, linux-kernel
In-Reply-To: <20240109180007.3373784-5-srinivas.pandruvada@linux.intel.com>
Hi Srinivas,
On 1/9/2024 11:30 PM, Srinivas Pandruvada wrote:
> From: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>
> On some platforms, ambient color sensors also support the x and y light
> colors, which represent the coordinates on the CIE 1931 chromaticity
> diagram. Add light chromaticity x and y.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> I don't have a system to test this patch.
> Hi Basavraj,
> Please test.
After fixing both comments in patch 1 all works fine.
Thanks,
--
Basavaraj
>
> v3:
> Simplilified as no special processing is required in als_parse_report()
>
> v2:
> Original patch from Basavaraj Natikar <Basavaraj.Natikar@amd.com> is
> modified to prevent failure when the new usage id is not found in the
> descriptor.
>
> drivers/iio/light/hid-sensor-als.c | 48 ++++++++++++++++++++++++++++++
> include/linux/hid-sensor-ids.h | 3 ++
> 2 files changed, 51 insertions(+)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 0d54eb59e47d..9c31febc84b8 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -17,6 +17,8 @@ enum {
> CHANNEL_SCAN_INDEX_INTENSITY,
> CHANNEL_SCAN_INDEX_ILLUM,
> CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + CHANNEL_SCAN_INDEX_CHROMATICITY_X,
> + CHANNEL_SCAN_INDEX_CHROMATICITY_Y,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -45,6 +47,8 @@ static const u32 als_usage_ids[] = {
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE,
> + HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X,
> + HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y,
> };
>
> static const u32 als_sensitivity_addresses[] = {
> @@ -86,6 +90,30 @@ static const struct iio_chan_spec als_channels[] = {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_COLOR_TEMP,
> },
> + {
> + .type = IIO_CHROMATICITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_X,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_X,
> + },
> + {
> + .type = IIO_CHROMATICITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_Y,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_Y,
> + },
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> @@ -129,6 +157,16 @@ static int als_read_raw(struct iio_dev *indio_dev,
> min = als_state->als[chan->scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> break;
> + case CHANNEL_SCAN_INDEX_CHROMATICITY_X:
> + report_id = als_state->als[chan->scan_index].report_id;
> + min = als_state->als[chan->scan_index].logical_minimum;
> + address = HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X;
> + break;
> + case CHANNEL_SCAN_INDEX_CHROMATICITY_Y:
> + report_id = als_state->als[chan->scan_index].report_id;
> + min = als_state->als[chan->scan_index].logical_minimum;
> + address = HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y;
> + break;
> default:
> report_id = -1;
> break;
> @@ -257,6 +295,16 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> als_state->scan.illum[scan_index] = sample_data;
> ret = 0;
> break;
> + case HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X:
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_CHROMATICITY_X];
> + als_state->scan.illum[scan_index] = sample_data;
> + ret = 0;
> + break;
> + case HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y:
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_CHROMATICITY_Y];
> + als_state->scan.illum[scan_index] = sample_data;
> + ret = 0;
> + break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> als_state->timestamp = hid_sensor_convert_timestamp(&als_state->common_attributes,
> *(s64 *)raw_data);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index 8af4fb3e0254..6730ee900ee1 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -22,6 +22,9 @@
> #define HID_USAGE_SENSOR_DATA_LIGHT 0x2004d0
> #define HID_USAGE_SENSOR_LIGHT_ILLUM 0x2004d1
> #define HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE 0x2004d2
> +#define HID_USAGE_SENSOR_LIGHT_CHROMATICITY 0x2004d3
> +#define HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X 0x2004d4
> +#define HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y 0x2004d5
>
> /* PROX (200011) */
> #define HID_USAGE_SENSOR_PROX 0x200011
^ permalink raw reply
* Re: [PATCH v3 1/4] iio: hid-sensor-als: Assign channels dynamically
From: Srinivas Pandruvada @ 2024-01-25 6:01 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, jic23, lars, Basavaraj.Natikar
Cc: linux-input, linux-iio, linux-kernel
In-Reply-To: <20defa34-f275-4d9f-95d8-788cb2eb55dc@amd.com>
Hi Basavraj,
On 1/24/24 21:51, Basavaraj Natikar wrote:
> Hi Srinivas,
>
>
> On 1/9/2024 11:30 PM, Srinivas Pandruvada wrote:
>> Instead of assuming that every channel defined statically by
>> als_channels[] is present, assign dynamically based on presence of the
>> respective usage id in the descriptor. This will allow to register ALS
>> with limited channel support. Append the timestamp as the last channel.
>>
>> When not all usage ids are present, the scan index is adjusted to
>> exclude unsupported channels.
>>
>> There is no intentional function changes done.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>> v3:
>> Addressed comments from Jonthan:
>> - Remove channel allocation and move to iio_priv()
>> - Parse all usage IDs in a single loop and continue
>> for failure. This way the temperature and chromaticity
>> will not need any special processing to parse usage ids.
>> - Don't leave empty channel indexes
>>
>> v2:
>> New change
>>
>> drivers/iio/light/hid-sensor-als.c | 56 +++++++++++++++++++++---------
>> 1 file changed, 39 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
>> index 5cd27f04b45e..72a7c01c97f8 100644
>> --- a/drivers/iio/light/hid-sensor-als.c
>> +++ b/drivers/iio/light/hid-sensor-als.c
>> @@ -25,17 +25,26 @@ struct als_state {
>> struct hid_sensor_hub_callbacks callbacks;
>> struct hid_sensor_common common_attributes;
>> struct hid_sensor_hub_attribute_info als[CHANNEL_SCAN_INDEX_MAX];
>> + struct iio_chan_spec channels[CHANNEL_SCAN_INDEX_MAX + 1];
>> struct {
>> u32 illum[CHANNEL_SCAN_INDEX_MAX];
>> + u32 scan_index[CHANNEL_SCAN_INDEX_MAX];
>> u64 timestamp __aligned(8);
>> } scan;
>> int scale_pre_decml;
>> int scale_post_decml;
>> int scale_precision;
>> int value_offset;
>> + int num_channels;
>> s64 timestamp;
>> };
>>
>> +/* The order of usage ids must match scan index starting from CHANNEL_SCAN_INDEX_INTENSITY */
>> +static const u32 als_usage_ids[] = {
>> + HID_USAGE_SENSOR_LIGHT_ILLUM,
>> + HID_USAGE_SENSOR_LIGHT_ILLUM,
>> +};
>> +
>> static const u32 als_sensitivity_addresses[] = {
>> HID_USAGE_SENSOR_DATA_LIGHT,
>> HID_USAGE_SENSOR_LIGHT_ILLUM,
>> @@ -216,11 +225,14 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
>> struct als_state *als_state = iio_priv(indio_dev);
>> int ret = -EINVAL;
>> u32 sample_data = *(u32 *)raw_data;
>> + int scan_index;
>>
>> switch (usage_id) {
>> case HID_USAGE_SENSOR_LIGHT_ILLUM:
>> - als_state->scan.illum[CHANNEL_SCAN_INDEX_INTENSITY] = sample_data;
>> - als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] = sample_data;
>> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_INTENSITY];
>> + als_state->scan.illum[scan_index] = sample_data;
>> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_ILLUM];
>> + als_state->scan.illum[scan_index] = sample_data;
> Could you please use original above change which works fine for some reason if
> we use this change als_state->scan.scan_index we are getting garbage value.
I think this is because of the issue which you found later for this patch.
>> ret = 0;
>> break;
>> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
>> @@ -237,27 +249,39 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
>> /* Parse report which is specific to an usage id*/
>> static int als_parse_report(struct platform_device *pdev,
>> struct hid_sensor_hub_device *hsdev,
>> - struct iio_chan_spec *channels,
>> unsigned usage_id,
>> struct als_state *st)
>> {
>> - int ret;
>> + struct iio_chan_spec *channels;
>> + int ret, index = 0;
>> int i;
>>
>> + channels = st->channels;
>> +
>> for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> Could you please change CHANNEL_SCAN_INDEX_ILLUM to CHANNEL_SCAN_INDEX_MAX
> which helps remaining 2 patches in the series.
>
Makes sense.
Thanks,
Srinivas
>> ret = sensor_hub_input_get_attribute_info(hsdev,
>> HID_INPUT_REPORT,
>> usage_id,
>> - HID_USAGE_SENSOR_LIGHT_ILLUM,
>> + als_usage_ids[i],
>> &st->als[i]);
>> if (ret < 0)
>> - return ret;
>> - als_adjust_channel_bit_mask(channels, i, st->als[i].size);
>> + continue;
>> +
>> + channels[index] = als_channels[i];
>> + st->scan.scan_index[i] = index;
>> +
>> + als_adjust_channel_bit_mask(channels, index, st->als[i].size);
>> + ++index;
>>
>> dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
>> st->als[i].report_id);
>> }
>>
>> + st->num_channels = index;
>> + /* Return success even if one usage id is present */
>> + if (index)
>> + ret = 0;
>> +
>> st->scale_precision = hid_sensor_format_scale(usage_id,
>> &st->als[CHANNEL_SCAN_INDEX_INTENSITY],
>> &st->scale_pre_decml, &st->scale_post_decml);
>> @@ -293,15 +317,7 @@ static int hid_als_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> - indio_dev->channels = devm_kmemdup(&pdev->dev, als_channels,
>> - sizeof(als_channels), GFP_KERNEL);
>> - if (!indio_dev->channels) {
>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>> - return -ENOMEM;
>> - }
>> -
>> ret = als_parse_report(pdev, hsdev,
>> - (struct iio_chan_spec *)indio_dev->channels,
>> hsdev->usage,
>> als_state);
>> if (ret) {
>> @@ -309,8 +325,14 @@ static int hid_als_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> - indio_dev->num_channels =
>> - ARRAY_SIZE(als_channels);
>> + /* Add timestamp channel */
>> + als_state->channels[als_state->num_channels] = als_channels[CHANNEL_SCAN_INDEX_TIMESTAMP];
>> + als_state->channels[als_state->num_channels].scan_index = als_state->num_channels;
>> +
>> + /* +1 for adding timestamp channel */
>> + indio_dev->num_channels = als_state->num_channels + 1;
>> +
>> + indio_dev->channels = als_state->channels;
>> indio_dev->info = &als_info;
>> indio_dev->name = name;
>> indio_dev->modes = INDIO_DIRECT_MODE;
^ permalink raw reply
* [PATCH] HID: hidraw: fix a problem of memory leak in hidraw_release()
From: Su Hui @ 2024-01-25 6:32 UTC (permalink / raw)
To: jikos, benjamin.tissoires
Cc: Su Hui, mail, linux-input, linux-kernel, kernel-janitors
'struct hidraw_list' is a circular queue whose head can be smaller than
tail. Using 'list->tail != list->head' to release all memory that should
be released.
Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
drivers/hid/hidraw.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 13c8dd8cd350..2bc762d31ac7 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -357,8 +357,11 @@ static int hidraw_release(struct inode * inode, struct file * file)
down_write(&minors_rwsem);
spin_lock_irqsave(&hidraw_table[minor]->list_lock, flags);
- for (int i = list->tail; i < list->head; i++)
- kfree(list->buffer[i].value);
+ while (list->tail != list->head) {
+ kfree(list->buffer[list->tail].value);
+ list->buffer[list->tail].value = NULL;
+ list->tail = (list->tail + 1) & (HIDRAW_BUFFER_SIZE - 1);
+ }
list_del(&list->node);
spin_unlock_irqrestore(&hidraw_table[minor]->list_lock, flags);
kfree(list);
--
2.30.2
^ permalink raw reply related
* Re: [PATCH] HID: hidraw: fix a problem of memory leak in hidraw_release()
From: Dan Carpenter @ 2024-01-25 7:11 UTC (permalink / raw)
To: Su Hui
Cc: jikos, benjamin.tissoires, mail, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <20240125063225.2796234-1-suhui@nfschina.com>
On Thu, Jan 25, 2024 at 02:32:26PM +0800, Su Hui wrote:
> 'struct hidraw_list' is a circular queue whose head can be smaller than
> tail. Using 'list->tail != list->head' to release all memory that should
> be released.
>
> Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
> Signed-off-by: Su Hui <suhui@nfschina.com>
This is very clever. How did you find that? Was it through static
analysis or just review? Perhaps using syzkaller?
Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
I imagine we could write a checker heuristic to identify ->tail and
->head struct members and then complain if they were ever used in a <
or > comparison.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] HID: hidraw: fix a problem of memory leak in hidraw_release()
From: Su Hui @ 2024-01-25 7:30 UTC (permalink / raw)
To: Dan Carpenter
Cc: jikos, benjamin.tissoires, mail, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <76300deb-f532-4d74-a97a-4cd321ae8f41@moroto.mountain>
On 2024/1/25 15:11, Dan Carpenter wrote:
> On Thu, Jan 25, 2024 at 02:32:26PM +0800, Su Hui wrote:
>> 'struct hidraw_list' is a circular queue whose head can be smaller than
>> tail. Using 'list->tail != list->head' to release all memory that should
>> be released.
>>
>> Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
>> Signed-off-by: Su Hui <suhui@nfschina.com>
> This is very clever. How did you find that? Was it through static
> analysis or just review? Perhaps using syzkaller?
Hi,
I just met this bug on a real machine and found this problem by
reviewing the code.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
>
> I imagine we could write a checker heuristic to identify ->tail and
> ->head struct members and then complain if they were ever used in a <
> or > comparison.
I can't agree any more, great idea!
Su Hui
^ permalink raw reply
* Re: [PATCH] HID: hidraw: fix a problem of memory leak in hidraw_release()
From: Jiri Kosina @ 2024-01-25 7:35 UTC (permalink / raw)
To: Su Hui; +Cc: benjamin.tissoires, mail, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <20240125063225.2796234-1-suhui@nfschina.com>
On Thu, 25 Jan 2024, Su Hui wrote:
> 'struct hidraw_list' is a circular queue whose head can be smaller than
> tail. Using 'list->tail != list->head' to release all memory that should
> be released.
>
> Fixes: a5623a203cff ("HID: hidraw: fix memory leak in hidraw_release()")
> Signed-off-by: Su Hui <suhui@nfschina.com>
Good catch, thanks. Applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [HID Patchsets for Samsung driver v4 0/6] Patchsets for Samsung driver
From: Jiri Kosina @ 2024-01-25 8:24 UTC (permalink / raw)
To: Sandeep C S
Cc: Benjamin Tissoires, gaudium.lee, ih0923.kim, suhyun_.kim,
jitender.s21, junwan.cho, linux-input, linux-kernel
In-Reply-To: <20240125043630.4031634-1-sandeep.cs@samsung.com>
On Thu, 25 Jan 2024, Sandeep C S wrote:
> Dear Jiri & Team,
>
> I hope this email finds you well. As per the review comments given by
> Mr. Jiri and Mr. Joe Perches in our last converstaion over mail. We have
> incorporated the feedback on our driver. Please check this set of series
> and help us to improve samsung driver.
>
> As of today, Opensource kernel Samsung driver only supports USB HID
> devices and do not have support for Bluetooth HID devices. Samsung would
> like to improve the samsung driver and extend it's support for bluetooth
> devices as well.
>
> Summary of changes in Samsung driver:
> 1. Add support for below bluetooth devices
>
> Samsung wireless Keyboard
> Samsung wireless GamePad
> Samsung Wireless Action Mouse
> Samsung Wireless Book Cover
> Samsung Wireless Universal Keyboard
> Samsung Wireless HOGP Keyboard
> 2. Add support for Special key processing on each of the above devices.
I have updated the changelogs so that they are up to the subsystem
standards (please check the git tree and try to adhere to similar format
for your next submissions), and applied, thanks for the patches!
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* RE: [HID Patchsets for Samsung driver v4 0/6] Patchsets for Samsung driver
From: sandeep.cs @ 2024-01-25 9:39 UTC (permalink / raw)
To: 'Jiri Kosina'
Cc: 'Benjamin Tissoires', gaudium.lee, ih0923.kim,
suhyun_.kim, jitender.s21, junwan.cho, linux-input, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.2401250923470.29548@cbobk.fhfr.pm>
>> Dear Jiri & Team,
>>
>> I hope this email finds you well. As per the review comments given by
>> Mr. Jiri and Mr. Joe Perches in our last converstaion over mail. We
>> have incorporated the feedback on our driver. Please check this set of
>> series and help us to improve samsung driver.
>>
>> As of today, Opensource kernel Samsung driver only supports USB HID
>> devices and do not have support for Bluetooth HID devices. Samsung
>> would like to improve the samsung driver and extend it's support for
>> bluetooth devices as well.
>>
>> Summary of changes in Samsung driver:
>> 1. Add support for below bluetooth devices
>>
>> Samsung wireless Keyboard
>> Samsung wireless GamePad
>> Samsung Wireless Action Mouse
>> Samsung Wireless Book Cover
>> Samsung Wireless Universal Keyboard
>> Samsung Wireless HOGP Keyboard
>> 2. Add support for Special key processing on each of the above devices.
>
>I have updated the changelogs so that they are up to the subsystem
standards
>(please check the git tree and try to adhere to similar format for your
next
>submissions), and applied, thanks for the patches!
Thank you for accepting our changes and for your feedback.
We will make sure to adhere to the subsystem standard for future
submissions.
Thanks & regards
Sandeep C S
^ permalink raw reply
* RE: [HID Patchsets for Samsung driver v4 0/6] Patchsets for Samsung driver
From: sandeep.cs @ 2024-01-25 9:41 UTC (permalink / raw)
To: 'Jiri Kosina'
Cc: 'Benjamin Tissoires', gaudium.lee, ih0923.kim,
suhyun_.kim, jitender.s21, junwan.cho, linux-input, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.2401250923470.29548@cbobk.fhfr.pm>
>On Thu, 25 Jan 2024, Sandeep C S wrote:
>
>> Dear Jiri & Team,
>>
>> I hope this email finds you well. As per the review comments given by
>> Mr. Jiri and Mr. Joe Perches in our last converstaion over mail. We
>> have incorporated the feedback on our driver. Please check this set of
>> series and help us to improve samsung driver.
>>
>> As of today, Opensource kernel Samsung driver only supports USB HID
>> devices and do not have support for Bluetooth HID devices. Samsung
>> would like to improve the samsung driver and extend it's support for
>> bluetooth devices as well.
>>
>> Summary of changes in Samsung driver:
>> 1. Add support for below bluetooth devices
>>
>> Samsung wireless Keyboard
>> Samsung wireless GamePad
>> Samsung Wireless Action Mouse
>> Samsung Wireless Book Cover
>> Samsung Wireless Universal Keyboard
>> Samsung Wireless HOGP Keyboard
>> 2. Add support for Special key processing on each of the above devices.
>
>I have updated the changelogs so that they are up to the subsystem
standards
>(please check the git tree and try to adhere to similar format for your
next
>submissions), and applied, thanks for the patches!
>
>--
>Jiri Kosina
>SUSE Labs
Thank you for accepting our changes! We will make sure follow the same
format for future submissions as well.
Thanks & regards
Sandeep C S
^ permalink raw reply
* Re: [PATCH v2 4/4] Input: da9063 - Add polling support
From: Biju Das @ 2024-01-25 11:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, linux-renesas-soc
In-Reply-To: <ZYD3cvHBHMZoACnQ@google.com>
Hi Dmitry,
Thanks for the feedback.
On Tue, Dec 19, 2023 at 1:52 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Dec 13, 2023 at 09:48:03PM +0000, Biju Das wrote:
> > +static void da9063_onkey_polled_poll(struct input_dev *input)
> > +{
> > + struct da9063_onkey *onkey = input_get_drvdata(input);
> > + const struct da906x_chip_config *config = onkey->config;
> > + unsigned int val;
> > + int error;
> > +
> > + error = regmap_read(onkey->regmap, config->onkey_status, &val);
> > + if (onkey->key_power && !error && (val & config->onkey_nonkey_mask)) {
> > + input_report_key(onkey->input, KEY_POWER, 1);
> > + input_sync(onkey->input);
> > + schedule_delayed_work(&onkey->work, 0);
>
> In the polling case you should not be scheduling any additional works as
> the driver may get confused if you repeatedly open and close input
> device.
OK.
>
> Also I think in threaded case it might be cleaner to avoid scheduling
> work and simply loop in the interrupt thread (since it can sleep).
Agreed. Will fix this in the next version.
Cheers,
Biju
^ permalink raw reply
* Re: [RFC PATCH 2/5] mfd: add 88pm88x driver
From: Lee Jones @ 2024-01-25 12:26 UTC (permalink / raw)
To: Karel Balej
Cc: Karel Balej, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20231217131838.7569-3-karelb@gimli.ms.mff.cuni.cz>
On Sun, 17 Dec 2023, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> Marvell 88PM880 and 8PM886 are two similar PMICs with mostly matching
> register mapping. They provide various functions such as onkey, battery,
> charger and regulators.
>
> Add support for 88PM886 found for instance in the samsung,coreprimevelte
> smartphone with which this was tested. Support for 88PM880 is not
> implemented here but should be straightforward to add.
>
> Implement only the most basic support omitting the currently unused
> registers and I2C subclients which should thus be added with the
> respective subdevices. However, add support for the onkey already.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
> drivers/mfd/88pm88x.c | 199 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/Kconfig | 11 ++
> drivers/mfd/Makefile | 1 +
> include/linux/mfd/88pm88x.h | 60 +++++++++++
> 4 files changed, 271 insertions(+)
> create mode 100644 drivers/mfd/88pm88x.c
> create mode 100644 include/linux/mfd/88pm88x.h
>
> diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c
> new file mode 100644
> index 000000000000..5db6c65b667d
> --- /dev/null
> +++ b/drivers/mfd/88pm88x.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/core.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
Alphabetical
> +#include <linux/mfd/88pm88x.h>
> +
> +/* interrupt status registers */
Use correct grammar in comments, including capital letters.
- Applies throughout
The comment is not required - we can see what they are from the
nomenclature.
> +#define PM88X_REG_INT_STATUS1 0x05
> +
> +#define PM88X_REG_INT_ENA_1 0x0a
> +#define PM88X_INT_ENA1_ONKEY BIT(0)
> +
> +enum pm88x_irq_number {
> + PM88X_IRQ_ONKEY,
> +
> + PM88X_MAX_IRQ
> +};
An enum for a single IRQ?
> +static struct regmap_irq pm88x_regmap_irqs[] = {
> + REGMAP_IRQ_REG(PM88X_IRQ_ONKEY, 0, PM88X_INT_ENA1_ONKEY),
> +};
> +
> +static struct regmap_irq_chip pm88x_regmap_irq_chip = {
> + .name = "88pm88x",
> + .irqs = pm88x_regmap_irqs,
> + .num_irqs = ARRAY_SIZE(pm88x_regmap_irqs),
> + .num_regs = 4,
> + .status_base = PM88X_REG_INT_STATUS1,
> + .ack_base = PM88X_REG_INT_STATUS1,
> + .unmask_base = PM88X_REG_INT_ENA_1,
> +};
> +
> +static struct reg_sequence pm886_presets[] = {
> + /* disable watchdog */
> + REG_SEQ0(PM88X_REG_WDOG, 0x01),
Easier to read if you place spaces between them.
> + /* GPIO1: DVC, GPIO0: input */
> + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
Shouldn't you set these up using Pintrl?
> + /* GPIO2: input */
> + REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> + /* DVC2, DVC1 */
Please unify all of the comments.
They all use a different structure.
> + REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> + /* GPIO5V_1:input, GPIO5V_2: input */
> + REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> + /* output 32 kHz from XO */
> + REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> + /* OSC_FREERUN = 1, to lock FLL */
> + REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> + /* XO_LJ = 1, enable low jitter for 32 kHz */
> + REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> + /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */
> + REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> + /* set the duty cycle of charger DC/DC to max */
> + REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
These all looks like they should be handled in their respective drivers?
"patch"ing these in seems like a hack.
> +};
Why this instead of
> +static struct resource onkey_resources[] = {
> + DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> +};
> +
> +static struct mfd_cell pm88x_devs[] = {
> + {
> + .name = "88pm88x-onkey",
> + .num_resources = ARRAY_SIZE(onkey_resources),
> + .resources = onkey_resources,
> + .id = -1,
> + },
> +};
It's not an MFD if it only supports a single device.
> +static struct pm88x_data pm886_a1_data = {
> + .whoami = PM886_A1_WHOAMI,
> + .presets = pm886_presets,
> + .num_presets = ARRAY_SIZE(pm886_presets),
> +};
Just pass the device ID through DT's .data, then match on that instead
of passing pointer to random data structures.
> +static const struct regmap_config pm88x_i2c_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0xfe,
Define this please.
> +};
> +
> +static int pm88x_power_off_handler(struct sys_off_data *data)
'data' is a terrible variable name. Please change throughout.
> +{
> + struct pm88x_chip *chip = data->cb_data;
> + int ret;
> +
> + ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_MISC_CONFIG1,
> + PM88X_SW_PDOWN, PM88X_SW_PDOWN);
> + if (ret) {
> + dev_err(&chip->client->dev, "Failed to power off the device: %d\n", ret);
> + return NOTIFY_BAD;
> + }
> + return NOTIFY_DONE;
> +}
> +
> +static int pm88x_setup_irq(struct pm88x_chip *chip)
> +{
> + int ret;
> +
> + /* set interrupt clearing mode to clear on write */
> + ret = regmap_update_bits(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_MISC_CONFIG2,
> + PM88X_INT_INV | PM88X_INT_CLEAR | PM88X_INT_MASK_MODE,
> + PM88X_INT_WC);
> + if (ret) {
> + dev_err(&chip->client->dev, "Failed to set interrupt clearing mode: %d\n", ret);
> + return ret;
> + }
> +
> + ret = devm_regmap_add_irq_chip(&chip->client->dev, chip->regmaps[PM88X_REGMAP_BASE],
> + chip->client->irq, IRQF_ONESHOT, -1, &pm88x_regmap_irq_chip,
> + &chip->irq_data);
> + if (ret) {
> + dev_err(&chip->client->dev, "Failed to request IRQ: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int pm88x_probe(struct i2c_client *client)
> +{
> + struct pm88x_chip *chip;
> + int ret = 0;
> + unsigned int chip_id;
> +
> + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->client = client;
> + chip->data = device_get_match_data(&client->dev);
Now where is this being past to?
What is going to consume this?
> + i2c_set_clientdata(client, chip);
> +
> + device_init_wakeup(&client->dev, 1);
> +
> + chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, &pm88x_i2c_regmap);
> + if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) {
Just define different regmaps if you really need them.
I only see one being used anyway.
> + ret = PTR_ERR(chip->regmaps[PM88X_REGMAP_BASE]);
> + dev_err(&client->dev, "Failed to initialize regmap: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_read(chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_ID, &chip_id);
> + if (ret) {
> + dev_err(&client->dev, "Failed to read chip ID: %d\n", ret);
> + return ret;
> + }
> + if (chip->data->whoami != chip_id) {
> + dev_err(&client->dev, "Device reported wrong chip ID: %u\n", chip_id);
Use dev_err_probe() throughout.
> + return -EINVAL;
> + }
> +
> + ret = pm88x_setup_irq(chip);
> + if (ret)
> + return ret;
> +
> + ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs),
> + NULL, 0, regmap_irq_get_domain(chip->irq_data));
> + if (ret) {
> + dev_err(&client->dev, "Failed to add devices: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_register_patch(chip->regmaps[PM88X_REGMAP_BASE], chip->data->presets,
> + chip->data->num_presets);
> + if (ret) {
> + dev_err(&client->dev, "Failed to register regmap patch: %d\n", ret);
> + return ret;
> + }
> +
> + ret = devm_register_power_off_handler(&client->dev, pm88x_power_off_handler, chip);
> + if (ret) {
> + dev_err(&client->dev, "Failed to register power off handler: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +const struct of_device_id pm88x_of_match[] = {
> + { .compatible = "marvell,88pm886-a1", .data = &pm886_a1_data },
> + { },
> +};
> +
> +static struct i2c_driver pm88x_i2c_driver = {
> + .driver = {
> + .name = "88pm88x",
> + .of_match_table = of_match_ptr(pm88x_of_match),
> + },
> + .probe = pm88x_probe,
> +};
> +module_i2c_driver(pm88x_i2c_driver);
> +
> +MODULE_DESCRIPTION("Marvell 88PM88X PMIC driver");
> +MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 90ce58fd629e..c593279fd766 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -794,6 +794,17 @@ config MFD_88PM860X
> select individual components like voltage regulators, RTC and
> battery-charger under the corresponding menus.
>
> +config MFD_88PM88X
"MFD_88PM88X_PMIC"?
> + bool "Marvell 88PM886"
"Marvell 88PM886 PMIC"?
> + depends on I2C=y
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + This enables support for Marvell 88PM886 Power Management IC.
> + This includes the I2C driver and the core APIs _only_, you have to
> + select individual components like onkey under the corresponding menus.
> +
> config MFD_MAX14577
> tristate "Maxim Semiconductor MAX14577/77836 MUIC + Charger Support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..14e42b045c92 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -7,6 +7,7 @@
> obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
> obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o
> obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> +obj-$(CONFIG_MFD_88PM88X) += 88pm88x.o
> obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> new file mode 100644
> index 000000000000..a34c57447827
> --- /dev/null
> +++ b/include/linux/mfd/88pm88x.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __LINUX_MFD_88PM88X_H
> +#define __LINUX_MFD_88PM88X_H
Drop the LINUX part.
> +
> +#include <linux/mfd/core.h>
> +
> +#define PM886_A1_WHOAMI 0xa1
s/WHOAMI/CHIP_ID/
> +#define PM88X_REG_ID 0x00
> +
> +#define PM88X_REG_STATUS1 0x01
> +#define PM88X_ONKEY_STS1 BIT(0)
> +
> +#define PM88X_REG_MISC_CONFIG1 0x14
> +#define PM88X_SW_PDOWN BIT(5)
> +
> +#define PM88X_REG_MISC_CONFIG2 0x15
> +#define PM88X_INT_INV BIT(0)
> +#define PM88X_INT_CLEAR BIT(1)
> +#define PM88X_INT_RC 0x00
> +#define PM88X_INT_WC BIT(1)
> +#define PM88X_INT_MASK_MODE BIT(2)
> +
> +#define PM88X_REG_WDOG 0x1d
> +
> +#define PM88X_REG_LOWPOWER2 0x21
> +#define PM88X_REG_LOWPOWER4 0x23
> +
> +#define PM88X_REG_GPIO_CTRL1 0x30
These don't really need to be spaced out, do they?
> +#define PM88X_REG_GPIO_CTRL2 0x31
> +
> +#define PM88X_REG_GPIO_CTRL3 0x32
> +
> +#define PM88X_REG_GPIO_CTRL4 0x33
> +
> +#define PM88X_REG_BK_OSC_CTRL1 0x50
> +#define PM88X_REG_BK_OSC_CTRL3 0x52
> +
> +#define PM88X_REG_AON_CTRL2 0xe2
> +
> +enum pm88x_regmap_index {
> + PM88X_REGMAP_BASE,
> +
> + PM88X_REGMAP_NR
> +};
> +
> +struct pm88x_data {
> + unsigned int whoami;
> + struct reg_sequence *presets;
> + unsigned int num_presets;
> +};
> +
> +struct pm88x_chip {
> + struct i2c_client *client;
> + struct regmap_irq_chip_data *irq_data;
Group this with the other regmap related member(s).
What are you using this for?
> + const struct pm88x_data *data;
Remove this.
> + struct regmap *regmaps[PM88X_REGMAP_NR];
> +};
> +#endif /* __LINUX_MFD_88PM88X_H */
> --
> 2.43.0
>
--
9)
4)
Lee Jones [李琼斯]
^ permalink raw reply
* [PATCH v3 0/3] Add polling support for DA9063 onkey driver
From: Biju Das @ 2024-01-25 13:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
On some platforms (eg: RZ/{G2UL,Five} SMARC EVK), there is no
onkey IRQ populated by default.
This patch series aims to add polling support.
v2->v3:
* Created patch#1 and patch#2 for removing work.
* Updated commit description as cleanup is done in patch#1
* Dropped scheduling work instead start using polling as in the polling
* case scheduling any additional works will create confused state for
* driver if we repeatedly open and close input device.
v1->v2:
* Updated commit description
* Fixed the logical mistake for optional IRQ handling.
Biju Das (3):
Input: da9063_onkey - Add da9063_onkey_report_key()
Input: da9063_onkey - Drop scheduling work
Input: da9063_onkey - Add polling support
drivers/input/misc/da9063_onkey.c | 130 +++++++++++++++++++-----------
1 file changed, 82 insertions(+), 48 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v3 3/3] Input: da9063_onkey - Add polling support
From: Biju Das @ 2024-01-25 13:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20240125133733.95081-1-biju.das.jz@bp.renesas.com>
On some platforms (eg: RZ/{G2UL,Five} SMARC EVK), there is no
onkey IRQ populated by default. Add polling support.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
* Updated commit description as cleanup is done in patch#1
* Dropped scheduling work instead start using polling as in the polling
* case scheduling any additional works will create confused state for
* driver if we repeatedly open and close input device.
v1->v2:
* Updated commit description
* Fixed the logical mistake for optional IRQ handling.
---
drivers/input/misc/da9063_onkey.c | 83 ++++++++++++++++++++++++-------
1 file changed, 65 insertions(+), 18 deletions(-)
diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
index e5256bf31140..71f1325b309b 100644
--- a/drivers/input/misc/da9063_onkey.c
+++ b/drivers/input/misc/da9063_onkey.c
@@ -19,6 +19,8 @@
#include <linux/mfd/da9062/core.h>
#include <linux/mfd/da9062/registers.h>
+#define DA9062_KEY_THRESHOLD_MSEC (200)
+
struct da906x_chip_config {
/* REGS */
int onkey_status;
@@ -41,6 +43,8 @@ struct da9063_onkey {
const struct da906x_chip_config *config;
char phys[32];
bool key_power;
+ unsigned int poll_interval;
+ unsigned int key_threshold_release_time;
};
static const struct da906x_chip_config da9063_regs = {
@@ -95,6 +99,22 @@ static bool da9063_poll_on(struct da9063_onkey *onkey)
goto err_poll;
}
+ if (onkey->poll_interval &&
+ onkey->key_threshold_release_time <= DA9062_KEY_THRESHOLD_MSEC) {
+ bool ret = true;
+
+ /* detect short or long key press */
+ if (!(val & config->onkey_nonkey_mask)) {
+ da9063_onkey_report_key(onkey, KEY_POWER, 0);
+ onkey->key_threshold_release_time = 0;
+ dev_dbg(onkey->dev, "KEY_POWER short press.\n");
+ ret = false;
+ } else {
+ onkey->key_threshold_release_time += 50;
+ }
+ return ret;
+ }
+
if (!(val & config->onkey_nonkey_mask)) {
error = regmap_update_bits(onkey->regmap,
config->onkey_pwr_signalling,
@@ -170,6 +190,21 @@ static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static void da9063_onkey_polled_poll(struct input_dev *input)
+{
+ struct da9063_onkey *onkey = input_get_drvdata(input);
+ const struct da906x_chip_config *config = onkey->config;
+ unsigned int val;
+ int error;
+
+ error = regmap_read(onkey->regmap, config->onkey_status, &val);
+ if (onkey->key_power && !error && (val & config->onkey_nonkey_mask)) {
+ da9063_onkey_report_key(onkey, KEY_POWER, 1);
+ while (da9063_poll_on(onkey))
+ msleep(50);
+ }
+}
+
static int da9063_onkey_probe(struct platform_device *pdev)
{
struct da9063_onkey *onkey;
@@ -206,25 +241,37 @@ static int da9063_onkey_probe(struct platform_device *pdev)
input_set_capability(onkey->input, EV_KEY, KEY_POWER);
- irq = platform_get_irq_byname(pdev, "ONKEY");
- if (irq < 0)
+ irq = platform_get_irq_byname_optional(pdev, "ONKEY");
+ if (irq >= 0) {
+ error = devm_request_threaded_irq(&pdev->dev, irq,
+ NULL, da9063_onkey_irq_handler,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ "ONKEY", onkey);
+ if (error)
+ return dev_err_probe(&pdev->dev, error,
+ "Failed to allocate onkey irq\n");
+
+ error = dev_pm_set_wake_irq(&pdev->dev, irq);
+ if (error)
+ dev_warn(&pdev->dev,
+ "Failed to set IRQ %d as a wake IRQ: %d\n",
+ irq, error);
+ else
+ device_init_wakeup(&pdev->dev, true);
+ } else if (irq != -ENXIO) {
return irq;
-
- error = devm_request_threaded_irq(&pdev->dev, irq,
- NULL, da9063_onkey_irq_handler,
- IRQF_TRIGGER_LOW | IRQF_ONESHOT,
- "ONKEY", onkey);
- if (error)
- return dev_err_probe(&pdev->dev, error,
- "Failed to allocate onkey IRQ\n");
-
- error = dev_pm_set_wake_irq(&pdev->dev, irq);
- if (error)
- dev_warn(&pdev->dev,
- "Failed to set IRQ %d as a wake IRQ: %d\n",
- irq, error);
- else
- device_init_wakeup(&pdev->dev, true);
+ } else {
+ input_set_drvdata(onkey->input, onkey);
+ device_property_read_u32(&pdev->dev, "poll-interval",
+ &onkey->poll_interval);
+ error = input_setup_polling(onkey->input,
+ da9063_onkey_polled_poll);
+ if (error)
+ return dev_err_probe(&pdev->dev, error,
+ "unable to set up polling\n");
+
+ input_set_poll_interval(onkey->input, onkey->poll_interval);
+ }
error = input_register_device(onkey->input);
if (error)
--
2.25.1
^ permalink raw reply related
* [PATCH v3 1/3] Input: da9063_onkey - Add da9063_onkey_report_key()
From: Biju Das @ 2024-01-25 13:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20240125133733.95081-1-biju.das.jz@bp.renesas.com>
Adding da9063_onkey_report_key() for reporting key events makes the code
simpler. So, simplify it.
While at it, update multiline code that can fit to single line.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3:
* New patch.
---
drivers/input/misc/da9063_onkey.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
index c338765e0ecd..06ad9d09ada8 100644
--- a/drivers/input/misc/da9063_onkey.c
+++ b/drivers/input/misc/da9063_onkey.c
@@ -75,6 +75,13 @@ static const struct da906x_chip_config da9062_regs = {
.name = "da9062-onkey",
};
+static void da9063_onkey_report_key(struct da9063_onkey *onkey,
+ unsigned int code, int value)
+{
+ input_report_key(onkey->input, code, value);
+ input_sync(onkey->input);
+}
+
static void da9063_poll_on(struct work_struct *work)
{
struct da9063_onkey *onkey = container_of(work,
@@ -87,12 +94,9 @@ static void da9063_poll_on(struct work_struct *work)
int error;
/* Poll to see when the pin is released */
- error = regmap_read(onkey->regmap,
- config->onkey_status,
- &val);
+ error = regmap_read(onkey->regmap, config->onkey_status, &val);
if (error) {
- dev_err(onkey->dev,
- "Failed to read ON status: %d\n", error);
+ dev_err(onkey->dev, "Failed to read ON status: %d\n", error);
goto err_poll;
}
@@ -107,8 +111,7 @@ static void da9063_poll_on(struct work_struct *work)
goto err_poll;
}
- input_report_key(onkey->input, KEY_POWER, 0);
- input_sync(onkey->input);
+ da9063_onkey_report_key(onkey, KEY_POWER, 0);
poll = false;
}
@@ -159,19 +162,13 @@ static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
unsigned int val;
int error;
- error = regmap_read(onkey->regmap,
- config->onkey_status,
- &val);
+ error = regmap_read(onkey->regmap, config->onkey_status, &val);
+ da9063_onkey_report_key(onkey, KEY_POWER, 1);
if (onkey->key_power && !error && (val & config->onkey_nonkey_mask)) {
- input_report_key(onkey->input, KEY_POWER, 1);
- input_sync(onkey->input);
schedule_delayed_work(&onkey->work, 0);
dev_dbg(onkey->dev, "KEY_POWER long press.\n");
} else {
- input_report_key(onkey->input, KEY_POWER, 1);
- input_sync(onkey->input);
- input_report_key(onkey->input, KEY_POWER, 0);
- input_sync(onkey->input);
+ da9063_onkey_report_key(onkey, KEY_POWER, 0);
dev_dbg(onkey->dev, "KEY_POWER short press.\n");
}
--
2.25.1
^ permalink raw reply related
* [PATCH v3 2/3] Input: da9063_onkey - Drop scheduling work
From: Biju Das @ 2024-01-25 13:37 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Biju Das, Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20240125133733.95081-1-biju.das.jz@bp.renesas.com>
On threaded case it might be cleaner to avoid scheduling work and simply
loop in the interrupt thread as it can sleep.
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/input/misc/da9063_onkey.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
index 06ad9d09ada8..e5256bf31140 100644
--- a/drivers/input/misc/da9063_onkey.c
+++ b/drivers/input/misc/da9063_onkey.c
@@ -13,7 +13,6 @@
#include <linux/platform_device.h>
#include <linux/pm_wakeirq.h>
#include <linux/property.h>
-#include <linux/workqueue.h>
#include <linux/regmap.h>
#include <linux/mfd/da9063/core.h>
#include <linux/mfd/da9063/registers.h>
@@ -36,7 +35,6 @@ struct da906x_chip_config {
};
struct da9063_onkey {
- struct delayed_work work;
struct input_dev *input;
struct device *dev;
struct regmap *regmap;
@@ -82,11 +80,8 @@ static void da9063_onkey_report_key(struct da9063_onkey *onkey,
input_sync(onkey->input);
}
-static void da9063_poll_on(struct work_struct *work)
+static bool da9063_poll_on(struct da9063_onkey *onkey)
{
- struct da9063_onkey *onkey = container_of(work,
- struct da9063_onkey,
- work.work);
const struct da906x_chip_config *config = onkey->config;
unsigned int val;
int fault_log = 0;
@@ -151,8 +146,7 @@ static void da9063_poll_on(struct work_struct *work)
}
err_poll:
- if (poll)
- schedule_delayed_work(&onkey->work, msecs_to_jiffies(50));
+ return poll;
}
static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
@@ -165,7 +159,8 @@ static irqreturn_t da9063_onkey_irq_handler(int irq, void *data)
error = regmap_read(onkey->regmap, config->onkey_status, &val);
da9063_onkey_report_key(onkey, KEY_POWER, 1);
if (onkey->key_power && !error && (val & config->onkey_nonkey_mask)) {
- schedule_delayed_work(&onkey->work, 0);
+ while (da9063_poll_on(onkey))
+ msleep(50);
dev_dbg(onkey->dev, "KEY_POWER long press.\n");
} else {
da9063_onkey_report_key(onkey, KEY_POWER, 0);
@@ -211,11 +206,6 @@ static int da9063_onkey_probe(struct platform_device *pdev)
input_set_capability(onkey->input, EV_KEY, KEY_POWER);
- error = devm_delayed_work_autocancel(&pdev->dev, &onkey->work,
- da9063_poll_on);
- if (error)
- return error;
-
irq = platform_get_irq_byname(pdev, "ONKEY");
if (irq < 0)
return irq;
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v5 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Kamel Bouhara @ 2024-01-25 16:07 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, catalin.popescu, mark.satterthwaite, bartp,
hannah.rossiter, Thomas Petazzoni, Gregory Clement,
bsp-development.geo
In-Reply-To: <ZY5An58Rffrcpfpn@nixie71>
On Thu, Dec 28, 2023 at 09:44:31PM -0600, Jeff LaBundy wrote:
> Hi Kamel,
>
Hello Jeff,
Thanks for your review again and sorry for those late answers below.
...
> +#include <linux/module.h>
> > +#include <linux/of.h>
>
> As I mention in the v3 review, the entire of.h is not necessary in the
> case of this driver; mod_devicetable.h is sufficient. Please see:
>
> dbce1a7d5dce ("Input: Explicitly include correct DT includes")
>
Ok.
> > +
> > +#define AXIOM_PROX_LEVEL -128
> > +/*
> > + * Register group u31 has 2 pages for usage table entries.
> > + */
> > +#define AXIOM_U31_MAX_USAGES ((2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE)
> > +#define AXIOM_U31_BYTES_PER_USAGE 6
> > +#define AXIOM_U31_PAGE0_LENGTH 0x0C
> > +#define AXIOM_U31_BOOTMODE_MASK BIT(7)
> > +#define AXIOM_U31_DEVID_MASK GENMASK(14, 0)
> > +
> > +#define AXIOM_CMD_HEADER_READ_MASK BIT(15)
> > +#define AXIOM_U41_MAX_TARGETS 10
> > +
> > +#define AXIOM_U46_AUX_CHANNELS 4
> > +#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
> > +
> > +#define AXIOM_COMMS_MAX_USAGE_PAGES 3
> > +#define AXIOM_COMMS_PAGE_SIZE 256
> > +#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(6, 0)
> > +
> > +#define AXIOM_REPORT_USAGE_ID 0x34
> > +#define AXIOM_DEVINFO_USAGE_ID 0x31
> > +#define AXIOM_USAGE_2HB_REPORT_ID 0x01
> > +#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
> > +#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
> > +
> > +#define AXIOM_PAGE_OFFSET_MASK GENMASK(6, 0)
> > +
> > +struct axiom_devinfo {
> > + u16 device_id;
>
> Assuming this is a packed struct into which data is directly read over
> I2C, this member needs declared as __be16 or __le16 depending on the
> endianness of the device, and then all accesses to it resolved using
> be16_to_cpu() or le16_to_cpu().
>
> > + u8 fw_minor;
> > + u8 fw_major;
> > + u8 fw_info_extra;
> > + u8 tcp_revision;
> > + u8 bootloader_fw_minor;
> > + u8 bootloader_fw_major;
> > + u16 jedec_id;
>
> And here.
>
Ack and applied to v6, thanks.
> > + u8 num_usages;
> > +} __packed;
> > +
> > +/*
> > + * Describes parameters of a specific usage, essentially a single element of
> > + * the "Usage Table"
> > + */
> > +struct axiom_usage_entry {
> > + u8 id;
> > + u8 is_report;
> > + u8 start_page;
> > + u8 num_pages;
> > +};
> > +
> > +/*
> > + * Represents state of a touch or target when detected prior a touch (eg.
> > + * hover or proximity events).
> > + */
>
> Nit: prior to a touch
>
Fixed.
> > +enum axiom_target_state {
> > + AXIOM_TARGET_STATE_NOT_PRESENT = 0,
> > + AXIOM_TARGET_STATE_PROX = 1,
> > + AXIOM_TARGET_STATE_HOVER = 2,
> > + AXIOM_TARGET_STATE_TOUCHING = 3,
> > +};
> > +
> > +struct axiom_u41_target {
> > + enum axiom_target_state state;
> > + u16 x;
> > + u16 y;
> > + s8 z;
> > + bool insert;
> > + bool touch;
> > +};
> > +
> > +struct axiom_target_report {
> > + u8 index;
> > + u8 present;
> > + u16 x;
> > + u16 y;
> > + s8 z;
> > +};
> > +
> > +struct axiom_cmd_header {
> > + __le16 target_address;
> > + __le16 length;
> > +} __packed;
> > +
> > +struct axiom_data {
> > + struct axiom_devinfo devinfo;
> > + struct device *dev;
> > + struct gpio_desc *reset_gpio;
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + u32 max_report_len;
> > + char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
>
> Please use standard kernel type definitions (e.g. u8).
Applied.
>
> > + struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
> > + struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> > + bool usage_table_populated;
> > + struct regulator *vdda;
> > + struct regulator *vddi;
> > +};
> > +
> > +/*
> > + * axiom devices are typically configured to report
> > + * touches at a rate of 100Hz (10ms). For systems
> > + * that require polling for reports.
>
> It's not entirely clear what this is saying; is the first period meant to be
> replaced with a comma? It's also odd to see some comments limited to half the
> column width of others. Please make another pass through these to give the
> commentary a consistent voice.
I believe second sentence is not required indeed, fixed the column
width as well, thanks.
>
> > + * When reports are polled, it will be expected to
> > + * occasionally observe the overflow bit being set
> > + * in the reports. This indicates that reports are not
> > + * being read fast enough.
> > + */
> > +#define POLL_INTERVAL_DEFAULT_MS 10
> > +
> > +/* Translate usage/page/offset triplet into physical address. */
> > +static u16 axiom_usage_to_target_address(struct axiom_data *ts, char usage, char page,
> > + char offset)
> > +{
> > + u32 i;
>
> It's more common in kernel code for iterators to be declared as 'int' than
> u32, even if they're only used as unsigned integers in this case.
>
Ack.
[...]
> + msg[1].len = len;
> > + msg[1].buf = (char *)buf;
>
> My comment here from v3 seems to have been missed; please check it.
>
Applied to v6.
> > +
> > + error = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > + if (error != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev,
> > + "Failed reading usage %#x page %#x, error=%d\n",
> > + usage, page, error);
> > + return -EIO;
> > + }
>
> As I mention in the v3 review, you should preserve the original error code
> in case of a negative return value instead of returning -EIO in all cases.
> Please check my original comment.
>
> I also recommend you call this 'ret' and not 'error', because a non-zero
> return value (2) actually indicates success. In the input subsystem at least,
> 'error' is typically used for return values that can only be zero or negative.
>
> > +
> > + usleep_range(250, 300);
> > +
> > + return 0;
> > +}
>
> During the v3 review, I suggested you use regmap, since SPI support may come
> later. You can override both I2C and SPI callbacks with your own in case the
> hardware requires it. What was the reason not to use regmap now, and minimize
> rip-up later?
Suggestion applied to v6.
>
> > +
> > +/*
> > + * One of the main purposes for reading the usage table is to identify
> > + * which usages reside at which target address.
> > + * When performing subsequent reads or writes to AXIOM, the target address
> > + * is used to specify which usage is being accessed.
> > + * Consider the following discovery code which will build up the usage table.
> > + */
> > +static u32 axiom_populate_usage_table(struct axiom_data *ts)
> > +{
> > + struct axiom_usage_entry *usage_table;
> > + u32 max_report_len = 0;
> > + char *rx_data = ts->rx_buf;
>
> Please use u8 here.
>
> > + u32 usage_id;
> > + int error;
> > +
> > + usage_table = ts->usage_table;
> > +
> > + /* Read the second page of usage u31 to get the usage table */
> > + error = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
> > + (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> > + if (error)
> > + return error;
> > +
> > + for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
> > + u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> > + u8 id = rx_data[offset + 0];
> > + u8 start_page = rx_data[offset + 1];
> > + u8 num_pages = rx_data[offset + 2];
> > + u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> > +
> > + if (!num_pages)
> > + usage_table[usage_id].is_report = true;
> > +
> > + /* Store the entry into the usage table */
> > + usage_table[usage_id].id = id;
> > + usage_table[usage_id].start_page = start_page;
> > + usage_table[usage_id].num_pages = num_pages;
> > +
> > + dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id,
> > + AXIOM_U31_BYTES_PER_USAGE, &rx_data[offset]);
> > +
> > + /* Identify the max report length the module will receive */
> > + if (usage_table[usage_id].is_report && max_offset > max_report_len)
> > + max_report_len = max_offset;
> > + }
> > +
> > + ts->usage_table_populated = true;
> > +
> > + return max_report_len;
> > +}
> > +
> > +static int axiom_discover(struct axiom_data *ts)
> > +{
> > + int error;
> > +
> > + /*
> > + * Fetch the first page of usage u31 to get the
> > + * device information and the number of usages
> > + */
> > + error = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 0, (char *)&ts->devinfo,
> > + AXIOM_U31_PAGE0_LENGTH);
>
> If you're set on using bespoke I2C helpers instead of regmap, then 'buf'
> should be defined as a void * as opposed to casting outside of axiom_i2c_read().
>
> > + if (error)
> > + return error;
> > +
> > + dev_dbg(ts->dev, " Boot Mode : %s\n",
> > + FIELD_GET(AXIOM_U31_BOOTMODE_MASK, ts->devinfo.device_id) ? "BLP" : "TCP");
> > + dev_dbg(ts->dev, " Device ID : %04lx\n",
> > + FIELD_GET(AXIOM_U31_DEVID_MASK, ts->devinfo.device_id));
> > + dev_dbg(ts->dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> > + ts->devinfo.fw_minor);
> > + dev_dbg(ts->dev, " Bootloader Rev : %02x.%02x\n", ts->devinfo.bootloader_fw_major,
> > + ts->devinfo.bootloader_fw_minor);
> > + dev_dbg(ts->dev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> > + dev_dbg(ts->dev, " Silicon : %04x\n", ts->devinfo.jedec_id);
> > + dev_dbg(ts->dev, " Number usages : %04x\n", ts->devinfo.num_usages);
> > +
> > + ts->max_report_len = axiom_populate_usage_table(ts);
> > + if (!ts->max_report_len || !ts->devinfo.num_usages)
>
> This seems worthy of a dev_err(), otherwise the customer has no way to
> know something is wrong with the controller's FW.
>
> > + return -EINVAL;
> > +
> > + dev_dbg(ts->dev, "Max Report Length: %u\n", ts->max_report_len);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Support function to axiom_process_u41_report.
> > + * Generates input-subsystem events for every target.
> > + * After calling this function the caller shall issue
> > + * a Sync to the input sub-system.
> > + */
> > +static bool axiom_process_u41_report_target(struct axiom_data *ts,
> > + struct axiom_target_report *target)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + struct axiom_u41_target *target_prev_state;
> > + enum axiom_target_state current_state;
> > + bool update = false;
> > + int slot;
> > +
> > + /* Verify the target index */
> > + if (target->index >= AXIOM_U41_MAX_TARGETS) {
> > + dev_dbg(ts->dev, "Invalid target index! %u\n", target->index);
>
> Should this be dev_err()?
>
> > + return false;
> > + }
> > +
> > + target_prev_state = &ts->targets[target->index];
> > +
> > + current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
> > +
> > + if (target->present) {
> > + if (target->z >= 0)
> > + current_state = AXIOM_TARGET_STATE_TOUCHING;
> > + else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> > + current_state = AXIOM_TARGET_STATE_HOVER;
> > + else if (target->z == AXIOM_PROX_LEVEL)
> > + current_state = AXIOM_TARGET_STATE_PROX;
> > + }
> > +
> > + if (target_prev_state->state == current_state &&
> > + target_prev_state->x == target->x &&
> > + target_prev_state->y == target->y &&
> > + target_prev_state->z == target->z) {
> > + return false;
> > + }
>
> No need for curly braces here; please refer to the kernel style guidelines.
>
> > +
> > + slot = target->index;
> > +
> > + dev_dbg(ts->dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
> > + target->index, slot, target->present,
> > + target->x, target->y, target->z);
> > +
> > + switch (current_state) {
> > + case AXIOM_TARGET_STATE_NOT_PRESENT:
> > + case AXIOM_TARGET_STATE_PROX:
> > + if (!target_prev_state->insert)
> > + break;
> > + update = true;
> > + target_prev_state->insert = false;
> > + input_mt_slot(input_dev, slot);
> > +
> > + if (!slot)
> > + input_report_key(input_dev, BTN_TOUCH, 0);
> > +
> > + input_mt_report_slot_inactive(input_dev);
> > + /*
> > + * make sure the previous coordinates are
> > + * all off screen when the finger comes back
> > + */
> > + target->x = 65535;
> > + target->y = 65535;
> > + target->z = AXIOM_PROX_LEVEL;
> > + break;
> > + case AXIOM_TARGET_STATE_HOVER:
> > + case AXIOM_TARGET_STATE_TOUCHING:
> > + target_prev_state->insert = true;
> > + update = true;
> > + input_mt_slot(input_dev, slot);
> > + input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
> > + input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> > + input_report_abs(input_dev, ABS_X, target->x);
>
> You do not need to explicitly report ABS_X and ABS_Y values, as calling
> input_mt_sync_frame() effectively takes care of this by way of pointer
> emulation.
Applied thanks.
>
> > + input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
> > + input_report_abs(input_dev, ABS_Y, target->y);
> > +
> > + if (current_state == AXIOM_TARGET_STATE_TOUCHING) {
> > + input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
> > + input_report_abs(input_dev, ABS_DISTANCE, 0);
> > + input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
> > + input_report_abs(input_dev, ABS_PRESSURE, target->z);
> > + } else {
> > + input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
> > + input_report_abs(input_dev, ABS_DISTANCE, -target->z);
> > + input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
> > + input_report_abs(input_dev, ABS_PRESSURE, 0);
> > + }
> > +
> > + if (!slot)
> > + input_report_key(input_dev, BTN_TOUCH, (current_state ==
> > + AXIOM_TARGET_STATE_TOUCHING));
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + target_prev_state->state = current_state;
> > + target_prev_state->x = target->x;
> > + target_prev_state->y = target->y;
> > + target_prev_state->z = target->z;
> > +
> > + return update;
> > +}
>
> I appreciate that some clean-up was done here, but it still seems you can
> get rid of the 'update' flag. Can you not re-shuffle this a bit so that
> you return true at the bottom of the function, and simply return false
> early for the few cases where there is no update?
>
Sure.
> > +
> > +/*
> > + * U41 is the output report of the 2D CTS and contains the status of targets
> > + * (including contacts and pre-contacts) along with their X,Y,Z values.
> > + * When a target has been removed (no longer detected),
> > + * the corresponding X,Y,Z values will be zeroed.
> > + */
> > +static bool axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct axiom_target_report target;
> > + bool update_done = false;
> > + u16 target_status;
> > + u32 i;
> > +
> > + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
>
> Please use get_unaligned_le16() instead of open-coding this math.
Interesting, thanks for the suggestions that will indeed improve
readability.
>
> > +
> > + for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> > + char target_step = i * 4;
>
> Please use u8 here.
Ack.
>
> > +
> > + target.index = i;
> > + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> > + target.x = (rx_buf[(target_step + 3)] | (rx_buf[target_step + 4] << 8));
> > + target.y = (rx_buf[(target_step + 5)] | (rx_buf[target_step + 6] << 8));
> > + target.z = (s8)(rx_buf[i + 43]);
> > + update_done |= axiom_process_u41_report_target(ts, &target);
> > + }
> > +
> > + return update_done;
> > +}
> > +
> > +/*
> > + * U46 report contains a low level measurement data generated by the CDS
> > + * algorithms from the AUX channels. This information is useful when tuning
> > + * multi-press to assess mechanical consistency in the unit's construction.
> > + */
>
> What does CDS stand for, and what in user space is interested in these
> events? I'm guessing some kind of production-line calibration tool? I
> appreciate the additional comments in this revision; please add a bit
> more here.
CDS here stands for Capacitive Displacement Sensor and I think you are
right about the calibration tool.
>
> > +static void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + u32 event_value;
> > + u16 aux_value;
> > + u32 i = 0;
>
> There is no need to initialize this iterator.
>
Fixed, thanks.
> > +
> > + for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
> > + char target_step = i * 2;
> > +
> > + aux_value = ((rx_buf[target_step + 2] << 8) | (rx_buf[target_step + 1]))
> > + & AXIOM_U46_AUX_MASK;
>
> This looks like another opportunity to use get_unaligned_le16().
>
Ack.
> > + event_value = (i << 16) | (aux_value);
> > + input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> > + }
> > +}
> > +
> > +/*
> > + * Validates the crc and demultiplexes the axiom reports to the appropriate
> > + * report handler
> > + */
> > +static int axiom_handle_events(struct axiom_data *ts)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + char *report_data = ts->rx_buf;
>
> Please use u8 here.
>
> > + struct device *dev = ts->dev;
> > + u16 crc_report;
> > + u16 crc_calc;
> > + int error;
> > + char len;
>
> And here.
>
> > +
> > + error = axiom_i2c_read(ts->client, AXIOM_REPORT_USAGE_ID, 0, report_data,
> > + ts->max_report_len);
> > + if (error)
> > + return error;
> > +
> > + len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) << 1;
> > + if (!len) {
> > + dev_err(dev, "Zero length report discarded.\n");
> > + return -ENODATA;
> > + }
>
> Since you're expecting at least two bytes to get a CRC, it seems you should
> check that len >= 2 instead of > 0, otherwise 'len - 2' below will panic.
>
> > +
> > + /* Validate the report CRC */
> > + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
>
> We can use get_unaligned_le16() here too.
>
> > + /* Length is in 16 bit words and remove the size of the CRC16 itself */
> > + crc_calc = crc16(0, report_data, (len - 2));
> > +
> > + if (crc_calc != crc_report) {
> > + dev_err(dev,
> > + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> > + crc_report, crc_calc);
> > + return -EINVAL;
> > + }
> > +
> > + switch (report_data[1]) {
> > + case AXIOM_USAGE_2DCTS_REPORT_ID:
> > + if (axiom_process_u41_report(ts, &report_data[1])) {
> > + input_mt_sync_frame(input_dev);
> > + input_sync(input_dev);
> > + }
> > + break;
> > +
> > + case AXIOM_USAGE_2AUX_REPORT_ID:
> > + /* This is an aux report (force) */
> > + axiom_process_u46_report(ts, &report_data[1]);
> > + input_mt_sync(input_dev);
>
> This call to input_mt_sync() seems unnecessary; we are not touching any MT
> slots in this case.
>
> > + input_sync(input_dev);
> > + break;
> > +
> > + case AXIOM_USAGE_2HB_REPORT_ID:
> > + /* This is a heartbeat report */
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void axiom_i2c_poll(struct input_dev *input_dev)
> > +{
> > + struct axiom_data *ts = input_get_drvdata(input_dev);
> > +
> > + axiom_handle_events(ts);
> > +}
> > +
> > +static irqreturn_t axiom_irq(int irq, void *dev_id)
> > +{
> > + struct axiom_data *ts = dev_id;
> > +
> > + axiom_handle_events(ts);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void axiom_reset(struct gpio_desc *reset_gpio)
> > +{
> > + gpiod_set_value_cansleep(reset_gpio, 1);
> > + usleep_range(1000, 2000);
> > + gpiod_set_value_cansleep(reset_gpio, 0);
> > + msleep(110);
> > +}
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct input_dev *input_dev;
> > + struct axiom_data *ts;
> > + u32 startup_delay_ms;
> > + u32 poll_interval;
> > + int target;
> > + int error;
> > +
> > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->client = client;
> > + i2c_set_clientdata(client, ts);
> > + ts->dev = dev;
> > +
> > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(ts->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
> > +
> > + if (ts->reset_gpio)
> > + axiom_reset(ts->reset_gpio);
> > +
> > + ts->vddi = devm_regulator_get_optional(dev, "VDDI");
>
> I don't think there is any rule against doing so, but I have never seen any
> customers name a regulator in all caps.
>
> > + if (!IS_ERR(ts->vddi)) {
> > + error = regulator_enable(ts->vddi);
> > + if (error)
> > + return dev_err_probe(&client->dev, error,
> > + "Failed to enable VDDI regulator\n");
> > + }
> > +
> > + ts->vdda = devm_regulator_get_optional(dev, "VDDA");
> > + if (!IS_ERR(ts->vdda)) {
> > + error = regulator_enable(ts->vdda);
> > + if (error) {
> > + dev_err(dev, "Failed to get VDDA regulator\n");
> > + regulator_disable(ts->vddi);
>
> You're turning off VDDI in case VDDA fails to be enabled, but you never turn
> either off in case the rest of probe (e.g. I2C read) fails, or any other time
> for that matter. Please schedule the regulator_disable() calls using
> devm_add_action_or_reset() so that they are automatically disabled in sequence
> in case probe terminates early, or the driver is unloaded.
I fixed this just by using devm_regulator_get_enable() instead, this
should also make sure regulators ar disabled in case of probe issue.
>
> > + return error;
> > + }
> > + if (!device_property_read_u32(dev, "startup-time-ms", &startup_delay_ms))
> > + msleep(startup_delay_ms);
>
> This seems like it should be a constraint handled by the regulator core and
> not your driver.
Not sure this fits well in regulator's ramp-delay feature, this delay is
required only after the regulator is enabled.
>
> > + }
> > +
> > + error = axiom_discover(ts);
> > + if (error)
> > + return dev_err_probe(dev, error, "Failed touchscreen discover\n");
> > +
> > + input_dev = devm_input_allocate_device(ts->dev);
> > + if (!input_dev)
> > + return -ENOMEM;
> > +
> > + input_dev->name = "TouchNetix axiom Touchscreen";
> > + input_dev->phys = "input/axiom_ts";
> > +
> > + /* Single Touch */
> > + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
>
> As I explained in the v3 review, you do not need to do this. Please refer to my
> previous comments.
>
> > +
> > + /* Multi Touch */
>
> This comment is unnecessary.
>
Fixed in v6.
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
>
> What is the point of this comment, and the one below? Should fuzz have been 4?
>
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
>
> It seems you are forcing 65535 by 65535 resolution; is there no way to
> adjust this? Most controllers can scale it in their FW. You should either
> accept a customer-defined resolution using touchscreen_parse_properties()
> and write it through to the FW, read it from FW and report it through
> input_set_abs_params(), or both.
Ack, I will take this as well, thanks.
>
> > +
> > + input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
>
> This is unnecessary; input_mt_init_slots() will do it based on the contact type.
>
> > +
> > + /* Registers the axiom device as a touchscreen instead of a mouse pointer */
> > + error = input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> > + if (error)
> > + return error;
> > +
> > + /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> > + set_bit(EV_REL, input_dev->evbit);
> > + set_bit(EV_MSC, input_dev->evbit);
> > + /* Declare that we support "RAW" Miscellaneous events */
> > + set_bit(MSC_RAW, input_dev->mscbit);
> > +
> > + ts->input_dev = input_dev;
> > + input_set_drvdata(ts->input_dev, ts);
> > +
> > + /* Ensure that all reports are initialised to not be present. */
> > + for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> > + ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
> > +
> > + error = input_register_device(input_dev);
> > + if (error)
> > + return dev_err_probe(ts->dev, error,
> > + "Could not register with Input Sub-system.\n");
> > +
> > + error = devm_request_threaded_irq(dev, client->irq, NULL,
> > + axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
> > + if (error < 0) {
>
> if (error)
>
> > + dev_warn(dev, "Request irq failed, falling back to polling mode");
> > +
> > + error = input_setup_polling(input_dev, axiom_i2c_poll);
> > + if (error)
> > + return dev_err_probe(ts->dev, error, "Unable to set up polling mode\n");
> > +
> > + if (!device_property_read_u32(ts->dev, "poll-interval", &poll_interval))
> > + input_set_poll_interval(input_dev, poll_interval);
> > + else
> > + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > + { "ax54a" },
> > + {},
>
> Nit: add a space inside the sentinel like you do below.
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_of_match[] = {
> > + { .compatible = "touchnetix,ax54a", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> > +
> > +static struct i2c_driver axiom_i2c_driver = {
> > + .driver = {
> > + .name = "axiom",
> > + .of_match_table = axiom_i2c_of_match,
> > + },
> > + .id_table = axiom_i2c_id_table,
> > + .probe = axiom_i2c_probe,
> > +};
> > +module_i2c_driver(axiom_i2c_driver);
>
> Nit: please add a newline here.
>
> > +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
> > +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
> > +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
> > +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("TouchNetix axiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >
>
> Kind regards,
> Jeff LaBundy
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ 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