* RE: [PATCH v3] HID: core: move Usage Page concatenation to Main item
From: Junge, Terry @ 2019-03-26 22:43 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Jiri Kosina, Benjamin Tissoires
Cc: oneukum@suse.de, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190326200331.25934-1-nsaenzjulienne@suse.de>
Hi Nicolas,
This patch looks good except for one comment/question below.
Thanks,
Terry
On Tuesday, March 26, 2019 1:04 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote:
>
>As seen on some USB wireless keyboards manufactured by Primax, the HID
>parser was using some assumptions that are not always true. In this case it's s
>the fact that, inside the scope of a main item, an Usage Page will always
>precede an Usage.
>
>The spec is not pretty clear as 6.2.2.7 states "Any usage that follows is
>interpreted as a Usage ID and concatenated with the Usage Page".
>While 6.2.2.8 states "When the parser encounters a main item it concatenates
>the last declared Usage Page with a Usage to form a complete usage value."
>Being somewhat contradictory it was decided to match Window's
>implementation, which follows 6.2.2.8.
>
>In summary, the patch moves the Usage Page concatenation from the local
>item parsing function to the main item parsing function.
>
>Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>---
>
>v2->v3: - Update patch title
>
>v1->v2: - Add usage concatenation to hid_scan_main()
> - Rework tests in hid-tools, making sure no-one is failing
>
> drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++------------
> include/linux/hid.h | 1 +
> 2 files changed, 29 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index
>9993b692598f..40c836ce3248 100644
>--- a/drivers/hid/hid-core.c
>+++ b/drivers/hid/hid-core.c
>@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
>hid_parser *parser, unsigned type)
> * Add a usage to the temporary parser table.
> */
>
>-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
>+static int hid_add_usage(struct hid_parser *parser, unsigned usage,
>+__u8 size)
> {
> if (parser->local.usage_index >= HID_MAX_USAGES) {
> hid_err(parser->device, "usage index exceeded\n");
> return -1;
> }
> parser->local.usage[parser->local.usage_index] = usage;
>+ parser->local.usage_size[parser->local.usage_index] = size;
> parser->local.collection_index[parser->local.usage_index] =
> parser->collection_stack_ptr ?
> parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
>@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> return 0;
> }
>
>- if (item->size <= 2)
>- data = (parser->global.usage_page << 16) + data;
>-
>- return hid_add_usage(parser, data);
>+ return hid_add_usage(parser, data, item->size);
>
> case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
>
>@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> return 0;
> }
>
>- if (item->size <= 2)
>- data = (parser->global.usage_page << 16) + data;
>-
> parser->local.usage_minimum = data;
> return 0;
>
>@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> return 0;
> }
>
>- if (item->size <= 2)
>- data = (parser->global.usage_page << 16) + data;
>-
> count = data - parser->local.usage_minimum;
> if (count + parser->local.usage_index >= HID_MAX_USAGES) {
> /*
>@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> }
>
> for (n = parser->local.usage_minimum; n <= data; n++)
>- if (hid_add_usage(parser, n)) {
>+ if (hid_add_usage(parser, n, item->size)) {
> dbg_hid("hid_add_usage failed\n");
> return -1;
> }
>@@ -547,6 +539,26 @@ static int hid_parser_local(struct hid_parser *parser,
>struct hid_item *item)
> return 0;
> }
>
>+/*
>+ * Concatenate Usage Pages into Usages where relevant:
>+ * As per specification, 6.2.2.8: "When the parser encounters a main
>+item it
>+ * concatenates the last declared Usage Page with a Usage to form a
>+complete
>+ * usage value."
>+ */
>+
>+static void hid_concatenate_usage_page(struct hid_parser *parser) {
>+ unsigned usages;
>+ int i;
>+
>+ usages = max_t(unsigned, parser->local.usage_index,
>+ parser->global.report_count);
I don't think we need to worry about global.report_count here,
just concatenate for the usages currently in the local queue so could
this be simplified by removing usages and just using local.usage_index?
for (i = 0; i < local.usage_index; i++)
>+
>+ for (i = 0; i < usages; i++)
>+ if (parser->local.usage_size[i] <= 2)
>+ parser->local.usage[i] += parser->global.usage_page
><< 16; }
>+
> /*
> * Process a main item.
> */
>@@ -556,6 +568,8 @@ static int hid_parser_main(struct hid_parser *parser,
>struct hid_item *item)
> __u32 data;
> int ret;
>
>+ hid_concatenate_usage_page(parser);
>+
> data = item_udata(item);
>
> switch (item->tag) {
>@@ -765,6 +779,8 @@ static int hid_scan_main(struct hid_parser *parser,
>struct hid_item *item)
> __u32 data;
> int i;
>
>+ hid_concatenate_usage_page(parser);
>+
> data = item_udata(item);
>
> switch (item->tag) {
>diff --git a/include/linux/hid.h b/include/linux/hid.h index
>f9707d1dcb58..d1fb4b678873 100644
>--- a/include/linux/hid.h
>+++ b/include/linux/hid.h
>@@ -417,6 +417,7 @@ struct hid_global {
>
> struct hid_local {
> unsigned usage[HID_MAX_USAGES]; /* usage array */
>+ __u8 usage_size[HID_MAX_USAGES]; /* usage size array */
> unsigned collection_index[HID_MAX_USAGES]; /* collection index
>array */
> unsigned usage_index;
> unsigned usage_minimum;
>--
>2.21.0
^ permalink raw reply
* [PATCH] Input: i8042 - signal wakeup from atkbd/psmouse
From: Dmitry Torokhov @ 2019-03-27 0:28 UTC (permalink / raw)
To: linux-input
Cc: Ravi Chandra Sadineni, Rafael J. Wysocki, Shaunak Saha,
linux-kernel
Instead of signalling wakeup directly from i8042, let psmouse and atkbd
drivers execute basic protocol handling and only then signal wakeup
condition. This solves the issue where we increment wakeup counter
simply because we are getting responses from keyboard/mouse to the
commands we ourselves send to them as part of suspend transition.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/keyboard/atkbd.c | 2 ++
drivers/input/mouse/psmouse-base.c | 2 ++
drivers/input/serio/i8042.c | 3 ---
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 850bb259c20e..3ad93e3e2f4c 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -401,6 +401,8 @@ static irqreturn_t atkbd_interrupt(struct serio *serio, unsigned char data,
if (ps2_handle_response(&atkbd->ps2dev, data))
goto out;
+ pm_wakeup_event(&serio->dev, 0);
+
if (!atkbd->enabled)
goto out;
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index d3ff1fc09af7..94f7ca5ad077 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -373,6 +373,8 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
if (ps2_handle_response(&psmouse->ps2dev, data))
goto out;
+ pm_wakeup_event(&serio->dev, 0);
+
if (psmouse->state <= PSMOUSE_RESYNCING)
goto out;
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 95a78ccbd847..6462f1798fbb 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -573,9 +573,6 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
port = &i8042_ports[port_no];
serio = port->exists ? port->serio : NULL;
- if (irq && serio)
- pm_wakeup_event(&serio->dev, 0);
-
filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
port_no, irq,
dfl & SERIO_PARITY ? ", bad parity" : "",
--
2.21.0.392.gf8f6787159e-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] HID: intel-ish-hid: ISH firmware loader client driver
From: Nick Crews @ 2019-03-27 0:39 UTC (permalink / raw)
To: Rushikesh S Kadam
Cc: benjamin.tissoires, jikos, jettrink, gwendal, linux-kernel,
linux-input, Srinivas Pandruvada
In-Reply-To: <31755c704928710da998353192157ddfd903080c.camel@linux.intel.com>
Hi Rushikesh, I know I've been reviewing this on Chromium, but I have
some more larges-scale design thoughts.
> > diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > new file mode 100644
> > index 0000000..85d71d3
> > --- /dev/null
> > +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > @@ -0,0 +1,1103 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ISH-TP client driver for ISH firmware loading
> > + *
> > + * Copyright (c) 2018, Intel Corporation.
> Year 2019.
>
> > + */
> > +
> > +#include <linux/firmware.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/intel-ish-client-if.h>
> > +#include <linux/property.h>
> > +#include <asm/cacheflush.h>
> > +
> > +/* ISH TX/RX ring buffer pool size */
> > +#define LOADER_CL_RX_RING_SIZE 1
> > +#define LOADER_CL_TX_RING_SIZE 1
> > +
> > +/*
> > + * ISH Shim firmware loader reserves 4 Kb buffer in SRAM. The buffer
> > is
> > + * used to temporarily hold the data transferred from host to Shim
> > firmware
> > + * loader. Reason for the odd size of 3968 bytes? Each IPC transfer
> > is 128
> > + * bytes (= 4 bytes header + 124 bytes payload). So the 4 Kb buffer
> > can
> > + * hold maximum of 32 IPC transfers, which means we can have a max
> > payload
> > + * of 3968 bytes (= 32 x 124 payload).
> > + */
> > +#define LOADER_SHIM_IPC_BUF_SIZE 3968
> > +
> > +/**
> > + * enum ish_loader_commands - ISH loader host commands.
> > + * LOADER_CMD_XFER_QUERY Query the Shim firmware loader for
> > capabilities
> > + * LOADER_CMD_XFER_FRAGMENT Transfer one firmware image framgment
> > at a
> > + * time. The command may be executed
> > multiple
> > + * times until the entire firmware image
> > is
> > + * downloaded to SRAM.
> > + * LOADER_CMD_START Start executing the main firmware.
> > + */
> > +enum ish_loader_commands {
> > + LOADER_CMD_XFER_QUERY = 0,
> > + LOADER_CMD_XFER_FRAGMENT,
> > + LOADER_CMD_START,
> > +};
> > +
> > +/* Command bit mask */
> > +#define CMD_MASK GENMASK(6, 0)
> > +#define IS_RESPONSE BIT(7)
> > +
> > +/*
> > + * ISH firmware max delay for one transmit failure is 1 Hz,
> > + * and firmware will retry 2 times, so 3 Hz is used for timeout.
> > + */
> > +#define ISHTP_SEND_TIMEOUT (3 * HZ)
> > +
> > +/*
> > + * Loader transfer modes:
> > + *
> > + * LOADER_XFER_MODE_ISHTP mode uses the existing ISH-TP mechanims to
> > + * transfer data. This may use IPC or DMA if supported in firmware.
> > + * The buffer size is limited to 4 Kb by the IPC/ISH-TP protocol for
> > + * both IPC & DMA (legacy).
> > + *
> > + * LOADER_XFER_MODE_DIRECT_DMA - firmware loading is a bit different
> > + * from the sensor data streaming. Here we download a large (300+
> > Kb)
> > + * image directly to ISH SRAM memory. There is limited benefit of
> > + * DMA'ing 300 Kb image in 4 Kb chucks limit. Hence, we introduce
> > + * this "direct dma" mode, where we do not use ISH-TP for DMA, but
> > + * instead manage the DMA directly in kernel driver and Shim
> > firmware
> > + * loader (allocate buf, break in chucks and transfer). This allows
> > + * to overcome 4 Kb limit, and optimize the data flow path in
> > firmware.
> > + */
> > +#define LOADER_XFER_MODE_DIRECT_DMA BIT(0)
> > +#define LOADER_XFER_MODE_ISHTP BIT(1)
> > +
> > +/* ISH Transport Loader client unique GUID */
> > +static const guid_t loader_ishtp_guid =
> > + GUID_INIT(0xc804d06a, 0x55bd, 0x4ea7,
> > + 0xad, 0xed, 0x1e, 0x31, 0x22, 0x8c, 0x76, 0xdc);
> > +
> > +#define FILENAME_SIZE 256
> > +
> > +/*
> > + * The firmware loading latency will be minimum if we can DMA the
> > + * entire ISH firmware image in one go. This requires that we
> > allocate
> > + * a large DMA buffer in kernel, which could be problematic on some
> > + * platforms. So here we limit the DMA buf size via a module_param.
> > + * We default to 4 pages, but a customer can set it to higher limit
> > if
> > + * deemed appropriate for his platform.
> > + */
> > +static int dma_buf_size_limit = 4 * PAGE_SIZE;
> > +
> > +/**
> > + * struct loader_msg_hdr - Header for ISH Loader commands.
> > + * @command: LOADER_CMD* commands. Bit 7 is the response.
> > + * @status: Command response status. Non 0, is error
> > condition.
> > + *
> > + * This structure is used as header for every command/data
> > sent/received
> > + * between Host driver and ISH Shim firmware loader.
> > + */
> > +struct loader_msg_hdr {
> > + u8 command;
> > + u8 reserved[2];
> > + u8 status;
> > +} __packed;
> > +
> > +struct loader_xfer_query {
> > + struct loader_msg_hdr hdr;
> > + u32 image_size;
> > +} __packed;
> > +
> > +struct ish_fw_version {
> > + u16 major;
> > + u16 minor;
> > + u16 hotfix;
> > + u16 build;
> > +} __packed;
> > +
> > +union loader_version {
> > + u32 value;
> > + struct {
> > + u8 major;
> > + u8 minor;
> > + u8 hotfix;
> > + u8 build;
> > + };
> > +} __packed;
> > +
> > +struct loader_capability {
> > + u32 max_fw_image_size;
> > + u32 xfer_mode;
> > + u32 max_dma_buf_size; /* only for dma mode, multiples of
> > cacheline */
> > +} __packed;
> > +
> > +struct shim_fw_info {
> > + struct ish_fw_version ish_fw_version;
> > + u32 protocol_version;
> > + union loader_version ldr_version;
> > + struct loader_capability ldr_capability;
> > +} __packed;
> > +
> > +struct loader_xfer_query_response {
> > + struct loader_msg_hdr hdr;
> > + struct shim_fw_info fw_info;
> > +} __packed;
> > +
> > +struct loader_xfer_fragment {
> > + struct loader_msg_hdr hdr;
> > + u32 xfer_mode;
> > + u32 offset;
> > + u32 size;
> > + u32 is_last;
> > +} __packed;
> > +
> > +struct loader_xfer_ipc_fragment {
> > + struct loader_xfer_fragment fragment;
> > + u8 data[] ____cacheline_aligned; /* variable length payload
> > here */
> > +} __packed;
> > +
> > +struct loader_xfer_dma_fragment {
> > + struct loader_xfer_fragment fragment;
> > + u64 ddr_phys_addr;
> > +} __packed;
> > +
> > +struct loader_start {
> > + struct loader_msg_hdr hdr;
> > +} __packed;
> > +
> > +/**
> > + * struct ishtp_cl_data - Encapsulate per ISH-TP Client Data
> > + * @flag_response Set true on receiving a firmware response to
> > host
> > + * loader command
> > + * @cmd_resp_wait: Wait queue for Host firmware loading, where the
> > + * client sends message to ISH firmware and wait
> > for
> > + * response
> > + * @work_ishtp_reset: Work queue for reset handling
> > + * @work_fw_load: Work queue for host firmware loading
> > + * @flag_retry Flag for indicating host firmware
> > loading should be
> > + * retried
> > + * @bad_recv_cnt: Running count of packets received with error
> > + *
> > + * This structure is used to store data per client
> > + */
> > +struct ishtp_cl_data {
> > + struct ishtp_cl *loader_ishtp_cl;
> > + struct ishtp_cl_device *cl_device;
> > +
> > + /* Completion flags */
> > + bool flag_response;
> > +
> > + /* Copy buffer received in firmware "response" here */
> > + void *response_data;
> > + size_t response_size;
> > +
> > + /* Wait queue for ISH firmware message event */
> > + wait_queue_head_t cmd_resp_wait;
> > +
> > + struct work_struct work_ishtp_reset;
> > + struct work_struct work_fw_load;
> > +
> > + /*
> > + * In certain failure scenrios, it makes sense to reset the
> > + * the ISH subsystem and retry Host firmware loading
> > + * (e.g. bad message packet, ENOMEM, etc.)
> > + * On the other hand, failures due to protocol mismatch, etc
> > + * are not recoverable. We do not retry.
> > + *
> > + * If set, the flag indictes that we should re-try the
> > particular
> > + * failure.
> > + */
> > + bool flag_retry;
> > +
> > + /* Statistics */
> > + unsigned int bad_recv_cnt;
> > +};
> > +
> > +#define IPC_FRAGMENT_DATA_PREAMBLE \
> > + offsetof(struct loader_xfer_ipc_fragment, data)
> > +
> > +#define cl_data_to_dev(client_data) ishtp_device((client_data)-
> > >cl_device)
> > +
> > +/**
> > + * get_firmware_variant() - Gets the filename of firmware image to
> > be
> > + * loaded based on platform variant.
> > + * @client_data Client data instance.
> > + * @filename Returns firmware filename.
> > + *
> > + * Queries the firmware-name device property string.
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int get_firmware_variant(struct ishtp_cl_data *client_data,
> > + char *filename)
> > +{
> > + int rv;
> > + const char *val;
> > + struct device *devc = ishtp_get_pci_device(client_data-
> > >cl_device);
> > +
> > + rv = device_property_read_string(devc, "firmware-name", &val);
> > + if (rv < 0) {
> > + dev_err(devc,
> > + "Error: ISH firmware-name device property
> > required\n");
> > + return rv;
> > + }
> > + return snprintf(filename, FILENAME_SIZE, "intel/%s", val);
> > +}
> > +
> > +/**
> > + * report_bad_packets() Report bad packets
> > + * @loader_ishtp_cl: Client instance to get stats
> > + * @recv_buf: Raw received host interface message
> > + *
> > + * Dumps error in case bad packet is received
> > + */
> > +static void report_bad_packet(struct ishtp_cl *loader_ishtp_cl,
> > + void *recv_buf)
> > +{
> > + struct loader_msg_hdr *hdr = recv_buf;
> > + struct ishtp_cl_data *client_data =
> > + ishtp_get_client_data(loader_ishtp_cl);
> > +
> > + client_data->bad_recv_cnt++;
> > + dev_err(cl_data_to_dev(client_data),
> > + "BAD packet: command=%02lx is_response=%u status=%02x
> > total_bad=%u\n",
> > + hdr->command & CMD_MASK,
> > + hdr->command & IS_RESPONSE ? 1 : 0,
> > + hdr->status,
> > + client_data->bad_recv_cnt);
> > +}
I would remove this function. Whenever you call it, you already have
use dev_err() to print the reason for the error. Consider removing
bad_rcv_count too unless you do something with it other than debugging.
At the very least, you call this function when the ISH doesn't return enough
data, but in here you try to access the fields in hdr. This could be accessing
irrelevant/illegal data.
Also a nit: The docstring function name has a naughty trailing s.
> > +
> > +/**
> > + * loader_ish_hw_reset() - Reset ISH HW in bad state
> > + * @loader_ishtp_cl Client instance to reset
> > + *
> > + * This function resets ISH hardware, which shall reload
> > + * the Shim firmware loader, initiate ISH-TP interface reset,
> > + * re-attach kernel loader driver, and repeat Host
> > + * firmware load.
> > + */
> > +static inline void loader_ish_hw_reset(struct ishtp_cl
> > *loader_ishtp_cl)
> > +{
> > + struct ishtp_cl_data *client_data =
> > + ishtp_get_client_data(loader_ishtp_cl);
> > +
> > + dev_warn(cl_data_to_dev(client_data), "Reset the ISH
> > subsystem\n");
> > + ish_hw_reset(ishtp_get_ishtp_device(loader_ishtp_cl));
> > +}
Delete this as a function. Before you actually called it in multiple places,
but now i's only called in one place, so just inline it there.
> > +
> > +/**
> > + * loader_cl_send() Send message from host to firmware
> > + * @client_data: Client data instance
> > + * @msg Message buffer to send
> > + * @msg_size Size of message
> > + *
> > + * Return: Received buffer size on success, negative error code on
> > failure.
> > + */
> > +static int loader_cl_send(struct ishtp_cl_data *client_data,
> > + u8 *msg, size_t msg_size)
> > +{
> > + int rv;
> > + size_t data_len;
> > + struct loader_msg_hdr *in_hdr;
> > + struct loader_msg_hdr *out_hdr = (struct loader_msg_hdr *)msg;
> > + struct ishtp_cl *loader_ishtp_cl = client_data-
> > >loader_ishtp_cl;
> > +
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "%s: command=%02lx is_response=%u status=%02x\n",
> > + __func__,
> > + out_hdr->command & CMD_MASK,
> > + out_hdr->command & IS_RESPONSE ? 1 : 0,
> > + out_hdr->status);
> > +
> > + client_data->flag_response = false;
> > + rv = ishtp_cl_send(loader_ishtp_cl, msg, msg_size);
> > + if (rv < 0) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "ishtp_cl_send error %d\n", rv);
> > + return rv;
> > + }
> > +
> > + wait_event_interruptible_timeout(client_data->cmd_resp_wait,
> > + client_data->flag_response,
> > + ISHTP_SEND_TIMEOUT);
> > + if (!client_data->flag_response) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "Timed out for response to command=%02lx",
> > + out_hdr->command & CMD_MASK);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + /* All response messages will contain a header */
> > + data_len = client_data->response_size;
> > + in_hdr = (struct loader_msg_hdr *)client_data->response_data;
If process_recv() fails then client_data->response_data could be NULL.
This brings up the question of what to do if process_recv() fails. I would think
that you would want it to set something like client_data->response_error
and then you could check for that in here and return it. For instance
right now if the ISH
doesn't return sizeof(struct loader_msg_hdr) bytes then it would be nice to get
-EMSGSIZE returned from here.
> > +
> > + /* Sanity checks */
> > + if (!(in_hdr->command & IS_RESPONSE)) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "Invalid response to command\n");
> > + return -EIO;
> > + }
> > +
> > + if (in_hdr->status) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "Loader returned status %d\n",
> > + in_hdr->status);
> > + return -EIO;
> > + }
> > +
> > + return data_len;
> > +}
So I think how you've changed this to handle where the data is stored is good,
but it could be better. I don't like how the users of loader_cl_send() need to
remember to kfree(client_data->response data), and that they just implicitly
assume that client_data->response data holds the result. Instead, make the
callers of loader_cl_send() allocate a buffer to hold the result, and then the
allocating and freeing happens in the same function. I think this is a much more
understandable form of memory management.
How about this function turns into:
/**
* loader_cl_send() Send message from host to firmware
* @client_data: Client data instance
* @in_data: Message buffer to send
* @in_size: Size of sent data
* @out_data: Buffer to fill with received data.
* @out_size: Max number of bytes to place in out_data.
*
* Return: Number of bytes placed into out_data, negative error code on failure.
*/
static int loader_cl_send(struct ishtp_cl_data *client_data,
u8 *in_data, size_t in_size,
u8 *out_data, size_t out_size)
{
client_data->response_data = out_data;
client_data->response_size_max = out_size;
Send the command.
Tweak process_recv() so where it does the memcpy() into
client_data->response_data,
add the additional check to make sure it doesn't copy more than
client_data->response_size_max bytes.
Wait for the completion flag.
Continue with the rest.
}
With these suggestions there are now six pieces of info getting
transmitted between
process_recv() and loader_cl_send() via client data:
client_data->cmd_resp_wait
client_data->flag_response
client_data->response_error
client_data->response_size
client_data->response_size_max
client_data->response_data
Consider turning these into:
client_data->response_info->wait_queue
client_data->response_info->data_available // or some better name?
client_data->response_info->error
client_data->response_info->size
client_data->response_info->size_max
client_data->response_info->data
for some encapsulation?
I'm thinking about this more, and basically it seems like we're
writing a library function to
send a command to the ISH and receive a response. All the clients who
use loader_cl_send()
shouldn't know about the client_data->response_info stuff at all. It
almost seems like this
entire functionality should be part of the ISH core? It's really
limiting that ishtp_cl_send() only
allows sending and no receiving! It should?!
> > +
> > +/**
> > + * process_recv() - Receive and parse incoming packet
> > + * @loader_ishtp_cl: Client instance to get stats
> > + * @rb_in_proc: ISH received message buffer
> > + *
> > + * Parse the incoming packet. If it is a response packet then it
> > will
> > + * update flag_response and wake up the caller waiting to for the
> > response.
> > + */
> > +static void process_recv(struct ishtp_cl *loader_ishtp_cl,
> > + struct ishtp_cl_rb *rb_in_proc)
> > +{
> > + size_t data_len = rb_in_proc->buf_idx;
> > + struct loader_msg_hdr *hdr =
> > + (struct loader_msg_hdr *)rb_in_proc->buffer.data;
> > + struct ishtp_cl_data *client_data =
> > + ishtp_get_client_data(loader_ishtp_cl);
> > +
> > + /*
> > + * All firmware messages have a header. Check buffer size
> > + * before accessing elements inside.
> > + */
> > + if (data_len < sizeof(struct loader_msg_hdr)) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "data size %zu is less than header %zu\n",
> > + data_len, sizeof(struct loader_msg_hdr));
> > + report_bad_packet(client_data->loader_ishtp_cl, hdr);
> > + goto end_error;
> > + }
> > +
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "%s: command=%02lx is_response=%u status=%02x\n",
> > + __func__,
> > + hdr->command & CMD_MASK,
> > + hdr->command & IS_RESPONSE ? 1 : 0,
> > + hdr->status);
> > +
> > + switch (hdr->command & CMD_MASK) {
> > + case LOADER_CMD_XFER_QUERY:
> > + case LOADER_CMD_XFER_FRAGMENT:
> > + case LOADER_CMD_START:
> > + /* Sanity check */
> > + if (client_data->response_data || client_data-
> > >flag_response) {
Following advice above, how about checking
client_data->response_info->data_available instead?
Or with advice above, corrupting old data might not be an issue,
since the destination buffer changes? Also I wouldn't call this a buffer
overrun below, it's a different problem.
> > + dev_err(cl_data_to_dev(client_data),
> > + "Buffer overrun: previous firmware
> > message not yet processed\n");
> > + report_bad_packet(client_data->loader_ishtp_cl,
> > hdr);
> > + break;
> > + }
> > +
> > + /*
> > + * Copy the buffer received in firmware response for
> > the
> > + * calling thread.
> > + */
> > + client_data->response_data = kmalloc(data_len,
> > GFP_KERNEL);
> > + if (!client_data->response_data)
> > + break;
> > +
> > + memcpy(client_data->response_data,
> > + rb_in_proc->buffer.data, data_len);
> > + client_data->response_size = data_len;
> > +
> > + /* Free the buffer */
> > + ishtp_cl_io_rb_recycle(rb_in_proc);
> > + rb_in_proc = NULL;
> > +
> > + /* Wake the calling thread */
> > + client_data->flag_response = true;
> > + wake_up_interruptible(&client_data->cmd_resp_wait);
> > + break;
> > +
> > + default:
> > + dev_err(cl_data_to_dev(client_data),
> > + "Invalid command=%02lx\n",
> > + hdr->command & CMD_MASK);
> > + report_bad_packet(client_data->loader_ishtp_cl, hdr);
> > + }
> > +
> > +end_error:
> > + /* Free the buffer if we did not do above */
> > + if (rb_in_proc)
> > + ishtp_cl_io_rb_recycle(rb_in_proc);
> > +}
> > +
> > +/**
> > + * loader_cl_event_cb() - bus driver callback for incoming message
> > + * @device: Pointer to the the ishtp client device for
> > which
> > + * this message is targeted
> > + *
> > + * Remove the packet from the list and process the message by
> > calling
> > + * process_recv
> > + */
> > +static void loader_cl_event_cb(struct ishtp_cl_device *cl_device)
> > +{
> > + struct ishtp_cl_rb *rb_in_proc;
> > + struct ishtp_cl_data *client_data;
> > + struct ishtp_cl *loader_ishtp_cl =
> > ishtp_get_drvdata(cl_device);
> > +
> > + client_data = ishtp_get_client_data(loader_ishtp_cl);
> > +
> > + while ((rb_in_proc = ishtp_cl_rx_get_rb(loader_ishtp_cl)) !=
> > NULL) {
> > + if (!rb_in_proc->buffer.data) {
> > + dev_warn(cl_data_to_dev(client_data),
> > + "rb_in_proc->buffer.data returned
> > null");
Maybe move this check into process_recv() and then you can set
client_data->response_info->error for it?
> > + continue;
> > + }
> > +
> > + /* Process the data packet from firmware */
> > + process_recv(loader_ishtp_cl, rb_in_proc);
> > + }
> > +}
> > +
> > +/**
> > + * ish_query_loader_prop() - Query ISH Shim firmware loader
> > + * @client_data: Client data instance
> > + * @fw: Poiner to fw data struct in host memory
> > + *
> > + * This function queries the ISH Shim firmware loader for
> > capabilities.
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int ish_query_loader_prop(struct ishtp_cl_data *client_data,
> > + const struct firmware *fw,
> > + struct shim_fw_info *fw_info)
> > +{
> > + int rv;
> > + size_t data_len;
> > + struct loader_msg_hdr *hdr;
> > + struct loader_xfer_query ldr_xfer_query;
> > + struct loader_xfer_query_response *ldr_xfer_query_resp;
> > +
> > + memset(&ldr_xfer_query, 0, sizeof(ldr_xfer_query));
> > + ldr_xfer_query.hdr.command = LOADER_CMD_XFER_QUERY;
> > + ldr_xfer_query.image_size = fw->size;
> > + rv = loader_cl_send(client_data,
> > + (u8 *)&ldr_xfer_query,
> > + sizeof(ldr_xfer_query));
> > + if (rv < 0) {
> > + client_data->flag_retry = true;
> > + goto end_error;
> > + }
> > +
> > + /* Check buffer size before accessing the elements */
> > + data_len = client_data->response_size;
Use rv instead of client_data->response_size, we want to minimize weird
unexplainable accesses of the fileds of client_data.
Also consider not using the variable data_len, it doesn't do too much besides
cause some indirection. With the change above it should be obvious
what is going on.
> > + if (data_len != sizeof(struct loader_xfer_query_response)) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "data size %zu is not equal to size of
> > loader_xfer_query_response %zu\n",
> > + data_len, sizeof(struct
> > loader_xfer_query_response));
> > + hdr = (struct loader_msg_hdr *)client_data-
> > >response_data;
Following suggestion above you'll use the
> > + report_bad_packet(client_data->loader_ishtp_cl, hdr);
> > + client_data->flag_retry = true;
> > + rv = -EMSGSIZE;
> > + goto end_error;
> > + }
> > +
> > + /* Save fw_info for use outside this function */
> > + ldr_xfer_query_resp =
> > + (struct loader_xfer_query_response *)client_data-
> > >response_data;
> > + *fw_info = ldr_xfer_query_resp->fw_info;
> > +
> > + /* Loader firmware properties */
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "ish_fw_version: major=%d minor=%d hotfix=%d build=%d
> > protocol_version=0x%x loader_version=%d\n",
> > + fw_info->ish_fw_version.major,
> > + fw_info->ish_fw_version.minor,
> > + fw_info->ish_fw_version.hotfix,
> > + fw_info->ish_fw_version.build,
> > + fw_info->protocol_version,
> > + fw_info->ldr_version.value);
> > +
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "loader_capability: max_fw_image_size=0x%x xfer_mode=%d
> > max_dma_buf_size=0x%x dma_buf_size_limit=0x%x\n",
> > + fw_info->ldr_capability.max_fw_image_size,
> > + fw_info->ldr_capability.xfer_mode,
> > + fw_info->ldr_capability.max_dma_buf_size,
> > + dma_buf_size_limit);
> > +
> > + /* Sanity checks */
> > + if (fw_info->ldr_capability.max_fw_image_size < fw->size) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "ISH firmware size %zu is greater than Shim
> > firmware loader max supported %d\n",
> > + fw->size,
> > + fw_info->ldr_capability.max_fw_image_size);
> > + rv = -ENOSPC;
> > + goto end_error;
> > + }
> > +
> > + /* For DMA the buffer size should be multiple of cacheline size
> > */
> > + if ((fw_info->ldr_capability.xfer_mode &
> > LOADER_XFER_MODE_DIRECT_DMA) &&
> > + (fw_info->ldr_capability.max_dma_buf_size %
> > L1_CACHE_BYTES)) {
> > + dev_err(cl_data_to_dev(client_data),
> > + "Shim firmware loader buffer size %d should be
> > multipe of cacheline\n",
> > + fw_info->ldr_capability.max_dma_buf_size);
> > + rv = -EINVAL;
> > + goto end_error;
> > + }
> > +
> > +end_error:
> > + /* Free ISH buffer if not done so in error case */
> > + kfree(client_data->response_data);
> > + client_data->response_data = NULL;
> > + return rv;
> > +}
> > +
> > +/**
> > + * ish_fw_xfer_ishtp() Loads ISH firmware using ishtp
> > interface
> > + * @client_data: Client data instance
> > + * @fw: Pointer to fw data struct in host
> > memory
> > + *
> > + * This function uses ISH-TP to transfer ISH firmware from host to
> > + * ISH SRAM. Lower layers may use IPC or DMA depending on firmware
> > + * support.
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int ish_fw_xfer_ishtp(struct ishtp_cl_data *client_data,
> > + const struct firmware *fw)
> > +{
> > + int rv;
> > + u32 fragment_offset, fragment_size, payload_max_size;
> > + struct loader_xfer_ipc_fragment *ldr_xfer_ipc_frag;
> > +
> > + payload_max_size =
> > + LOADER_SHIM_IPC_BUF_SIZE - IPC_FRAGMENT_DATA_PREAMBLE;
> > +
> > + ldr_xfer_ipc_frag = kzalloc(LOADER_SHIM_IPC_BUF_SIZE,
> > GFP_KERNEL);
> > + if (!ldr_xfer_ipc_frag) {
> > + client_data->flag_retry = true;
> > + return -ENOMEM;
> > + }
> > +
> > + ldr_xfer_ipc_frag->fragment.hdr.command =
> > LOADER_CMD_XFER_FRAGMENT;
> > + ldr_xfer_ipc_frag->fragment.xfer_mode = LOADER_XFER_MODE_ISHTP;
> > +
> > + /* Break the firmware image into fragments and send as ISH-TP
> > payload */
> > + fragment_offset = 0;
> > + while (fragment_offset < fw->size) {
> > + if (fragment_offset + payload_max_size < fw->size) {
> > + fragment_size = payload_max_size;
> > + ldr_xfer_ipc_frag->fragment.is_last = 0;
> > + } else {
> > + fragment_size = fw->size - fragment_offset;
> > + ldr_xfer_ipc_frag->fragment.is_last = 1;
> > + }
> > +
> > + ldr_xfer_ipc_frag->fragment.offset = fragment_offset;
> > + ldr_xfer_ipc_frag->fragment.size = fragment_size;
> > + memcpy(ldr_xfer_ipc_frag->data,
> > + &fw->data[fragment_offset],
> > + fragment_size);
> > +
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "xfer_mode=ipc offset=0x%08x size=0x%08x
> > is_last=%d\n",
> > + ldr_xfer_ipc_frag->fragment.offset,
> > + ldr_xfer_ipc_frag->fragment.size,
> > + ldr_xfer_ipc_frag->fragment.is_last);
> > +
> > + rv = loader_cl_send(client_data,
> > + (u8 *)ldr_xfer_ipc_frag,
> > + IPC_FRAGMENT_DATA_PREAMBLE +
> > fragment_size);
> > + if (rv < 0) {
> > + client_data->flag_retry = true;
> > + goto end_err_resp_buf_release;
> > + }
> > +
> > + /* Free ISH buffer once response is processed */
> > + kfree(client_data->response_data);
> > + client_data->response_data = NULL;
> > +
> > + fragment_offset += fragment_size;
> > + }
> > +
> > + kfree(ldr_xfer_ipc_frag);
> > + return 0;
> > +
> > +end_err_resp_buf_release:
> > + /* Free ISH buffer if not done already, in error case */
> > + kfree(client_data->response_data);
> > + client_data->response_data = NULL;
> > + kfree(ldr_xfer_ipc_frag);
> > + return rv;
> > +}
> > +
> > +/**
> > + * ish_fw_xfer_direct_dma() - Loads ISH firmware using direct dma
> > + * @client_data: Client data instance
> > + * @fw: Poiner to fw data struct in host memory
> > + *
> > + * Host firmware load is a unique case where we need to download
> > + * a large firmware image (200+ Kb). This function implements
> > + * direct DMA transfer in kernel and ISH firmware. This allows
> > + * us to overcome the ISH-TP 4 Kb limit, and allows us to DMA
> > + * directly to ISH UMA at location of choice.
> > + * Function depends on corresponding support in ISH firmware.
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int ish_fw_xfer_direct_dma(struct ishtp_cl_data *client_data,
> > + const struct firmware *fw,
> > + struct shim_fw_info fw_info)
> > +{
> > + int rv;
> > + void *dma_buf;
> > + dma_addr_t dma_buf_phy;
> > + u32 fragment_offset, fragment_size, payload_max_size;
> > + struct loader_xfer_dma_fragment ldr_xfer_dma_frag;
> > + struct device *devc = ishtp_get_pci_device(client_data-
> > >cl_device);
> > + u32 shim_fw_buf_size =
> > + fw_info.ldr_capability.max_dma_buf_size;
> > +
> > + /*
> > + * payload_max_size should be set to minimum of
> > + * (1) Size of firmware to be loaded,
> > + * (2) Max DMA buf size supported by Shim firmware,
> > + * (3) DMA buffer size limit set by boot_param
> > dma_buf_size_limit.
> > + */
> > + payload_max_size = min3(fw->size,
> > + (size_t)shim_fw_buf_size,
> > + (size_t)dma_buf_size_limit);
> > +
> > + /*
> > + * Buffer size should be multiple of cacheline size
> > + * if it's not, select the previous cacheline boundary.
> > + */
> > + payload_max_size &= ~(L1_CACHE_BYTES - 1);
> > +
> > + dma_buf = kmalloc(payload_max_size, GFP_KERNEL | GFP_DMA32);
> > + if (!dma_buf) {
> > + client_data->flag_retry = true;
> > + return -ENOMEM;
> > + }
> > +
> > + dma_buf_phy = dma_map_single(devc, dma_buf, payload_max_size,
> > + DMA_TO_DEVICE);
> > + if (dma_mapping_error(devc, dma_buf_phy)) {
> > + dev_err(cl_data_to_dev(client_data), "DMA map
> > failed\n");
> > + client_data->flag_retry = true;
> > + rv = -ENOMEM;
> > + goto end_err_dma_buf_release;
> > + }
> > +
> > + ldr_xfer_dma_frag.fragment.hdr.command =
> > LOADER_CMD_XFER_FRAGMENT;
> > + ldr_xfer_dma_frag.fragment.xfer_mode =
> > LOADER_XFER_MODE_DIRECT_DMA;
> > + ldr_xfer_dma_frag.ddr_phys_addr = (u64)dma_buf_phy;
> > +
> > + /* Send the firmware image in chucks of payload_max_size */
> > + fragment_offset = 0;
> > + while (fragment_offset < fw->size) {
> > + if (fragment_offset + payload_max_size < fw->size) {
> > + fragment_size = payload_max_size;
> > + ldr_xfer_dma_frag.fragment.is_last = 0;
> > + } else {
> > + fragment_size = fw->size - fragment_offset;
> > + ldr_xfer_dma_frag.fragment.is_last = 1;
> > + }
> > +
> > + ldr_xfer_dma_frag.fragment.offset = fragment_offset;
> > + ldr_xfer_dma_frag.fragment.size = fragment_size;
> > + memcpy(dma_buf, &fw->data[fragment_offset],
> > fragment_size);
> > +
> > + dma_sync_single_for_device(devc, dma_buf_phy,
> > + payload_max_size,
> > + DMA_TO_DEVICE);
> > +
> > + /*
> > + * Flush cache here because the
> > dma_sync_single_for_device()
> > + * does not do for x86.
> > + */
> > + clflush_cache_range(dma_buf, payload_max_size);
> > +
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "xfer_mode=dma offset=0x%08x size=0x%x
> > is_last=%d ddr_phys_addr=0x%016llx\n",
> > + ldr_xfer_dma_frag.fragment.offset,
> > + ldr_xfer_dma_frag.fragment.size,
> > + ldr_xfer_dma_frag.fragment.is_last,
> > + ldr_xfer_dma_frag.ddr_phys_addr);
> > +
> > + rv = loader_cl_send(client_data,
> > + (u8 *)&ldr_xfer_dma_frag,
> > + sizeof(ldr_xfer_dma_frag));
> > + if (rv < 0) {
> > + client_data->flag_retry = true;
> > + goto end_err_resp_buf_release;
> > + }
> > +
> > + /* Free ISH buffer once response is processed */
> > + kfree(client_data->response_data);
> > + client_data->response_data = NULL;
> > +
> > + fragment_offset += fragment_size;
> > + }
> > +
> > + dma_unmap_single(devc, dma_buf_phy, payload_max_size,
> > DMA_TO_DEVICE);
> > + kfree(dma_buf);
> > + return 0;
> > +
> > +end_err_resp_buf_release:
> > + /* Free ISH buffer if not done already, in error case */
> > + kfree(client_data->response_data);
> > + client_data->response_data = NULL;
> > + dma_unmap_single(devc, dma_buf_phy, payload_max_size,
> > DMA_TO_DEVICE);
> > +end_err_dma_buf_release:
> > + kfree(dma_buf);
> > + return rv;
> > +}
> > +
> > +/**
> > + * ish_fw_start() Start executing ISH main firmware
> > + * @client_data: client data instance
> > + *
> > + * This function sends message to Shim firmware loader to start
> > + * the execution of ISH main firmware.
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int ish_fw_start(struct ishtp_cl_data *client_data)
> > +{
> > + int rv;
> > + struct loader_start ldr_start;
> > +
> > + memset(&ldr_start, 0, sizeof(ldr_start));
> > + ldr_start.hdr.command = LOADER_CMD_START;
> > + rv = loader_cl_send(client_data,
> > + (u8 *)&ldr_start,
> > + sizeof(ldr_start));
> > +
> > + /* Free ISH buffer once response is processed */
> > + kfree(client_data->response_data);
> > + client_data->response_data = NULL;
> > + return rv;
> > +}
> > +
> > +/**
> > + * load_fw_from_host() Loads ISH firmware from host
> > + * @client_data: Client data instance
> > + *
> > + * This function loads the ISH firmware to ISH sram and starts
> > execution
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int load_fw_from_host(struct ishtp_cl_data *client_data)
> > +{
> > + int rv;
> > + u32 xfer_mode;
> > + char *filename;
> > + const struct firmware *fw;
> > + struct shim_fw_info fw_info;
> > +
> > + client_data->flag_retry = false;
> > +
> > + filename = kzalloc(FILENAME_SIZE, GFP_KERNEL);
> > + if (!filename) {
> > + rv = -ENOMEM;
> > + goto end_error;
> > + }
> > +
> > + /* Get filename of the ISH firmware to be loaded */
> > + rv = get_firmware_variant(client_data, filename);
> > + if (rv < 0)
> > + goto end_err_filename_buf_release;
> > +
> > + rv = request_firmware(&fw, filename,
> > cl_data_to_dev(client_data));
> > + if (rv < 0)
> > + goto end_err_filename_buf_release;
> > +
> > + /* Step 1: Query Shim firmware loader properties */
> > +
> > + rv = ish_query_loader_prop(client_data, fw, &fw_info);
> > + if (rv < 0)
> > + goto end_err_fw_release;
> > +
> > + /* Step 2: Send the main firmware image to be loaded, to ISH
> > sram */
> > +
> > + xfer_mode = fw_info.ldr_capability.xfer_mode;
> > + if (xfer_mode & LOADER_XFER_MODE_DIRECT_DMA) {
> > + rv = ish_fw_xfer_direct_dma(client_data, fw, fw_info);
> > + } else if (xfer_mode & LOADER_XFER_MODE_ISHTP) {
> > + rv = ish_fw_xfer_ishtp(client_data, fw);
> > + } else {
> > + dev_err(cl_data_to_dev(client_data),
> > + "No transfer mode selected in firmware\n");
> > + rv = -EINVAL;
> > + }
> > + if (rv < 0)
> > + goto end_err_fw_release;
> > +
> > + /* Step 3: Start ISH main firmware exeuction */
> > +
> > + rv = ish_fw_start(client_data);
> > + if (rv < 0)
> > + goto end_err_fw_release;
> > +
> > + release_firmware(fw);
> > + kfree(filename);
> > + dev_info(cl_data_to_dev(client_data), "ISH firmware %s
> > loaded\n",
> > + filename);
> > + return 0;
> > +
> > +end_err_fw_release:
> > + release_firmware(fw);
> > +end_err_filename_buf_release:
> > + kfree(filename);
> > +end_error:
> > + if (client_data->flag_retry) {
> > + dev_warn(cl_data_to_dev(client_data),
> > + "ISH host firmware load failed %d. Reset ISH &
> > try again..\n",
> > + rv);
> > + loader_ish_hw_reset(client_data->loader_ishtp_cl);
This could just keep failing infinitely, right? Do you want to add
some retry counter,
and after some limit then give up or something? What happens if the ISH firmware
never succeeds in loading?
> > + } else {
> > + dev_err(cl_data_to_dev(client_data),
> > + "ISH host firmware load failed %d\n", rv);
> > + }
> > + return rv;
> > +}
And there were many typos in comments that I saw, comb through them
carefully again.
Cheers,
Nick
^ permalink raw reply
* [PATCH v3 0/4] Add Apple SPI keyboard and trackpad driver
From: Ronald Tschalär @ 2019-03-27 1:48 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
This changeset adds a driver for the SPI keyboard and trackpad on recent
MacBook's and MacBook Pro's. The driver has seen a fair amount of use
over the last 2 years (basically anybody running linux on these
machines), with only relatively small changes in the last year or so.
For those interested, the driver development has been hosted at
https://github.com/cb22/macbook12-spi-driver/ (as well as my clone at
https://github.com/roadrunner2/macbook12-spi-driver/).
The first patch is just a placeholder for now and is provided in case
somebody wants to compile the driver while it's being reviewed here; the
real patch has been submitted to dri-devel and is being discussed there,
with the intent/hope that I can get an Ack and permission to merge it
through the input subsystem tree here as part of this patch series.
The second and third patches add a new dev_print_hex_dump() helper as
the dev_xxx() analog of print_hex_dump().
The fourth patch finally contains the new applespi driver.
Changes in v3:
Applied all feedback from review by Andy Shevchenko, including:
- move dev_print_hex_dump() to driver core
- clean up keyboard modifier bits testing/modifying
- remove DEV() macro
- minor style issues
The full set of changes to applespi can be viewed at
https://github.com/roadrunner2/macbook12-spi-driver/ as individual
commits f832caa..3a6262e in the upstreaming-review branch.
Ronald Tschalär (4):
drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
lib/hexdump.c: factor out generic hexdump formatting for reuse.
driver core: add dev_print_hex_dump() logging function.
Input: add Apple SPI keyboard and trackpad driver.
drivers/base/core.c | 43 +
drivers/gpu/drm/bridge/Kconfig | 2 +-
drivers/input/keyboard/Kconfig | 15 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/applespi.c | 1988 +++++++++++++++++++++++++++++
include/linux/device.h | 15 +
include/linux/printk.h | 12 +
lib/hexdump.c | 95 +-
8 files changed, 2146 insertions(+), 25 deletions(-)
create mode 100644 drivers/input/keyboard/applespi.c
--
2.20.1
^ permalink raw reply
* [PATCH v3 1/4] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
From: Ronald Tschalär @ 2019-03-27 1:48 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel,
Inki Dae, Andrzej Hajda
In-Reply-To: <20190327014807.7472-1-ronald@innovation.ch>
commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
INPUT. However, this causes problems with other drivers, in particular
an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
future commit):
drivers/clk/Kconfig:9:error: recursive dependency detected!
drivers/clk/Kconfig:9: symbol COMMON_CLK is selected by MFD_INTEL_LPSS
drivers/mfd/Kconfig:566: symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
drivers/mfd/Kconfig:580: symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
drivers/input/keyboard/Kconfig:73: symbol KEYBOARD_APPLESPI depends on INPUT
drivers/input/Kconfig:8: symbol INPUT is selected by DRM_SIL_SII8620
drivers/gpu/drm/bridge/Kconfig:83: symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
drivers/gpu/drm/bridge/Kconfig:1: symbol DRM_BRIDGE is selected by DRM_PL111
drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on COMMON_CLK
According to the docs, select should only be used for non-visible
symbols. Furthermore almost all other references to INPUT throughout the
kernel config are depends, not selects. Hence this change.
CC: Inki Dae <inki.dae@samsung.com>
CC: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
drivers/gpu/drm/bridge/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 2fee47b0d50b..eabedc83f25c 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
config DRM_SIL_SII8620
tristate "Silicon Image SII8620 HDMI/MHL bridge"
depends on OF
+ depends on INPUT
select DRM_KMS_HELPER
imply EXTCON
- select INPUT
select RC_CORE
help
Silicon Image SII8620 HDMI/MHL bridge chip driver.
--
2.20.1
^ permalink raw reply related
* [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.
From: Ronald Tschalär @ 2019-03-27 1:48 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327014807.7472-1-ronald@innovation.ch>
This introduces print_hex_dump_to_cb() which contains all the hexdump
formatting minus the actual printk() call, allowing an arbitrary print
function to be supplied instead. And print_hex_dump() is re-implemented
using print_hex_dump_to_cb().
This allows other hex-dump logging functions to be provided which call
printk() differently or even log the hexdump somewhere entirely
different.
---
include/linux/printk.h | 12 ++++++
lib/hexdump.c | 95 +++++++++++++++++++++++++++++++-----------
2 files changed, 83 insertions(+), 24 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 77740a506ebb..4ebdacd7a287 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -483,10 +483,16 @@ enum {
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
bool ascii);
+typedef
+void (*hex_dump_callback)(const char *level, void *arg, const char *fmt, ...);
#ifdef CONFIG_PRINTK
extern void print_hex_dump(const char *level, const char *prefix_str,
int prefix_type, int rowsize, int groupsize,
const void *buf, size_t len, bool ascii);
+extern void print_hex_dump_to_cb(const char *level, const char *prefix_str,
+ int prefix_type, int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii,
+ hex_dump_callback print, void *print_arg);
#if defined(CONFIG_DYNAMIC_DEBUG)
#define print_hex_dump_bytes(prefix_str, prefix_type, buf, len) \
dynamic_hex_dump(prefix_str, prefix_type, 16, 1, buf, len, true)
@@ -500,6 +506,12 @@ static inline void print_hex_dump(const char *level, const char *prefix_str,
const void *buf, size_t len, bool ascii)
{
}
+extern void print_hex_dump_to_cb(const char *level, const char *prefix_str,
+ int prefix_type, int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii,
+ hex_dump_callback *print, void *print_arg);
+{
+}
static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
const void *buf, size_t len)
{
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 81b70ed37209..43583cf6accd 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -210,7 +210,8 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
#ifdef CONFIG_PRINTK
/**
- * print_hex_dump - print a text hex dump to syslog for a binary blob of data
+ * print_hex_dump_to_cb - print a text hex dump using given callback for a
+ * binary blob of data
* @level: kernel log level (e.g. KERN_DEBUG)
* @prefix_str: string to prefix each line with;
* caller supplies trailing spaces for alignment if desired
@@ -221,28 +222,18 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @ascii: include ASCII after the hex output
+ * @print: the print function, called once for each line
+ * @print_arg: an arbitrary argument to pass to the print function
*
- * Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
- * to the kernel log at the specified kernel log level, with an optional
- * leading prefix.
- *
- * print_hex_dump() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
- * print_hex_dump() iterates over the entire input @buf, breaking it into
- * "line size" chunks to format and print.
- *
- * E.g.:
- * print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
- * 16, 1, frame->data, frame->len, true);
+ * This is a low level helper function - normally you want to use
+ * print_hex_dump() or other wrapper around it.
*
- * Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
- * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f @ABCDEFGHIJKLMNO
- * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
- * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c pqrstuvwxyz{|}~.
+ * See print_hex_dump() for more details and examples.
*/
-void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
- int rowsize, int groupsize,
- const void *buf, size_t len, bool ascii)
+void print_hex_dump_to_cb(const char *level, const char *prefix_str,
+ int prefix_type, int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii,
+ hex_dump_callback print, void *print_arg)
{
const u8 *ptr = buf;
int i, linelen, remaining = len;
@@ -260,18 +251,74 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
switch (prefix_type) {
case DUMP_PREFIX_ADDRESS:
- printk("%s%s%p: %s\n",
- level, prefix_str, ptr + i, linebuf);
+ print(level, print_arg, "%s%p: %s\n", prefix_str,
+ ptr + i, linebuf);
break;
case DUMP_PREFIX_OFFSET:
- printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf);
+ print(level, print_arg, "%s%.8x: %s\n", prefix_str, i,
+ linebuf);
break;
default:
- printk("%s%s%s\n", level, prefix_str, linebuf);
+ print(level, print_arg, "%s%s\n", prefix_str, linebuf);
break;
}
}
}
+EXPORT_SYMBOL(print_hex_dump_to_cb);
+
+static void print_to_printk(const char *level, void *arg, const char *fmt, ...)
+{
+ va_list args;
+ struct va_format vaf;
+
+ va_start(args, fmt);
+
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ printk("%s%pV", level, &vaf);
+
+ va_end(args);
+}
+
+/**
+ * print_hex_dump - print a text hex dump to syslog for a binary blob of data
+ * @level: kernel log level (e.g. KERN_DEBUG)
+ * @prefix_str: string to prefix each line with;
+ * caller supplies trailing spaces for alignment if desired
+ * @prefix_type: controls whether prefix of an offset, address, or none
+ * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @ascii: include ASCII after the hex output
+ *
+ * Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
+ * to the kernel log at the specified kernel log level, with an optional
+ * leading prefix.
+ *
+ * print_hex_dump() works on one "line" of output at a time, i.e.,
+ * 16 or 32 bytes of input data converted to hex + ASCII output.
+ * print_hex_dump() iterates over the entire input @buf, breaking it into
+ * "line size" chunks to format and print.
+ *
+ * E.g.:
+ * print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
+ * 16, 1, frame->data, frame->len, true);
+ *
+ * Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
+ * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f @ABCDEFGHIJKLMNO
+ * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
+ * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c pqrstuvwxyz{|}~.
+ */
+void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii)
+{
+ print_hex_dump_to_cb(level, prefix_str, prefix_type, rowsize, groupsize,
+ buf, len, ascii, print_to_printk, NULL);
+}
EXPORT_SYMBOL(print_hex_dump);
#if !defined(CONFIG_DYNAMIC_DEBUG)
--
2.20.1
^ permalink raw reply related
* [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.
From: Ronald Tschalär @ 2019-03-27 1:48 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327014807.7472-1-ronald@innovation.ch>
This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
instead of straight printk() to match other dev_xxx() logging functions.
---
drivers/base/core.c | 43 ++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 15 +++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0073b09bb99f..eb260d482750 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3097,6 +3097,49 @@ define_dev_printk_level(_dev_warn, KERN_WARNING);
define_dev_printk_level(_dev_notice, KERN_NOTICE);
define_dev_printk_level(_dev_info, KERN_INFO);
+static void
+print_to_dev_printk(const char *level, void *arg, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+
+ vaf.fmt = fmt;
+ vaf.va = &args;
+
+ __dev_printk(level, (const struct device *)arg, &vaf);
+
+ va_end(args);
+}
+
+/**
+ * _dev_print_hex_dump - print a text hex dump to syslog for a binary blob of
+ * data
+ * @level: kernel log level (e.g. KERN_DEBUG)
+ * @dev: the device to which this log message pertains
+ * @prefix_str: string to prefix each line with;
+ * caller supplies trailing spaces for alignment if desired
+ * @prefix_type: controls whether prefix of an offset, address, or none
+ * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @ascii: include ASCII after the hex output
+ *
+ * See print_hex_dump() for additional details and examples.
+ */
+void _dev_print_hex_dump(const char *level, const struct device *dev,
+ const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii)
+{
+ print_hex_dump_to_cb(level, prefix_str, prefix_type, rowsize, groupsize,
+ buf, len, ascii, print_to_dev_printk, (void *)dev);
+}
+EXPORT_SYMBOL(_dev_print_hex_dump);
+
#endif
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
diff --git a/include/linux/device.h b/include/linux/device.h
index 6cb4640b6160..dc6fcae3002a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1405,6 +1405,10 @@ __printf(2, 3)
void _dev_notice(const struct device *dev, const char *fmt, ...);
__printf(2, 3)
void _dev_info(const struct device *dev, const char *fmt, ...);
+void _dev_print_hex_dump(const char *level, const struct device *dev,
+ const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii);
#else
@@ -1445,6 +1449,12 @@ void _dev_notice(const struct device *dev, const char *fmt, ...)
static inline __printf(2, 3)
void _dev_info(const struct device *dev, const char *fmt, ...)
{}
+static inline
+void _dev_print_hex_dump(const char *level, const struct device *dev,
+ const char *prefix_str, int prefix_type,
+ int rowsize, int groupsize,
+ const void *buf, size_t len, bool ascii);
+{}
#endif
@@ -1467,6 +1477,11 @@ void _dev_info(const struct device *dev, const char *fmt, ...)
_dev_notice(dev, dev_fmt(fmt), ##__VA_ARGS__)
#define dev_info(dev, fmt, ...) \
_dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
+#define dev_print_hex_dump(level, dev, prefix_str, prefix_type, \
+ rowsize, groupsize, buf, len, ascii) \
+ _dev_print_hex_dump(level, dev, dev_fmt(prefix_str), \
+ prefix_type, rowsize, groupsize, buf, len, \
+ ascii);
#if defined(CONFIG_DYNAMIC_DEBUG)
#define dev_dbg(dev, fmt, ...) \
--
2.20.1
^ permalink raw reply related
* [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Ronald Tschalär @ 2019-03-27 1:48 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki
Cc: Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327014807.7472-1-ronald@innovation.ch>
The keyboard and trackpad on recent MacBook's (since 8,1) and
MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
of USB, as previously. The higher level protocol is not publicly
documented and hence has been reverse engineered. As a consequence there
are still a number of unknown fields and commands. However, the known
parts have been working well and received extensive testing and use.
In order for this driver to work, the proper SPI drivers need to be
loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
reason enabling this driver in the config implies enabling the above
drivers.
CC: Federico Lorenzi <federico@travelground.com>
CC: Lukas Wunner <lukas@wunner.de>
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=99891
Link: https://bugzilla.kernel.org/show_bug.cgi?id=108331
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
drivers/input/keyboard/Kconfig | 15 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/applespi.c | 1988 +++++++++++++++++++++++++++++
3 files changed, 2004 insertions(+)
create mode 100644 drivers/input/keyboard/applespi.c
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a878351f1643..d0a9e7fa2508 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -70,6 +70,21 @@ config KEYBOARD_AMIGA
config ATARI_KBD_CORE
bool
+config KEYBOARD_APPLESPI
+ tristate "Apple SPI keyboard and trackpad"
+ depends on ACPI && EFI
+ depends on SPI
+ depends on X86 || COMPILE_TEST
+ imply SPI_PXA2XX
+ imply SPI_PXA2XX_PCI
+ imply MFD_INTEL_LPSS_PCI
+ help
+ Say Y here if you are running Linux on any Apple MacBook8,1 or later,
+ or any MacBookPro13,* or MacBookPro14,*.
+
+ To compile this driver as a module, choose M here: the
+ module will be called applespi.
+
config KEYBOARD_ATARI
tristate "Atari keyboard"
depends on ATARI
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 182e92985dbf..9283fee2505a 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_KEYBOARD_ADP5520) += adp5520-keys.o
obj-$(CONFIG_KEYBOARD_ADP5588) += adp5588-keys.o
obj-$(CONFIG_KEYBOARD_ADP5589) += adp5589-keys.o
obj-$(CONFIG_KEYBOARD_AMIGA) += amikbd.o
+obj-$(CONFIG_KEYBOARD_APPLESPI) += applespi.o
obj-$(CONFIG_KEYBOARD_ATARI) += atakbd.o
obj-$(CONFIG_KEYBOARD_ATKBD) += atkbd.o
obj-$(CONFIG_KEYBOARD_BCM) += bcm-keypad.o
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
new file mode 100644
index 000000000000..39e38d98869e
--- /dev/null
+++ b/drivers/input/keyboard/applespi.c
@@ -0,0 +1,1988 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MacBook (Pro) SPI keyboard and touchpad driver
+ *
+ * Copyright (c) 2015-2018 Federico Lorenzi
+ * Copyright (c) 2017-2018 Ronald Tschalär
+ */
+
+/*
+ * The keyboard and touchpad controller on the MacBookAir6, MacBookPro12,
+ * MacBook8 and newer can be driven either by USB or SPI. However the USB
+ * pins are only connected on the MacBookAir6 and 7 and the MacBookPro12.
+ * All others need this driver. The interface is selected using ACPI methods:
+ *
+ * * UIEN ("USB Interface Enable"): If invoked with argument 1, disables SPI
+ * and enables USB. If invoked with argument 0, disables USB.
+ * * UIST ("USB Interface Status"): Returns 1 if USB is enabled, 0 otherwise.
+ * * SIEN ("SPI Interface Enable"): If invoked with argument 1, disables USB
+ * and enables SPI. If invoked with argument 0, disables SPI.
+ * * SIST ("SPI Interface Status"): Returns 1 if SPI is enabled, 0 otherwise.
+ * * ISOL: Resets the four GPIO pins used for SPI. Intended to be invoked with
+ * argument 1, then once more with argument 0.
+ *
+ * UIEN and UIST are only provided on models where the USB pins are connected.
+ *
+ * SPI-based Protocol
+ * ------------------
+ *
+ * The device and driver exchange messages (struct message); each message is
+ * encapsulated in one or more packets (struct spi_packet). There are two types
+ * of exchanges: reads, and writes. A read is signaled by a GPE, upon which one
+ * message can be read from the device. A write exchange consists of writing a
+ * command message, immediately reading a short status packet, and then, upon
+ * receiving a GPE, reading the response message. Write exchanges cannot be
+ * interleaved, i.e. a new write exchange must not be started till the previous
+ * write exchange is complete. Whether a received message is part of a read or
+ * write exchange is indicated in the encapsulating packet's flags field.
+ *
+ * A single message may be too large to fit in a single packet (which has a
+ * fixed, 256-byte size). In that case it will be split over multiple,
+ * consecutive packets.
+ */
+
+#include <linux/acpi.h>
+#include <linux/crc16.h>
+#include <linux/delay.h>
+#include <linux/efi.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/ktime.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/spi/spi.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include <asm/barrier.h>
+#include <asm-generic/unaligned.h>
+
+#define APPLESPI_PACKET_SIZE 256
+#define APPLESPI_STATUS_SIZE 4
+
+#define PACKET_TYPE_READ 0x20
+#define PACKET_TYPE_WRITE 0x40
+#define PACKET_DEV_KEYB 0x01
+#define PACKET_DEV_TPAD 0x02
+#define PACKET_DEV_INFO 0xd0
+
+#define MAX_ROLLOVER 6
+
+#define MAX_FINGERS 11
+#define MAX_FINGER_ORIENTATION 16384
+#define MAX_PKTS_PER_MSG 2
+
+#define KBD_BL_LEVEL_MIN 32U
+#define KBD_BL_LEVEL_MAX 255U
+#define KBD_BL_LEVEL_SCALE 1000000U
+#define KBD_BL_LEVEL_ADJ \
+ ((KBD_BL_LEVEL_MAX - KBD_BL_LEVEL_MIN) * KBD_BL_LEVEL_SCALE / 255U)
+
+#define EFI_BL_LEVEL_NAME L"KeyboardBacklightLevel"
+#define EFI_BL_LEVEL_GUID EFI_GUID(0xa076d2af, 0x9678, 0x4386, 0x8b, 0x58, 0x1f, 0xc8, 0xef, 0x04, 0x16, 0x19)
+
+#define DBG_CMD_TP_INI BIT(0)
+#define DBG_CMD_BL BIT(1)
+#define DBG_CMD_CL BIT(2)
+#define DBG_RD_KEYB BIT(8)
+#define DBG_RD_TPAD BIT(9)
+#define DBG_RD_UNKN BIT(10)
+#define DBG_RD_IRQ BIT(11)
+#define DBG_RD_CRC BIT(12)
+#define DBG_TP_DIM BIT(16)
+
+#define debug_print(mask, applespi, fmt, ...) \
+ do { \
+ if (debug & (mask)) \
+ dev_printk(KERN_DEBUG, &(applespi)->spi->dev, fmt, \
+ ##__VA_ARGS__); \
+ } while (0)
+
+#define debug_print_header(mask, applespi) \
+ debug_print(mask, applespi, "--- %s ---------------------------\n", \
+ applespi_debug_facility(mask))
+
+#define debug_print_buffer(mask, applespi, fmt, buf, len) \
+ do { \
+ if (debug & (mask)) \
+ dev_print_hex_dump(KERN_DEBUG, &(applespi)->spi->dev, \
+ fmt, DUMP_PREFIX_NONE, 32, 1, buf, \
+ len, false); \
+ } while (0)
+
+#define APPLE_FLAG_FKEY 0x01
+
+#define SPI_RW_CHG_DELAY_US 100 /* from experimentation, in µs */
+
+#define SYNAPTICS_VENDOR_ID 0x06cb
+
+static unsigned int fnmode = 1;
+module_param(fnmode, uint, 0644);
+MODULE_PARM_DESC(fnmode, "Mode of Fn key on Apple keyboards (0 = disabled, [1] = fkeyslast, 2 = fkeysfirst)");
+
+static unsigned int fnremap;
+module_param(fnremap, uint, 0644);
+MODULE_PARM_DESC(fnremap, "Remap Fn key ([0] = no-remap; 1 = left-ctrl, 2 = left-shift, 3 = left-alt, 4 = left-meta, 6 = right-shift, 7 = right-alt, 8 = right-meta)");
+
+static bool iso_layout;
+module_param(iso_layout, bool, 0644);
+MODULE_PARM_DESC(iso_layout, "Enable/Disable hardcoded ISO-layout of the keyboard. ([0] = disabled, 1 = enabled)");
+
+static unsigned int debug;
+module_param(debug, uint, 0644);
+MODULE_PARM_DESC(debug, "Enable/Disable debug logging. This is a bitmask.");
+
+static char touchpad_dimensions[40];
+module_param_string(touchpad_dimensions, touchpad_dimensions,
+ sizeof(touchpad_dimensions), 0444);
+MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as XxY+W+H .");
+
+/**
+ * struct keyboard_protocol - keyboard message.
+ * message.type = 0x0110, message.length = 0x000a
+ *
+ * @unknown1: unknown
+ * @modifiers: bit-set of modifier/control keys pressed
+ * @unknown2: unknown
+ * @keys_pressed: the (non-modifier) keys currently pressed
+ * @fn_pressed: whether the fn key is currently pressed
+ * @crc16: crc over the whole message struct (message header +
+ * this struct) minus this @crc16 field
+ */
+struct keyboard_protocol {
+ __u8 unknown1;
+ __u8 modifiers;
+ __u8 unknown2;
+ __u8 keys_pressed[MAX_ROLLOVER];
+ __u8 fn_pressed;
+ __le16 crc16;
+};
+
+/**
+ * struct tp_finger - single trackpad finger structure, le16-aligned
+ *
+ * @origin: zero when switching track finger
+ * @abs_x: absolute x coodinate
+ * @abs_y: absolute y coodinate
+ * @rel_x: relative x coodinate
+ * @rel_y: relative y coodinate
+ * @tool_major: tool area, major axis
+ * @tool_minor: tool area, minor axis
+ * @orientation: 16384 when point, else 15 bit angle
+ * @touch_major: touch area, major axis
+ * @touch_minor: touch area, minor axis
+ * @unused: zeros
+ * @pressure: pressure on forcetouch touchpad
+ * @multi: one finger: varies, more fingers: constant
+ * @crc16: on last finger: crc over the whole message struct
+ * (i.e. message header + this struct) minus the last
+ * @crc16 field; unknown on all other fingers.
+ */
+struct tp_finger {
+ __le16 origin;
+ __le16 abs_x;
+ __le16 abs_y;
+ __le16 rel_x;
+ __le16 rel_y;
+ __le16 tool_major;
+ __le16 tool_minor;
+ __le16 orientation;
+ __le16 touch_major;
+ __le16 touch_minor;
+ __le16 unused[2];
+ __le16 pressure;
+ __le16 multi;
+ __le16 crc16;
+};
+
+/**
+ * struct touchpad_protocol - touchpad message.
+ * message.type = 0x0210
+ *
+ * @unknown1: unknown
+ * @clicked: 1 if a button-click was detected, 0 otherwise
+ * @unknown2: unknown
+ * @number_of_fingers: the number of fingers being reported in @fingers
+ * @clicked2: same as @clicked
+ * @unknown3: unknown
+ * @fingers: the data for each finger
+ */
+struct touchpad_protocol {
+ __u8 unknown1[1];
+ __u8 clicked;
+ __u8 unknown2[28];
+ __u8 number_of_fingers;
+ __u8 clicked2;
+ __u8 unknown3[16];
+ struct tp_finger fingers[0];
+};
+
+/**
+ * struct command_protocol_tp_info - get touchpad info.
+ * message.type = 0x1020, message.length = 0x0000
+ *
+ * @crc16: crc over the whole message struct (message header +
+ * this struct) minus this @crc16 field
+ */
+struct command_protocol_tp_info {
+ __le16 crc16;
+};
+
+/**
+ * struct touchpad_info - touchpad info response.
+ * message.type = 0x1020, message.length = 0x006e
+ *
+ * @unknown1: unknown
+ * @model_flags: flags (vary by model number, but significance otherwise
+ * unknown)
+ * @model_no: the touchpad model number
+ * @unknown2: unknown
+ * @crc16: crc over the whole message struct (message header +
+ * this struct) minus this @crc16 field
+ */
+struct touchpad_info_protocol {
+ __u8 unknown1[105];
+ __u8 model_flags;
+ __u8 model_no;
+ __u8 unknown2[3];
+ __le16 crc16;
+};
+
+/**
+ * struct command_protocol_mt_init - initialize multitouch.
+ * message.type = 0x0252, message.length = 0x0002
+ *
+ * @cmd: value: 0x0102
+ * @crc16: crc over the whole message struct (message header +
+ * this struct) minus this @crc16 field
+ */
+struct command_protocol_mt_init {
+ __le16 cmd;
+ __le16 crc16;
+};
+
+/**
+ * struct command_protocol_capsl - toggle caps-lock led
+ * message.type = 0x0151, message.length = 0x0002
+ *
+ * @unknown: value: 0x01 (length?)
+ * @led: 0 off, 2 on
+ * @crc16: crc over the whole message struct (message header +
+ * this struct) minus this @crc16 field
+ */
+struct command_protocol_capsl {
+ __u8 unknown;
+ __u8 led;
+ __le16 crc16;
+};
+
+/**
+ * struct command_protocol_bl - set keyboard backlight brightness
+ * message.type = 0xB051, message.length = 0x0006
+ *
+ * @const1: value: 0x01B0
+ * @level: the brightness level to set
+ * @const2: value: 0x0001 (backlight off), 0x01F4 (backlight on)
+ * @crc16: crc over the whole message struct (message header +
+ * this struct) minus this @crc16 field
+ */
+struct command_protocol_bl {
+ __le16 const1;
+ __le16 level;
+ __le16 const2;
+ __le16 crc16;
+};
+
+/**
+ * struct message - a complete spi message.
+ *
+ * Each message begins with fixed header, followed by a message-type specific
+ * payload, and ends with a 16-bit crc. Because of the varying lengths of the
+ * payload, the crc is defined at the end of each payload struct, rather than
+ * in this struct.
+ *
+ * @type: the message type
+ * @zero: always 0
+ * @counter: incremented on each message, rolls over after 255; there is a
+ * separate counter for each message type.
+ * @rsp_buf_len:response buffer length (the exact nature of this field is quite
+ * speculative). On a request/write this is often the same as
+ * @length, though in some cases it has been seen to be much larger
+ * (e.g. 0x400); on a response/read this the same as on the
+ * request; for reads that are not responses it is 0.
+ * @length: length of the remainder of the data in the whole message
+ * structure (after re-assembly in case of being split over
+ * multiple spi-packets), minus the trailing crc. The total size
+ * of the message struct is therefore @length + 10.
+ */
+struct message {
+ __le16 type;
+ __u8 zero;
+ __u8 counter;
+ __le16 rsp_buf_len;
+ __le16 length;
+ union {
+ struct keyboard_protocol keyboard;
+ struct touchpad_protocol touchpad;
+ struct touchpad_info_protocol tp_info;
+ struct command_protocol_tp_info tp_info_command;
+ struct command_protocol_mt_init init_mt_command;
+ struct command_protocol_capsl capsl_command;
+ struct command_protocol_bl bl_command;
+ __u8 data[0];
+ };
+};
+
+/* type + zero + counter + rsp_buf_len + length */
+#define MSG_HEADER_SIZE 8
+
+/**
+ * struct spi_packet - a complete spi packet; always 256 bytes. This carries
+ * the (parts of the) message in the data. But note that this does not
+ * necessarily contain a complete message, as in some cases (e.g. many
+ * fingers pressed) the message is split over multiple packets (see the
+ * @offset, @remaining, and @length fields). In general the data parts in
+ * spi_packet's are concatenated until @remaining is 0, and the result is an
+ * message.
+ *
+ * @flags: 0x40 = write (to device), 0x20 = read (from device); note that
+ * the response to a write still has 0x40.
+ * @device: 1 = keyboard, 2 = touchpad
+ * @offset: specifies the offset of this packet's data in the complete
+ * message; i.e. > 0 indicates this is a continuation packet (in
+ * the second packet for a message split over multiple packets
+ * this would then be the same as the @length in the first packet)
+ * @remaining: number of message bytes remaining in subsequents packets (in
+ * the first packet of a message split over two packets this would
+ * then be the same as the @length in the second packet)
+ * @length: length of the valid data in the @data in this packet
+ * @data: all or part of a message
+ * @crc16: crc over this whole structure minus this @crc16 field. This
+ * covers just this packet, even on multi-packet messages (in
+ * contrast to the crc in the message).
+ */
+struct spi_packet {
+ __u8 flags;
+ __u8 device;
+ __le16 offset;
+ __le16 remaining;
+ __le16 length;
+ __u8 data[246];
+ __le16 crc16;
+};
+
+struct spi_settings {
+ u64 spi_cs_delay; /* cs-to-clk delay in us */
+ u64 reset_a2r_usec; /* active-to-receive delay? */
+ u64 reset_rec_usec; /* ? (cur val: 10) */
+};
+
+/* this mimics struct drm_rect */
+struct applespi_tp_info {
+ int x_min;
+ int y_min;
+ int x_max;
+ int y_max;
+};
+
+struct applespi_data {
+ struct spi_device *spi;
+ struct spi_settings spi_settings;
+ struct input_dev *keyboard_input_dev;
+ struct input_dev *touchpad_input_dev;
+
+ u8 *tx_buffer;
+ u8 *tx_status;
+ u8 *rx_buffer;
+
+ u8 *msg_buf;
+ unsigned int saved_msg_len;
+
+ struct applespi_tp_info tp_info;
+
+ u8 last_keys_pressed[MAX_ROLLOVER];
+ u8 last_keys_fn_pressed[MAX_ROLLOVER];
+ u8 last_fn_pressed;
+ struct input_mt_pos pos[MAX_FINGERS];
+ int slots[MAX_FINGERS];
+ acpi_handle handle;
+ int gpe;
+ acpi_handle sien;
+ acpi_handle sist;
+
+ struct spi_transfer dl_t;
+ struct spi_transfer rd_t;
+ struct spi_message rd_m;
+
+ struct spi_transfer ww_t;
+ struct spi_transfer wd_t;
+ struct spi_transfer wr_t;
+ struct spi_transfer st_t;
+ struct spi_message wr_m;
+
+ bool want_tp_info_cmd;
+ bool want_mt_init_cmd;
+ bool want_cl_led_on;
+ bool have_cl_led_on;
+ unsigned int want_bl_level;
+ unsigned int have_bl_level;
+ unsigned int cmd_msg_cntr;
+ /* lock to protect the above parameters and flags below */
+ spinlock_t cmd_msg_lock;
+ bool cmd_msg_queued;
+ unsigned int cmd_log_mask;
+
+ struct led_classdev backlight_info;
+
+ bool suspended;
+ bool drain;
+ wait_queue_head_t drain_complete;
+ bool read_active;
+ bool write_active;
+
+ struct work_struct work;
+ struct touchpad_info_protocol rcvd_tp_info;
+};
+
+static const unsigned char applespi_scancodes[] = {
+ 0, 0, 0, 0,
+ KEY_A, KEY_B, KEY_C, KEY_D, KEY_E, KEY_F, KEY_G, KEY_H, KEY_I, KEY_J,
+ KEY_K, KEY_L, KEY_M, KEY_N, KEY_O, KEY_P, KEY_Q, KEY_R, KEY_S, KEY_T,
+ KEY_U, KEY_V, KEY_W, KEY_X, KEY_Y, KEY_Z,
+ KEY_1, KEY_2, KEY_3, KEY_4, KEY_5, KEY_6, KEY_7, KEY_8, KEY_9, KEY_0,
+ KEY_ENTER, KEY_ESC, KEY_BACKSPACE, KEY_TAB, KEY_SPACE, KEY_MINUS,
+ KEY_EQUAL, KEY_LEFTBRACE, KEY_RIGHTBRACE, KEY_BACKSLASH, 0,
+ KEY_SEMICOLON, KEY_APOSTROPHE, KEY_GRAVE, KEY_COMMA, KEY_DOT, KEY_SLASH,
+ KEY_CAPSLOCK,
+ KEY_F1, KEY_F2, KEY_F3, KEY_F4, KEY_F5, KEY_F6, KEY_F7, KEY_F8, KEY_F9,
+ KEY_F10, KEY_F11, KEY_F12, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ KEY_RIGHT, KEY_LEFT, KEY_DOWN, KEY_UP,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_102ND,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RO, 0, KEY_YEN, 0, 0, 0, 0, 0,
+ 0, KEY_KATAKANAHIRAGANA, KEY_MUHENKAN
+};
+
+/*
+ * This must have exactly as many entries as there are bits in
+ * struct keyboard_protocol.modifiers .
+ */
+static const unsigned char applespi_controlcodes[] = {
+ KEY_LEFTCTRL,
+ KEY_LEFTSHIFT,
+ KEY_LEFTALT,
+ KEY_LEFTMETA,
+ 0,
+ KEY_RIGHTSHIFT,
+ KEY_RIGHTALT,
+ KEY_RIGHTMETA
+};
+
+struct applespi_key_translation {
+ u16 from;
+ u16 to;
+ u8 flags;
+};
+
+static const struct applespi_key_translation applespi_fn_codes[] = {
+ { KEY_BACKSPACE, KEY_DELETE },
+ { KEY_ENTER, KEY_INSERT },
+ { KEY_F1, KEY_BRIGHTNESSDOWN, APPLE_FLAG_FKEY },
+ { KEY_F2, KEY_BRIGHTNESSUP, APPLE_FLAG_FKEY },
+ { KEY_F3, KEY_SCALE, APPLE_FLAG_FKEY },
+ { KEY_F4, KEY_DASHBOARD, APPLE_FLAG_FKEY },
+ { KEY_F5, KEY_KBDILLUMDOWN, APPLE_FLAG_FKEY },
+ { KEY_F6, KEY_KBDILLUMUP, APPLE_FLAG_FKEY },
+ { KEY_F7, KEY_PREVIOUSSONG, APPLE_FLAG_FKEY },
+ { KEY_F8, KEY_PLAYPAUSE, APPLE_FLAG_FKEY },
+ { KEY_F9, KEY_NEXTSONG, APPLE_FLAG_FKEY },
+ { KEY_F10, KEY_MUTE, APPLE_FLAG_FKEY },
+ { KEY_F11, KEY_VOLUMEDOWN, APPLE_FLAG_FKEY },
+ { KEY_F12, KEY_VOLUMEUP, APPLE_FLAG_FKEY },
+ { KEY_RIGHT, KEY_END },
+ { KEY_LEFT, KEY_HOME },
+ { KEY_DOWN, KEY_PAGEDOWN },
+ { KEY_UP, KEY_PAGEUP },
+ { }
+};
+
+static const struct applespi_key_translation apple_iso_keyboard[] = {
+ { KEY_GRAVE, KEY_102ND },
+ { KEY_102ND, KEY_GRAVE },
+ { }
+};
+
+struct applespi_tp_model_info {
+ u16 model;
+ struct applespi_tp_info tp_info;
+};
+
+static const struct applespi_tp_model_info applespi_tp_models[] = {
+ {
+ .model = 0x04, /* MB8 MB9 MB10 */
+ .tp_info = { -5087, -182, 5579, 6089 },
+ },
+ {
+ .model = 0x05, /* MBP13,1 MBP13,2 MBP14,1 MBP14,2 */
+ .tp_info = { -6243, -170, 6749, 7685 },
+ },
+ {
+ .model = 0x06, /* MBP13,3 MBP14,3 */
+ .tp_info = { -7456, -163, 7976, 9283 },
+ },
+ {}
+};
+
+static const char *applespi_debug_facility(unsigned int log_mask)
+{
+ switch (log_mask) {
+ case DBG_CMD_TP_INI:
+ return "Touchpad Initialization";
+ case DBG_CMD_BL:
+ return "Backlight Command";
+ case DBG_CMD_CL:
+ return "Caps-Lock Command";
+ case DBG_RD_KEYB:
+ return "Keyboard Event";
+ case DBG_RD_TPAD:
+ return "Touchpad Event";
+ case DBG_RD_UNKN:
+ return "Unknown Event";
+ case DBG_RD_IRQ:
+ return "Interrupt Request";
+ case DBG_RD_CRC:
+ return "Corrupted packet";
+ case DBG_TP_DIM:
+ return "Touchpad Dimensions";
+ default:
+ return "-Unrecognized log mask-";
+ }
+}
+
+static void applespi_setup_read_txfrs(struct applespi_data *applespi)
+{
+ struct spi_message *msg = &applespi->rd_m;
+ struct spi_transfer *dl_t = &applespi->dl_t;
+ struct spi_transfer *rd_t = &applespi->rd_t;
+
+ memset(dl_t, 0, sizeof(*dl_t));
+ memset(rd_t, 0, sizeof(*rd_t));
+
+ dl_t->delay_usecs = applespi->spi_settings.spi_cs_delay;
+
+ rd_t->rx_buf = applespi->rx_buffer;
+ rd_t->len = APPLESPI_PACKET_SIZE;
+
+ spi_message_init(msg);
+ spi_message_add_tail(dl_t, msg);
+ spi_message_add_tail(rd_t, msg);
+}
+
+static void applespi_setup_write_txfrs(struct applespi_data *applespi)
+{
+ struct spi_message *msg = &applespi->wr_m;
+ struct spi_transfer *wt_t = &applespi->ww_t;
+ struct spi_transfer *dl_t = &applespi->wd_t;
+ struct spi_transfer *wr_t = &applespi->wr_t;
+ struct spi_transfer *st_t = &applespi->st_t;
+
+ memset(wt_t, 0, sizeof(*wt_t));
+ memset(dl_t, 0, sizeof(*dl_t));
+ memset(wr_t, 0, sizeof(*wr_t));
+ memset(st_t, 0, sizeof(*st_t));
+
+ /*
+ * All we need here is a delay at the beginning of the message before
+ * asserting cs. But the current spi API doesn't support this, so we
+ * end up with an extra unnecessary (but harmless) cs assertion and
+ * deassertion.
+ */
+ wt_t->delay_usecs = SPI_RW_CHG_DELAY_US;
+ wt_t->cs_change = 1;
+
+ dl_t->delay_usecs = applespi->spi_settings.spi_cs_delay;
+
+ wr_t->tx_buf = applespi->tx_buffer;
+ wr_t->len = APPLESPI_PACKET_SIZE;
+ wr_t->delay_usecs = SPI_RW_CHG_DELAY_US;
+
+ st_t->rx_buf = applespi->tx_status;
+ st_t->len = APPLESPI_STATUS_SIZE;
+
+ spi_message_init(msg);
+ spi_message_add_tail(wt_t, msg);
+ spi_message_add_tail(dl_t, msg);
+ spi_message_add_tail(wr_t, msg);
+ spi_message_add_tail(st_t, msg);
+}
+
+static int applespi_async(struct applespi_data *applespi,
+ struct spi_message *message, void (*complete)(void *))
+{
+ message->complete = complete;
+ message->context = applespi;
+
+ return spi_async(applespi->spi, message);
+}
+
+static inline bool applespi_check_write_status(struct applespi_data *applespi,
+ int sts)
+{
+ static u8 status_ok[] = { 0xac, 0x27, 0x68, 0xd5 };
+
+ if (sts < 0) {
+ dev_warn(&(applespi)->spi->dev, "Error writing to device: %d\n",
+ sts);
+ return false;
+ }
+
+ if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {
+ dev_warn(&(applespi)->spi->dev,
+ "Error writing to device: %*ph\n",
+ APPLESPI_STATUS_SIZE, applespi->tx_status);
+ return false;
+ }
+
+ return true;
+}
+
+static int applespi_get_spi_settings(struct applespi_data *applespi)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&(applespi)->spi->dev);
+ const union acpi_object *o;
+ struct spi_settings *settings = &applespi->spi_settings;
+
+ if (!acpi_dev_get_property(adev, "spiCSDelay", ACPI_TYPE_BUFFER, &o))
+ settings->spi_cs_delay = *(u64 *)o->buffer.pointer;
+ else
+ dev_warn(&(applespi)->spi->dev,
+ "Property spiCSDelay not found\n");
+
+ if (!acpi_dev_get_property(adev, "resetA2RUsec", ACPI_TYPE_BUFFER, &o))
+ settings->reset_a2r_usec = *(u64 *)o->buffer.pointer;
+ else
+ dev_warn(&(applespi)->spi->dev,
+ "Property resetA2RUsec not found\n");
+
+ if (!acpi_dev_get_property(adev, "resetRecUsec", ACPI_TYPE_BUFFER, &o))
+ settings->reset_rec_usec = *(u64 *)o->buffer.pointer;
+ else
+ dev_warn(&(applespi)->spi->dev,
+ "Property resetRecUsec not found\n");
+
+ dev_dbg(&(applespi)->spi->dev,
+ "SPI settings: spi_cs_delay=%llu reset_a2r_usec=%llu reset_rec_usec=%llu\n",
+ settings->spi_cs_delay, settings->reset_a2r_usec,
+ settings->reset_rec_usec);
+
+ return 0;
+}
+
+static int applespi_setup_spi(struct applespi_data *applespi)
+{
+ int sts;
+
+ sts = applespi_get_spi_settings(applespi);
+ if (sts)
+ return sts;
+
+ spin_lock_init(&applespi->cmd_msg_lock);
+ init_waitqueue_head(&applespi->drain_complete);
+
+ return 0;
+}
+
+static int applespi_enable_spi(struct applespi_data *applespi)
+{
+ acpi_status acpi_sts;
+ unsigned long long spi_status;
+
+ /* check if SPI is already enabled, so we can skip the delay below */
+ acpi_sts = acpi_evaluate_integer(applespi->sist, NULL, NULL,
+ &spi_status);
+ if (ACPI_SUCCESS(acpi_sts) && spi_status)
+ return 0;
+
+ /* SIEN(1) will enable SPI communication */
+ acpi_sts = acpi_execute_simple_method(applespi->sien, NULL, 1);
+ if (ACPI_FAILURE(acpi_sts)) {
+ dev_err(&(applespi)->spi->dev, "SIEN failed: %s\n",
+ acpi_format_exception(acpi_sts));
+ return -ENODEV;
+ }
+
+ /*
+ * Allow the SPI interface to come up before returning. Without this
+ * delay, the SPI commands to enable multitouch mode may not reach
+ * the trackpad controller, causing pointer movement to break upon
+ * resume from sleep.
+ */
+ msleep(50);
+
+ return 0;
+}
+
+static int applespi_send_cmd_msg(struct applespi_data *applespi);
+
+static void applespi_msg_complete(struct applespi_data *applespi,
+ bool is_write_msg, bool is_read_compl)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ if (is_read_compl)
+ applespi->read_active = false;
+ if (is_write_msg)
+ applespi->write_active = false;
+
+ if (applespi->drain && !applespi->write_active)
+ wake_up_all(&applespi->drain_complete);
+
+ if (is_write_msg) {
+ applespi->cmd_msg_queued = false;
+ applespi_send_cmd_msg(applespi);
+ }
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+}
+
+static void applespi_async_write_complete(void *context)
+{
+ struct applespi_data *applespi = context;
+
+ debug_print_header(applespi->cmd_log_mask, applespi);
+ debug_print_buffer(applespi->cmd_log_mask, applespi, "write ",
+ applespi->tx_buffer, APPLESPI_PACKET_SIZE);
+ debug_print_buffer(applespi->cmd_log_mask, applespi, "status ",
+ applespi->tx_status, APPLESPI_STATUS_SIZE);
+
+ if (!applespi_check_write_status(applespi, applespi->wr_m.status)) {
+ /*
+ * If we got an error, we presumably won't get the expected
+ * response message either.
+ */
+ applespi_msg_complete(applespi, true, false);
+ }
+}
+
+static int applespi_send_cmd_msg(struct applespi_data *applespi)
+{
+ u16 crc;
+ int sts;
+ struct spi_packet *packet = (struct spi_packet *)applespi->tx_buffer;
+ struct message *message = (struct message *)packet->data;
+ u16 msg_len;
+ u8 device;
+
+ /* check if draining */
+ if (applespi->drain)
+ return 0;
+
+ /* check whether send is in progress */
+ if (applespi->cmd_msg_queued)
+ return 0;
+
+ /* set up packet */
+ memset(packet, 0, APPLESPI_PACKET_SIZE);
+
+ /* are we processing init commands? */
+ if (applespi->want_tp_info_cmd) {
+ applespi->want_tp_info_cmd = false;
+ applespi->want_mt_init_cmd = true;
+ applespi->cmd_log_mask = DBG_CMD_TP_INI;
+
+ /* build init command */
+ device = PACKET_DEV_INFO;
+
+ message->type = cpu_to_le16(0x1020);
+ msg_len = sizeof(message->tp_info_command);
+
+ message->zero = 0x02;
+ message->rsp_buf_len = cpu_to_le16(0x0200);
+
+ } else if (applespi->want_mt_init_cmd) {
+ applespi->want_mt_init_cmd = false;
+ applespi->cmd_log_mask = DBG_CMD_TP_INI;
+
+ /* build init command */
+ device = PACKET_DEV_TPAD;
+
+ message->type = cpu_to_le16(0x0252);
+ msg_len = sizeof(message->init_mt_command);
+
+ message->init_mt_command.cmd = cpu_to_le16(0x0102);
+
+ /* do we need caps-lock command? */
+ } else if (applespi->want_cl_led_on != applespi->have_cl_led_on) {
+ applespi->have_cl_led_on = applespi->want_cl_led_on;
+ applespi->cmd_log_mask = DBG_CMD_CL;
+
+ /* build led command */
+ device = PACKET_DEV_KEYB;
+
+ message->type = cpu_to_le16(0x0151);
+ msg_len = sizeof(message->capsl_command);
+
+ message->capsl_command.unknown = 0x01;
+ message->capsl_command.led = applespi->have_cl_led_on ? 2 : 0;
+
+ /* do we need backlight command? */
+ } else if (applespi->want_bl_level != applespi->have_bl_level) {
+ applespi->have_bl_level = applespi->want_bl_level;
+ applespi->cmd_log_mask = DBG_CMD_BL;
+
+ /* build command buffer */
+ device = PACKET_DEV_KEYB;
+
+ message->type = cpu_to_le16(0xB051);
+ msg_len = sizeof(message->bl_command);
+
+ message->bl_command.const1 = cpu_to_le16(0x01B0);
+ message->bl_command.level =
+ cpu_to_le16(applespi->have_bl_level);
+
+ if (applespi->have_bl_level > 0)
+ message->bl_command.const2 = cpu_to_le16(0x01F4);
+ else
+ message->bl_command.const2 = cpu_to_le16(0x0001);
+
+ /* everything's up-to-date */
+ } else {
+ return 0;
+ }
+
+ /* finalize packet */
+ packet->flags = PACKET_TYPE_WRITE;
+ packet->device = device;
+ packet->length = cpu_to_le16(MSG_HEADER_SIZE + msg_len);
+
+ message->counter = applespi->cmd_msg_cntr++ % (U8_MAX + 1);
+
+ message->length = cpu_to_le16(msg_len - 2);
+ if (!message->rsp_buf_len)
+ message->rsp_buf_len = message->length;
+
+ crc = crc16(0, (u8 *)message, le16_to_cpu(packet->length) - 2);
+ put_unaligned_le16(crc, &message->data[msg_len - 2]);
+
+ crc = crc16(0, (u8 *)packet, sizeof(*packet) - 2);
+ packet->crc16 = cpu_to_le16(crc);
+
+ /* send command */
+ sts = applespi_async(applespi, &applespi->wr_m,
+ applespi_async_write_complete);
+ if (sts) {
+ dev_warn(&(applespi)->spi->dev,
+ "Error queueing async write to device: %d\n", sts);
+ return sts;
+ }
+
+ applespi->cmd_msg_queued = true;
+ applespi->write_active = true;
+
+ return 0;
+}
+
+static void applespi_init(struct applespi_data *applespi, bool is_resume)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ if (is_resume)
+ applespi->want_mt_init_cmd = true;
+ else
+ applespi->want_tp_info_cmd = true;
+ applespi_send_cmd_msg(applespi);
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+}
+
+static int applespi_set_capsl_led(struct applespi_data *applespi,
+ bool capslock_on)
+{
+ unsigned long flags;
+ int sts;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ applespi->want_cl_led_on = capslock_on;
+ sts = applespi_send_cmd_msg(applespi);
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ return sts;
+}
+
+static void applespi_set_bl_level(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct applespi_data *applespi =
+ container_of(led_cdev, struct applespi_data, backlight_info);
+ unsigned long flags;
+ int sts;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ if (value == 0) {
+ applespi->want_bl_level = value;
+ } else {
+ /*
+ * The backlight does not turn on till level 32, so we scale
+ * the range here so that from a user's perspective it turns
+ * on at 1.
+ */
+ applespi->want_bl_level =
+ ((value * KBD_BL_LEVEL_ADJ) / KBD_BL_LEVEL_SCALE +
+ KBD_BL_LEVEL_MIN);
+ }
+
+ sts = applespi_send_cmd_msg(applespi);
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+}
+
+static int applespi_event(struct input_dev *dev, unsigned int type,
+ unsigned int code, int value)
+{
+ struct applespi_data *applespi = input_get_drvdata(dev);
+
+ switch (type) {
+ case EV_LED:
+ applespi_set_capsl_led(applespi, !!test_bit(LED_CAPSL, dev->led));
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+/* lifted from the BCM5974 driver and renamed from raw2int */
+/* convert 16-bit little endian to signed integer */
+static inline int le16_to_int(__le16 x)
+{
+ return (signed short)le16_to_cpu(x);
+}
+
+static int applespi_dbg_dim_min_x;
+static int applespi_dbg_dim_max_x;
+static int applespi_dbg_dim_min_y;
+static int applespi_dbg_dim_max_y;
+static bool applespi_dbg_dim_updated;
+
+static void applespi_debug_update_dimensions(const struct tp_finger *f)
+{
+ #define UPDATE_DIMENSIONS(val, op, last) \
+ do { \
+ if (le16_to_int(val) op last) { \
+ last = le16_to_int(val); \
+ applespi_dbg_dim_updated = true; \
+ } \
+ } while (0)
+
+ UPDATE_DIMENSIONS(f->abs_x, <, applespi_dbg_dim_min_x);
+ UPDATE_DIMENSIONS(f->abs_x, >, applespi_dbg_dim_max_x);
+ UPDATE_DIMENSIONS(f->abs_y, <, applespi_dbg_dim_min_y);
+ UPDATE_DIMENSIONS(f->abs_y, >, applespi_dbg_dim_max_y);
+
+ #undef UPDATE_DIMENSIONS
+}
+
+static void applespi_debug_print_dimensions(struct applespi_data *applespi)
+{
+ static ktime_t last_print;
+
+ if (applespi_dbg_dim_updated &&
+ ktime_ms_delta(ktime_get(), last_print) > 1000) {
+ debug_print(DBG_TP_DIM, applespi,
+ "New touchpad dimensions: %dx%d+%u+%u\n",
+ applespi_dbg_dim_min_x, applespi_dbg_dim_min_y,
+ applespi_dbg_dim_max_x - applespi_dbg_dim_min_x,
+ applespi_dbg_dim_max_y - applespi_dbg_dim_min_y);
+ applespi_dbg_dim_updated = false;
+ last_print = ktime_get();
+ }
+}
+
+static void report_finger_data(struct input_dev *input, int slot,
+ const struct input_mt_pos *pos,
+ const struct tp_finger *f)
+{
+ input_mt_slot(input, slot);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
+
+ input_report_abs(input, ABS_MT_TOUCH_MAJOR,
+ le16_to_int(f->touch_major) << 1);
+ input_report_abs(input, ABS_MT_TOUCH_MINOR,
+ le16_to_int(f->touch_minor) << 1);
+ input_report_abs(input, ABS_MT_WIDTH_MAJOR,
+ le16_to_int(f->tool_major) << 1);
+ input_report_abs(input, ABS_MT_WIDTH_MINOR,
+ le16_to_int(f->tool_minor) << 1);
+ input_report_abs(input, ABS_MT_ORIENTATION,
+ MAX_FINGER_ORIENTATION - le16_to_int(f->orientation));
+ input_report_abs(input, ABS_MT_POSITION_X, pos->x);
+ input_report_abs(input, ABS_MT_POSITION_Y, pos->y);
+}
+
+static void report_tp_state(struct applespi_data *applespi,
+ struct touchpad_protocol *t)
+{
+ const struct tp_finger *f;
+ struct input_dev *input;
+ const struct applespi_tp_info *tp_info = &applespi->tp_info;
+ int i, n;
+
+ /* touchpad_input_dev is set async in worker */
+ input = smp_load_acquire(&applespi->touchpad_input_dev);
+ if (!input)
+ return; /* touchpad isn't initialized yet */
+
+ n = 0;
+
+ for (i = 0; i < t->number_of_fingers; i++) {
+ f = &t->fingers[i];
+ if (le16_to_int(f->touch_major) == 0)
+ continue;
+ applespi->pos[n].x = le16_to_int(f->abs_x);
+ applespi->pos[n].y = tp_info->y_min + tp_info->y_max -
+ le16_to_int(f->abs_y);
+ n++;
+
+ if (debug & DBG_TP_DIM)
+ applespi_debug_update_dimensions(f);
+ }
+
+ if (debug & DBG_TP_DIM)
+ applespi_debug_print_dimensions(applespi);
+
+ input_mt_assign_slots(input, applespi->slots, applespi->pos, n, 0);
+
+ for (i = 0; i < n; i++)
+ report_finger_data(input, applespi->slots[i],
+ &applespi->pos[i], &t->fingers[i]);
+
+ input_mt_sync_frame(input);
+ input_report_key(input, BTN_LEFT, t->clicked);
+
+ input_sync(input);
+}
+
+static const struct applespi_key_translation *
+applespi_find_translation(const struct applespi_key_translation *table, u16 key)
+{
+ const struct applespi_key_translation *trans;
+
+ for (trans = table; trans->from; trans++)
+ if (trans->from == key)
+ return trans;
+
+ return NULL;
+}
+
+static unsigned int applespi_translate_fn_key(unsigned int key, int fn_pressed)
+{
+ const struct applespi_key_translation *trans;
+ int do_translate;
+
+ trans = applespi_find_translation(applespi_fn_codes, key);
+ if (trans) {
+ if (trans->flags & APPLE_FLAG_FKEY)
+ do_translate = (fnmode == 2 && fn_pressed) ||
+ (fnmode == 1 && !fn_pressed);
+ else
+ do_translate = fn_pressed;
+
+ if (do_translate)
+ key = trans->to;
+ }
+
+ return key;
+}
+
+static unsigned int applespi_translate_iso_layout(unsigned int key)
+{
+ const struct applespi_key_translation *trans;
+
+ trans = applespi_find_translation(apple_iso_keyboard, key);
+ if (trans)
+ key = trans->to;
+
+ return key;
+}
+
+static unsigned int applespi_code_to_key(u8 code, int fn_pressed)
+{
+ unsigned int key = applespi_scancodes[code];
+
+ if (fnmode)
+ key = applespi_translate_fn_key(key, fn_pressed);
+ if (iso_layout)
+ key = applespi_translate_iso_layout(key);
+ return key;
+}
+
+static void
+applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
+{
+ unsigned char tmp;
+
+ if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
+ !applespi_controlcodes[fnremap - 1])
+ return;
+
+ tmp = keyboard_protocol->fn_pressed;
+ keyboard_protocol->fn_pressed =
+ !!(keyboard_protocol->modifiers & BIT(fnremap - 1));
+ if (tmp)
+ keyboard_protocol->modifiers |= BIT(fnremap - 1);
+ else
+ keyboard_protocol->modifiers &= ~BIT(fnremap - 1);
+}
+
+static void
+applespi_handle_keyboard_event(struct applespi_data *applespi,
+ struct keyboard_protocol *keyboard_protocol)
+{
+ int i, j;
+ unsigned int key;
+ bool still_pressed;
+ bool is_overflow;
+
+ compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
+ sizeof_field(struct keyboard_protocol, modifiers) * 8,
+ "applespi_controlcodes has wrong number of entries");
+
+ /* check for rollover overflow, which is signalled by all keys == 1 */
+ is_overflow = true;
+
+ for (i = 0; i < MAX_ROLLOVER; i++) {
+ if (keyboard_protocol->keys_pressed[i] != 1) {
+ is_overflow = false;
+ break;
+ }
+ }
+
+ if (is_overflow)
+ return;
+
+ /* remap fn key if desired */
+ applespi_remap_fn_key(keyboard_protocol);
+
+ /* check released keys */
+ for (i = 0; i < MAX_ROLLOVER; i++) {
+ still_pressed = false;
+ for (j = 0; j < MAX_ROLLOVER; j++) {
+ if (applespi->last_keys_pressed[i] ==
+ keyboard_protocol->keys_pressed[j]) {
+ still_pressed = true;
+ break;
+ }
+ }
+
+ if (still_pressed)
+ continue;
+
+ key = applespi_code_to_key(applespi->last_keys_pressed[i],
+ applespi->last_keys_fn_pressed[i]);
+ input_report_key(applespi->keyboard_input_dev, key, 0);
+ applespi->last_keys_fn_pressed[i] = 0;
+ }
+
+ /* check pressed keys */
+ for (i = 0; i < MAX_ROLLOVER; i++) {
+ if (keyboard_protocol->keys_pressed[i] <
+ ARRAY_SIZE(applespi_scancodes) &&
+ keyboard_protocol->keys_pressed[i] > 0) {
+ key = applespi_code_to_key(
+ keyboard_protocol->keys_pressed[i],
+ keyboard_protocol->fn_pressed);
+ input_report_key(applespi->keyboard_input_dev, key, 1);
+ applespi->last_keys_fn_pressed[i] =
+ keyboard_protocol->fn_pressed;
+ }
+ }
+
+ /* check control keys */
+ for (i = 0; i < ARRAY_SIZE(applespi_controlcodes); i++) {
+ if (keyboard_protocol->modifiers & BIT(i))
+ input_report_key(applespi->keyboard_input_dev,
+ applespi_controlcodes[i], 1);
+ else
+ input_report_key(applespi->keyboard_input_dev,
+ applespi_controlcodes[i], 0);
+ }
+
+ /* check function key */
+ if (keyboard_protocol->fn_pressed && !applespi->last_fn_pressed)
+ input_report_key(applespi->keyboard_input_dev, KEY_FN, 1);
+ else if (!keyboard_protocol->fn_pressed && applespi->last_fn_pressed)
+ input_report_key(applespi->keyboard_input_dev, KEY_FN, 0);
+ applespi->last_fn_pressed = keyboard_protocol->fn_pressed;
+
+ /* done */
+ input_sync(applespi->keyboard_input_dev);
+ memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed,
+ sizeof(applespi->last_keys_pressed));
+}
+
+static const struct applespi_tp_info *applespi_find_touchpad_info(__u8 model)
+{
+ const struct applespi_tp_model_info *info;
+
+ for (info = applespi_tp_models; info->model; info++) {
+ if (info->model == model)
+ return &info->tp_info;
+ }
+
+ return NULL;
+}
+
+static int
+applespi_register_touchpad_device(struct applespi_data *applespi,
+ struct touchpad_info_protocol *rcvd_tp_info)
+{
+ const struct applespi_tp_info *tp_info;
+ struct input_dev *touchpad_input_dev;
+ int sts;
+
+ /* set up touchpad dimensions */
+ tp_info = applespi_find_touchpad_info(rcvd_tp_info->model_no);
+ if (!tp_info) {
+ dev_warn(&(applespi)->spi->dev,
+ "Unknown touchpad model %x - falling back to MB8 touchpad\n",
+ rcvd_tp_info->model_no);
+ tp_info = &applespi_tp_models[0].tp_info;
+ }
+
+ applespi->tp_info = *tp_info;
+
+ if (touchpad_dimensions[0]) {
+ int x, y, w, h;
+
+ sts = sscanf(touchpad_dimensions, "%dx%d+%u+%u", &x, &y, &w, &h);
+ if (sts == 4) {
+ dev_info(&(applespi)->spi->dev,
+ "Overriding touchpad dimensions from module param\n");
+ applespi->tp_info.x_min = x;
+ applespi->tp_info.y_min = y;
+ applespi->tp_info.x_max = x + w;
+ applespi->tp_info.y_max = y + h;
+ } else {
+ dev_warn(&(applespi)->spi->dev,
+ "Invalid touchpad dimensions '%s': must be in the form XxY+W+H\n",
+ touchpad_dimensions);
+ touchpad_dimensions[0] = '\0';
+ }
+ }
+ if (!touchpad_dimensions[0]) {
+ snprintf(touchpad_dimensions, sizeof(touchpad_dimensions),
+ "%dx%d+%u+%u",
+ applespi->tp_info.x_min,
+ applespi->tp_info.y_min,
+ applespi->tp_info.x_max - applespi->tp_info.x_min,
+ applespi->tp_info.y_max - applespi->tp_info.y_min);
+ }
+
+ /* create touchpad input device */
+ touchpad_input_dev = devm_input_allocate_device(&(applespi)->spi->dev);
+ if (!touchpad_input_dev) {
+ dev_err(&(applespi)->spi->dev,
+ "Failed to allocate touchpad input device\n");
+ return -ENOMEM;
+ }
+
+ touchpad_input_dev->name = "Apple SPI Touchpad";
+ touchpad_input_dev->phys = "applespi/input1";
+ touchpad_input_dev->dev.parent = &(applespi)->spi->dev;
+ touchpad_input_dev->id.bustype = BUS_SPI;
+ touchpad_input_dev->id.vendor = SYNAPTICS_VENDOR_ID;
+ touchpad_input_dev->id.product =
+ rcvd_tp_info->model_no << 8 | rcvd_tp_info->model_flags;
+
+ /* basic properties */
+ input_set_capability(touchpad_input_dev, EV_REL, REL_X);
+ input_set_capability(touchpad_input_dev, EV_REL, REL_Y);
+
+ __set_bit(INPUT_PROP_POINTER, touchpad_input_dev->propbit);
+ __set_bit(INPUT_PROP_BUTTONPAD, touchpad_input_dev->propbit);
+
+ /* finger touch area */
+ input_set_abs_params(touchpad_input_dev, ABS_MT_TOUCH_MAJOR,
+ 0, 5000, 0, 0);
+ input_set_abs_params(touchpad_input_dev, ABS_MT_TOUCH_MINOR,
+ 0, 5000, 0, 0);
+
+ /* finger approach area */
+ input_set_abs_params(touchpad_input_dev, ABS_MT_WIDTH_MAJOR,
+ 0, 5000, 0, 0);
+ input_set_abs_params(touchpad_input_dev, ABS_MT_WIDTH_MINOR,
+ 0, 5000, 0, 0);
+
+ /* finger orientation */
+ input_set_abs_params(touchpad_input_dev, ABS_MT_ORIENTATION,
+ -MAX_FINGER_ORIENTATION, MAX_FINGER_ORIENTATION,
+ 0, 0);
+
+ /* finger position */
+ input_set_abs_params(touchpad_input_dev, ABS_MT_POSITION_X,
+ applespi->tp_info.x_min, applespi->tp_info.x_max,
+ 0, 0);
+ input_set_abs_params(touchpad_input_dev, ABS_MT_POSITION_Y,
+ applespi->tp_info.y_min, applespi->tp_info.y_max,
+ 0, 0);
+
+ /* touchpad button */
+ input_set_capability(touchpad_input_dev, EV_KEY, BTN_LEFT);
+
+ /* multitouch */
+ input_mt_init_slots(touchpad_input_dev, MAX_FINGERS,
+ INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED |
+ INPUT_MT_TRACK);
+
+ /* register input device */
+ sts = input_register_device(touchpad_input_dev);
+ if (sts) {
+ dev_err(&(applespi)->spi->dev,
+ "Unable to register touchpad input device (%d)\n", sts);
+ return sts;
+ }
+
+ /* touchpad_input_dev is read async in spi callback */
+ smp_store_release(&applespi->touchpad_input_dev, touchpad_input_dev);
+
+ return 0;
+}
+
+static void applespi_worker(struct work_struct *work)
+{
+ struct applespi_data *applespi =
+ container_of(work, struct applespi_data, work);
+
+ applespi_register_touchpad_device(applespi, &applespi->rcvd_tp_info);
+}
+
+static void applespi_handle_cmd_response(struct applespi_data *applespi,
+ struct spi_packet *packet,
+ struct message *message)
+{
+ if (packet->device == PACKET_DEV_INFO &&
+ le16_to_cpu(message->type) == 0x1020) {
+ /*
+ * We're not allowed to sleep here, but registering an input
+ * device can sleep.
+ */
+ applespi->rcvd_tp_info = message->tp_info;
+ schedule_work(&applespi->work);
+ return;
+ }
+
+ if (le16_to_cpu(message->length) != 0x0000) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received unexpected write response: length=%x\n",
+ le16_to_cpu(message->length));
+ return;
+ }
+
+ if (packet->device == PACKET_DEV_TPAD &&
+ le16_to_cpu(message->type) == 0x0252 &&
+ le16_to_cpu(message->rsp_buf_len) == 0x0002)
+ dev_info(&(applespi)->spi->dev, "modeswitch done.\n");
+}
+
+static bool applespi_verify_crc(struct applespi_data *applespi, u8 *buffer,
+ size_t buflen)
+{
+ u16 crc;
+
+ crc = crc16(0, buffer, buflen);
+ if (crc) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received corrupted packet (crc mismatch)\n");
+ debug_print_header(DBG_RD_CRC, applespi);
+ debug_print_buffer(DBG_RD_CRC, applespi, "read ", buffer,
+ buflen);
+
+ return false;
+ }
+
+ return true;
+}
+
+static void applespi_debug_print_read_packet(struct applespi_data *applespi,
+ struct spi_packet *packet)
+{
+ unsigned int dbg_mask;
+
+ if (packet->flags == PACKET_TYPE_READ &&
+ packet->device == PACKET_DEV_KEYB)
+ dbg_mask = DBG_RD_KEYB;
+ else if (packet->flags == PACKET_TYPE_READ &&
+ packet->device == PACKET_DEV_TPAD)
+ dbg_mask = DBG_RD_TPAD;
+ else if (packet->flags == PACKET_TYPE_WRITE)
+ dbg_mask = applespi->cmd_log_mask;
+ else
+ dbg_mask = DBG_RD_UNKN;
+
+ debug_print_header(dbg_mask, applespi);
+ debug_print_buffer(dbg_mask, applespi, "read ", applespi->rx_buffer,
+ APPLESPI_PACKET_SIZE);
+}
+
+static void applespi_got_data(struct applespi_data *applespi)
+{
+ struct spi_packet *packet;
+ struct message *message;
+ unsigned int msg_len;
+ unsigned int off;
+ unsigned int rem;
+ unsigned int len;
+
+ /* process packet header */
+ if (!applespi_verify_crc(applespi, applespi->rx_buffer,
+ APPLESPI_PACKET_SIZE)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ if (applespi->drain) {
+ applespi->read_active = false;
+ applespi->write_active = false;
+
+ wake_up_all(&applespi->drain_complete);
+ }
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ return;
+ }
+
+ packet = (struct spi_packet *)applespi->rx_buffer;
+
+ applespi_debug_print_read_packet(applespi, packet);
+
+ off = le16_to_cpu(packet->offset);
+ rem = le16_to_cpu(packet->remaining);
+ len = le16_to_cpu(packet->length);
+
+ if (len > sizeof(packet->data)) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received corrupted packet (invalid packet length %u)\n",
+ len);
+ goto msg_complete;
+ }
+
+ /* handle multi-packet messages */
+ if (rem > 0 || off > 0) {
+ if (off != applespi->saved_msg_len) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received unexpected offset (got %u, expected %u)\n",
+ off, applespi->saved_msg_len);
+ goto msg_complete;
+ }
+
+ if (off + rem > MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received message too large (size %u)\n",
+ off + rem);
+ goto msg_complete;
+ }
+
+ if (off + len > MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received message too large (size %u)\n",
+ off + len);
+ goto msg_complete;
+ }
+
+ memcpy(applespi->msg_buf + off, &packet->data, len);
+ applespi->saved_msg_len += len;
+
+ if (rem > 0)
+ return;
+
+ message = (struct message *)applespi->msg_buf;
+ msg_len = applespi->saved_msg_len;
+ } else {
+ message = (struct message *)&packet->data;
+ msg_len = len;
+ }
+
+ /* got complete message - verify */
+ if (!applespi_verify_crc(applespi, (u8 *)message, msg_len))
+ goto msg_complete;
+
+ if (le16_to_cpu(message->length) != msg_len - MSG_HEADER_SIZE - 2) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received corrupted packet (invalid message length %u - expected %u)\n",
+ le16_to_cpu(message->length),
+ msg_len - MSG_HEADER_SIZE - 2);
+ goto msg_complete;
+ }
+
+ /* handle message */
+ if (packet->flags == PACKET_TYPE_READ &&
+ packet->device == PACKET_DEV_KEYB) {
+ applespi_handle_keyboard_event(applespi, &message->keyboard);
+
+ } else if (packet->flags == PACKET_TYPE_READ &&
+ packet->device == PACKET_DEV_TPAD) {
+ struct touchpad_protocol *tp;
+ size_t tp_len;
+
+ tp = &message->touchpad;
+ tp_len = sizeof(*tp) +
+ tp->number_of_fingers * sizeof(tp->fingers[0]);
+
+ if (le16_to_cpu(message->length) + 2 != tp_len) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Received corrupted packet (invalid message length %u - num-fingers %u, tp-len %zu)\n",
+ le16_to_cpu(message->length),
+ tp->number_of_fingers, tp_len);
+ goto msg_complete;
+ }
+
+ if (tp->number_of_fingers > MAX_FINGERS) {
+ dev_warn_ratelimited(&(applespi)->spi->dev,
+ "Number of reported fingers (%u) exceeds max (%u))\n",
+ tp->number_of_fingers,
+ MAX_FINGERS);
+ tp->number_of_fingers = MAX_FINGERS;
+ }
+
+ report_tp_state(applespi, tp);
+
+ } else if (packet->flags == PACKET_TYPE_WRITE) {
+ applespi_handle_cmd_response(applespi, packet, message);
+ }
+
+msg_complete:
+ applespi->saved_msg_len = 0;
+
+ applespi_msg_complete(applespi, packet->flags == PACKET_TYPE_WRITE,
+ true);
+}
+
+static void applespi_async_read_complete(void *context)
+{
+ struct applespi_data *applespi = context;
+
+ if (applespi->rd_m.status < 0) {
+ dev_warn(&(applespi)->spi->dev,
+ "Error reading from device: %d\n",
+ applespi->rd_m.status);
+ /*
+ * We don't actually know if this was a pure read, or a response
+ * to a write. But this is a rare error condition that should
+ * never occur, so clearing both flags to avoid deadlock.
+ */
+ applespi_msg_complete(applespi, true, true);
+ } else {
+ applespi_got_data(applespi);
+ }
+
+ acpi_finish_gpe(NULL, applespi->gpe);
+}
+
+static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context)
+{
+ struct applespi_data *applespi = context;
+ int sts;
+ unsigned long flags;
+
+ debug_print_header(DBG_RD_IRQ, applespi);
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ if (!applespi->suspended) {
+ sts = applespi_async(applespi, &applespi->rd_m,
+ applespi_async_read_complete);
+ if (sts)
+ dev_warn(&(applespi)->spi->dev,
+ "Error queueing async read to device: %d\n",
+ sts);
+ else
+ applespi->read_active = true;
+ }
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ return ACPI_INTERRUPT_HANDLED;
+}
+
+static int applespi_get_saved_bl_level(struct applespi_data *applespi)
+{
+ struct efivar_entry *efivar_entry;
+ u16 efi_data = 0;
+ unsigned long efi_data_len;
+ int sts;
+
+ efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
+ if (!efivar_entry)
+ return -ENOMEM;
+
+ memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
+ sizeof(EFI_BL_LEVEL_NAME));
+ efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID;
+ efi_data_len = sizeof(efi_data);
+
+ sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
+ if (sts && sts != -ENOENT)
+ dev_warn(&(applespi)->spi->dev,
+ "Error getting backlight level from EFI vars: %d\n",
+ sts);
+
+ kfree(efivar_entry);
+
+ return sts ? sts : efi_data;
+}
+
+static void applespi_save_bl_level(struct applespi_data *applespi,
+ unsigned int level)
+{
+ efi_guid_t efi_guid;
+ u32 efi_attr;
+ unsigned long efi_data_len;
+ u16 efi_data;
+ int sts;
+
+ /* Save keyboard backlight level */
+ efi_guid = EFI_BL_LEVEL_GUID;
+ efi_data = (u16)level;
+ efi_data_len = sizeof(efi_data);
+ efi_attr = EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS;
+
+ sts = efivar_entry_set_safe(EFI_BL_LEVEL_NAME, efi_guid, efi_attr, true,
+ efi_data_len, &efi_data);
+ if (sts)
+ dev_warn(&(applespi)->spi->dev,
+ "Error saving backlight level to EFI vars: %d\n", sts);
+}
+
+static int applespi_probe(struct spi_device *spi)
+{
+ struct applespi_data *applespi;
+ acpi_status acpi_sts;
+ int sts, i;
+ unsigned long long gpe, usb_status;
+
+ /* check if the USB interface is present and enabled already */
+ acpi_sts = acpi_evaluate_integer(ACPI_HANDLE(&spi->dev), "UIST", NULL,
+ &usb_status);
+ if (ACPI_SUCCESS(acpi_sts) && usb_status) {
+ /* let the USB driver take over instead */
+ dev_info(&spi->dev, "USB interface already enabled\n");
+ return -ENODEV;
+ }
+
+ /* allocate driver data */
+ applespi = devm_kzalloc(&spi->dev, sizeof(*applespi), GFP_KERNEL);
+ if (!applespi)
+ return -ENOMEM;
+
+ applespi->spi = spi;
+ applespi->handle = ACPI_HANDLE(&spi->dev);
+
+ INIT_WORK(&applespi->work, applespi_worker);
+
+ /* store the driver data */
+ spi_set_drvdata(spi, applespi);
+
+ /* create our buffers */
+ applespi->tx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE,
+ GFP_KERNEL);
+ applespi->tx_status = devm_kmalloc(&spi->dev, APPLESPI_STATUS_SIZE,
+ GFP_KERNEL);
+ applespi->rx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE,
+ GFP_KERNEL);
+ applespi->msg_buf = devm_kmalloc_array(&spi->dev, MAX_PKTS_PER_MSG,
+ APPLESPI_PACKET_SIZE,
+ GFP_KERNEL);
+
+ if (!applespi->tx_buffer || !applespi->tx_status ||
+ !applespi->rx_buffer || !applespi->msg_buf)
+ return -ENOMEM;
+
+ /* set up our spi messages */
+ applespi_setup_read_txfrs(applespi);
+ applespi_setup_write_txfrs(applespi);
+
+ /* cache ACPI method handles */
+ if (ACPI_FAILURE(acpi_get_handle(applespi->handle, "SIEN",
+ &applespi->sien)) ||
+ ACPI_FAILURE(acpi_get_handle(applespi->handle, "SIST",
+ &applespi->sist))) {
+ dev_err(&(applespi)->spi->dev,
+ "Failed to get required ACPI method handles\n");
+ return -ENODEV;
+ }
+
+ /* switch on the SPI interface */
+ sts = applespi_setup_spi(applespi);
+ if (sts)
+ return sts;
+
+ sts = applespi_enable_spi(applespi);
+ if (sts)
+ return sts;
+
+ /* setup the keyboard input dev */
+ applespi->keyboard_input_dev = devm_input_allocate_device(&spi->dev);
+
+ if (!applespi->keyboard_input_dev)
+ return -ENOMEM;
+
+ applespi->keyboard_input_dev->name = "Apple SPI Keyboard";
+ applespi->keyboard_input_dev->phys = "applespi/input0";
+ applespi->keyboard_input_dev->dev.parent = &spi->dev;
+ applespi->keyboard_input_dev->id.bustype = BUS_SPI;
+
+ applespi->keyboard_input_dev->evbit[0] =
+ BIT_MASK(EV_KEY) | BIT_MASK(EV_LED) | BIT_MASK(EV_REP);
+ applespi->keyboard_input_dev->ledbit[0] = BIT_MASK(LED_CAPSL);
+
+ input_set_drvdata(applespi->keyboard_input_dev, applespi);
+ applespi->keyboard_input_dev->event = applespi_event;
+
+ for (i = 0; i < ARRAY_SIZE(applespi_scancodes); i++)
+ if (applespi_scancodes[i])
+ input_set_capability(applespi->keyboard_input_dev,
+ EV_KEY, applespi_scancodes[i]);
+
+ for (i = 0; i < ARRAY_SIZE(applespi_controlcodes); i++)
+ if (applespi_controlcodes[i])
+ input_set_capability(applespi->keyboard_input_dev,
+ EV_KEY, applespi_controlcodes[i]);
+
+ for (i = 0; i < ARRAY_SIZE(applespi_fn_codes); i++)
+ if (applespi_fn_codes[i].to)
+ input_set_capability(applespi->keyboard_input_dev,
+ EV_KEY, applespi_fn_codes[i].to);
+
+ input_set_capability(applespi->keyboard_input_dev, EV_KEY, KEY_FN);
+
+ sts = input_register_device(applespi->keyboard_input_dev);
+ if (sts) {
+ dev_err(&(applespi)->spi->dev,
+ "Unable to register keyboard input device (%d)\n", sts);
+ return -ENODEV;
+ }
+
+ /*
+ * The applespi device doesn't send interrupts normally (as is described
+ * in its DSDT), but rather seems to use ACPI GPEs.
+ */
+ acpi_sts = acpi_evaluate_integer(applespi->handle, "_GPE", NULL, &gpe);
+ if (ACPI_FAILURE(acpi_sts)) {
+ dev_err(&(applespi)->spi->dev,
+ "Failed to obtain GPE for SPI slave device: %s\n",
+ acpi_format_exception(acpi_sts));
+ return -ENODEV;
+ }
+ applespi->gpe = (int)gpe;
+
+ acpi_sts = acpi_install_gpe_handler(NULL, applespi->gpe,
+ ACPI_GPE_LEVEL_TRIGGERED,
+ applespi_notify, applespi);
+ if (ACPI_FAILURE(acpi_sts)) {
+ dev_err(&(applespi)->spi->dev,
+ "Failed to install GPE handler for GPE %d: %s\n",
+ applespi->gpe, acpi_format_exception(acpi_sts));
+ return -ENODEV;
+ }
+
+ applespi->suspended = false;
+
+ acpi_sts = acpi_enable_gpe(NULL, applespi->gpe);
+ if (ACPI_FAILURE(acpi_sts)) {
+ dev_err(&(applespi)->spi->dev,
+ "Failed to enable GPE handler for GPE %d: %s\n",
+ applespi->gpe, acpi_format_exception(acpi_sts));
+ acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
+ return -ENODEV;
+ }
+
+ /* trigger touchpad setup */
+ applespi_init(applespi, false);
+
+ /*
+ * By default this device is not enabled for wakeup; but USB keyboards
+ * generally are, so the expectation is that by default the keyboard
+ * will wake the system.
+ */
+ device_wakeup_enable(&spi->dev);
+
+ /* set up keyboard-backlight */
+ sts = applespi_get_saved_bl_level(applespi);
+ if (sts >= 0)
+ applespi_set_bl_level(&applespi->backlight_info, sts);
+
+ applespi->backlight_info.name = "spi::kbd_backlight";
+ applespi->backlight_info.default_trigger = "kbd-backlight";
+ applespi->backlight_info.brightness_set = applespi_set_bl_level;
+
+ sts = devm_led_classdev_register(&spi->dev, &applespi->backlight_info);
+ if (sts)
+ dev_warn(&(applespi)->spi->dev,
+ "Unable to register keyboard backlight class dev (%d)\n",
+ sts);
+
+ return 0;
+}
+
+static void applespi_drain_writes(struct applespi_data *applespi)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ applespi->drain = true;
+ wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
+ applespi->cmd_msg_lock);
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+}
+
+static void applespi_drain_reads(struct applespi_data *applespi)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
+ applespi->cmd_msg_lock);
+
+ applespi->suspended = true;
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+}
+
+static int applespi_remove(struct spi_device *spi)
+{
+ struct applespi_data *applespi = spi_get_drvdata(spi);
+
+ applespi_drain_writes(applespi);
+
+ acpi_disable_gpe(NULL, applespi->gpe);
+ acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
+ device_wakeup_disable(&spi->dev);
+
+ applespi_drain_reads(applespi);
+
+ return 0;
+}
+
+static void applespi_shutdown(struct spi_device *spi)
+{
+ struct applespi_data *applespi = spi_get_drvdata(spi);
+
+ applespi_save_bl_level(applespi, applespi->have_bl_level);
+}
+
+static int applespi_poweroff_late(struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct applespi_data *applespi = spi_get_drvdata(spi);
+
+ applespi_save_bl_level(applespi, applespi->have_bl_level);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int applespi_suspend(struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct applespi_data *applespi = spi_get_drvdata(spi);
+ acpi_status acpi_sts;
+ int sts;
+
+ /* turn off caps-lock - it'll stay on otherwise */
+ sts = applespi_set_capsl_led(applespi, false);
+ if (sts)
+ dev_warn(&(applespi)->spi->dev,
+ "Failed to turn off caps-lock led (%d)\n", sts);
+
+ applespi_drain_writes(applespi);
+
+ /* disable the interrupt */
+ acpi_sts = acpi_disable_gpe(NULL, applespi->gpe);
+ if (ACPI_FAILURE(acpi_sts))
+ dev_err(&(applespi)->spi->dev,
+ "Failed to disable GPE handler for GPE %d: %s\n",
+ applespi->gpe, acpi_format_exception(acpi_sts));
+
+ applespi_drain_reads(applespi);
+
+ return 0;
+}
+
+static int applespi_resume(struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct applespi_data *applespi = spi_get_drvdata(spi);
+ acpi_status acpi_sts;
+ unsigned long flags;
+
+ /* ensure our flags and state reflect a newly resumed device */
+ spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+ applespi->drain = false;
+ applespi->have_cl_led_on = false;
+ applespi->have_bl_level = 0;
+ applespi->cmd_msg_queued = false;
+ applespi->read_active = false;
+ applespi->write_active = false;
+
+ applespi->suspended = false;
+
+ spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+ /* switch on the SPI interface */
+ applespi_enable_spi(applespi);
+
+ /* re-enable the interrupt */
+ acpi_sts = acpi_enable_gpe(NULL, applespi->gpe);
+ if (ACPI_FAILURE(acpi_sts))
+ dev_err(&(applespi)->spi->dev,
+ "Failed to re-enable GPE handler for GPE %d: %s\n",
+ applespi->gpe, acpi_format_exception(acpi_sts));
+
+ /* switch the touchpad into multitouch mode */
+ applespi_init(applespi, true);
+
+ return 0;
+}
+#endif
+
+static const struct acpi_device_id applespi_acpi_match[] = {
+ { "APP000D", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, applespi_acpi_match);
+
+const struct dev_pm_ops applespi_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(applespi_suspend, applespi_resume)
+ .poweroff_late = applespi_poweroff_late,
+};
+
+static struct spi_driver applespi_driver = {
+ .driver = {
+ .name = "applespi",
+ .acpi_match_table = applespi_acpi_match,
+ .pm = &applespi_pm_ops,
+ },
+ .probe = applespi_probe,
+ .remove = applespi_remove,
+ .shutdown = applespi_shutdown,
+};
+
+module_spi_driver(applespi_driver)
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MacBook(Pro) SPI Keyboard/Touchpad driver");
+MODULE_AUTHOR("Federico Lorenzi");
+MODULE_AUTHOR("Ronald Tschalär");
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] HID: intel-ish-hid: ISH firmware loader client driver
From: Srinivas Pandruvada @ 2019-03-27 2:22 UTC (permalink / raw)
To: Nick Crews, Rushikesh S Kadam
Cc: benjamin.tissoires, jikos, jettrink, gwendal, linux-kernel,
linux-input
In-Reply-To: <CAHX4x87iQgjX8wHOYNCD7nYQJZKoirgCimPax11bMX_DGMEudw@mail.gmail.com>
On Tue, 2019-03-26 at 18:39 -0600, Nick Crews wrote:
> Hi Rushikesh, I know I've been reviewing this on Chromium, but I have
> some more larges-scale design thoughts.
Hi Nick.
Does this fundamentally change, the way it is done here or can wait for
subsequent revisions later?
Thanks,
Srinivas
>
> > > diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > > b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > > new file mode 100644
> > > index 0000000..85d71d3
> > > --- /dev/null
> > > +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > > @@ -0,0 +1,1103 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * ISH-TP client driver for ISH firmware loading
> > > + *
> > > + * Copyright (c) 2018, Intel Corporation.
> >
> > Year 2019.
> >
> > > + */
> > > +
> > > +#include <linux/firmware.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/intel-ish-client-if.h>
> > > +#include <linux/property.h>
> > > +#include <asm/cacheflush.h>
> > > +
> > > +/* ISH TX/RX ring buffer pool size */
> > > +#define LOADER_CL_RX_RING_SIZE 1
> > > +#define LOADER_CL_TX_RING_SIZE 1
> > > +
> > > +/*
> > > + * ISH Shim firmware loader reserves 4 Kb buffer in SRAM. The
> > > buffer
> > > is
> > > + * used to temporarily hold the data transferred from host to
> > > Shim
> > > firmware
> > > + * loader. Reason for the odd size of 3968 bytes? Each IPC
> > > transfer
> > > is 128
> > > + * bytes (= 4 bytes header + 124 bytes payload). So the 4 Kb
> > > buffer
> > > can
> > > + * hold maximum of 32 IPC transfers, which means we can have a
> > > max
> > > payload
> > > + * of 3968 bytes (= 32 x 124 payload).
> > > + */
> > > +#define LOADER_SHIM_IPC_BUF_SIZE 3968
> > > +
> > > +/**
> > > + * enum ish_loader_commands - ISH loader host commands.
> > > + * LOADER_CMD_XFER_QUERY Query the Shim firmware loader for
> > > capabilities
> > > + * LOADER_CMD_XFER_FRAGMENT Transfer one firmware image
> > > framgment
> > > at a
> > > + * time. The command may be executed
> > > multiple
> > > + * times until the entire firmware
> > > image
> > > is
> > > + * downloaded to SRAM.
> > > + * LOADER_CMD_START Start executing the main firmware.
> > > + */
> > > +enum ish_loader_commands {
> > > + LOADER_CMD_XFER_QUERY = 0,
> > > + LOADER_CMD_XFER_FRAGMENT,
> > > + LOADER_CMD_START,
> > > +};
> > > +
> > > +/* Command bit mask */
> > > +#define CMD_MASK GENMASK(6,
> > > 0)
> > > +#define IS_RESPONSE BIT(7)
> > > +
> > > +/*
> > > + * ISH firmware max delay for one transmit failure is 1 Hz,
> > > + * and firmware will retry 2 times, so 3 Hz is used for timeout.
> > > + */
> > > +#define ISHTP_SEND_TIMEOUT (3 * HZ)
> > > +
> > > +/*
> > > + * Loader transfer modes:
> > > + *
> > > + * LOADER_XFER_MODE_ISHTP mode uses the existing ISH-TP
> > > mechanims to
> > > + * transfer data. This may use IPC or DMA if supported in
> > > firmware.
> > > + * The buffer size is limited to 4 Kb by the IPC/ISH-TP protocol
> > > for
> > > + * both IPC & DMA (legacy).
> > > + *
> > > + * LOADER_XFER_MODE_DIRECT_DMA - firmware loading is a bit
> > > different
> > > + * from the sensor data streaming. Here we download a large
> > > (300+
> > > Kb)
> > > + * image directly to ISH SRAM memory. There is limited benefit
> > > of
> > > + * DMA'ing 300 Kb image in 4 Kb chucks limit. Hence, we
> > > introduce
> > > + * this "direct dma" mode, where we do not use ISH-TP for DMA,
> > > but
> > > + * instead manage the DMA directly in kernel driver and Shim
> > > firmware
> > > + * loader (allocate buf, break in chucks and transfer). This
> > > allows
> > > + * to overcome 4 Kb limit, and optimize the data flow path in
> > > firmware.
> > > + */
> > > +#define LOADER_XFER_MODE_DIRECT_DMA BIT(0)
> > > +#define LOADER_XFER_MODE_ISHTP BIT(1)
> > > +
> > > +/* ISH Transport Loader client unique GUID */
> > > +static const guid_t loader_ishtp_guid =
> > > + GUID_INIT(0xc804d06a, 0x55bd, 0x4ea7,
> > > + 0xad, 0xed, 0x1e, 0x31, 0x22, 0x8c, 0x76, 0xdc);
> > > +
> > > +#define FILENAME_SIZE 256
> > > +
> > > +/*
> > > + * The firmware loading latency will be minimum if we can DMA
> > > the
> > > + * entire ISH firmware image in one go. This requires that we
> > > allocate
> > > + * a large DMA buffer in kernel, which could be problematic on
> > > some
> > > + * platforms. So here we limit the DMA buf size via a
> > > module_param.
> > > + * We default to 4 pages, but a customer can set it to higher
> > > limit
> > > if
> > > + * deemed appropriate for his platform.
> > > + */
> > > +static int dma_buf_size_limit = 4 * PAGE_SIZE;
> > > +
> > > +/**
> > > + * struct loader_msg_hdr - Header for ISH Loader commands.
> > > + * @command: LOADER_CMD* commands. Bit 7 is the
> > > response.
> > > + * @status: Command response status. Non 0, is error
> > > condition.
> > > + *
> > > + * This structure is used as header for every command/data
> > > sent/received
> > > + * between Host driver and ISH Shim firmware loader.
> > > + */
> > > +struct loader_msg_hdr {
> > > + u8 command;
> > > + u8 reserved[2];
> > > + u8 status;
> > > +} __packed;
> > > +
> > > +struct loader_xfer_query {
> > > + struct loader_msg_hdr hdr;
> > > + u32 image_size;
> > > +} __packed;
> > > +
> > > +struct ish_fw_version {
> > > + u16 major;
> > > + u16 minor;
> > > + u16 hotfix;
> > > + u16 build;
> > > +} __packed;
> > > +
> > > +union loader_version {
> > > + u32 value;
> > > + struct {
> > > + u8 major;
> > > + u8 minor;
> > > + u8 hotfix;
> > > + u8 build;
> > > + };
> > > +} __packed;
> > > +
> > > +struct loader_capability {
> > > + u32 max_fw_image_size;
> > > + u32 xfer_mode;
> > > + u32 max_dma_buf_size; /* only for dma mode, multiples of
> > > cacheline */
> > > +} __packed;
> > > +
> > > +struct shim_fw_info {
> > > + struct ish_fw_version ish_fw_version;
> > > + u32 protocol_version;
> > > + union loader_version ldr_version;
> > > + struct loader_capability ldr_capability;
> > > +} __packed;
> > > +
> > > +struct loader_xfer_query_response {
> > > + struct loader_msg_hdr hdr;
> > > + struct shim_fw_info fw_info;
> > > +} __packed;
> > > +
> > > +struct loader_xfer_fragment {
> > > + struct loader_msg_hdr hdr;
> > > + u32 xfer_mode;
> > > + u32 offset;
> > > + u32 size;
> > > + u32 is_last;
> > > +} __packed;
> > > +
> > > +struct loader_xfer_ipc_fragment {
> > > + struct loader_xfer_fragment fragment;
> > > + u8 data[] ____cacheline_aligned; /* variable length payload
> > > here */
> > > +} __packed;
> > > +
> > > +struct loader_xfer_dma_fragment {
> > > + struct loader_xfer_fragment fragment;
> > > + u64 ddr_phys_addr;
> > > +} __packed;
> > > +
> > > +struct loader_start {
> > > + struct loader_msg_hdr hdr;
> > > +} __packed;
> > > +
> > > +/**
> > > + * struct ishtp_cl_data - Encapsulate per ISH-TP Client Data
> > > + * @flag_response Set true on receiving a firmware response
> > > to
> > > host
> > > + * loader command
> > > + * @cmd_resp_wait: Wait queue for Host firmware loading, where
> > > the
> > > + * client sends message to ISH firmware and
> > > wait
> > > for
> > > + * response
> > > + * @work_ishtp_reset: Work queue for reset handling
> > > + * @work_fw_load: Work queue for host firmware loading
> > > + * @flag_retry Flag for indicating host firmware
> > > loading should be
> > > + * retried
> > > + * @bad_recv_cnt: Running count of packets received with
> > > error
> > > + *
> > > + * This structure is used to store data per client
> > > + */
> > > +struct ishtp_cl_data {
> > > + struct ishtp_cl *loader_ishtp_cl;
> > > + struct ishtp_cl_device *cl_device;
> > > +
> > > + /* Completion flags */
> > > + bool flag_response;
> > > +
> > > + /* Copy buffer received in firmware "response" here */
> > > + void *response_data;
> > > + size_t response_size;
> > > +
> > > + /* Wait queue for ISH firmware message event */
> > > + wait_queue_head_t cmd_resp_wait;
> > > +
> > > + struct work_struct work_ishtp_reset;
> > > + struct work_struct work_fw_load;
> > > +
> > > + /*
> > > + * In certain failure scenrios, it makes sense to reset the
> > > + * the ISH subsystem and retry Host firmware loading
> > > + * (e.g. bad message packet, ENOMEM, etc.)
> > > + * On the other hand, failures due to protocol mismatch,
> > > etc
> > > + * are not recoverable. We do not retry.
> > > + *
> > > + * If set, the flag indictes that we should re-try the
> > > particular
> > > + * failure.
> > > + */
> > > + bool flag_retry;
> > > +
> > > + /* Statistics */
> > > + unsigned int bad_recv_cnt;
> > > +};
> > > +
> > > +#define IPC_FRAGMENT_DATA_PREAMBLE \
> > > + offsetof(struct loader_xfer_ipc_fragment, data)
> > > +
> > > +#define cl_data_to_dev(client_data) ishtp_device((client_data)-
> > > > cl_device)
> > >
> > > +
> > > +/**
> > > + * get_firmware_variant() - Gets the filename of firmware image
> > > to
> > > be
> > > + * loaded based on platform variant.
> > > + * @client_data Client data instance.
> > > + * @filename Returns firmware filename.
> > > + *
> > > + * Queries the firmware-name device property string.
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int get_firmware_variant(struct ishtp_cl_data
> > > *client_data,
> > > + char *filename)
> > > +{
> > > + int rv;
> > > + const char *val;
> > > + struct device *devc = ishtp_get_pci_device(client_data-
> > > > cl_device);
> > >
> > > +
> > > + rv = device_property_read_string(devc, "firmware-name",
> > > &val);
> > > + if (rv < 0) {
> > > + dev_err(devc,
> > > + "Error: ISH firmware-name device property
> > > required\n");
> > > + return rv;
> > > + }
> > > + return snprintf(filename, FILENAME_SIZE, "intel/%s", val);
> > > +}
> > > +
> > > +/**
> > > + * report_bad_packets() Report bad packets
> > > + * @loader_ishtp_cl: Client instance to get stats
> > > + * @recv_buf: Raw received host interface message
> > > + *
> > > + * Dumps error in case bad packet is received
> > > + */
> > > +static void report_bad_packet(struct ishtp_cl *loader_ishtp_cl,
> > > + void *recv_buf)
> > > +{
> > > + struct loader_msg_hdr *hdr = recv_buf;
> > > + struct ishtp_cl_data *client_data =
> > > + ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + client_data->bad_recv_cnt++;
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "BAD packet: command=%02lx is_response=%u
> > > status=%02x
> > > total_bad=%u\n",
> > > + hdr->command & CMD_MASK,
> > > + hdr->command & IS_RESPONSE ? 1 : 0,
> > > + hdr->status,
> > > + client_data->bad_recv_cnt);
> > > +}
>
> I would remove this function. Whenever you call it, you already have
> use dev_err() to print the reason for the error. Consider removing
> bad_rcv_count too unless you do something with it other than
> debugging.
>
> At the very least, you call this function when the ISH doesn't return
> enough
> data, but in here you try to access the fields in hdr. This could be
> accessing
> irrelevant/illegal data.
>
> Also a nit: The docstring function name has a naughty trailing s.
>
> > > +
> > > +/**
> > > + * loader_ish_hw_reset() - Reset ISH HW in bad state
> > > + * @loader_ishtp_cl Client instance to reset
> > > + *
> > > + * This function resets ISH hardware, which shall reload
> > > + * the Shim firmware loader, initiate ISH-TP interface reset,
> > > + * re-attach kernel loader driver, and repeat Host
> > > + * firmware load.
> > > + */
> > > +static inline void loader_ish_hw_reset(struct ishtp_cl
> > > *loader_ishtp_cl)
> > > +{
> > > + struct ishtp_cl_data *client_data =
> > > + ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + dev_warn(cl_data_to_dev(client_data), "Reset the ISH
> > > subsystem\n");
> > > + ish_hw_reset(ishtp_get_ishtp_device(loader_ishtp_cl));
> > > +}
>
> Delete this as a function. Before you actually called it in multiple
> places,
> but now i's only called in one place, so just inline it there.
>
> > > +
> > > +/**
> > > + * loader_cl_send() Send message from host to firmware
> > > + * @client_data: Client data instance
> > > + * @msg Message buffer to send
> > > + * @msg_size Size of message
> > > + *
> > > + * Return: Received buffer size on success, negative error code
> > > on
> > > failure.
> > > + */
> > > +static int loader_cl_send(struct ishtp_cl_data *client_data,
> > > + u8 *msg, size_t msg_size)
> > > +{
> > > + int rv;
> > > + size_t data_len;
> > > + struct loader_msg_hdr *in_hdr;
> > > + struct loader_msg_hdr *out_hdr = (struct loader_msg_hdr
> > > *)msg;
> > > + struct ishtp_cl *loader_ishtp_cl = client_data-
> > > > loader_ishtp_cl;
> > >
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "%s: command=%02lx is_response=%u status=%02x\n",
> > > + __func__,
> > > + out_hdr->command & CMD_MASK,
> > > + out_hdr->command & IS_RESPONSE ? 1 : 0,
> > > + out_hdr->status);
> > > +
> > > + client_data->flag_response = false;
> > > + rv = ishtp_cl_send(loader_ishtp_cl, msg, msg_size);
> > > + if (rv < 0) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "ishtp_cl_send error %d\n", rv);
> > > + return rv;
> > > + }
> > > +
> > > + wait_event_interruptible_timeout(client_data-
> > > >cmd_resp_wait,
> > > + client_data-
> > > >flag_response,
> > > + ISHTP_SEND_TIMEOUT);
> > > + if (!client_data->flag_response) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Timed out for response to command=%02lx",
> > > + out_hdr->command & CMD_MASK);
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + /* All response messages will contain a header */
> > > + data_len = client_data->response_size;
> > > + in_hdr = (struct loader_msg_hdr *)client_data-
> > > >response_data;
>
> If process_recv() fails then client_data->response_data could be
> NULL.
> This brings up the question of what to do if process_recv() fails. I
> would think
> that you would want it to set something like client_data-
> >response_error
> and then you could check for that in here and return it. For instance
> right now if the ISH
> doesn't return sizeof(struct loader_msg_hdr) bytes then it would be
> nice to get
> -EMSGSIZE returned from here.
>
> > > +
> > > + /* Sanity checks */
> > > + if (!(in_hdr->command & IS_RESPONSE)) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Invalid response to command\n");
> > > + return -EIO;
> > > + }
> > > +
> > > + if (in_hdr->status) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Loader returned status %d\n",
> > > + in_hdr->status);
> > > + return -EIO;
> > > + }
> > > +
> > > + return data_len;
> > > +}
>
> So I think how you've changed this to handle where the data is stored
> is good,
> but it could be better. I don't like how the users of
> loader_cl_send() need to
> remember to kfree(client_data->response data), and that they just
> implicitly
> assume that client_data->response data holds the result. Instead,
> make the
> callers of loader_cl_send() allocate a buffer to hold the result, and
> then the
> allocating and freeing happens in the same function. I think this is
> a much more
> understandable form of memory management.
>
> How about this function turns into:
> /**
> * loader_cl_send() Send message from host to firmware
> * @client_data: Client data instance
> * @in_data: Message buffer to send
> * @in_size: Size of sent data
> * @out_data: Buffer to fill with received data.
> * @out_size: Max number of bytes to place in out_data.
> *
> * Return: Number of bytes placed into out_data, negative error code
> on failure.
> */
> static int loader_cl_send(struct ishtp_cl_data *client_data,
> u8 *in_data, size_t in_size,
> u8 *out_data, size_t
> out_size)
>
> {
> client_data->response_data = out_data;
> client_data->response_size_max = out_size;
>
> Send the command.
> Tweak process_recv() so where it does the memcpy() into
> client_data->response_data,
> add the additional check to make sure it doesn't copy more than
> client_data->response_size_max bytes.
> Wait for the completion flag.
> Continue with the rest.
> }
>
> With these suggestions there are now six pieces of info getting
> transmitted between
> process_recv() and loader_cl_send() via client data:
> client_data->cmd_resp_wait
> client_data->flag_response
> client_data->response_error
> client_data->response_size
> client_data->response_size_max
> client_data->response_data
> Consider turning these into:
> client_data->response_info->wait_queue
> client_data->response_info->data_available // or some better name?
> client_data->response_info->error
> client_data->response_info->size
> client_data->response_info->size_max
> client_data->response_info->data
> for some encapsulation?
>
> I'm thinking about this more, and basically it seems like we're
> writing a library function to
> send a command to the ISH and receive a response. All the clients who
> use loader_cl_send()
> shouldn't know about the client_data->response_info stuff at all. It
> almost seems like this
> entire functionality should be part of the ISH core? It's really
> limiting that ishtp_cl_send() only
> allows sending and no receiving! It should?!
>
> > > +
> > > +/**
> > > + * process_recv() - Receive and parse incoming packet
> > > + * @loader_ishtp_cl: Client instance to get stats
> > > + * @rb_in_proc: ISH received message buffer
> > > + *
> > > + * Parse the incoming packet. If it is a response packet then it
> > > will
> > > + * update flag_response and wake up the caller waiting to for
> > > the
> > > response.
> > > + */
> > > +static void process_recv(struct ishtp_cl *loader_ishtp_cl,
> > > + struct ishtp_cl_rb *rb_in_proc)
> > > +{
> > > + size_t data_len = rb_in_proc->buf_idx;
> > > + struct loader_msg_hdr *hdr =
> > > + (struct loader_msg_hdr *)rb_in_proc->buffer.data;
> > > + struct ishtp_cl_data *client_data =
> > > + ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + /*
> > > + * All firmware messages have a header. Check buffer size
> > > + * before accessing elements inside.
> > > + */
> > > + if (data_len < sizeof(struct loader_msg_hdr)) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "data size %zu is less than header %zu\n",
> > > + data_len, sizeof(struct loader_msg_hdr));
> > > + report_bad_packet(client_data->loader_ishtp_cl,
> > > hdr);
> > > + goto end_error;
> > > + }
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "%s: command=%02lx is_response=%u status=%02x\n",
> > > + __func__,
> > > + hdr->command & CMD_MASK,
> > > + hdr->command & IS_RESPONSE ? 1 : 0,
> > > + hdr->status);
> > > +
> > > + switch (hdr->command & CMD_MASK) {
> > > + case LOADER_CMD_XFER_QUERY:
> > > + case LOADER_CMD_XFER_FRAGMENT:
> > > + case LOADER_CMD_START:
> > > + /* Sanity check */
> > > + if (client_data->response_data || client_data-
> > > > flag_response) {
>
> Following advice above, how about checking
> client_data->response_info->data_available instead?
> Or with advice above, corrupting old data might not be an issue,
> since the destination buffer changes? Also I wouldn't call this a
> buffer
> overrun below, it's a different problem.
>
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Buffer overrun: previous firmware
> > > message not yet processed\n");
> > > + report_bad_packet(client_data-
> > > >loader_ishtp_cl,
> > > hdr);
> > > + break;
> > > + }
> > > +
> > > + /*
> > > + * Copy the buffer received in firmware response
> > > for
> > > the
> > > + * calling thread.
> > > + */
> > > + client_data->response_data = kmalloc(data_len,
> > > GFP_KERNEL);
> > > + if (!client_data->response_data)
> > > + break;
> > > +
> > > + memcpy(client_data->response_data,
> > > + rb_in_proc->buffer.data, data_len);
> > > + client_data->response_size = data_len;
> > > +
> > > + /* Free the buffer */
> > > + ishtp_cl_io_rb_recycle(rb_in_proc);
> > > + rb_in_proc = NULL;
> > > +
> > > + /* Wake the calling thread */
> > > + client_data->flag_response = true;
> > > + wake_up_interruptible(&client_data->cmd_resp_wait);
> > > + break;
> > > +
> > > + default:
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Invalid command=%02lx\n",
> > > + hdr->command & CMD_MASK);
> > > + report_bad_packet(client_data->loader_ishtp_cl,
> > > hdr);
> > > + }
> > > +
> > > +end_error:
> > > + /* Free the buffer if we did not do above */
> > > + if (rb_in_proc)
> > > + ishtp_cl_io_rb_recycle(rb_in_proc);
> > > +}
> > > +
> > > +/**
> > > + * loader_cl_event_cb() - bus driver callback for incoming
> > > message
> > > + * @device: Pointer to the the ishtp client device for
> > > which
> > > + * this message is targeted
> > > + *
> > > + * Remove the packet from the list and process the message by
> > > calling
> > > + * process_recv
> > > + */
> > > +static void loader_cl_event_cb(struct ishtp_cl_device
> > > *cl_device)
> > > +{
> > > + struct ishtp_cl_rb *rb_in_proc;
> > > + struct ishtp_cl_data *client_data;
> > > + struct ishtp_cl *loader_ishtp_cl =
> > > ishtp_get_drvdata(cl_device);
> > > +
> > > + client_data = ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + while ((rb_in_proc = ishtp_cl_rx_get_rb(loader_ishtp_cl))
> > > !=
> > > NULL) {
> > > + if (!rb_in_proc->buffer.data) {
> > > + dev_warn(cl_data_to_dev(client_data),
> > > + "rb_in_proc->buffer.data returned
> > > null");
>
> Maybe move this check into process_recv() and then you can set
> client_data->response_info->error for it?
>
> > > + continue;
> > > + }
> > > +
> > > + /* Process the data packet from firmware */
> > > + process_recv(loader_ishtp_cl, rb_in_proc);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * ish_query_loader_prop() - Query ISH Shim firmware loader
> > > + * @client_data: Client data instance
> > > + * @fw: Poiner to fw data struct in host
> > > memory
> > > + *
> > > + * This function queries the ISH Shim firmware loader for
> > > capabilities.
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int ish_query_loader_prop(struct ishtp_cl_data
> > > *client_data,
> > > + const struct firmware *fw,
> > > + struct shim_fw_info *fw_info)
> > > +{
> > > + int rv;
> > > + size_t data_len;
> > > + struct loader_msg_hdr *hdr;
> > > + struct loader_xfer_query ldr_xfer_query;
> > > + struct loader_xfer_query_response *ldr_xfer_query_resp;
> > > +
> > > + memset(&ldr_xfer_query, 0, sizeof(ldr_xfer_query));
> > > + ldr_xfer_query.hdr.command = LOADER_CMD_XFER_QUERY;
> > > + ldr_xfer_query.image_size = fw->size;
> > > + rv = loader_cl_send(client_data,
> > > + (u8 *)&ldr_xfer_query,
> > > + sizeof(ldr_xfer_query));
> > > + if (rv < 0) {
> > > + client_data->flag_retry = true;
> > > + goto end_error;
> > > + }
> > > +
> > > + /* Check buffer size before accessing the elements */
> > > + data_len = client_data->response_size;
>
> Use rv instead of client_data->response_size, we want to minimize
> weird
> unexplainable accesses of the fileds of client_data.
>
> Also consider not using the variable data_len, it doesn't do too much
> besides
> cause some indirection. With the change above it should be obvious
> what is going on.
>
> > > + if (data_len != sizeof(struct loader_xfer_query_response))
> > > {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "data size %zu is not equal to size of
> > > loader_xfer_query_response %zu\n",
> > > + data_len, sizeof(struct
> > > loader_xfer_query_response));
> > > + hdr = (struct loader_msg_hdr *)client_data-
> > > > response_data;
>
> Following suggestion above you'll use the
>
> > > + report_bad_packet(client_data->loader_ishtp_cl,
> > > hdr);
> > > + client_data->flag_retry = true;
> > > + rv = -EMSGSIZE;
> > > + goto end_error;
> > > + }
> > > +
> > > + /* Save fw_info for use outside this function */
> > > + ldr_xfer_query_resp =
> > > + (struct loader_xfer_query_response *)client_data-
> > > > response_data;
> > >
> > > + *fw_info = ldr_xfer_query_resp->fw_info;
> > > +
> > > + /* Loader firmware properties */
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "ish_fw_version: major=%d minor=%d hotfix=%d
> > > build=%d
> > > protocol_version=0x%x loader_version=%d\n",
> > > + fw_info->ish_fw_version.major,
> > > + fw_info->ish_fw_version.minor,
> > > + fw_info->ish_fw_version.hotfix,
> > > + fw_info->ish_fw_version.build,
> > > + fw_info->protocol_version,
> > > + fw_info->ldr_version.value);
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "loader_capability: max_fw_image_size=0x%x
> > > xfer_mode=%d
> > > max_dma_buf_size=0x%x dma_buf_size_limit=0x%x\n",
> > > + fw_info->ldr_capability.max_fw_image_size,
> > > + fw_info->ldr_capability.xfer_mode,
> > > + fw_info->ldr_capability.max_dma_buf_size,
> > > + dma_buf_size_limit);
> > > +
> > > + /* Sanity checks */
> > > + if (fw_info->ldr_capability.max_fw_image_size < fw->size) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "ISH firmware size %zu is greater than Shim
> > > firmware loader max supported %d\n",
> > > + fw->size,
> > > + fw_info->ldr_capability.max_fw_image_size);
> > > + rv = -ENOSPC;
> > > + goto end_error;
> > > + }
> > > +
> > > + /* For DMA the buffer size should be multiple of cacheline
> > > size
> > > */
> > > + if ((fw_info->ldr_capability.xfer_mode &
> > > LOADER_XFER_MODE_DIRECT_DMA) &&
> > > + (fw_info->ldr_capability.max_dma_buf_size %
> > > L1_CACHE_BYTES)) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Shim firmware loader buffer size %d should
> > > be
> > > multipe of cacheline\n",
> > > + fw_info->ldr_capability.max_dma_buf_size);
> > > + rv = -EINVAL;
> > > + goto end_error;
> > > + }
> > > +
> > > +end_error:
> > > + /* Free ISH buffer if not done so in error case */
> > > + kfree(client_data->response_data);
> > > + client_data->response_data = NULL;
> > > + return rv;
> > > +}
> > > +
> > > +/**
> > > + * ish_fw_xfer_ishtp() Loads ISH firmware using ishtp
> > > interface
> > > + * @client_data: Client data instance
> > > + * @fw: Pointer to fw data struct in host
> > > memory
> > > + *
> > > + * This function uses ISH-TP to transfer ISH firmware from host
> > > to
> > > + * ISH SRAM. Lower layers may use IPC or DMA depending on
> > > firmware
> > > + * support.
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int ish_fw_xfer_ishtp(struct ishtp_cl_data *client_data,
> > > + const struct firmware *fw)
> > > +{
> > > + int rv;
> > > + u32 fragment_offset, fragment_size, payload_max_size;
> > > + struct loader_xfer_ipc_fragment *ldr_xfer_ipc_frag;
> > > +
> > > + payload_max_size =
> > > + LOADER_SHIM_IPC_BUF_SIZE -
> > > IPC_FRAGMENT_DATA_PREAMBLE;
> > > +
> > > + ldr_xfer_ipc_frag = kzalloc(LOADER_SHIM_IPC_BUF_SIZE,
> > > GFP_KERNEL);
> > > + if (!ldr_xfer_ipc_frag) {
> > > + client_data->flag_retry = true;
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + ldr_xfer_ipc_frag->fragment.hdr.command =
> > > LOADER_CMD_XFER_FRAGMENT;
> > > + ldr_xfer_ipc_frag->fragment.xfer_mode =
> > > LOADER_XFER_MODE_ISHTP;
> > > +
> > > + /* Break the firmware image into fragments and send as ISH-
> > > TP
> > > payload */
> > > + fragment_offset = 0;
> > > + while (fragment_offset < fw->size) {
> > > + if (fragment_offset + payload_max_size < fw->size)
> > > {
> > > + fragment_size = payload_max_size;
> > > + ldr_xfer_ipc_frag->fragment.is_last = 0;
> > > + } else {
> > > + fragment_size = fw->size - fragment_offset;
> > > + ldr_xfer_ipc_frag->fragment.is_last = 1;
> > > + }
> > > +
> > > + ldr_xfer_ipc_frag->fragment.offset =
> > > fragment_offset;
> > > + ldr_xfer_ipc_frag->fragment.size = fragment_size;
> > > + memcpy(ldr_xfer_ipc_frag->data,
> > > + &fw->data[fragment_offset],
> > > + fragment_size);
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "xfer_mode=ipc offset=0x%08x size=0x%08x
> > > is_last=%d\n",
> > > + ldr_xfer_ipc_frag->fragment.offset,
> > > + ldr_xfer_ipc_frag->fragment.size,
> > > + ldr_xfer_ipc_frag->fragment.is_last);
> > > +
> > > + rv = loader_cl_send(client_data,
> > > + (u8 *)ldr_xfer_ipc_frag,
> > > + IPC_FRAGMENT_DATA_PREAMBLE +
> > > fragment_size);
> > > + if (rv < 0) {
> > > + client_data->flag_retry = true;
> > > + goto end_err_resp_buf_release;
> > > + }
> > > +
> > > + /* Free ISH buffer once response is processed */
> > > + kfree(client_data->response_data);
> > > + client_data->response_data = NULL;
> > > +
> > > + fragment_offset += fragment_size;
> > > + }
> > > +
> > > + kfree(ldr_xfer_ipc_frag);
> > > + return 0;
> > > +
> > > +end_err_resp_buf_release:
> > > + /* Free ISH buffer if not done already, in error case */
> > > + kfree(client_data->response_data);
> > > + client_data->response_data = NULL;
> > > + kfree(ldr_xfer_ipc_frag);
> > > + return rv;
> > > +}
> > > +
> > > +/**
> > > + * ish_fw_xfer_direct_dma() - Loads ISH firmware using direct
> > > dma
> > > + * @client_data: Client data instance
> > > + * @fw: Poiner to fw data struct in host
> > > memory
> > > + *
> > > + * Host firmware load is a unique case where we need to download
> > > + * a large firmware image (200+ Kb). This function implements
> > > + * direct DMA transfer in kernel and ISH firmware. This allows
> > > + * us to overcome the ISH-TP 4 Kb limit, and allows us to DMA
> > > + * directly to ISH UMA at location of choice.
> > > + * Function depends on corresponding support in ISH firmware.
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int ish_fw_xfer_direct_dma(struct ishtp_cl_data
> > > *client_data,
> > > + const struct firmware *fw,
> > > + struct shim_fw_info fw_info)
> > > +{
> > > + int rv;
> > > + void *dma_buf;
> > > + dma_addr_t dma_buf_phy;
> > > + u32 fragment_offset, fragment_size, payload_max_size;
> > > + struct loader_xfer_dma_fragment ldr_xfer_dma_frag;
> > > + struct device *devc = ishtp_get_pci_device(client_data-
> > > > cl_device);
> > >
> > > + u32 shim_fw_buf_size =
> > > + fw_info.ldr_capability.max_dma_buf_size;
> > > +
> > > + /*
> > > + * payload_max_size should be set to minimum of
> > > + * (1) Size of firmware to be loaded,
> > > + * (2) Max DMA buf size supported by Shim firmware,
> > > + * (3) DMA buffer size limit set by boot_param
> > > dma_buf_size_limit.
> > > + */
> > > + payload_max_size = min3(fw->size,
> > > + (size_t)shim_fw_buf_size,
> > > + (size_t)dma_buf_size_limit);
> > > +
> > > + /*
> > > + * Buffer size should be multiple of cacheline size
> > > + * if it's not, select the previous cacheline boundary.
> > > + */
> > > + payload_max_size &= ~(L1_CACHE_BYTES - 1);
> > > +
> > > + dma_buf = kmalloc(payload_max_size, GFP_KERNEL |
> > > GFP_DMA32);
> > > + if (!dma_buf) {
> > > + client_data->flag_retry = true;
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + dma_buf_phy = dma_map_single(devc, dma_buf,
> > > payload_max_size,
> > > + DMA_TO_DEVICE);
> > > + if (dma_mapping_error(devc, dma_buf_phy)) {
> > > + dev_err(cl_data_to_dev(client_data), "DMA map
> > > failed\n");
> > > + client_data->flag_retry = true;
> > > + rv = -ENOMEM;
> > > + goto end_err_dma_buf_release;
> > > + }
> > > +
> > > + ldr_xfer_dma_frag.fragment.hdr.command =
> > > LOADER_CMD_XFER_FRAGMENT;
> > > + ldr_xfer_dma_frag.fragment.xfer_mode =
> > > LOADER_XFER_MODE_DIRECT_DMA;
> > > + ldr_xfer_dma_frag.ddr_phys_addr = (u64)dma_buf_phy;
> > > +
> > > + /* Send the firmware image in chucks of payload_max_size */
> > > + fragment_offset = 0;
> > > + while (fragment_offset < fw->size) {
> > > + if (fragment_offset + payload_max_size < fw->size)
> > > {
> > > + fragment_size = payload_max_size;
> > > + ldr_xfer_dma_frag.fragment.is_last = 0;
> > > + } else {
> > > + fragment_size = fw->size - fragment_offset;
> > > + ldr_xfer_dma_frag.fragment.is_last = 1;
> > > + }
> > > +
> > > + ldr_xfer_dma_frag.fragment.offset =
> > > fragment_offset;
> > > + ldr_xfer_dma_frag.fragment.size = fragment_size;
> > > + memcpy(dma_buf, &fw->data[fragment_offset],
> > > fragment_size);
> > > +
> > > + dma_sync_single_for_device(devc, dma_buf_phy,
> > > + payload_max_size,
> > > + DMA_TO_DEVICE);
> > > +
> > > + /*
> > > + * Flush cache here because the
> > > dma_sync_single_for_device()
> > > + * does not do for x86.
> > > + */
> > > + clflush_cache_range(dma_buf, payload_max_size);
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "xfer_mode=dma offset=0x%08x size=0x%x
> > > is_last=%d ddr_phys_addr=0x%016llx\n",
> > > + ldr_xfer_dma_frag.fragment.offset,
> > > + ldr_xfer_dma_frag.fragment.size,
> > > + ldr_xfer_dma_frag.fragment.is_last,
> > > + ldr_xfer_dma_frag.ddr_phys_addr);
> > > +
> > > + rv = loader_cl_send(client_data,
> > > + (u8 *)&ldr_xfer_dma_frag,
> > > + sizeof(ldr_xfer_dma_frag));
> > > + if (rv < 0) {
> > > + client_data->flag_retry = true;
> > > + goto end_err_resp_buf_release;
> > > + }
> > > +
> > > + /* Free ISH buffer once response is processed */
> > > + kfree(client_data->response_data);
> > > + client_data->response_data = NULL;
> > > +
> > > + fragment_offset += fragment_size;
> > > + }
> > > +
> > > + dma_unmap_single(devc, dma_buf_phy, payload_max_size,
> > > DMA_TO_DEVICE);
> > > + kfree(dma_buf);
> > > + return 0;
> > > +
> > > +end_err_resp_buf_release:
> > > + /* Free ISH buffer if not done already, in error case */
> > > + kfree(client_data->response_data);
> > > + client_data->response_data = NULL;
> > > + dma_unmap_single(devc, dma_buf_phy, payload_max_size,
> > > DMA_TO_DEVICE);
> > > +end_err_dma_buf_release:
> > > + kfree(dma_buf);
> > > + return rv;
> > > +}
> > > +
> > > +/**
> > > + * ish_fw_start() Start executing ISH main firmware
> > > + * @client_data: client data instance
> > > + *
> > > + * This function sends message to Shim firmware loader to start
> > > + * the execution of ISH main firmware.
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int ish_fw_start(struct ishtp_cl_data *client_data)
> > > +{
> > > + int rv;
> > > + struct loader_start ldr_start;
> > > +
> > > + memset(&ldr_start, 0, sizeof(ldr_start));
> > > + ldr_start.hdr.command = LOADER_CMD_START;
> > > + rv = loader_cl_send(client_data,
> > > + (u8 *)&ldr_start,
> > > + sizeof(ldr_start));
> > > +
> > > + /* Free ISH buffer once response is processed */
> > > + kfree(client_data->response_data);
> > > + client_data->response_data = NULL;
> > > + return rv;
> > > +}
> > > +
> > > +/**
> > > + * load_fw_from_host() Loads ISH firmware from host
> > > + * @client_data: Client data instance
> > > + *
> > > + * This function loads the ISH firmware to ISH sram and starts
> > > execution
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int load_fw_from_host(struct ishtp_cl_data *client_data)
> > > +{
> > > + int rv;
> > > + u32 xfer_mode;
> > > + char *filename;
> > > + const struct firmware *fw;
> > > + struct shim_fw_info fw_info;
> > > +
> > > + client_data->flag_retry = false;
> > > +
> > > + filename = kzalloc(FILENAME_SIZE, GFP_KERNEL);
> > > + if (!filename) {
> > > + rv = -ENOMEM;
> > > + goto end_error;
> > > + }
> > > +
> > > + /* Get filename of the ISH firmware to be loaded */
> > > + rv = get_firmware_variant(client_data, filename);
> > > + if (rv < 0)
> > > + goto end_err_filename_buf_release;
> > > +
> > > + rv = request_firmware(&fw, filename,
> > > cl_data_to_dev(client_data));
> > > + if (rv < 0)
> > > + goto end_err_filename_buf_release;
> > > +
> > > + /* Step 1: Query Shim firmware loader properties */
> > > +
> > > + rv = ish_query_loader_prop(client_data, fw, &fw_info);
> > > + if (rv < 0)
> > > + goto end_err_fw_release;
> > > +
> > > + /* Step 2: Send the main firmware image to be loaded, to
> > > ISH
> > > sram */
> > > +
> > > + xfer_mode = fw_info.ldr_capability.xfer_mode;
> > > + if (xfer_mode & LOADER_XFER_MODE_DIRECT_DMA) {
> > > + rv = ish_fw_xfer_direct_dma(client_data, fw,
> > > fw_info);
> > > + } else if (xfer_mode & LOADER_XFER_MODE_ISHTP) {
> > > + rv = ish_fw_xfer_ishtp(client_data, fw);
> > > + } else {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "No transfer mode selected in firmware\n");
> > > + rv = -EINVAL;
> > > + }
> > > + if (rv < 0)
> > > + goto end_err_fw_release;
> > > +
> > > + /* Step 3: Start ISH main firmware exeuction */
> > > +
> > > + rv = ish_fw_start(client_data);
> > > + if (rv < 0)
> > > + goto end_err_fw_release;
> > > +
> > > + release_firmware(fw);
> > > + kfree(filename);
> > > + dev_info(cl_data_to_dev(client_data), "ISH firmware %s
> > > loaded\n",
> > > + filename);
> > > + return 0;
> > > +
> > > +end_err_fw_release:
> > > + release_firmware(fw);
> > > +end_err_filename_buf_release:
> > > + kfree(filename);
> > > +end_error:
> > > + if (client_data->flag_retry) {
> > > + dev_warn(cl_data_to_dev(client_data),
> > > + "ISH host firmware load failed %d. Reset
> > > ISH &
> > > try again..\n",
> > > + rv);
> > > + loader_ish_hw_reset(client_data->loader_ishtp_cl);
>
> This could just keep failing infinitely, right? Do you want to add
> some retry counter,
> and after some limit then give up or something? What happens if the
> ISH firmware
> never succeeds in loading?
>
> > > + } else {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "ISH host firmware load failed %d\n", rv);
> > > + }
> > > + return rv;
> > > +}
>
> And there were many typos in comments that I saw, comb through them
> carefully again.
>
> Cheers,
> Nick
^ permalink raw reply
* [PATCH RESEND v4 0/4] Add support for lradc on A83T
From: megous via linux-sunxi @ 2019-03-27 2:33 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
robh-DgEjT+Ai2ygdnm+yROfE0A, Dmitry Torokhov,
mark.rutland-5wv7dgnIgG8
Cc: Ondrej Jirman, Ziping Chen, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA
From: Ondrej Jirman <megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
(This is a re-send, with input subsystem maintainer added. Sorry,
for the extra noise, I did only send the v4 to the previous recipients
of v3, and didn't notice input subsystem was missing back then too.
Thank you wens for pointing it out.)
This series implements r_lradc (low res ADC) support for A83T.
This is a continuation of v3 patch series from 2017 by Ziping Chen. I've
rebased it on top of linux-next and changed the compatibe string as
requested back then.[1]
There was some talk of iio based lradc driver back then[2], but noone
stepped forward to make it. Let's finish this series, so that it's
at least possible to add support for tablet keys on various A83T tablets
as that's the main use of lradc module.
[1] https://lkml.org/lkml/2017/6/26/558
[2] https://lkml.org/lkml/2017/6/27/877
Please take a look, and apply the patches if you have no objections.
regards,
Ondrej Jirman
Changes for v4:
- changed comaptible string to allwinner,sun8i-a83t-r-lradc
(drop -keys suffix)
- dropped 0 prefix in the r_lradc DT node
- added sample A83T lradc user (TBS A711 tablet)
Changes for v3:
- Fix some issuses raised by Maxime.
- Added Rob's ACK.
Changes for v2:
- Add an A83T specific compatible.
Ondrej Jirman (1):
ARM: dts: sun8i: tbs-a711: Add support for volume keys input
Ziping Chen (3):
input: sun4i-a10-lradc-keys: Add support for A83T
dt-bindings: input: Add R_LRADC support for A83T
ARM: dts: sunxi: Add R_LRADC support for A83T
.../bindings/input/sun4i-lradc-keys.txt | 6 ++-
arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 20 ++++++++++
arch/arm/boot/dts/sun8i-a83t.dtsi | 7 ++++
drivers/input/keyboard/sun4i-lradc-keys.c | 38 +++++++++++++++++--
4 files changed, 65 insertions(+), 6 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH RESEND v4 1/4] input: sun4i-a10-lradc-keys: Add support for A83T
From: megous via linux-sunxi @ 2019-03-27 2:33 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
robh-DgEjT+Ai2ygdnm+yROfE0A, Dmitry Torokhov,
mark.rutland-5wv7dgnIgG8
Cc: Ziping Chen, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <20190327023339.25975-1-megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
From: Ziping Chen <techping.chan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Allwinner A83T SoC has a low res adc like the one in Allwinner A10 SoC,
however, the A10 SoC's vref of lradc internally is divided by 2/3 and
the A83T SoC's vref of lradc internally is divided by 3/4, thus add
a hardware variant for it to be compatible with various devices.
Signed-off-by: Ziping Chen <techping.chan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
drivers/input/keyboard/sun4i-lradc-keys.c | 38 ++++++++++++++++++++---
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
index 57272df34cd5..df3eec72a9b2 100644
--- a/drivers/input/keyboard/sun4i-lradc-keys.c
+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
@@ -46,6 +46,7 @@
#define CONTINUE_TIME_SEL(x) ((x) << 16) /* 4 bits */
#define KEY_MODE_SEL(x) ((x) << 12) /* 2 bits */
#define LEVELA_B_CNT(x) ((x) << 8) /* 4 bits */
+#define HOLD_KEY_EN(x) ((x) << 7)
#define HOLD_EN(x) ((x) << 6)
#define LEVELB_VOL(x) ((x) << 4) /* 2 bits */
#define SAMPLE_RATE(x) ((x) << 2) /* 2 bits */
@@ -63,6 +64,25 @@
#define CHAN0_KEYDOWN_IRQ BIT(1)
#define CHAN0_DATA_IRQ BIT(0)
+/* struct lradc_variant - Describe sun4i-a10-lradc-keys hardware variant
+ * @divisor_numerator: The numerator of lradc Vref internally divisor
+ * @divisor_denominator: The denominator of lradc Vref internally divisor
+ */
+struct lradc_variant {
+ u8 divisor_numerator;
+ u8 divisor_denominator;
+};
+
+static const struct lradc_variant lradc_variant_a10 = {
+ .divisor_numerator = 2,
+ .divisor_denominator = 3
+};
+
+static const struct lradc_variant r_lradc_variant_a83t = {
+ .divisor_numerator = 3,
+ .divisor_denominator = 4
+};
+
struct sun4i_lradc_keymap {
u32 voltage;
u32 keycode;
@@ -74,6 +94,7 @@ struct sun4i_lradc_data {
void __iomem *base;
struct regulator *vref_supply;
struct sun4i_lradc_keymap *chan0_map;
+ const struct lradc_variant *variant;
u32 chan0_map_count;
u32 chan0_keycode;
u32 vref;
@@ -128,9 +149,9 @@ static int sun4i_lradc_open(struct input_dev *dev)
if (error)
return error;
- /* lradc Vref internally is divided by 2/3 */
- lradc->vref = regulator_get_voltage(lradc->vref_supply) * 2 / 3;
-
+ lradc->vref = regulator_get_voltage(lradc->vref_supply) *
+ lradc->variant->divisor_numerator /
+ lradc->variant->divisor_denominator;
/*
* Set sample time to 4 ms / 250 Hz. Wait 2 * 4 ms for key to
* stabilize on press, wait (1 + 1) * 4 ms for key release
@@ -222,6 +243,12 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
if (error)
return error;
+ lradc->variant = of_device_get_match_data(&pdev->dev);
+ if (!lradc->variant) {
+ dev_err(&pdev->dev, "Missing sun4i-a10-lradc-keys variant\n");
+ return -EINVAL;
+ }
+
lradc->vref_supply = devm_regulator_get(dev, "vref");
if (IS_ERR(lradc->vref_supply))
return PTR_ERR(lradc->vref_supply);
@@ -265,7 +292,10 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
}
static const struct of_device_id sun4i_lradc_of_match[] = {
- { .compatible = "allwinner,sun4i-a10-lradc-keys", },
+ { .compatible = "allwinner,sun4i-a10-lradc-keys",
+ .data = &lradc_variant_a10 },
+ { .compatible = "allwinner,sun8i-a83t-r-lradc",
+ .data = &r_lradc_variant_a83t },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v4 2/4] dt-bindings: input: Add R_LRADC support for A83T
From: megous via linux-sunxi @ 2019-03-27 2:33 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
robh-DgEjT+Ai2ygdnm+yROfE0A, Dmitry Torokhov,
mark.rutland-5wv7dgnIgG8
Cc: Ziping Chen, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <20190327023339.25975-1-megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
From: Ziping Chen <techping.chan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Allwinner A83T SoC has a low res ADC like the one in Allwinner A10 SoC.
Add binding for it.
Signed-off-by: Ziping Chen <techping.chan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
.../devicetree/bindings/input/sun4i-lradc-keys.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt b/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
index 1458c3179a63..496125c6bfb7 100644
--- a/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
+++ b/Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
@@ -2,12 +2,14 @@ Allwinner sun4i low res adc attached tablet keys
------------------------------------------------
Required properties:
- - compatible: "allwinner,sun4i-a10-lradc-keys"
+ - compatible: should be one of the following string:
+ "allwinner,sun4i-a10-lradc-keys"
+ "allwinner,sun8i-a83t-r-lradc"
- reg: mmio address range of the chip
- interrupts: interrupt to which the chip is connected
- vref-supply: powersupply for the lradc reference voltage
-Each key is represented as a sub-node of "allwinner,sun4i-a10-lradc-keys":
+Each key is represented as a sub-node of the compatible mentioned above:
Required subnode-properties:
- label: Descriptive name of the key.
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v4 3/4] ARM: dts: sunxi: Add R_LRADC support for A83T
From: megous via linux-sunxi @ 2019-03-27 2:33 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
robh-DgEjT+Ai2ygdnm+yROfE0A, Dmitry Torokhov,
mark.rutland-5wv7dgnIgG8
Cc: Ziping Chen, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <20190327023339.25975-1-megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
From: Ziping Chen <techping.chan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Allwinner A83T SoC has a low res adc like the one in Allwinner A10 SoC.
Now the driver has been modified to support it.
Add support for it.
Signed-off-by: Ziping Chen <techping.chan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/boot/dts/sun8i-a83t.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index fcb7ef5ce2df..de1b77e49f06 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -1024,6 +1024,13 @@
status = "disabled";
};
+ r_lradc: lradc@1f03c00 {
+ compatible = "allwinner,sun8i-a83t-r-lradc";
+ reg = <0x01f03c00 0x100>;
+ interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+
r_pio: pinctrl@1f02c00 {
compatible = "allwinner,sun8i-a83t-r-pinctrl";
reg = <0x01f02c00 0x400>;
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v4 4/4] ARM: dts: sun8i: tbs-a711: Add support for volume keys input
From: megous via linux-sunxi @ 2019-03-27 2:33 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
robh-DgEjT+Ai2ygdnm+yROfE0A, Dmitry Torokhov,
mark.rutland-5wv7dgnIgG8
Cc: Ondrej Jirman, Ziping Chen, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-input-u79uwXL29TY76Z2rM5mHXA,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <20190327023339.25975-1-megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
From: Ondrej Jirman <megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
TBS A711 tablet has volume up/down keys connected to r_lradc. Add
support for these keys.
Signed-off-by: Ondrej Jirman <megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
---
arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
index 1e840ab5a541..6ecdc11b7059 100644
--- a/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts
@@ -46,6 +46,7 @@
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/pwm/pwm.h>
+#include <dt-bindings/input/input.h>
/ {
model = "TBS A711 Tablet";
@@ -200,6 +201,25 @@
status = "okay";
};
+&r_lradc {
+ vref-supply = <®_aldo2>;
+ status = "okay";
+
+ button@210 {
+ label = "Volume Up";
+ linux,code = <KEY_VOLUMEUP>;
+ channel = <0>;
+ voltage = <210000>;
+ };
+
+ button@410 {
+ label = "Volume Down";
+ linux,code = <KEY_VOLUMEDOWN>;
+ channel = <0>;
+ voltage = <410000>;
+ };
+};
+
&r_rsb {
status = "okay";
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.
From: Greg Kroah-Hartman @ 2019-03-27 2:37 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327014807.7472-4-ronald@innovation.ch>
On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> instead of straight printk() to match other dev_xxx() logging functions.
> ---
> drivers/base/core.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/device.h | 15 +++++++++++++++
> 2 files changed, 58 insertions(+)
No signed-off-by?
Anyway, no, please do not do this. Please do not dump large hex values
like this to the kernel log, it does not help anyone.
You can do this while debugging, sure, but not for "real" kernel code.
Worst case, just create a debugfs file for your device that you can read
the binary data from if you really need it. For any "normal" operation,
this is not something that you should ever need.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] input: keyboard: snvs: make sure irq is handled correctly
From: Anson Huang @ 2019-03-27 2:47 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, Fabio Estevam,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: dl-linux-imx
SNVS IRQ is requested before necessary driver data initialized,
if there is a pending IRQ during driver probe phase, kernel
NULL pointer panic will occur in IRQ handler. To avoid such
scenario, need to move the IRQ request to after driver data
initialization done. This patch is inspired by NXP's internal
kernel tree.
Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/input/keyboard/snvs_pwrkey.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index effb632..6ff41fd 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return error;
}
- error = devm_request_irq(&pdev->dev, pdata->irq,
- imx_snvs_pwrkey_interrupt,
- 0, pdev->name, pdev);
-
- if (error) {
- dev_err(&pdev->dev, "interrupt not available.\n");
- return error;
- }
-
error = input_register_device(input);
if (error < 0) {
dev_err(&pdev->dev, "failed to register input device\n");
@@ -166,6 +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
pdata->input = input;
platform_set_drvdata(pdev, pdata);
+ error = devm_request_irq(&pdev->dev, pdata->irq,
+ imx_snvs_pwrkey_interrupt,
+ 0, pdev->name, pdev);
+ if (error) {
+ dev_err(&pdev->dev, "interrupt not available.\n");
+ return error;
+ }
+
device_init_wakeup(&pdev->dev, pdata->wakeup);
return 0;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] input: keyboard: snvs: make sure irq is handled correctly
From: dmitry.torokhov @ 2019-03-27 4:29 UTC (permalink / raw)
To: Anson Huang
Cc: Fabio Estevam, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, dl-linux-imx
In-Reply-To: <1553654517-7810-1-git-send-email-Anson.Huang@nxp.com>
Hi Anson,
On Wed, Mar 27, 2019 at 02:47:06AM +0000, Anson Huang wrote:
> SNVS IRQ is requested before necessary driver data initialized,
> if there is a pending IRQ during driver probe phase, kernel
> NULL pointer panic will occur in IRQ handler. To avoid such
> scenario, need to move the IRQ request to after driver data
> initialization done. This patch is inspired by NXP's internal
> kernel tree.
>
> Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/input/keyboard/snvs_pwrkey.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index effb632..6ff41fd 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return error;
> }
>
> - error = devm_request_irq(&pdev->dev, pdata->irq,
> - imx_snvs_pwrkey_interrupt,
> - 0, pdev->name, pdev);
> -
> - if (error) {
> - dev_err(&pdev->dev, "interrupt not available.\n");
> - return error;
> - }
> -
> error = input_register_device(input);
> if (error < 0) {
> dev_err(&pdev->dev, "failed to register input device\n");
> @@ -166,6 +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> pdata->input = input;
> platform_set_drvdata(pdev, pdata);
>
> + error = devm_request_irq(&pdev->dev, pdata->irq,
> + imx_snvs_pwrkey_interrupt,
> + 0, pdev->name, pdev);
> + if (error) {
> + dev_err(&pdev->dev, "interrupt not available.\n");
> + return error;
> + }
Instead of moving devm_request_irq() around could you simply move
pdata->input = input assignment higher? It is perfectly fine to try
calling input_event() on input device that is allocated but not yet
registered.
> +
> device_init_wakeup(&pdev->dev, pdata->wakeup);
Unrelated suggestion:
Can you try calling dev_pm_set_wake_irq(&pdev->dev, pdata->irq) here and
I think you will be able to get rid of suspend/resume methods in the
driver.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: elan_i2c - Add many hardware ID for lenovo laptop
From: Dmitry Torokhov @ 2019-03-27 4:29 UTC (permalink / raw)
To: KT Liao; +Cc: linux-kernel, linux-input
In-Reply-To: <1552479463-3686-1-git-send-email-kt.liao@emc.com.tw>
On Wed, Mar 13, 2019 at 08:17:43PM +0800, KT Liao wrote:
> There are many Lenovo laptop which need elan_i2c support.
> Elan collects the list ands add to add to elan_acpi_id[]
>
> Signed-off-by: KT Liao <kt.liao@emc.com.tw>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: elan_i2c - Add i2c_reset in sysfs for ELAN touchpad recovery
From: Dmitry Torokhov @ 2019-03-27 4:34 UTC (permalink / raw)
To: KT Liao; +Cc: linux-kernel, linux-input, ulrik.debie-os, Roger.Whittaker
In-Reply-To: <1549094096-4082-1-git-send-email-kt.liao@emc.com.tw>
Hi KT,
On Sat, Feb 02, 2019 at 03:54:56PM +0800, KT Liao wrote:
> Roger from SUSE reported the touchpad on Lenovo yoga2 crush sometimes.
> He found that rmmod/modprobe elan_i2c will recover the issue.
> He add the workaround on SUSE and solve the problem.
> Recently, the workaround fails in kernel 4.20 becasue IRQ mismatch.
> genirq: Flags mismatch irq 0. 00002002 (ELAN0600:00) vs. 00015a00 (timer)
> I can't reproduce the issue in SUSE with the same kernel.
> And it's a 5 years old laptop, ELAN can't find the module for testing.
> Instead of IRQ debugging IRQ, I tried another approach.
> I added i2c_reset in sysfs to avoid IRQ requesting in probe.
How will users discover this flag? I do not think this is the best
approach. Can we detect that the touchpad firmware crashed from the
kernel and reset automatically?
Thanks.
>
> Signed-off-by: KT Liao <kt.liao@emc.com.tw>
> Acked-by: Roger Whittaker <Roger.Whittaker@suse.com>
> ---
> drivers/input/mouse/elan_i2c_core.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index 2690a4b..388b1f0 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -670,6 +670,29 @@ static ssize_t calibrate_store(struct device *dev,
> return retval ?: count;
> }
>
> +static ssize_t i2c_reset_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct elan_tp_data *data = i2c_get_clientdata(client);
> + int retval;
> +
> + retval = mutex_lock_interruptible(&data->sysfs_mutex);
> + if (retval)
> + return retval;
> +
> + disable_irq(client->irq);
> +
> + retval = elan_initialize(data);
> + if (retval)
> + dev_err(dev, "failed to re-initialize touchpad: %d\n", retval);
> +
> + enable_irq(client->irq);
> + mutex_unlock(&data->sysfs_mutex);
> + return retval ?: count;
> +}
> +
> static ssize_t elan_sysfs_read_mode(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -702,6 +725,7 @@ static DEVICE_ATTR(mode, S_IRUGO, elan_sysfs_read_mode, NULL);
> static DEVICE_ATTR(update_fw, S_IWUSR, NULL, elan_sysfs_update_fw);
>
> static DEVICE_ATTR_WO(calibrate);
> +static DEVICE_ATTR_WO(i2c_reset);
>
> static struct attribute *elan_sysfs_entries[] = {
> &dev_attr_product_id.attr,
> @@ -710,6 +734,7 @@ static struct attribute *elan_sysfs_entries[] = {
> &dev_attr_iap_version.attr,
> &dev_attr_fw_checksum.attr,
> &dev_attr_calibrate.attr,
> + &dev_attr_i2c_reset.attr,
> &dev_attr_mode.attr,
> &dev_attr_update_fw.attr,
> NULL,
> --
> 2.7.4
>
--
Dmitry
^ permalink raw reply
* RE: [PATCH] input: keyboard: snvs: make sure irq is handled correctly
From: Anson Huang @ 2019-03-27 5:05 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: Fabio Estevam, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, dl-linux-imx
In-Reply-To: <20190327042903.GA40550@dtor-ws>
Hi, Dmitry
Best Regards!
Anson Huang
> -----Original Message-----
> From: dmitry.torokhov@gmail.com [mailto:dmitry.torokhov@gmail.com]
> Sent: 2019年3月27日 12:29
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] input: keyboard: snvs: make sure irq is handled
> correctly
>
> Hi Anson,
>
> On Wed, Mar 27, 2019 at 02:47:06AM +0000, Anson Huang wrote:
> > SNVS IRQ is requested before necessary driver data initialized, if
> > there is a pending IRQ during driver probe phase, kernel NULL pointer
> > panic will occur in IRQ handler. To avoid such scenario, need to move
> > the IRQ request to after driver data initialization done. This patch
> > is inspired by NXP's internal kernel tree.
> >
> > Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key
> > driver")
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > drivers/input/keyboard/snvs_pwrkey.c | 17 ++++++++---------
> > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/snvs_pwrkey.c
> > b/drivers/input/keyboard/snvs_pwrkey.c
> > index effb632..6ff41fd 100644
> > --- a/drivers/input/keyboard/snvs_pwrkey.c
> > +++ b/drivers/input/keyboard/snvs_pwrkey.c
> > @@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct
> platform_device *pdev)
> > return error;
> > }
> >
> > - error = devm_request_irq(&pdev->dev, pdata->irq,
> > - imx_snvs_pwrkey_interrupt,
> > - 0, pdev->name, pdev);
> > -
> > - if (error) {
> > - dev_err(&pdev->dev, "interrupt not available.\n");
> > - return error;
> > - }
> > -
> > error = input_register_device(input);
> > if (error < 0) {
> > dev_err(&pdev->dev, "failed to register input device\n");
> @@ -166,6
> > +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device
> *pdev)
> > pdata->input = input;
> > platform_set_drvdata(pdev, pdata);
> >
> > + error = devm_request_irq(&pdev->dev, pdata->irq,
> > + imx_snvs_pwrkey_interrupt,
> > + 0, pdev->name, pdev);
> > + if (error) {
> > + dev_err(&pdev->dev, "interrupt not available.\n");
> > + return error;
> > + }
>
> Instead of moving devm_request_irq() around could you simply move
> pdata->input = input assignment higher? It is perfectly fine to try
> calling input_event() on input device that is allocated but not yet registered.
OK, make sense.
>
> > +
> > device_init_wakeup(&pdev->dev, pdata->wakeup);
>
> Unrelated suggestion:
>
> Can you try calling dev_pm_set_wake_irq(&pdev->dev, pdata->irq) here and
> I think you will be able to get rid of suspend/resume methods in the driver.
OK, I will look into it, and send another patch to improve this.
Thanks.
Anson
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* [PATCH V2] input: keyboard: snvs: initialize necessary driver data before enabling IRQ
From: Anson Huang @ 2019-03-27 6:07 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, Fabio Estevam,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: dl-linux-imx
SNVS IRQ is requested before necessary driver data initialized,
if there is a pending IRQ during driver probe phase, kernel
NULL pointer panic will occur in IRQ handler. To avoid such
scenario, just initialize necessary driver data before enabling
IRQ. This patch is inspired by NXP's internal kernel tree.
Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V1:
- move the platform data initialization to before IRQ enable instead of moving the IRQ enable
to the end of probe function.
---
drivers/input/keyboard/snvs_pwrkey.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index effb632..4c67cf3 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,6 +148,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return error;
}
+ pdata->input = input;
+ platform_set_drvdata(pdev, pdata);
+
error = devm_request_irq(&pdev->dev, pdata->irq,
imx_snvs_pwrkey_interrupt,
0, pdev->name, pdev);
@@ -163,9 +166,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return error;
}
- pdata->input = input;
- platform_set_drvdata(pdev, pdata);
-
device_init_wakeup(&pdev->dev, pdata->wakeup);
return 0;
--
2.7.4
^ permalink raw reply related
* [PATCH] input: keyboard: snvs: use dev_pm_set_wake_irq() to simplify code
From: Anson Huang @ 2019-03-27 6:08 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, Fabio Estevam,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: dl-linux-imx
With calling dev_pm_set_wake_irq() to set SNVS ON/OFF button
as wakeup source for suspend, generic wake irq mechanism
will automatically enable it as wakeup source when suspend,
then the enable_irq_wake()/disable_irq_wake() can be removed
in suspend/resume callback, it simplifies the code.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/input/keyboard/snvs_pwrkey.c | 30 ++++--------------------------
1 file changed, 4 insertions(+), 26 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 4c67cf3..5342d8d 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -15,6 +15,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
@@ -167,28 +168,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
}
device_init_wakeup(&pdev->dev, pdata->wakeup);
-
- return 0;
-}
-
-static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
-
- if (device_may_wakeup(&pdev->dev))
- enable_irq_wake(pdata->irq);
-
- return 0;
-}
-
-static int __maybe_unused imx_snvs_pwrkey_resume(struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
-
- if (device_may_wakeup(&pdev->dev))
- disable_irq_wake(pdata->irq);
+ error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
+ if (error)
+ dev_err(&pdev->dev, "irq wake enable failed.\n");
return 0;
}
@@ -199,13 +181,9 @@ static const struct of_device_id imx_snvs_pwrkey_ids[] = {
};
MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
-static SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops, imx_snvs_pwrkey_suspend,
- imx_snvs_pwrkey_resume);
-
static struct platform_driver imx_snvs_pwrkey_driver = {
.driver = {
.name = "snvs_pwrkey",
- .pm = &imx_snvs_pwrkey_pm_ops,
.of_match_table = imx_snvs_pwrkey_ids,
},
.probe = imx_snvs_pwrkey_probe,
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.
From: Andy Shevchenko @ 2019-03-27 7:46 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki, Lukas Wunner, Federico Lorenzi, linux-input,
Linux Kernel Mailing List
In-Reply-To: <20190327014807.7472-3-ronald@innovation.ch>
On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
>
> This introduces print_hex_dump_to_cb() which contains all the hexdump
> formatting minus the actual printk() call, allowing an arbitrary print
> function to be supplied instead. And print_hex_dump() is re-implemented
> using print_hex_dump_to_cb().
>
> This allows other hex-dump logging functions to be provided which call
> printk() differently or even log the hexdump somewhere entirely
> different.
No Sign-off?
In any case, don't do it like this. smaller non-recursive printf() is
better than one big receursive call.
When it looks like an optimization, it's actually a regression.
And yes, debugfs idea is not bad.
P.S. Also check %*ph specifier.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
From: Oliver Neukum @ 2019-03-27 9:35 UTC (permalink / raw)
To: Nicolas Saenz Julienne, Jiri Kosina, Benjamin Tissoires
Cc: Terry.Junge, linux-input, linux-kernel
In-Reply-To: <20190326200331.25934-1-nsaenzjulienne@suse.de>
On Di, 2019-03-26 at 21:03 +0100, Nicolas Saenz Julienne wrote:
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser *parser, unsigned type)
> * Add a usage to the temporary parser table.
> */
>
> -static int hid_add_usage(struct hid_parser *parser, unsigned usage)
> +static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8 size)
No need to use the __u8 style inside the kernel. u8 will do.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH v3 4/4] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-03-27 9:35 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Dmitry Torokhov, Henrik Rydberg, Sergey Senozhatsky,
Steven Rostedt, Greg Kroah-Hartman, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190327014807.7472-5-ronald@innovation.ch>
On Tue, Mar 26, 2019 at 06:48:07PM -0700, Ronald Tschalär wrote:
> The keyboard and trackpad on recent MacBook's (since 8,1) and
> MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> of USB, as previously. The higher level protocol is not publicly
> documented and hence has been reverse engineered. As a consequence there
> are still a number of unknown fields and commands. However, the known
> parts have been working well and received extensive testing and use.
>
> In order for this driver to work, the proper SPI drivers need to be
> loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> reason enabling this driver in the config implies enabling the above
> drivers.
> +// SPDX-License-Identifier: GPL-2.0
According to last changes this should be GPL-2.0-only
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/efi.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/ktime.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/spi/spi.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
> +
> +#include <asm/barrier.h>
> +#include <asm-generic/unaligned.h>
generic?!
#include <asm/unaligned.h>
should work.
> +static const char *applespi_debug_facility(unsigned int log_mask)
> +{
> + switch (log_mask) {
> + case DBG_CMD_TP_INI:
> + return "Touchpad Initialization";
> + case DBG_CMD_BL:
> + return "Backlight Command";
> + case DBG_CMD_CL:
> + return "Caps-Lock Command";
> + case DBG_RD_KEYB:
> + return "Keyboard Event";
> + case DBG_RD_TPAD:
> + return "Touchpad Event";
> + case DBG_RD_UNKN:
> + return "Unknown Event";
> + case DBG_RD_IRQ:
> + return "Interrupt Request";
> + case DBG_RD_CRC:
> + return "Corrupted packet";
> + case DBG_TP_DIM:
> + return "Touchpad Dimensions";
> + default:
> + return "-Unrecognized log mask-";
I don't think '-' surroundings are needed, but this is rather minor. Up to you.
> + }
> +}
--
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