* [PATCH v2 02/10] HID: intel-ish-hid: Hide members of struct ishtp_cl_device
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
ISH clients don't need to access any field of struct ishtp_cl_device. To
avoid this create an interface functions instead where it is required.
In the case of ishtp_cl_allocate(), modify the parameters so that the
clients don't have to dereference.
Clients can also use tracing, here a new interface is added to get the
common trace function pointer, instead of direct call.
The new interface functions defined in one external header file, named
intel-ish-client-if.h. This is the only header files all ISHTP clients
must include.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 56 +++++++++++---------
drivers/hid/intel-ish-hid/ishtp-hid.c | 4 +-
drivers/hid/intel-ish-hid/ishtp-hid.h | 2 +-
drivers/hid/intel-ish-hid/ishtp/bus.c | 27 ++++++++++
drivers/hid/intel-ish-hid/ishtp/client.c | 4 +-
drivers/hid/intel-ish-hid/ishtp/client.h | 2 +-
include/linux/intel-ish-client-if.h | 18 +++++++
7 files changed, 84 insertions(+), 29 deletions(-)
create mode 100644 include/linux/intel-ish-client-if.h
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index b86290611a5f..17d5bd4f52c1 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/hid.h>
+#include <linux/intel-ish-client-if.h>
#include <linux/sched.h>
#include "ishtp/ishtp-dev.h"
#include "ishtp/client.h"
@@ -24,6 +25,8 @@
#define HID_CL_RX_RING_SIZE 32
#define HID_CL_TX_RING_SIZE 16
+#define cl_data_to_dev(client_data) ishtp_device(client_data->cl_device)
+
/**
* report_bad_packets() - Report bad packets
* @hid_ishtp_cl: Client instance to get stats
@@ -39,7 +42,7 @@ static void report_bad_packet(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
struct hostif_msg *recv_msg = recv_buf;
struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
- dev_err(&client_data->cl_device->dev, "[hid-ish]: BAD packet %02X\n"
+ dev_err(cl_data_to_dev(client_data), "[hid-ish]: BAD packet %02X\n"
"total_bad=%u cur_pos=%u\n"
"[%02X %02X %02X %02X]\n"
"payload_len=%u\n"
@@ -85,7 +88,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
do {
if (cur_pos + sizeof(struct hostif_msg) > total_len) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: error, received %u which is less than data header %u\n",
(unsigned int)data_len,
(unsigned int)sizeof(struct hostif_msg_hdr));
@@ -124,12 +127,12 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
client_data->hid_dev_count = (unsigned int)*payload;
if (!client_data->hid_devices)
client_data->hid_devices = devm_kcalloc(
- &client_data->cl_device->dev,
+ cl_data_to_dev(client_data),
client_data->hid_dev_count,
sizeof(struct device_info),
GFP_KERNEL);
if (!client_data->hid_devices) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"Mem alloc failed for hid device info\n");
wake_up_interruptible(&client_data->init_wait);
break;
@@ -137,7 +140,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
for (i = 0; i < client_data->hid_dev_count; ++i) {
if (1 + sizeof(struct device_info) * i >=
payload_len) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: [ENUM_DEVICES]: content size %zu is bigger than payload_len %zu\n",
1 + sizeof(struct device_info)
* i, payload_len);
@@ -172,7 +175,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
}
if (!client_data->hid_descr[curr_hid_dev])
client_data->hid_descr[curr_hid_dev] =
- devm_kmalloc(&client_data->cl_device->dev,
+ devm_kmalloc(cl_data_to_dev(client_data),
payload_len, GFP_KERNEL);
if (client_data->hid_descr[curr_hid_dev]) {
memcpy(client_data->hid_descr[curr_hid_dev],
@@ -197,7 +200,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
}
if (!client_data->report_descr[curr_hid_dev])
client_data->report_descr[curr_hid_dev] =
- devm_kmalloc(&client_data->cl_device->dev,
+ devm_kmalloc(cl_data_to_dev(client_data),
payload_len, GFP_KERNEL);
if (client_data->report_descr[curr_hid_dev]) {
memcpy(client_data->report_descr[curr_hid_dev],
@@ -516,12 +519,12 @@ static int ishtp_enum_enum_devices(struct ishtp_cl *hid_ishtp_cl)
sizeof(struct hostif_msg));
}
if (!client_data->enum_devices_done) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: timed out waiting for enum_devices\n");
return -ETIMEDOUT;
}
if (!client_data->hid_devices) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: failed to allocate HID dev structures\n");
return -ENOMEM;
}
@@ -564,13 +567,13 @@ static int ishtp_get_hid_descriptor(struct ishtp_cl *hid_ishtp_cl, int index)
client_data->hid_descr_done,
3 * HZ);
if (!client_data->hid_descr_done) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: timed out for hid_descr_done\n");
return -EIO;
}
if (!client_data->hid_descr[index]) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: allocation HID desc fail\n");
return -ENOMEM;
}
@@ -611,12 +614,12 @@ static int ishtp_get_report_descriptor(struct ishtp_cl *hid_ishtp_cl,
client_data->report_descr_done,
3 * HZ);
if (!client_data->report_descr_done) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: timed out for report descr\n");
return -EIO;
}
if (!client_data->report_descr[index]) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: failed to alloc report descr\n");
return -ENOMEM;
}
@@ -646,12 +649,12 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
int i;
int rv;
- dev_dbg(&client_data->cl_device->dev, "%s\n", __func__);
+ dev_dbg(cl_data_to_dev(client_data), "%s\n", __func__);
hid_ishtp_trace(client_data, "%s reset flag: %d\n", __func__, reset);
rv = ishtp_cl_link(hid_ishtp_cl, ISHTP_HOST_CLIENT_ID_ANY);
if (rv) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"ishtp_cl_link failed\n");
return -ENOMEM;
}
@@ -666,7 +669,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
fw_client = ishtp_fw_cl_get_client(dev, &hid_ishtp_guid);
if (!fw_client) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"ish client uuid not found\n");
return -ENOENT;
}
@@ -676,7 +679,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
rv = ishtp_cl_connect(hid_ishtp_cl);
if (rv) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"client connect fail\n");
goto err_cl_unlink;
}
@@ -707,7 +710,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
if (!reset) {
rv = ishtp_hid_probe(i, client_data);
if (rv) {
- dev_err(&client_data->cl_device->dev,
+ dev_err(cl_data_to_dev(client_data),
"[hid-ish]: HID probe for #%u failed: %d\n",
i, rv);
goto err_cl_disconnect;
@@ -763,7 +766,7 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
hid_ishtp_cl_deinit(hid_ishtp_cl);
- hid_ishtp_cl = ishtp_cl_allocate(cl_device->ishtp_dev);
+ hid_ishtp_cl = ishtp_cl_allocate(cl_device);
if (!hid_ishtp_cl)
return;
@@ -777,15 +780,17 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
rv = hid_ishtp_cl_init(hid_ishtp_cl, 1);
if (!rv)
break;
- dev_err(&client_data->cl_device->dev, "Retry reset init\n");
+ dev_err(cl_data_to_dev(client_data), "Retry reset init\n");
}
if (rv) {
- dev_err(&client_data->cl_device->dev, "Reset Failed\n");
+ dev_err(cl_data_to_dev(client_data), "Reset Failed\n");
hid_ishtp_trace(client_data, "%s Failed hid_ishtp_cl %p\n",
__func__, hid_ishtp_cl);
}
}
+void (*hid_print_trace)(void *dev, const char *format, ...);
+
/**
* hid_ishtp_cl_probe() - ISHTP client driver probe
* @cl_device: ISHTP client device instance
@@ -803,12 +808,13 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
if (!cl_device)
return -ENODEV;
- client_data = devm_kzalloc(&cl_device->dev, sizeof(*client_data),
+ client_data = devm_kzalloc(ishtp_device(cl_device),
+ sizeof(*client_data),
GFP_KERNEL);
if (!client_data)
return -ENOMEM;
- hid_ishtp_cl = ishtp_cl_allocate(cl_device->ishtp_dev);
+ hid_ishtp_cl = ishtp_cl_allocate(cl_device);
if (!hid_ishtp_cl)
return -ENOMEM;
@@ -822,6 +828,8 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
INIT_WORK(&client_data->work, hid_ishtp_cl_reset_handler);
+ hid_print_trace = ishtp_trace_callback(cl_device);
+
rv = hid_ishtp_cl_init(hid_ishtp_cl, 0);
if (rv) {
ishtp_cl_free(hid_ishtp_cl);
@@ -848,7 +856,7 @@ static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
hid_ishtp_cl);
- dev_dbg(&cl_device->dev, "%s\n", __func__);
+ dev_dbg(ishtp_device(cl_device), "%s\n", __func__);
hid_ishtp_cl->state = ISHTP_CL_DISCONNECTING;
ishtp_cl_disconnect(hid_ishtp_cl);
ishtp_put_device(cl_device);
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c b/drivers/hid/intel-ish-hid/ishtp-hid.c
index 5c7e127b0483..95a5b87d9105 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
@@ -14,6 +14,7 @@
*/
#include <linux/hid.h>
+#include <linux/intel-ish-client-if.h>
#include <uapi/linux/input.h>
#include "ishtp/client.h"
#include "ishtp-hid.h"
@@ -241,7 +242,8 @@ int ishtp_hid_probe(unsigned int cur_hid_dev,
hid->ll_driver = &ishtp_hid_ll_driver;
hid->bus = BUS_INTEL_ISHTP;
- hid->dev.parent = &client_data->cl_device->dev;
+ hid->dev.parent = ishtp_device(client_data->cl_device);
+
hid->version = le16_to_cpu(ISH_HID_VERSION);
hid->vendor = le16_to_cpu(client_data->hid_devices[cur_hid_dev].vid);
hid->product = le16_to_cpu(client_data->hid_devices[cur_hid_dev].pid);
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.h b/drivers/hid/intel-ish-hid/ishtp-hid.h
index 40663639e43b..8af44e855a49 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.h
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
@@ -24,7 +24,7 @@
#define IS_RESPONSE 0x80
/* Used to dump to Linux trace buffer, if enabled */
-#define hid_ishtp_trace(client, ...) \
+#define hid_ishtp_trace(client, ...) \
client->cl_device->ishtp_dev->print_log(\
client->cl_device->ishtp_dev, __VA_ARGS__)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 6348fee8aadc..308853eab91c 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -827,6 +827,33 @@ int ishtp_use_dma_transfer(void)
return ishtp_use_dma;
}
+/**
+ * ishtp_device() - Return device pointer
+ *
+ * This interface is used to return device pointer from ishtp_cl_device
+ * instance.
+ *
+ * Return: device *.
+ */
+struct device *ishtp_device(struct ishtp_cl_device *device)
+{
+ return &device->dev;
+}
+EXPORT_SYMBOL(ishtp_device);
+
+/**
+ * ishtp_trace_callback() - Return trace callback
+ *
+ * This interface is used to return trace callback function pointer.
+ *
+ * Return: void *.
+ */
+void *ishtp_trace_callback(struct ishtp_cl_device *cl_device)
+{
+ return cl_device->ishtp_dev->print_log;
+}
+EXPORT_SYMBOL(ishtp_trace_callback);
+
/**
* ishtp_bus_register() - Function to register bus
*
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index faeccdb1475b..760e48a368a8 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -126,7 +126,7 @@ static void ishtp_cl_init(struct ishtp_cl *cl, struct ishtp_device *dev)
*
* Return: The allocated client instance or NULL on failure
*/
-struct ishtp_cl *ishtp_cl_allocate(struct ishtp_device *dev)
+struct ishtp_cl *ishtp_cl_allocate(struct ishtp_cl_device *cl_device)
{
struct ishtp_cl *cl;
@@ -134,7 +134,7 @@ struct ishtp_cl *ishtp_cl_allocate(struct ishtp_device *dev)
if (!cl)
return NULL;
- ishtp_cl_init(cl, dev);
+ ishtp_cl_init(cl, cl_device->ishtp_dev);
return cl;
}
EXPORT_SYMBOL(ishtp_cl_allocate);
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.h b/drivers/hid/intel-ish-hid/ishtp/client.h
index e0df3eb611e6..992625891a6c 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.h
+++ b/drivers/hid/intel-ish-hid/ishtp/client.h
@@ -170,7 +170,7 @@ static inline bool ishtp_cl_cmp_id(const struct ishtp_cl *cl1,
}
/* exported functions from ISHTP under client management scope */
-struct ishtp_cl *ishtp_cl_allocate(struct ishtp_device *dev);
+struct ishtp_cl *ishtp_cl_allocate(struct ishtp_cl_device *cl_device);
void ishtp_cl_free(struct ishtp_cl *cl);
int ishtp_cl_link(struct ishtp_cl *cl, int id);
void ishtp_cl_unlink(struct ishtp_cl *cl);
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
new file mode 100644
index 000000000000..11e285172735
--- /dev/null
+++ b/include/linux/intel-ish-client-if.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Intel ISH client Interface definitions
+ *
+ * Copyright (c) 2019, Intel Corporation.
+ */
+
+#ifndef _INTEL_ISH_CLIENT_IF_H_
+#define _INTEL_ISH_CLIENT_IF_H_
+
+struct ishtp_cl_device;
+
+/* Get the device * from ishtp device instance */
+struct device *ishtp_device(struct ishtp_cl_device *cl_device);
+/* Trace interface for clients */
+void *ishtp_trace_callback(struct ishtp_cl_device *cl_device);
+
+#endif /* _INTEL_ISH_CLIENT_IF_H_ */
--
2.17.2
^ permalink raw reply related
* [PATCH v2 03/10] HID: intel-ish-hid: Simplify ishtp_cl_link()
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
All callers will only use ISHTP_HOST_CLIENT_ID_ANY, so get rid of
option to pass this additional id.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 2 +-
drivers/hid/intel-ish-hid/ishtp/client.c | 14 ++++----------
drivers/hid/intel-ish-hid/ishtp/client.h | 2 +-
3 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 17d5bd4f52c1..94a00ffeb09b 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -652,7 +652,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
dev_dbg(cl_data_to_dev(client_data), "%s\n", __func__);
hid_ishtp_trace(client_data, "%s reset flag: %d\n", __func__, reset);
- rv = ishtp_cl_link(hid_ishtp_cl, ISHTP_HOST_CLIENT_ID_ANY);
+ rv = ishtp_cl_link(hid_ishtp_cl);
if (rv) {
dev_err(cl_data_to_dev(client_data),
"ishtp_cl_link failed\n");
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 760e48a368a8..657b46dcefa6 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -168,9 +168,6 @@ EXPORT_SYMBOL(ishtp_cl_free);
/**
* ishtp_cl_link() - Reserve a host id and link the client instance
* @cl: client device instance
- * @id: host client id to use. It can be ISHTP_HOST_CLIENT_ID_ANY if any
- * id from the available can be used
- *
*
* This allocates a single bit in the hostmap. This function will make sure
* that not many client sessions are opened at the same time. Once allocated
@@ -179,11 +176,11 @@ EXPORT_SYMBOL(ishtp_cl_free);
*
* Return: 0 or error code on failure
*/
-int ishtp_cl_link(struct ishtp_cl *cl, int id)
+int ishtp_cl_link(struct ishtp_cl *cl)
{
struct ishtp_device *dev;
- unsigned long flags, flags_cl;
- int ret = 0;
+ unsigned long flags, flags_cl;
+ int id, ret = 0;
if (WARN_ON(!cl || !cl->dev))
return -EINVAL;
@@ -197,10 +194,7 @@ int ishtp_cl_link(struct ishtp_cl *cl, int id)
goto unlock_dev;
}
- /* If Id is not assigned get one*/
- if (id == ISHTP_HOST_CLIENT_ID_ANY)
- id = find_first_zero_bit(dev->host_clients_map,
- ISHTP_CLIENTS_MAX);
+ id = find_first_zero_bit(dev->host_clients_map, ISHTP_CLIENTS_MAX);
if (id >= ISHTP_CLIENTS_MAX) {
spin_unlock_irqrestore(&dev->device_lock, flags);
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.h b/drivers/hid/intel-ish-hid/ishtp/client.h
index 992625891a6c..afa8b1f521d0 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.h
+++ b/drivers/hid/intel-ish-hid/ishtp/client.h
@@ -172,7 +172,7 @@ static inline bool ishtp_cl_cmp_id(const struct ishtp_cl *cl1,
/* exported functions from ISHTP under client management scope */
struct ishtp_cl *ishtp_cl_allocate(struct ishtp_cl_device *cl_device);
void ishtp_cl_free(struct ishtp_cl *cl);
-int ishtp_cl_link(struct ishtp_cl *cl, int id);
+int ishtp_cl_link(struct ishtp_cl *cl);
void ishtp_cl_unlink(struct ishtp_cl *cl);
int ishtp_cl_disconnect(struct ishtp_cl *cl);
int ishtp_cl_connect(struct ishtp_cl *cl);
--
2.17.2
^ permalink raw reply related
* [PATCH v2 04/10] HID: intel-ish-hid: Move driver registry functions
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Move the driver registry with the ishtp bus to the common interface
file, which clients can include.
Also rename __ishtp_cl_driver_register() to ishtp_cl_driver_register()
and removed define for ishtp_cl_driver_register.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 2 +-
drivers/hid/intel-ish-hid/ishtp/bus.c | 8 +++---
drivers/hid/intel-ish-hid/ishtp/bus.h | 27 +-------------------
include/linux/intel-ish-client-if.h | 25 ++++++++++++++++++
4 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 94a00ffeb09b..4afb39713213 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -953,7 +953,7 @@ static int __init ish_hid_init(void)
int rv;
/* Register ISHTP client device driver with ISHTP Bus */
- rv = ishtp_cl_driver_register(&hid_ishtp_cl_driver);
+ rv = ishtp_cl_driver_register(&hid_ishtp_cl_driver, THIS_MODULE);
return rv;
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 308853eab91c..95a97534fcda 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -485,7 +485,7 @@ static void ishtp_bus_remove_device(struct ishtp_cl_device *device)
}
/**
- * __ishtp_cl_driver_register() - Client driver register
+ * ishtp_cl_driver_register() - Client driver register
* @driver: the client driver instance
* @owner: Owner of this driver module
*
@@ -494,8 +494,8 @@ static void ishtp_bus_remove_device(struct ishtp_cl_device *device)
*
* Return: Return value of driver_register or -ENODEV if not ready
*/
-int __ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
- struct module *owner)
+int ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
+ struct module *owner)
{
int err;
@@ -512,7 +512,7 @@ int __ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
return 0;
}
-EXPORT_SYMBOL(__ishtp_cl_driver_register);
+EXPORT_SYMBOL(ishtp_cl_driver_register);
/**
* ishtp_cl_driver_unregister() - Client driver unregister
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.h b/drivers/hid/intel-ish-hid/ishtp/bus.h
index c96e7fb42f01..4aed195719de 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.h
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.h
@@ -17,6 +17,7 @@
#include <linux/device.h>
#include <linux/mod_devicetable.h>
+#include <linux/intel-ish-client-if.h>
struct ishtp_cl;
struct ishtp_cl_device;
@@ -52,26 +53,6 @@ struct ishtp_cl_device {
void (*event_cb)(struct ishtp_cl_device *device);
};
-/**
- * struct ishtp_cl_device - ISHTP device handle
- * @driver: driver instance on a bus
- * @name: Name of the device for probe
- * @probe: driver callback for device probe
- * @remove: driver callback on device removal
- *
- * Client drivers defines to get probed/removed for ISHTP client device.
- */
-struct ishtp_cl_driver {
- struct device_driver driver;
- const char *name;
- const guid_t *guid;
- int (*probe)(struct ishtp_cl_device *dev);
- int (*remove)(struct ishtp_cl_device *dev);
- int (*reset)(struct ishtp_cl_device *dev);
- const struct dev_pm_ops *pm;
-};
-
-
int ishtp_bus_new_client(struct ishtp_device *dev);
void ishtp_remove_all_clients(struct ishtp_device *dev);
int ishtp_cl_device_bind(struct ishtp_cl *cl);
@@ -105,12 +86,6 @@ void ishtp_get_device(struct ishtp_cl_device *);
void ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void *data);
void *ishtp_get_drvdata(struct ishtp_cl_device *cl_device);
-int __ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
- struct module *owner);
-#define ishtp_cl_driver_register(driver) \
- __ishtp_cl_driver_register(driver, THIS_MODULE)
-void ishtp_cl_driver_unregister(struct ishtp_cl_driver *driver);
-
int ishtp_register_event_cb(struct ishtp_cl_device *device,
void (*read_cb)(struct ishtp_cl_device *));
int ishtp_fw_cl_by_uuid(struct ishtp_device *dev, const guid_t *cuuid);
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index 11e285172735..abc0b8122f07 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -10,6 +10,31 @@
struct ishtp_cl_device;
+/**
+ * struct ishtp_cl_device - ISHTP device handle
+ * @driver: driver instance on a bus
+ * @name: Name of the device for probe
+ * @probe: driver callback for device probe
+ * @remove: driver callback on device removal
+ *
+ * Client drivers defines to get probed/removed for ISHTP client device.
+ */
+struct ishtp_cl_driver {
+ struct device_driver driver;
+ const char *name;
+ const guid_t *guid;
+ int (*probe)(struct ishtp_cl_device *dev);
+ int (*remove)(struct ishtp_cl_device *dev);
+ int (*reset)(struct ishtp_cl_device *dev);
+ const struct dev_pm_ops *pm;
+};
+
+int ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
+ struct module *owner);
+void ishtp_cl_driver_unregister(struct ishtp_cl_driver *driver);
+int ishtp_register_event_cb(struct ishtp_cl_device *device,
+ void (*read_cb)(struct ishtp_cl_device *));
+
/* Get the device * from ishtp device instance */
struct device *ishtp_device(struct ishtp_cl_device *cl_device);
/* Trace interface for clients */
--
2.17.2
^ permalink raw reply related
* [PATCH v2 05/10] HID: intel-ish-hid: Store ishtp_cl_device instance in device
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Store ishtp_cl_device pointer in device struct private data. In this
way we can get ishtp_cl_device * from device struct pointer.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 95a97534fcda..2ca65864192f 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -466,6 +466,7 @@ static struct ishtp_cl_device *ishtp_bus_add_device(struct ishtp_device *dev,
}
ishtp_device_ready = true;
+ dev_set_drvdata(&device->dev, device);
return device;
}
--
2.17.2
^ permalink raw reply related
* [PATCH v2 06/10] HID: intel-ish-hid: Move the common functions from client.h
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Move the interface functions in client.h to common include. These are
already abstracted well to use as is. Also move any associated structures
used by these functions.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp/client.h | 24 -----------
drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 26 -----------
include/linux/intel-ish-client-if.h | 48 +++++++++++++++++++++
3 files changed, 48 insertions(+), 50 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.h b/drivers/hid/intel-ish-hid/ishtp/client.h
index afa8b1f521d0..6ed00947d6bc 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.h
+++ b/drivers/hid/intel-ish-hid/ishtp/client.h
@@ -19,15 +19,6 @@
#include <linux/types.h>
#include "ishtp-dev.h"
-/* Client state */
-enum cl_state {
- ISHTP_CL_INITIALIZING = 0,
- ISHTP_CL_CONNECTING,
- ISHTP_CL_CONNECTED,
- ISHTP_CL_DISCONNECTING,
- ISHTP_CL_DISCONNECTED
-};
-
/* Tx and Rx ring size */
#define CL_DEF_RX_RING_SIZE 2
#define CL_DEF_TX_RING_SIZE 2
@@ -169,19 +160,4 @@ static inline bool ishtp_cl_cmp_id(const struct ishtp_cl *cl1,
(cl1->fw_client_id == cl2->fw_client_id);
}
-/* exported functions from ISHTP under client management scope */
-struct ishtp_cl *ishtp_cl_allocate(struct ishtp_cl_device *cl_device);
-void ishtp_cl_free(struct ishtp_cl *cl);
-int ishtp_cl_link(struct ishtp_cl *cl);
-void ishtp_cl_unlink(struct ishtp_cl *cl);
-int ishtp_cl_disconnect(struct ishtp_cl *cl);
-int ishtp_cl_connect(struct ishtp_cl *cl);
-int ishtp_cl_send(struct ishtp_cl *cl, uint8_t *buf, size_t length);
-int ishtp_cl_flush_queues(struct ishtp_cl *cl);
-
-/* exported functions from ISHTP client buffer management scope */
-int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb);
-bool ishtp_cl_tx_empty(struct ishtp_cl *cl);
-struct ishtp_cl_rb *ishtp_cl_rx_get_rb(struct ishtp_cl *cl);
-
#endif /* _ISHTP_CLIENT_H_ */
diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
index e54ce1ef27dd..e0a320e67a41 100644
--- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
+++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
@@ -79,32 +79,6 @@ struct ishtp_fw_client {
uint8_t client_id;
};
-/**
- * struct ishtp_msg_data - ISHTP message data struct
- * @size: Size of data in the *data
- * @data: Pointer to data
- */
-struct ishtp_msg_data {
- uint32_t size;
- unsigned char *data;
-};
-
-/*
- * struct ishtp_cl_rb - request block structure
- * @list: Link to list members
- * @cl: ISHTP client instance
- * @buffer: message header
- * @buf_idx: Index into buffer
- * @read_time: unused at this time
- */
-struct ishtp_cl_rb {
- struct list_head list;
- struct ishtp_cl *cl;
- struct ishtp_msg_data buffer;
- unsigned long buf_idx;
- unsigned long read_time;
-};
-
/*
* Control info for IPC messages ISHTP/IPC sending FIFO -
* list with inline data buffer
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index abc0b8122f07..7ce172f656f8 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -9,6 +9,16 @@
#define _INTEL_ISH_CLIENT_IF_H_
struct ishtp_cl_device;
+struct ishtp_cl;
+
+/* Client state */
+enum cl_state {
+ ISHTP_CL_INITIALIZING = 0,
+ ISHTP_CL_CONNECTING,
+ ISHTP_CL_CONNECTED,
+ ISHTP_CL_DISCONNECTING,
+ ISHTP_CL_DISCONNECTED
+};
/**
* struct ishtp_cl_device - ISHTP device handle
@@ -29,6 +39,32 @@ struct ishtp_cl_driver {
const struct dev_pm_ops *pm;
};
+/**
+ * struct ishtp_msg_data - ISHTP message data struct
+ * @size: Size of data in the *data
+ * @data: Pointer to data
+ */
+struct ishtp_msg_data {
+ uint32_t size;
+ unsigned char *data;
+};
+
+/*
+ * struct ishtp_cl_rb - request block structure
+ * @list: Link to list members
+ * @cl: ISHTP client instance
+ * @buffer: message header
+ * @buf_idx: Index into buffer
+ * @read_time: unused at this time
+ */
+struct ishtp_cl_rb {
+ struct list_head list;
+ struct ishtp_cl *cl;
+ struct ishtp_msg_data buffer;
+ unsigned long buf_idx;
+ unsigned long read_time;
+};
+
int ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
struct module *owner);
void ishtp_cl_driver_unregister(struct ishtp_cl_driver *driver);
@@ -40,4 +76,16 @@ struct device *ishtp_device(struct ishtp_cl_device *cl_device);
/* Trace interface for clients */
void *ishtp_trace_callback(struct ishtp_cl_device *cl_device);
+struct ishtp_cl *ishtp_cl_allocate(struct ishtp_cl_device *cl_device);
+void ishtp_cl_free(struct ishtp_cl *cl);
+int ishtp_cl_link(struct ishtp_cl *cl);
+void ishtp_cl_unlink(struct ishtp_cl *cl);
+int ishtp_cl_disconnect(struct ishtp_cl *cl);
+int ishtp_cl_connect(struct ishtp_cl *cl);
+int ishtp_cl_send(struct ishtp_cl *cl, uint8_t *buf, size_t length);
+int ishtp_cl_flush_queues(struct ishtp_cl *cl);
+int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb);
+bool ishtp_cl_tx_empty(struct ishtp_cl *cl);
+struct ishtp_cl_rb *ishtp_cl_rx_get_rb(struct ishtp_cl *cl);
+
#endif /* _INTEL_ISH_CLIENT_IF_H_ */
--
2.17.2
^ permalink raw reply related
* [PATCH v2 07/10] HID: intel-ish-hid: Add interface functions for struct ishtp_cl
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Instead of directly accessing members of struct ishtp_cl, create interface
functions to access them.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp/client.c | 42 ++++++++++++++++++++++++
include/linux/intel-ish-client-if.h | 7 ++++
2 files changed, 49 insertions(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index 657b46dcefa6..b7ac5e3b1e82 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -1063,3 +1063,45 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
eoi:
return;
}
+
+void *ishtp_get_client_data(struct ishtp_cl *cl)
+{
+ return cl->client_data;
+}
+EXPORT_SYMBOL(ishtp_get_client_data);
+
+void ishtp_set_client_data(struct ishtp_cl *cl, void *data)
+{
+ cl->client_data = data;
+}
+EXPORT_SYMBOL(ishtp_set_client_data);
+
+struct ishtp_device *ishtp_get_ishtp_device(struct ishtp_cl *cl)
+{
+ return cl->dev;
+}
+EXPORT_SYMBOL(ishtp_get_ishtp_device);
+
+void ishtp_set_tx_ring_size(struct ishtp_cl *cl, int size)
+{
+ cl->tx_ring_size = size;
+}
+EXPORT_SYMBOL(ishtp_set_tx_ring_size);
+
+void ishtp_set_rx_ring_size(struct ishtp_cl *cl, int size)
+{
+ cl->rx_ring_size = size;
+}
+EXPORT_SYMBOL(ishtp_set_rx_ring_size);
+
+void ishtp_set_connection_state(struct ishtp_cl *cl, int state)
+{
+ cl->state = state;
+}
+EXPORT_SYMBOL(ishtp_set_connection_state);
+
+void ishtp_cl_set_fw_client_id(struct ishtp_cl *cl, int fw_client_id)
+{
+ cl->fw_client_id = fw_client_id;
+}
+EXPORT_SYMBOL(ishtp_cl_set_fw_client_id);
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index 7ce172f656f8..526e3048e09f 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -87,5 +87,12 @@ int ishtp_cl_flush_queues(struct ishtp_cl *cl);
int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb);
bool ishtp_cl_tx_empty(struct ishtp_cl *cl);
struct ishtp_cl_rb *ishtp_cl_rx_get_rb(struct ishtp_cl *cl);
+void *ishtp_get_client_data(struct ishtp_cl *cl);
+void ishtp_set_client_data(struct ishtp_cl *cl, void *data);
+struct ishtp_device *ishtp_get_ishtp_device(struct ishtp_cl *cl);
+void ishtp_set_tx_ring_size(struct ishtp_cl *cl, int size);
+void ishtp_set_rx_ring_size(struct ishtp_cl *cl, int size);
+void ishtp_set_connection_state(struct ishtp_cl *cl, int state);
+void ishtp_cl_set_fw_client_id(struct ishtp_cl *cl, int fw_client_id);
#endif /* _INTEL_ISH_CLIENT_IF_H_ */
--
2.17.2
^ permalink raw reply related
* [PATCH v2 08/10] HID: intel-ish-hid: Move functions related to bus and device
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Move function idefinitions related to bus and device to common header file.
Also create new function to get fw client id and move ish_hw_reset() from
inline to exported function.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 26 +++++++++++++++++++++
drivers/hid/intel-ish-hid/ishtp/bus.h | 11 ---------
drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 5 ----
include/linux/intel-ish-client-if.h | 12 ++++++++++
4 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 2ca65864192f..66a485a41481 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -170,6 +170,19 @@ struct ishtp_fw_client *ishtp_fw_cl_get_client(struct ishtp_device *dev,
}
EXPORT_SYMBOL(ishtp_fw_cl_get_client);
+/**
+ * ishtp_get_fw_client_id() - Get fw client id
+ *
+ * This interface is used to reset HW get FW client id.
+ *
+ * Return: firmware client id.
+ */
+int ishtp_get_fw_client_id(struct ishtp_fw_client *fw_client)
+{
+ return fw_client->client_id;
+}
+EXPORT_SYMBOL(ishtp_get_fw_client_id);
+
/**
* ishtp_fw_cl_by_id() - return index to fw_clients for client_id
* @dev: the ishtp device structure
@@ -855,6 +868,19 @@ void *ishtp_trace_callback(struct ishtp_cl_device *cl_device)
}
EXPORT_SYMBOL(ishtp_trace_callback);
+/**
+ * ish_hw_reset() - Call HW reset IPC callback
+ *
+ * This interface is used to reset HW in case of error.
+ *
+ * Return: value from IPC hw_reset callback
+ */
+int ish_hw_reset(struct ishtp_device *dev)
+{
+ return dev->ops->hw_reset(dev);
+}
+EXPORT_SYMBOL(ish_hw_reset);
+
/**
* ishtp_bus_register() - Function to register bus
*
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.h b/drivers/hid/intel-ish-hid/ishtp/bus.h
index 4aed195719de..93d516f5a853 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.h
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.h
@@ -80,16 +80,5 @@ void ishtp_recv(struct ishtp_device *dev);
void ishtp_reset_handler(struct ishtp_device *dev);
void ishtp_reset_compl_handler(struct ishtp_device *dev);
-void ishtp_put_device(struct ishtp_cl_device *);
-void ishtp_get_device(struct ishtp_cl_device *);
-
-void ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void *data);
-void *ishtp_get_drvdata(struct ishtp_cl_device *cl_device);
-
-int ishtp_register_event_cb(struct ishtp_cl_device *device,
- void (*read_cb)(struct ishtp_cl_device *));
int ishtp_fw_cl_by_uuid(struct ishtp_device *dev, const guid_t *cuuid);
-struct ishtp_fw_client *ishtp_fw_cl_get_client(struct ishtp_device *dev,
- const guid_t *uuid);
-
#endif /* _LINUX_ISHTP_CL_BUS_H */
diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
index e0a320e67a41..3cfef084b9fc 100644
--- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
+++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
@@ -238,11 +238,6 @@ static inline int ish_ipc_reset(struct ishtp_device *dev)
return dev->ops->ipc_reset(dev);
}
-static inline int ish_hw_reset(struct ishtp_device *dev)
-{
- return dev->ops->hw_reset(dev);
-}
-
/* Exported function */
void ishtp_device_init(struct ishtp_device *dev);
int ishtp_start(struct ishtp_device *dev);
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index 526e3048e09f..e98bfbb1e07e 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -9,7 +9,9 @@
#define _INTEL_ISH_CLIENT_IF_H_
struct ishtp_cl_device;
+struct ishtp_device;
struct ishtp_cl;
+struct ishtp_fw_client;
/* Client state */
enum cl_state {
@@ -95,4 +97,14 @@ void ishtp_set_rx_ring_size(struct ishtp_cl *cl, int size);
void ishtp_set_connection_state(struct ishtp_cl *cl, int state);
void ishtp_cl_set_fw_client_id(struct ishtp_cl *cl, int fw_client_id);
+void ishtp_put_device(struct ishtp_cl_device *cl_dev);
+void ishtp_get_device(struct ishtp_cl_device *cl_dev);
+void ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void *data);
+void *ishtp_get_drvdata(struct ishtp_cl_device *cl_device);
+int ishtp_register_event_cb(struct ishtp_cl_device *device,
+ void (*read_cb)(struct ishtp_cl_device *));
+struct ishtp_fw_client *ishtp_fw_cl_get_client(struct ishtp_device *dev,
+ const guid_t *uuid);
+int ishtp_get_fw_client_id(struct ishtp_fw_client *fw_client);
+int ish_hw_reset(struct ishtp_device *dev);
#endif /* _INTEL_ISH_CLIENT_IF_H_ */
--
2.17.2
^ permalink raw reply related
* [PATCH v2 09/10] HID: intel-ish-hid: Use the new interface functions in HID ish client
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Only include intel-ish-client-if.h, which has all interfaces required to
implment ISHTP client. There is no longer any direct field access from
core ISHTP only include files.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp-hid-client.c | 68 +++++++++-----------
drivers/hid/intel-ish-hid/ishtp-hid.c | 2 -
drivers/hid/intel-ish-hid/ishtp-hid.h | 6 +-
3 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 4afb39713213..56777a43e69c 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -17,8 +17,6 @@
#include <linux/hid.h>
#include <linux/intel-ish-client-if.h>
#include <linux/sched.h>
-#include "ishtp/ishtp-dev.h"
-#include "ishtp/client.h"
#include "ishtp-hid.h"
/* Rx ring buffer pool size */
@@ -40,7 +38,7 @@ static void report_bad_packet(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
size_t cur_pos, size_t payload_len)
{
struct hostif_msg *recv_msg = recv_buf;
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
dev_err(cl_data_to_dev(client_data), "[hid-ish]: BAD packet %02X\n"
"total_bad=%u cur_pos=%u\n"
@@ -77,7 +75,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
struct report_list *reports_list;
char *reports;
size_t report_len;
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
int curr_hid_dev = client_data->cur_hid_dev;
struct ishtp_hid_data *hid_data = NULL;
struct hid_device *hid = NULL;
@@ -93,7 +91,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
(unsigned int)data_len,
(unsigned int)sizeof(struct hostif_msg_hdr));
++client_data->bad_recv_cnt;
- ish_hw_reset(hid_ishtp_cl->dev);
+ ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
@@ -106,7 +104,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
++client_data->bad_recv_cnt;
report_bad_packet(hid_ishtp_cl, recv_msg, cur_pos,
payload_len);
- ish_hw_reset(hid_ishtp_cl->dev);
+ ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
@@ -121,7 +119,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
report_bad_packet(hid_ishtp_cl, recv_msg,
cur_pos,
payload_len);
- ish_hw_reset(hid_ishtp_cl->dev);
+ ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
client_data->hid_dev_count = (unsigned int)*payload;
@@ -170,7 +168,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
report_bad_packet(hid_ishtp_cl, recv_msg,
cur_pos,
payload_len);
- ish_hw_reset(hid_ishtp_cl->dev);
+ ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
if (!client_data->hid_descr[curr_hid_dev])
@@ -195,7 +193,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
report_bad_packet(hid_ishtp_cl, recv_msg,
cur_pos,
payload_len);
- ish_hw_reset(hid_ishtp_cl->dev);
+ ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
if (!client_data->report_descr[curr_hid_dev])
@@ -313,7 +311,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
++client_data->bad_recv_cnt;
report_bad_packet(hid_ishtp_cl, recv_msg, cur_pos,
payload_len);
- ish_hw_reset(hid_ishtp_cl->dev);
+ ish_hw_reset(ishtp_get_ishtp_device(hid_ishtp_cl));
break;
}
@@ -493,7 +491,7 @@ int ishtp_hid_link_ready_wait(struct ishtp_cl_data *client_data)
static int ishtp_enum_enum_devices(struct ishtp_cl *hid_ishtp_cl)
{
struct hostif_msg msg;
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
int retry_count;
int rv;
@@ -530,7 +528,7 @@ static int ishtp_enum_enum_devices(struct ishtp_cl *hid_ishtp_cl)
}
client_data->num_hid_devices = client_data->hid_dev_count;
- dev_info(&hid_ishtp_cl->device->dev,
+ dev_info(ishtp_device(client_data->cl_device),
"[hid-ish]: enum_devices_done OK, num_hid_devices=%d\n",
client_data->num_hid_devices);
@@ -549,7 +547,7 @@ static int ishtp_enum_enum_devices(struct ishtp_cl *hid_ishtp_cl)
static int ishtp_get_hid_descriptor(struct ishtp_cl *hid_ishtp_cl, int index)
{
struct hostif_msg msg;
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
int rv;
/* Get HID descriptor */
@@ -596,7 +594,7 @@ static int ishtp_get_report_descriptor(struct ishtp_cl *hid_ishtp_cl,
int index)
{
struct hostif_msg msg;
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
int rv;
/* Get report descriptor */
@@ -644,7 +642,7 @@ static int ishtp_get_report_descriptor(struct ishtp_cl *hid_ishtp_cl,
static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
{
struct ishtp_device *dev;
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
struct ishtp_fw_client *fw_client;
int i;
int rv;
@@ -661,11 +659,11 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
client_data->init_done = 0;
- dev = hid_ishtp_cl->dev;
+ dev = ishtp_get_ishtp_device(hid_ishtp_cl);
/* Connect to FW client */
- hid_ishtp_cl->rx_ring_size = HID_CL_RX_RING_SIZE;
- hid_ishtp_cl->tx_ring_size = HID_CL_TX_RING_SIZE;
+ ishtp_set_tx_ring_size(hid_ishtp_cl, HID_CL_TX_RING_SIZE);
+ ishtp_set_rx_ring_size(hid_ishtp_cl, HID_CL_RX_RING_SIZE);
fw_client = ishtp_fw_cl_get_client(dev, &hid_ishtp_guid);
if (!fw_client) {
@@ -673,9 +671,9 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
"ish client uuid not found\n");
return -ENOENT;
}
-
- hid_ishtp_cl->fw_client_id = fw_client->client_id;
- hid_ishtp_cl->state = ISHTP_CL_CONNECTING;
+ ishtp_cl_set_fw_client_id(hid_ishtp_cl,
+ ishtp_get_fw_client_id(fw_client));
+ ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_CONNECTING);
rv = ishtp_cl_connect(hid_ishtp_cl);
if (rv) {
@@ -687,7 +685,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
hid_ishtp_trace(client_data, "%s client connected\n", __func__);
/* Register read callback */
- ishtp_register_event_cb(hid_ishtp_cl->device, ish_cl_event_cb);
+ ishtp_register_event_cb(client_data->cl_device, ish_cl_event_cb);
rv = ishtp_enum_enum_devices(hid_ishtp_cl);
if (rv)
@@ -725,7 +723,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
return 0;
err_cl_disconnect:
- hid_ishtp_cl->state = ISHTP_CL_DISCONNECTING;
+ ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_DISCONNECTING);
ishtp_cl_disconnect(hid_ishtp_cl);
err_cl_unlink:
ishtp_cl_unlink(hid_ishtp_cl);
@@ -762,7 +760,7 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
hid_ishtp_cl);
- dev_dbg(&cl_device->dev, "%s\n", __func__);
+ dev_dbg(ishtp_device(client_data->cl_device), "%s\n", __func__);
hid_ishtp_cl_deinit(hid_ishtp_cl);
@@ -771,7 +769,7 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
return;
ishtp_set_drvdata(cl_device, hid_ishtp_cl);
- hid_ishtp_cl->client_data = client_data;
+ ishtp_set_client_data(hid_ishtp_cl, client_data);
client_data->hid_ishtp_cl = hid_ishtp_cl;
client_data->num_hid_devices = 0;
@@ -789,7 +787,7 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
}
}
-void (*hid_print_trace)(void *dev, const char *format, ...);
+void (*hid_print_trace)(void *unused, const char *format, ...);
/**
* hid_ishtp_cl_probe() - ISHTP client driver probe
@@ -819,7 +817,7 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
return -ENOMEM;
ishtp_set_drvdata(cl_device, hid_ishtp_cl);
- hid_ishtp_cl->client_data = client_data;
+ ishtp_set_client_data(hid_ishtp_cl, client_data);
client_data->hid_ishtp_cl = hid_ishtp_cl;
client_data->cl_device = cl_device;
@@ -851,13 +849,13 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
{
struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
hid_ishtp_cl);
dev_dbg(ishtp_device(cl_device), "%s\n", __func__);
- hid_ishtp_cl->state = ISHTP_CL_DISCONNECTING;
+ ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_DISCONNECTING);
ishtp_cl_disconnect(hid_ishtp_cl);
ishtp_put_device(cl_device);
ishtp_hid_remove(client_data);
@@ -881,7 +879,7 @@ static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
static int hid_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
{
struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
hid_ishtp_cl);
@@ -891,8 +889,6 @@ static int hid_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
return 0;
}
-#define to_ishtp_cl_device(d) container_of(d, struct ishtp_cl_device, dev)
-
/**
* hid_ishtp_cl_suspend() - ISHTP client driver suspend
* @device: device instance
@@ -903,9 +899,9 @@ static int hid_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
*/
static int hid_ishtp_cl_suspend(struct device *device)
{
- struct ishtp_cl_device *cl_device = to_ishtp_cl_device(device);
+ struct ishtp_cl_device *cl_device = dev_get_drvdata(device);
struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
hid_ishtp_cl);
@@ -924,9 +920,9 @@ static int hid_ishtp_cl_suspend(struct device *device)
*/
static int hid_ishtp_cl_resume(struct device *device)
{
- struct ishtp_cl_device *cl_device = to_ishtp_cl_device(device);
+ struct ishtp_cl_device *cl_device = dev_get_drvdata(device);
struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
- struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+ struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl);
hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
hid_ishtp_cl);
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c b/drivers/hid/intel-ish-hid/ishtp-hid.c
index 95a5b87d9105..62c03561adaa 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
@@ -16,7 +16,6 @@
#include <linux/hid.h>
#include <linux/intel-ish-client-if.h>
#include <uapi/linux/input.h>
-#include "ishtp/client.h"
#include "ishtp-hid.h"
/**
@@ -154,7 +153,6 @@ static void ishtp_hid_request(struct hid_device *hid, struct hid_report *rep,
static int ishtp_wait_for_response(struct hid_device *hid)
{
struct ishtp_hid_data *hid_data = hid->driver_data;
- struct ishtp_cl_data *client_data = hid_data->client_data;
int rv;
hid_ishtp_trace(client_data, "%s hid %p\n", __func__, hid);
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.h b/drivers/hid/intel-ish-hid/ishtp-hid.h
index 8af44e855a49..e27d3d6c1920 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.h
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
@@ -24,9 +24,9 @@
#define IS_RESPONSE 0x80
/* Used to dump to Linux trace buffer, if enabled */
-#define hid_ishtp_trace(client, ...) \
- client->cl_device->ishtp_dev->print_log(\
- client->cl_device->ishtp_dev, __VA_ARGS__)
+extern void (*hid_print_trace)(void *unused, const char *format, ...);
+#define hid_ishtp_trace(client, ...) \
+ (hid_print_trace)(NULL, __VA_ARGS__)
/* ISH Transport protocol (ISHTP in short) GUID */
static const guid_t hid_ishtp_guid =
--
2.17.2
^ permalink raw reply related
* [PATCH v2 10/10] HID: intel-ish-hid: Add interface function for PCI device pointer
From: Srinivas Pandruvada @ 2019-03-18 19:14 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Srinivas Pandruvada
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
Instead of directly accessing PCI device poitner via struct ishtp_cl,
create interface function for same. This is required for DMA transfer.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 13 +++++++++++++
include/linux/intel-ish-client-if.h | 2 ++
2 files changed, 15 insertions(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 66a485a41481..fb8ca12955b4 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -855,6 +855,19 @@ struct device *ishtp_device(struct ishtp_cl_device *device)
}
EXPORT_SYMBOL(ishtp_device);
+/**
+ * ishtp_get_pci_device() - Return PCI device dev pointer
+ * This interface is used to return PCI device pointer
+ * from ishtp_cl_device instance.
+ *
+ * Return: device *.
+ */
+struct device *ishtp_get_pci_device(struct ishtp_cl_device *device)
+{
+ return device->ishtp_dev->devc;
+}
+EXPORT_SYMBOL(ishtp_get_pci_device);
+
/**
* ishtp_trace_callback() - Return trace callback
*
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index e98bfbb1e07e..16255c2ca2f4 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -77,6 +77,8 @@ int ishtp_register_event_cb(struct ishtp_cl_device *device,
struct device *ishtp_device(struct ishtp_cl_device *cl_device);
/* Trace interface for clients */
void *ishtp_trace_callback(struct ishtp_cl_device *cl_device);
+/* Get device pointer of PCI device for DMA acces */
+struct device *ishtp_get_pci_device(struct ishtp_cl_device *cl_device);
struct ishtp_cl *ishtp_cl_allocate(struct ishtp_cl_device *cl_device);
void ishtp_cl_free(struct ishtp_cl *cl);
--
2.17.2
^ permalink raw reply related
* Re: [PATCH v3] hid: logitech: check the return value of create_singlethread_workqueue
From: Jiri Kosina @ 2019-03-19 10:48 UTC (permalink / raw)
To: Kangjie Lu; +Cc: pakki001, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20190314052402.31448-1-kjlu@umn.edu>
On Thu, 14 Mar 2019, Kangjie Lu wrote:
> create_singlethread_workqueue may fail and return NULL. The fix
> checks if it is NULL to avoid NULL pointer dereference.
> Also, the fix moves the call of create_singlethread_workqueue
> earlier to avoid resource-release issues.
>
> --
> V3: do not introduce memory leaks.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
The signoff has to be in the commit log, not in the "notes" area. I've
fixed that and applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 00/10] HID: intel-ish-hid: Clean up external interfaces
From: Jiri Kosina @ 2019-03-19 10:59 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20190318191428.6527-1-srinivas.pandruvada@linux.intel.com>
On Mon, 18 Mar 2019, Srinivas Pandruvada wrote:
> NOT FOR kernel v5.1.
>
> v2
> - Rebased on the top of
> "HID: intel-ish: enable raw interface to HID devices on ISH"
> which Jiri already applied.
> - Also exported devc pointer for DMA
I've already reviewed v1 before, so I just went through these highlighted
changes above, and applied.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 3/4] input: da9063_onkey: convert header to SPDX
From: Simon Horman @ 2019-03-19 12:09 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-kernel, linux-renesas-soc, Lee Jones, Mark Brown,
Support Opensource, Dmitry Torokhov, linux-input
In-Reply-To: <20190318155728.28365-4-wsa+renesas@sang-engineering.com>
On Mon, Mar 18, 2019 at 04:57:26PM +0100, Wolfram Sang wrote:
> Covnert the header of the source file to SPDX.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> drivers/input/misc/da9063_onkey.c | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
> index 3e9c353d82ef..7b8ae905a6fb 100644
> --- a/drivers/input/misc/da9063_onkey.c
> +++ b/drivers/input/misc/da9063_onkey.c
> @@ -1,16 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0+
> /*
> * OnKey device driver for DA9063, DA9062 and DA9061 PMICs
> * Copyright (C) 2015 Dialog Semiconductor Ltd.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License
> - * as published by the Free Software Foundation; either version 2
> - * of the License, or (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> */
>
> #include <linux/module.h>
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH 2/6] input: da9063_onkey: remove platform_data support
From: Simon Horman @ 2019-03-19 12:49 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-kernel, linux-renesas-soc, Lee Jones, Mark Brown,
Support Opensource, Dmitry Torokhov, linux-input
In-Reply-To: <20190318154759.21978-3-wsa+renesas@sang-engineering.com>
On Mon, Mar 18, 2019 at 04:47:54PM +0100, Wolfram Sang wrote:
> There are no in-kernel users anymore, so remove this outdated interface.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> drivers/input/misc/da9063_onkey.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/misc/da9063_onkey.c b/drivers/input/misc/da9063_onkey.c
> index 3e9c353d82ef..e3a273c74123 100644
> --- a/drivers/input/misc/da9063_onkey.c
> +++ b/drivers/input/misc/da9063_onkey.c
> @@ -22,7 +22,6 @@
> #include <linux/regmap.h>
> #include <linux/of.h>
> #include <linux/mfd/da9063/core.h>
> -#include <linux/mfd/da9063/pdata.h>
> #include <linux/mfd/da9063/registers.h>
> #include <linux/mfd/da9062/core.h>
> #include <linux/mfd/da9062/registers.h>
> @@ -201,8 +200,6 @@ static void da9063_cancel_poll(void *data)
>
> static int da9063_onkey_probe(struct platform_device *pdev)
> {
> - struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
> - struct da9063_pdata *pdata = dev_get_platdata(da9063->dev);
> struct da9063_onkey *onkey;
> const struct of_device_id *match;
> int irq;
> @@ -229,12 +226,8 @@ static int da9063_onkey_probe(struct platform_device *pdev)
> return -ENXIO;
> }
>
> - if (pdata)
> - onkey->key_power = pdata->key_power;
> - else
> - onkey->key_power =
> - !of_property_read_bool(pdev->dev.of_node,
> - "dlg,disable-key-power");
> + onkey->key_power = !of_property_read_bool(pdev->dev.of_node,
> + "dlg,disable-key-power");
>
> onkey->input = devm_input_allocate_device(&pdev->dev);
> if (!onkey->input) {
> --
> 2.11.0
>
^ permalink raw reply
* Re: must hold the driver_input_lock in hid_debug_rdesc_show
From: Jiri Kosina @ 2019-03-19 14:42 UTC (permalink / raw)
To: He, Bo
Cc: benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Zhang, Jun, Zhang, Yanmin
In-Reply-To: <CD6925E8781EFD4D8E11882D20FC406D52A6168C@SHSMSX104.ccr.corp.intel.com>
On Thu, 14 Mar 2019, He, Bo wrote:
> we see the below kernel panic logs when run suspend resume test with
> usb mouse and usb keyboard connected.
>
> the scenario is the userspace call the hid_debug_rdesc_show to dump
> the input device while the device is removed. the patch
> hold the driver_input_lock to avoid the race.
>
> [ 5381.757295] selinux: SELinux: Could not stat
> /sys/devices/pci0000:00/0000:00:15.0/usb1/1-2/1-2:1.0/0003:03F0:0325.0320/input/input960/input960::scrolllock:
> No such file or directory.
> [ 5382.636498] BUG: unable to handle kernel paging request at 0000000783316040
> [ 5382.651950] CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
> [ 5382.663797] RIP: 0010:hid_dump_device+0x9b/0x160
> [ 5382.758853] Call Trace:
> [ 5382.761581] hid_debug_rdesc_show+0x72/0x1d0
> [ 5382.766343] seq_read+0xe0/0x410
> [ 5382.769941] full_proxy_read+0x5f/0x90
> [ 5382.774121] __vfs_read+0x3a/0x170
> [ 5382.788392] vfs_read+0xa0/0x150
> [ 5382.791984] ksys_read+0x58/0xc0
> [ 5382.801404] __x64_sys_read+0x1a/0x20
> [ 5382.805483] do_syscall_64+0x55/0x110
> [ 5382.809559] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Signed-off-by: he, bo <bo.he@intel.com>
> Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
I rewrote the changelog to explain the situation a bit more clearly, and
applied. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-03-19 14:59 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190227072958.GA10349@innovation.ch>
On Tue, Feb 26, 2019 at 11:29:58PM -0800, Life is hard, and then you die wrote:
> On Tue, Feb 26, 2019 at 11:20:59AM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 21, 2019 at 02:56:09AM -0800, Ronald Tschalär wrote:
> > > +config KEYBOARD_APPLESPI
> > > + tristate "Apple SPI keyboard and trackpad"
> >
> > > + depends on ACPI && SPI && EFI
> >
> > I would rather want to see separate line for SPI...
> >
> > > + depends on X86 || COMPILE_TEST
> >
> > ...like here
> >
> > depends on SPI
>
> Sure. Generally, what is the criteria/rule here for splitting
> conjunctions into separate 'depends'?
Rule of common sense.
For example UEFI and ACPI may have some relations, SPI and ACPI kinda
orthogonal.
> > + #define DEV(applespi) (&(applespi)->spi->dev)
> > > + if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {
> >
> > > + dev_warn(DEV(applespi), "Error writing to device: %*ph\n",
> >
> > Hmm... DEV() is too generic name for custom macro. And frankly I don't think
> > it's good to have in the first place.
>
> Yeah, I've been having trouble coming up with a better (but still
> succinct) name - CORE_DEV()? RAW_DEV()? DEV_OF()? However, because
> this expression is used in many places throughout the driver (mostly,
> but not only, for logging statements) I feel like it's good to factor
> it out. But I'll defer to your .
Please remove this macro for good. Otherwise big subsystems / drivers usually do something like
#define foo_err(...) dev_err(...)
...
Don't know if it would help here, the driver is standalone and not so big.
> > > +static void
> > > +applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> > > +{
> > > + unsigned char tmp;
> >
> > > + unsigned long *modifiers =
> > > + (unsigned long *)&keyboard_protocol->modifiers;
> >
> > I would leave it on one online despite checkpatch warning (also, instead of
> > (unsigned long *) the (void *) might be used as a small trick).
> >
> > > +
> > > + if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> > > + !applespi_controlcodes[fnremap - 1])
> > > + return;
> > > +
> > > + tmp = keyboard_protocol->fn_pressed;
> > > + keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
> > > + if (tmp)
> >
> > > + __set_bit(fnremap - 1, modifiers);
> > > + else
> > > + __clear_bit(fnremap - 1, modifiers);
> >
> > Oh, this is not good. modifiers should be really unsigned long bounary,
> > otherwise it is potential overflow.
> >
> > Best to fix is to define them as unsigned long in the first place.
>
> Can't do that directly, because keyboard_protocol->modifiers is a
> field in the data received from the device, i.e. defined by that
> protocol. Instead I could make a copy of the modifiers and pass that
> around separately (i.e. in addition to the keyboard_protocol struct).
>
> However, the implied size assertions here would basically still apply:
>
> MAX_MODIFIERS == sizeof(keyboard_protocol->modifiers) * 8
> ARRAY_SIZE(applespi_controlcodes) == sizeof(keyboard_protocol->modifiers) * 8
>
> (hmm, MAX_MODIFIERS is really redundant - getting rid of it...)
>
> Would using compiletime_assert()'s be an acceptable alternate approach
> here? It would serve to both document the size constraint and to
> protect against overflow due to an error in some future edit. E.g.
>
> applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> {
> unsigned char tmp;
> unsigned long *modifiers = (void *)&keyboard_protocol->modifiers;
> +
> + compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
> + sizeof_field(struct keyboard_protocol, modifiers) * 8,
> + "applespi_controlcodes has wrong number of entries");
>
> if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> !applespi_controlcodes[fnremap - 1])
> return;
>
> tmp = keyboard_protocol->fn_pressed;
> keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
> if (tmp)
>
> __set_bit(fnremap - 1, modifiers);
> else
> __clear_bit(fnremap - 1, modifiers);
> }
Perhaps, simple
__set_bit(b, x) -> x |= BIT(b);
__clear_bit(b, x) -> x &= ~BIT(b);
?
> > > +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH 1/1] hid: hid-sensor-custom: simplify getting .driver_data
From: Wolfram Sang @ 2019-03-19 16:36 UTC (permalink / raw)
To: linux-kernel
Cc: linux-renesas-soc, Wolfram Sang, Jiri Kosina, Jonathan Cameron,
Srinivas Pandruvada, Benjamin Tissoires, linux-input, linux-iio
We should get 'driver_data' from 'struct device' directly. Going via
platform_device is an unneeded step back and forth.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Build tested only. buildbot is happy.
drivers/hid/hid-sensor-custom.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index bb012bc032e0..462e653a7bbb 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -157,8 +157,7 @@ static int usage_id_cmp(const void *p1, const void *p2)
static ssize_t enable_sensor_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
+ struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
return sprintf(buf, "%d\n", sensor_inst->enable);
}
@@ -237,8 +236,7 @@ static ssize_t enable_sensor_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
+ struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
int value;
int ret = -EINVAL;
@@ -283,8 +281,7 @@ static const struct attribute_group enable_sensor_attr_group = {
static ssize_t show_value(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
+ struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
struct hid_sensor_hub_attribute_info *attribute;
int index, usage, field_index;
char name[HID_CUSTOM_NAME_LENGTH];
@@ -392,8 +389,7 @@ static ssize_t show_value(struct device *dev, struct device_attribute *attr,
static ssize_t store_value(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct hid_sensor_custom *sensor_inst = platform_get_drvdata(pdev);
+ struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
int index, field_index, usage;
char name[HID_CUSTOM_NAME_LENGTH];
int value;
--
2.11.0
^ permalink raw reply related
* [PATCH] HID: logitech: Handle 0 scroll events for the m560
From: Peter Hutterer @ 2019-03-19 22:48 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Aimo Metsälä, nlopezcasad,
Jiri Kosina, linux-kernel
hidpp_scroll_counter_handle_scroll() doesn't expect a 0-value scroll event, it
gets interpreted as a negative scroll direction event. This can cause scroll
direction resets and thus broken scrolling.
Reported-and-tested-by: Aimo Metsälä <aimetsal@outlook.com>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
drivers/hid/hid-logitech-hidpp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 15ed6177a7a3..f040c8a7f9a9 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2608,8 +2608,9 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
input_report_rel(mydata->input, REL_Y, v);
v = hid_snto32(data[6], 8);
- hidpp_scroll_counter_handle_scroll(
- &hidpp->vertical_wheel_counter, v);
+ if (v != 0)
+ hidpp_scroll_counter_handle_scroll(
+ &hidpp->vertical_wheel_counter, v);
input_sync(mydata->input);
}
--
2.20.1
^ permalink raw reply related
* RE: must hold the driver_input_lock in hid_debug_rdesc_show
From: He, Bo @ 2019-03-20 1:50 UTC (permalink / raw)
To: Jiri Kosina
Cc: benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Zhang, Jun, Zhang, Yanmin
In-Reply-To: <nycvar.YFH.7.76.1903191541410.19912@cbobk.fhfr.pm>
thanks, without the patch we can reproduce with the way in 10 hours Suspend/Resume test, with the test, we can't reproduce for 30 hours.
-----Original Message-----
From: Jiri Kosina <jikos@kernel.org>
Sent: Tuesday, March 19, 2019 10:42 PM
To: He, Bo <bo.he@intel.com>
Cc: benjamin.tissoires@redhat.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Zhang, Jun <jun.zhang@intel.com>; Zhang, Yanmin <yanmin.zhang@intel.com>
Subject: Re: must hold the driver_input_lock in hid_debug_rdesc_show
On Thu, 14 Mar 2019, He, Bo wrote:
> we see the below kernel panic logs when run suspend resume test with
> usb mouse and usb keyboard connected.
>
> the scenario is the userspace call the hid_debug_rdesc_show to dump
> the input device while the device is removed. the patch hold the
> driver_input_lock to avoid the race.
>
> [ 5381.757295] selinux: SELinux: Could not stat
> /sys/devices/pci0000:00/0000:00:15.0/usb1/1-2/1-2:1.0/0003:03F0:0325.0320/input/input960/input960::scrolllock:
> No such file or directory.
> [ 5382.636498] BUG: unable to handle kernel paging request at 0000000783316040
> [ 5382.651950] CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
> [ 5382.663797] RIP: 0010:hid_dump_device+0x9b/0x160 [ 5382.758853]
> Call Trace:
> [ 5382.761581] hid_debug_rdesc_show+0x72/0x1d0 [ 5382.766343]
> seq_read+0xe0/0x410 [ 5382.769941] full_proxy_read+0x5f/0x90 [
> 5382.774121] __vfs_read+0x3a/0x170 [ 5382.788392]
> vfs_read+0xa0/0x150 [ 5382.791984] ksys_read+0x58/0xc0 [ 5382.801404]
> __x64_sys_read+0x1a/0x20 [ 5382.805483] do_syscall_64+0x55/0x110 [
> 5382.809559] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Signed-off-by: he, bo <bo.he@intel.com>
> Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
I rewrote the changelog to explain the situation a bit more clearly, and applied. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: logitech: Handle 0 scroll events for the m560
From: Benjamin Tissoires @ 2019-03-20 14:29 UTC (permalink / raw)
To: Peter Hutterer
Cc: open list:HID CORE LAYER, Aimo Metsälä,
Nestor Lopez Casado, Jiri Kosina, lkml
In-Reply-To: <20190319224823.GA26366@jelly>
On Tue, Mar 19, 2019 at 11:48 PM Peter Hutterer
<peter.hutterer@who-t.net> wrote:
>
> hidpp_scroll_counter_handle_scroll() doesn't expect a 0-value scroll event, it
> gets interpreted as a negative scroll direction event. This can cause scroll
> direction resets and thus broken scrolling.
>
> Reported-and-tested-by: Aimo Metsälä <aimetsal@outlook.com>
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
Added the 'Fixes' and "cc: stable" tags and patch applied to
for-5.1/upstream-fixes
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 15ed6177a7a3..f040c8a7f9a9 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2608,8 +2608,9 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
> input_report_rel(mydata->input, REL_Y, v);
>
> v = hid_snto32(data[6], 8);
> - hidpp_scroll_counter_handle_scroll(
> - &hidpp->vertical_wheel_counter, v);
> + if (v != 0)
> + hidpp_scroll_counter_handle_scroll(
> + &hidpp->vertical_wheel_counter, v);
>
> input_sync(mydata->input);
> }
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Benjamin Tissoires @ 2019-03-20 14:37 UTC (permalink / raw)
To: hotwater438
Cc: Jiri Kosina, Hans de Goede, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml
In-Reply-To: <LaQHUFs--3-1@tutanota.com>
Hi Vladislav,
I really appreciate the contribution, there are a few issues with the
formatting of the patch, thanks for taking care of those (I'll inline
the problems).
On Wed, Mar 20, 2019 at 2:28 PM <hotwater438@tutanota.com> wrote:
>
> Desciption: This patch add's additional quirk to force
> IRQ_TRIGGER_FALLING flag which resolves issues with ELAN1200:04F3:303E
> touchpad. Issues are the next:
> - Journalctl floods with next report:
> i2c_hid i2c-ELAN1200:00: i2c_hid_get_input: incomplete report (16/65535)
> - Five finger tap kills i2c_hid
> - When you scroll anything, and release one finger, driver thought you are still scrolling
You don't need to be that "formal" in the commit message.
The "description" part describes too much what the patch is (while we
can just read the code), the important part is the issues you are
fixing.
I'd like you to rewrite this in a way that doesn't feel like you are
writing a bug report.
For instance, you could write (but you can change this too):
---
The ELAN1200:04F3:303E touchpad exposes several issues, all caused by
an error setting the correct IRQ_TRIGGER flag:
- ...
- ...
- ...
Fix all of these with a new quirk that corrects the trigger flag
announced by the ACPI tables.
---
I know writing good commit message is a tricky part :)
>
> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
> <hdegoede@redhat.com>:
> i2c_hid_init_irq now checks for a quirk, so we must setup the quirks before we init the irq, and we cannot setup the quirk earlier, so we must init the irq later.
I am pretty sure you can paraphrase Hans in your commit message
without having to formally quote him (Hans, please shout if you
disagree).
But this is a definitively good point to raise: your patch touches a
general path, so it might have unexpected issues if not carefully
reviewed by us.
>
>
> Patch is created by help of ELANTECH and RedHat guys (Hans De Goede <hdegoede@redhat.com>, Benjamin Tissoires <btissoir@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>) and me (Vladislav Dalechyn <hotwater438@tutanota.com>.
Don't take it wrong, but that's not how we give credit in a patch.
In your case, if you do not have any Elantech engineer who agreed to
sign off the patch, you can definitively mention them this way.
But the other guys (Hans, Andy and myself) are well known kernel
developers in the input and HID subsystems, so we usually take the
fame by adding our Reviewed-by or Signed-off-by tag at the end of the
commit message. (Also note that Andy is working for Intel ;-P )
If you feel that these persons have an equal participation in the
creation of the patch, you should ask for their Signed-off-by in the
patch. My gut feelings tell me they would gladly give a Reviewed-by
because they helped you on the patch, but they are not the "author",
so the Signed-off-by should be you only.
And yes, you should sign your work here saying that you wrote this
code in accordance to the Developer's Certificate of Origin 1.1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)
Then, either Jiri or myself will have a special role, and we will add
our own signed-of-by as the maintainers of the HID tree certifying
that this patch has been accepted by one of us.
>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/i2c-hid/i2c-hid-core.c | 17 +++++++++++------
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b6d93f4ad037..dc44b5661669 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -389,6 +389,7 @@
> #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
> #define USB_DEVICE_ID_HP_X2 0x074d
> #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
> +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
I know Hans wanted you to use USB here, but I'd think we should have a
I2C_DEVICE_ID_*
There is no guarantees that this same PID will be used in a different
chip over USB.
We tend to not car about USB/I2C for the vendor IDs: the vendors
decided to reuse their USB VID, which makes our life easier.
>
> #define USB_VENDOR_ID_ELECOM 0x056e
> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 90164fed08d3..16b55c45e2e8 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -50,7 +50,8 @@
> #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
> -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
> +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
your mail client is playing with tabs and spaces, but I don't expect
the line above to be changed at all.
> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
>
> /* flags */
> #define I2C_HID_STARTED 0
> @@ -182,8 +183,10 @@ static const struct i2c_hid_quirks {
> I2C_HID_QUIRK_NO_RUNTIME_PM },
> { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
> I2C_HID_QUIRK_NO_RUNTIME_PM },
> + { USB_VENDOR_ID_ELAN, USB_DEVICE_ID_ELAN_TOUCHPAD,
> + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
> { USB_VENDOR_ID_ELAN, HID_ANY_ID,
> - I2C_HID_QUIRK_BOGUS_IRQ },
> + I2C_HID_QUIRK_BOGUS_IRQ },
same here, this line should not be changed.
> { 0, 0 }
> };
>
> @@ -854,6 +857,8 @@ static int i2c_hid_init_irq(struct i2c_client *client)
>
> if (!irq_get_trigger_type(client->irq))
> irqflags = IRQF_TRIGGER_LOW;
> + if (ihid->quirks & I2C_HID_QUIRK_FORCE_TRIGGER_FALLING)
> + irqflags = IRQF_TRIGGER_FALLING;
again, indentation problems, tabs are missing
>
> ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
> irqflags | IRQF_ONESHOT, client->name, ihid);
> @@ -1123,10 +1128,6 @@ static int i2c_hid_probe(struct i2c_client *client,
> if (ret < 0)
> goto err_pm;
>
> - ret = i2c_hid_init_irq(client);
> - if (ret < 0)
> - goto err_pm;
> -
> hid = hid_allocate_device();
> if (IS_ERR(hid)) {
> ret = PTR_ERR(hid);
the next call is "goto err_irq", which should be changed to "goto err_pm"
> @@ -1149,6 +1150,10 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
>
> + ret = i2c_hid_init_irq(client);
> + if (ret < 0)
> + goto err_pm;
this should be "err_mem_free"
> +
> ret = hid_add_device(hid);
> if (ret) {
> if (ret != -ENODEV)
next one should be "err_irq"
and the err_irq and err_mem_free should be inverted at the end of probe.
Cheers,
Benjamin
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Hans de Goede @ 2019-03-20 15:39 UTC (permalink / raw)
To: Benjamin Tissoires, hotwater438
Cc: Jiri Kosina, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJLGABHATo-fOc_jgc8uNeLDv7cemsHuQMj7zRJWmfyp4g@mail.gmail.com>
H,
On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
<snip>
>> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
>> <hdegoede@redhat.com>:
>> i2c_hid_init_irq now checks for a quirk, so we must setup the quirks before we init the irq, and we cannot setup the quirk earlier, so we must init the irq later.
>
> I am pretty sure you can paraphrase Hans in your commit message
> without having to formally quote him (Hans, please shout if you
> disagree).
Right, it is fine to explain why the irq init is moved without
quoting or even referring to me.
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -389,6 +389,7 @@
>> #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
>> #define USB_DEVICE_ID_HP_X2 0x074d
>> #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
>> +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
>
> I know Hans wanted you to use USB here, but I'd think we should have a
> I2C_DEVICE_ID_*
> There is no guarantees that this same PID will be used in a different
> chip over USB.
> We tend to not car about USB/I2C for the vendor IDs: the vendors
> decided to reuse their USB VID, which makes our life easier.
Ok, no objection from me to use an I2C_DEVICE_ID prefix, here it
is after all an I2C device.
>> #define USB_VENDOR_ID_ELECOM 0x056e
>> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index 90164fed08d3..16b55c45e2e8 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -50,7 +50,8 @@
>> #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
>> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
>> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
>> -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>> +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
>>
>> /* flags */
>> #define I2C_HID_STARTED 0
>> @@ -182,8 +183,10 @@ static const struct i2c_hid_quirks {
>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>> { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>> + { USB_VENDOR_ID_ELAN, USB_DEVICE_ID_ELAN_TOUCHPAD,
>> + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
>> { USB_VENDOR_ID_ELAN, HID_ANY_ID,
>> - I2C_HID_QUIRK_BOGUS_IRQ },
>> + I2C_HID_QUIRK_BOGUS_IRQ },
Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
is also used on Elan devices, I suspect that these Elan devices
likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
and then they probably will no longer need the bogus IRQ flag,
if you know about bugreports with an acpidump for any of the devices
needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
declared there, I suspect it will be declared as level-low, just like
with the laptop this patch was written for. And it probably need to
be edge-falling instead of level-low just like this case.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Kai-Heng Feng @ 2019-03-20 16:53 UTC (permalink / raw)
To: Hans de Goede
Cc: Benjamin Tissoires, hotwater438, Jiri Kosina, Stephen Boyd,
bigeasy, Dmitry Torokhov, open list:HID CORE LAYER, lkml
In-Reply-To: <9151d116-958c-9298-9427-fe803a163e9f@redhat.com>
Hi Hans,
at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> H,
>
> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
> <snip>
>
>>> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
>>> <hdegoede@redhat.com>:
>>> i2c_hid_init_irq now checks for a quirk, so we must setup the quirks
>>> before we init the irq, and we cannot setup the quirk earlier, so we
>>> must init the irq later.
>> I am pretty sure you can paraphrase Hans in your commit message
>> without having to formally quote him (Hans, please shout if you
>> disagree).
>
> Right, it is fine to explain why the irq init is moved without
> quoting or even referring to me.
>
>>> --- a/drivers/hid/hid-ids.h
>>> +++ b/drivers/hid/hid-ids.h
>>> @@ -389,6 +389,7 @@
>>> #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
>>> #define USB_DEVICE_ID_HP_X2 0x074d
>>> #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
>>> +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
>> I know Hans wanted you to use USB here, but I'd think we should have a
>> I2C_DEVICE_ID_*
>> There is no guarantees that this same PID will be used in a different
>> chip over USB.
>> We tend to not car about USB/I2C for the vendor IDs: the vendors
>> decided to reuse their USB VID, which makes our life easier.
>
> Ok, no objection from me to use an I2C_DEVICE_ID prefix, here it
> is after all an I2C device.
>
>>> #define USB_VENDOR_ID_ELECOM 0x056e
>>> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> index 90164fed08d3..16b55c45e2e8 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> @@ -50,7 +50,8 @@
>>> #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
>>> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
>>> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
>>> -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>>> +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>>> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
>>>
>>> /* flags */
>>> #define I2C_HID_STARTED 0
>>> @@ -182,8 +183,10 @@ static const struct i2c_hid_quirks {
>>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>>> { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
>>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>>> + { USB_VENDOR_ID_ELAN, USB_DEVICE_ID_ELAN_TOUCHPAD,
>>> + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
>>> { USB_VENDOR_ID_ELAN, HID_ANY_ID,
>>> - I2C_HID_QUIRK_BOGUS_IRQ },
>>> + I2C_HID_QUIRK_BOGUS_IRQ },
>
> Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
> is also used on Elan devices, I suspect that these Elan devices
> likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
> and then they probably will no longer need the bogus IRQ flag,
> if you know about bugreports with an acpidump for any of the devices
> needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
> declared there, I suspect it will be declared as level-low, just like
> with the laptop this patch was written for. And it probably need to
> be edge-falling instead of level-low just like this case.
First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
doesn’t solve the issue for me.
I talked to Elan once, and they confirm the correct IRQ trigger is level
low. So forcing falling trigger may break other platforms.
Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
Once the Interrupt() is used instead, the issue goes away.
But I am not sure how to patch its DSDT/SSDT in i2c-hid.
Kai-Heng
>
> Regards,
>
> Hans
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Andy Shevchenko @ 2019-03-20 17:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: hotwater438, Jiri Kosina, Hans de Goede, Kai Heng Feng,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJLGABHATo-fOc_jgc8uNeLDv7cemsHuQMj7zRJWmfyp4g@mail.gmail.com>
On Wed, Mar 20, 2019 at 4:38 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Wed, Mar 20, 2019 at 2:28 PM <hotwater438@tutanota.com> wrote:
> > Patch is created by help of ELANTECH and RedHat guys (Hans De Goede <hdegoede@redhat.com>, Benjamin Tissoires <btissoir@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>) and me (Vladislav Dalechyn <hotwater438@tutanota.com>.
>
> Don't take it wrong, but that's not how we give credit in a patch.
> In your case, if you do not have any Elantech engineer who agreed to
> sign off the patch, you can definitively mention them this way.
> But the other guys (Hans, Andy and myself) are well known kernel
> developers in the input and HID subsystems, so we usually take the
> fame by adding our Reviewed-by or Signed-off-by tag at the end of the
> commit message. (Also note that Andy is working for Intel ;-P )
>
> If you feel that these persons have an equal participation in the
> creation of the patch, you should ask for their Signed-off-by in the
> patch. My gut feelings tell me they would gladly give a Reviewed-by
> because they helped you on the patch, but they are not the "author",
> so the Signed-off-by should be you only.
Just in case, we have Co-developed-by: tag and for my opinion Hans'
name should go under this category.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Andy Shevchenko @ 2019-03-20 17:18 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <F03846A5-6C42-4290-AF72-F644C809A50A@canonical.com>
On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> > Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
> > is also used on Elan devices, I suspect that these Elan devices
> > likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
> > and then they probably will no longer need the bogus IRQ flag,
> > if you know about bugreports with an acpidump for any of the devices
> > needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
> > declared there, I suspect it will be declared as level-low, just like
> > with the laptop this patch was written for. And it probably need to
> > be edge-falling instead of level-low just like this case.
>
> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> doesn’t solve the issue for me.
>
> I talked to Elan once, and they confirm the correct IRQ trigger is level
> low. So forcing falling trigger may break other platforms.
As far as I understood Vladislav the quirk he got from Elan as well.
> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> Once the Interrupt() is used instead, the issue goes away.
IIRC i2c core tries to get interrupt from Interrupt() resource and
then falls back to GpioInt().
See i2c_acpi_get_info() and i2c_device_probe().
> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Kai-Heng Feng @ 2019-03-21 4:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <CAHp75VcOG9H8u+tZyNN682r7+jsRZi8VoPPZ2uJPch1gNyFgmQ@mail.gmail.com>
at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
>>> Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
>>> is also used on Elan devices, I suspect that these Elan devices
>>> likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
>>> and then they probably will no longer need the bogus IRQ flag,
>>> if you know about bugreports with an acpidump for any of the devices
>>> needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
>>> declared there, I suspect it will be declared as level-low, just like
>>> with the laptop this patch was written for. And it probably need to
>>> be edge-falling instead of level-low just like this case.
>>
>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
>> doesn’t solve the issue for me.
>>
>> I talked to Elan once, and they confirm the correct IRQ trigger is level
>> low. So forcing falling trigger may break other platforms.
>
> As far as I understood Vladislav the quirk he got from Elan as well.
Ok, then this is really weird.
>
>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>> Once the Interrupt() is used instead, the issue goes away.
>
> IIRC i2c core tries to get interrupt from Interrupt() resource and
> then falls back to GpioInt().
> See i2c_acpi_get_info() and i2c_device_probe().
Here’s its ASL:
Scope (\_SB.PCI0.I2C4)
{
Device (TPD0)
{
Name (_ADR, One) // _ADR: Address
Name (_HID, "DELL08AE") // _HID: Hardware ID
Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
Name (_UID, One) // _UID: Unique ID
Name (_S0W, 0x04) // _S0W: S0 Device Wake State
Name (SBFB, ResourceTemplate ()
{
I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
AddressingMode7Bit, "\\_SB.PCI0.I2C4",
0x00, ResourceConsumer, , Exclusive,
)
})
Name (SBFG, ResourceTemplate ()
{
GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO1", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0012
}
})
Name (SBFI, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
{
0x0000003C,
}
})
Method (_INI, 0, NotSerialized) // _INI: Initialize
{
}
Method (_STA, 0, NotSerialized) // _STA: Status
{
If ((TCPD == One))
{
Return (0x0F)
}
Return (Zero)
}
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
If ((OSYS < 0x07DC))
{
Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
}
Return (ConcatenateResTemplate (SBFB, SBFG))
}
Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID I2C Device */))
{
If ((Arg2 == Zero))
{
If ((Arg1 == One))
{
Return (Buffer (One)
{
0x03 // .
})
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
ElseIf ((Arg2 == One))
{
Return (0x20)
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
{
If ((Arg2 == Zero))
{
If ((Arg1 == One))
{
Return (Buffer (One)
{
0x03 // .
})
}
}
If ((Arg2 == One))
{
Return (ConcatenateResTemplate (SBFB, SBFG))
}
Return (Buffer (One)
{
0x00 // .
})
}
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
}
}
Change SBFG to SBFI in its _CRS can workaround the issue.
Is ASL in this form possible to do the flow you described?
Kai-Heng
>
>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
>
> --
> With Best Regards,
> Andy Shevchenko
^ 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