* [PATCH v4 5/6] drm/vmwgfx: Use VMware hypercall API
From: Alexey Makhalov @ 2023-12-28 19:24 UTC (permalink / raw)
To: linux-kernel, virtualization, bp, hpa, dave.hansen, mingo, tglx
Cc: x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, namit, timothym, akaher,
jsipek, dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov
In-Reply-To: <20231228192421.29894-1-alexey.makhalov@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
Switch from VMWARE_HYPERCALL macro to vmware_hypercall API.
Eliminate arch specific code.
drivers/gpu/drm/vmwgfx/vmwgfx_msg_arm64.h: implement arm64 variant
of vmware_hypercall. And keep it here until introduction of ARM64
VMWare hypervisor interface.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
Reviewed-by: Zack Rusin <zackr@vmware.com>
---
drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 173 +++++++------------
drivers/gpu/drm/vmwgfx/vmwgfx_msg_arm64.h | 197 +++++++++++++++-------
drivers/gpu/drm/vmwgfx/vmwgfx_msg_x86.h | 185 --------------------
3 files changed, 197 insertions(+), 358 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index 2651fe0ef518..1f15990d3934 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -48,8 +48,6 @@
#define RETRIES 3
-#define VMW_HYPERVISOR_MAGIC 0x564D5868
-
#define VMW_PORT_CMD_MSG 30
#define VMW_PORT_CMD_HB_MSG 0
#define VMW_PORT_CMD_OPEN_CHANNEL (MSG_TYPE_OPEN << 16 | VMW_PORT_CMD_MSG)
@@ -104,20 +102,18 @@ static const char* const mksstat_kern_name_desc[MKSSTAT_KERN_COUNT][2] =
*/
static int vmw_open_channel(struct rpc_channel *channel, unsigned int protocol)
{
- unsigned long eax, ebx, ecx, edx, si = 0, di = 0;
+ u32 ecx, edx, esi, edi;
- VMW_PORT(VMW_PORT_CMD_OPEN_CHANNEL,
- (protocol | GUESTMSG_FLAG_COOKIE), si, di,
- 0,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall6(VMW_PORT_CMD_OPEN_CHANNEL,
+ (protocol | GUESTMSG_FLAG_COOKIE), 0,
+ &ecx, &edx, &esi, &edi);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0)
return -EINVAL;
channel->channel_id = HIGH_WORD(edx);
- channel->cookie_high = si;
- channel->cookie_low = di;
+ channel->cookie_high = esi;
+ channel->cookie_low = edi;
return 0;
}
@@ -133,17 +129,13 @@ static int vmw_open_channel(struct rpc_channel *channel, unsigned int protocol)
*/
static int vmw_close_channel(struct rpc_channel *channel)
{
- unsigned long eax, ebx, ecx, edx, si, di;
-
- /* Set up additional parameters */
- si = channel->cookie_high;
- di = channel->cookie_low;
+ u32 ecx;
- VMW_PORT(VMW_PORT_CMD_CLOSE_CHANNEL,
- 0, si, di,
- channel->channel_id << 16,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall5(VMW_PORT_CMD_CLOSE_CHANNEL,
+ 0, channel->channel_id << 16,
+ channel->cookie_high,
+ channel->cookie_low,
+ &ecx);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0)
return -EINVAL;
@@ -163,24 +155,18 @@ static int vmw_close_channel(struct rpc_channel *channel)
static unsigned long vmw_port_hb_out(struct rpc_channel *channel,
const char *msg, bool hb)
{
- unsigned long si, di, eax, ebx, ecx, edx;
+ u32 ebx, ecx;
unsigned long msg_len = strlen(msg);
/* HB port can't access encrypted memory. */
if (hb && !cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
- unsigned long bp = channel->cookie_high;
- u32 channel_id = (channel->channel_id << 16);
-
- si = (uintptr_t) msg;
- di = channel->cookie_low;
-
- VMW_PORT_HB_OUT(
+ vmware_hypercall_hb_out(
(MESSAGE_STATUS_SUCCESS << 16) | VMW_PORT_CMD_HB_MSG,
- msg_len, si, di,
- VMWARE_HYPERVISOR_HB | channel_id |
- VMWARE_HYPERVISOR_OUT,
- VMW_HYPERVISOR_MAGIC, bp,
- eax, ebx, ecx, edx, si, di);
+ msg_len,
+ channel->channel_id << 16,
+ (uintptr_t) msg, channel->cookie_low,
+ channel->cookie_high,
+ &ebx);
return ebx;
}
@@ -194,14 +180,13 @@ static unsigned long vmw_port_hb_out(struct rpc_channel *channel,
memcpy(&word, msg, bytes);
msg_len -= bytes;
msg += bytes;
- si = channel->cookie_high;
- di = channel->cookie_low;
-
- VMW_PORT(VMW_PORT_CMD_MSG | (MSG_TYPE_SENDPAYLOAD << 16),
- word, si, di,
- channel->channel_id << 16,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+
+ vmware_hypercall5(VMW_PORT_CMD_MSG |
+ (MSG_TYPE_SENDPAYLOAD << 16),
+ word, channel->channel_id << 16,
+ channel->cookie_high,
+ channel->cookie_low,
+ &ecx);
}
return ecx;
@@ -220,22 +205,17 @@ static unsigned long vmw_port_hb_out(struct rpc_channel *channel,
static unsigned long vmw_port_hb_in(struct rpc_channel *channel, char *reply,
unsigned long reply_len, bool hb)
{
- unsigned long si, di, eax, ebx, ecx, edx;
+ u32 ebx, ecx, edx;
/* HB port can't access encrypted memory */
if (hb && !cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
- unsigned long bp = channel->cookie_low;
- u32 channel_id = (channel->channel_id << 16);
-
- si = channel->cookie_high;
- di = (uintptr_t) reply;
-
- VMW_PORT_HB_IN(
+ vmware_hypercall_hb_in(
(MESSAGE_STATUS_SUCCESS << 16) | VMW_PORT_CMD_HB_MSG,
- reply_len, si, di,
- VMWARE_HYPERVISOR_HB | channel_id,
- VMW_HYPERVISOR_MAGIC, bp,
- eax, ebx, ecx, edx, si, di);
+ reply_len,
+ channel->channel_id << 16,
+ channel->cookie_high,
+ (uintptr_t) reply, channel->cookie_low,
+ &ebx);
return ebx;
}
@@ -245,14 +225,13 @@ static unsigned long vmw_port_hb_in(struct rpc_channel *channel, char *reply,
while (reply_len) {
unsigned int bytes = min_t(unsigned long, reply_len, 4);
- si = channel->cookie_high;
- di = channel->cookie_low;
-
- VMW_PORT(VMW_PORT_CMD_MSG | (MSG_TYPE_RECVPAYLOAD << 16),
- MESSAGE_STATUS_SUCCESS, si, di,
- channel->channel_id << 16,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall7(VMW_PORT_CMD_MSG |
+ (MSG_TYPE_RECVPAYLOAD << 16),
+ MESSAGE_STATUS_SUCCESS,
+ channel->channel_id << 16,
+ channel->cookie_high,
+ channel->cookie_low,
+ &ebx, &ecx, &edx);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0)
break;
@@ -276,22 +255,18 @@ static unsigned long vmw_port_hb_in(struct rpc_channel *channel, char *reply,
*/
static int vmw_send_msg(struct rpc_channel *channel, const char *msg)
{
- unsigned long eax, ebx, ecx, edx, si, di;
+ u32 ebx, ecx;
size_t msg_len = strlen(msg);
int retries = 0;
while (retries < RETRIES) {
retries++;
- /* Set up additional parameters */
- si = channel->cookie_high;
- di = channel->cookie_low;
-
- VMW_PORT(VMW_PORT_CMD_SENDSIZE,
- msg_len, si, di,
- channel->channel_id << 16,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall5(VMW_PORT_CMD_SENDSIZE,
+ msg_len, channel->channel_id << 16,
+ channel->cookie_high,
+ channel->cookie_low,
+ &ecx);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
/* Expected success. Give up. */
@@ -329,7 +304,7 @@ STACK_FRAME_NON_STANDARD(vmw_send_msg);
static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
size_t *msg_len)
{
- unsigned long eax, ebx, ecx, edx, si, di;
+ u32 ebx, ecx, edx;
char *reply;
size_t reply_len;
int retries = 0;
@@ -341,15 +316,11 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
while (retries < RETRIES) {
retries++;
- /* Set up additional parameters */
- si = channel->cookie_high;
- di = channel->cookie_low;
-
- VMW_PORT(VMW_PORT_CMD_RECVSIZE,
- 0, si, di,
- channel->channel_id << 16,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall7(VMW_PORT_CMD_RECVSIZE,
+ 0, channel->channel_id << 16,
+ channel->cookie_high,
+ channel->cookie_low,
+ &ebx, &ecx, &edx);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
DRM_ERROR("Failed to get reply size for host message.\n");
@@ -384,16 +355,12 @@ static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
reply[reply_len] = '\0';
-
- /* Ack buffer */
- si = channel->cookie_high;
- di = channel->cookie_low;
-
- VMW_PORT(VMW_PORT_CMD_RECVSTATUS,
- MESSAGE_STATUS_SUCCESS, si, di,
- channel->channel_id << 16,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall5(VMW_PORT_CMD_RECVSTATUS,
+ MESSAGE_STATUS_SUCCESS,
+ channel->channel_id << 16,
+ channel->cookie_high,
+ channel->cookie_low,
+ &ecx);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
kfree(reply);
@@ -652,13 +619,7 @@ static inline void reset_ppn_array(PPN64 *arr, size_t size)
*/
static inline void hypervisor_ppn_reset_all(void)
{
- unsigned long eax, ebx, ecx, edx, si = 0, di = 0;
-
- VMW_PORT(VMW_PORT_CMD_MKSGS_RESET,
- 0, si, di,
- 0,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall1(VMW_PORT_CMD_MKSGS_RESET, 0);
}
/**
@@ -669,13 +630,7 @@ static inline void hypervisor_ppn_reset_all(void)
*/
static inline void hypervisor_ppn_add(PPN64 pfn)
{
- unsigned long eax, ebx, ecx, edx, si = 0, di = 0;
-
- VMW_PORT(VMW_PORT_CMD_MKSGS_ADD_PPN,
- (unsigned long)pfn, si, di,
- 0,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall1(VMW_PORT_CMD_MKSGS_ADD_PPN, (unsigned long)pfn);
}
/**
@@ -686,13 +641,7 @@ static inline void hypervisor_ppn_add(PPN64 pfn)
*/
static inline void hypervisor_ppn_remove(PPN64 pfn)
{
- unsigned long eax, ebx, ecx, edx, si = 0, di = 0;
-
- VMW_PORT(VMW_PORT_CMD_MKSGS_REMOVE_PPN,
- (unsigned long)pfn, si, di,
- 0,
- VMW_HYPERVISOR_MAGIC,
- eax, ebx, ecx, edx, si, di);
+ vmware_hypercall1(VMW_PORT_CMD_MKSGS_REMOVE_PPN, (unsigned long)pfn);
}
#if IS_ENABLED(CONFIG_DRM_VMWGFX_MKSSTATS)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg_arm64.h b/drivers/gpu/drm/vmwgfx/vmwgfx_msg_arm64.h
index 4f40167ad61f..29bd0af83038 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg_arm64.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg_arm64.h
@@ -34,6 +34,8 @@
#define VMWARE_HYPERVISOR_HB BIT(0)
#define VMWARE_HYPERVISOR_OUT BIT(1)
+#define VMWARE_HYPERVISOR_MAGIC 0x564D5868
+
#define X86_IO_MAGIC 0x86
#define X86_IO_W7_SIZE_SHIFT 0
@@ -45,86 +47,159 @@
#define X86_IO_W7_IMM_SHIFT 5
#define X86_IO_W7_IMM_MASK (0xff << X86_IO_W7_IMM_SHIFT)
-static inline void vmw_port(unsigned long cmd, unsigned long in_ebx,
- unsigned long in_si, unsigned long in_di,
- unsigned long flags, unsigned long magic,
- unsigned long *eax, unsigned long *ebx,
- unsigned long *ecx, unsigned long *edx,
- unsigned long *si, unsigned long *di)
+static inline
+unsigned long vmware_hypercall1(unsigned long cmd, unsigned long in1)
{
- register u64 x0 asm("x0") = magic;
- register u64 x1 asm("x1") = in_ebx;
+ register u64 x0 asm("x0") = VMWARE_HYPERVISOR_MAGIC;
+ register u64 x1 asm("x1") = in1;
register u64 x2 asm("x2") = cmd;
- register u64 x3 asm("x3") = flags | VMWARE_HYPERVISOR_PORT;
- register u64 x4 asm("x4") = in_si;
- register u64 x5 asm("x5") = in_di;
+ register u64 x3 asm("x3") = VMWARE_HYPERVISOR_PORT;
+ register u64 x7 asm("x7") = ((u64)X86_IO_MAGIC << 32) |
+ X86_IO_W7_WITH |
+ X86_IO_W7_DIR |
+ (2 << X86_IO_W7_SIZE_SHIFT);
+ asm_inline volatile (
+ "mrs xzr, mdccsr_el0; "
+ : "+r" (x0)
+ : "r" (x1), "r" (x2), "r" (x3), "r" (x7)
+ : "memory");
+
+ return x0;
+}
+
+static inline
+unsigned long vmware_hypercall5(unsigned long cmd, unsigned long in1,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, uint32_t *out2)
+{
+ register u64 x0 asm("x0") = VMWARE_HYPERVISOR_MAGIC;
+ register u64 x1 asm("x1") = in1;
+ register u64 x2 asm("x2") = cmd;
+ register u64 x3 asm("x3") = in3 | VMWARE_HYPERVISOR_PORT;
+ register u64 x4 asm("x4") = in4;
+ register u64 x5 asm("x5") = in5;
register u64 x7 asm("x7") = ((u64)X86_IO_MAGIC << 32) |
X86_IO_W7_WITH |
X86_IO_W7_DIR |
(2 << X86_IO_W7_SIZE_SHIFT);
- asm volatile("mrs xzr, mdccsr_el0 \n\t"
- : "+r"(x0), "+r"(x1), "+r"(x2),
- "+r"(x3), "+r"(x4), "+r"(x5)
- : "r"(x7)
- :);
- *eax = x0;
- *ebx = x1;
- *ecx = x2;
- *edx = x3;
- *si = x4;
- *di = x5;
+ asm_inline volatile (
+ "mrs xzr, mdccsr_el0; "
+ : "+r" (x0), "+r" (x2)
+ : "r" (x1), "r" (x3), "r" (x4), "r" (x5), "r" (x7)
+ : "memory");
+
+ *out2 = x2;
+ return x0;
}
-static inline void vmw_port_hb(unsigned long cmd, unsigned long in_ecx,
- unsigned long in_si, unsigned long in_di,
- unsigned long flags, unsigned long magic,
- unsigned long bp, u32 w7dir,
- unsigned long *eax, unsigned long *ebx,
- unsigned long *ecx, unsigned long *edx,
- unsigned long *si, unsigned long *di)
+static inline
+unsigned long vmware_hypercall6(unsigned long cmd, unsigned long in1,
+ unsigned long in3, uint32_t *out2,
+ uint32_t *out3, uint32_t *out4,
+ uint32_t *out5)
{
- register u64 x0 asm("x0") = magic;
+ register u64 x0 asm("x0") = VMWARE_HYPERVISOR_MAGIC;
+ register u64 x1 asm("x1") = in1;
+ register u64 x2 asm("x2") = cmd;
+ register u64 x3 asm("x3") = in3 | VMWARE_HYPERVISOR_PORT;
+ register u64 x4 asm("x4");
+ register u64 x5 asm("x5");
+ register u64 x7 asm("x7") = ((u64)X86_IO_MAGIC << 32) |
+ X86_IO_W7_WITH |
+ X86_IO_W7_DIR |
+ (2 << X86_IO_W7_SIZE_SHIFT);
+
+ asm_inline volatile (
+ "mrs xzr, mdccsr_el0; "
+ : "+r" (x0), "+r" (x2), "+r" (x3), "=r" (x4), "=r" (x5)
+ : "r" (x1), "r" (x7)
+ : "memory");
+
+ *out2 = x2;
+ *out3 = x3;
+ *out4 = x4;
+ *out5 = x5;
+ return x0;
+}
+
+static inline
+unsigned long vmware_hypercall7(unsigned long cmd, unsigned long in1,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, uint32_t *out1,
+ uint32_t *out2, uint32_t *out3)
+{
+ register u64 x0 asm("x0") = VMWARE_HYPERVISOR_MAGIC;
+ register u64 x1 asm("x1") = in1;
+ register u64 x2 asm("x2") = cmd;
+ register u64 x3 asm("x3") = in3 | VMWARE_HYPERVISOR_PORT;
+ register u64 x4 asm("x4") = in4;
+ register u64 x5 asm("x5") = in5;
+ register u64 x7 asm("x7") = ((u64)X86_IO_MAGIC << 32) |
+ X86_IO_W7_WITH |
+ X86_IO_W7_DIR |
+ (2 << X86_IO_W7_SIZE_SHIFT);
+
+ asm_inline volatile (
+ "mrs xzr, mdccsr_el0; "
+ : "+r" (x0), "+r" (x1), "+r" (x2), "+r" (x3)
+ : "r" (x4), "r" (x5), "r" (x7)
+ : "memory");
+
+ *out1 = x1;
+ *out2 = x2;
+ *out3 = x3;
+ return x0;
+}
+
+static inline
+unsigned long vmware_hypercall_hb(unsigned long cmd, unsigned long in2,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, unsigned long in6,
+ uint32_t *out1, int dir)
+{
+ register u64 x0 asm("x0") = VMWARE_HYPERVISOR_MAGIC;
register u64 x1 asm("x1") = cmd;
- register u64 x2 asm("x2") = in_ecx;
- register u64 x3 asm("x3") = flags | VMWARE_HYPERVISOR_PORT_HB;
- register u64 x4 asm("x4") = in_si;
- register u64 x5 asm("x5") = in_di;
- register u64 x6 asm("x6") = bp;
+ register u64 x2 asm("x2") = in2;
+ register u64 x3 asm("x3") = in3 | VMWARE_HYPERVISOR_PORT_HB;
+ register u64 x4 asm("x4") = in4;
+ register u64 x5 asm("x5") = in5;
+ register u64 x6 asm("x6") = in6;
register u64 x7 asm("x7") = ((u64)X86_IO_MAGIC << 32) |
X86_IO_W7_STR |
X86_IO_W7_WITH |
- w7dir;
-
- asm volatile("mrs xzr, mdccsr_el0 \n\t"
- : "+r"(x0), "+r"(x1), "+r"(x2),
- "+r"(x3), "+r"(x4), "+r"(x5)
- : "r"(x6), "r"(x7)
- :);
- *eax = x0;
- *ebx = x1;
- *ecx = x2;
- *edx = x3;
- *si = x4;
- *di = x5;
-}
+ dir;
-#define VMW_PORT(cmd, in_ebx, in_si, in_di, flags, magic, eax, ebx, ecx, edx, \
- si, di) \
- vmw_port(cmd, in_ebx, in_si, in_di, flags, magic, &eax, &ebx, &ecx, \
- &edx, &si, &di)
+ asm_inline volatile (
+ "mrs xzr, mdccsr_el0; "
+ : "+r" (x0), "+r" (x1)
+ : "r" (x2), "r" (x3), "r" (x4), "r" (x5),
+ "r" (x6), "r" (x7)
+ : "memory");
-#define VMW_PORT_HB_OUT(cmd, in_ecx, in_si, in_di, flags, magic, bp, eax, ebx, \
- ecx, edx, si, di) \
- vmw_port_hb(cmd, in_ecx, in_si, in_di, flags, magic, bp, \
- 0, &eax, &ebx, &ecx, &edx, &si, &di)
+ *out1 = x1;
+ return x0;
+}
-#define VMW_PORT_HB_IN(cmd, in_ecx, in_si, in_di, flags, magic, bp, eax, ebx, \
- ecx, edx, si, di) \
- vmw_port_hb(cmd, in_ecx, in_si, in_di, flags, magic, bp, \
- X86_IO_W7_DIR, &eax, &ebx, &ecx, &edx, &si, &di)
+static inline
+unsigned long vmware_hypercall_hb_out(unsigned long cmd, unsigned long in2,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, unsigned long in6,
+ uint32_t *out1)
+{
+ return vmware_hypercall_hb(cmd, in2, in3, in4, in5, in6, out1, 0);
+}
+static inline
+unsigned long vmware_hypercall_hb_in(unsigned long cmd, unsigned long in2,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, unsigned long in6,
+ uint32_t *out1)
+{
+ return vmware_hypercall_hb(cmd, in2, in3, in4, in5, in6, out1,
+ X86_IO_W7_DIR);
+}
#endif
#endif /* _VMWGFX_MSG_ARM64_H */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg_x86.h b/drivers/gpu/drm/vmwgfx/vmwgfx_msg_x86.h
index 23899d743a90..13304d34cc6e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg_x86.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg_x86.h
@@ -37,191 +37,6 @@
#include <asm/vmware.h>
-/**
- * Hypervisor-specific bi-directional communication channel. Should never
- * execute on bare metal hardware. The caller must make sure to check for
- * supported hypervisor before using these macros.
- *
- * The last two parameters are both input and output and must be initialized.
- *
- * @cmd: [IN] Message Cmd
- * @in_ebx: [IN] Message Len, through EBX
- * @in_si: [IN] Input argument through SI, set to 0 if not used
- * @in_di: [IN] Input argument through DI, set ot 0 if not used
- * @flags: [IN] hypercall flags + [channel id]
- * @magic: [IN] hypervisor magic value
- * @eax: [OUT] value of EAX register
- * @ebx: [OUT] e.g. status from an HB message status command
- * @ecx: [OUT] e.g. status from a non-HB message status command
- * @edx: [OUT] e.g. channel id
- * @si: [OUT]
- * @di: [OUT]
- */
-#define VMW_PORT(cmd, in_ebx, in_si, in_di, \
- flags, magic, \
- eax, ebx, ecx, edx, si, di) \
-({ \
- asm volatile (VMWARE_HYPERCALL : \
- "=a"(eax), \
- "=b"(ebx), \
- "=c"(ecx), \
- "=d"(edx), \
- "=S"(si), \
- "=D"(di) : \
- "a"(magic), \
- "b"(in_ebx), \
- "c"(cmd), \
- "d"(flags), \
- "S"(in_si), \
- "D"(in_di) : \
- "memory"); \
-})
-
-
-/**
- * Hypervisor-specific bi-directional communication channel. Should never
- * execute on bare metal hardware. The caller must make sure to check for
- * supported hypervisor before using these macros.
- *
- * The last 3 parameters are both input and output and must be initialized.
- *
- * @cmd: [IN] Message Cmd
- * @in_ecx: [IN] Message Len, through ECX
- * @in_si: [IN] Input argument through SI, set to 0 if not used
- * @in_di: [IN] Input argument through DI, set to 0 if not used
- * @flags: [IN] hypercall flags + [channel id]
- * @magic: [IN] hypervisor magic value
- * @bp: [IN]
- * @eax: [OUT] value of EAX register
- * @ebx: [OUT] e.g. status from an HB message status command
- * @ecx: [OUT] e.g. status from a non-HB message status command
- * @edx: [OUT] e.g. channel id
- * @si: [OUT]
- * @di: [OUT]
- */
-#ifdef __x86_64__
-
-#define VMW_PORT_HB_OUT(cmd, in_ecx, in_si, in_di, \
- flags, magic, bp, \
- eax, ebx, ecx, edx, si, di) \
-({ \
- asm volatile ( \
- UNWIND_HINT_SAVE \
- "push %%rbp;" \
- UNWIND_HINT_UNDEFINED \
- "mov %12, %%rbp;" \
- VMWARE_HYPERCALL_HB_OUT \
- "pop %%rbp;" \
- UNWIND_HINT_RESTORE : \
- "=a"(eax), \
- "=b"(ebx), \
- "=c"(ecx), \
- "=d"(edx), \
- "=S"(si), \
- "=D"(di) : \
- "a"(magic), \
- "b"(cmd), \
- "c"(in_ecx), \
- "d"(flags), \
- "S"(in_si), \
- "D"(in_di), \
- "r"(bp) : \
- "memory", "cc"); \
-})
-
-
-#define VMW_PORT_HB_IN(cmd, in_ecx, in_si, in_di, \
- flags, magic, bp, \
- eax, ebx, ecx, edx, si, di) \
-({ \
- asm volatile ( \
- UNWIND_HINT_SAVE \
- "push %%rbp;" \
- UNWIND_HINT_UNDEFINED \
- "mov %12, %%rbp;" \
- VMWARE_HYPERCALL_HB_IN \
- "pop %%rbp;" \
- UNWIND_HINT_RESTORE : \
- "=a"(eax), \
- "=b"(ebx), \
- "=c"(ecx), \
- "=d"(edx), \
- "=S"(si), \
- "=D"(di) : \
- "a"(magic), \
- "b"(cmd), \
- "c"(in_ecx), \
- "d"(flags), \
- "S"(in_si), \
- "D"(in_di), \
- "r"(bp) : \
- "memory", "cc"); \
-})
-
-#elif defined(__i386__)
-
-/*
- * In the 32-bit version of this macro, we store bp in a memory location
- * because we've ran out of registers.
- * Now we can't reference that memory location while we've modified
- * %esp or %ebp, so we first push it on the stack, just before we push
- * %ebp, and then when we need it we read it from the stack where we
- * just pushed it.
- */
-#define VMW_PORT_HB_OUT(cmd, in_ecx, in_si, in_di, \
- flags, magic, bp, \
- eax, ebx, ecx, edx, si, di) \
-({ \
- asm volatile ("push %12;" \
- "push %%ebp;" \
- "mov 0x04(%%esp), %%ebp;" \
- VMWARE_HYPERCALL_HB_OUT \
- "pop %%ebp;" \
- "add $0x04, %%esp;" : \
- "=a"(eax), \
- "=b"(ebx), \
- "=c"(ecx), \
- "=d"(edx), \
- "=S"(si), \
- "=D"(di) : \
- "a"(magic), \
- "b"(cmd), \
- "c"(in_ecx), \
- "d"(flags), \
- "S"(in_si), \
- "D"(in_di), \
- "m"(bp) : \
- "memory", "cc"); \
-})
-
-
-#define VMW_PORT_HB_IN(cmd, in_ecx, in_si, in_di, \
- flags, magic, bp, \
- eax, ebx, ecx, edx, si, di) \
-({ \
- asm volatile ("push %12;" \
- "push %%ebp;" \
- "mov 0x04(%%esp), %%ebp;" \
- VMWARE_HYPERCALL_HB_IN \
- "pop %%ebp;" \
- "add $0x04, %%esp;" : \
- "=a"(eax), \
- "=b"(ebx), \
- "=c"(ecx), \
- "=d"(edx), \
- "=S"(si), \
- "=D"(di) : \
- "a"(magic), \
- "b"(cmd), \
- "c"(in_ecx), \
- "d"(flags), \
- "S"(in_si), \
- "D"(in_di), \
- "m"(bp) : \
- "memory", "cc"); \
-})
-#endif /* defined(__i386__) */
-
#endif /* defined(__i386__) || defined(__x86_64__) */
#endif /* _VMWGFX_MSG_X86_H */
--
2.39.0
^ permalink raw reply related
* [PATCH v4 6/6] x86/vmware: Add TDX hypercall support
From: Alexey Makhalov @ 2023-12-28 19:24 UTC (permalink / raw)
To: linux-kernel, virtualization, bp, hpa, dave.hansen, mingo, tglx
Cc: x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, namit, timothym, akaher,
jsipek, dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov
In-Reply-To: <20231228192421.29894-1-alexey.makhalov@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
VMware hypercalls use I/O port, VMCALL or VMMCALL instructions.
Add __tdx_hypercall path to support TDX guests.
No change in high bandwidth hypercalls, as only low bandwidth
ones are supported for TDX guests.
Co-developed-by: Tim Merrifield <timothym@vmware.com>
Signed-off-by: Tim Merrifield <timothym@vmware.com>
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
---
arch/x86/include/asm/vmware.h | 79 +++++++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/vmware.c | 24 +++++++++++
2 files changed, 103 insertions(+)
diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
index 84a31f579a30..3bd593c6591d 100644
--- a/arch/x86/include/asm/vmware.h
+++ b/arch/x86/include/asm/vmware.h
@@ -18,6 +18,12 @@
* arg2 - Hypercall command
* arg3 bits [15:0] - Port number, LB and direction flags
*
+ * - Low bandwidth TDX hypercalls (x86_64 only) are similar to LB
+ * hypercalls. They also have up to 6 input and 6 output on registers
+ * arguments, with different argument to register mapping:
+ * %r12 (arg0), %rbx (arg1), %r13 (arg2), %rdx (arg3),
+ * %rsi (arg4), %rdi (arg5).
+ *
* - High bandwidth (HB) hypercalls are I/O port based only. They have
* up to 7 input and 7 output arguments passed and returned using
* registers: %eax (arg0), %ebx (arg1), %ecx (arg2), %edx (arg3),
@@ -54,12 +60,61 @@
#define VMWARE_CMD_GETHZ 45
#define VMWARE_CMD_GETVCPU_INFO 68
#define VMWARE_CMD_STEALCLOCK 91
+/*
+ * Hypercall command mask:
+ * bits [6:0] command, range [0, 127]
+ * bits [19:16] sub-command, range [0, 15]
+ */
+#define VMWARE_CMD_MASK 0xf007fU
#define CPUID_VMWARE_FEATURES_ECX_VMMCALL BIT(0)
#define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1)
extern u8 vmware_hypercall_mode;
+#define VMWARE_TDX_VENDOR_LEAF 0x1af7e4909ULL
+#define VMWARE_TDX_HCALL_FUNC 1
+
+extern unsigned long vmware_tdx_hypercall(unsigned long cmd,
+ struct tdx_module_args *args);
+
+/*
+ * TDCALL[TDG.VP.VMCALL] uses %rax (arg0) and %rcx (arg2). Therefore,
+ * we remap those registers to %r12 and %r13, respectively.
+ */
+static inline
+unsigned long vmware_tdx_hypercall_args(unsigned long cmd, unsigned long in1,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5,
+ uint32_t *out1, uint32_t *out2,
+ uint32_t *out3, uint32_t *out4,
+ uint32_t *out5)
+{
+ unsigned long ret;
+
+ struct tdx_module_args args = {
+ .rbx = in1,
+ .rdx = in3,
+ .rsi = in4,
+ .rdi = in5,
+ };
+
+ ret = vmware_tdx_hypercall(cmd, &args);
+
+ if (out1)
+ *out1 = args.rbx;
+ if (out2)
+ *out2 = args.r13;
+ if (out3)
+ *out3 = args.rdx;
+ if (out4)
+ *out4 = args.rsi;
+ if (out5)
+ *out5 = args.rdi;
+
+ return ret;
+}
+
/*
* The low bandwidth call. The low word of %edx is presumed to have OUT bit
* set. The high word of %edx may contain input data from the caller.
@@ -87,6 +142,10 @@ unsigned long vmware_hypercall1(unsigned long cmd, unsigned long in1)
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0,
+ NULL, NULL, NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -105,6 +164,10 @@ unsigned long vmware_hypercall3(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0,
+ out1, out2, NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=b" (*out1), "=c" (*out2)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -124,6 +187,10 @@ unsigned long vmware_hypercall4(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0,
+ out1, out2, out3, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -143,6 +210,10 @@ unsigned long vmware_hypercall5(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, in3, in4, in5,
+ NULL, out2, NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=c" (*out2)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -165,6 +236,10 @@ unsigned long vmware_hypercall6(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, in3, 0, 0,
+ NULL, out2, out3, out4, out5);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=c" (*out2), "=d" (*out3), "=S" (*out4),
"=D" (*out5)
@@ -186,6 +261,10 @@ unsigned long vmware_hypercall7(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, in3, in4, in5,
+ out1, out2, out3, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 3aa1adaed18f..a88dcd25aefe 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -428,6 +428,30 @@ static bool __init vmware_legacy_x2apic_available(void)
(eax & BIT(VCPU_LEGACY_X2APIC));
}
+#ifdef CONFIG_INTEL_TDX_GUEST
+unsigned long vmware_tdx_hypercall(unsigned long cmd,
+ struct tdx_module_args *args)
+{
+ if (!hypervisor_is_type(X86_HYPER_VMWARE))
+ return ULONG_MAX;
+
+ if (cmd & ~VMWARE_CMD_MASK) {
+ pr_warn_once("Out of range command %lx\n", cmd);
+ return ULONG_MAX;
+ }
+
+ args->r10 = VMWARE_TDX_VENDOR_LEAF;
+ args->r11 = VMWARE_TDX_HCALL_FUNC;
+ args->r12 = VMWARE_HYPERVISOR_MAGIC;
+ args->r13 = cmd;
+
+ __tdx_hypercall(args);
+
+ return args->r12;
+}
+EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
+#endif
+
#ifdef CONFIG_AMD_MEM_ENCRYPT
static void vmware_sev_es_hcall_prepare(struct ghcb *ghcb,
struct pt_regs *regs)
--
2.39.0
^ permalink raw reply related
* [PATCH] Input: gpio-keys - filter gpio_keys -EPROBE_DEFER error messages
From: Hermes Zhang @ 2023-12-29 1:36 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: kernel, Hermes Zhang, linux-input, linux-kernel
From: Hermes Zhang <chenhuiz@axis.com>
commit ae42f9288846 ("gpio: Return EPROBE_DEFER if gc->to_irq is NULL")
make gpiod_to_irq() possible to return -EPROBE_DEFER when the racing
happens. This causes the following error message to be printed:
gpio-keys gpio_keys: Unable to get irq number for GPIO 0, error -517
Fix that by changing dev_err() to dev_err_probe()
Signed-off-by: Hermes Zhang <chenhuiz@axis.com>
---
drivers/input/keyboard/gpio_keys.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2e7c2c046e67..193856599669 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -581,9 +581,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
irq = gpiod_to_irq(bdata->gpiod);
if (irq < 0) {
error = irq;
- dev_err(dev,
- "Unable to get irq number for GPIO %d, error %d\n",
- button->gpio, error);
+ dev_err_probe(dev, error,
+ "Unable to get irq number for GPIO %d\n",
+ button->gpio);
return error;
}
bdata->irq = irq;
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v5 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Jeff LaBundy @ 2023-12-29 3:44 UTC (permalink / raw)
To: Kamel Bouhara
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: <20231211121430.1689139-4-kamel.bouhara@bootlin.com>
Hi Kamel,
On Mon, Dec 11, 2023 at 01:14:29PM +0100, Kamel Bouhara wrote:
> Add a new driver for the TouchNetix's axiom family of
> touchscreen controllers. This driver only supports i2c
> and can be later adapted for SPI and USB support.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
> MAINTAINERS | 1 +
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/touchnetix_axiom.c | 667 +++++++++++++++++++
> 4 files changed, 681 insertions(+)
> create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4752d8436dbb..337ddac6c74b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21436,6 +21436,7 @@ M: Kamel Bouhara <kamel.bouhara@bootlin.com>
> L: linux-input@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
> +F: drivers/input/touchscreen/touchnetix_axiom.c
>
> THUNDERBOLT DMA TRAFFIC TEST DRIVER
> M: Isaac Hazan <isaac.hazan@intel.com>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..f36bee8d8696 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -803,6 +803,18 @@ config TOUCHSCREEN_MIGOR
> To compile this driver as a module, choose M here: the
> module will be called migor_ts.
>
> +config TOUCHSCREEN_TOUCHNETIX_AXIOM
> + tristate "TouchNetix AXIOM based touchscreen controllers"
> + depends on I2C
> + help
> + Say Y here if you have a axiom touchscreen connected to
> + your system.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called axiom.
> +
> config TOUCHSCREEN_TOUCHRIGHT
> tristate "Touchright serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..8e32a2df5e18 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
> obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI) += surface3_spi.o
> obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
> +obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM) += touchnetix_axiom.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
> obj-$(CONFIG_TOUCHSCREEN_TS4800) += ts4800-ts.o
> diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
> new file mode 100644
> index 000000000000..4ade2d1adba0
> --- /dev/null
> +++ b/drivers/input/touchscreen/touchnetix_axiom.c
> @@ -0,0 +1,667 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TouchNetix axiom Touchscreen Driver
> + *
> + * Copyright (C) 2020-2023 TouchNetix Ltd.
> + *
> + * Author(s): Bart Prescott <bartp@baasheep.co.uk>
> + * Pedro Torruella <pedro.torruella@touchnetix.com>
> + * Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> + * Kamel Bouhara <kamel.bouhara@bootlin.com>
> + *
> + */
> +#include <linux/bitfield.h>
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#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")
> +
> +#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.
> + 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
> +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).
> + 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.
> + * 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.
> +
> + /* At the moment the convention is that u31 is always at physical address 0x0 */
> + if (!ts->usage_table_populated) {
> + if (usage == AXIOM_DEVINFO_USAGE_ID)
> + return ((page << 8) + offset);
> + else
> + return 0xffff;
> + }
> +
> + for (i = 0; i < ts->devinfo.num_usages; i++) {
> + if (ts->usage_table[i].id != usage)
> + continue;
> +
> + if (page >= ts->usage_table[i].num_pages) {
> + dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> + usage, page, offset);
> + return 0xffff;
> + }
> + break;
> + }
> +
> + return ((ts->usage_table[i].start_page + page) << 8) + offset;
> +}
> +
> +static int axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> + struct axiom_cmd_header cmd_header;
> + struct i2c_msg msg[2];
> + int error;
> +
> + cmd_header.target_address = cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
> + cmd_header.length = cpu_to_le16(len | AXIOM_CMD_HEADER_READ_MASK);
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = sizeof(cmd_header);
> + msg[0].buf = (u8 *)&cmd_header;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = (char *)buf;
My comment here from v3 seems to have been missed; please check it.
> +
> + 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?
> +
> +/*
> + * 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.
> + 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?
> +
> +/*
> + * 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.
> +
> + for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> + char target_step = i * 4;
Please use u8 here.
> +
> + 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.
> +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.
> +
> + 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().
> + 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.
> + 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.
> + }
> +
> + 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.
> + /* 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.
> +
> + 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
^ permalink raw reply
* Re: [PATCH] Input: usbtouchscreen - Return directly after a failed kmalloc() in nexio_init()
From: Dmitry Torokhov @ 2023-12-29 7:42 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-input, kernel-janitors, Oliver Graute,
Uwe Kleine-König, ye xingchen, LKML, cocci
In-Reply-To: <9365c845-baa1-44d1-add9-ec8ca4d365eb@web.de>
On Tue, Dec 26, 2023 at 09:08:12PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Dec 2023 21:00:25 +0100
>
> The kfree() function was called in one case by
> the nexio_init() function during error handling
> even if the passed variable contained a null pointer.
Which is perfectly valid thing to do, like free(), kfree() accepts NULL
argument.
> This issue was detected by using the Coccinelle software.
This tells me precisely nothing.
>
> Thus return directly after a call of the function “kmalloc” failed
> at the beginning.
This is simply a matter of preference, the original author preferred
that style, I see no objective reason to change it.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/input/touchscreen/usbtouchscreen.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index 60354ebc7242..1873c7918a78 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -977,7 +977,7 @@ static int nexio_init(struct usbtouch_usb *usbtouch)
>
> buf = kmalloc(NEXIO_BUFSIZE, GFP_NOIO);
> if (!buf)
> - goto out_buf;
> + return ret;
>
> /* two empty reads */
> for (i = 0; i < 2; i++) {
> --
> 2.43.0
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: MT - Return directly after a failed kzalloc() in input_mt_init_slots()
From: Dmitry Torokhov @ 2023-12-29 7:43 UTC (permalink / raw)
To: Markus Elfring; +Cc: linux-input, kernel-janitors, Henrik Rydberg, LKML, cocci
In-Reply-To: <5088a905-4f29-41d3-a96e-5b66aad551f1@web.de>
On Tue, Dec 26, 2023 at 08:43:37PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Dec 2023 20:36:09 +0100
>
> The kfree() function was called in one case by
> the input_mt_init_slots() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
>
> Thus return directly after a call of the function “kzalloc” failed
> at the beginning.
This is not needed. The same arguments as on the patch to
usbtouchscreen.c.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: gpio-keys - filter gpio_keys -EPROBE_DEFER error messages
From: Dmitry Torokhov @ 2023-12-29 7:46 UTC (permalink / raw)
To: Hermes Zhang; +Cc: kernel, Hermes Zhang, linux-input, linux-kernel
In-Reply-To: <20231229013657.692600-1-Hermes.Zhang@axis.com>
On Fri, Dec 29, 2023 at 09:36:57AM +0800, Hermes Zhang wrote:
> From: Hermes Zhang <chenhuiz@axis.com>
>
> commit ae42f9288846 ("gpio: Return EPROBE_DEFER if gc->to_irq is NULL")
> make gpiod_to_irq() possible to return -EPROBE_DEFER when the racing
> happens. This causes the following error message to be printed:
>
> gpio-keys gpio_keys: Unable to get irq number for GPIO 0, error -517
>
> Fix that by changing dev_err() to dev_err_probe()
>
> Signed-off-by: Hermes Zhang <chenhuiz@axis.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: Input: usbtouchscreen - Return directly after a failed kmalloc() in nexio_init()
From: Markus Elfring @ 2023-12-29 8:56 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: kernel-janitors, Oliver Graute, Uwe Kleine-König,
ye xingchen, LKML, cocci
In-Reply-To: <ZY54UDosMHwj6D3Y@google.com>
>> The kfree() function was called in one case by
>> the nexio_init() function during error handling
>> even if the passed variable contained a null pointer.
>
> Which is perfectly valid thing to do, like free(), kfree() accepts NULL argument.
I find such a function call with this special parameter not so useful.
>> Thus return directly after a call of the function “kmalloc” failed
>> at the beginning.
>
> This is simply a matter of preference, the original author preferred
> that style, I see no objective reason to change it.
Would you ever like to avoid redundant data processing a bit more?
Regards,
Markus
^ permalink raw reply
* Re: Input: MT - Return directly after a failed kzalloc() in input_mt_init_slots()
From: Markus Elfring @ 2023-12-29 9:00 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: kernel-janitors, Henrik Rydberg, LKML, cocci
In-Reply-To: <ZY54hX3VLswwKgMH@google.com>
>> Thus return directly after a call of the function “kzalloc” failed
>> at the beginning.
>
> This is not needed. The same arguments as on the patch to
> usbtouchscreen.c.
I suggest to avoid redundant data processing a bit more.
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: Add Himax HX83102J touchscreen
From: Allen Lin @ 2023-12-29 9:08 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
benjamin.tissoires, linux-input, devicetree, linux-kernel
In-Reply-To: <08623087-bf1c-411e-87de-d40ffab6e2bc@linaro.org>
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2023年12月28日 週四 下午6:36寫道：
>
> On 27/12/2023 06:35, Allen_Lin wrote:
> > Add the HX83102j touchscreen device tree bindings documents.
> >
> > Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> > ---
>
> Where is the changelog? There is no cover letter attached, so changelog
> is supposed to be here. There were several comments, so does it mean you
> ignored them?
>
Cover letter is not in this mail but in the mail with this title
"[PATCH v3 0/2] Add HX83102j driver for HIMAX HID touchscreen"
Hi,
This driver implements for Himax HID touchscreen HX83102j.
Using SPI interface to receive/send HID packets.
Patchs notes as below
1. Add the Maintainer and devicetree bindings document for driver
2. Add the driver code and modify Kconfig/Makefile to support the driver
change in v2 :
- Fix kernel test robot build warnings.
change in v3 :
- Modify code according to review suggesions.
Thanks.
Allen_Lin (2):
dt-bindings: input: Add Himax HX83102J touchscreen
Input: Add Himax HX83102J touchscreen driver
.../bindings/input/himax,hx83102j.yaml | 65 +
MAINTAINERS | 7 +
drivers/hid/Kconfig | 8 +
drivers/hid/Makefile | 2 +
drivers/hid/hid-himax-83102j.c | 1096 +++++++++++++++++
drivers/hid/hid-himax-83102j.h | 202 +++
6 files changed, 1380 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/himax,hx83102j.yaml
create mode 100644 drivers/hid/hid-himax-83102j.c
create mode 100644 drivers/hid/hid-himax-83102j.h
--
2.34.1
>
> > .../bindings/input/himax,hx83102j.yaml | 65 +++++++++++++++++++
> > MAINTAINERS | 6 ++
> > 2 files changed, 71 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/input/himax,hx83102j.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/himax,hx83102j.yaml b/Documentation/devicetree/bindings/input/himax,hx83102j.yaml
> > new file mode 100644
> > index 000000000000..872b478c5753
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/himax,hx83102j.yaml
> > @@ -0,0 +1,65 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/himax,hx83102j.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Himax hx83102j touchscreen
> > +
>
> ...
>
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + hid-himax-spi@0 {
>
> Still not the name I asked - it should be generic, like touchscreen.
>
I will fix it, thanks for your review. if driver code needs to be
modified, i will fix it together.
> Best regards,
> Krzysztof
>
Best regards
Allen
^ permalink raw reply
* Fixing ISH hingle angle sensor invalid value
From: Yauhen Kharuzhy @ 2023-12-29 17:57 UTC (permalink / raw)
To: linux-input; +Cc: xiang.ye, hdegoede
Hi,
I have a device (Lenovo Yoga Book tablet) with a hinge angle sensor
exposed by Intel ISH. This virtual sensor should compute the hingle angle
based on the accelerometers data. It is supported by the
hid-sensor-custom-intel-hinge driver.
The sensor has three channels: angles of base and screen with respect to the
ground, and the hinge angle between them. The base and sceen angles are
reported right, but the hinge angle is always 360°.
What is the correct way to fix this? We can modify the driver and fixup
the hingle angle value in it, or add such quirk in a userspace apllication
like iio-sensor-proxy (this requires to have a blacklist of devices in
userspace apps). Should we try to correct the value of this sensor or
just blacklist it and re-implement the same functionality in userspace
based on accelerometers data?
When fixing this in the driver, we should decide how to distinguish
'fully open' and 'fully closed' states: we may need to check the lid
switch status for this.
--
Yauhen Kharuzhy
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Werner Sembach @ 2023-12-29 19:13 UTC (permalink / raw)
To: Hans de Goede, Pavel Machek, Jani Nikula, jikos,
Jelle van der Waa
Cc: Lee Jones, linux-kernel, dri-devel@lists.freedesktop.org,
Miguel Ojeda, linux-input, ojeda, linux-leds
In-Reply-To: <ac02143c-d417-49e5-9c6e-150cbda71ba7@tuxedocomputers.com>
Hi Hans & the others,
[snip]
> I also stumbled across a new Problem:
>
> We have an upcoming device that has a per-key keyboard backlight, but does the
> control completely via a wmi/acpi interface. So no usable hidraw here for a
> potential userspace driver implementation ...
>
> So a quick summary for the ideas floating in this thread so far:
>
> 1. Expand leds interface allowing arbitrary modes with semi arbitrary optional
> attributes:
>
> - Pro:
>
> - Still offers all default attributes for use with UPower
>
> - Fairly simple to implement from the preexisting codebase
>
> - Could be implemented for all (to me) known internal keyboard backlights
>
> - Con:
>
> - Violates the simplicity paradigm of the leds interface (e.g. with
> this one leds entry controls possible multiple leds)
>
> 2. Implement per-key keyboards as auxdisplay
>
> - Pro:
>
> - Already has a concept for led positions
>
> - Is conceptually closer to "multiple leds forming a singular entity"
>
> - Con:
>
> - No preexisting UPower support
>
> - No concept for special hardware lighting modes
>
> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>
> 3. Implement in input subsystem
>
> - Pro:
>
> - Preexisting concept for keys and key purpose
>
> - Con:
>
> - Not in scope for subsystem
>
> - No other preexisting light infrastructure
>
> 4. Implement a simple leds driver only supporting a small subset of the
> capabilities and make it disable-able for a userspace driver to take over
>
> - Pro:
>
> - Most simple to implement basic support
>
> - In scope for led subsystem simplicity paradigm
>
> - Con:
>
> - Not all built in keyboard backlights can be implemented in a
> userspace only driver
>
> Kind Regards,
>
> Werner
Just a gentle bump and request for comments again. 4. would be better then
nothing but it is not a universal future proof solution so I'm hesitant to put
work into it even though it would be the simplest driver. I still tend towards
1. as the leds interface already got expanded once with the multicolor stuff.
The only other way I see would be to implement a platform driver with no
standardized api or implement a complete new api/subsystem from the ground up.
Kind Regards,
Werner
^ permalink raw reply
* Re: Fixing ISH hingle angle sensor invalid value
From: Yauhen Kharuzhy @ 2023-12-29 21:50 UTC (permalink / raw)
To: linux-input, Hans de Goede
In-Reply-To: <20231229175737.edjdf6c5mfrsyyt5@jekhomev>
Hmm,
Second challenge is to detect if the angle is valid et all: if the
hinge is close to vertical, calculated angles are changing in all
possible ranges. So, re-implementing this "virtual sensor" in
userspace using accelerometers seems to be a simpler way than trying
to fixup invalid data that arrived from the ISH firmware.
пт, 29 дек. 2023 г. в 19:57, Yauhen Kharuzhy <jekhor@gmail.com>:
>
> Hi,
>
> I have a device (Lenovo Yoga Book tablet) with a hinge angle sensor
> exposed by Intel ISH. This virtual sensor should compute the hingle angle
> based on the accelerometers data. It is supported by the
> hid-sensor-custom-intel-hinge driver.
>
> The sensor has three channels: angles of base and screen with respect to the
> ground, and the hinge angle between them. The base and sceen angles are
> reported right, but the hinge angle is always 360°.
>
> What is the correct way to fix this? We can modify the driver and fixup
> the hingle angle value in it, or add such quirk in a userspace apllication
> like iio-sensor-proxy (this requires to have a blacklist of devices in
> userspace apps). Should we try to correct the value of this sensor or
> just blacklist it and re-implement the same functionality in userspace
> based on accelerometers data?
>
> When fixing this in the driver, we should decide how to distinguish
> 'fully open' and 'fully closed' states: we may need to check the lid
> switch status for this.
>
> --
> Yauhen Kharuzhy
--
Yauhen Kharuzhy
^ permalink raw reply
* Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max
From: Chris Morgan @ 2023-12-30 5:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Paul Cercueil, Peter Hutterer, Chris Morgan,
linux-input, svv, biswarupp, contact
In-Reply-To: <ZYflxnNC-72vh6De@google.com>
On Sun, Dec 24, 2023 at 12:03:18AM -0800, Dmitry Torokhov wrote:
> On Sat, Dec 23, 2023 at 04:16:46PM +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 12/23/23 16:01, Paul Cercueil wrote:
> > > Hi Hans,
> > >
> > > Le samedi 23 décembre 2023 à 15:29 +0100, Hans de Goede a écrit :
> > >> Hi Paul,
> > >>
> > >> On 12/20/23 14:39, Paul Cercueil wrote:
> > >>> Hi Dmitry,
> > >>>
> > >>> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> > >>>> Hi Paul,
> > >>>>
> > >>>> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> > >>>>> Hi Peter,
> > >>>>>
> > >>>>> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> > >>>>> écrit :
> > >>>>>> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > >>>>>>> From: Chris Morgan <macromorgan@hotmail.com>
> > >>>>>>>
> > >>>>>>> Stop checking if the minimum abs value is greater than the
> > >>>>>>> maximum
> > >>>>>>> abs
> > >>>>>>> value. When the axis is inverted this condition is allowed.
> > >>>>>>> Without
> > >>>>>>> relaxing this check, it is not possible to use uinput on
> > >>>>>>> devices in
> > >>>>>>> userspace with an inverted axis, such as the adc-joystick
> > >>>>>>> found
> > >>>>>>> on
> > >>>>>>> many handheld gaming devices.
> > >>>>>>
> > >>>>>> As mentioned in the other thread [1] a fair bit of userspace
> > >>>>>> relies
> > >>>>>> on
> > >>>>>> that general assumption so removing it will likely cause all
> > >>>>>> sorts of
> > >>>>>> issues.
> > >>>>>
> > >>>>> There is some userspace that works with it though, so why
> > >>>>> restrict
> > >>>>> it
> > >>>>> artificially?
> > >>>>>
> > >>>>> The fact that some other userspace code wouldn't work with it
> > >>>>> sounds a
> > >>>>> bit irrelevant. They just never encountered that min>max usage
> > >>>>> before.
> > >>>>>
> > >>>>> And removing this check won't cause all sort of issues, why
> > >>>>> would
> > >>>>> it?
> > >>>>> It's not like the current software actively probes min>max and
> > >>>>> crash
> > >>>>> badly if it doesn't return -EINVAL...
> > >>>>
> > >>>> It will cause weird movements because calculations expect min be
> > >>>> the
> > >>>> minimum, and max the maximum, and not encode left/right or
> > >>>> up/down.
> > >>>> Putting this into adc joystick binding was a mistake.
> > >>>
> > >>> I don't see why it was a mistake, it's only one of the ways to
> > >>> specify
> > >>> that the axis is inverted. This information is between the firmware
> > >>> (DT) and the kernel, that doesn't mean the information has to be
> > >>> relayed as-is to the userspace.
> > >>>
> > >>> Unlike what you wrote in your other answer, when talking about
> > >>> input
> > >>> the kernel doesn't really normalize anything - it gives you the
> > >>> min/max
> > >>> values, and the raw samples, not normalized samples (they don't get
> > >>> translated to a pre-specified range, or even clamped).
> > >>>
> > >>> I don't really like the idea of having the driver tamper with the
> > >>> samples, but if the specification really is that max>min, then it
> > >>> would
> > >>> be up to evdev/joydev (if the individual drivers are allowed
> > >>> min>max)
> > >>> or adc-joystick (if they are not) to process the samples.
> > >>
> > >> I don't see why a driver, especially a userspace driver which
> > >> then injects things back into the kernel using uinput, would
> > >> not take care of inverting the samples itself and then just
> > >> present userspace with normalized data where min is simply 0
> > >> (as result of normalization as part of inversion) and
> > >> max is (original_max - original_min).
> > >
> > > Yes, I totally agree.
> > >
> > > What I was saying is, as Chris is only "piping" events from adc-
> > > joystick into uinput, that the problem is more that evdev/joydev don't
> > > handle axis inversion and provide min>max values that most of the
> > > userspace (and some kernel drivers e.g. uinput) don't support.
> >
> > Ah I see, that sounds like a joydev adc-joystick / driver bug
> > to me then.
>
> joydev/mousedev/evdev are simply consumers of events coming from the
> drivers that handle hardware. Even though they reside in the kernel,
> they still consumers of events, much like userspace is, and they operate
> under the same assumption that if min and max are specified then max is
> not less than min.
>
> We always had HW drivers invert the axis to match our coordinate system
> (for absolute coordinates 0,0 is in the lower left corner, for relative
> right and up are positive and left and down are negative). You can see
> that in psmouse (psmouse_report_standard_motion) and synaptics drivers,
> one of the earliest in the kernel.
>
> The rest of the stack operates under this assumption.
>
> >
> > >> Note that this is exactly what is being done for touchscreens,
> > >> where having the touchscreen mounted e.g. upside-down is
> > >> a long standing issue and this is thus also a long solved issue,
> > >> see: drivers/input/touchscreen.c which contains generic
> > >> code for parsing device-properties including swapped / inverted
> > >> axis as well as generic code for reporting the position to the
> > >> input core, where the helpers from drivers/input/touchscreen.c
> > >> take care of the swap + invert including normalization when
> > >> doing inversion.
> > >>
> > >> Specifically this contains in touchscreen_parse_properties() :
> > >>
> > >> prop->max_x = input_abs_get_max(input, axis_x);
> > >> prop->max_y = input_abs_get_max(input, axis_y);
> > >>
> > >> if (prop->invert_x) {
> > >> absinfo = &input->absinfo[axis_x];
> > >> absinfo->maximum -= absinfo->minimum;
> > >> absinfo->minimum = 0;
> > >> }
> > >>
> > >> if (prop->invert_y) {
> > >> absinfo = &input->absinfo[axis_y];
> > >> absinfo->maximum -= absinfo->minimum;
> > >> absinfo->minimum = 0;
> > >> }
> > >>
> > >> and then when reporting touches:
> > >>
> > >> void touchscreen_report_pos(struct input_dev *input,
> > >> const struct touchscreen_properties
> > >> *prop,
> > >> unsigned int x, unsigned int y,
> > >> bool multitouch)
> > >> {
> > >> if (prop->invert_x)
> > >> x = prop->max_x - x;
> > >>
> > >> if (prop->invert_y)
> > >> y = prop->max_y - y;
> > >>
> > >> if (prop->swap_x_y)
> > >> swap(x, y);
> > >>
> > >> input_report_abs(input, multitouch ? ABS_MT_POSITION_X :
> > >> ABS_X, x);
> > >> input_report_abs(input, multitouch ? ABS_MT_POSITION_Y :
> > >> ABS_Y, y);
> > >> }
> > >>
> > >> One of the tasks of a driver / the kernel is to provide some
> > >> level of hardware abstraction to isolate userspace from
> > >> hw details. IMHO taking care of the axis-inversion for userspace
> > >> with something like the above is part of the kernels' HAL task.
> > >
> > > Totally agree, but this is not done anywhere, is it? evdev seems to
> > > just pass the hardware values alongside some basic meta-data (min/max
> > > values, fuzz etc.), it does not tamper with the data. Should evdev
> > > handle axis inversion? Should it be in adc-joystick (and every other
> > > driver that needs that) instead?
> >
> > For touchcreens we have chosen to have a set of generic helpers
> > and then make using those helpers the responsibility of the driver.
> >
> > Part of the reason for doing this is because some touchscreen drivers
> > already were doing axis inversion inside the driver triggering on
> > things like e.g. DMI matches, or maybe custom pre standardization
> > device properties, etc.
> >
> > So the decision was made to add a set of helpers and convert drivers
> > one by one. Where drivers can e.g. still set prop->invert_x manually,
> > but then they also need to take care of the min/max adjustments
> > manually (min is typically 0 for touchscreens though).
> >
> > I expect that there will also be enough existing special handling
> > in the joystick code that piece-meal conversion using helpers
> > is likely best.
>
> Not only touchscreens and joysticks. As I mentioned, PS/2 mice have
> their reported relative Y motion inverted since forever, touchpads like
> Synaptics or Elan invert Y axis as well, and so on.
>
> >
> > With that said having axis inversion support in the evdev core
> > does sound interesting, but that means also storing the max-value
> > inside the core for abs axis and this will likely be a big
> > change / lots of work.
>
> evdev is not the only consumer, so if anything it should be in the input
> core, but there are enough quirks that I think touchscreen helpers are
> the best, at least for now.
>
> Thanks.
I feel like what I've seen here leads me to believe that we should
handle it in the driver. The only other *thing* I've seen that's
leading me to my solution is that it should theoretically be possible
for a joystick to have negative values (every place I check shows
int or s32 or something). I do see the way the touchscreen handles it
in touchscreen_parse_properties() and touchscreen_apply_prop_to_x_y()
and I was thinking there might be a better way to do it.
I'm under the assumption (still new, sorry) that I can't simply modify
an existing struct since userspace relies heavily on it and we can't
modify userspace... specifically by adding a bool to the input_absinfo.
So that said I was thinking we should add a helper to the input.c
that inverts an abs when requested by taking the absmin, absmax, and
value, then inverting it by doing ((absmax + absmin) - value). By doing
it this way we should be able to invert the values correctly, even when
dealing with negatives... With this example ((1023 + 15) - 15) would
give me 1023 which is what I would expect the inverted value to be, and
((1023 + -1023) - -1023) should give me 1023 which is again what I
would expect the inverse of -1023 to be in this case. We'll leave it up
to the individual driver to actually track the inverted status and to
apply inversion as necessary.
If you think this is the best course I'll add a helper to the input.c
function and then make use of it in the adc-joystick.c for both
polled and non-polled cases.
Thank you,
Chris
>
> --
> Dmitry
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS 0b670b54119902de75fcd20a50585cf7b573f801
From: kernel test robot @ 2023-12-30 8:07 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 0b670b54119902de75fcd20a50585cf7b573f801 Input: gpio-keys - filter gpio_keys -EPROBE_DEFER error messages
Warning ids grouped by kconfigs:
gcc_recent_errors
|-- arc-randconfig-r123-20231229
| `-- kernel-fork.c:sparse:sparse:incorrect-type-in-assignment-(different-address-spaces)-expected-struct-file-assigned-old_exe_file-got-struct-file-noderef-__rcu-assigned-_val_
|-- loongarch-randconfig-r062-20231230
| `-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- nios2-allmodconfig
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- nios2-randconfig-001-20231229
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- nios2-randconfig-r064-20231230
| `-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
`-- riscv-allmodconfig
|-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
`-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
clang_recent_errors
|-- arm64-allmodconfig
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- arm64-randconfig-r063-20231230
| |-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
| `-- drivers-input-touchscreen-iqs7211.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- hexagon-allmodconfig
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- hexagon-randconfig-r051-20231230
| `-- drivers-input-touchscreen-edt-ft5x06.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- i386-randconfig-052-20231230
| |-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
| `-- drivers-input-touchscreen-iqs7211.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- i386-randconfig-053-20231230
| `-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- powerpc-allmodconfig
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- riscv-randconfig-r052-20231230
| `-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- riscv-randconfig-r061-20231230
| `-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- x86_64-allmodconfig
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- x86_64-buildonly-randconfig-001-20231229
| |-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-pata_ftide010.o
| `-- WARNING:modpost:missing-MODULE_DESCRIPTION()-in-drivers-ata-sata_gemini.o
|-- x86_64-randconfig-011-20231229
| `-- PLEASE-submit-a-bug-report-to-https:github.com-llvm-llvm-project-issues-and-include-the-crash-backtrace-preprocessed-source-and-associated-run-script.
|-- x86_64-randconfig-012-20231229
| `-- PLEASE-submit-a-bug-report-to-https:github.com-llvm-llvm-project-issues-and-include-the-crash-backtrace-preprocessed-source-and-associated-run-script.
|-- x86_64-randconfig-101-20231230
| |-- drivers-input-misc-iqs7222.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
| `-- drivers-input-touchscreen-edt-ft5x06.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
|-- x86_64-randconfig-102-20231230
| `-- drivers-input-touchscreen-iqs7211.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
`-- x86_64-randconfig-104-20231230
`-- drivers-input-touchscreen-edt-ft5x06.c:WARNING:Threaded-IRQ-with-no-primary-handler-requested-without-IRQF_ONESHOT-(unless-it-is-nested-IRQ)
elapsed time: 1446m
configs tested: 191
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allmodconfig gcc
arc allnoconfig gcc
arc allyesconfig gcc
arc defconfig gcc
arc randconfig-001-20231229 gcc
arc randconfig-002-20231229 gcc
arm allmodconfig gcc
arm allnoconfig gcc
arm allyesconfig gcc
arm defconfig clang
arm footbridge_defconfig gcc
arm lpc32xx_defconfig clang
arm milbeaut_m10v_defconfig clang
arm mps2_defconfig gcc
arm multi_v4t_defconfig gcc
arm randconfig-001-20231229 clang
arm randconfig-002-20231229 clang
arm randconfig-003-20231229 clang
arm randconfig-004-20231229 clang
arm spear3xx_defconfig clang
arm spear6xx_defconfig gcc
arm wpcm450_defconfig gcc
arm64 allmodconfig clang
arm64 allnoconfig gcc
arm64 defconfig gcc
arm64 randconfig-001-20231229 clang
arm64 randconfig-002-20231229 clang
arm64 randconfig-003-20231229 clang
arm64 randconfig-004-20231229 clang
csky allmodconfig gcc
csky allnoconfig gcc
csky allyesconfig gcc
csky defconfig gcc
csky randconfig-001-20231229 gcc
csky randconfig-002-20231229 gcc
hexagon allmodconfig clang
hexagon allnoconfig clang
hexagon allyesconfig clang
hexagon defconfig clang
hexagon randconfig-001-20231229 clang
hexagon randconfig-002-20231229 clang
i386 allmodconfig clang
i386 allnoconfig clang
i386 allyesconfig clang
i386 buildonly-randconfig-001-20231229 clang
i386 buildonly-randconfig-002-20231229 clang
i386 buildonly-randconfig-003-20231229 clang
i386 buildonly-randconfig-004-20231229 clang
i386 buildonly-randconfig-005-20231229 clang
i386 buildonly-randconfig-006-20231229 clang
i386 defconfig gcc
i386 randconfig-001-20231229 clang
i386 randconfig-002-20231229 clang
i386 randconfig-003-20231229 clang
i386 randconfig-004-20231229 clang
i386 randconfig-005-20231229 clang
i386 randconfig-006-20231229 clang
i386 randconfig-011-20231229 gcc
i386 randconfig-012-20231229 gcc
i386 randconfig-013-20231229 gcc
i386 randconfig-014-20231229 gcc
i386 randconfig-015-20231229 gcc
i386 randconfig-016-20231229 gcc
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
loongarch randconfig-001-20231229 gcc
loongarch randconfig-002-20231229 gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k apollo_defconfig gcc
m68k defconfig gcc
m68k m5208evb_defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allnoconfig clang
mips allyesconfig gcc
mips cavium_octeon_defconfig gcc
mips cu1830-neo_defconfig clang
mips gcw0_defconfig gcc
mips ip22_defconfig gcc
mips loongson1b_defconfig gcc
mips loongson2k_defconfig gcc
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
nios2 randconfig-001-20231229 gcc
nios2 randconfig-002-20231229 gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-001-20231229 gcc
parisc randconfig-002-20231229 gcc
parisc64 defconfig gcc
powerpc allmodconfig clang
powerpc allnoconfig gcc
powerpc allyesconfig clang
powerpc canyonlands_defconfig gcc
powerpc cm5200_defconfig gcc
powerpc klondike_defconfig gcc
powerpc pasemi_defconfig gcc
powerpc randconfig-001-20231229 clang
powerpc randconfig-002-20231229 clang
powerpc randconfig-003-20231229 clang
powerpc tqm5200_defconfig clang
powerpc tqm8548_defconfig gcc
powerpc64 randconfig-001-20231229 clang
powerpc64 randconfig-002-20231229 clang
powerpc64 randconfig-003-20231229 clang
riscv allmodconfig gcc
riscv allnoconfig clang
riscv allyesconfig gcc
riscv defconfig gcc
riscv randconfig-001-20231229 clang
riscv randconfig-002-20231229 clang
riscv rv32_defconfig clang
s390 allmodconfig gcc
s390 allnoconfig gcc
s390 allyesconfig gcc
s390 defconfig gcc
s390 randconfig-001-20231229 gcc
s390 randconfig-002-20231229 gcc
s390 zfcpdump_defconfig gcc
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh defconfig gcc
sh edosk7760_defconfig gcc
sh lboxre2_defconfig gcc
sh randconfig-001-20231229 gcc
sh randconfig-002-20231229 gcc
sh sdk7780_defconfig gcc
sh se7619_defconfig gcc
sh ul2_defconfig gcc
sparc allmodconfig gcc
sparc64 alldefconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
sparc64 randconfig-001-20231229 gcc
sparc64 randconfig-002-20231229 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um randconfig-001-20231229 clang
um randconfig-002-20231229 clang
um x86_64_defconfig gcc
x86_64 allnoconfig gcc
x86_64 allyesconfig clang
x86_64 buildonly-randconfig-001-20231229 clang
x86_64 buildonly-randconfig-002-20231229 clang
x86_64 buildonly-randconfig-003-20231229 clang
x86_64 buildonly-randconfig-004-20231229 clang
x86_64 buildonly-randconfig-005-20231229 clang
x86_64 buildonly-randconfig-006-20231229 clang
x86_64 defconfig gcc
x86_64 randconfig-001-20231229 gcc
x86_64 randconfig-002-20231229 gcc
x86_64 randconfig-003-20231229 gcc
x86_64 randconfig-004-20231229 gcc
x86_64 randconfig-005-20231229 gcc
x86_64 randconfig-006-20231229 gcc
x86_64 randconfig-011-20231229 clang
x86_64 randconfig-012-20231229 clang
x86_64 randconfig-013-20231229 clang
x86_64 randconfig-014-20231229 clang
x86_64 randconfig-015-20231229 clang
x86_64 randconfig-016-20231229 clang
x86_64 randconfig-071-20231229 clang
x86_64 randconfig-072-20231229 clang
x86_64 randconfig-073-20231229 clang
x86_64 randconfig-074-20231229 clang
x86_64 randconfig-075-20231229 clang
x86_64 randconfig-076-20231229 clang
x86_64 rhel-8.3-rust clang
xtensa allnoconfig gcc
xtensa randconfig-001-20231229 gcc
xtensa randconfig-002-20231229 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: Add Himax HX83102J touchscreen
From: Krzysztof Kozlowski @ 2023-12-30 14:19 UTC (permalink / raw)
To: Allen Lin
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
benjamin.tissoires, linux-input, devicetree, linux-kernel
In-Reply-To: <TY0PR06MB561188EBD127F8ECF4A7052B9E9DA@TY0PR06MB5611.apcprd06.prod.outlook.com>
On 29/12/2023 10:08, Allen Lin wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2023年12月28日 週四 下午6:36寫道：
>>
>> On 27/12/2023 06:35, Allen_Lin wrote:
>>> Add the HX83102j touchscreen device tree bindings documents.
>>>
>>> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
>>> ---
>>
>> Where is the changelog? There is no cover letter attached, so changelog
>> is supposed to be here. There were several comments, so does it mean you
>> ignored them?
>>
> Cover letter is not in this mail but in the mail with this title
> "[PATCH v3 0/2] Add HX83102j driver for HIMAX HID touchscreen"
There was no cover letter attached to this thread. Don't send cover
letters in separate threads.
>
> Hi,
> This driver implements for Himax HID touchscreen HX83102j.
>
> Using SPI interface to receive/send HID packets.
>
> Patchs notes as below
> 1. Add the Maintainer and devicetree bindings document for driver
> 2. Add the driver code and modify Kconfig/Makefile to support the driver
>
> change in v2 :
> - Fix kernel test robot build warnings.
> change in v3 :
> - Modify code according to review suggesions.
Not detailed enough. What did you change exactly?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v6 1/4] dt-bindings: touchscreen: add touch-overlay property
From: Jeff LaBundy @ 2023-12-30 18:10 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20230510-feature-ts_virtobj_patch-v6-1-d8a605975153@wolfvision.net>
Hi Javier,
This is excellent, just one small comment below.
On Wed, Dec 20, 2023 at 09:39:43AM +0100, Javier Carrasco wrote:
> The touch-overlay encompasses a number of touch areas that define a
> clipped touchscreen area and/or buttons with a specific functionality.
>
> A clipped touchscreen area avoids getting events from regions that are
> physically hidden by overlay frames.
>
> For touchscreens with printed overlay buttons, sub-nodes with a suitable
> key code can be defined to report key events instead of the original
> touch events.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
> .../bindings/input/touchscreen/touchscreen.yaml | 119 +++++++++++++++++++++
> 1 file changed, 119 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> index 431c13335c40..d5ac90667bef 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> @@ -87,6 +87,125 @@ properties:
> touchscreen-y-plate-ohms:
> description: Resistance of the Y-plate in Ohms
>
> + touch-overlay:
> + description: list of nodes defining segments (touch areas) on the
> + touchscreen.
> +
> + This object can be used to describe a series of segments to
> + restrict the region within touch events are reported or buttons
> + with a specific functionality.
> +
> + This is of special interest if the touchscreen is shipped with a physical
> + overlay on top of it with a frame that hides some part of the original
> + touchscreen area. Printed buttons on that overlay are also a typical
> + use case.
> +
> + A new touchscreen area is defined as a sub-node without a key code. If a
> + key code is defined in the sub-node, it will be interpreted as a button.
> +
> + The x-origin and y-origin properties of a touchscreen area define the
> + offset of a new origin from where the touchscreen events are referenced.
> + This offset is applied to the events accordingly. The x-size and y-size
> + properties define the size of the touchscreen effective area.
> +
> + The following example shows a new touchscreen area with the new origin
> + (0',0') for the touch events generated by the device.
> +
> + Touchscreen (full area)
> + ┌────────────────────────────────────────┐
> + │ ┌───────────────────────────────┐ │
> + │ │ │ │
> + │ ├ y-size │ │
> + │ │ │ │
> + │ │ touchscreen area │ │
> + │ │ (no key code) │ │
> + │ │ │ │
> + │ │ x-size │ │
> + │ ┌└──────────────┴────────────────┘ │
> + │(0',0') │
> + ┌└────────────────────────────────────────┘
> + (0,0)
> +
> + where (0',0') = (0+x-origin,0+y-origin)
> +
> + Sub-nodes with key codes report the touch events on their surface as key
> + events instead.
> +
> + The following example shows a touchscreen with a single button on it.
> +
> + Touchscreen (full area)
> + ┌───────────────────────────────────┐
> + │ │
> + │ │
> + │ ┌─────────┐ │
> + │ │button 0 │ │
> + │ │KEY_POWER│ │
> + │ └─────────┘ │
> + │ │
> + │ │
> + ┌└───────────────────────────────────┘
> + (0,0)
> +
> + Segments defining buttons and clipped toushcreen areas can be combined
> + as shown in the following example.
> + In that case only the events within the touchscreen area are reported
> + as touch events. Events within the button areas report their associated
> + key code. Any events outside the defined areas are ignored.
> +
> + Touchscreen (full area)
> + ┌─────────┬──────────────────────────────┐
> + │ │ │
> + │ │ ┌───────────────────────┐ │
> + │ button 0│ │ │ │
> + │KEY_POWER│ │ │ │
> + │ │ │ │ │
> + ├─────────┤ │ touchscreen area │ │
> + │ │ │ (no key code) │ │
> + │ │ │ │ │
> + │ button 1│ │ │ │
> + │ KEY_INFO│ ┌└───────────────────────┘ │
> + │ │(0',0') │
> + ┌└─────────┴──────────────────────────────┘
> + (0,0)
> +
> + type: object
> +
> + patternProperties:
> + '^segment-':
> + type: object
> + description:
> + Each segment is represented as a sub-node.
> + properties:
> + x-origin:
> + description: horizontal origin of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + y-origin:
> + description: vertical origin of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + x-size:
> + description: horizontal resolution of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + y-size:
> + description: vertical resolution of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + label:
> + $ref: /schemas/types.yaml#/definitions/string
> + description: descriptive name of the button
Please consider replacing the word "button" with "segment"; as we see in
patch [3/4], the label can describe a touch surface and not just a button.
> +
> + linux,code: true
> +
> + required:
> + - x-origin
> + - y-origin
> + - x-size
> + - y-size
> +
> + unevaluatedProperties: false
> +
> dependencies:
> touchscreen-size-x: [ touchscreen-size-y ]
> touchscreen-size-y: [ touchscreen-size-x ]
>
> --
> 2.39.2
>
>
With that changed, feel free to add:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Obviously we need Dmitry and the binding maintainers to review as well; I am
merely expressing my own agreement as a future customer of this function.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v6 2/4] Input: touch-overlay - Add touchscreen overlay handling
From: Jeff LaBundy @ 2023-12-30 20:29 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20230510-feature-ts_virtobj_patch-v6-2-d8a605975153@wolfvision.net>
Hi Javier,
Truly excellent work; I'm glad to see this keep moving forward, and the
new linked list approach looks well-suited to this application.
On Wed, Dec 20, 2023 at 09:39:44AM +0100, Javier Carrasco wrote:
> Some touch devices provide mechanical overlays with different objects
> like buttons or clipped touchscreen surfaces.
>
> In order to support these objects, add a series of helper functions
> to the input subsystem to transform them into overlay objects via
> device tree nodes.
>
> These overlay objects consume the raw touch events and report the
> expected input events depending on the object properties.
>
> Note that the current implementation allows for multiple definitions
> of touchscreen areas (regions that report touch events), but only the
> first one will be used for the touchscreen device that the consumers
> typically provide.
> Should the need for multiple touchscreen areas arise, additional
> touchscreen devices would be required at the consumer side.
> There is no limitation in the number of touch areas defined as buttons.
Having reviewed this version in detail, it's clear how the implementation
imposes this restriction. However, it's not clear why we have to have this
restriction straight out of the gate; it also breaks the "square doughnut"
example we discussed in v5, where a button resides inside a touch surface
which is split into four discrete rectangles that report X/Y coordinates
relative to the same origin.
From my naive point of view, a driver should merely pass a contact's ID
(for HW that can track contacts), coordinates, and pressure. Your helper(s)
are then responisble for iterating over the list, determining the segment
in which the coordinates fall, and then reporting the event by way of
input_report_abs() or input_report_key() based on whether or not a keycode
is defined.
I think the problem with that approach is that touchscreen drivers only
report coordinates when the pressure is nonzero. The process of dropping
a contact, i.e. button release for some segments, happens inside input-mt
by virtue of the driver calling input_mt_sync_frame().
It makes sense now why you are duplicating the contact tracking to a degree
here. Therefore, it's starting to look more and more like the overlay segment
handling needs to move into the input-mt core, where much of the information
you need already exists.
If we look at input_mt_report_pointer_emulation(), the concept of button
release is already happening; all we really want to do here is gently
expand the core to understand that some ranges of coordinates are simply
quantized to a keycode with binary pressure (i.e. press/release).
In addition to removing duplicate code as well as the restriction of supporting
only one X/Y surface, moving overlay support into the input-mt core would
remove the need to modify each touchscreen driver one at a time with what
are largely the same nontrivial changes. If we think about it more, the
touchscreen controller itself is not changing, so the driver really shouldn't
have to change much either.
Stated another way, I think it's a better design pattern if we let drivers
continue to do their job of merely lobbing hardware state to the input
subsytem via input_mt_slot(), touchscreen_report_pos() and input_mt_sync_frame(),
then leave it to the input subsystem alone to iterate over the list and
determine whether some coordinates must be handled differently.
The main drawback to this approach is that the overlay buttons would need
to go back to being part of the touchscreen input device as in v1, as opposed
to giving the driver the flexibility of splitting the buttons and X/Y surfaces
into two separate input devices.
When we first discussed this with Peter, we agreed that splitting them into two
input devices grants the most flexibility, in case user space opts to inhibit
one but not the other, etc. However since the buttons and X/Y surfaces are all
part of the same physical substrate, it seems the chances of user space being
interested in one but not the other are low.
Furthermore, folding the buttons and X/Y surfaces back into the same input
device would remove the need for each touchscreen driver to preemptively
allocate a second input device, but then remove it later as in patch [4/4]
in case the helpers did not find any buttons.
What are your thoughts on evolving the approach in this way? It's obviously
another big change and carries some risk to the core, so I'm curious to hear
Dmitry's and others' thoughts as well. I appreciate that you've been iterating
on this for some time, and good is not the enemy of great; therefore, maybe
a compromise is to move forward with the current approach in support of the
hardware you have today, then work it into the input-mt core over time. But
it would be nice to avoid ripping up participating touchscreen drivers twice.
Thank you for your patience and continued effort. In the meantime, please note
some minor comments that are independent of this architectural decision.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
> MAINTAINERS | 7 +
> drivers/input/Makefile | 2 +-
> drivers/input/touch-overlay.c | 283 ++++++++++++++++++++++++++++++++++++
> include/linux/input/touch-overlay.h | 24 +++
> 4 files changed, 315 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 97f51d5ec1cf..3218d8694735 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22031,6 +22031,13 @@ L: platform-driver-x86@vger.kernel.org
> S: Maintained
> F: drivers/platform/x86/toshiba-wmi.c
>
> +TOUCH OVERLAY
> +M: Javier Carrasco <javier.carrasco@wolfvision.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: drivers/input/touch-overlay.c
> +F: include/linux/input/touch-overlay.h
> +
> TPM DEVICE DRIVER
> M: Peter Huewe <peterhuewe@gmx.de>
> M: Jarkko Sakkinen <jarkko@kernel.org>
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index c78753274921..393e9f4d00dc 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -7,7 +7,7 @@
>
> obj-$(CONFIG_INPUT) += input-core.o
> input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> -input-core-y += touchscreen.o
> +input-core-y += touchscreen.o touch-overlay.o
>
> obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> obj-$(CONFIG_INPUT_SPARSEKMAP) += sparse-keymap.o
> diff --git a/drivers/input/touch-overlay.c b/drivers/input/touch-overlay.c
> new file mode 100644
> index 000000000000..0a0646ceb755
> --- /dev/null
> +++ b/drivers/input/touch-overlay.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Helper functions for overlay objects on touchscreens
> + *
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touch-overlay.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +
> +struct button {
> + u32 key;
> + bool pressed;
> + int slot;
> +};
> +
> +struct touch_overlay_segment {
> + struct list_head list;
> + u32 x_origin;
> + u32 y_origin;
> + u32 x_size;
> + u32 y_size;
> + struct button *btn;
I think you can simply declare the struct itself here as opposed to a pointer to
one; this would avoid a second call to devm_kzalloc().
> +};
> +
> +static int touch_overlay_get_segment(struct fwnode_handle *segment_node,
> + struct touch_overlay_segment *segment,
> + struct input_dev *keypad,
> + struct device *dev)
> +{
> + int error;
> +
> + error = fwnode_property_read_u32(segment_node, "x-origin",
> + &segment->x_origin);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "y-origin",
> + &segment->y_origin);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "x-size",
> + &segment->x_size);
> + if (error)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "y-size",
> + &segment->y_size);
> + if (error)
> + return error;
> +
> + if (fwnode_property_present(segment_node, "linux,code")) {
Instead of walking the device tree twice by calling fwnode_property_present()
followed by fwnode_property_read_u32(), you can simply check whether the
latter returns -EINVAL, which indicates the optional property was absent.
> + segment->btn = devm_kzalloc(dev, sizeof(*segment->btn),
> + GFP_KERNEL);
> + if (!segment->btn)
> + return -ENOMEM;
> +
> + error = fwnode_property_read_u32(segment_node, "linux,code",
> + &segment->btn->key);
> + if (error)
> + return error;
> +
> + input_set_capability(keypad, EV_KEY, segment->btn->key);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * touch_overlay_map - map overlay objects from the device tree and set
> + * key capabilities if buttons are defined.
> + * @list: pointer to the list that will hold the segments
> + * @keypad: pointer to the already allocated input_dev
> + *
> + * Returns 0 on success and error number otherwise.
> + *
> + * If a keypad input device is provided and overlay buttons are defined,
> + * its button capabilities are set accordingly.
> + */
> +int touch_overlay_map(struct list_head *list, struct input_dev *keypad)
> +{
> + struct fwnode_handle *overlay, *fw_segment;
> + struct device *dev = keypad->dev.parent;
> + struct touch_overlay_segment *segment;
> + int error;
> +
> + overlay = device_get_named_child_node(dev, "touch-overlay");
> + if (!overlay)
> + return 0;
> +
> + fwnode_for_each_child_node(overlay, fw_segment) {
> + segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
> + if (!segment) {
> + error = -ENOMEM;
> + goto put_overlay;
For this and the below case where you exit the loop early in case of an
error, you must call fwnode_handle_put(fw_segment) manually. The reference
count is handled automatically only when the loop iterates and terminates
naturally.
Since nothing else happens between the loop and the 'put_overlay' label,
you can also replace the goto with a break and remove the label altogether.
> + }
> + error = touch_overlay_get_segment(fw_segment, segment, keypad,
> + dev);
> + if (error)
> + goto put_overlay;
> +
> + list_add_tail(&segment->list, list);
> + }
> +
> +put_overlay:
> + fwnode_handle_put(overlay);
> + return error;
> +}
> +EXPORT_SYMBOL(touch_overlay_map);
> +
> +/**
> + * touch_overlay_get_touchscreen_abs - get abs size from the touchscreen area.
> + * @list: pointer to the list that holds the segments
> + * @x: horizontal abs
> + * @y: vertical abs
> + */
> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (!segment->btn) {
> + *x = segment->x_size - 1;
> + *y = segment->y_size - 1;
> + break;
> + }
> + }
> +}
> +EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
> +
> +static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
> + u32 x, u32 y)
> +{
> + if (!seg)
> + return false;
> +
> + if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
> + y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_mapped_touchscreen - check if a touchscreen area is mapped
> + * @list: pointer to the list that holds the segments
> + *
> + * Returns true if a touchscreen area is mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_touchscreen(struct list_head *list)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (!segment->btn)
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
> +
> +/**
> + * touch_overlay_mapped_buttons - check if overlay buttons are mapped
> + * @list: pointer to the list that holds the segments
> + *
> + * Returns true if overlay buttons mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_buttons(struct list_head *list)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (segment->btn)
> + return true;
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_buttons);
> +
> +static bool touch_overlay_mt_on_touchscreen(struct list_head *list,
> + u32 *x, u32 *y)
> +{
> + struct touch_overlay_segment *segment;
> + bool contact = x && y;
> + struct list_head *ptr;
> +
> + /* Let the caller handle events with no coordinates (release) */
> + if (!contact)
> + return false;
> +
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (!segment->btn &&
> + touch_overlay_segment_event(segment, *x, *y)) {
> + *x -= segment->x_origin;
> + *y -= segment->y_origin;
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static bool touch_overlay_button_event(struct input_dev *input,
> + struct touch_overlay_segment *segment,
> + const u32 *x, const u32 *y, u32 slot)
> +{
> + bool contact = x && y;
> +
> + if (!contact && segment->btn->pressed && segment->btn->slot == slot) {
> + segment->btn->pressed = false;
> + input_report_key(input, segment->btn->key, false);
> + input_sync(input);
> + return true;
> + } else if (contact && touch_overlay_segment_event(segment, *x, *y)) {
> + segment->btn->pressed = true;
> + segment->btn->slot = slot;
> + input_report_key(input, segment->btn->key, true);
> + input_sync(input);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_process_event - process input events according to the overlay
> + * mapping. This function acts as a filter to release the calling driver from
> + * the events that are either related to overlay buttons or out of the overlay
> + * touchscreen area if defined.
> + * @list: pointer to the list that holds the segments
> + * @input: pointer to the input device associated to the event
> + * @x: pointer to the x coordinate (NULL if not available - no contact)
> + * @y: pointer to the y coordinate (NULL if not available - no contact)
> + * @slot: slot associated to the event
> + *
> + * Returns true if the event was processed (reported for valid key events
> + * and dropped for events outside the overlay touchscreen area) or false
> + * if the event must be processed by the caller. In that case this function
> + * shifts the (x,y) coordinates to the overlay touchscreen axis if required.
> + */
> +bool touch_overlay_process_event(struct list_head *list,
> + struct input_dev *input,
> + u32 *x, u32 *y, u32 slot)
> +{
> + struct touch_overlay_segment *segment;
> + struct list_head *ptr;
> +
> + /*
> + * buttons must be prioritized over overlay touchscreens to account for
> + * overlappings e.g. a button inside the touchscreen area.
> + */
> + list_for_each(ptr, list) {
> + segment = list_entry(ptr, struct touch_overlay_segment, list);
> + if (segment->btn &&
> + touch_overlay_button_event(input, segment, x, y, slot)) {
> + return true;
> + }
> + }
> +
> + /*
> + * valid touch events on the overlay touchscreen are left for the client
> + * to be processed/reported according to its (possibly) unique features.
> + */
> + return !touch_overlay_mt_on_touchscreen(list, x, y);
> +}
> +EXPORT_SYMBOL(touch_overlay_process_event);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
> diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
> new file mode 100644
> index 000000000000..df974eb46dd4
> --- /dev/null
> +++ b/include/linux/input/touch-overlay.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#ifndef _TOUCH_OVERLAY
> +#define _TOUCH_OVERLAY
> +
> +#include <linux/types.h>
> +
> +struct input_dev;
> +
> +int touch_overlay_map(struct list_head *list, struct input_dev *keypad);
> +
> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y);
> +
> +bool touch_overlay_mapped_touchscreen(struct list_head *list);
> +
> +bool touch_overlay_mapped_buttons(struct list_head *list);
> +
> +bool touch_overlay_process_event(struct list_head *list, struct input_dev *input,
> + u32 *x, u32 *y, u32 slot);
> +
> +#endif
>
> --
> 2.39.2
>
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH 0/2] Allow inverted axes for adc-joystick
From: Chris Morgan @ 2023-12-31 20:56 UTC (permalink / raw)
To: linux-input
Cc: dmitry.torokhov, hdegoede, paul, peter.hutterer, svv, biswarupp,
contact, Chris Morgan
From: Chris Morgan <macromorgan@hotmail.com>
Handle inverted axes in the adc-joystick driver so that reported input
events downstream from the driver conform with assumptions that
abs_min < abs_max. Add a new helper function for inputs so that abs
can be inverted that drivers can use.
Chris Morgan (2):
Input: add input_invert_abs()
Input: adc-joystick: Handle inverted axes
drivers/input/input.c | 19 +++++++++++++++++++
drivers/input/joystick/adc-joystick.c | 13 ++++++++++++-
include/linux/input.h | 1 +
3 files changed, 32 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply
* [PATCH 1/2] Input: add input_invert_abs()
From: Chris Morgan @ 2023-12-31 20:56 UTC (permalink / raw)
To: linux-input
Cc: dmitry.torokhov, hdegoede, paul, peter.hutterer, svv, biswarupp,
contact, Chris Morgan
In-Reply-To: <20231231205643.129435-1-macroalpha82@gmail.com>
From: Chris Morgan <macromorgan@hotmail.com>
Add a helper function to make it easier for a driver to invert abs
values when needed. It is up to the driver itself to track axes that
need to be inverted and normalize the data before it is passed on.
This function assumes that drivers will set the min and max values
so that min < max and then will simply call this function each time
the values need to be inverted.
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
drivers/input/input.c | 19 +++++++++++++++++++
include/linux/input.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 8c5fdb0f858a..f135aed165a1 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -552,6 +552,25 @@ void input_copy_abs(struct input_dev *dst, unsigned int dst_axis,
}
EXPORT_SYMBOL(input_copy_abs);
+/**
+ * input_invert_abs - Invert the abs value for an inverted axis.
+ * @dev: Input device with absolute events
+ * @axis: ABS_* value selecting the destination axis for the event to
+ * invert.
+ * @val: Value to be inverted based on min and max values of the axis.
+ *
+ * Return an inverted value for a given ABS axis based on its min and
+ * max values.
+ */
+int input_invert_abs(struct input_dev *dev, unsigned int axis, int val)
+{
+ int min = dev->absinfo[axis].minimum;
+ int max = dev->absinfo[axis].maximum;
+
+ return (max + min) - val;
+}
+EXPORT_SYMBOL(input_invert_abs);
+
/**
* input_grab_device - grabs device for exclusive use
* @handle: input handle that wants to own the device
diff --git a/include/linux/input.h b/include/linux/input.h
index de6503c0edb8..deb5f8bb0ec7 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -477,6 +477,7 @@ void input_set_abs_params(struct input_dev *dev, unsigned int axis,
int min, int max, int fuzz, int flat);
void input_copy_abs(struct input_dev *dst, unsigned int dst_axis,
const struct input_dev *src, unsigned int src_axis);
+int input_invert_abs(struct input_dev *dev, unsigned int axis, int val);
#define INPUT_GENERATE_ABS_ACCESSORS(_suffix, _item) \
static inline int input_abs_get_##_suffix(struct input_dev *dev, \
--
2.34.1
^ permalink raw reply related
* [PATCH 2/2] Input: adc-joystick: Handle inverted axes
From: Chris Morgan @ 2023-12-31 20:56 UTC (permalink / raw)
To: linux-input
Cc: dmitry.torokhov, hdegoede, paul, peter.hutterer, svv, biswarupp,
contact, Chris Morgan
In-Reply-To: <20231231205643.129435-1-macroalpha82@gmail.com>
From: Chris Morgan <macromorgan@hotmail.com>
When one or more axes are inverted, (where min > max), normalize the
data so that min < max and call a helper function to invert the
values reported to the input stack.
This ensures we can continue defining the device correctly in the
device tree while not breaking downstream assumptions that min is
always less than max.
Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
drivers/input/joystick/adc-joystick.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..4e8d446987b6 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -18,6 +18,7 @@ struct adc_joystick_axis {
s32 range[2];
s32 fuzz;
s32 flat;
+ bool inverted;
};
struct adc_joystick {
@@ -38,6 +39,8 @@ static void adc_joystick_poll(struct input_dev *input)
ret = iio_read_channel_raw(&joy->chans[i], &val);
if (ret < 0)
return;
+ if (joy->axes[i].inverted)
+ val = input_invert_abs(input, i, val);
input_report_abs(input, joy->axes[i].code, val);
}
input_sync(input);
@@ -86,6 +89,8 @@ static int adc_joystick_handle(const void *data, void *private)
val = sign_extend32(val, msb);
else
val &= GENMASK(msb, 0);
+ if (joy->axes[i].inverted)
+ val = input_invert_abs(joy->input, i, val);
input_report_abs(joy->input, joy->axes[i].code, val);
}
@@ -168,11 +173,17 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
goto err_fwnode_put;
}
+ if (axes[i].range[0] > axes[i].range[1]) {
+ dev_dbg(dev, "abs-axis %d inverted\n", i);
+ axes[i].inverted = 1;
+ }
+
fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz);
fwnode_property_read_u32(child, "abs-flat", &axes[i].flat);
input_set_abs_params(joy->input, axes[i].code,
- axes[i].range[0], axes[i].range[1],
+ min_array(axes[i].range, 2),
+ max_array(axes[i].range, 2),
axes[i].fuzz, axes[i].flat);
input_set_capability(joy->input, EV_ABS, axes[i].code);
}
--
2.34.1
^ permalink raw reply related
* [PATCH v3 0/4] Add support for slider gestures and OTP variants
From: Jeff LaBundy @ 2024-01-01 20:00 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, jeff
This series introduces support for some additional features offered by the
Azoteq IQS269A capacitive touch controller.
Patches 1 and 2 add support for slider gestures (e.g. tap or swipe). Gestures
are recognized by the hardware itself based on touch activity across the chan-
nels associated with the slider. This feature is useful for lightweight systems
that do not post-process absolute coordinates to determine gestures expressed
by the user.
Gestures are presented to user space as keycodes. An example use-case is an
array of multimedia keys as seen in the following demo:
https://youtu.be/k_vMRQiHLgA
Patches 3 and 4 add support for the device's available OTP variants, which
trade features or exhibit errata that require workarounds.
Jeff LaBundy (4):
dt-bindings: input: iqs269a: Add bindings for slider gestures
Input: iqs269a - add support for slider gestures
dt-bindings: input: iqs269a: Add bindings for OTP variants
Input: iqs269a - add support for OTP variants
.../devicetree/bindings/input/iqs269a.yaml | 98 +++++-
drivers/input/misc/iqs269a.c | 312 ++++++++++++++++--
2 files changed, 377 insertions(+), 33 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v3 1/4] dt-bindings: input: iqs269a: Add bindings for slider gestures
From: Jeff LaBundy @ 2024-01-01 20:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, jeff
In-Reply-To: <ZZMZzeX77VeHdIeL@nixie71>
This patch adds bindings for slider gestures that can be expressed
by the device.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v3:
- None
Changes in v2:
- Renamed 'azoteq,slider-swipe' to 'azoteq,gesture-swipe' to match IQS626A and
noted it as depending upon 'linux,keycodes'
- Dropped the $ref declaration for 'linux,keycodes' and referenced input.yaml,
which has since been updated to define this property
.../devicetree/bindings/input/iqs269a.yaml | 68 +++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/iqs269a.yaml b/Documentation/devicetree/bindings/input/iqs269a.yaml
index 3c430d38594f..b42f07542d27 100644
--- a/Documentation/devicetree/bindings/input/iqs269a.yaml
+++ b/Documentation/devicetree/bindings/input/iqs269a.yaml
@@ -9,6 +9,9 @@ title: Azoteq IQS269A Capacitive Touch Controller
maintainers:
- Jeff LaBundy <jeff@labundy.com>
+allOf:
+ - $ref: input.yaml#
+
description: |
The Azoteq IQS269A is an 8-channel capacitive touch controller that features
additional Hall-effect and inductive sensing capabilities.
@@ -204,6 +207,63 @@ properties:
default: 1
description: Specifies the slider coordinate filter strength.
+ linux,keycodes:
+ minItems: 1
+ maxItems: 8
+ description: |
+ Specifies the numeric keycodes associated with each available gesture in
+ the following order (enter 0 for unused gestures):
+ 0: Slider 0 tap
+ 1: Slider 0 hold
+ 2: Slider 0 positive flick or swipe
+ 3: Slider 0 negative flick or swipe
+ 4: Slider 1 tap
+ 5: Slider 1 hold
+ 6: Slider 1 positive flick or swipe
+ 7: Slider 1 negative flick or swipe
+
+ azoteq,gesture-swipe:
+ type: boolean
+ description:
+ Directs the device to interpret axial gestures as a swipe (finger remains
+ on slider) instead of a flick (finger leaves slider).
+
+ azoteq,timeout-tap-ms:
+ multipleOf: 16
+ minimum: 0
+ maximum: 4080
+ default: 400
+ description:
+ Specifies the length of time (in ms) within which a slider touch must be
+ released in order to be interpreted as a tap. Default and maximum values
+ as well as step size are reduced by a factor of 4 with device version 2.
+
+ azoteq,timeout-swipe-ms:
+ multipleOf: 16
+ minimum: 0
+ maximum: 4080
+ default: 2000
+ description:
+ Specifies the length of time (in ms) within which an axial gesture must be
+ completed in order to be interpreted as a flick or swipe. Default and max-
+ imum values as well as step size are reduced by a factor of 4 with device
+ version 2.
+
+ azoteq,thresh-swipe:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 255
+ default: 128
+ description:
+ Specifies the number of points across which an axial gesture must travel
+ in order to be interpreted as a flick or swipe.
+
+dependencies:
+ azoteq,gesture-swipe: ["linux,keycodes"]
+ azoteq,timeout-tap-ms: ["linux,keycodes"]
+ azoteq,timeout-swipe-ms: ["linux,keycodes"]
+ azoteq,thresh-swipe: ["linux,keycodes"]
+
patternProperties:
"^channel@[0-7]$":
type: object
@@ -484,6 +544,14 @@ examples:
azoteq,hall-enable;
azoteq,suspend-mode = <2>;
+ linux,keycodes = <KEY_PLAYPAUSE>,
+ <KEY_STOPCD>,
+ <KEY_NEXTSONG>,
+ <KEY_PREVIOUSSONG>;
+
+ azoteq,timeout-tap-ms = <400>;
+ azoteq,timeout-swipe-ms = <800>;
+
channel@0 {
reg = <0x0>;
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/4] Input: iqs269a - add support for slider gestures
From: Jeff LaBundy @ 2024-01-01 20:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, jeff
In-Reply-To: <ZZMZzeX77VeHdIeL@nixie71>
This patch adds support for slider gestures that can be expressed
by the device. Each gesture (e.g. tap or hold) can be mapped to a
unique keycode for either slider 0 or 1.
With this change, raw slider coordinates are reported only if the
slider has no keycodes defined. This prevents unwanted mouse cur-
sor movement when expressing axial gestures (e.g. swipe) and also
eliminates some unnecessary I2C traffic.
Different revisions of silicon use different tap and swipe timeout
step sizes. Apply an appropriate scaling factor depending on which
revision is found.
To facilitate this change, store the iqs269_ver_info struct in the
driver's private data so that other functions can use it after the
driver has probed.
Last but not least, a former reserved field in iqs269_ver_info now
contains useful information; give it a name (fw_num).
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v3:
- None
Changes in v2:
- Rebased onto latest driver
- Renamed 'azoteq,slider-swipe' to 'azoteq,gesture-swipe' to match IQS626A
- Renamed 'gs_code' to 'sl_code'
drivers/input/misc/iqs269a.c | 220 ++++++++++++++++++++++++++++++-----
1 file changed, 191 insertions(+), 29 deletions(-)
diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index 1abce35b955e..0d0b5cdc7830 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -9,6 +9,7 @@
* axial sliders presented by the device.
*/
+#include <linux/bits.h>
#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/device.h>
@@ -26,6 +27,8 @@
#define IQS269_VER_INFO 0x00
#define IQS269_VER_INFO_PROD_NUM 0x4F
+#define IQS269_VER_INFO_FW_NUM_2 0x03
+#define IQS269_VER_INFO_FW_NUM_3 0x10
#define IQS269_SYS_FLAGS 0x02
#define IQS269_SYS_FLAGS_SHOW_RESET BIT(15)
@@ -53,6 +56,7 @@
#define IQS269_SYS_SETTINGS_ULP_UPDATE_MASK GENMASK(10, 8)
#define IQS269_SYS_SETTINGS_ULP_UPDATE_SHIFT 8
#define IQS269_SYS_SETTINGS_ULP_UPDATE_MAX 7
+#define IQS269_SYS_SETTINGS_SLIDER_SWIPE BIT(7)
#define IQS269_SYS_SETTINGS_RESEED_OFFSET BIT(6)
#define IQS269_SYS_SETTINGS_EVENT_MODE BIT(5)
#define IQS269_SYS_SETTINGS_EVENT_MODE_LP BIT(4)
@@ -69,6 +73,7 @@
#define IQS269_FILT_STR_MAX 3
#define IQS269_EVENT_MASK_SYS BIT(6)
+#define IQS269_EVENT_MASK_GESTURE BIT(3)
#define IQS269_EVENT_MASK_DEEP BIT(2)
#define IQS269_EVENT_MASK_TOUCH BIT(1)
#define IQS269_EVENT_MASK_PROX BIT(0)
@@ -97,6 +102,10 @@
#define IQS269_MISC_B_TRACKING_UI_ENABLE BIT(4)
#define IQS269_MISC_B_FILT_STR_SLIDER GENMASK(1, 0)
+#define IQS269_TIMEOUT_TAP_MS_MAX 4080
+#define IQS269_TIMEOUT_SWIPE_MS_MAX 4080
+#define IQS269_THRESH_SWIPE_MAX 255
+
#define IQS269_CHx_ENG_A_MEAS_CAP_SIZE BIT(15)
#define IQS269_CHx_ENG_A_RX_GND_INACTIVE BIT(13)
#define IQS269_CHx_ENG_A_LOCAL_CAP_SIZE BIT(12)
@@ -175,6 +184,20 @@ enum iqs269_event_id {
IQS269_EVENT_DEEP_UP,
};
+enum iqs269_slider_id {
+ IQS269_SLIDER_NONE,
+ IQS269_SLIDER_KEY,
+ IQS269_SLIDER_RAW,
+};
+
+enum iqs269_gesture_id {
+ IQS269_GESTURE_TAP,
+ IQS269_GESTURE_HOLD,
+ IQS269_GESTURE_FLICK_POS,
+ IQS269_GESTURE_FLICK_NEG,
+ IQS269_NUM_GESTURES,
+};
+
struct iqs269_switch_desc {
unsigned int code;
bool enabled;
@@ -234,7 +257,7 @@ struct iqs269_ver_info {
u8 prod_num;
u8 sw_num;
u8 hw_num;
- u8 padding;
+ u8 fw_num;
} __packed;
struct iqs269_ch_reg {
@@ -285,16 +308,33 @@ struct iqs269_private {
struct regmap *regmap;
struct mutex lock;
struct iqs269_switch_desc switches[ARRAY_SIZE(iqs269_events)];
+ struct iqs269_ver_info ver_info;
struct iqs269_sys_reg sys_reg;
struct completion ati_done;
struct input_dev *keypad;
struct input_dev *slider[IQS269_NUM_SL];
unsigned int keycode[ARRAY_SIZE(iqs269_events) * IQS269_NUM_CH];
+ unsigned int sl_code[IQS269_NUM_SL][IQS269_NUM_GESTURES];
unsigned int ch_num;
bool hall_enable;
bool ati_current;
};
+static enum iqs269_slider_id iqs269_slider_type(struct iqs269_private *iqs269,
+ int slider_num)
+{
+ int i;
+
+ if (!iqs269->sys_reg.slider_select[slider_num])
+ return IQS269_SLIDER_NONE;
+
+ for (i = 0; i < IQS269_NUM_GESTURES; i++)
+ if (iqs269->sl_code[slider_num][i] != KEY_RESERVED)
+ return IQS269_SLIDER_KEY;
+
+ return IQS269_SLIDER_RAW;
+}
+
static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
unsigned int ch_num, unsigned int mode)
{
@@ -1004,6 +1044,76 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
general |= (val << IQS269_SYS_SETTINGS_ULP_UPDATE_SHIFT);
}
+ if (device_property_present(&client->dev, "linux,keycodes")) {
+ int scale = 1;
+ int count = device_property_count_u32(&client->dev,
+ "linux,keycodes");
+ if (count > IQS269_NUM_GESTURES * IQS269_NUM_SL) {
+ dev_err(&client->dev, "Too many keycodes present\n");
+ return -EINVAL;
+ } else if (count < 0) {
+ dev_err(&client->dev, "Failed to count keycodes: %d\n",
+ count);
+ return count;
+ }
+
+ error = device_property_read_u32_array(&client->dev,
+ "linux,keycodes",
+ *iqs269->sl_code, count);
+ if (error) {
+ dev_err(&client->dev, "Failed to read keycodes: %d\n",
+ error);
+ return error;
+ }
+
+ if (device_property_present(&client->dev,
+ "azoteq,gesture-swipe"))
+ general |= IQS269_SYS_SETTINGS_SLIDER_SWIPE;
+
+ /*
+ * Early revisions of silicon use a more granular step size for
+ * tap and swipe gesture timeouts; scale them appropriately.
+ */
+ if (iqs269->ver_info.fw_num < IQS269_VER_INFO_FW_NUM_3)
+ scale = 4;
+
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,timeout-tap-ms", &val)) {
+ if (val > IQS269_TIMEOUT_TAP_MS_MAX / scale) {
+ dev_err(&client->dev, "Invalid timeout: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->timeout_tap = val / (16 / scale);
+ }
+
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,timeout-swipe-ms",
+ &val)) {
+ if (val > IQS269_TIMEOUT_SWIPE_MS_MAX / scale) {
+ dev_err(&client->dev, "Invalid timeout: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->timeout_swipe = val / (16 / scale);
+ }
+
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,thresh-swipe", &val)) {
+ if (val > IQS269_THRESH_SWIPE_MAX) {
+ dev_err(&client->dev, "Invalid threshold: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->thresh_swipe = val;
+ }
+
+ sys_reg->event_mask &= ~IQS269_EVENT_MASK_GESTURE;
+ }
+
general &= ~IQS269_SYS_SETTINGS_RESEED_OFFSET;
if (device_property_present(&client->dev, "azoteq,reseed-offset"))
general |= IQS269_SYS_SETTINGS_RESEED_OFFSET;
@@ -1012,10 +1122,11 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
/*
* As per the datasheet, enable streaming during normal-power mode if
- * either slider is in use. In that case, the device returns to event
- * mode during low-power mode.
+ * raw coordinates will be read from either slider. In that case, the
+ * device returns to event mode during low-power mode.
*/
- if (sys_reg->slider_select[0] || sys_reg->slider_select[1])
+ if (iqs269_slider_type(iqs269, 0) == IQS269_SLIDER_RAW ||
+ iqs269_slider_type(iqs269, 1) == IQS269_SLIDER_RAW)
general |= IQS269_SYS_SETTINGS_EVENT_MODE_LP;
general |= IQS269_SYS_SETTINGS_REDO_ATI;
@@ -1106,19 +1217,37 @@ static int iqs269_input_init(struct iqs269_private *iqs269)
}
for (i = 0; i < IQS269_NUM_SL; i++) {
- if (!iqs269->sys_reg.slider_select[i])
+ if (iqs269_slider_type(iqs269, i) == IQS269_SLIDER_NONE)
continue;
iqs269->slider[i] = devm_input_allocate_device(&client->dev);
if (!iqs269->slider[i])
return -ENOMEM;
+ iqs269->slider[i]->keycodemax = ARRAY_SIZE(iqs269->sl_code[i]);
+ iqs269->slider[i]->keycode = iqs269->sl_code[i];
+ iqs269->slider[i]->keycodesize = sizeof(**iqs269->sl_code);
+
iqs269->slider[i]->name = i ? "iqs269a_slider_1"
: "iqs269a_slider_0";
iqs269->slider[i]->id.bustype = BUS_I2C;
- input_set_capability(iqs269->slider[i], EV_KEY, BTN_TOUCH);
- input_set_abs_params(iqs269->slider[i], ABS_X, 0, 255, 0, 0);
+ for (j = 0; j < IQS269_NUM_GESTURES; j++)
+ if (iqs269->sl_code[i][j] != KEY_RESERVED)
+ input_set_capability(iqs269->slider[i], EV_KEY,
+ iqs269->sl_code[i][j]);
+
+ /*
+ * Present the slider as a narrow trackpad if one or more chan-
+ * nels have been selected to participate, but no gestures have
+ * been mapped to a keycode.
+ */
+ if (iqs269_slider_type(iqs269, i) == IQS269_SLIDER_RAW) {
+ input_set_capability(iqs269->slider[i],
+ EV_KEY, BTN_TOUCH);
+ input_set_abs_params(iqs269->slider[i],
+ ABS_X, 0, 255, 0, 0);
+ }
error = input_register_device(iqs269->slider[i]);
if (error) {
@@ -1167,28 +1296,62 @@ static int iqs269_report(struct iqs269_private *iqs269)
if (be16_to_cpu(flags.system) & IQS269_SYS_FLAGS_IN_ATI)
return 0;
- error = regmap_raw_read(iqs269->regmap, IQS269_SLIDER_X, slider_x,
- sizeof(slider_x));
- if (error) {
- dev_err(&client->dev, "Failed to read slider position: %d\n",
- error);
- return error;
+ if (iqs269_slider_type(iqs269, 0) == IQS269_SLIDER_RAW ||
+ iqs269_slider_type(iqs269, 1) == IQS269_SLIDER_RAW) {
+ error = regmap_raw_read(iqs269->regmap, IQS269_SLIDER_X,
+ slider_x, sizeof(slider_x));
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to read slider position: %d\n", error);
+ return error;
+ }
}
for (i = 0; i < IQS269_NUM_SL; i++) {
- if (!iqs269->sys_reg.slider_select[i])
+ flags.gesture >>= (i * IQS269_NUM_GESTURES);
+
+ switch (iqs269_slider_type(iqs269, i)) {
+ case IQS269_SLIDER_NONE:
continue;
- /*
- * Report BTN_TOUCH if any channel that participates in the
- * slider is in a state of touch.
- */
- if (flags.states[IQS269_ST_OFFS_TOUCH] &
- iqs269->sys_reg.slider_select[i]) {
- input_report_key(iqs269->slider[i], BTN_TOUCH, 1);
- input_report_abs(iqs269->slider[i], ABS_X, slider_x[i]);
- } else {
- input_report_key(iqs269->slider[i], BTN_TOUCH, 0);
+ case IQS269_SLIDER_KEY:
+ for (j = 0; j < IQS269_NUM_GESTURES; j++)
+ input_report_key(iqs269->slider[i],
+ iqs269->sl_code[i][j],
+ flags.gesture & BIT(j));
+
+ if (!(flags.gesture & (BIT(IQS269_GESTURE_FLICK_NEG) |
+ BIT(IQS269_GESTURE_FLICK_POS) |
+ BIT(IQS269_GESTURE_TAP))))
+ break;
+
+ input_sync(iqs269->slider[i]);
+
+ /*
+ * Momentary gestures are followed by a complementary
+ * release cycle so as to emulate a full keystroke.
+ */
+ for (j = 0; j < IQS269_NUM_GESTURES; j++)
+ if (j != IQS269_GESTURE_HOLD)
+ input_report_key(iqs269->slider[i],
+ iqs269->sl_code[i][j],
+ 0);
+ break;
+
+ case IQS269_SLIDER_RAW:
+ /*
+ * The slider is considered to be in a state of touch
+ * if any selected channels are in a state of touch.
+ */
+ state = flags.states[IQS269_ST_OFFS_TOUCH];
+ state &= iqs269->sys_reg.slider_select[i];
+
+ input_report_key(iqs269->slider[i], BTN_TOUCH, state);
+
+ if (state)
+ input_report_abs(iqs269->slider[i],
+ ABS_X, slider_x[i]);
+ break;
}
input_sync(iqs269->slider[i]);
@@ -1595,7 +1758,6 @@ static const struct regmap_config iqs269_regmap_config = {
static int iqs269_probe(struct i2c_client *client)
{
- struct iqs269_ver_info ver_info;
struct iqs269_private *iqs269;
int error;
@@ -1617,14 +1779,14 @@ static int iqs269_probe(struct i2c_client *client)
mutex_init(&iqs269->lock);
init_completion(&iqs269->ati_done);
- error = regmap_raw_read(iqs269->regmap, IQS269_VER_INFO, &ver_info,
- sizeof(ver_info));
+ error = regmap_raw_read(iqs269->regmap, IQS269_VER_INFO,
+ &iqs269->ver_info, sizeof(iqs269->ver_info));
if (error)
return error;
- if (ver_info.prod_num != IQS269_VER_INFO_PROD_NUM) {
+ if (iqs269->ver_info.prod_num != IQS269_VER_INFO_PROD_NUM) {
dev_err(&client->dev, "Unrecognized product number: 0x%02X\n",
- ver_info.prod_num);
+ iqs269->ver_info.prod_num);
return -EINVAL;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v3 3/4] dt-bindings: input: iqs269a: Add bindings for OTP variants
From: Jeff LaBundy @ 2024-01-01 20:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-input, devicetree, jeff
In-Reply-To: <ZZMZzeX77VeHdIeL@nixie71>
This patch adds bindings for the D0 order code of the device. This
order code represents an OTP variant that enables a touch-and-hold
function in place of slider 1.
Also included is the ability to specify the 00 order code (default
option with no OTP customization) explicitly.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v3:
- None
Changes in v2:
- None
.../devicetree/bindings/input/iqs269a.yaml | 30 ++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/iqs269a.yaml b/Documentation/devicetree/bindings/input/iqs269a.yaml
index b42f07542d27..2c3f693b8982 100644
--- a/Documentation/devicetree/bindings/input/iqs269a.yaml
+++ b/Documentation/devicetree/bindings/input/iqs269a.yaml
@@ -20,7 +20,10 @@ description: |
properties:
compatible:
- const: azoteq,iqs269a
+ enum:
+ - azoteq,iqs269a
+ - azoteq,iqs269a-00
+ - azoteq,iqs269a-d0
reg:
maxItems: 1
@@ -207,6 +210,16 @@ properties:
default: 1
description: Specifies the slider coordinate filter strength.
+ azoteq,touch-hold-ms:
+ multipleOf: 256
+ minimum: 256
+ maximum: 65280
+ default: 5120
+ description:
+ Specifies the length of time (in ms) for which the channel selected by
+ 'azoteq,gpio3-select' must be held in a state of touch in order for an
+ approximately 60-ms pulse to be asserted on the GPIO4 pin.
+
linux,keycodes:
minItems: 1
maxItems: 8
@@ -514,6 +527,21 @@ patternProperties:
additionalProperties: false
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - azoteq,iqs269a-d0
+then:
+ patternProperties:
+ "^channel@[0-7]$":
+ properties:
+ azoteq,slider1-select: false
+else:
+ properties:
+ azoteq,touch-hold-ms: false
+
required:
- compatible
- reg
--
2.34.1
^ permalink raw reply related
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