* [PATCH v3 5/6] drm/vmwgfx: Use vmware_hypercall API
From: Alexey Makhalov @ 2023-12-19 21:57 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: <20231219215751.9445-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 v3 4/6] input/vmmouse: Use vmware_hypercall API
From: Alexey Makhalov @ 2023-12-19 21:57 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: <20231219215751.9445-1-alexey.makhalov@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
Switch from VMWARE_HYPERCALL macro to vmware_hypercall API.
Eliminate arch specific code. No functional changes intended.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
Reviewed-by: Zack Rusin <zackr@vmware.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/mouse/vmmouse.c | 76 ++++++++++-------------------------
1 file changed, 22 insertions(+), 54 deletions(-)
diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c
index ea9eff7c8099..fb1d986a6895 100644
--- a/drivers/input/mouse/vmmouse.c
+++ b/drivers/input/mouse/vmmouse.c
@@ -21,19 +21,16 @@
#include "psmouse.h"
#include "vmmouse.h"
-#define VMMOUSE_PROTO_MAGIC 0x564D5868U
-
/*
* Main commands supported by the vmmouse hypervisor port.
*/
-#define VMMOUSE_PROTO_CMD_GETVERSION 10
-#define VMMOUSE_PROTO_CMD_ABSPOINTER_DATA 39
-#define VMMOUSE_PROTO_CMD_ABSPOINTER_STATUS 40
-#define VMMOUSE_PROTO_CMD_ABSPOINTER_COMMAND 41
-#define VMMOUSE_PROTO_CMD_ABSPOINTER_RESTRICT 86
+#define VMWARE_CMD_ABSPOINTER_DATA 39
+#define VMWARE_CMD_ABSPOINTER_STATUS 40
+#define VMWARE_CMD_ABSPOINTER_COMMAND 41
+#define VMWARE_CMD_ABSPOINTER_RESTRICT 86
/*
- * Subcommands for VMMOUSE_PROTO_CMD_ABSPOINTER_COMMAND
+ * Subcommands for VMWARE_CMD_ABSPOINTER_COMMAND
*/
#define VMMOUSE_CMD_ENABLE 0x45414552U
#define VMMOUSE_CMD_DISABLE 0x000000f5U
@@ -76,28 +73,6 @@ struct vmmouse_data {
char dev_name[128];
};
-/*
- * Hypervisor-specific bi-directional communication channel
- * implementing the vmmouse protocol. Should never execute on
- * bare metal hardware.
- */
-#define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4) \
-({ \
- unsigned long __dummy1, __dummy2; \
- __asm__ __volatile__ (VMWARE_HYPERCALL : \
- "=a"(out1), \
- "=b"(out2), \
- "=c"(out3), \
- "=d"(out4), \
- "=S"(__dummy1), \
- "=D"(__dummy2) : \
- "a"(VMMOUSE_PROTO_MAGIC), \
- "b"(in1), \
- "c"(VMMOUSE_PROTO_CMD_##cmd), \
- "d"(0) : \
- "memory"); \
-})
-
/**
* vmmouse_report_button - report button state on the correct input device
*
@@ -145,14 +120,12 @@ static psmouse_ret_t vmmouse_report_events(struct psmouse *psmouse)
struct input_dev *abs_dev = priv->abs_dev;
struct input_dev *pref_dev;
u32 status, x, y, z;
- u32 dummy1, dummy2, dummy3;
unsigned int queue_length;
unsigned int count = 255;
while (count--) {
/* See if we have motion data. */
- VMMOUSE_CMD(ABSPOINTER_STATUS, 0,
- status, dummy1, dummy2, dummy3);
+ status = vmware_hypercall1(VMWARE_CMD_ABSPOINTER_STATUS, 0);
if ((status & VMMOUSE_ERROR) == VMMOUSE_ERROR) {
psmouse_err(psmouse, "failed to fetch status data\n");
/*
@@ -172,7 +145,8 @@ static psmouse_ret_t vmmouse_report_events(struct psmouse *psmouse)
}
/* Now get it */
- VMMOUSE_CMD(ABSPOINTER_DATA, 4, status, x, y, z);
+ status = vmware_hypercall4(VMWARE_CMD_ABSPOINTER_DATA, 4,
+ &x, &y, &z);
/*
* And report what we've got. Prefer to report button
@@ -247,14 +221,10 @@ static psmouse_ret_t vmmouse_process_byte(struct psmouse *psmouse)
static void vmmouse_disable(struct psmouse *psmouse)
{
u32 status;
- u32 dummy1, dummy2, dummy3, dummy4;
-
- VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_DISABLE,
- dummy1, dummy2, dummy3, dummy4);
- VMMOUSE_CMD(ABSPOINTER_STATUS, 0,
- status, dummy1, dummy2, dummy3);
+ vmware_hypercall1(VMWARE_CMD_ABSPOINTER_COMMAND, VMMOUSE_CMD_DISABLE);
+ status = vmware_hypercall1(VMWARE_CMD_ABSPOINTER_STATUS, 0);
if ((status & VMMOUSE_ERROR) != VMMOUSE_ERROR)
psmouse_warn(psmouse, "failed to disable vmmouse device\n");
}
@@ -271,26 +241,24 @@ static void vmmouse_disable(struct psmouse *psmouse)
static int vmmouse_enable(struct psmouse *psmouse)
{
u32 status, version;
- u32 dummy1, dummy2, dummy3, dummy4;
/*
* Try enabling the device. If successful, we should be able to
* read valid version ID back from it.
*/
- VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_ENABLE,
- dummy1, dummy2, dummy3, dummy4);
+ vmware_hypercall1(VMWARE_CMD_ABSPOINTER_COMMAND, VMMOUSE_CMD_ENABLE);
/*
* See if version ID can be retrieved.
*/
- VMMOUSE_CMD(ABSPOINTER_STATUS, 0, status, dummy1, dummy2, dummy3);
+ status = vmware_hypercall1(VMWARE_CMD_ABSPOINTER_STATUS, 0);
if ((status & 0x0000ffff) == 0) {
psmouse_dbg(psmouse, "empty flags - assuming no device\n");
return -ENXIO;
}
- VMMOUSE_CMD(ABSPOINTER_DATA, 1 /* single item */,
- version, dummy1, dummy2, dummy3);
+ version = vmware_hypercall1(VMWARE_CMD_ABSPOINTER_DATA,
+ 1 /* single item */);
if (version != VMMOUSE_VERSION_ID) {
psmouse_dbg(psmouse, "Unexpected version value: %u vs %u\n",
(unsigned) version, VMMOUSE_VERSION_ID);
@@ -301,11 +269,11 @@ static int vmmouse_enable(struct psmouse *psmouse)
/*
* Restrict ioport access, if possible.
*/
- VMMOUSE_CMD(ABSPOINTER_RESTRICT, VMMOUSE_RESTRICT_CPL0,
- dummy1, dummy2, dummy3, dummy4);
+ vmware_hypercall1(VMWARE_CMD_ABSPOINTER_RESTRICT,
+ VMMOUSE_RESTRICT_CPL0);
- VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_REQUEST_ABSOLUTE,
- dummy1, dummy2, dummy3, dummy4);
+ vmware_hypercall1(VMWARE_CMD_ABSPOINTER_COMMAND,
+ VMMOUSE_CMD_REQUEST_ABSOLUTE);
return 0;
}
@@ -342,7 +310,7 @@ static bool vmmouse_check_hypervisor(void)
*/
int vmmouse_detect(struct psmouse *psmouse, bool set_properties)
{
- u32 response, version, dummy1, dummy2;
+ u32 response, version, type;
if (!vmmouse_check_hypervisor()) {
psmouse_dbg(psmouse,
@@ -351,9 +319,9 @@ int vmmouse_detect(struct psmouse *psmouse, bool set_properties)
}
/* Check if the device is present */
- response = ~VMMOUSE_PROTO_MAGIC;
- VMMOUSE_CMD(GETVERSION, 0, version, response, dummy1, dummy2);
- if (response != VMMOUSE_PROTO_MAGIC || version == 0xffffffffU)
+ response = ~VMWARE_HYPERVISOR_MAGIC;
+ version = vmware_hypercall3(VMWARE_CMD_GETVERSION, 0, &response, &type);
+ if (response != VMWARE_HYPERVISOR_MAGIC || version == 0xffffffffU)
return -ENXIO;
if (set_properties) {
--
2.39.0
^ permalink raw reply related
* [PATCH v3 3/6] ptp/vmware: Use vmware_hypercall API
From: Alexey Makhalov @ 2023-12-19 21:57 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: <20231219215751.9445-1-alexey.makhalov@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
Switch from VMWARE_HYPERCALL macro to vmware_hypercall API.
Eliminate arch specific code. No functional changes intended.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
Reviewed-by: Jeff Sipek <jsipek@vmware.com>
---
drivers/ptp/ptp_vmw.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/ptp/ptp_vmw.c b/drivers/ptp/ptp_vmw.c
index 27c5547aa8a9..e5bb521b9b82 100644
--- a/drivers/ptp/ptp_vmw.c
+++ b/drivers/ptp/ptp_vmw.c
@@ -14,7 +14,6 @@
#include <asm/hypervisor.h>
#include <asm/vmware.h>
-#define VMWARE_MAGIC 0x564D5868
#define VMWARE_CMD_PCLK(nr) ((nr << 16) | 97)
#define VMWARE_CMD_PCLK_GETTIME VMWARE_CMD_PCLK(0)
@@ -24,15 +23,10 @@ static struct ptp_clock *ptp_vmw_clock;
static int ptp_vmw_pclk_read(u64 *ns)
{
- u32 ret, nsec_hi, nsec_lo, unused1, unused2, unused3;
-
- asm volatile (VMWARE_HYPERCALL :
- "=a"(ret), "=b"(nsec_hi), "=c"(nsec_lo), "=d"(unused1),
- "=S"(unused2), "=D"(unused3) :
- "a"(VMWARE_MAGIC), "b"(0),
- "c"(VMWARE_CMD_PCLK_GETTIME), "d"(0) :
- "memory");
+ u32 ret, nsec_hi, nsec_lo;
+ ret = vmware_hypercall3(VMWARE_CMD_PCLK_GETTIME, 0,
+ &nsec_hi, &nsec_lo);
if (ret == 0)
*ns = ((u64)nsec_hi << 32) | nsec_lo;
return ret;
--
2.39.0
^ permalink raw reply related
* [PATCH v3 2/6] x86/vmware: Introduce vmware_hypercall API
From: Alexey Makhalov @ 2023-12-19 21:57 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: <20231219215751.9445-1-alexey.makhalov@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
Introduce vmware_hypercall family of functions. It is a common
implementation to be used by the VMware guest code and virtual
device drivers in architecture independent manner.
The API consists of vmware_hypercallX and vmware_hypercall_hb_{out,in}
set of functions by analogy with KVM hypercall API. Architecture
specific implementation is hidden inside.
It will simplify future enhancements in VMware hypercalls such
as SEV-ES and TDX related changes without needs to modify a
caller in device drivers code.
Current implementation extends an idea from commit bac7b4e84323
("x86/vmware: Update platform detection code for VMCALL/VMMCALL
hypercalls") to have a slow, but safe path in VMWARE_HYPERCALL
earlier during the boot when alternatives are not yet applied.
This logic was inherited from VMWARE_CMD from the commit mentioned
above. Default alternative code was optimized by size to reduce
excessive nop alignment once alternatives are applied. Total
default code size is 26 bytes, in worse case (3 bytes alternative)
remaining 23 bytes will be aligned by only 3 long NOP instructions.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
Reviewed-by: Jeff Sipek <jsipek@vmware.com>
---
arch/x86/include/asm/vmware.h | 262 ++++++++++++++++++++++++++--------
arch/x86/kernel/cpu/vmware.c | 35 ++---
2 files changed, 220 insertions(+), 77 deletions(-)
diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
index 3636faa8b4fe..719e41260ece 100644
--- a/arch/x86/include/asm/vmware.h
+++ b/arch/x86/include/asm/vmware.h
@@ -40,69 +40,219 @@
extern u8 vmware_hypercall_mode;
-/* The low bandwidth call. The low word of edx is presumed clear. */
-#define VMWARE_HYPERCALL \
- ALTERNATIVE_2("movw $" __stringify(VMWARE_HYPERVISOR_PORT) ", %%dx; " \
- "inl (%%dx), %%eax", \
- "vmcall", X86_FEATURE_VMCALL, \
- "vmmcall", X86_FEATURE_VMW_VMMCALL)
-
/*
- * The high bandwidth out call. The low word of edx is presumed to have the
- * HB and OUT bits set.
+ * 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.
*/
-#define VMWARE_HYPERCALL_HB_OUT \
- ALTERNATIVE_2("movw $" __stringify(VMWARE_HYPERVISOR_PORT_HB) ", %%dx; " \
- "rep outsb", \
+#define VMWARE_HYPERCALL \
+ ALTERNATIVE_3("cmpb $" \
+ __stringify(CPUID_VMWARE_FEATURES_ECX_VMMCALL) \
+ ", %[mode]\n\t" \
+ "jg 2f\n\t" \
+ "je 1f\n\t" \
+ "movw %[port], %%dx\n\t" \
+ "inl (%%dx), %%eax\n\t" \
+ "jmp 3f\n\t" \
+ "1: vmmcall\n\t" \
+ "jmp 3f\n\t" \
+ "2: vmcall\n\t" \
+ "3:\n\t", \
+ "movw %[port], %%dx\n\t" \
+ "inl (%%dx), %%eax", X86_FEATURE_HYPERVISOR, \
"vmcall", X86_FEATURE_VMCALL, \
"vmmcall", X86_FEATURE_VMW_VMMCALL)
+static inline
+unsigned long vmware_hypercall1(unsigned long cmd, unsigned long in1)
+{
+ unsigned long out0;
+
+ asm_inline volatile (VMWARE_HYPERCALL
+ : "=a" (out0)
+ : [port] "i" (VMWARE_HYPERVISOR_PORT),
+ [mode] "m" (vmware_hypercall_mode),
+ "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (in1),
+ "c" (cmd),
+ "d" (0)
+ : "cc", "memory");
+ return out0;
+}
+
+static inline
+unsigned long vmware_hypercall3(unsigned long cmd, unsigned long in1,
+ uint32_t *out1, uint32_t *out2)
+{
+ unsigned long out0;
+
+ asm_inline volatile (VMWARE_HYPERCALL
+ : "=a" (out0), "=b" (*out1), "=c" (*out2)
+ : [port] "i" (VMWARE_HYPERVISOR_PORT),
+ [mode] "m" (vmware_hypercall_mode),
+ "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (in1),
+ "c" (cmd),
+ "d" (0)
+ : "cc", "memory");
+ return out0;
+}
+
+static inline
+unsigned long vmware_hypercall4(unsigned long cmd, unsigned long in1,
+ uint32_t *out1, uint32_t *out2,
+ uint32_t *out3)
+{
+ unsigned long out0;
+
+ asm_inline volatile (VMWARE_HYPERCALL
+ : "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
+ : [port] "i" (VMWARE_HYPERVISOR_PORT),
+ [mode] "m" (vmware_hypercall_mode),
+ "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (in1),
+ "c" (cmd),
+ "d" (0)
+ : "cc", "memory");
+ return out0;
+}
+
+static inline
+unsigned long vmware_hypercall5(unsigned long cmd, unsigned long in1,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, uint32_t *out2)
+{
+ unsigned long out0;
+
+ asm_inline volatile (VMWARE_HYPERCALL
+ : "=a" (out0), "=c" (*out2)
+ : [port] "i" (VMWARE_HYPERVISOR_PORT),
+ [mode] "m" (vmware_hypercall_mode),
+ "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (in1),
+ "c" (cmd),
+ "d" (in3),
+ "S" (in4),
+ "D" (in5)
+ : "cc", "memory");
+ return out0;
+}
+
+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)
+{
+ unsigned long out0;
+
+ asm_inline volatile (VMWARE_HYPERCALL
+ : "=a" (out0), "=c" (*out2), "=d" (*out3), "=S" (*out4),
+ "=D" (*out5)
+ : [port] "i" (VMWARE_HYPERVISOR_PORT),
+ [mode] "m" (vmware_hypercall_mode),
+ "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (in1),
+ "c" (cmd),
+ "d" (in3)
+ : "cc", "memory");
+ return out0;
+}
+
+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)
+{
+ unsigned long out0;
+
+ asm_inline volatile (VMWARE_HYPERCALL
+ : "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
+ : [port] "i" (VMWARE_HYPERVISOR_PORT),
+ [mode] "m" (vmware_hypercall_mode),
+ "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (in1),
+ "c" (cmd),
+ "d" (in3),
+ "S" (in4),
+ "D" (in5)
+ : "cc", "memory");
+ return out0;
+}
+
+
+#ifdef CONFIG_X86_64
+#define VMW_BP_REG "%%rbp"
+#define VMW_BP_CONSTRAINT "r"
+#else
+#define VMW_BP_REG "%%ebp"
+#define VMW_BP_CONSTRAINT "m"
+#endif
+
/*
- * The high bandwidth in call. The low word of edx is presumed to have the
- * HB bit set.
+ * High bandwidth calls are not supported on encrypted memory guests.
+ * The caller should check cc_platform_has(CC_ATTR_MEM_ENCRYPT) and use
+ * low bandwidth hypercall it memory encryption is set.
+ * This assumption simplifies HB hypercall impementation to just I/O port
+ * based approach without alternative patching.
*/
-#define VMWARE_HYPERCALL_HB_IN \
- ALTERNATIVE_2("movw $" __stringify(VMWARE_HYPERVISOR_PORT_HB) ", %%dx; " \
- "rep insb", \
- "vmcall", X86_FEATURE_VMCALL, \
- "vmmcall", X86_FEATURE_VMW_VMMCALL)
+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)
+{
+ unsigned long out0;
+
+ asm_inline volatile (
+ UNWIND_HINT_SAVE
+ "push " VMW_BP_REG "\n\t"
+ UNWIND_HINT_UNDEFINED
+ "mov %[in6], " VMW_BP_REG "\n\t"
+ "rep outsb\n\t"
+ "pop " VMW_BP_REG "\n\t"
+ UNWIND_HINT_RESTORE
+ : "=a" (out0), "=b" (*out1)
+ : "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (cmd),
+ "c" (in2),
+ "d" (in3 | VMWARE_HYPERVISOR_PORT_HB),
+ "S" (in4),
+ "D" (in5),
+ [in6] VMW_BP_CONSTRAINT (in6)
+ : "cc", "memory");
+ return out0;
+}
+
+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)
+{
+ unsigned long out0;
-#define VMWARE_PORT(cmd, eax, ebx, ecx, edx) \
- __asm__("inl (%%dx), %%eax" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_VMCALL(cmd, eax, ebx, ecx, edx) \
- __asm__("vmcall" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(0), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx) \
- __asm__("vmmcall" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(0), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_CMD(cmd, eax, ebx, ecx, edx) do { \
- switch (vmware_hypercall_mode) { \
- case CPUID_VMWARE_FEATURES_ECX_VMCALL: \
- VMWARE_VMCALL(cmd, eax, ebx, ecx, edx); \
- break; \
- case CPUID_VMWARE_FEATURES_ECX_VMMCALL: \
- VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx); \
- break; \
- default: \
- VMWARE_PORT(cmd, eax, ebx, ecx, edx); \
- break; \
- } \
- } while (0)
+ asm_inline volatile (
+ UNWIND_HINT_SAVE
+ "push " VMW_BP_REG "\n\t"
+ UNWIND_HINT_UNDEFINED
+ "mov %[in6], " VMW_BP_REG "\n\t"
+ "rep insb\n\t"
+ "pop " VMW_BP_REG "\n\t"
+ UNWIND_HINT_RESTORE
+ : "=a" (out0), "=b" (*out1)
+ : "a" (VMWARE_HYPERVISOR_MAGIC),
+ "b" (cmd),
+ "c" (in2),
+ "d" (in3 | VMWARE_HYPERVISOR_PORT_HB),
+ "S" (in4),
+ "D" (in5),
+ [in6] VMW_BP_CONSTRAINT (in6)
+ : "cc", "memory");
+ return out0;
+}
+#undef VMW_BP_REG
+#undef VMW_BP_CONSTRAINT
+#undef VMWARE_HYPERCALL
#endif
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 4db8e1daa4a1..3aa1adaed18f 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -67,9 +67,10 @@ EXPORT_SYMBOL_GPL(vmware_hypercall_mode);
static inline int __vmware_platform(void)
{
- uint32_t eax, ebx, ecx, edx;
- VMWARE_CMD(GETVERSION, eax, ebx, ecx, edx);
- return eax != (uint32_t)-1 && ebx == VMWARE_HYPERVISOR_MAGIC;
+ uint32_t eax, ebx, ecx;
+
+ eax = vmware_hypercall3(VMWARE_CMD_GETVERSION, 0, &ebx, &ecx);
+ return eax != UINT_MAX && ebx == VMWARE_HYPERVISOR_MAGIC;
}
static unsigned long vmware_get_tsc_khz(void)
@@ -121,21 +122,12 @@ static void __init vmware_cyc2ns_setup(void)
pr_info("using clock offset of %llu ns\n", d->cyc2ns_offset);
}
-static int vmware_cmd_stealclock(uint32_t arg1, uint32_t arg2)
+static int vmware_cmd_stealclock(uint32_t addr_hi, uint32_t addr_lo)
{
- uint32_t result, info;
-
- asm volatile (VMWARE_HYPERCALL :
- "=a"(result),
- "=c"(info) :
- "a"(VMWARE_HYPERVISOR_MAGIC),
- "b"(0),
- "c"(VMWARE_CMD_STEALCLOCK),
- "d"(0),
- "S"(arg1),
- "D"(arg2) :
- "memory");
- return result;
+ uint32_t info;
+
+ return vmware_hypercall5(VMWARE_CMD_STEALCLOCK, 0, 0, addr_hi, addr_lo,
+ &info);
}
static bool stealclock_enable(phys_addr_t pa)
@@ -344,10 +336,10 @@ static void __init vmware_set_capabilities(void)
static void __init vmware_platform_setup(void)
{
- uint32_t eax, ebx, ecx, edx;
+ uint32_t eax, ebx, ecx;
uint64_t lpj, tsc_khz;
- VMWARE_CMD(GETHZ, eax, ebx, ecx, edx);
+ eax = vmware_hypercall3(VMWARE_CMD_GETHZ, UINT_MAX, &ebx, &ecx);
if (ebx != UINT_MAX) {
lpj = tsc_khz = eax | (((uint64_t)ebx) << 32);
@@ -429,8 +421,9 @@ static uint32_t __init vmware_platform(void)
/* Checks if hypervisor supports x2apic without VT-D interrupt remapping. */
static bool __init vmware_legacy_x2apic_available(void)
{
- uint32_t eax, ebx, ecx, edx;
- VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
+ uint32_t eax;
+
+ eax = vmware_hypercall1(VMWARE_CMD_GETVCPU_INFO, 0);
return !(eax & BIT(VCPU_RESERVED)) &&
(eax & BIT(VCPU_LEGACY_X2APIC));
}
--
2.39.0
^ permalink raw reply related
* [PATCH v3 1/6] x86/vmware: Move common macros to vmware.h
From: Alexey Makhalov @ 2023-12-19 21:57 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: <20231219215751.9445-1-alexey.makhalov@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
Move VMware hypercall macros to vmware.h. This is a prerequisite for
the introduction of vmware_hypercall API. No functional changes besides
exporting vmware_hypercall_mode symbol.
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
---
arch/x86/include/asm/vmware.h | 69 ++++++++++++++++++++++++++++++-----
arch/x86/kernel/cpu/vmware.c | 57 +++--------------------------
2 files changed, 66 insertions(+), 60 deletions(-)
diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
index ac9fc51e2b18..3636faa8b4fe 100644
--- a/arch/x86/include/asm/vmware.h
+++ b/arch/x86/include/asm/vmware.h
@@ -8,25 +8,37 @@
/*
* The hypercall definitions differ in the low word of the %edx argument
- * in the following way: the old port base interface uses the port
- * number to distinguish between high- and low bandwidth versions.
+ * in the following way: the old I/O port based interface uses the port
+ * number to distinguish between high- and low bandwidth versions, and
+ * uses IN/OUT instructions to define transfer direction.
*
* The new vmcall interface instead uses a set of flags to select
* bandwidth mode and transfer direction. The flags should be loaded
* into %dx by any user and are automatically replaced by the port
- * number if the VMWARE_HYPERVISOR_PORT method is used.
+ * number if the I/O port method is used.
*
* In short, new driver code should strictly use the new definition of
* %dx content.
*/
-/* Old port-based version */
-#define VMWARE_HYPERVISOR_PORT 0x5658
-#define VMWARE_HYPERVISOR_PORT_HB 0x5659
+#define VMWARE_HYPERVISOR_HB BIT(0)
+#define VMWARE_HYPERVISOR_OUT BIT(1)
-/* Current vmcall / vmmcall version */
-#define VMWARE_HYPERVISOR_HB BIT(0)
-#define VMWARE_HYPERVISOR_OUT BIT(1)
+#define VMWARE_HYPERVISOR_PORT 0x5658
+#define VMWARE_HYPERVISOR_PORT_HB (VMWARE_HYPERVISOR_PORT | \
+ VMWARE_HYPERVISOR_HB)
+
+#define VMWARE_HYPERVISOR_MAGIC 0x564d5868U
+
+#define VMWARE_CMD_GETVERSION 10
+#define VMWARE_CMD_GETHZ 45
+#define VMWARE_CMD_GETVCPU_INFO 68
+#define VMWARE_CMD_STEALCLOCK 91
+
+#define CPUID_VMWARE_FEATURES_ECX_VMMCALL BIT(0)
+#define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1)
+
+extern u8 vmware_hypercall_mode;
/* The low bandwidth call. The low word of edx is presumed clear. */
#define VMWARE_HYPERCALL \
@@ -54,4 +66,43 @@
"rep insb", \
"vmcall", X86_FEATURE_VMCALL, \
"vmmcall", X86_FEATURE_VMW_VMMCALL)
+
+#define VMWARE_PORT(cmd, eax, ebx, ecx, edx) \
+ __asm__("inl (%%dx), %%eax" : \
+ "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
+ "a"(VMWARE_HYPERVISOR_MAGIC), \
+ "c"(VMWARE_CMD_##cmd), \
+ "d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) : \
+ "memory")
+
+#define VMWARE_VMCALL(cmd, eax, ebx, ecx, edx) \
+ __asm__("vmcall" : \
+ "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
+ "a"(VMWARE_HYPERVISOR_MAGIC), \
+ "c"(VMWARE_CMD_##cmd), \
+ "d"(0), "b"(UINT_MAX) : \
+ "memory")
+
+#define VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx) \
+ __asm__("vmmcall" : \
+ "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
+ "a"(VMWARE_HYPERVISOR_MAGIC), \
+ "c"(VMWARE_CMD_##cmd), \
+ "d"(0), "b"(UINT_MAX) : \
+ "memory")
+
+#define VMWARE_CMD(cmd, eax, ebx, ecx, edx) do { \
+ switch (vmware_hypercall_mode) { \
+ case CPUID_VMWARE_FEATURES_ECX_VMCALL: \
+ VMWARE_VMCALL(cmd, eax, ebx, ecx, edx); \
+ break; \
+ case CPUID_VMWARE_FEATURES_ECX_VMMCALL: \
+ VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx); \
+ break; \
+ default: \
+ VMWARE_PORT(cmd, eax, ebx, ecx, edx); \
+ break; \
+ } \
+ } while (0)
+
#endif
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 11f83d07925e..4db8e1daa4a1 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -41,60 +41,14 @@
#define CPUID_VMWARE_INFO_LEAF 0x40000000
#define CPUID_VMWARE_FEATURES_LEAF 0x40000010
-#define CPUID_VMWARE_FEATURES_ECX_VMMCALL BIT(0)
-#define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1)
-#define VMWARE_HYPERVISOR_MAGIC 0x564D5868
-
-#define VMWARE_CMD_GETVERSION 10
-#define VMWARE_CMD_GETHZ 45
-#define VMWARE_CMD_GETVCPU_INFO 68
-#define VMWARE_CMD_LEGACY_X2APIC 3
-#define VMWARE_CMD_VCPU_RESERVED 31
-#define VMWARE_CMD_STEALCLOCK 91
+#define VCPU_LEGACY_X2APIC 3
+#define VCPU_RESERVED 31
#define STEALCLOCK_NOT_AVAILABLE (-1)
#define STEALCLOCK_DISABLED 0
#define STEALCLOCK_ENABLED 1
-#define VMWARE_PORT(cmd, eax, ebx, ecx, edx) \
- __asm__("inl (%%dx), %%eax" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_VMCALL(cmd, eax, ebx, ecx, edx) \
- __asm__("vmcall" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(0), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx) \
- __asm__("vmmcall" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(0), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_CMD(cmd, eax, ebx, ecx, edx) do { \
- switch (vmware_hypercall_mode) { \
- case CPUID_VMWARE_FEATURES_ECX_VMCALL: \
- VMWARE_VMCALL(cmd, eax, ebx, ecx, edx); \
- break; \
- case CPUID_VMWARE_FEATURES_ECX_VMMCALL: \
- VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx); \
- break; \
- default: \
- VMWARE_PORT(cmd, eax, ebx, ecx, edx); \
- break; \
- } \
- } while (0)
-
struct vmware_steal_time {
union {
uint64_t clock; /* stolen time counter in units of vtsc */
@@ -108,7 +62,8 @@ struct vmware_steal_time {
};
static unsigned long vmware_tsc_khz __ro_after_init;
-static u8 vmware_hypercall_mode __ro_after_init;
+u8 vmware_hypercall_mode __ro_after_init;
+EXPORT_SYMBOL_GPL(vmware_hypercall_mode);
static inline int __vmware_platform(void)
{
@@ -476,8 +431,8 @@ static bool __init vmware_legacy_x2apic_available(void)
{
uint32_t eax, ebx, ecx, edx;
VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
- return !(eax & BIT(VMWARE_CMD_VCPU_RESERVED)) &&
- (eax & BIT(VMWARE_CMD_LEGACY_X2APIC));
+ return !(eax & BIT(VCPU_RESERVED)) &&
+ (eax & BIT(VCPU_LEGACY_X2APIC));
}
#ifdef CONFIG_AMD_MEM_ENCRYPT
--
2.39.0
^ permalink raw reply related
* [PATCH v3 0/6] VMware hypercalls enhancements
From: Alexey Makhalov @ 2023-12-19 21:57 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
VMware hypercalls invocations were all spread out across the kernel
implementing same ABI as in-place asm-inline. With encrypted memory
and confidential computing it became harder to maintain every changes
in these hypercall implementations.
Intention of this patchset is to introduce arch independent VMware
hypercall API layer other subsystems such as device drivers can call
to, while hiding architecture specific implementation behind.
Second patch introduces the vmware_hypercall low and high bandwidth
families of functions, with little enhancements there.
Sixth patch adds tdx hypercall support
arm64 implementation of vmware_hypercalls is in drivers/gpu/drm/
vmwgfx/vmwgfx_msg_arm64.h and going to be moved to arch/arm64 with
a separate patchset with the introduction of VMware Linux guest
support for arm64.
No functional changes in drivers/input/mouse/vmmouse.c and
drivers/ptp/ptp_vmw.c
v2->v3 changes: (no functional changes in patches 1-5)
- Improved commit message in patches 1, 2 and 5 as was suggested by
Borislav Petkov.
- To address Dave Hansen's concern, patch 6 was reorganized to avoid
exporting bare __tdx_hypercall and to make exported vmware_tdx_hypercall
VMWare guest specific.
v1->v2 changes (no functional changes):
- Improved commit message in patches 2 and 5.
- Added Reviewed-by for all patches.
- Added Ack from Dmitry Torokhov in patch 4. No fixes regarding reported
by Simon Horman gcc error in this patch.
Alexey Makhalov (6):
x86/vmware: Move common macros to vmware.h
x86/vmware: Introduce vmware_hypercall API
ptp/vmware: Use vmware_hypercall API
input/vmmouse: Use vmware_hypercall API
drm/vmwgfx: Use vmware_hypercall API
x86/vmware: Add TDX hypercall support
arch/x86/include/asm/vmware.h | 338 ++++++++++++++++++++--
arch/x86/kernel/cpu/vmware.c | 116 +++-----
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 ------------
drivers/input/mouse/vmmouse.c | 76 ++---
drivers/ptp/ptp_vmw.c | 12 +-
7 files changed, 577 insertions(+), 520 deletions(-)
--
2.39.0
^ permalink raw reply
* [PATCH 2/2] HID: wacom: Add additional tests of confidence behavior
From: Gerecke, Jason @ 2023-12-19 21:33 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina
Cc: Aaron Skomra, Jason Gerecke, Joshua Dickens, Ping Cheng,
Tatsunosuke Tobita, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Ping Cheng
In-Reply-To: <20231219213344.38434-1-jason.gerecke@wacom.com>
From: Jason Gerecke <jason.gerecke@wacom.com>
Test for proper driver behavior when the touch confidence bit is set
or cleared. Test the three flavors of touch confidence loss (tipswitch
cleared before confidence, tipswitch and confidence cleared at the same
time, and tipswitch only cleared when touch is actually removed). Also
test two flavors of touch confidence gain (confidence added to a touch
that was "never" confident, and confidence added to a touch that was
previously confident).
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
.../selftests/hid/tests/test_wacom_generic.py | 278 +++++++++++++++++-
1 file changed, 277 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/hid/tests/test_wacom_generic.py b/tools/testing/selftests/hid/tests/test_wacom_generic.py
index f92fe8e02c1b..a7ee54e5090b 100644
--- a/tools/testing/selftests/hid/tests/test_wacom_generic.py
+++ b/tools/testing/selftests/hid/tests/test_wacom_generic.py
@@ -27,6 +27,7 @@ from .descriptors_wacom import (
)
import attr
+from collections import namedtuple
from enum import Enum
from hidtools.hut import HUT
from hidtools.hid import HidUnit
@@ -862,6 +863,8 @@ class TestPTHX60_Pen(TestOpaqueCTLTablet):
class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest):
+ ContactIds = namedtuple("ContactIds", "contact_id, tracking_id, slot_num")
+
def create_device(self):
return test_multitouch.Digitizer(
"DTH 2452",
@@ -869,6 +872,57 @@ class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest
input_info=(0x3, 0x056A, 0x0383),
)
+ def make_contact(self, contact_id=0, t=0):
+ """
+ Make a single touch contact that can move over time.
+
+ Creates a touch object that has a well-known position in space that
+ does not overlap with other contacts. The value of `t` may be
+ incremented over time to move the point along a linear path.
+ """
+ x = 50 + 10 * contact_id + t
+ y = 100 + 100 * contact_id + t
+ return test_multitouch.Touch(contact_id, x, y)
+
+ def make_contacts(self, n, t=0):
+ """
+ Make multiple touch contacts that can move over time.
+
+ Returns a list of `n` touch objects that are positioned at well-known
+ locations. The value of `t` may be incremented over time to move the
+ points along a linear path.
+ """
+ return [ self.make_contact(id, t) for id in range(0, n) ]
+
+ def assert_contact(self, uhdev, evdev, contact_ids, t=0):
+ """
+ Assert properties of a contact generated by make_contact.
+ """
+ contact_id = contact_ids.contact_id
+ tracking_id = contact_ids.tracking_id
+ slot_num = contact_ids.slot_num
+
+ x = 50 + 10 * contact_id + t
+ y = 100 + 100 * contact_id + t
+
+ # If the data isn't supposed to be stored in any slots, there is
+ # nothing we can check for in the evdev stream.
+ if slot_num is None:
+ assert tracking_id == -1
+ return
+
+ assert evdev.slots[slot_num][libevdev.EV_ABS.ABS_MT_TRACKING_ID] == tracking_id
+ if tracking_id != -1:
+ assert evdev.slots[slot_num][libevdev.EV_ABS.ABS_MT_POSITION_X] == x
+ assert evdev.slots[slot_num][libevdev.EV_ABS.ABS_MT_POSITION_Y] == y
+
+ def assert_contacts(self, uhdev, evdev, data, t=0):
+ """
+ Assert properties of a list of contacts generated by make_contacts.
+ """
+ for contact_ids in data:
+ self.assert_contact(uhdev, evdev, contact_ids, t)
+
def test_contact_id_0(self):
"""
Bring a finger in contact with the tablet, then hold it down and remove it.
@@ -919,4 +973,226 @@ class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest
slot = self.get_slot(uhdev, t0, 0)
- assert not events
\ No newline at end of file
+ assert not events
+
+ def test_confidence_multitouch(self):
+ """
+ Bring multiple fingers in contact with the tablet, some with the
+ confidence bit set, and some without.
+
+ Ensure that all confident touches are reported and that all non-
+ confident touches are ignored.
+ """
+ uhdev = self.uhdev
+ evdev = uhdev.get_evdev()
+
+ touches = self.make_contacts(5)
+ touches[0].confidence = False
+ touches[2].confidence = False
+ touches[4].confidence = False
+
+ r = uhdev.event(touches)
+ events = uhdev.next_sync_events()
+ self.debug_reports(r, uhdev, events)
+
+ assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH, 1) in events
+
+ self.assert_contacts(uhdev, evdev,
+ [ self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = None),
+ self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0),
+ self.ContactIds(contact_id = 2, tracking_id = -1, slot_num = None),
+ self.ContactIds(contact_id = 3, tracking_id = 1, slot_num = 1),
+ self.ContactIds(contact_id = 4, tracking_id = -1, slot_num = None) ])
+
+ def confidence_change_assert_playback(self, uhdev, evdev, timeline):
+ """
+ Assert proper behavior of contacts that move and change tipswitch /
+ confidence status over time.
+
+ Given a `timeline` list of touch states to iterate over, verify
+ that the contacts move and are reported as up/down as expected
+ by the state of the tipswitch and confidence bits.
+ """
+ t = 0
+
+ for state in timeline:
+ touches = self.make_contacts(len(state), t)
+
+ for item in zip(touches, state):
+ item[0].tipswitch = item[1][1]
+ item[0].confidence = item[1][2]
+
+ r = uhdev.event(touches)
+ events = uhdev.next_sync_events()
+ self.debug_reports(r, uhdev, events)
+
+ ids = [ x[0] for x in state ]
+ self.assert_contacts(uhdev, evdev, ids, t)
+
+ t += 1
+
+ def test_confidence_loss_a(self):
+ """
+ Transition a confident contact to a non-confident contact by
+ first clearing the tipswitch.
+
+ Ensure that the driver reports the transitioned contact as
+ being removed and that other contacts continue to report
+ normally. This mode of confidence loss is used by the
+ DTH-2452.
+ """
+ uhdev = self.uhdev
+ evdev = uhdev.get_evdev()
+
+ self.confidence_change_assert_playback(uhdev, evdev, [
+ # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # Both fingers confidently in contact
+ [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=1: Contact 0 == !Down + confident; Contact 1 == Down + confident
+ # First finger looses confidence and clears only the tipswitch flag
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=2: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger has lost confidence and has both flags cleared
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=3: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger has lost confidence and has both flags cleared
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)]
+ ])
+
+ def test_confidence_loss_b(self):
+ """
+ Transition a confident contact to a non-confident contact by
+ cleraing both tipswitch and confidence bits simultaneously.
+
+ Ensure that the driver reports the transitioned contact as
+ being removed and that other contacts continue to report
+ normally. This mode of confidence loss is used by some
+ AES devices.
+ """
+ uhdev = self.uhdev
+ evdev = uhdev.get_evdev()
+
+ self.confidence_change_assert_playback(uhdev, evdev, [
+ # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # Both fingers confidently in contact
+ [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=1: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger looses confidence and has both flags cleared simultaneously
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=2: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger has lost confidence and has both flags cleared
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=3: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger has lost confidence and has both flags cleared
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)]
+ ])
+
+ def test_confidence_loss_c(self):
+ """
+ Transition a confident contact to a non-confident contact by
+ clearing only the confidence bit.
+
+ Ensure that the driver reports the transitioned contact as
+ being removed and that other contacts continue to report
+ normally.
+ """
+ uhdev = self.uhdev
+ evdev = uhdev.get_evdev()
+
+ self.confidence_change_assert_playback(uhdev, evdev, [
+ # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # Both fingers confidently in contact
+ [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=1: Contact 0 == Down + !confident; Contact 1 == Down + confident
+ # First finger looses confidence and clears only the confidence flag
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), True, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=2: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger has lost confidence and has both flags cleared
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=3: Contact 0 == !Down + !confident; Contact 1 == Down + confident
+ # First finger has lost confidence and has both flags cleared
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)]
+ ])
+
+ def test_confidence_gain_a(self):
+ """
+ Transition a contact that was always non-confident to confident.
+
+ Ensure that the confident contact is reported normally.
+ """
+ uhdev = self.uhdev
+ evdev = uhdev.get_evdev()
+
+ self.confidence_change_assert_playback(uhdev, evdev, [
+ # t=0: Contact 0 == Down + !confident; Contact 1 == Down + confident
+ # Only second finger is confidently in contact
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = None), True, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)],
+
+ # t=1: Contact 0 == Down + !confident; Contact 1 == Down + confident
+ # First finger gains confidence
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = None), True, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)],
+
+ # t=2: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # First finger remains confident
+ [(self.ContactIds(contact_id = 0, tracking_id = 1, slot_num = 1), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)],
+
+ # t=3: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # First finger remains confident
+ [(self.ContactIds(contact_id = 0, tracking_id = 1, slot_num = 1), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)]
+ ])
+
+ def test_confidence_gain_b(self):
+ """
+ Transition a contact from non-confident to confident.
+
+ Ensure that the confident contact is reported normally.
+ """
+ uhdev = self.uhdev
+ evdev = uhdev.get_evdev()
+
+ self.confidence_change_assert_playback(uhdev, evdev, [
+ # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # First and second finger confidently in contact
+ [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=1: Contact 0 == Down + !confident; Contact 1 == Down + confident
+ # Firtst finger looses confidence
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), True, False),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=2: Contact 0 == Down + confident; Contact 1 == Down + confident
+ # First finger gains confidence
+ [(self.ContactIds(contact_id = 0, tracking_id = 2, slot_num = 0), True, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)],
+
+ # t=3: Contact 0 == !Down + confident; Contact 1 == Down + confident
+ # First finger goes up
+ [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, True),
+ (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)]
+ ])
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] HID: wacom: Correct behavior when processing some confidence == false touches
From: Gerecke, Jason @ 2023-12-19 21:33 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina
Cc: Aaron Skomra, Jason Gerecke, Joshua Dickens, Ping Cheng,
Tatsunosuke Tobita, Aaron Armstrong Skomra, Jason Gerecke,
Joshua Dickens, Ping Cheng, stable
From: Jason Gerecke <jason.gerecke@wacom.com>
There appear to be a few different ways that Wacom devices can deal with
confidence:
1. If the device looses confidence in a touch, it will first clear
the tipswitch flag in one report, and then clear the confidence
flag in a second report. This behavior is used by e.g. DTH-2452.
2. If the device looses confidence in a touch, it will clear both
the tipswitch and confidence flags within the same report. This
behavior is used by some AES devices.
3. If the device looses confidence in a touch, it will clear *only*
the confidence bit. The tipswitch bit will remain set so long as
the touch is tracked. This behavior may be used in future devices.
The driver does not currently handle situation 3 properly. Touches that
loose confidence will remain "in prox" and essentially frozen in place
until the tipswitch bit is finally cleared. Not only does this result
in userspace seeing a stuck touch, but it also prevents pen arbitration
from working properly (the pen won't send events until all touches are
up, but we don't currently process events from non-confident touches).
This commit centralizes the checking of the confidence bit in the
wacom_wac_finger_slot() function and has 'prox' depend on it. In the
case where situation 3 is encountered, the treat the touch as though
it was removed, allowing both userspace and the pen arbitration to
act normally.
Signed-off-by: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
Signed-off-by: Ping Cheng <ping.cheng@wacom.com>
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Fixes: 7fb0413baa7f ("HID: wacom: Use "Confidence" flag to prevent reporting invalid contacts")
Cc: stable@vger.kernel.org
---
drivers/hid/wacom_wac.c | 32 ++++----------------------------
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 471db78dbbf0..8289ce763704 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -2649,8 +2649,8 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
{
struct hid_data *hid_data = &wacom_wac->hid_data;
bool mt = wacom_wac->features.touch_max > 1;
- bool prox = hid_data->tipswitch &&
- report_touch_events(wacom_wac);
+ bool touch_down = hid_data->tipswitch && hid_data->confidence;
+ bool prox = touch_down && report_touch_events(wacom_wac);
if (touch_is_muted(wacom_wac)) {
if (!wacom_wac->shared->touch_down)
@@ -2700,24 +2700,6 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac,
}
}
-static bool wacom_wac_slot_is_active(struct input_dev *dev, int key)
-{
- struct input_mt *mt = dev->mt;
- struct input_mt_slot *s;
-
- if (!mt)
- return false;
-
- for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
- if (s->key == key &&
- input_mt_get_value(s, ABS_MT_TRACKING_ID) >= 0) {
- return true;
- }
- }
-
- return false;
-}
-
static void wacom_wac_finger_event(struct hid_device *hdev,
struct hid_field *field, struct hid_usage *usage, __s32 value)
{
@@ -2768,14 +2750,8 @@ static void wacom_wac_finger_event(struct hid_device *hdev,
}
if (usage->usage_index + 1 == field->report_count) {
- if (equivalent_usage == wacom_wac->hid_data.last_slot_field) {
- bool touch_removed = wacom_wac_slot_is_active(wacom_wac->touch_input,
- wacom_wac->hid_data.id) && !wacom_wac->hid_data.tipswitch;
-
- if (wacom_wac->hid_data.confidence || touch_removed) {
- wacom_wac_finger_slot(wacom_wac, wacom_wac->touch_input);
- }
- }
+ if (equivalent_usage == wacom_wac->hid_data.last_slot_field)
+ wacom_wac_finger_slot(wacom_wac, wacom_wac->touch_input);
}
}
--
2.43.0
^ permalink raw reply related
* Re: [RFC] dt-bindings: input: Clarify that abs_min must be less than abs_max
From: Dmitry Torokhov @ 2023-12-19 20:34 UTC (permalink / raw)
To: Artur Rojek
Cc: Chris Morgan, linux-input, devicetree, conor+dt,
krzysztof.kozlowski+dt, robh+dt, Chris Morgan, Paul Cercueil,
Peter Hutterer
In-Reply-To: <ZYH97KVDO4lFsbmi@google.com>
Sorry, meant to add Peter Hutterer to the conversation, but forgot
before hitting send...
On Tue, Dec 19, 2023 at 12:32:44PM -0800, Dmitry Torokhov wrote:
> On Fri, Dec 15, 2023 at 12:19:51PM +0100, Artur Rojek wrote:
> > On 2023-12-15 03:40, Chris Morgan wrote:
> > > From: Chris Morgan <macromorgan@hotmail.com>
> > >
> > > uinput refuses to work with abs devices where the min value is greater
> > > than the max value. uinput_validate_absinfo() returns -EINVAL if this
> > > is the case and prevents using uinput on such a device. Since uinput
> > > has worked this way since at least kernel 2.6 (or prior) I presume that
> > > this is the correct way of doing things, and that this documentation
> > > needs to be clarified that min must always be less than max.
> > >
> > > uinput is used in my use case to bind together adc-joystick devices
> > > with gpio-keys devices to create a single unified gamepad for
> > > userspace.
> > >
> > > Note that there are several boards that will need to be corrected,
> > > all but a few of them I maintain. Submitting as an RFC for now to get
> > > comments from the input team and the original author in case there is
> > > something I am missing.
> > >
> > > Fixes: 7956b0d4694f ("dt-bindings: input: Add docs for ADC driven
> > > joystick")
> > >
> > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > ---
> > > Documentation/devicetree/bindings/input/adc-joystick.yaml | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > > b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > > index 6c244d66f8ce..8f5cdd5ef190 100644
> > > --- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > > +++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > > @@ -73,8 +73,9 @@ patternProperties:
> > > description: >
> > > Minimum and maximum values produced by the axis.
> > > For an ABS_X axis this will be the left-most and right-most
> > > - inclination of the joystick. If min > max, it is left to
> > > userspace to
> > > - treat the axis as inverted.
> > > + inclination of the joystick. The axis must always be
> > > expressed as
> > > + min < max, if the axis is inverted it is left to userspace to
> > > handle
> > > + the inversion.
> >
> > Hi Chris,
> >
> > Device Tree is supposed to depict the actual state of the hardware.
> > I worded the adc-joytick's adc-range property specifically, so that it
> > covers a case of GCW Zero hardware [1], which has a joystick, where the
> > ABS_X axis reports increasing values for the left-wards inclination of
> > the joystick, and decreasing values for the right-wards inclination. You
> > are saying that there are even more boards that need to be corrected -
> > those are all situations, where DT depicts the actual behavior of the
> > hardware.
> > What you are trying to do is change hardware description, because of how
> > a driver in an OS works. You should instead fix behavior of said driver,
> > even if nobody stumbled upon that issue since 2.6 :) We fixed libSDL [2]
> > for the same reason.
>
> We have several places in the kernel (such as mousedev and joydev) where
> we expect that max is greater or equal to min if they are specified. I
> am sure that at least some userspace components also have this
> assumption. In general, we expect min to be a minimum value reported and
> max being maximum value reported, and orientation expressed via
> different properties (see [1]).
>
> Since we codified min > max as inversion for adc-joystick devices in the
> bindings, I think we need to handle this *in that driver* and leave the
> rest alone.
>
> [1] Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
>
--
Dmitry
^ permalink raw reply
* Re: [RFC] dt-bindings: input: Clarify that abs_min must be less than abs_max
From: Dmitry Torokhov @ 2023-12-19 20:32 UTC (permalink / raw)
To: Artur Rojek
Cc: Chris Morgan, linux-input, devicetree, conor+dt,
krzysztof.kozlowski+dt, robh+dt, Chris Morgan, Paul Cercueil
In-Reply-To: <03a9a56362b0559234d4a21a4de3e32e@artur-rojek.eu>
On Fri, Dec 15, 2023 at 12:19:51PM +0100, Artur Rojek wrote:
> On 2023-12-15 03:40, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> >
> > uinput refuses to work with abs devices where the min value is greater
> > than the max value. uinput_validate_absinfo() returns -EINVAL if this
> > is the case and prevents using uinput on such a device. Since uinput
> > has worked this way since at least kernel 2.6 (or prior) I presume that
> > this is the correct way of doing things, and that this documentation
> > needs to be clarified that min must always be less than max.
> >
> > uinput is used in my use case to bind together adc-joystick devices
> > with gpio-keys devices to create a single unified gamepad for
> > userspace.
> >
> > Note that there are several boards that will need to be corrected,
> > all but a few of them I maintain. Submitting as an RFC for now to get
> > comments from the input team and the original author in case there is
> > something I am missing.
> >
> > Fixes: 7956b0d4694f ("dt-bindings: input: Add docs for ADC driven
> > joystick")
> >
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> > Documentation/devicetree/bindings/input/adc-joystick.yaml | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > index 6c244d66f8ce..8f5cdd5ef190 100644
> > --- a/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > +++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
> > @@ -73,8 +73,9 @@ patternProperties:
> > description: >
> > Minimum and maximum values produced by the axis.
> > For an ABS_X axis this will be the left-most and right-most
> > - inclination of the joystick. If min > max, it is left to
> > userspace to
> > - treat the axis as inverted.
> > + inclination of the joystick. The axis must always be
> > expressed as
> > + min < max, if the axis is inverted it is left to userspace to
> > handle
> > + the inversion.
>
> Hi Chris,
>
> Device Tree is supposed to depict the actual state of the hardware.
> I worded the adc-joytick's adc-range property specifically, so that it
> covers a case of GCW Zero hardware [1], which has a joystick, where the
> ABS_X axis reports increasing values for the left-wards inclination of
> the joystick, and decreasing values for the right-wards inclination. You
> are saying that there are even more boards that need to be corrected -
> those are all situations, where DT depicts the actual behavior of the
> hardware.
> What you are trying to do is change hardware description, because of how
> a driver in an OS works. You should instead fix behavior of said driver,
> even if nobody stumbled upon that issue since 2.6 :) We fixed libSDL [2]
> for the same reason.
We have several places in the kernel (such as mousedev and joydev) where
we expect that max is greater or equal to min if they are specified. I
am sure that at least some userspace components also have this
assumption. In general, we expect min to be a minimum value reported and
max being maximum value reported, and orientation expressed via
different properties (see [1]).
Since we codified min > max as inversion for adc-joystick devices in the
bindings, I think we need to handle this *in that driver* and leave the
rest alone.
[1] Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v5 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Marco Felsch @ 2023-12-19 19:54 UTC (permalink / raw)
To: Kamel Bouhara
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Jeff LaBundy, 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 23-12-11, 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>
> +
> +#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;
> + u8 fw_minor;
> + u8 fw_major;
> + u8 fw_info_extra;
> + u8 tcp_revision;
> + u8 bootloader_fw_minor;
> + u8 bootloader_fw_major;
> + u16 jedec_id;
> + 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).
> + */
> +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];
> + struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
> + struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
Still, why do you not use 0xff as max value? This elimates looping over
the usage table within axiom_usage_to_target_address() since you can
use the usage directly via ts->usage_table[usage] IIRC.
> + 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.
> + * 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;
> +
> + /* 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;
> +
> + 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;
> + }
> +
> + usleep_range(250, 300);
> +
> + return 0;
> +}
> +
> +/*
> + * 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;
> + 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 (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)
> + 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);
> + 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;
> + }
> +
> + 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);
> + 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;
> +}
> +
> +/*
> + * 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));
> +
> + for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> + char target_step = i * 4;
> +
> + 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.
> + */
> +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;
> +
> + 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;
> + 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;
> + struct device *dev = ts->dev;
> + u16 crc_report;
> + u16 crc_calc;
> + int error;
> + char len;
> +
> + 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;
> + }
> +
> + /* Validate the report CRC */
> + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
> + /* 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);
> + 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");
A rough review, this can't work since you made the regulator names
lower-case. Same below.
> + if (!IS_ERR(ts->vddi)) {
No !IS_ERR is required here since we do require it from API pov. DTS
enabled platforms will provide a dummy regulator if not specified, ACPI
is not supported right now so you don't need to care about. Same below.
> + error = regulator_enable(ts->vddi);
If we don't provide any (runtime-)pm you can use:
devm_regulator_get_enable(). Same below.
> + 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);
> + return error;
> + }
> + if (!device_property_read_u32(dev, "startup-time-ms", &startup_delay_ms))
^
This property is not documented and also is related to the regulator
itself.
> + msleep(startup_delay_ms);
> + }
> +
> + 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);
> +
> + /* Multi Touch */
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + 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);
> +
> + input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
> +
> + /* 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) {
> + dev_warn(dev, "Request irq failed, falling back to polling mode");
Why do you print a warning? I thought this is possible use-case in case
that the IRQ line was not connected "on purpose".
> + 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))
Nit: poll_intervall can be local within this if block:
u32 poll_interval = POLL_INTERVAL_DEFAULT_MS;
> + input_set_poll_interval(input_dev, poll_interval);
> + else
> + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
and this else can be dropped.
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> + { "ax54a" },
> + {},
> +};
> +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);
> +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
>
>
^ permalink raw reply
* [PATCH v10 1/6] pwm: Rename pwm_apply_state() to pwm_apply_might_sleep()
From: Sean Young @ 2023-12-19 16:30 UTC (permalink / raw)
To: linux-media, linux-pwm, Ivaylo Dimitrov, Thierry Reding,
Uwe Kleine-König, Jonathan Corbet, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Daniel Vetter, Javier Martinez Canillas, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Jean Delvare, Guenter Roeck,
Support Opensource, Dmitry Torokhov, Pavel Machek, Lee Jones,
Sean Young, Mauro Carvalho Chehab, Hans de Goede,
Ilpo Järvinen, Mark Gross, Liam Girdwood, Mark Brown,
Daniel Thompson, Jingoo Han, Helge Deller
Cc: Jani Nikula, linux-doc, linux-kernel, intel-gfx, dri-devel,
linux-hwmon, linux-input, linux-leds, platform-driver-x86,
linux-arm-kernel, linux-fbdev
In-Reply-To: <cover.1703003288.git.sean@mess.org>
In order to introduce a pwm api which can be used from atomic context,
we will need two functions for applying pwm changes:
int pwm_apply_might_sleep(struct pwm *, struct pwm_state *);
int pwm_apply_atomic(struct pwm *, struct pwm_state *);
This commit just deals with renaming pwm_apply_state(), a following
commit will introduce the pwm_apply_atomic() function.
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Mark Brown <broonie@kernel.org>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> # for input
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Acked-by: Lee Jones <lee@kernel.org>
Signed-off-by: Sean Young <sean@mess.org>
---
Documentation/driver-api/pwm.rst | 8 +++---
MAINTAINERS | 2 +-
.../gpu/drm/i915/display/intel_backlight.c | 6 ++--
drivers/gpu/drm/solomon/ssd130x.c | 2 +-
drivers/hwmon/pwm-fan.c | 8 +++---
drivers/input/misc/da7280.c | 4 +--
drivers/input/misc/pwm-beeper.c | 4 +--
drivers/input/misc/pwm-vibra.c | 8 +++---
drivers/leds/leds-pwm.c | 2 +-
drivers/leds/rgb/leds-pwm-multicolor.c | 4 +--
drivers/media/rc/pwm-ir-tx.c | 4 +--
drivers/platform/x86/lenovo-yogabook.c | 2 +-
drivers/pwm/core.c | 18 ++++++------
drivers/pwm/pwm-twl-led.c | 2 +-
drivers/pwm/pwm-vt8500.c | 2 +-
drivers/pwm/sysfs.c | 10 +++----
drivers/regulator/pwm-regulator.c | 4 +--
drivers/video/backlight/lm3630a_bl.c | 2 +-
drivers/video/backlight/lp855x_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 12 ++++----
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 28 +++++++++----------
22 files changed, 68 insertions(+), 68 deletions(-)
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index bb264490a87a..f1d8197c8c43 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -41,7 +41,7 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also exist.
After being requested, a PWM has to be configured using::
- int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
+ int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state);
This API controls both the PWM period/duty_cycle config and the
enable/disable state.
@@ -57,13 +57,13 @@ If supported by the driver, the signal can be optimized, for example to improve
EMI by phase shifting the individual channels of a chip.
The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
-around pwm_apply_state() and should not be used if the user wants to change
+around pwm_apply_might_sleep() and should not be used if the user wants to change
several parameter at once. For example, if you see pwm_config() and
pwm_{enable,disable}() calls in the same function, this probably means you
-should switch to pwm_apply_state().
+should switch to pwm_apply_might_sleep().
The PWM user API also allows one to query the PWM state that was passed to the
-last invocation of pwm_apply_state() using pwm_get_state(). Note this is
+last invocation of pwm_apply_might_sleep() using pwm_get_state(). Note this is
different to what the driver has actually implemented if the request cannot be
satisfied exactly with the hardware in use. There is currently no way for
consumers to get the actually implemented settings.
diff --git a/MAINTAINERS b/MAINTAINERS
index 97f51d5ec1cf..c58480595220 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17576,7 +17576,7 @@ F: drivers/video/backlight/pwm_bl.c
F: include/dt-bindings/pwm/
F: include/linux/pwm.h
F: include/linux/pwm_backlight.h
-K: pwm_(config|apply_state|ops)
+K: pwm_(config|apply_might_sleep|ops)
PXA GPIO DRIVER
M: Robert Jarzmik <robert.jarzmik@free.fr>
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index 2e8f17c04522..ff9b9918b0a1 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,
struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_might_sleep(panel->backlight.pwm, &panel->backlight.pwm_state);
}
static void
@@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn
intel_backlight_set_pwm_level(old_conn_state, level);
panel->backlight.pwm_state.enabled = false;
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_might_sleep(panel->backlight.pwm, &panel->backlight.pwm_state);
}
void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
@@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
panel->backlight.pwm_state.enabled = true;
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_might_sleep(panel->backlight.pwm, &panel->backlight.pwm_state);
}
static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index e0174f82e353..cce043a4a1dc 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -319,7 +319,7 @@ static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
pwm_init_state(ssd130x->pwm, &pwmstate);
pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
- pwm_apply_state(ssd130x->pwm, &pwmstate);
+ pwm_apply_might_sleep(ssd130x->pwm, &pwmstate);
/* Enable the PWM */
pwm_enable(ssd130x->pwm);
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 6e4516c2ab89..b67bc9e833c0 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -151,7 +151,7 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
}
state->enabled = true;
- ret = pwm_apply_state(ctx->pwm, state);
+ ret = pwm_apply_might_sleep(ctx->pwm, state);
if (ret) {
dev_err(ctx->dev, "failed to enable PWM\n");
goto disable_regulator;
@@ -181,7 +181,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
state->enabled = false;
state->duty_cycle = 0;
- ret = pwm_apply_state(ctx->pwm, state);
+ ret = pwm_apply_might_sleep(ctx->pwm, state);
if (ret) {
dev_err(ctx->dev, "failed to disable PWM\n");
return ret;
@@ -207,7 +207,7 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
period = state->period;
state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
- ret = pwm_apply_state(ctx->pwm, state);
+ ret = pwm_apply_might_sleep(ctx->pwm, state);
if (ret)
return ret;
ret = pwm_fan_power_on(ctx);
@@ -278,7 +278,7 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
state,
&enable_regulator);
- pwm_apply_state(ctx->pwm, state);
+ pwm_apply_might_sleep(ctx->pwm, state);
pwm_fan_switch_power(ctx, enable_regulator);
pwm_fan_update_state(ctx, 0);
}
diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
index ce82548916bb..c1fa75c0f970 100644
--- a/drivers/input/misc/da7280.c
+++ b/drivers/input/misc/da7280.c
@@ -352,7 +352,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled)
state.duty_cycle = period_mag_multi;
}
- error = pwm_apply_state(haptics->pwm_dev, &state);
+ error = pwm_apply_might_sleep(haptics->pwm_dev, &state);
if (error)
dev_err(haptics->dev, "Failed to apply pwm state: %d\n", error);
@@ -1175,7 +1175,7 @@ static int da7280_probe(struct i2c_client *client)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(haptics->pwm_dev, &state);
state.enabled = false;
- error = pwm_apply_state(haptics->pwm_dev, &state);
+ error = pwm_apply_might_sleep(haptics->pwm_dev, &state);
if (error) {
dev_err(dev, "Failed to apply PWM state: %d\n", error);
return error;
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 1e731d8397c6..5b9aedf4362f 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -39,7 +39,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
state.period = period;
pwm_set_relative_duty_cycle(&state, 50, 100);
- error = pwm_apply_state(beeper->pwm, &state);
+ error = pwm_apply_might_sleep(beeper->pwm, &state);
if (error)
return error;
@@ -138,7 +138,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(beeper->pwm, &state);
state.enabled = false;
- error = pwm_apply_state(beeper->pwm, &state);
+ error = pwm_apply_might_sleep(beeper->pwm, &state);
if (error) {
dev_err(dev, "failed to apply initial PWM state: %d\n",
error);
diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
index acac79c488aa..3e5ed685ed8f 100644
--- a/drivers/input/misc/pwm-vibra.c
+++ b/drivers/input/misc/pwm-vibra.c
@@ -56,7 +56,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
state.enabled = true;
- err = pwm_apply_state(vibrator->pwm, &state);
+ err = pwm_apply_might_sleep(vibrator->pwm, &state);
if (err) {
dev_err(pdev, "failed to apply pwm state: %d\n", err);
return err;
@@ -67,7 +67,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
state.duty_cycle = vibrator->direction_duty_cycle;
state.enabled = true;
- err = pwm_apply_state(vibrator->pwm_dir, &state);
+ err = pwm_apply_might_sleep(vibrator->pwm_dir, &state);
if (err) {
dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
pwm_disable(vibrator->pwm);
@@ -160,7 +160,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(vibrator->pwm, &state);
state.enabled = false;
- err = pwm_apply_state(vibrator->pwm, &state);
+ err = pwm_apply_might_sleep(vibrator->pwm, &state);
if (err) {
dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
err);
@@ -174,7 +174,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(vibrator->pwm_dir, &state);
state.enabled = false;
- err = pwm_apply_state(vibrator->pwm_dir, &state);
+ err = pwm_apply_might_sleep(vibrator->pwm_dir, &state);
if (err) {
dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
err);
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 2b3bf1353b70..4e3936a39d0e 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -54,7 +54,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
led_dat->pwmstate.duty_cycle = duty;
led_dat->pwmstate.enabled = true;
- return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
+ return pwm_apply_might_sleep(led_dat->pwm, &led_dat->pwmstate);
}
__attribute__((nonnull))
diff --git a/drivers/leds/rgb/leds-pwm-multicolor.c b/drivers/leds/rgb/leds-pwm-multicolor.c
index 46cd062b8b24..e1a81e0109e8 100644
--- a/drivers/leds/rgb/leds-pwm-multicolor.c
+++ b/drivers/leds/rgb/leds-pwm-multicolor.c
@@ -51,8 +51,8 @@ static int led_pwm_mc_set(struct led_classdev *cdev,
priv->leds[i].state.duty_cycle = duty;
priv->leds[i].state.enabled = duty > 0;
- ret = pwm_apply_state(priv->leds[i].pwm,
- &priv->leds[i].state);
+ ret = pwm_apply_might_sleep(priv->leds[i].pwm,
+ &priv->leds[i].state);
if (ret)
break;
}
diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index c5f37c03af9c..cf51e2760975 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -68,7 +68,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
for (i = 0; i < count; i++) {
state.enabled = !(i % 2);
- pwm_apply_state(pwm, &state);
+ pwm_apply_might_sleep(pwm, &state);
edge = ktime_add_us(edge, txbuf[i]);
delta = ktime_us_delta(edge, ktime_get());
@@ -77,7 +77,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
}
state.enabled = false;
- pwm_apply_state(pwm, &state);
+ pwm_apply_might_sleep(pwm, &state);
return count;
}
diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo-yogabook.c
index b8d0239192cb..fd62bf746ebd 100644
--- a/drivers/platform/x86/lenovo-yogabook.c
+++ b/drivers/platform/x86/lenovo-yogabook.c
@@ -435,7 +435,7 @@ static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level)
.enabled = level,
};
- pwm_apply_state(data->kbd_bl_pwm, &state);
+ pwm_apply_might_sleep(data->kbd_bl_pwm, &state);
gpiod_set_value(data->kbd_bl_led_enable, level ? 1 : 0);
return 0;
}
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f60b715abe62..c2d78136625d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -326,8 +326,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
}
EXPORT_SYMBOL_GPL(pwm_request_from_chip);
-static void pwm_apply_state_debug(struct pwm_device *pwm,
- const struct pwm_state *state)
+static void pwm_apply_debug(struct pwm_device *pwm,
+ const struct pwm_state *state)
{
struct pwm_state *last = &pwm->last;
struct pwm_chip *chip = pwm->chip;
@@ -433,11 +433,11 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
}
/**
- * pwm_apply_state() - atomically apply a new state to a PWM device
+ * pwm_apply_might_sleep() - atomically apply a new state to a PWM device
* @pwm: PWM device
* @state: new state to apply
*/
-int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
{
struct pwm_chip *chip;
int err;
@@ -445,7 +445,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
/*
* Some lowlevel driver's implementations of .apply() make use of
* mutexes, also with some drivers only returning when the new
- * configuration is active calling pwm_apply_state() from atomic context
+ * configuration is active calling pwm_apply_might_sleep() from atomic context
* is a bad idea. So make it explicit that calling this function might
* sleep.
*/
@@ -475,11 +475,11 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
* only do this after pwm->state was applied as some
* implementations of .get_state depend on this
*/
- pwm_apply_state_debug(pwm, state);
+ pwm_apply_debug(pwm, state);
return 0;
}
-EXPORT_SYMBOL_GPL(pwm_apply_state);
+EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
/**
* pwm_capture() - capture and report a PWM signal
@@ -537,7 +537,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
state.period = pargs.period;
state.polarity = pargs.polarity;
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_might_sleep(pwm, &state);
}
/*
@@ -560,7 +560,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
state.duty_cycle = state.period - state.duty_cycle;
}
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_might_sleep(pwm, &state);
}
EXPORT_SYMBOL_GPL(pwm_adjust_config);
diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 8a870d0db3c6..c670ccb81653 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -172,7 +172,7 @@ static int twl4030_pwmled_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* We cannot skip calling ->config even if state->period ==
* pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
* because we might have exited early in the last call to
- * pwm_apply_state because of !state->enabled and so the two values in
+ * pwm_apply_might_sleep because of !state->enabled and so the two values in
* pwm->state might not be configured in hardware.
*/
ret = twl4030_pwmled_config(chip, pwm,
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index bdea60389487..7bfeacee05d0 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -206,7 +206,7 @@ static int vt8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* We cannot skip calling ->config even if state->period ==
* pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
* because we might have exited early in the last call to
- * pwm_apply_state because of !state->enabled and so the two values in
+ * pwm_apply_might_sleep because of !state->enabled and so the two values in
* pwm->state might not be configured in hardware.
*/
err = vt8500_pwm_config(chip, pwm, state->duty_cycle, state->period);
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 4edb994fa2e1..1698609d91c8 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -62,7 +62,7 @@ static ssize_t period_store(struct device *child,
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
state.period = val;
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_might_sleep(pwm, &state);
mutex_unlock(&export->lock);
return ret ? : size;
@@ -97,7 +97,7 @@ static ssize_t duty_cycle_store(struct device *child,
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
state.duty_cycle = val;
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_might_sleep(pwm, &state);
mutex_unlock(&export->lock);
return ret ? : size;
@@ -144,7 +144,7 @@ static ssize_t enable_store(struct device *child,
goto unlock;
}
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_might_sleep(pwm, &state);
unlock:
mutex_unlock(&export->lock);
@@ -194,7 +194,7 @@ static ssize_t polarity_store(struct device *child,
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
state.polarity = polarity;
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_might_sleep(pwm, &state);
mutex_unlock(&export->lock);
return ret ? : size;
@@ -401,7 +401,7 @@ static int pwm_class_apply_state(struct pwm_export *export,
struct pwm_device *pwm,
struct pwm_state *state)
{
- int ret = pwm_apply_state(pwm, state);
+ int ret = pwm_apply_might_sleep(pwm, state);
/* release lock taken in pwm_class_get_state */
mutex_unlock(&export->lock);
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 2aff6db748e2..698c420e0869 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -90,7 +90,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
pwm_set_relative_duty_cycle(&pstate,
drvdata->duty_cycle_table[selector].dutycycle, 100);
- ret = pwm_apply_state(drvdata->pwm, &pstate);
+ ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
if (ret) {
dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
return ret;
@@ -216,7 +216,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
- ret = pwm_apply_state(drvdata->pwm, &pstate);
+ ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
if (ret) {
dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
return ret;
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 8fcb62be597b..a3412c936ca2 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -180,7 +180,7 @@ static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
- return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
+ return pwm_apply_might_sleep(pchip->pwmd, &pchip->pwmd_state);
}
/* update and get brightness */
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index da1f124db69c..7075bfab59c4 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -234,7 +234,7 @@ static int lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
state.duty_cycle = div_u64(br * state.period, max_br);
state.enabled = state.duty_cycle;
- return pwm_apply_state(lp->pwm, &state);
+ return pwm_apply_might_sleep(lp->pwm, &state);
}
static int lp855x_bl_update_status(struct backlight_device *bl)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 289bd9ce4d36..35c716e9043c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -103,7 +103,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
pwm_get_state(pb->pwm, &state);
state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
state.enabled = true;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_might_sleep(pb->pwm, &state);
pwm_backlight_power_on(pb);
} else {
@@ -120,7 +120,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
* inactive output.
*/
state.enabled = !pb->power_supply && !pb->enable_gpio;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_might_sleep(pb->pwm, &state);
}
if (pb->notify_after)
@@ -528,7 +528,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (!state.period && (data->pwm_period_ns > 0))
state.period = data->pwm_period_ns;
- ret = pwm_apply_state(pb->pwm, &state);
+ ret = pwm_apply_might_sleep(pb->pwm, &state);
if (ret) {
dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
ret);
@@ -633,7 +633,7 @@ static void pwm_backlight_remove(struct platform_device *pdev)
pwm_get_state(pb->pwm, &state);
state.duty_cycle = 0;
state.enabled = false;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_might_sleep(pb->pwm, &state);
if (pb->exit)
pb->exit(&pdev->dev);
@@ -649,7 +649,7 @@ static void pwm_backlight_shutdown(struct platform_device *pdev)
pwm_get_state(pb->pwm, &state);
state.duty_cycle = 0;
state.enabled = false;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_might_sleep(pb->pwm, &state);
}
#ifdef CONFIG_PM_SLEEP
@@ -673,7 +673,7 @@ static int pwm_backlight_suspend(struct device *dev)
pwm_get_state(pb->pwm, &state);
state.duty_cycle = 0;
state.enabled = false;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_might_sleep(pb->pwm, &state);
if (pb->notify_after)
pb->notify_after(pb->dev, 0);
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 5ae48e36fccb..1a4f90ea7d5a 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -347,7 +347,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
pwm_init_state(par->pwm, &pwmstate);
pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
- pwm_apply_state(par->pwm, &pwmstate);
+ pwm_apply_might_sleep(par->pwm, &pwmstate);
/* Enable the PWM */
pwm_enable(par->pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f87655c06c82..b64b8a82415c 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -92,8 +92,8 @@ struct pwm_device {
* @state: state to fill with the current PWM state
*
* The returned PWM state represents the state that was applied by a previous call to
- * pwm_apply_state(). Drivers may have to slightly tweak that state before programming it to
- * hardware. If pwm_apply_state() was never called, this returns either the current hardware
+ * pwm_apply_might_sleep(). Drivers may have to slightly tweak that state before programming it to
+ * hardware. If pwm_apply_might_sleep() was never called, this returns either the current hardware
* state (if supported) or the default settings.
*/
static inline void pwm_get_state(const struct pwm_device *pwm,
@@ -157,20 +157,20 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
}
/**
- * pwm_init_state() - prepare a new state to be applied with pwm_apply_state()
+ * pwm_init_state() - prepare a new state to be applied with pwm_apply_might_sleep()
* @pwm: PWM device
* @state: state to fill with the prepared PWM state
*
* This functions prepares a state that can later be tweaked and applied
- * to the PWM device with pwm_apply_state(). This is a convenient function
+ * to the PWM device with pwm_apply_might_sleep(). This is a convenient function
* that first retrieves the current PWM state and the replaces the period
* and polarity fields with the reference values defined in pwm->args.
* Once the function returns, you can adjust the ->enabled and ->duty_cycle
- * fields according to your needs before calling pwm_apply_state().
+ * fields according to your needs before calling pwm_apply_might_sleep().
*
* ->duty_cycle is initially set to zero to avoid cases where the current
* ->duty_cycle value exceed the pwm_args->period one, which would trigger
- * an error if the user calls pwm_apply_state() without adjusting ->duty_cycle
+ * an error if the user calls pwm_apply_might_sleep() without adjusting ->duty_cycle
* first.
*/
static inline void pwm_init_state(const struct pwm_device *pwm,
@@ -226,7 +226,7 @@ pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
*
* pwm_init_state(pwm, &state);
* pwm_set_relative_duty_cycle(&state, 50, 100);
- * pwm_apply_state(pwm, &state);
+ * pwm_apply_might_sleep(pwm, &state);
*
* This functions returns -EINVAL if @duty_cycle and/or @scale are
* inconsistent (@scale == 0 or @duty_cycle > @scale).
@@ -304,7 +304,7 @@ struct pwm_chip {
#if IS_ENABLED(CONFIG_PWM)
/* PWM user APIs */
-int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
+int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
int pwm_adjust_config(struct pwm_device *pwm);
/**
@@ -332,7 +332,7 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
state.duty_cycle = duty_ns;
state.period = period_ns;
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_might_sleep(pwm, &state);
}
/**
@@ -353,7 +353,7 @@ static inline int pwm_enable(struct pwm_device *pwm)
return 0;
state.enabled = true;
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_might_sleep(pwm, &state);
}
/**
@@ -372,7 +372,7 @@ static inline void pwm_disable(struct pwm_device *pwm)
return;
state.enabled = false;
- pwm_apply_state(pwm, &state);
+ pwm_apply_might_sleep(pwm, &state);
}
/* PWM provider APIs */
@@ -403,8 +403,8 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
struct fwnode_handle *fwnode,
const char *con_id);
#else
-static inline int pwm_apply_state(struct pwm_device *pwm,
- const struct pwm_state *state)
+static inline int pwm_apply_might_sleep(struct pwm_device *pwm,
+ const struct pwm_state *state)
{
might_sleep();
return -ENOTSUPP;
@@ -521,7 +521,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
state.period = pwm->args.period;
state.usage_power = false;
- pwm_apply_state(pwm, &state);
+ pwm_apply_might_sleep(pwm, &state);
}
struct pwm_lookup {
--
2.43.0
^ permalink raw reply related
* Re: element sizes in input_event struct on riscv32
From: Arnd Bergmann @ 2023-12-19 13:53 UTC (permalink / raw)
To: Dmitry Torokhov, Antonios Salios, Deepa Dinamani
Cc: rydberg, linux-input, linux-kernel, Jan Henrik Weinstock,
Lukas Jünger
In-Reply-To: <ZYEFCHBC75rjCE0n@google.com>
On Tue, Dec 19, 2023, at 02:50, Dmitry Torokhov wrote:
> Hi Antonious,
>
> On Thu, Dec 14, 2023 at 11:11:18AM +0100, Antonios Salios wrote:
>> Hi all.
>>
>> I'm having trouble getting evdev to run in a simulated Buildroot
>> environment on riscv32. Evtest (and the x11 driver) seems to be
>> receiving garbage data from input devices.
>>
>> Analyzing the input_event struct shows that the kernel uses 32-bit (aka
>> __kernel_ulong_t) values for __sec & __usec.
>> Evtest on the other hand interprets these variables as 64-bit time_t
>> values in a timeval struct, resulting in a mismatch between the kernel
>> and userspace.
>>
>> What would be the correct size for these values on a 32-bit
>> architecture that uses 64-bit time_t values?
>
> I think there is misunderstanding - we do not have *2* 64-bit values on
> 32-but architectures. Here is what was done:
>
> Input: extend usable life of event timestamps to 2106 on 32 bit systems
Thanks for forwarding this to me. You are definitely right that
the user-space structure is intended to use a pair of __kernel_ulong_t
for the timestamp. Usually if an application gets this wrong, it is the
result of having copied old kernel headers the source directory that
need to be updated.
For evtest in particular, I don't see how that is possible, the source
code at [1] shows that it just includes the global linux/input.h,
which on riscv32 would have to be at least from linux-5.6 anyway
because older versions are too old to build a time64 glibc.
Antonios, can you check which header was used to build your copy
of evtest, and in case this came from /usr/include/linux, which
version it corresponds to?
Arnd
[1] https://gitlab.freedesktop.org/libevdev/evtest/-/blob/master/evtest.c?ref_type=heads
^ permalink raw reply
* Re: [PATCH v13 0/4] Input: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-12-19 9:06 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, linux-input, Rob Herring
In-Reply-To: <20231213-topic-goodix-berlin-upstream-initial-v13-0-5d7a26a5eaa2@linaro.org>
Hi Dmitry,
On 13/12/2023 18:13, Neil Armstrong wrote:
> These touchscreen ICs support SPI, I2C and I3C interface, up to
> 10 finger touch, stylus and gestures events.
>
<snip>
Gentle ping! Is there any chance this driver could land for v6.8-rc1 ?
Thanks!
Neil
<snip>
^ permalink raw reply
* Re: [PATCH 0/2] Fix regression in ALS
From: Greg KH @ 2023-12-19 7:08 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Srinivas Pandruvada, jikos, jic23, lars, Basavaraj.Natikar,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20231218162227.00002197@Huawei.com>
On Mon, Dec 18, 2023 at 04:22:27PM +0000, Jonathan Cameron wrote:
> On Sun, 17 Dec 2023 12:07:01 -0800
> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
>
> > Addition of color temperature and chromaticity support breaks ALS sensor
> > on several platforms. Till we have a good solution, revert these commits
> > for 6.7 cycle.
> >
> > Srinivas Pandruvada (2):
> > Revert "iio: hid-sensor-als: Add light chromaticity support"
> > Revert "iio: hid-sensor-als: Add light color temperature support"
> >
> > drivers/iio/light/hid-sensor-als.c | 100 +----------------------------
> > include/linux/hid-sensor-ids.h | 4 --
> > 2 files changed, 2 insertions(+), 102 deletions(-)
>
> +CC Greg KH. (resent as I messed up Greg's address first time around)
>
> Hi Greg,
>
> This is a regression fix that I'd like to get in asap. Currently light sensors
> on a wide range of laptops are broken. I was hoping we'd fix the the problem
> rather than need to revert, but time is running out so revert it is.
>
> I don't have anything else that needs to be rushed in before the merge cycle,
> so if you are happy to pick these two reverts directly that would be great.
>
> Message ID of the cover letter is
>
> 20231217200703.719876-1-srinivas.pandruvada@linux.intel.com
>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> If not I should be able to do a pull request in next couple of days
> with these in.
Now queued up, thanks.
greg k-h
^ permalink raw reply
* Re: element sizes in input_event struct on riscv32
From: Dmitry Torokhov @ 2023-12-19 2:50 UTC (permalink / raw)
To: Antonios Salios, Arnd Bergmann, Deepa Dinamani
Cc: rydberg, linux-input, linux-kernel, Jan Henrik Weinstock,
Lukas Jünger
In-Reply-To: <c812ea74dd02d1baf85dc6fb32701e103984d25d.camel@mwa.re>
Hi Antonious,
On Thu, Dec 14, 2023 at 11:11:18AM +0100, Antonios Salios wrote:
> Hi all.
>
> I'm having trouble getting evdev to run in a simulated Buildroot
> environment on riscv32. Evtest (and the x11 driver) seems to be
> receiving garbage data from input devices.
>
> Analyzing the input_event struct shows that the kernel uses 32-bit (aka
> __kernel_ulong_t) values for __sec & __usec.
> Evtest on the other hand interprets these variables as 64-bit time_t
> values in a timeval struct, resulting in a mismatch between the kernel
> and userspace.
>
> What would be the correct size for these values on a 32-bit
> architecture that uses 64-bit time_t values?
I think there is misunderstanding - we do not have *2* 64-bit values on
32-but architectures. Here is what was done:
commit 152194fe9c3f03232b9c0d0264793a7fa4af82f8
Author: Deepa Dinamani <deepa.kernel@gmail.com>
Date: Sun Jan 7 17:44:42 2018 -0800
Input: extend usable life of event timestamps to 2106 on 32 bit systems
The input events use struct timeval to store event time, unfortunately
this structure is not y2038 safe and is being replaced in kernel with
y2038 safe structures.
Because of ABI concerns we can not change the size or the layout of
structure input_event, so we opt to re-interpreting the 'seconds' part
of timestamp as an unsigned value, effectively doubling the range of
values, to year 2106.
Newer glibc that has support for 32 bit applications to use 64 bit
time_t supplies __USE_TIME_BITS64 define [1], that we can use to present
the userspace with updated input_event layout. The updated layout will
cause the compile time breakage, alerting applications and distributions
maintainers to the issue. Existing 32 binaries will continue working
without any changes until 2038.
Ultimately userspace applications should switch to using monotonic or
boot time clocks, as realtime clock is not very well suited for input
event timestamps as it can go backwards (see a80b83b7b8 "Input: evdev -
add CLOCK_BOOTTIME support" by by John Stultz). With monotonic clock the
practical range of reported times will always fit into the pair of 32
bit values, as we do not expect any system to stay up for a hundred
years without a single reboot.
[1] https://sourceware.org/glibc/wiki/Y2038ProofnessDesign
I'll let Arnd and Deepa to comment further.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 4/4] Input: da9063 - Add polling support
From: Dmitry Torokhov @ 2023-12-19 1:52 UTC (permalink / raw)
To: Biju Das
Cc: Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20231213214803.9931-5-biju.das.jz@bp.renesas.com>
On Wed, Dec 13, 2023 at 09:48:03PM +0000, Biju Das wrote:
> +static void da9063_onkey_polled_poll(struct input_dev *input)
> +{
> + struct da9063_onkey *onkey = input_get_drvdata(input);
> + const struct da906x_chip_config *config = onkey->config;
> + unsigned int val;
> + int error;
> +
> + error = regmap_read(onkey->regmap, config->onkey_status, &val);
> + if (onkey->key_power && !error && (val & config->onkey_nonkey_mask)) {
> + input_report_key(onkey->input, KEY_POWER, 1);
> + input_sync(onkey->input);
> + schedule_delayed_work(&onkey->work, 0);
In the polling case you should not be scheduling any additional works as
the driver may get confused if you repeatedly open and close input
device.
Also I think in threaded case it might be cleaner to avoid scheduling
work and simply loop in the interrupt thread (since it can sleep).
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 3/4] Input: da9063 - Use dev_err_probe()
From: Dmitry Torokhov @ 2023-12-19 1:50 UTC (permalink / raw)
To: Biju Das
Cc: Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20231213214803.9931-4-biju.das.jz@bp.renesas.com>
On Wed, Dec 13, 2023 at 09:48:02PM +0000, Biju Das wrote:
> Replace dev_err()->dev_err_probe() to simplify probe().
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 2/4] Input: da9063 - Drop redundant prints in probe()
From: Dmitry Torokhov @ 2023-12-19 1:50 UTC (permalink / raw)
To: Biju Das
Cc: Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20231213214803.9931-3-biju.das.jz@bp.renesas.com>
On Wed, Dec 13, 2023 at 09:48:01PM +0000, Biju Das wrote:
> The memory allocation core code already prints error message in case of
> OOM. So, drop additional print messages for OOM cases.
>
> While at it, input_register_device() is already printing error messages on
> failure. Drop the redundant print.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
> * New patch.
> ---
> drivers/input/misc/da9063_onkey.c | 23 ++++-------------------
> 1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
> index 9351ce0bb405..80878274204e 100644
> --- a/drivers/input/misc/da9063_onkey.c
> +++ b/drivers/input/misc/da9063_onkey.c
> @@ -185,10 +185,8 @@ static int da9063_onkey_probe(struct platform_device *pdev)
>
> onkey = devm_kzalloc(&pdev->dev, sizeof(struct da9063_onkey),
> GFP_KERNEL);
> - if (!onkey) {
> - dev_err(&pdev->dev, "Failed to allocate memory.\n");
> + if (!onkey)
> return -ENOMEM;
> - }
>
> onkey->config = device_get_match_data(&pdev->dev);
> if (!onkey->config)
> @@ -206,10 +204,8 @@ static int da9063_onkey_probe(struct platform_device *pdev)
> "dlg,disable-key-power");
>
> onkey->input = devm_input_allocate_device(&pdev->dev);
> - if (!onkey->input) {
> - dev_err(&pdev->dev, "Failed to allocated input device.\n");
> + if (!onkey->input)
> return -ENOMEM;
> - }
>
> onkey->input->name = onkey->config->name;
> snprintf(onkey->phys, sizeof(onkey->phys), "%s/input0",
> @@ -221,12 +217,8 @@ static int da9063_onkey_probe(struct platform_device *pdev)
>
> error = devm_delayed_work_autocancel(&pdev->dev, &onkey->work,
> da9063_poll_on);
> - if (error) {
> - dev_err(&pdev->dev,
> - "Failed to add cancel poll action: %d\n",
> - error);
> + if (error)
> return error;
> - }
>
> irq = platform_get_irq_byname(pdev, "ONKEY");
> if (irq < 0)
> @@ -250,14 +242,7 @@ static int da9063_onkey_probe(struct platform_device *pdev)
> else
> device_init_wakeup(&pdev->dev, true);
>
> - error = input_register_device(onkey->input);
> - if (error) {
> - dev_err(&pdev->dev,
> - "Failed to register input device: %d\n", error);
> - return error;
> - }
> -
> - return 0;
> + return input_register_device(onkey->input);
When there are multiple exit points I prefer all of them to use form
error = action(...);
if (error)
return error;
...
return 0;
Fixed up and applied, thank you.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/4] Input: da9063 - Simplify obtaining OF match data
From: Dmitry Torokhov @ 2023-12-19 1:48 UTC (permalink / raw)
To: Biju Das
Cc: Support Opensource, linux-input, Geert Uytterhoeven,
Prabhakar Mahadev Lad, Biju Das, linux-renesas-soc
In-Reply-To: <20231213214803.9931-2-biju.das.jz@bp.renesas.com>
On Wed, Dec 13, 2023 at 09:48:00PM +0000, Biju Das wrote:
> Simplify probe() by replacing of_match_node() for retrieving match data by
> device_get_match_data().
>
> Some minor cleanups:
> * Remove the trailing comma in the terminator entry for the OF
> table making code robust against (theoretical) misrebases or other
> similar things where the new entry goes _after_ the termination without
> the compiler noticing.
> * Move OF table near to the user.
> * Arrange variables in reverse xmas tree order in probe().
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code
From: Guilherme G. Piccoli @ 2023-12-19 0:03 UTC (permalink / raw)
To: Jiri Kosina
Cc: Rahul Rameshbabu, djogorchock, linux-input, benjamin.tissoires,
kernel, kernel-dev
In-Reply-To: <nycvar.YFH.7.76.2312190048380.24250@cbobk.fhfr.pm>
On 18/12/2023 20:49, Jiri Kosina wrote:
> [...]
>
> Not immediately, but if you are able to eventually remove that
> likely-superfluous hunk with a Tested-by: tag, I'll happily merge that
> patch.
>
> Thanks,
>
Of course, I'll do that and mention that it was a suggestion from Rahul!
Thanks,
Guilherme
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code
From: Jiri Kosina @ 2023-12-18 23:49 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: Rahul Rameshbabu, djogorchock, linux-input, benjamin.tissoires,
kernel, kernel-dev
In-Reply-To: <59290abe-c780-5287-d27a-745f4f00ab8a@igalia.com>
On Mon, 18 Dec 2023, Guilherme G. Piccoli wrote:
> > My plan is to send this to Linus in a day or two, to have this fixed for
> > good in 6.7 final to be on the safe side.
> >
> > We can always remove the potentially superfluous check (thanks Rahul for
> > spotting that) later once we get the testing results.
> >
> > Thanks,
> >
>
> Thanks Jiri and Rahul! I agree with the approach, better to have it
> fixed ASAP indeed.
>
> I understand no action is necessary from my side now.
Not immediately, but if you are able to eventually remove that
likely-superfluous hunk with a Tested-by: tag, I'll happily merge that
patch.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code
From: Guilherme G. Piccoli @ 2023-12-18 23:29 UTC (permalink / raw)
To: Jiri Kosina, Rahul Rameshbabu
Cc: djogorchock, linux-input, benjamin.tissoires, kernel, kernel-dev
In-Reply-To: <nycvar.YFH.7.76.2312182325460.24250@cbobk.fhfr.pm>
On 18/12/2023 19:27, Jiri Kosina wrote:
> On Mon, 18 Dec 2023, Guilherme G. Piccoli wrote:
> [...]
> My plan is to send this to Linus in a day or two, to have this fixed for
> good in 6.7 final to be on the safe side.
>
> We can always remove the potentially superfluous check (thanks Rahul for
> spotting that) later once we get the testing results.
>
> Thanks,
>
Thanks Jiri and Rahul! I agree with the approach, better to have it
fixed ASAP indeed.
I understand no action is necessary from my side now.
Cheers,
Guilherme
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code
From: Jiri Kosina @ 2023-12-18 22:27 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: Rahul Rameshbabu, djogorchock, linux-input, benjamin.tissoires,
kernel, kernel-dev
In-Reply-To: <dcd91e66-11ce-c576-5eb7-8756a1b6f222@igalia.com>
On Mon, 18 Dec 2023, Guilherme G. Piccoli wrote:
> I think I see ... I covered both bases in this patch, but IIUC after
> your points above and better looking the code:
>
> (a) imu_avg_delta_ms is set to JC_IMU_DFLT_AVG_DELTA_MS and it can only
> change iff imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG.
>
> (b) But the if path related with the imu_delta_samples_count is the one
> that also guards the divide-by-zero, so effectively that error condition
> cannot happen outside that if path, i.e., my hunk changed nothing.
> Ugh...my bad.
>
> At the same time, the hunk is harmless - it's up to Jiri to decide, we
> can drop it (either directly by git rebasing or I can send a V2 if Jiri
> prefers), or we can keep it.
>
> I can try to test internally, github testing may be a bit uncertain in
> the timeframe (specially during holidays season). If Jiri thinks the
> hunk is harmless and no change is necessary, I'd rather not bother
> people for testing now (I don't have the HW).
My plan is to send this to Linus in a day or two, to have this fixed for
good in 6.7 final to be on the safe side.
We can always remove the potentially superfluous check (thanks Rahul for
spotting that) later once we get the testing results.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code
From: Rahul Rameshbabu @ 2023-12-18 21:56 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: jikos, djogorchock, linux-input, benjamin.tissoires, kernel,
kernel-dev
In-Reply-To: <dcd91e66-11ce-c576-5eb7-8756a1b6f222@igalia.com>
On Mon, 18 Dec, 2023 18:46:09 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:
>
> Hi Rahul, thanks for your review.
>
> I think I see ... I covered both bases in this patch, but IIUC after
> your points above and better looking the code:
>
> (a) imu_avg_delta_ms is set to JC_IMU_DFLT_AVG_DELTA_MS and it can only
> change iff imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG.
>
> (b) But the if path related with the imu_delta_samples_count is the one
> that also guards the divide-by-zero, so effectively that error condition
> cannot happen outside that if path, i.e., my hunk changed nothing.
> Ugh...my bad.
Right, no worries. I really appreciate this patch though for covering
the cases involving the gyro and the accelerometer.
>
> At the same time, the hunk is harmless - it's up to Jiri to decide, we
> can drop it (either directly by git rebasing or I can send a V2 if Jiri
> prefers), or we can keep it.
Agreed.
>
> I can try to test internally, github testing may be a bit uncertain in
> the timeframe (specially during holidays season). If Jiri thinks the
> hunk is harmless and no change is necessary, I'd rather not bother
> people for testing now (I don't have the HW).
>
Makes sense. Like discussed above, the change here is harmless in nature
but has no functional change at the same time.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox