* [PATCH v10 4/4] Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:03 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 177 ++++++++++++++++++++++++++
3 files changed, 192 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cc7b88118158..c821fe3ee794 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -433,6 +433,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select REGMAP
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7ef677cf7a10..a81cb5aa21a5 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..502b143e9e05
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN 1
+#define GOODIX_BERLIN_REGISTER_WIDTH 4
+#define GOODIX_BERLIN_SPI_READ_DUMMY_LEN 3
+#define GOODIX_BERLIN_SPI_READ_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH + \
+ GOODIX_BERLIN_SPI_READ_DUMMY_LEN)
+#define GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH)
+
+#define GOODIX_BERLIN_SPI_WRITE_FLAG 0xF0
+#define GOODIX_BERLIN_SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ if (reg_size != GOODIX_BERLIN_REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = GOODIX_BERLIN_SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memset(buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + GOODIX_BERLIN_REGISTER_WIDTH,
+ 0xff, GOODIX_BERLIN_SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+ else
+ memcpy(val_buf, buf + GOODIX_BERLIN_SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return error;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - GOODIX_BERLIN_REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = GOODIX_BERLIN_SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN,
+ data + GOODIX_BERLIN_REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+
+ kfree(buf);
+ return error;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+
+ regmap_config = goodix_berlin_spi_regmap_conf;
+ regmap_config.max_raw_read = max_size - GOODIX_BERLIN_SPI_READ_PREFIX_LEN;
+ regmap_config.max_raw_write = max_size - GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, ®map_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ error = goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] HID: fix a crash in hid_debug_events_release
From: Rahul Rameshbabu @ 2023-10-23 15:03 UTC (permalink / raw)
To: Charles Yi; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20231023093500.1391443-1-be286@163.com>
Hi Charles,
On Mon, 23 Oct, 2023 17:35:00 +0800 "Charles Yi" <be286@163.com> wrote:
> hid_debug_events_release() access released memory by
> hid_device_release(). This is fixed by the patch.
>
A couple of things here. Can you add a Fixes: tag?
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
Before any v2 however, it would be nice to understand where this issue
is coming from. I am wondering if it's really a core issue or rather an
issue with a higher level device specific driver making use of the hid
subsystem. I am having a hard time seeing how this issue occurs
currently. A stack trace in a follow-up email to this one pertaining to
the crash would be helpful. If you are resolving a syzbot report, a link
to that report would suffice.
This patch doesn't make a lot of sense to me as-is because
hid_debug_events_release is about release resources related to hid debug
events (at least from my current understanding). It should not be
free-ing the underlying hid device/instance itself.
> Signed-off-by: Charles Yi <be286@163.com>
> ---
> drivers/hid/hid-core.c | 12 ++++++++++--
> drivers/hid/hid-debug.c | 3 +++
> include/linux/hid.h | 3 +++
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 8992e3c1e769..e0181218ad85 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
> * Free a device structure, all reports, and all fields.
> */
>
> -static void hid_device_release(struct device *dev)
> +void hiddev_free(struct kref *ref)
> {
> - struct hid_device *hid = to_hid_device(dev);
> + struct hid_device *hid = container_of(ref, struct hid_device, ref);
>
> hid_close_report(hid);
> kfree(hid->dev_rdesc);
> kfree(hid);
> }
>
> +static void hid_device_release(struct device *dev)
> +{
> + struct hid_device *hid = to_hid_device(dev);
> +
> + kref_put(&hid->ref, hiddev_free);
> +}
> +
> /*
> * Fetch a report description item from the data stream. We support long
> * items, though they are not used yet.
> @@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
> spin_lock_init(&hdev->debug_list_lock);
> sema_init(&hdev->driver_input_lock, 1);
> mutex_init(&hdev->ll_open_lock);
> + kref_init(&hdev->ref);
>
> hid_bpf_device_init(hdev);
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index e7ef1ea107c9..7dd83ec74f8a 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
> goto out;
> }
> list->hdev = (struct hid_device *) inode->i_private;
> + kref_get(&list->hdev->ref);
> file->private_data = list;
> mutex_init(&list->read_mutex);
>
> @@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
> list_del(&list->node);
> spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
> kfifo_free(&list->hid_debug_fifo);
> +
> + kref_put(&list->hdev->ref, hiddev_free);
> kfree(list);
>
> return 0;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 964ca1f15e3f..3b08a2957229 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
> struct list_head debug_list;
> spinlock_t debug_list_lock;
> wait_queue_head_t debug_wait;
> + struct kref ref;
>
> unsigned int id; /* system unique id */
>
> @@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
> #endif /* CONFIG_BPF */
> };
>
> +void hiddev_free(struct kref *ref);
> +
> #define to_hid_device(pdev) \
> container_of(pdev, struct hid_device, dev)
--
Thank you for the patch,
Rahul Rameshbabu
^ permalink raw reply
* [PATCH v10 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:03 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs.
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 3 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
drivers/input/touchscreen/goodix_berlin_core.c | 594 +++++++++++++++++++++++++
4 files changed, 757 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..950da599ae1a 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -416,6 +416,9 @@ config TOUCHSCREEN_GOODIX
To compile this driver as a module, choose M here: the
module will be called goodix.
+config TOUCHSCREEN_GOODIX_BERLIN_CORE
+ tristate
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..2e2f3e70cd2c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
new file mode 100644
index 000000000000..235f44947a28
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_berlin_berlin driver.
+ */
+
+#ifndef __GOODIX_BERLIN_H_
+#define __GOODIX_BERLIN_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+
+#define GOODIX_BERLIN_MAX_TOUCH 10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
+
+#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
+#define GOODIX_BERLIN_STATUS_OFFSET 0
+#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
+
+#define GOODIX_BERLIN_BYTES_PER_POINT 8
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
+
+/* Read n finger events */
+#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
+ (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
+ GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+
+#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+
+struct goodix_berlin_fw_version {
+ u8 rom_pid[6];
+ u8 rom_vid[3];
+ u8 rom_vid_reserved;
+ u8 patch_pid[8];
+ u8 patch_vid[4];
+ u8 patch_vid_reserved;
+ u8 sensor_id;
+ u8 reserved[2];
+ __le16 checksum;
+} __packed;
+
+struct goodix_berlin_ic_info_version {
+ u8 info_customer_id;
+ u8 info_version_id;
+ u8 ic_die_id;
+ u8 ic_version_id;
+ __le32 config_id;
+ u8 config_version;
+ u8 frame_data_customer_id;
+ u8 frame_data_version_id;
+ u8 touch_data_customer_id;
+ u8 touch_data_version_id;
+ u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+ __le16 freqhop_feature;
+ __le16 calibration_feature;
+ __le16 gesture_feature;
+ __le16 side_touch_feature;
+ __le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+ __le32 cmd_addr;
+ __le16 cmd_max_len;
+ __le32 cmd_reply_addr;
+ __le16 cmd_reply_len;
+ __le32 fw_state_addr;
+ __le16 fw_state_len;
+ __le32 fw_buffer_addr;
+ __le16 fw_buffer_max_len;
+ __le32 frame_data_addr;
+ __le16 frame_data_head_len;
+ __le16 fw_attr_len;
+ __le16 fw_log_len;
+ u8 pack_max_num;
+ u8 pack_compress_version;
+ __le16 stylus_struct_len;
+ __le16 mutual_struct_len;
+ __le16 self_struct_len;
+ __le16 noise_struct_len;
+ __le32 touch_data_addr;
+ __le16 touch_data_head_len;
+ __le16 point_struct_len;
+ __le16 reserved1;
+ __le16 reserved2;
+ __le32 mutual_rawdata_addr;
+ __le32 mutual_diffdata_addr;
+ __le32 mutual_refdata_addr;
+ __le32 self_rawdata_addr;
+ __le32 self_diffdata_addr;
+ __le32 self_refdata_addr;
+ __le32 iq_rawdata_addr;
+ __le32 iq_refdata_addr;
+ __le32 im_rawdata_addr;
+ __le16 im_readata_len;
+ __le32 noise_rawdata_addr;
+ __le16 noise_rawdata_len;
+ __le32 stylus_rawdata_addr;
+ __le16 stylus_rawdata_len;
+ __le32 noise_data_addr;
+ __le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch_data {
+ u8 id;
+ u8 unused;
+ __le16 x;
+ __le16 y;
+ __le16 w;
+} __packed;
+
+struct goodix_berlin_core {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *avdd;
+ struct regulator *iovdd;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties props;
+ struct goodix_berlin_fw_version fw_version;
+ struct input_dev *input_dev;
+ int irq;
+
+ /* Runtime parameters extracted from IC_INFO buffer */
+ u32 touch_data_addr;
+};
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap);
+
+extern const struct dev_pm_ops goodix_berlin_pm_ops;
+
+#endif
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
new file mode 100644
index 000000000000..a15f628de2f8
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -0,0 +1,594 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/bitfield.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+/*
+ * Goodix "Berlin" Touchscreen ID driver
+ *
+ * This driver is distinct from goodix.c since hardware interface
+ * is different enough to require a new driver.
+ * None of the register address or data structure are close enough
+ * to the previous generations.
+ *
+ * Currently only handles Multitouch events with already
+ * programmed firmware and "config" for "Revision D" Berlin IC.
+ *
+ * Support is missing for:
+ * - ESD Management
+ * - Firmware update/flashing
+ * - "Config" update/flashing
+ * - Stylus Events
+ * - Gesture Events
+ * - Support for older revisions (A & B)
+ */
+
+static bool goodix_berlin_checksum_valid(const u8 *data, int size)
+{
+ u32 cal_checksum = 0;
+ u16 r_checksum;
+ u32 i;
+
+ if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+ return false;
+
+ for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+ cal_checksum += data[i];
+
+ r_checksum = get_unaligned_le16(&data[i]);
+
+ return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
+}
+
+static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
+ const u8 *data, int size)
+{
+ int i;
+
+ /*
+ * If the device is missing or doesn't respond the buffer
+ * could be filled with bus default line state, 0x00 or 0xff,
+ * so declare success the first time we encounter neither.
+ */
+ for (i = 0; i < size; i++)
+ if (data[i] > 0 && data[i] < 0xff)
+ return false;
+
+ return true;
+}
+
+static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
+{
+ u8 tx_buf[8], rx_buf[8];
+ int retry = 3;
+ int error;
+
+ memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
+ while (retry--) {
+ error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
+ sizeof(tx_buf));
+ if (error)
+ return error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
+ sizeof(rx_buf));
+ if (error)
+ return error;
+
+ if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
+ return 0;
+
+ usleep_range(5000, 5100);
+ }
+
+ dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
+
+ return -EINVAL;
+}
+
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
+{
+ int error = 0;
+
+ if (on) {
+ error = regulator_enable(cd->iovdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+ return error;
+ }
+
+ /* Vendor waits 3ms for IOVDD to settle */
+ usleep_range(3000, 3100);
+
+ error = regulator_enable(cd->avdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+ goto err_iovdd_disable;
+ }
+
+ /* Vendor waits 15ms for IOVDD to settle */
+ usleep_range(15000, 15100);
+
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ /* Vendor waits 4ms for Firmware to initialize */
+ usleep_range(4000, 4100);
+
+ error = goodix_berlin_dev_confirm(cd);
+ if (error)
+ goto err_dev_reset;
+
+ /* Vendor waits 100ms for Firmware to fully boot */
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+ }
+
+err_dev_reset:
+ gpiod_set_value(cd->reset_gpio, 1);
+
+ regulator_disable(cd->avdd);
+
+err_iovdd_disable:
+ regulator_disable(cd->iovdd);
+
+ return error;
+}
+
+static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
+{
+ u8 buf[sizeof(struct goodix_berlin_fw_version)];
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
+ if (error) {
+ dev_err(cd->dev, "error reading fw version, %d\n", error);
+ return error;
+ }
+
+ if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+ dev_err(cd->dev, "invalid fw version: checksum error\n");
+ return -EINVAL;
+ }
+
+ memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
+
+ return 0;
+}
+
+/* Only extract necessary data for runtime */
+static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
+{
+ struct goodix_berlin_ic_info_misc misc;
+ unsigned int offset = 0;
+ u8 param_num;
+
+ offset += sizeof(__le16); /* length */
+ offset += sizeof(struct goodix_berlin_ic_info_version);
+ offset += sizeof(struct goodix_berlin_ic_info_feature);
+
+ /* IC_INFO Parameters, variable width structure */
+ offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* active_scan_rate_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* mutual_freq_num*/
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_tx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_rx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* stylus_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset + sizeof(misc) > length)
+ goto invalid_offset;
+
+ /* goodix_berlin_ic_info_misc */
+ memcpy(&misc, &data[offset], sizeof(misc));
+
+ cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+
+ return 0;
+
+invalid_offset:
+ dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
+ offset, length);
+ return -EINVAL;
+}
+
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ __le16 length_raw;
+ u8 *afe_data;
+ u16 length;
+ int error;
+
+ afe_data = kzalloc(GOODIX_BERLIN_IC_INFO_MAX_LEN, GFP_KERNEL);
+ if (!afe_data)
+ return -ENOMEM;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_info(cd->dev, "failed get ic info length, %d\n", error);
+ goto free_afe_data;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_info(cd->dev, "invalid ic info length %d\n", length);
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_info(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ goto free_afe_data;
+ }
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ if (!goodix_berlin_checksum_valid(afe_data, length)) {
+ dev_info(cd->dev, "fw info checksum error\n");
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ if (error)
+ goto free_afe_data;
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ error = -EINVAL;
+ goto free_afe_data;
+ }
+
+ return 0;
+
+free_afe_data:
+ kfree(afe_data);
+
+ return error;
+}
+
+static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
+ const void *buf, int touch_num)
+{
+ const struct goodix_berlin_touch_data *touch_data = buf;
+ int i;
+
+ /* Report finger touches */
+ for (i = 0; i < touch_num; i++) {
+ unsigned int id;
+
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
+
+ if (id >= GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn_ratelimited(cd->dev, "invalid finger id %d\n", id);
+ continue;
+ }
+
+ input_mt_slot(cd->input_dev, id);
+ input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
+
+ touchscreen_report_pos(cd->input_dev, &cd->props,
+ __le16_to_cpu(touch_data[i].x),
+ __le16_to_cpu(touch_data[i].y),
+ true);
+ input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
+ __le16_to_cpu(touch_data[i].w));
+ }
+
+ input_mt_sync_frame(cd->input_dev);
+ input_sync(cd->input_dev);
+}
+
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
+ const void *pre_buf, u32 pre_buf_len)
+{
+ u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
+ u8 point_type, touch_num;
+ int error;
+
+ /* copy pre-data to buffer */
+ memcpy(buffer, pre_buf, pre_buf_len);
+
+ touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
+ buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+
+ if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
+ return;
+ }
+
+ if (touch_num) {
+ /* read more data if more than 2 touch events */
+ if (unlikely(touch_num > 2)) {
+ error = regmap_raw_read(cd->regmap,
+ cd->touch_data_addr + pre_buf_len,
+ &buffer[pre_buf_len],
+ (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+ error);
+ return;
+ }
+ }
+
+ point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
+ buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+
+ if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ return;
+ }
+
+ if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+ dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
+ &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+ return;
+ }
+ }
+
+ goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num);
+}
+
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ return 0;
+}
+
+static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
+{
+ struct goodix_berlin_core *cd = data;
+ u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
+ u8 event_status;
+ int error;
+
+ /* First, read buffer with space for 2 touch events */
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
+ GOODIX_BERLIN_IRQ_READ_LEN(2));
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
+ return IRQ_HANDLED;
+ }
+
+ if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
+ return IRQ_HANDLED;
+
+ if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+ dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
+ GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
+ return IRQ_HANDLED;
+ }
+
+ event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
+
+ if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
+
+ if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+ case GOODIX_BERLIN_REQUEST_CODE_RESET:
+ if (cd->reset_gpio)
+ goodix_berlin_request_handle_reset(cd);
+ break;
+
+ default:
+ dev_warn(cd->dev, "unsupported request code 0x%x\n",
+ buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+ }
+ }
+
+ /* Clear up status field */
+ regmap_write(cd->regmap, cd->touch_data_addr, 0);
+
+ return IRQ_HANDLED;
+}
+
+static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
+ const struct input_id *id)
+{
+ struct input_dev *input_dev;
+ int error;
+
+ input_dev = devm_input_allocate_device(cd->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ cd->input_dev = input_dev;
+ input_set_drvdata(input_dev, cd);
+
+ input_dev->name = "Goodix Berlin Capacitive TouchScreen";
+ input_dev->phys = "input/ts";
+
+ input_dev->id = *id;
+
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(cd->input_dev, true, &cd->props);
+
+ error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return error;
+
+ error = input_register_device(cd->input_dev);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int goodix_berlin_pm_suspend(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+
+ disable_irq(cd->irq);
+
+ return goodix_berlin_power_on(cd, false);
+}
+
+static int goodix_berlin_pm_resume(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+ int error;
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error)
+ return error;
+
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
+ goodix_berlin_pm_suspend,
+ goodix_berlin_pm_resume);
+
+static void goodix_berlin_power_off(void *data)
+{
+ struct goodix_berlin_core *cd = data;
+
+ goodix_berlin_power_on(cd, false);
+}
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap)
+{
+ struct goodix_berlin_core *cd;
+ int error;
+
+ if (irq <= 0) {
+ dev_err(dev, "Missing interrupt number\n");
+ return -EINVAL;
+ }
+
+ cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+ if (!cd)
+ return -ENOMEM;
+
+ cd->dev = dev;
+ cd->regmap = regmap;
+ cd->irq = irq;
+
+ cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cd->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
+ "Failed to request reset gpio\n");
+
+ cd->avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(cd->avdd))
+ return dev_err_probe(dev, PTR_ERR(cd->avdd),
+ "Failed to request avdd regulator\n");
+
+ cd->iovdd = devm_regulator_get(dev, "iovdd");
+ if (IS_ERR(cd->iovdd))
+ return dev_err_probe(dev, PTR_ERR(cd->iovdd),
+ "Failed to request iovdd regulator\n");
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error) {
+ dev_err(dev, "failed power on");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
+ if (error)
+ return error;
+
+ error = goodix_berlin_read_version(cd);
+ if (error) {
+ dev_err(dev, "failed to get version info");
+ return error;
+ }
+
+ error = goodix_berlin_get_ic_info(cd);
+ if (error) {
+ dev_err(dev, "invalid ic info, abort");
+ return error;
+ }
+
+ error = goodix_berlin_input_dev_config(cd, id);
+ if (error) {
+ dev_err(dev, "failed set input device");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(dev, irq, NULL,
+ goodix_berlin_threadirq_func,
+ IRQF_ONESHOT, "goodix-berlin", cd);
+ if (error) {
+ dev_err(dev, "request threaded irq failed: %d\n", error);
+ return error;
+ }
+
+ dev_set_drvdata(dev, cd);
+
+ dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(goodix_berlin_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v10 0/4] Input: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:03 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong, Rob Herring
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v10:
- Fix according to Dmitry's review:
- move goodix_berlin_get_ic_info() afe_data to heap
- merge the goodix_berlin_parse_finger() loops and skip invalid fingers instead of returning
- remove unwanted goodix_berlin_touch_handler() "static" for buffer
- only call goodix_berlin_request_handle_reset() if gpio was provided
- use "error = func(); if(error) return error;" instead of "return func()" when function handles multiple error cases
- Link to v9: https://lore.kernel.org/r/20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org
Changes in v9:
- Rebased on next-20231020
- Link to v8: https://lore.kernel.org/r/20231003-topic-goodix-berlin-upstream-initial-v8-0-171606102ed6@linaro.org
Changes in v8:
- Add missing bitfield.h include in core
- Link to v7: https://lore.kernel.org/r/20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org
Changes in v7:
- rebased on v6.6-rc3
- Link to v6: https://lore.kernel.org/r/20230912-topic-goodix-berlin-upstream-initial-v6-0-b4ecfa49fb9d@linaro.org
Changes in v6:
- rebased on v6.6-rc1
- changed commit message prefix to match the other Input commits
- Link to v5: https://lore.kernel.org/r/20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org
Changes in v5:
- rebased on next-20230801
- Link to v4: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@linaro.org
Changes in v4:
- Core updates:
- drop kconfig depends, deps will be handled by _SPI and _I2C
- change power_on() error labels
- print errors on all dev_err() prints
- remove useless default variable initialization
- switch irq touch checksum error to dev_err()
- add Jeff's review tag
- I2C changes
- change REGMAP_I2C Kconfig from depends to select
- add Jeff's review tag
- SPI changes
- add select REGMAP to Kconfig
- added GOODIX_BERLIN_ prefix to defines
- switched from ret to error
- add Jeff's review tag
- Link to v3: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org
Changes in v3:
- Another guge cleanups after Jeff's review:
- appended goodix_berlin_ before all defines
- removed some unused defines
- removed retries on most of read functions, can be added back later
- added __le to ic_info structures
- reworked and simplified irq handling, dropped enum and ts_event structs
- added struct for touch data
- simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
- moved touch_data_addr to the end of the main code_data
- reworked probe to get_irq last and right before setip input device
- cleaned probe by removing the "cd->dev"
- added short paragraph to justify new driver for berlin devices
- defined all offsets & masks
- Added bindings review tag
- Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
Changes in v2:
- Huge cleanups after Jeff's review:
- switch to error instead of ret
- drop dummy vendor/product ids
- drop unused defined/enums
- drop unused ic_info and only keep needes values
- cleanup namings and use goodix_berlin_ everywhere
- fix regulator setup
- fix default variables value when assigned afterwars
- removed indirections
- dropped debugfs
- cleaned input_dev setup
- dropped _remove()
- sync'ed i2c and spi drivers
- fixed yaml bindings
- Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
---
Neil Armstrong (4):
dt-bindings: input: document Goodix Berlin Touchscreen IC
Input: add core support for Goodix Berlin Touchscreen IC
Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++
drivers/input/touchscreen/Kconfig | 31 ++
drivers/input/touchscreen/Makefile | 3 +
drivers/input/touchscreen/goodix_berlin.h | 159 ++++++
drivers/input/touchscreen/goodix_berlin_core.c | 594 +++++++++++++++++++++
drivers/input/touchscreen/goodix_berlin_i2c.c | 74 +++
drivers/input/touchscreen/goodix_berlin_spi.c | 177 ++++++
7 files changed, 1133 insertions(+)
---
base-commit: 2030579113a1b1b5bfd7ff24c0852847836d8fd1
change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply
* [PATCH v10 1/4] dt-bindings: input: document Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:03 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong, Rob Herring
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org>
Document the Goodix GT9916 wich is part of the "Berlin" serie
of Touchscreen controllers IC from Goodix.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
new file mode 100644
index 000000000000..d90f045ac06c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/goodix,gt9916.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Goodix Berlin series touchscreen controller
+
+description: The Goodix Berlin series of touchscreen controllers
+ be connected to either I2C or SPI buses.
+
+maintainers:
+ - Neil Armstrong <neil.armstrong@linaro.org>
+
+allOf:
+ - $ref: touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - goodix,gt9916
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description: Analog power supply regulator on AVDD pin
+
+ vddio-supply:
+ description: power supply regulator on VDDIO pin
+
+ spi-max-frequency: true
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - avdd-supply
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ touchscreen@5d {
+ compatible = "goodix,gt9916";
+ reg = <0x5d>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ num-cs = <1>;
+ cs-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ touchscreen@0 {
+ compatible = "goodix,gt9916";
+ reg = <0>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ spi-max-frequency = <1000000>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+
+...
--
2.34.1
^ permalink raw reply related
* [PATCH v10 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:03 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 +++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 74 +++++++++++++++++++++++++++
3 files changed, 89 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 950da599ae1a..cc7b88118158 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -419,6 +419,20 @@ config TOUCHSCREEN_GOODIX
config TOUCHSCREEN_GOODIX_BERLIN_CORE
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ select REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 2e2f3e70cd2c..7ef677cf7a10 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..4d5bcc101569
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+ int error;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ error = goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v9 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-23 14:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZTX4dPa3CxZacDph@google.com>
Hi Dmitry,
On 23/10/2023 06:37, Dmitry Torokhov wrote:
> Hi Neil,
>
> On Sat, Oct 21, 2023 at 01:09:24PM +0200, Neil Armstrong wrote:
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
>> +{
>> + u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
>
> You probably already saw the kernel test robot message, I think we
> should allocate this buffer in the heap (and free it once its no longer
> needed).
Indeed I haven't received it on 9 patch submitting, anyway moved it to head.
>
>> + __le16 length_raw;
>> + u16 length;
>> + int error;
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + &length_raw, sizeof(length_raw));
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
>> + return error;
>> + }
>> +
>> + length = le16_to_cpu(length_raw);
>> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d\n", length);
>> + return -EINVAL;
>> + }
>> +
>> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
>> + afe_data, length);
>> + if (error) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
>> + return error;
>> + }
>> +
>> + /* check whether the data is valid (ex. bus default values) */
>> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
>
> This cast is not needed.
>
>> + dev_err(cd->dev, "fw info data invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
>
> This cast is not needed either.
>
>> + dev_info(cd->dev, "fw info checksum error\n");
>> + return -EINVAL;
>> + }
>> +
>> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
>> + if (error)
>> + return error;
>> +
>> + /* check some key info */
>> + if (!cd->touch_data_addr) {
>> + dev_err(cd->dev, "touch_data_addr is null\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
>> + const void *buf, int touch_num)
>> +{
>> + const struct goodix_berlin_touch_data *touch_data = buf;
>> + int i;
>> +
>> + /* Check for data validity */
>> + for (i = 0; i < touch_num; i++) {
>> + unsigned int id;
>> +
>> + id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
>> +
>> + if (id >= GOODIX_BERLIN_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid finger id %d\n", id);
>> + return;
>
> Is it important to abort entire packet if one if the slots has incorrect
> data? Can we simply skip over these contacts?
Indeed, merged the for loops and simply skip the invalid finger id.
>
>> + }
>> + }
>> +
>> + /* Report finger touches */
>> + for (i = 0; i < touch_num; i++) {
>> + input_mt_slot(cd->input_dev,
>> + FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
>> + touch_data[i].id));
>> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
>> +
>> + touchscreen_report_pos(cd->input_dev, &cd->props,
>> + __le16_to_cpu(touch_data[i].x),
>> + __le16_to_cpu(touch_data[i].y),
>> + true);
>> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
>> + __le16_to_cpu(touch_data[i].w));
>> + }
>> +
>> + input_mt_sync_frame(cd->input_dev);
>> + input_sync(cd->input_dev);
>> +}
>> +
>> +static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
>> + const void *pre_buf, u32 pre_buf_len)
>> +{
>> + static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
>
> No, please no static data. The drivers should be ready to handle more
> than one device on a system. If the buffer is large-ish put it into
> goodix_berlin_core.
This is a typo from vendor kernel, I didn't want a static buffer here...
The buffer is only 26 bytes, it's small enough to stay here in non-static.
>
>
>> + u8 point_type, touch_num;
>> + int error;
>> +
>> + /* copy pre-data to buffer */
>> + memcpy(buffer, pre_buf, pre_buf_len);
>> +
>> + touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
>> + buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
>> +
>> + if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
>> + return;
>> + }
>> +
>> + if (touch_num) {
>> + /* read more data if more than 2 touch events */
>> + if (unlikely(touch_num > 2)) {
>> + error = regmap_raw_read(cd->regmap,
>> + cd->touch_data_addr + pre_buf_len,
>> + &buffer[pre_buf_len],
>> + (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
>> + if (error) {
>> + dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
>> + error);
>> + return;
>> + }
>> + }
>> +
>> + point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
>> + buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
>> +
>> + if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
>> + point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
>> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
>> + return;
>> + }
>> +
>> + if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
>> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
>> + dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
>> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
>> + &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
>> + return;
>> + }
>> + }
>> +
>> + goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
>> + touch_num);
>> +}
>> +
>> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
>> +{
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
>
> The reset line handling is optional, we should skip waits if there is
> no GPIO defined for the board.
Ack, instead I only call this if gpio is valid.
>
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> + u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
>> + u8 event_status;
>> + int error;
>> +
>> + /* First, read buffer with space for 2 touch events */
>> + error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
>> + GOODIX_BERLIN_IRQ_READ_LEN(2));
>> + if (error) {
>> + dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
>> + return IRQ_HANDLED;
>> +
>> + if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
>> + dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
>> + GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
>> +
>> + if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
>> + goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
>> +
>> + if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
>> + switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
>> + case GOODIX_BERLIN_REQUEST_CODE_RESET:
>> + goodix_berlin_request_handle_reset(cd);
>> + break;
>> +
>> + default:
>> + dev_warn(cd->dev, "unsupported request code 0x%x\n",
>> + buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
>> + }
>> + }
>> +
>> + /* Clear up status field */
>> + regmap_write(cd->regmap, cd->touch_data_addr, 0);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
>> + const struct input_id *id)
>> +{
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + input_dev = devm_input_allocate_device(cd->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + cd->input_dev = input_dev;
>> + input_set_drvdata(input_dev, cd);
>> +
>> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
>> + input_dev->phys = "input/ts";
>> +
>> + input_dev->id = *id;
>> +
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
>> +
>> + error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
>> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
>> + if (error)
>> + return error;
>> +
>> + return input_register_device(cd->input_dev);
>
> Please in functions with multiple possible failure paths use format
>
> error = op(...);
> if (error)
> return error;
>
> return 0;
Ack, will check for this in the whole patchset.
Thanks for review,
Neil
>
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Daniel Thompson @ 2023-10-23 13:34 UTC (permalink / raw)
To: Sean Young
Cc: Hans de Goede, Uwe Kleine-König, linux-media, linux-pwm,
Ivaylo Dimitrov, Thierry Reding, Jonathan Corbet, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Daniel Vetter, Javier Martinez Canillas, Jean Delvare,
Guenter Roeck, Support Opensource, Dmitry Torokhov, Pavel Machek,
Lee Jones, Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Jingoo Han, Helge Deller, linux-doc,
linux-kernel, intel-gfx, dri-devel, linux-hwmon, linux-input,
linux-leds, platform-driver-x86, linux-arm-kernel, linux-fbdev
In-Reply-To: <ZTT9fvEF+lqfzGJ/@gofer.mess.org>
On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> Hi Hans,
>
> On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > >> On 10/17/23 11:17, Sean Young wrote:
> > > I think it's very subjective if you consider this
> > > churn or not.
> >
> > I consider it churn because I don't think adding a postfix
> > for what is the default/expected behavior is a good idea
> > (with GPIOs not sleeping is the expected behavior).
> >
> > I agree that this is very subjective and very much goes
> > into the territory of bikeshedding. So please consider
> > the above my 2 cents on this and lets leave it at that.
>
> You have a valid point. Let's focus on having descriptive function names.
For a couple of days I've been trying to resist the bikeshedding (esp.
given the changes to backlight are tiny) so I'll try to keep it as
brief as I can:
1. I dislike the do_it() and do_it_cansleep() pairing. It is
difficult to detect when a client driver calls do_it() by mistake.
In fact a latent bug of this nature can only be detected by runtime
testing with the small number of PWMs that do not support
configuration from an atomic context.
In contrast do_it() and do_it_atomic()[*] means that although
incorrectly calling do_it() from an atomic context can be pretty
catastrophic it is also trivially detected (with any PWM driver)
simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.
No objections (beyond churn) to fully spelt out pairings such as
do_it_cansleep() and do_it_atomic()[*]!
2. If there is an API rename can we make sure the patch contains no
other changes (e.g. don't introduce any new API in the same patch).
Seperating renames makes the patches easier to review!
It makes each one smaller and easier to review!
Daniel.
[*] or do_it_nosleep()... etc.
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Hans de Goede @ 2023-10-23 13:17 UTC (permalink / raw)
To: Sean Young
Cc: Uwe Kleine-König, linux-media, linux-pwm, Ivaylo Dimitrov,
Thierry Reding, Jonathan Corbet, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
Javier Martinez Canillas, Jean Delvare, Guenter Roeck,
Support Opensource, Dmitry Torokhov, Pavel Machek, Lee Jones,
Mauro Carvalho Chehab, Ilpo Järvinen, Mark Gross,
Liam Girdwood, Mark Brown, Daniel Thompson, Jingoo Han,
Helge Deller, linux-doc, linux-kernel, intel-gfx, dri-devel,
linux-hwmon, linux-input, linux-leds, platform-driver-x86,
linux-arm-kernel, linux-fbdev
In-Reply-To: <ZTT9fvEF+lqfzGJ/@gofer.mess.org>
Hi Sean,
On 10/22/23 12:46, Sean Young wrote:
> Hi Hans,
>
> On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
>> On 10/19/23 12:51, Uwe Kleine-König wrote:
>>> On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
>>>> On 10/17/23 11:17, Sean Young wrote:
>>>>> Some drivers require sleeping, for example if the pwm device is connected
>>>>> over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
>>>>> with the generated IR signal when sleeping occurs.
>>>>>
>>>>> This patch makes it possible to use pwm when the driver does not sleep,
>>>>> by introducing the pwm_can_sleep() function.
>>>>>
>>>>> Signed-off-by: Sean Young <sean@mess.org>
>>>>
>>>> I have no objection to this patch by itself, but it seems a bit
>>>> of unnecessary churn to change all current callers of pwm_apply_state()
>>>> to a new API.
>>>
>>> The idea is to improve the semantic of the function name, see
>>> https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
>>> for more context.
>>
>> Hmm, so the argument here is that the GPIO API has this, but GPIOs
>> generally speaking can be set atomically, so there not being able
>> to set it atomically is special.
>>
>> OTOH we have many many many other kernel functions which may sleep
>> and we don't all postfix them with _can_sleep.
>>
>> And for PWM controllers pwm_apply_state is IMHO sorta expected to
>> sleep. Many of these are attached over I2C so things will sleep,
>> others have a handshake to wait for the current dutycycle to
>> end before you can apply a second change on top of an earlier
>> change during the current dutycycle which often also involves
>> sleeping.
>>
>> So the natural/expeected thing for pwm_apply_state() is to sleep
>> and thus it does not need a postfix for this IMHO.
>
> Most pwm drivers look like they can be made to work in atomic context,
> I think. Like you say this is not the case for all of them. Whatever
> we choose to be the default for pwm_apply_state(), we should have a
> clear function name for the alternative. This is essentially why
> pam_apply_cansleep() was picked.
>
> The alternative to pwm_apply_cansleep() is to have a function name
> which implies it can be used from atomic context. However,
> pwm_apply_atomic() is not great because the "atomic" could be
> confused with the PWM atomic API, not the kernel process/atomic
> context.
Well pwm_apply_state() is the atomic PWM interface right?
So to me pwm_apply_state_atomic() would be clearly about
running atomically.
> So what should the non-sleeping function be called then?
> - pwm_apply_cannotsleep()
> - pwm_apply_nosleep()
> - pwm_apply_nonsleeping()
> - pwm_apply_atomic_context()
I would just go with:
pwm_apply_state_atomic()
but if this is disliked by others then lets just rename
pwm_apply_state() to pwm_apply_state_cansleep() as
is done in this patch and use plain pwm_apply_state()
for the new atomic-context version.
Regards,
Hans
>
>>> I think it's very subjective if you consider this
>>> churn or not.
>>
>> I consider it churn because I don't think adding a postfix
>> for what is the default/expected behavior is a good idea
>> (with GPIOs not sleeping is the expected behavior).
>>
>> I agree that this is very subjective and very much goes
>> into the territory of bikeshedding. So please consider
>> the above my 2 cents on this and lets leave it at that.
>
> You have a valid point. Let's focus on having descriptive function names.
>
>>> While it's nice to have every caller converted in a single
>>> step, I'd go for
>>>
>>> #define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
>>>
>>> , keep that macro for a while and convert all users step by step. This
>>> way we don't needlessly break oot code and the changes to convert to the
>>> new API can go via their usual trees without time pressure.
>>
>> I don't think there are enough users of pwm_apply_state() to warrant
>> such an exercise.
>>
>> So if people want to move ahead with the _can_sleep postfix addition
>> (still not a fan) here is my acked-by for the drivers/platform/x86
>> changes, for merging this through the PWM tree in a single commit:
>>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>
> Thanks,
>
> Sean
>
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-23 11:58 UTC (permalink / raw)
To: Jeff LaBundy
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thomas Weißschuh,
Shuah Khan, linux-kernel-mentees, linux-kernel, Conor Dooley,
Krzysztof Kozlowski
In-Reply-To: <ZTW0p2WG3/m1Tx+Z@nixie71>
Hello Jeff,
On 10/23/23 05:17, Jeff LaBundy wrote:
> Hi Anshul,
>
> On Tue, Oct 17, 2023 at 09:13:44AM +0530, Anshul Dalal wrote:
>> Adds bindings for the Adafruit Seesaw Gamepad.
>>
>> The gamepad functions as an i2c device with the default address of 0x50
>> and has an IRQ pin that can be enabled in the driver to allow for a rising
>> edge trigger on each button press or joystick movement.
>>
>> Product page:
>> https://www.adafruit.com/product/5743
>> Arduino driver:
>> https://github.com/adafruit/Adafruit_Seesaw
>>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>
> Perhaps this ship has sailed, but is there any reason this simple device
> cannot be added to Documentation/devicetree/bindings/trivial-devices.yaml
> as opposed to having its own binding?
>
> It has no vendor-specific properties, and the only properties are the
> standard properties already understood by the I2C core. In case I have
> misunderstood, please let me know.
>
The driver currently implements only a subset of the functionality in
the Adafruit Seesaw specification. I eventually plan on adding adding
full support for the Seesaw framework in the form of a driver for the
atsamd09 seesaw breakout board:
https://learn.adafruit.com/adafruit-seesaw-atsamd09-breakout
Then I think it would be better for this driver to use the newly exposed
seesaw APIs by the atsamd09 driver instead of relying on kernel's i2c APIs.
I would also like to add support for the provided interrupt pin later
down the line which is documented in the binding along with description
of the non-standard action button layout.
Above were my reasons for going for a standalone binding, please let me
know if you disagree.
>> ---
>>
>> Changes for v5:
>> - Added link to the datasheet
>>
>> Changes for v4:
>> - Fixed the URI for the id field
>> - Added `interrupts` property
>>
>> Changes for v3:
>> - Updated id field to reflect updated file name from previous version
>> - Added `reg` property
>>
>> Changes for v2:
>> - Renamed file to `adafruit,seesaw-gamepad.yaml`
>> - Removed quotes for `$id` and `$schema`
>> - Removed "Bindings for" from the description
>> - Changed node name to the generic name "joystick"
>> - Changed compatible to 'adafruit,seesaw-gamepad' instead of
>> 'adafruit,seesaw_gamepad'
>>
>> .../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++
>> 1 file changed, 60 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>> new file mode 100644
>> index 000000000000..3f0d1c5a3b9b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>> @@ -0,0 +1,60 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Adafruit Mini I2C Gamepad with seesaw
>> +
>> +maintainers:
>> + - Anshul Dalal <anshulusr@gmail.com>
>> +
>> +description: |
>> + Adafruit Mini I2C Gamepad
>> +
>> + +-----------------------------+
>> + | ___ |
>> + | / \ (X) |
>> + | | S | __ __ (Y) (A) |
>> + | \___/ |ST| |SE| (B) |
>> + | |
>> + +-----------------------------+
>> +
>> + S -> 10-bit percision bidirectional analog joystick
>> + ST -> Start
>> + SE -> Select
>> + X, A, B, Y -> Digital action buttons
>> +
>> + Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
>> + Product page: https://www.adafruit.com/product/5743
>> + Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
>> +
>> +properties:
>> + compatible:
>> + const: adafruit,seesaw-gamepad
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> + description:
>> + The gamepad's IRQ pin triggers a rising edge if interrupts are enabled.
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + joystick@50 {
>> + compatible = "adafruit,seesaw-gamepad";
>> + reg = <0x50>;
>> + };
>> + };
>> --
>> 2.42.0
>>
>
> Kind regards,
> Jeff LaBundy
Thank you,
Anshul Dalal
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Miguel Ojeda @ 2023-10-23 11:44 UTC (permalink / raw)
To: Jani Nikula
Cc: Pavel Machek, Lee Jones, linux-kernel, Werner Sembach,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds
In-Reply-To: <87sf61bm8t.fsf@intel.com>
On Mon, Oct 23, 2023 at 1:40 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> One could also reasonably make the argument that controlling the
> individual keyboard key backlights should be part of the input
> subsystem. It's not a display per se. (Unless you actually have small
> displays on the keycaps, and I think that's a thing too.)
>
> There's force feedback, there could be light feedback? There's also
> drivers/input/input-leds.c for the keycaps that have leds, like caps
> lock, num lock, etc.
>
> Anyway, just throwing ideas around, no strong opinions, really.
Yeah, sounds quite reasonable too, in fact it may make more sense
there given the LEDs are associated per-key rather than being an
uniform matrix in a rectangle if I understand correctly. If the input
subsystem wants to take it, that would be great.
Cheers,
Miguel
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Jani Nikula @ 2023-10-23 11:40 UTC (permalink / raw)
To: Miguel Ojeda, Pavel Machek
Cc: Lee Jones, linux-kernel, Werner Sembach,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds
In-Reply-To: <CANiq72mfP+dOLFR352O0UNVF8m8yTi_VmOY1zzQdTBjPWCRowg@mail.gmail.com>
On Mon, 16 Oct 2023, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Fri, Oct 13, 2023 at 9:56 PM Pavel Machek <pavel@ucw.cz> wrote:
>>
>> So... a bit of rationale. The keyboard does not really fit into the
>> LED subsystem; LEDs are expected to be independent ("hdd led") and not
>> a matrix of them.
>
> Makes sense.
>
>> We do see various strange displays these days -- they commonly have
>> rounded corners and holes in them. I'm not sure how that's currently
>> supported, but I believe it is reasonable to view keyboard as a
>> display with slightly weird placing of pixels.
>>
>> Plus, I'd really like to play tetris on one of those :-).
>>
>> So, would presenting them as auxdisplay be acceptable? Or are there
>> better options?
>
> It sounds like a fair use case -- auxdisplay are typically simple
> character-based or small graphical displays, e.g. 128x64, that may not
> be a "main" / usual screen as typically understood, but the concept is
> a bit fuzzy and we are a bit of a catch-all.
>
> And "keyboard backlight display with a pixel/color per-key" does not
> sound like a "main" screen, and having some cute effects displayed
> there are the kind of thing that one could do in the usual small
> graphical ones too. :)
>
> But if somebody prefers to create new categories (or subcategories
> within auxdisplay) to hold these, that could be nice too (in the
> latter case, I would perhaps suggest reorganizing all of the existing
> ones while at it).
One could also reasonably make the argument that controlling the
individual keyboard key backlights should be part of the input
subsystem. It's not a display per se. (Unless you actually have small
displays on the keycaps, and I think that's a thing too.)
There's force feedback, there could be light feedback? There's also
drivers/input/input-leds.c for the keycaps that have leds, like caps
lock, num lock, etc.
Anyway, just throwing ideas around, no strong opinions, really.
BR,
Jani.
--
Jani Nikula, Intel
^ permalink raw reply
* Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
From: Jani Nikula @ 2023-10-23 10:47 UTC (permalink / raw)
To: Sean Young, linux-media, linux-pwm, Ivaylo Dimitrov,
Thierry Reding, Uwe Kleine-König, Jonathan Corbet,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Daniel Vetter, Javier Martinez Canillas, Jean Delvare,
Guenter Roeck, Support Opensource, Dmitry Torokhov, Pavel Machek,
Lee Jones, Sean Young, Mauro Carvalho Chehab, Hans de Goede,
Ilpo Järvinen, Mark Gross, Liam Girdwood, Mark Brown,
Daniel Thompson, Jingoo Han, Helge Deller
Cc: linux-doc, linux-kernel, intel-gfx, dri-devel, linux-hwmon,
linux-input, linux-leds, platform-driver-x86, linux-arm-kernel,
linux-fbdev
In-Reply-To: <a7fcd19938d5422abc59c968ff7b3d5c275577ed.1697534024.git.sean@mess.org>
On Tue, 17 Oct 2023, Sean Young <sean@mess.org> wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 2e8f17c045222..cf516190cde8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,
> struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
>
> pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> - pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> + pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
> }
>
> static void
> @@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn
> intel_backlight_set_pwm_level(old_conn_state, level);
>
> panel->backlight.pwm_state.enabled = false;
> - pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> + pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
> }
>
> void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
> @@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
>
> pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> panel->backlight.pwm_state.enabled = true;
> - pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> + pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
> }
>
> static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
The i915 parts are
Acked-by: Jani Nikula <jani.nikula@intel.com>
for merging via whichever tree you find most convenient, and with
whatever naming you end up with.
--
Jani Nikula, Intel
^ permalink raw reply
* Re: [PATCH] Input: cyttsp5 - improve error handling and remove regmap
From: kernel test robot @ 2023-10-23 10:35 UTC (permalink / raw)
To: James Hilliard, linux-input
Cc: oe-kbuild-all, James Hilliard, Linus Walleij, Dmitry Torokhov,
linux-kernel
In-Reply-To: <20231023085234.105572-1-james.hilliard1@gmail.com>
Hi James,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus hid/for-next linus/master v6.6-rc7 next-20231023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/James-Hilliard/Input-cyttsp5-improve-error-handling-and-remove-regmap/20231023-165327
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20231023085234.105572-1-james.hilliard1%40gmail.com
patch subject: [PATCH] Input: cyttsp5 - improve error handling and remove regmap
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231023/202310231838.JHHtGKmB-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231023/202310231838.JHHtGKmB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310231838.JHHtGKmB-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/input/touchscreen/cyttsp5.c: In function 'cyttsp5_get_hid_descriptor':
>> drivers/input/touchscreen/cyttsp5.c:684:22: warning: unused variable 'reg' [-Wunused-variable]
684 | unsigned int reg = HID_DESC_REG;
| ^~~
vim +/reg +684 drivers/input/touchscreen/cyttsp5.c
677
678 static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts,
679 struct cyttsp5_hid_desc *desc)
680 {
681 struct device *dev = ts->dev;
682 u8 cmd[2] = { 0 };
683 int rc;
> 684 unsigned int reg = HID_DESC_REG;
685
686 put_unaligned_le16(HID_DESC_REG, cmd);
687
688 rc = cyttsp5_write(ts, HID_DESC_REG, cmd, 2);
689 if (rc) {
690 dev_err(dev, "Failed to get HID descriptor, rc=%d\n", rc);
691 return rc;
692 }
693
694 rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
695 msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT_MS));
696 if (rc <= 0) {
697 dev_err(ts->dev, "HID get descriptor timed out\n");
698 rc = -ETIMEDOUT;
699 return rc;
700 }
701
702 memcpy(desc, ts->response_buf, sizeof(*desc));
703
704 /* Check HID descriptor length and version */
705 if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) ||
706 le16_to_cpu(desc->bcd_version) != HID_VERSION) {
707 dev_err(dev, "Unsupported HID version\n");
708 return -ENODEV;
709 }
710
711 return 0;
712 }
713
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* [PATCH] HID: fix a crash in hid_debug_events_release
From: Charles Yi @ 2023-10-23 9:35 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Charles Yi
hid_debug_events_release() access released memory by
hid_device_release(). This is fixed by the patch.
Signed-off-by: Charles Yi <be286@163.com>
---
drivers/hid/hid-core.c | 12 ++++++++++--
drivers/hid/hid-debug.c | 3 +++
include/linux/hid.h | 3 +++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8992e3c1e769..e0181218ad85 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
* Free a device structure, all reports, and all fields.
*/
-static void hid_device_release(struct device *dev)
+void hiddev_free(struct kref *ref)
{
- struct hid_device *hid = to_hid_device(dev);
+ struct hid_device *hid = container_of(ref, struct hid_device, ref);
hid_close_report(hid);
kfree(hid->dev_rdesc);
kfree(hid);
}
+static void hid_device_release(struct device *dev)
+{
+ struct hid_device *hid = to_hid_device(dev);
+
+ kref_put(&hid->ref, hiddev_free);
+}
+
/*
* Fetch a report description item from the data stream. We support long
* items, though they are not used yet.
@@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
spin_lock_init(&hdev->debug_list_lock);
sema_init(&hdev->driver_input_lock, 1);
mutex_init(&hdev->ll_open_lock);
+ kref_init(&hdev->ref);
hid_bpf_device_init(hdev);
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index e7ef1ea107c9..7dd83ec74f8a 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
goto out;
}
list->hdev = (struct hid_device *) inode->i_private;
+ kref_get(&list->hdev->ref);
file->private_data = list;
mutex_init(&list->read_mutex);
@@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
list_del(&list->node);
spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
kfifo_free(&list->hid_debug_fifo);
+
+ kref_put(&list->hdev->ref, hiddev_free);
kfree(list);
return 0;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 964ca1f15e3f..3b08a2957229 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
struct list_head debug_list;
spinlock_t debug_list_lock;
wait_queue_head_t debug_wait;
+ struct kref ref;
unsigned int id; /* system unique id */
@@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
#endif /* CONFIG_BPF */
};
+void hiddev_free(struct kref *ref);
+
#define to_hid_device(pdev) \
container_of(pdev, struct hid_device, dev)
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: Lee Jones @ 2023-10-23 9:20 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <E3224624-7FF4-48F6-BA53-08312B69EF9F@cirrus.com>
On Fri, 20 Oct 2023, James Ogletree wrote:
>
> Thank you for your thorough review. Anything not replied to below will be
> incorporated in the next version.
>
> >> +/*
> >> + * CS40L50 Advanced Haptic Driver with waveform memory,
> >
> > s/Driver/device/
>
> CS40L50 is a “haptic driver”, like a "motor driver" in a car. It is an
> unfortunate name in this context, but it is straight from the datasheet.
Understood. That's fine then.
> >> +static const struct mfd_cell cs40l50_devs[] = {
> >> + {
> >> + .name = "cs40l50-vibra",
> >> + },
> >
> >
> > Where are the other devices? Without them, it's not an MFD.
>
> The driver will need to support I2S streaming to the device at some point
> in the future, for which a codec driver will be added. I thought it better to
> submit this as an MFD driver now, rather than as an Input driver, so as
> not to have to move everything later.
>
> Should I add the “cs40l50-codec” mfd_cell now, even though it does not
> exist yet?
What is your timeline for this to be authored?
Does the device function well without it?
> >> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> >> +{
> >> + int error, fractional, integer, stored;
> >
> > err or ret is traditional.
>
> We received feedback to change from “ret” to “error” in the input
> subsystem, and now the opposite in MFD. I have no problem adopting
> “err” here, but is it understood that styles will be mixed across
> components?
That surprises me:
% git grep "int .*error" | wc -l
6152
vs
% git grep "int .*err" | grep -v error | wc -l
34753
% git grep "int .*ret" | wc -l
76584
> >> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
> >> +{
> >> + struct cs40l50_private *cs40l50 = data;
> >> + int error = 0;
> >> + u32 val;
> >> +
> >> + mutex_lock(&cs40l50->lock);
> >> +
> >> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> >> + switch (val) {
> >> + case 0:
> >> + mutex_unlock(&cs40l50->lock);
> >> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
> >> + return IRQ_HANDLED;
> >> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
> >> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> >
> > These all appear to be no-ops?
>
> Correct.
Then why do the exist?
> >> + case CS40L50_MBOX_RUNTIME_SHORT:
> >> + dev_err(cs40l50->dev, "Runtime short detected\n");
> >> + error = cs40l50_error_release(cs40l50);
> >> + if (error)
> >> + goto out_mutex;
> >> + break;
> >> + default:
> >> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
> >> + error = -EINVAL;
> >> + goto out_mutex;
> >> + }
> >> + }
> >> +
> >> + error = -EIO;
> >> +
> >> +out_mutex:
> >> + mutex_unlock(&cs40l50->lock);
> >> +
> >> + return IRQ_RETVAL(!error);
> >> +}
> >
> > Should the last two drivers live in drivers/mailbox?
>
> Adopting the mailbox framework seems like an excessive amount
> of overhead for our requirements.
MFD isn't a dumping a ground for miscellaneous functionality.
MFD requests resources and registers devices.
Mailbox functionality should live in drivers/mailbox.
> >> +static irqreturn_t cs40l50_error(int irq, void *data);
> >
> > Why is this being forward declared?
> >
> >> +static const struct cs40l50_irq cs40l50_irqs[] = {
> >> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
> >
> > I assume that last parameter is half of a function name.
> >
> > Better to have 2 different structures and do 2 requests I feel.
>
> I think I will combine the two handler functions into one, so as not
> to need the struct handler parameter, or the forward declaration.
Or the MACRO - win, win win.
> >> +{
> >> + struct device *dev = cs40l50->dev;
> >> + int error;
> >> +
> >> + mutex_init(&cs40l50->lock);
> >
> > Don't you need to destroy this in the error path?
>
> My understanding based on past feedback is that mutex_destroy()
> is an empty function unless mutex debugging is enabled, and there
> is no need cleanup the mutex explicitly. I will change this if you
> disagree with that feedback.
It just seems odd to create something and not tear it down.
> >> +struct cs40l50_irq {
> >> + const char *name;
> >> + int irq;
> >> + irqreturn_t (*handler)(int irq, void *data);
> >> +};
> >> +
> >> +struct cs40l50_private {
> >> + struct device *dev;
> >> + struct regmap *regmap;
> >> + struct cs_dsp dsp;
> >> + struct mutex lock;
> >> + struct gpio_desc *reset_gpio;
> >> + struct regmap_irq_chip_data *irq_data;
> >> + struct input_dev *input;
> >
> > Where is this used?
> >
> >> + const struct firmware *wmfw;
> >
> > Or this.
> >
> >> + struct cs_hap haptics;
> >
> > Or this?
> >
> >> + u32 devid;
> >> + u32 revid;
> >
> > Are these used after they're set?
>
> These are all used in the input driver, patch 4/4 of this series. If
> this is not acceptable in some way, I will change it per your
> suggestions.
Do they need to be shared with other devices?
If not, they should live where they are used.
--
Lee Jones [李琼斯]
^ permalink raw reply
* [PATCH] Input: cyttsp5 - improve error handling and remove regmap
From: James Hilliard @ 2023-10-23 8:52 UTC (permalink / raw)
To: linux-input; +Cc: James Hilliard, Linus Walleij, Dmitry Torokhov, linux-kernel
The vendor cyttsp5 driver does not use regmap for i2c support, it
would appear this is due to regmap not providing sufficient levels
of control to handle various error conditions that may be present
under some configuration/firmware variants.
To improve reliability lets refactor the cyttsp5 i2c interface to
function more like the vendor driver and implement some of the error
handling retry/recovery techniques present there.
As part of this rather than assuming the device is in bootloader mode
we should first check that the device is in bootloader and only
attempt to launch the app if it actually is in the bootloader.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/input/touchscreen/cyttsp5.c | 260 ++++++++++++++++++----------
1 file changed, 170 insertions(+), 90 deletions(-)
diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
index db5a885ecd72..334f535dc131 100644
--- a/drivers/input/touchscreen/cyttsp5.c
+++ b/drivers/input/touchscreen/cyttsp5.c
@@ -18,17 +18,18 @@
#include <linux/input/touchscreen.h>
#include <linux/interrupt.h>
#include <linux/i2c.h>
-#include <linux/mod_devicetable.h>
#include <linux/module.h>
-#include <linux/regmap.h>
+#include <linux/of_device.h>
#include <asm/unaligned.h>
#define CYTTSP5_NAME "cyttsp5"
#define CY_I2C_DATA_SIZE (2 * 256)
#define HID_VERSION 0x0100
#define CY_MAX_INPUT 512
+#define CY_PIP_1P7_EMPTY_BUF 0xFF00
#define CYTTSP5_PREALLOCATED_CMD_BUFFER 32
#define CY_BITS_PER_BTN 1
+#define CY_CORE_STARTUP_RETRY_COUNT 10
#define CY_NUM_BTN_EVENT_ID GENMASK(CY_BITS_PER_BTN - 1, 0)
#define MAX_AREA 255
@@ -67,6 +68,7 @@
#define HID_BTN_REPORT_ID 0x3
#define HID_APP_RESPONSE_REPORT_ID 0x1F
#define HID_APP_OUTPUT_REPORT_ID 0x2F
+#define HID_BL_REPORT_ID 0xFF
#define HID_BL_RESPONSE_REPORT_ID 0x30
#define HID_BL_OUTPUT_REPORT_ID 0x40
#define HID_RESPONSE_REPORT_ID 0xF0
@@ -205,7 +207,6 @@ struct cyttsp5 {
struct input_dev *input;
char phys[NAME_MAX];
int num_prv_rec;
- struct regmap *regmap;
struct touchscreen_properties prop;
struct regulator *vdd;
};
@@ -218,49 +219,65 @@ struct cyttsp5 {
*/
static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max)
{
- int error;
+ struct i2c_client *client = to_i2c_client(ts->dev);
+ struct i2c_msg msgs[2];
+ u8 msg_count = 1;
+ int rc;
u32 size;
- u8 temp[2];
- /* Read the frame to retrieve the size */
- error = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, sizeof(temp));
- if (error)
- return error;
+ if (!buf)
+ return -EINVAL;
- size = get_unaligned_le16(temp);
- if (!size || size == 2)
+ msgs[0].addr = client->addr;
+ msgs[0].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
+ msgs[0].len = 2;
+ msgs[0].buf = buf;
+ rc = i2c_transfer(client->adapter, msgs, msg_count);
+ if (rc < 0 || rc != msg_count)
+ return (rc < 0) ? rc : -EIO;
+
+ size = get_unaligned_le16(&buf[0]);
+ /*
+ * Before PIP 1.7, empty buffer is 0x0002
+ * From PIP 1.7, empty buffer is 0xFFXX
+ */
+ if (!size || size == 2 || size >= CY_PIP_1P7_EMPTY_BUF)
return 0;
if (size > max)
return -EINVAL;
- /* Get the real value */
- return regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, size);
+ rc = i2c_master_recv(client, buf, size);
+
+ return (rc < 0) ? rc : rc != (int)size ? -EIO : 0;
}
static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data,
size_t size)
{
- u8 cmd[HID_OUTPUT_MAX_CMD_SIZE];
+ u8 cmd[HID_OUTPUT_MAX_CMD_SIZE + 2];
+ struct i2c_client *client = to_i2c_client(ts->dev);
+ struct i2c_msg msgs[2];
+ u8 msg_count = 1;
+ int rc;
- if (size + 1 > HID_OUTPUT_MAX_CMD_SIZE)
+ if (size > HID_OUTPUT_MAX_CMD_SIZE + 2)
return -E2BIG;
- /* High bytes of register address needed as first byte of cmd */
- cmd[0] = (reg >> 8) & 0xFF;
-
/* Copy the rest of the data */
if (data)
- memcpy(&cmd[1], data, size);
+ memcpy(&cmd[0], data, size);
- /*
- * The hardware wants to receive a frame with the address register
- * contained in the first two bytes. As the regmap_write function
- * add the register adresse in the frame, we use the low byte as
- * first frame byte for the address register and the first
- * data byte is the high register + left of the cmd to send
- */
- return regmap_bulk_write(ts->regmap, reg & 0xFF, cmd, size + 1);
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags & I2C_M_TEN;
+ msgs[0].len = size;
+ msgs[0].buf = cmd;
+ rc = i2c_transfer(client->adapter, msgs, msg_count);
+
+ if (rc < 0 || rc != msg_count)
+ return (rc < 0) ? rc : -EIO;
+
+ return 0;
}
static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data,
@@ -535,22 +552,29 @@ static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
scd->len_x = get_unaligned_le16(&scd_dev->len_x);
scd->len_y = get_unaligned_le16(&scd_dev->len_y);
+ if (scd_dev->max_num_of_tch_per_refresh_cycle == 0)
+ return -EINVAL;
+
return 0;
}
static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
{
int rc;
- u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE];
+ u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE + 2];
+
+ /* Set Output register */
+ memcpy(&cmd[0], &ts->hid_desc.output_register,
+ sizeof(ts->hid_desc.output_register));
/* HI bytes of Output register address */
- put_unaligned_le16(HID_OUTPUT_GET_SYSINFO_SIZE, cmd);
- cmd[2] = HID_APP_OUTPUT_REPORT_ID;
- cmd[3] = 0x0; /* Reserved */
- cmd[4] = HID_OUTPUT_GET_SYSINFO;
+ put_unaligned_le16(HID_OUTPUT_GET_SYSINFO_SIZE, &cmd[2]);
+ cmd[4] = HID_APP_OUTPUT_REPORT_ID;
+ cmd[5] = 0x0; /* Reserved */
+ cmd[6] = HID_OUTPUT_GET_SYSINFO;
rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
- HID_OUTPUT_GET_SYSINFO_SIZE);
+ HID_OUTPUT_GET_SYSINFO_SIZE + 2);
if (rc) {
dev_err(ts->dev, "Failed to write command %d", rc);
return rc;
@@ -559,7 +583,7 @@ static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT_MS));
if (rc <= 0) {
- dev_err(ts->dev, "HID output cmd execution timed out\n");
+ dev_err(ts->dev, "HID output get sysinfo cmd execution timed out\n");
rc = -ETIMEDOUT;
return rc;
}
@@ -610,21 +634,25 @@ static int cyttsp5_power_control(struct cyttsp5 *ts, bool on)
static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
{
int rc;
- u8 cmd[HID_OUTPUT_BL_LAUNCH_APP_SIZE];
+ u8 cmd[HID_OUTPUT_BL_LAUNCH_APP_SIZE + 2];
u16 crc;
- put_unaligned_le16(HID_OUTPUT_BL_LAUNCH_APP_SIZE, cmd);
- cmd[2] = HID_BL_OUTPUT_REPORT_ID;
- cmd[3] = 0x0; /* Reserved */
- cmd[4] = HID_OUTPUT_BL_SOP;
- cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
- put_unaligned_le16(0x00, &cmd[6]);
- crc = crc_itu_t(0xFFFF, &cmd[4], 4);
- put_unaligned_le16(crc, &cmd[8]);
- cmd[10] = HID_OUTPUT_BL_EOP;
+ /* Set Output register */
+ memcpy(&cmd[0], &ts->hid_desc.output_register,
+ sizeof(ts->hid_desc.output_register));
+
+ put_unaligned_le16(HID_OUTPUT_BL_LAUNCH_APP_SIZE, &cmd[2]);
+ cmd[4] = HID_BL_OUTPUT_REPORT_ID;
+ cmd[5] = 0x0; /* Reserved */
+ cmd[6] = HID_OUTPUT_BL_SOP;
+ cmd[7] = HID_OUTPUT_BL_LAUNCH_APP;
+ put_unaligned_le16(0x00, &cmd[8]);
+ crc = crc_itu_t(0xFFFF, &cmd[6], 4);
+ put_unaligned_le16(crc, &cmd[10]);
+ cmd[12] = HID_OUTPUT_BL_EOP;
rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
- HID_OUTPUT_BL_LAUNCH_APP_SIZE);
+ HID_OUTPUT_BL_LAUNCH_APP_SIZE + 2);
if (rc) {
dev_err(ts->dev, "Failed to write command %d", rc);
return rc;
@@ -633,7 +661,7 @@ static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT_MS));
if (rc <= 0) {
- dev_err(ts->dev, "HID output cmd execution timed out\n");
+ dev_err(ts->dev, "HID output bl launch app cmd execution timed out\n");
rc = -ETIMEDOUT;
return rc;
}
@@ -651,9 +679,13 @@ static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts,
struct cyttsp5_hid_desc *desc)
{
struct device *dev = ts->dev;
+ u8 cmd[2] = { 0 };
int rc;
+ unsigned int reg = HID_DESC_REG;
+
+ put_unaligned_le16(HID_DESC_REG, cmd);
- rc = cyttsp5_write(ts, HID_DESC_REG, NULL, 0);
+ rc = cyttsp5_write(ts, HID_DESC_REG, cmd, 2);
if (rc) {
dev_err(dev, "Failed to get HID descriptor, rc=%d\n", rc);
return rc;
@@ -708,7 +740,8 @@ static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
if (size == 0) {
/* reset */
report_id = 0;
- size = 2;
+ } else if (size == 2 || size >= CY_PIP_1P7_EMPTY_BUF) {
+ return IRQ_HANDLED;
} else {
report_id = ts->input_buf[2];
}
@@ -733,19 +766,38 @@ static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
return IRQ_HANDLED;
}
+static int cyttsp5_deassert_read(struct cyttsp5 *ts, u8 *buf, int size)
+{
+ struct i2c_client *client = to_i2c_client(ts->dev);
+ int rc;
+
+ if (!buf || !size || size > CY_I2C_DATA_SIZE)
+ return -EINVAL;
+
+ rc = i2c_master_recv(client, buf, size);
+
+ return (rc < 0) ? rc : rc != size ? -EIO : 0;
+}
+
static int cyttsp5_deassert_int(struct cyttsp5 *ts)
{
u16 size;
- u8 buf[2];
+ u8 retry = 3;
int error;
- error = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, sizeof(buf));
- if (error < 0)
- return error;
+ do {
+ error = cyttsp5_deassert_read(ts, ts->input_buf, 2);
+ if (error < 0)
+ return error;
- size = get_unaligned_le16(&buf[0]);
- if (size == 2 || size == 0)
- return 0;
+ size = get_unaligned_le16(&ts->input_buf[0]);
+ if (size == 2 || size == 0 || size >= CY_PIP_1P7_EMPTY_BUF)
+ return 0;
+
+ error = cyttsp5_deassert_read(ts, ts->input_buf, size);
+ if (error < 0)
+ return error;
+ } while (retry--);
return -EINVAL;
}
@@ -774,39 +826,65 @@ static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
static int cyttsp5_startup(struct cyttsp5 *ts)
{
+ int retry = CY_CORE_STARTUP_RETRY_COUNT;
int error;
+reset:
error = cyttsp5_deassert_int(ts);
if (error) {
dev_err(ts->dev, "Error on deassert int r=%d\n", error);
- return -ENODEV;
+ }
+
+ error = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
+ if (error < 0) {
+ dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", error);
+ if (retry--)
+ goto reset;
+ return error;
}
/*
* Launch the application as the device starts in bootloader mode
* because of a power-on-reset
*/
- error = cyttsp5_hid_output_bl_launch_app(ts);
- if (error < 0) {
- dev_err(ts->dev, "Error on launch app r=%d\n", error);
- return error;
- }
+ if (ts->hid_desc.packet_id == HID_BL_REPORT_ID) {
+ error = cyttsp5_hid_output_bl_launch_app(ts);
+ if (error < 0) {
+ dev_err(ts->dev, "Error on launch app r=%d\n", error);
+ if (retry--)
+ goto reset;
+ return error;
+ }
- error = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
- if (error < 0) {
- dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", error);
- return error;
+ error = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
+ if (error < 0) {
+ dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", error);
+ if (retry--)
+ goto reset;
+ return error;
+ }
+
+ if (ts->hid_desc.packet_id == HID_BL_REPORT_ID) {
+ dev_err(ts->dev, "Error on launch app still in bootloader\n");
+ if (retry--)
+ goto reset;
+ return -EPROTO;
+ }
}
error = cyttsp5_fill_all_touch(ts);
if (error < 0) {
dev_err(ts->dev, "Error on report descriptor r=%d\n", error);
+ if (retry--)
+ goto reset;
return error;
}
error = cyttsp5_hid_output_get_sysinfo(ts);
if (error) {
dev_err(ts->dev, "Error on getting sysinfo r=%d\n", error);
+ if (retry--)
+ goto reset;
return error;
}
@@ -820,8 +898,7 @@ static void cyttsp5_cleanup(void *data)
regulator_disable(ts->vdd);
}
-static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
- const char *name)
+static struct cyttsp5 *cyttsp5_probe(struct device *dev, int irq, const char *name)
{
struct cyttsp5 *ts;
struct cyttsp5_sysinfo *si;
@@ -829,10 +906,9 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
if (!ts)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
/* Initialize device info */
- ts->regmap = regmap;
ts->dev = dev;
si = &ts->sysinfo;
dev_set_drvdata(dev, ts);
@@ -843,21 +919,21 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
ts->vdd = devm_regulator_get(dev, "vdd");
if (IS_ERR(ts->vdd)) {
error = PTR_ERR(ts->vdd);
- return error;
+ return ERR_PTR(error);
}
error = devm_add_action_or_reset(dev, cyttsp5_cleanup, ts);
if (error)
- return error;
+ return ERR_PTR(error);
error = regulator_enable(ts->vdd);
if (error)
- return error;
+ return ERR_PTR(error);
ts->input = devm_input_allocate_device(dev);
if (!ts->input) {
dev_err(dev, "Error, failed to allocate input device\n");
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
}
ts->input->name = "cyttsp5";
@@ -870,7 +946,7 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
if (IS_ERR(ts->reset_gpio)) {
error = PTR_ERR(ts->reset_gpio);
dev_err(dev, "Failed to request reset gpio, error %d\n", error);
- return error;
+ return ERR_PTR(error);
}
gpiod_set_value_cansleep(ts->reset_gpio, 0);
@@ -878,22 +954,22 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
msleep(20);
error = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
- IRQF_ONESHOT, name, ts);
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT, name, ts);
if (error) {
dev_err(dev, "unable to request IRQ\n");
- return error;
+ return ERR_PTR(error);
}
error = cyttsp5_startup(ts);
if (error) {
dev_err(ts->dev, "Fail initial startup r=%d\n", error);
- return error;
+ return ERR_PTR(error);
}
error = cyttsp5_parse_dt_key_code(dev);
if (error < 0) {
dev_err(ts->dev, "Error while parsing dts %d\n", error);
- return error;
+ return ERR_PTR(error);
}
touchscreen_parse_properties(ts->input, true, &ts->prop);
@@ -902,25 +978,29 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
for (i = 0; i < si->num_btns; i++)
__set_bit(si->key_code[i], ts->input->keybit);
- return cyttsp5_setup_input_device(dev);
+ error = cyttsp5_setup_input_device(dev);
+ if (error < 0)
+ return ERR_PTR(error);
+
+ return ts;
}
static int cyttsp5_i2c_probe(struct i2c_client *client)
{
- struct regmap *regmap;
- static const struct regmap_config config = {
- .reg_bits = 8,
- .val_bits = 8,
- };
+ struct cyttsp5 *ts;
- regmap = devm_regmap_init_i2c(client, &config);
- if (IS_ERR(regmap)) {
- dev_err(&client->dev, "regmap allocation failed: %ld\n",
- PTR_ERR(regmap));
- return PTR_ERR(regmap);
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(&client->dev, "I2C functionality not Supported\n");
+ return -EIO;
}
- return cyttsp5_probe(&client->dev, regmap, client->irq, client->name);
+ ts = cyttsp5_probe(&client->dev, client->irq, client->name);
+
+ if (IS_ERR(ts))
+ return PTR_ERR(ts);
+
+ i2c_set_clientdata(client, ts);
+ return 0;
}
static const struct of_device_id cyttsp5_of_match[] = {
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v5 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-10-23 5:55 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Anshul Dalal, linux-input, devicetree, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Thomas Weißschuh, Shuah Khan, linux-kernel-mentees,
linux-kernel
In-Reply-To: <ZTWza+S+t+UZKlwu@nixie71>
Hi Jeff,
Oct 23, 2023 01:42:47 Jeff LaBundy <jeff@labundy.com>:
> Hi Anshul,
>
> On Tue, Oct 17, 2023 at 09:13:45AM +0530, Anshul Dalal wrote:
>> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
>> has bidirectional thumb stick input and six buttons.
>>
>> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
>> to transmit the ADC data for the joystick and digital pin state for the
>> buttons. I have only implemented the functionality required to receive the
>> thumb stick and button state.
>>
>> Steps in reading the gamepad state over i2c:
>> 1. Reset the registers
>> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
>> `BUTTON_MASK`: A bit-map for the six digital pins internally
>> connected to the joystick buttons.
>> 3. Enable internal pullup resistors for the `BUTTON_MASK`
>> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
>> 5. Poll the device for button and joystick state done by:
>> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>>
>> Product page:
>> https://www.adafruit.com/product/5743
>> Arduino driver:
>> https://github.com/adafruit/Adafruit_Seesaw
>>
>> Driver tested on RPi Zero 2W
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>> ---
>>
>> Changes for v5:
>> - Added link to the datasheet
>> - Added debug log message when `seesaw_read_data` fails
>>
>> Changes for v4:
>> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
>> - Removed `hardware_id` field from `struct seesaw_gamepad`
>> - Removed redundant checks for the number of bytes written and received by
>> `i2c_master_send` and `i2c_master_recv`
>> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
>> - Changed `result & (1UL << BUTTON_)` to
>> `test_bit(BUTTON_, (long *)&result)`
>> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
>> - Fixed formatting issues
>> - Changed button reporting:
>> Since the gamepad had the action buttons in a non-standard layout:
>> (X)
>> (Y) (A)
>> (B)
>> Therefore moved to using generic directional action button event codes
>> instead of BTN_[ABXY].
>>
>> Changes for v3:
>> - no updates
>>
>> Changes for v2:
>> adafruit-seesaw.c:
>> - Renamed file from 'adafruit_seesaw.c'
>> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
>> - Changed count parameter for receiving joystick x on line 118:
>> `2` to `sizeof(write_buf)`
>> - Fixed invalid buffer size on line 123 and 126:
>> `data->y` to `sizeof(data->y)`
>> - Added comment for the `mdelay(10)` on line 169
>> - Changed inconsistent indentation on line 271
>> Kconfig:
>> - Fixed indentation for the help text
>> - Updated module name
>> Makefile:
>> - Updated module object file name
>> MAINTAINERS:
>> - Updated file name for the driver and bindings
>>
>> MAINTAINERS | 7 +
>> drivers/input/joystick/Kconfig | 9 +
>> drivers/input/joystick/Makefile | 1 +
>> drivers/input/joystick/adafruit-seesaw.c | 273 +++++++++++++++++++++++
>> 4 files changed, 290 insertions(+)
>> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6c4cce45a09d..a314f9b48e21 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
>> W: https://ez.analog.com/linux-software-drivers
>> F: drivers/input/touchscreen/ad7879.c
>>
>> +ADAFRUIT MINI I2C GAMEPAD
>> +M: Anshul Dalal <anshulusr@gmail.com>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>> +F: drivers/input/joystick/adafruit-seesaw.c
>> +
>> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
>> M: Jiri Kosina <jikos@kernel.org>
>> S: Maintained
>> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
>> index ac6925ce8366..df9cd1830b29 100644
>> --- a/drivers/input/joystick/Kconfig
>> +++ b/drivers/input/joystick/Kconfig
>> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
>> To compile this driver as a module, choose M here: the
>> module will be called sensehat_joystick.
>>
>> +config JOYSTICK_SEESAW
>> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
>> + depends on I2C
>> + help
>> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called adafruit-seesaw.
>> +
>> endif
>> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
>> index 3937535f0098..9976f596a920 100644
>> --- a/drivers/input/joystick/Makefile
>> +++ b/drivers/input/joystick/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
>> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
>> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
>> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
>> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
>> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
>> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
>> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
>> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
>> new file mode 100644
>> index 000000000000..2a1eae8d2861
>> --- /dev/null
>> +++ b/drivers/input/joystick/adafruit-seesaw.c
>> @@ -0,0 +1,273 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
>> + *
>> + * Driver for Adafruit Mini I2C Gamepad
>> + *
>> + * Based on the work of:
>> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
>> + *
>> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
>> + * Product page: https://www.adafruit.com/product/5743
>> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
>> + */
>> +
>> +#include <asm-generic/unaligned.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +/* clang-format off */
>
> I don't think we need this directive; at least, no other input drivers have
> it, or really any drivers for that matter.
>
>> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
>> +
>> +#define SEESAW_STATUS_BASE 0
>> +#define SEESAW_GPIO_BASE 1
>> +#define SEESAW_ADC_BASE 9
>> +
>> +#define SEESAW_GPIO_DIRCLR_BULK 3
>> +#define SEESAW_GPIO_BULK 4
>> +#define SEESAW_GPIO_BULK_SET 5
>> +#define SEESAW_GPIO_PULLENSET 11
>> +
>> +#define SEESAW_STATUS_HW_ID 1
>> +#define SEESAW_STATUS_SWRST 127
>> +
>> +#define SEESAW_ADC_OFFSET 7
>> +
>> +#define BUTTON_A 5
>> +#define BUTTON_B 1
>> +#define BUTTON_X 6
>> +#define BUTTON_Y 2
>> +#define BUTTON_START 16
>> +#define BUTTON_SELECT 0
>
> Please namespace these (e.g. SEESAW_BUTTON_A) to make it clear they refer
> to device-specific bits and not standard keycodes (e.g. BTN_A). In fact,
> these seem better off as part of an array of structs; more on that below.
>
>> +
>> +#define ANALOG_X 14
>> +#define ANALOG_Y 15
>
> Please namespace these as well.
>
>> +
>> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
>> +#define SEESAW_JOYSTICK_FUZZ 2
>> +#define SEESAW_JOYSTICK_FLAT 4
>> +
>> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
>> +#define SEESAW_GAMEPAD_POLL_MIN 8
>> +#define SEESAW_GAMEPAD_POLL_MAX 32
>> +/* clang-format on */
>> +
>> +u32 BUTTON_MASK = BIT(BUTTON_A) | BIT(BUTTON_B) | BIT(BUTTON_X) |
>> + BIT(BUTTON_Y) | BIT(BUTTON_START) | BIT(BUTTON_SELECT);
>> +
>> +struct seesaw_gamepad {
>> + char physical_path[32];
>> + struct input_dev *input_dev;
>> + struct i2c_client *i2c_client;
>> +};
>> +
>> +struct seesaw_data {
>> + __be16 x;
>> + __be16 y;
>> + u8 button_a, button_b, button_x, button_y, button_start, button_select;
>
> Please keep these each on a separate line.
>
>> +};
>
> Please declare this struct as __packed, as that is how it appears to be used.
>
>> +
>> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
>> +{
>> + int err;
>
> Please use 'ret' for return variables that can indicate a positive value on success.
>
>> + unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
>> + unsigned char read_buf[4];
>
> Please use standard kernel type definitions (i.e. u8 in this case).
>
>> +
>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (err < 0)
>> + return err;
>
> You correctly return err (or rather, ret) for negative values, but you should also
> check that ret matches the size of the data sent. For 0 <= ret < sizeof(writebuf),
> return -EIO.
The driver did this originally.
I then requested it to be removed as this case
can never happen.
i2c_master_send will either return size of(writebuf) or an error.
>> + err = i2c_master_recv(client, read_buf, sizeof(read_buf));
>> + if (err < 0)
>> + return err;
>
> And here.
>
>> +
>> + u32 result = get_unaligned_be32(&read_buf);
>
> Please do not mix declarations and code; all declarations must be at the
> top of the function.
>
>> +
>> + data->button_a = !test_bit(BUTTON_A, (long *)&result);
>> + data->button_b = !test_bit(BUTTON_B, (long *)&result);
>> + data->button_x = !test_bit(BUTTON_X, (long *)&result);
>> + data->button_y = !test_bit(BUTTON_Y, (long *)&result);
>> + data->button_start = !test_bit(BUTTON_START, (long *)&result);
>> + data->button_select = !test_bit(BUTTON_SELECT, (long *)&result);
>> +
>> + write_buf[0] = SEESAW_ADC_BASE;
>> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (err < 0)
>> + return err;
>> + err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
>> + if (err < 0)
>> + return err;
>
> This is starting to look like a 16-bit register map. To that end, please
> consider using regmap instead of open-coding each of these standard write-
> then-read operations.
>
> Using regmap would also save you the trouble of managing the endianness
> yourself, as well as having to check for incomplete transfers since its
> functions return zero or a negative error code only.
>
>> + /*
>> + * ADC reads left as max and right as 0, must be reversed since kernel
>> + * expects reports in opposite order.
>> + */
>> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
>> +
>> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>> + if (err < 0)
>> + return err;
>> + err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
>> + if (err < 0)
>> + return err;
>> + data->y = be16_to_cpu(data->y);
>> +
>> + return 0;
>> +}
>> +
>> +static void seesaw_poll(struct input_dev *input)
>> +{
>> + struct seesaw_gamepad *private = input_get_drvdata(input);
>> + struct seesaw_data data;
>> + int err;
>> +
>> + err = seesaw_read_data(private->i2c_client, &data);
>> + if (err != 0) {
>> + dev_dbg(&input->dev, "failed to read joystick state: %d\n",
>> + err);
>
> This should be dev_err_ratelimited().
>
>> + return;
>> + }
>> +
>> + input_report_abs(input, ABS_X, data.x);
>> + input_report_abs(input, ABS_Y, data.y);
>> + input_report_key(input, BTN_EAST, data.button_a);
>> + input_report_key(input, BTN_SOUTH, data.button_b);
>> + input_report_key(input, BTN_NORTH, data.button_x);
>> + input_report_key(input, BTN_WEST, data.button_y);
>> + input_report_key(input, BTN_START, data.button_start);
>> + input_report_key(input, BTN_SELECT, data.button_select);
>
> I think you can make this much cleaner and smaller by defining an array
> of structs, each with a key code and bit position. You can then simply
> iterate over the array and call input_report_key() once per element as
> in the following:
>
> struct seesaw_btn_desc {
> unsigned int code;
> unsigned int shift;
> };
>
> static const struct seesaw_btn_desc seesaw_btns[] = {
> {
> .code = BTN_EAST,
> .mask = 5,
> },
> [...]
> };
>
> And then:
>
> btn_status = ...;
>
> for (i = 0; i < ARRAY_SIZE(seesaw_btns); i++)
> input_report_key(input, seesaw_btns[i].code,
> btn_status & seesaw_btns[i].mask);
>
> This would also make it easier to quickly discern what keycodes are mapped
> to which bits in the register.
>
>> + input_sync(input);
>> +}
>> +
>> +static int seesaw_probe(struct i2c_client *client)
>> +{
>> + int err;
>> + struct seesaw_gamepad *private;
>
> I'd rather this be called something like 'seesaw' rather than private.
>
>> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
>> + SEESAW_STATUS_SWRST, 0xFF };
>> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
>> +
>> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
>> + if (err < 0)
>> + return err;
>> +
>> + /* Wait for the registers to reset before proceeding */
>> + mdelay(10);
>> +
>> + private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
>> + if (!private)
>> + return -ENOMEM;
>> +
>> + err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
>> + if (err < 0)
>> + return err;
>> +
>> + unsigned char hardware_id;
>
> Same comment as earlier with regard to mixed declarations.
>
>> +
>> + err = i2c_master_recv(client, &hardware_id, 1);
>> + if (err < 0)
>> + return err;
>> +
>> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
>> + hardware_id);
>> +
>> + private->i2c_client = client;
>> + scnprintf(private->physical_path, sizeof(private->physical_path),
>> + "i2c/%s", dev_name(&client->dev));
>
> This seems overly complex; can we not simply set input_dev->phys to the
> literal "i2c/seesaw-gamepad"? Why to copy at runtime and incur the cost
> of carrying 'physical_path' throughout the life of the module?
>
>> + i2c_set_clientdata(client, private);
>> +
>> + private->input_dev = devm_input_allocate_device(&client->dev);
>> + if (!private->input_dev)
>> + return -ENOMEM;
>> +
>> + private->input_dev->id.bustype = BUS_I2C;
>> + private->input_dev->name = "Adafruit Seesaw Gamepad";
>> + private->input_dev->phys = private->physical_path;
>> + input_set_drvdata(private->input_dev, private);
>> + input_set_abs_params(private->input_dev, ABS_X, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + input_set_abs_params(private->input_dev, ABS_Y, 0,
>> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
>> + SEESAW_JOYSTICK_FLAT);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_EAST);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_SOUTH);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_NORTH);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_WEST);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_START);
>> + input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
>
> Same comment with regard to creating an array of structs, and hence only
> having to call input_set_capability() from within a small loop.
>
>> +
>> + err = input_setup_polling(private->input_dev, seesaw_poll);
>> + if (err) {
>> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
>> + return err;
>> + }
>> +
>> + input_set_poll_interval(private->input_dev,
>> + SEESAW_GAMEPAD_POLL_INTERVAL);
>> + input_set_max_poll_interval(private->input_dev,
>> + SEESAW_GAMEPAD_POLL_MAX);
>> + input_set_min_poll_interval(private->input_dev,
>> + SEESAW_GAMEPAD_POLL_MIN);
>> +
>> + err = input_register_device(private->input_dev);
>> + if (err) {
>> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
>> + return err;
>> + }
>> +
>> + /* Set Pin Mode to input and enable pull-up resistors */
>> + unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
>> + BUTTON_MASK >> 24, BUTTON_MASK >> 16,
>> + BUTTON_MASK >> 8, BUTTON_MASK };
>> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
>> + if (err < 0)
>> + return err;
>> + pin_mode[1] = SEESAW_GPIO_PULLENSET;
>> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
>> + if (err < 0)
>> + return err;
>> + pin_mode[1] = SEESAW_GPIO_BULK_SET;
>> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
>> + if (err < 0)
>> + return err;
>
> Please configure the HW before the input device is live and being polled.
>
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_seesaw_match[] = {
>> + {
>> + .compatible = "adafruit,seesaw-gamepad",
>> + },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_seesaw_match);
>> +#endif /* CONFIG_OF */
>
> Please correct me if I am wrong, but it does not seem that OF support is
> required by this driver. There are no properties beyond the standard ones
> understood by the I2C core, which can match based on the ID table below.
>
>> +
>> +/* clang-format off */
>> +static const struct i2c_device_id seesaw_id_table[] = {
>> + { SEESAW_DEVICE_NAME, 0 },
>> + { /* Sentinel */ }
>> +};
>> +/* clang-format on */
>
> Again, I don't see any need for these directives.
>
>> +
>
> Nit: unnecessary NL.
>
>> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
>> +
>> +static struct i2c_driver seesaw_driver = {
>> + .driver = {
>> + .name = SEESAW_DEVICE_NAME,
>> + .of_match_table = of_match_ptr(of_seesaw_match),
>> + },
>> + .id_table = seesaw_id_table,
>> + .probe = seesaw_probe,
>> +};
>> +module_i2c_driver(seesaw_driver);
>> +
>> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
>> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.42.0
>>
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v9 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Dmitry Torokhov @ 2023-10-23 4:37 UTC (permalink / raw)
To: Neil Armstrong
Cc: linux-input, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20231021-topic-goodix-berlin-upstream-initial-v9-2-13fb4e887156@linaro.org>
Hi Neil,
On Sat, Oct 21, 2023 at 01:09:24PM +0200, Neil Armstrong wrote:
> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
> +{
> + u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
You probably already saw the kernel test robot message, I think we
should allocate this buffer in the heap (and free it once its no longer
needed).
> + __le16 length_raw;
> + u16 length;
> + int error;
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> + &length_raw, sizeof(length_raw));
> + if (error) {
> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
> + return error;
> + }
> +
> + length = le16_to_cpu(length_raw);
> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
> + dev_info(cd->dev, "invalid ic info length %d\n", length);
> + return -EINVAL;
> + }
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> + afe_data, length);
> + if (error) {
> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
> + return error;
> + }
> +
> + /* check whether the data is valid (ex. bus default values) */
> + if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
This cast is not needed.
> + dev_err(cd->dev, "fw info data invalid\n");
> + return -EINVAL;
> + }
> +
> + if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
This cast is not needed either.
> + dev_info(cd->dev, "fw info checksum error\n");
> + return -EINVAL;
> + }
> +
> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
> + if (error)
> + return error;
> +
> + /* check some key info */
> + if (!cd->touch_data_addr) {
> + dev_err(cd->dev, "touch_data_addr is null\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
> + const void *buf, int touch_num)
> +{
> + const struct goodix_berlin_touch_data *touch_data = buf;
> + int i;
> +
> + /* Check for data validity */
> + for (i = 0; i < touch_num; i++) {
> + unsigned int id;
> +
> + id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
> +
> + if (id >= GOODIX_BERLIN_MAX_TOUCH) {
> + dev_warn(cd->dev, "invalid finger id %d\n", id);
> + return;
Is it important to abort entire packet if one if the slots has incorrect
data? Can we simply skip over these contacts?
> + }
> + }
> +
> + /* Report finger touches */
> + for (i = 0; i < touch_num; i++) {
> + input_mt_slot(cd->input_dev,
> + FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
> + touch_data[i].id));
> + input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
> +
> + touchscreen_report_pos(cd->input_dev, &cd->props,
> + __le16_to_cpu(touch_data[i].x),
> + __le16_to_cpu(touch_data[i].y),
> + true);
> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
> + __le16_to_cpu(touch_data[i].w));
> + }
> +
> + input_mt_sync_frame(cd->input_dev);
> + input_sync(cd->input_dev);
> +}
> +
> +static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
> + const void *pre_buf, u32 pre_buf_len)
> +{
> + static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
No, please no static data. The drivers should be ready to handle more
than one device on a system. If the buffer is large-ish put it into
goodix_berlin_core.
> + u8 point_type, touch_num;
> + int error;
> +
> + /* copy pre-data to buffer */
> + memcpy(buffer, pre_buf, pre_buf_len);
> +
> + touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
> + buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
> +
> + if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
> + return;
> + }
> +
> + if (touch_num) {
> + /* read more data if more than 2 touch events */
> + if (unlikely(touch_num > 2)) {
> + error = regmap_raw_read(cd->regmap,
> + cd->touch_data_addr + pre_buf_len,
> + &buffer[pre_buf_len],
> + (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
> + if (error) {
> + dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
> + error);
> + return;
> + }
> + }
> +
> + point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
> + buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
> +
> + if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
> + point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
> + return;
> + }
> +
> + if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
> + dev_err(cd->dev, "touch data checksum error, data: %*ph\n",
> + touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
> + &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
> + return;
> + }
> + }
> +
> + goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
> + touch_num);
> +}
> +
> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
> +{
> + gpiod_set_value(cd->reset_gpio, 1);
> + usleep_range(2000, 2100);
> + gpiod_set_value(cd->reset_gpio, 0);
> +
> + msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
The reset line handling is optional, we should skip waits if there is
no GPIO defined for the board.
> +
> + return 0;
> +}
> +
> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
> +{
> + struct goodix_berlin_core *cd = data;
> + u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
> + u8 event_status;
> + int error;
> +
> + /* First, read buffer with space for 2 touch events */
> + error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
> + GOODIX_BERLIN_IRQ_READ_LEN(2));
> + if (error) {
> + dev_err_ratelimited(cd->dev, "failed get event head data, %d\n", error);
> + return IRQ_HANDLED;
> + }
> +
> + if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
> + return IRQ_HANDLED;
> +
> + if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
> + dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
> + GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
> + return IRQ_HANDLED;
> + }
> +
> + event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
> +
> + if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
> + goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
> +
> + if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
> + switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
> + case GOODIX_BERLIN_REQUEST_CODE_RESET:
> + goodix_berlin_request_handle_reset(cd);
> + break;
> +
> + default:
> + dev_warn(cd->dev, "unsupported request code 0x%x\n",
> + buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
> + }
> + }
> +
> + /* Clear up status field */
> + regmap_write(cd->regmap, cd->touch_data_addr, 0);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
> + const struct input_id *id)
> +{
> + struct input_dev *input_dev;
> + int error;
> +
> + input_dev = devm_input_allocate_device(cd->dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + cd->input_dev = input_dev;
> + input_set_drvdata(input_dev, cd);
> +
> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
> + input_dev->phys = "input/ts";
> +
> + input_dev->id = *id;
> +
> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
> +
> + error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error)
> + return error;
> +
> + return input_register_device(cd->input_dev);
Please in functions with multiple possible failure paths use format
error = op(...);
if (error)
return error;
return 0;
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v9 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
From: kernel test robot @ 2023-10-23 3:24 UTC (permalink / raw)
To: Neil Armstrong, Dmitry Torokhov, linux-input
Cc: oe-kbuild-all, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-arm-msm, devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231021-topic-goodix-berlin-upstream-initial-v9-3-13fb4e887156@linaro.org>
Hi Neil,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 2030579113a1b1b5bfd7ff24c0852847836d8fd1]
url: https://github.com/intel-lab-lkp/linux/commits/Neil-Armstrong/dt-bindings-input-document-Goodix-Berlin-Touchscreen-IC/20231021-191942
base: 2030579113a1b1b5bfd7ff24c0852847836d8fd1
patch link: https://lore.kernel.org/r/20231021-topic-goodix-berlin-upstream-initial-v9-3-13fb4e887156%40linaro.org
patch subject: [PATCH v9 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20231023/202310231123.eHyxswnW-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231023/202310231123.eHyxswnW-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310231123.eHyxswnW-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/input/touchscreen/goodix_berlin_core.c: In function 'goodix_berlin_get_ic_info':
>> drivers/input/touchscreen/goodix_berlin_core.c:285:1: warning: the frame size of 1148 bytes is larger than 1024 bytes [-Wframe-larger-than=]
285 | }
| ^
vim +285 drivers/input/touchscreen/goodix_berlin_core.c
7aae63b22cf7e9 Neil Armstrong 2023-10-21 235
7aae63b22cf7e9 Neil Armstrong 2023-10-21 236 static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
7aae63b22cf7e9 Neil Armstrong 2023-10-21 237 {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 238 u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
7aae63b22cf7e9 Neil Armstrong 2023-10-21 239 __le16 length_raw;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 240 u16 length;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 241 int error;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 242
7aae63b22cf7e9 Neil Armstrong 2023-10-21 243 error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
7aae63b22cf7e9 Neil Armstrong 2023-10-21 244 &length_raw, sizeof(length_raw));
7aae63b22cf7e9 Neil Armstrong 2023-10-21 245 if (error) {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 246 dev_info(cd->dev, "failed get ic info length, %d\n", error);
7aae63b22cf7e9 Neil Armstrong 2023-10-21 247 return error;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 248 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 249
7aae63b22cf7e9 Neil Armstrong 2023-10-21 250 length = le16_to_cpu(length_raw);
7aae63b22cf7e9 Neil Armstrong 2023-10-21 251 if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 252 dev_info(cd->dev, "invalid ic info length %d\n", length);
7aae63b22cf7e9 Neil Armstrong 2023-10-21 253 return -EINVAL;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 254 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 255
7aae63b22cf7e9 Neil Armstrong 2023-10-21 256 error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
7aae63b22cf7e9 Neil Armstrong 2023-10-21 257 afe_data, length);
7aae63b22cf7e9 Neil Armstrong 2023-10-21 258 if (error) {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 259 dev_info(cd->dev, "failed get ic info data, %d\n", error);
7aae63b22cf7e9 Neil Armstrong 2023-10-21 260 return error;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 261 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 262
7aae63b22cf7e9 Neil Armstrong 2023-10-21 263 /* check whether the data is valid (ex. bus default values) */
7aae63b22cf7e9 Neil Armstrong 2023-10-21 264 if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 265 dev_err(cd->dev, "fw info data invalid\n");
7aae63b22cf7e9 Neil Armstrong 2023-10-21 266 return -EINVAL;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 267 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 268
7aae63b22cf7e9 Neil Armstrong 2023-10-21 269 if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 270 dev_info(cd->dev, "fw info checksum error\n");
7aae63b22cf7e9 Neil Armstrong 2023-10-21 271 return -EINVAL;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 272 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 273
7aae63b22cf7e9 Neil Armstrong 2023-10-21 274 error = goodix_berlin_convert_ic_info(cd, afe_data, length);
7aae63b22cf7e9 Neil Armstrong 2023-10-21 275 if (error)
7aae63b22cf7e9 Neil Armstrong 2023-10-21 276 return error;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 277
7aae63b22cf7e9 Neil Armstrong 2023-10-21 278 /* check some key info */
7aae63b22cf7e9 Neil Armstrong 2023-10-21 279 if (!cd->touch_data_addr) {
7aae63b22cf7e9 Neil Armstrong 2023-10-21 280 dev_err(cd->dev, "touch_data_addr is null\n");
7aae63b22cf7e9 Neil Armstrong 2023-10-21 281 return -EINVAL;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 282 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 283
7aae63b22cf7e9 Neil Armstrong 2023-10-21 284 return 0;
7aae63b22cf7e9 Neil Armstrong 2023-10-21 @285 }
7aae63b22cf7e9 Neil Armstrong 2023-10-21 286
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Jeff LaBundy @ 2023-10-22 23:47 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thomas Weißschuh,
Shuah Khan, linux-kernel-mentees, linux-kernel, Conor Dooley,
Krzysztof Kozlowski
In-Reply-To: <20231017034356.1436677-1-anshulusr@gmail.com>
Hi Anshul,
On Tue, Oct 17, 2023 at 09:13:44AM +0530, Anshul Dalal wrote:
> Adds bindings for the Adafruit Seesaw Gamepad.
>
> The gamepad functions as an i2c device with the default address of 0x50
> and has an IRQ pin that can be enabled in the driver to allow for a rising
> edge trigger on each button press or joystick movement.
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
Perhaps this ship has sailed, but is there any reason this simple device
cannot be added to Documentation/devicetree/bindings/trivial-devices.yaml
as opposed to having its own binding?
It has no vendor-specific properties, and the only properties are the
standard properties already understood by the I2C core. In case I have
misunderstood, please let me know.
> ---
>
> Changes for v5:
> - Added link to the datasheet
>
> Changes for v4:
> - Fixed the URI for the id field
> - Added `interrupts` property
>
> Changes for v3:
> - Updated id field to reflect updated file name from previous version
> - Added `reg` property
>
> Changes for v2:
> - Renamed file to `adafruit,seesaw-gamepad.yaml`
> - Removed quotes for `$id` and `$schema`
> - Removed "Bindings for" from the description
> - Changed node name to the generic name "joystick"
> - Changed compatible to 'adafruit,seesaw-gamepad' instead of
> 'adafruit,seesaw_gamepad'
>
> .../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++
> 1 file changed, 60 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> new file mode 100644
> index 000000000000..3f0d1c5a3b9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Adafruit Mini I2C Gamepad with seesaw
> +
> +maintainers:
> + - Anshul Dalal <anshulusr@gmail.com>
> +
> +description: |
> + Adafruit Mini I2C Gamepad
> +
> + +-----------------------------+
> + | ___ |
> + | / \ (X) |
> + | | S | __ __ (Y) (A) |
> + | \___/ |ST| |SE| (B) |
> + | |
> + +-----------------------------+
> +
> + S -> 10-bit percision bidirectional analog joystick
> + ST -> Start
> + SE -> Select
> + X, A, B, Y -> Digital action buttons
> +
> + Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> + Product page: https://www.adafruit.com/product/5743
> + Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
> +
> +properties:
> + compatible:
> + const: adafruit,seesaw-gamepad
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> + description:
> + The gamepad's IRQ pin triggers a rising edge if interrupts are enabled.
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + joystick@50 {
> + compatible = "adafruit,seesaw-gamepad";
> + reg = <0x50>;
> + };
> + };
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v5 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Jeff LaBundy @ 2023-10-22 23:42 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thomas Weißschuh,
Shuah Khan, linux-kernel-mentees, linux-kernel
In-Reply-To: <20231017034356.1436677-2-anshulusr@gmail.com>
Hi Anshul,
On Tue, Oct 17, 2023 at 09:13:45AM +0530, Anshul Dalal wrote:
> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Driver tested on RPi Zero 2W
>
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
>
> Changes for v5:
> - Added link to the datasheet
> - Added debug log message when `seesaw_read_data` fails
>
> Changes for v4:
> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
> - Removed `hardware_id` field from `struct seesaw_gamepad`
> - Removed redundant checks for the number of bytes written and received by
> `i2c_master_send` and `i2c_master_recv`
> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
> - Changed `result & (1UL << BUTTON_)` to
> `test_bit(BUTTON_, (long *)&result)`
> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
> - Fixed formatting issues
> - Changed button reporting:
> Since the gamepad had the action buttons in a non-standard layout:
> (X)
> (Y) (A)
> (B)
> Therefore moved to using generic directional action button event codes
> instead of BTN_[ABXY].
>
> Changes for v3:
> - no updates
>
> Changes for v2:
> adafruit-seesaw.c:
> - Renamed file from 'adafruit_seesaw.c'
> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> - Changed count parameter for receiving joystick x on line 118:
> `2` to `sizeof(write_buf)`
> - Fixed invalid buffer size on line 123 and 126:
> `data->y` to `sizeof(data->y)`
> - Added comment for the `mdelay(10)` on line 169
> - Changed inconsistent indentation on line 271
> Kconfig:
> - Fixed indentation for the help text
> - Updated module name
> Makefile:
> - Updated module object file name
> MAINTAINERS:
> - Updated file name for the driver and bindings
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 273 +++++++++++++++++++++++
> 4 files changed, 290 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6c4cce45a09d..a314f9b48e21 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> W: https://ez.analog.com/linux-software-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADAFRUIT MINI I2C GAMEPAD
> +M: Anshul Dalal <anshulusr@gmail.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> +F: drivers/input/joystick/adafruit-seesaw.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <jikos@kernel.org>
> S: Maintained
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index ac6925ce8366..df9cd1830b29 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
> To compile this driver as a module, choose M here: the
> module will be called sensehat_joystick.
>
> +config JOYSTICK_SEESAW
> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> + depends on I2C
> + help
> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adafruit-seesaw.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 3937535f0098..9976f596a920 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..2a1eae8d2861
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Adafruit Mini I2C Gamepad
> + *
> + * Based on the work of:
> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> + *
> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> + * Product page: https://www.adafruit.com/product/5743
> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> + */
> +
> +#include <asm-generic/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +/* clang-format off */
I don't think we need this directive; at least, no other input drivers have
it, or really any drivers for that matter.
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define BUTTON_A 5
> +#define BUTTON_B 1
> +#define BUTTON_X 6
> +#define BUTTON_Y 2
> +#define BUTTON_START 16
> +#define BUTTON_SELECT 0
Please namespace these (e.g. SEESAW_BUTTON_A) to make it clear they refer
to device-specific bits and not standard keycodes (e.g. BTN_A). In fact,
these seem better off as part of an array of structs; more on that below.
> +
> +#define ANALOG_X 14
> +#define ANALOG_Y 15
Please namespace these as well.
> +
> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> +#define SEESAW_JOYSTICK_FUZZ 2
> +#define SEESAW_JOYSTICK_FLAT 4
> +
> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> +#define SEESAW_GAMEPAD_POLL_MIN 8
> +#define SEESAW_GAMEPAD_POLL_MAX 32
> +/* clang-format on */
> +
> +u32 BUTTON_MASK = BIT(BUTTON_A) | BIT(BUTTON_B) | BIT(BUTTON_X) |
> + BIT(BUTTON_Y) | BIT(BUTTON_START) | BIT(BUTTON_SELECT);
> +
> +struct seesaw_gamepad {
> + char physical_path[32];
> + struct input_dev *input_dev;
> + struct i2c_client *i2c_client;
> +};
> +
> +struct seesaw_data {
> + __be16 x;
> + __be16 y;
> + u8 button_a, button_b, button_x, button_y, button_start, button_select;
Please keep these each on a separate line.
> +};
Please declare this struct as __packed, as that is how it appears to be used.
> +
> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> +{
> + int err;
Please use 'ret' for return variables that can indicate a positive value on success.
> + unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
> + unsigned char read_buf[4];
Please use standard kernel type definitions (i.e. u8 in this case).
> +
> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (err < 0)
> + return err;
You correctly return err (or rather, ret) for negative values, but you should also
check that ret matches the size of the data sent. For 0 <= ret < sizeof(writebuf),
return -EIO.
> + err = i2c_master_recv(client, read_buf, sizeof(read_buf));
> + if (err < 0)
> + return err;
And here.
> +
> + u32 result = get_unaligned_be32(&read_buf);
Please do not mix declarations and code; all declarations must be at the
top of the function.
> +
> + data->button_a = !test_bit(BUTTON_A, (long *)&result);
> + data->button_b = !test_bit(BUTTON_B, (long *)&result);
> + data->button_x = !test_bit(BUTTON_X, (long *)&result);
> + data->button_y = !test_bit(BUTTON_Y, (long *)&result);
> + data->button_start = !test_bit(BUTTON_START, (long *)&result);
> + data->button_select = !test_bit(BUTTON_SELECT, (long *)&result);
> +
> + write_buf[0] = SEESAW_ADC_BASE;
> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (err < 0)
> + return err;
> + err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
> + if (err < 0)
> + return err;
This is starting to look like a 16-bit register map. To that end, please
consider using regmap instead of open-coding each of these standard write-
then-read operations.
Using regmap would also save you the trouble of managing the endianness
yourself, as well as having to check for incomplete transfers since its
functions return zero or a negative error code only.
> + /*
> + * ADC reads left as max and right as 0, must be reversed since kernel
> + * expects reports in opposite order.
> + */
> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
> +
> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (err < 0)
> + return err;
> + err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
> + if (err < 0)
> + return err;
> + data->y = be16_to_cpu(data->y);
> +
> + return 0;
> +}
> +
> +static void seesaw_poll(struct input_dev *input)
> +{
> + struct seesaw_gamepad *private = input_get_drvdata(input);
> + struct seesaw_data data;
> + int err;
> +
> + err = seesaw_read_data(private->i2c_client, &data);
> + if (err != 0) {
> + dev_dbg(&input->dev, "failed to read joystick state: %d\n",
> + err);
This should be dev_err_ratelimited().
> + return;
> + }
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> + input_report_key(input, BTN_EAST, data.button_a);
> + input_report_key(input, BTN_SOUTH, data.button_b);
> + input_report_key(input, BTN_NORTH, data.button_x);
> + input_report_key(input, BTN_WEST, data.button_y);
> + input_report_key(input, BTN_START, data.button_start);
> + input_report_key(input, BTN_SELECT, data.button_select);
I think you can make this much cleaner and smaller by defining an array
of structs, each with a key code and bit position. You can then simply
iterate over the array and call input_report_key() once per element as
in the following:
struct seesaw_btn_desc {
unsigned int code;
unsigned int shift;
};
static const struct seesaw_btn_desc seesaw_btns[] = {
{
.code = BTN_EAST,
.mask = 5,
},
[...]
};
And then:
btn_status = ...;
for (i = 0; i < ARRAY_SIZE(seesaw_btns); i++)
input_report_key(input, seesaw_btns[i].code,
btn_status & seesaw_btns[i].mask);
This would also make it easier to quickly discern what keycodes are mapped
to which bits in the register.
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err;
> + struct seesaw_gamepad *private;
I'd rather this be called something like 'seesaw' rather than private.
> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF };
> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
> +
> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
> + if (err < 0)
> + return err;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
> +
> + private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
> + if (!private)
> + return -ENOMEM;
> +
> + err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
> + if (err < 0)
> + return err;
> +
> + unsigned char hardware_id;
Same comment as earlier with regard to mixed declarations.
> +
> + err = i2c_master_recv(client, &hardware_id, 1);
> + if (err < 0)
> + return err;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + private->i2c_client = client;
> + scnprintf(private->physical_path, sizeof(private->physical_path),
> + "i2c/%s", dev_name(&client->dev));
This seems overly complex; can we not simply set input_dev->phys to the
literal "i2c/seesaw-gamepad"? Why to copy at runtime and incur the cost
of carrying 'physical_path' throughout the life of the module?
> + i2c_set_clientdata(client, private);
> +
> + private->input_dev = devm_input_allocate_device(&client->dev);
> + if (!private->input_dev)
> + return -ENOMEM;
> +
> + private->input_dev->id.bustype = BUS_I2C;
> + private->input_dev->name = "Adafruit Seesaw Gamepad";
> + private->input_dev->phys = private->physical_path;
> + input_set_drvdata(private->input_dev, private);
> + input_set_abs_params(private->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(private->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_capability(private->input_dev, EV_KEY, BTN_EAST);
> + input_set_capability(private->input_dev, EV_KEY, BTN_SOUTH);
> + input_set_capability(private->input_dev, EV_KEY, BTN_NORTH);
> + input_set_capability(private->input_dev, EV_KEY, BTN_WEST);
> + input_set_capability(private->input_dev, EV_KEY, BTN_START);
> + input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
Same comment with regard to creating an array of structs, and hence only
having to call input_set_capability() from within a small loop.
> +
> + err = input_setup_polling(private->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
> + input_set_max_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(private->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
> + BUTTON_MASK >> 24, BUTTON_MASK >> 16,
> + BUTTON_MASK >> 8, BUTTON_MASK };
> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + if (err < 0)
> + return err;
> + pin_mode[1] = SEESAW_GPIO_PULLENSET;
> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + if (err < 0)
> + return err;
> + pin_mode[1] = SEESAW_GPIO_BULK_SET;
> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + if (err < 0)
> + return err;
Please configure the HW before the input device is live and being polled.
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_seesaw_match[] = {
> + {
> + .compatible = "adafruit,seesaw-gamepad",
> + },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_seesaw_match);
> +#endif /* CONFIG_OF */
Please correct me if I am wrong, but it does not seem that OF support is
required by this driver. There are no properties beyond the standard ones
understood by the I2C core, which can match based on the ID table below.
> +
> +/* clang-format off */
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME, 0 },
> + { /* Sentinel */ }
> +};
> +/* clang-format on */
Again, I don't see any need for these directives.
> +
Nit: unnecessary NL.
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + .of_match_table = of_match_ptr(of_seesaw_match),
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Jeff LaBundy @ 2023-10-22 21:54 UTC (permalink / raw)
To: Kamel Bouhara
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
Marco Felsch, mark.satterthwaite, bartp, hannah.rossiter,
Thomas Petazzoni, Gregory Clement, bsp-development.geo
In-Reply-To: <20231012074034.1090436-4-kamel.bouhara@bootlin.com>
Hi Kamel,
On Thu, Oct 12, 2023 at 09:40:34AM +0200, Kamel Bouhara wrote:
> Add a new driver for the TouchNetix's axiom family of
> touchscreen controllers. This driver only supports i2c
> and can be later adapted for SPI and USB support.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
> MAINTAINERS | 1 +
> drivers/input/touchscreen/Kconfig | 13 +
> drivers/input/touchscreen/Makefile | 1 +
> .../input/touchscreen/touchnetix_axiom_i2c.c | 740 ++++++++++++++++++
> 4 files changed, 755 insertions(+)
> create mode 100644 drivers/input/touchscreen/touchnetix_axiom_i2c.c
Please do not include 'i2c' in the filename. If the driver is expanded in
the future to support SPI, it would make sense to have touchnetix_axiom.c,
touchnetix_axiom_i2c.c and touchnetix_axiom_spi.c. To prevent this driver
from having to be renamed in that case, just call it touchnetix_axiom.c.
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 12ae8bc6b8cf..2d1e0b025e89 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21415,6 +21415,7 @@ M: Kamel Bouhara <kamel.bouhara@bootlin.com>
> L: linux-input@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
> +F: drivers/input/touchscreen/touchnetix_axiom_i2c.c
>
> THUNDERBOLT DMA TRAFFIC TEST DRIVER
> M: Isaac Hazan <isaac.hazan@intel.com>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..58665ccbe077 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -803,6 +803,19 @@ config TOUCHSCREEN_MIGOR
> To compile this driver as a module, choose M here: the
> module will be called migor_ts.
>
> +config TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C
> + tristate "TouchNetix AXIOM based touchscreen controllers"
> + depends on I2C
> + depends on GPIOLIB || COMPILE_TEST
All gpiod_*() functions used in this driver have a dummy function for the
CONFIG_GPIOLIB=n case, so this dependency is unnecessary.
> + help
> + Say Y here if you have a axiom touchscreen connected to
> + your system.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called axiom_i2c.
> +
> config TOUCHSCREEN_TOUCHRIGHT
> tristate "Touchright serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..23b6fb8864b0 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
> obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI) += surface3_spi.o
> obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
> +obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C) += touchnetix_axiom_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
> obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
> obj-$(CONFIG_TOUCHSCREEN_TS4800) += ts4800-ts.o
> diff --git a/drivers/input/touchscreen/touchnetix_axiom_i2c.c b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
> new file mode 100644
> index 000000000000..fb6239a87341
> --- /dev/null
> +++ b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
> @@ -0,0 +1,740 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TouchNetix aXiom Touchscreen Driver
> + *
> + * Copyright (C) 2020-2023 TouchNetix Ltd.
> + *
> + * Author(s): Bart Prescott <bartp@baasheep.co.uk>
> + * Pedro Torruella <pedro.torruella@touchnetix.com>
> + * Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> + * Kamel Bouhara <kamel.bouhara@bootlin.com>
> + *
> + */
> +
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
Please #include mod_devicetable.h as well.
> +#include <linux/of.h>
> +#include <linux/pm.h>
In addition to Marco's comment about unused includes, pm.h does not appear
to be used either.
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +/*
> + * Runtime TCP mode: device is executing normal code and is
> + * accessible via the Touch Controller Mode
> + */
> +#define BOOT_TCP 0
> +/*
> + * Bootloader BLP mode: device is executing bootloader and is
> + * accessible via the Boot Loader Protocol.
> + */
> +#define BOOT_BLP 1
> +#define AXIOM_PROX_LEVEL -128
> +/*
> + * Register group u31 has 2 pages for usage table entries.
> + * (2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE = 85
> + */
> +#define AXIOM_U31_MAX_USAGES 85
> +#define AXIOM_U31_BYTES_PER_USAGE 6
> +#define AXIOM_U31_PAGE0_LENGTH 0x0C
> +#define AXIOM_U31_BOOTMODE_MASK BIT(7)
> +#define AXIOM_U31_FW_INFO_VARIANT_MASK GENMASK(6, 0)
> +#define AXIOM_U31_FW_INFO_STATUS_MASK BIT(7)
> +
> +#define AXIOM_U41_MAX_TARGETS 10
> +
> +#define AXIOM_U46_AUX_CHANNELS 4
> +#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
> +
> +#define AXIOM_COMMS_MAX_USAGE_PAGES 3
> +#define AXIOM_COMMS_PAGE_SIZE 256
> +#define AXIOM_COMMS_OVERFLOW_MASK BIT(7)
> +#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(7, 0)
> +
> +#define AXIOM_REBASELINE_CMD 0x03
> +
> +#define AXIOM_REPORT_USAGE_ID 0x34
> +#define AXIOM_DEVINFO_USAGE_ID 0x31
> +#define AXIOM_USAGE_2HB_REPORT_ID 0x01
> +#define AXIOM_REBASELINE_USAGE_ID 0x02
> +#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
> +#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
> +
> +#define AXIOM_PAGE_MASK GENMASK(15, 8)
> +#define AXIOM_PAGE_OFFSET_MASK GENMASK(7, 0)
> +
> +struct axiom_devinfo {
> + char bootloader_fw_major;
Please use standard kernel type definitions, specifically u8 in place of char.
> + char bootloader_fw_minor;
> + char bootmode;
> + u16 device_id;
> + char fw_major;
> + char fw_minor;
> + char fw_info_extra;
> + char tcp_revision;
> + u16 jedec_id;
> + char num_usages;
> + char silicon_revision;
> +};
> +
> +/*
> + * Describes parameters of a specific usage, essenstially a single element of
> + * the "Usage Table"
> + */
> +struct usage_entry {
> + char id;
> + char is_report;
> + char start_page;
> + char num_pages;
> +};
> +
> +/*
> + * Holds state of a touch or target when detected prior a touch (eg.
> + * hover or proximity events).
> + */
Nit: this comment is misleading. The enum itself does not hold state; it
represents state. A variable defined using this enum holds the state.
> +enum axiom_target_state {
> + TARGET_STATE_NOT_PRESENT = 0,
> + TARGET_STATE_PROX = 1,
> + TARGET_STATE_HOVER = 2,
> + TARGET_STATE_TOUCHING = 3,
> + TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
> + TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
> +};
Please namespace these, i.e. AXIOM_TARGET_STATE_*.
> +
> +struct u41_target {
> + enum axiom_target_state state;
> + u16 x;
> + u16 y;
> + s8 z;
> + bool insert;
> + bool touch;
> +};
Please namespace this struct definition as you have done below.
> +
> +struct axiom_target_report {
> + u8 index;
> + u8 present;
> + u16 x;
> + u16 y;
> + s8 z;
> +};
> +
> +struct axiom_cmd_header {
> + u16 target_address;
> + u16 length:15;
> + u16 read:1;
> +} __packed;
> +
> +struct axiom_data {
> + struct axiom_devinfo devinfo;
> + struct device *dev;
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *irq_gpio;
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + u32 max_report_len;
> + u32 report_overflow_counter;
> + u32 report_counter;
> + char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
> + struct u41_target targets[AXIOM_U41_MAX_TARGETS];
> + struct usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> + bool usage_table_populated;
> +};
> +
> +/*
> + * aXiom devices are typically configured to report
> + * touches at a rate of 100Hz (10ms). For systems
> + * that require polling for reports, 100ms seems like
> + * an acceptable polling rate.
> + * When reports are polled, it will be expected to
> + * occasionally observe the overflow bit being set
> + * in the reports. This indicates that reports are not
> + * being read fast enough.
> + */
> +#define POLL_INTERVAL_DEFAULT_MS 100
I'd rather we take this information from device tree; it seems 'poll-interval'
is a common property used by other drivers.
> +
> +/* Translate usage/page/offset triplet into physical address. */
> +static u16
> +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> + char offset)
The line break after the function type is a bit confusing; please use this
more common style and namespace all functions:
static u16 axiom_usage_to_target_address(...,
...);
Note any line breaks are aligned to the opening parenthesis.
> +{
> + struct axiom_devinfo *device_info;
> + struct usage_entry *usage_table;
> + u32 i;
> +
> + device_info = &ts->devinfo;
> + usage_table = ts->usage_table;
> +
> + /* At the moment the convention is that u31 is always at physical address 0x0 */
> + if (!ts->usage_table_populated) {
> + if (usage == AXIOM_DEVINFO_USAGE_ID)
> + return ((page << 8) + offset);
> + else
> + return 0xffff;
> + }
> +
> + for (i = 0; i < device_info->num_usages; i++) {
> + if (usage_table[i].id != usage)
> + continue;
> +
> + if (page >= usage_table[i].num_pages) {
> + dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> + usage, page, offset);
> + return 0xffff;
> + }
> + }
> +
> + return ((usage_table[i].start_page + page) << 8) + offset;
> +}
> +
> +static int
> +axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> + struct axiom_cmd_header cmd_header;
> + struct i2c_msg msg[2];
> + int ret;
> +
> + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> + cmd_header.length = cpu_to_le16(len);
> + cmd_header.read = 1;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = sizeof(cmd_header);
> + msg[0].buf = (u8 *)&cmd_header;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = (char *)buf;
Again, please use u8 in place of char, as was done for the first element.
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
Please use ARRAY_SIZE(msg) above as you do below.
> + if (ret != ARRAY_SIZE(msg)) {
> + dev_err(&client->dev,
> + "Failed reading usage %#x page %#x, error=%d\n",
> + usage, page, ret);
> + return -EIO;
> + }
This check papers over negative error codes that may have been returned by
i2c_transfer(). For ret < 0 you should return ret, and only return -EIO for
0 <= ret < ARRAY_SIZE(msg).
More importantly, however, if this device supports multiple transports and
you expect SPI support can be added in the future, you really should use
regmap throughout in order to avoid ripping up this driver later.
> +
> + return 0;
> +}
> +
> +static int
> +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> + struct axiom_cmd_header cmd_header;
> + struct i2c_msg msg[2];
> + int ret;
> +
> + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> + cmd_header.length = cpu_to_le16(len);
> + cmd_header.read = 0;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = sizeof(cmd_header);
> + msg[0].buf = (u8 *)&cmd_header;
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = 0;
> + msg[1].len = len;
> + msg[1].buf = (char *)buf;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "Failed to write usage %#x page %#x, error=%d\n", usage,
> + page, ret);
> + return ret;
> + }
The error handling between your read and write wrappers is inconsistent;
please see my comment above.
Is there any reason i2c_master_send() cannot work here? I'm guessing the
controller needs a repeated start in between the two messages?
For these kind of special requirements, it's helpful to add some comments
as to why the HW calls for additional housekeeping.
> +
> + return 0;
> +}
> +
> +/*
> + * Decodes and populates the local Usage Table.
> + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> + * device.
> + */
What is a usage table? These comments aren't helpful unless some of the
underlying concepts are defined as well.
> +static u32 axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> +{
> + u32 usage_id = 0;
There is no need to initialize this iterator.
> + u32 max_report_len = 0;
> + struct axiom_devinfo *device_info;
> + struct usage_entry *usage_table;
> +
> + device_info = &ts->devinfo;
> + usage_table = ts->usage_table;
> +
> + for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> + u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> + char id = rx_data[offset + 0];
> + char start_page = rx_data[offset + 1];
> + char num_pages = rx_data[offset + 2];
Please consider whether you can use a packed struct for this decoding.
> + u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> +
> + if (!num_pages)
> + usage_table[usage_id].is_report = true;
> +
> + /* Store the entry into the usage table */
> + usage_table[usage_id].id = id;
> + usage_table[usage_id].start_page = start_page;
> + usage_table[usage_id].num_pages = num_pages;
> +
> + dev_dbg(ts->dev, "Usage %2u Info: %*ph\n", usage_id,
> + AXIOM_U31_BYTES_PER_USAGE,
> + &rx_data[offset]);
Nit: this line break seems unnecessary.
> +
> + /* Identify the max report length the module will receive */
> + if (usage_table[usage_id].is_report && max_offset > max_report_len)
> + max_report_len = max_offset;
> + }
> + ts->usage_table_populated = true;
> +
> + return max_report_len;
> +}
> +
> +/* Retrieve, store and print the axiom device information */
This comment does not seem particularly helpful.
> +static int axiom_discover(struct axiom_data *ts)
> +{
> + struct axiom_devinfo *devinfo = &ts->devinfo;
> + struct device *dev = ts->dev;
> + char *rx_data = ts->rx_buf;
> + int ret;
In input, variables that represent a negative error code (fail) or zero (pass)
tend to be called 'error'.
> +
> + /*
> + * Fetch the first page of usage u31 to get the
> + * device information and the number of usages
> + */
> + ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 0, rx_data,
> + AXIOM_U31_PAGE0_LENGTH);
> + if (ret)
> + return ret;
> +
> + devinfo->bootmode = (rx_data[0] & AXIOM_U31_BOOTMODE_MASK);
> + devinfo->device_id = ((rx_data[1] & AXIOM_PAGE_OFFSET_MASK) << 8) | rx_data[0];
> + devinfo->fw_minor = rx_data[2];
> + devinfo->fw_major = rx_data[3];
> + devinfo->fw_info_extra = rx_data[4];
> + devinfo->bootloader_fw_minor = rx_data[6];
> + devinfo->bootloader_fw_major = rx_data[7];
> + devinfo->jedec_id = (rx_data[8]) | (rx_data[9] << 8);
> + devinfo->num_usages = rx_data[10];
> + devinfo->silicon_revision = rx_data[11];
Opinions may vary here, but mine is that it is a waste of memory and time
to read and parse all of this data, only to print it at debug level. Unless
these variables are used elsewhere or reported to user space via sysfs, I
would drop all of this. It seems like cruft leftover from a vendor driver.
If you feel strongly about keeping these variables, then axiom_devinfo should
be defined as a packed struct to prevent having to disect rx_data[] byte by
byte. You should just read into &devinfo directly.
> +
> + dev_dbg(dev, " Boot Mode: %s\n", ts->devinfo.bootmode ? "BLP" : "TCP");
> + dev_dbg(dev, " Device ID : %04x\n", ts->devinfo.device_id);
> + dev_dbg(dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> + ts->devinfo.fw_minor);
> + dev_dbg(dev, " Bootloader Rev : %02x.%02x\n",
> + ts->devinfo.bootloader_fw_major,
> + ts->devinfo.bootloader_fw_minor);
> + dev_dbg(dev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> + dev_dbg(dev, " Silicon : %02x\n", ts->devinfo.jedec_id);
> + dev_dbg(dev, " Num Usages : %04x\n", ts->devinfo.num_usages);
> +
> + /* Read the second page of usage u31 to get the usage table */
> + ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
> + (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> + if (ret)
> + return ret;
> +
> + ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
> + dev_dbg(dev, "Max Report Length: %u\n", ts->max_report_len);
> +
> + return 0;
> +}
> +
> +/*
> + * Support function to axiom_process_u41_report.
> + * Generates input-subsystem events for every target.
> + * After calling this function the caller shall issue
> + * a Sync to the input sub-system.
> + */
> +static bool
> +axiom_process_u41_report_target(struct axiom_data *ts,
> + struct axiom_target_report *target)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + enum axiom_target_state current_state;
> + struct u41_target *target_prev_state;
> + struct device *dev = ts->dev;
> + bool update = false;
> + int slot;
> +
> + /* Verify the target index */
> + if (target->index >= AXIOM_U41_MAX_TARGETS) {
> + dev_dbg(dev, "Invalid target index! %u\n", target->index);
> + return false;
> + }
> +
> + target_prev_state = &ts->targets[target->index];
> +
> + current_state = TARGET_STATE_NOT_PRESENT;
> +
> + if (target->present) {
> + if (target->z >= 0)
> + current_state = TARGET_STATE_TOUCHING;
> + else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> + current_state = TARGET_STATE_HOVER;
> + else if (target->z AXIOM_PROX_LEVEL)
> + current_state = TARGET_STATE_PROX;
> + }
> +
> + if (target_prev_state->state == current_state &&
> + target_prev_state->x == target->x &&
> + target_prev_state->y == target->y &&
> + target_prev_state->z == target->z) {
> + return false;
> + }
> +
> + slot = target->index;
> +
> + dev_dbg(dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
> + target->index, slot, target->present,
> + target->x, target->y, target->z);
> +
> + switch (current_state) {
> + case TARGET_STATE_NOT_PRESENT:
> + case TARGET_STATE_PROX:
> + if (target_prev_state->insert)
> + break;
> + update = true;
> + target_prev_state->insert = false;
> + input_mt_slot(input_dev, slot);
> +
> + if (!slot)
> + input_report_key(input_dev, BTN_LEFT, 0);
> +
> + input_mt_report_slot_inactive(input_dev);
> + /*
> + * make sure the previous coordinates are
> + * all off screen when the finger comes back
> + */
> + target->x = 65535;
> + target->y = 65535;
> + target->z = AXIOM_PROX_LEVEL;
> + break;
> + case TARGET_STATE_HOVER:
> + case TARGET_STATE_TOUCHING:
> + target_prev_state->insert = true;
> + update = true;
> + input_mt_slot(input_dev, slot);
> + input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
> + input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> + input_report_abs(input_dev, ABS_X, target->x);
> + input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
> + input_report_abs(input_dev, ABS_Y, target->y);
> +
> + if (current_state == TARGET_STATE_TOUCHING) {
> + input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
> + input_report_abs(input_dev, ABS_DISTANCE, 0);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
> + input_report_abs(input_dev, ABS_PRESSURE, target->z);
> + } else {
> + input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
> + input_report_abs(input_dev, ABS_DISTANCE, -target->z);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
> + input_report_abs(input_dev, ABS_PRESSURE, 0);
> + }
> +
> + if (!slot)
> + input_report_key(input_dev, BTN_LEFT, (current_state ==
> + TARGET_STATE_TOUCHING));
> + break;
> + }
> +
> + target_prev_state->state = current_state;
> + target_prev_state->x = target->x;
> + target_prev_state->y = target->y;
> + target_prev_state->z = target->z;
> +
> + if (update)
> + input_mt_sync_frame(input_dev);
> +
> + return update;
> +}
> +
> +/*
> + * Take a raw buffer with u41 report data and decode it.
> + * Also generate input events if needed.
> + * rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
> + */
> +static void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + struct axiom_target_report target;
> + bool update_done = false;
> + u16 target_status;
> + u32 i;
> +
> + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
> +
> + for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> + char target_step = rx_buf[(i * 4)];
> +
> + target.index = i;
> + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> + target.x = ((target_step + 3) | ((target_step + 4) << 8));
> + target.y = ((target_step + 5) | ((target_step + 6) << 8));
> + target.z = (s8)(rx_buf[i + 43]);
> + update_done |= axiom_process_u41_report_target(ts, &target);
> + }
> +
> + if (update_done)
> + input_sync(input_dev);
> +}
> +
> +static void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + u32 event_value;
> + u16 aux_value;
> + u32 i = 0;
> +
> + for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
> + char target_step = rx_buf[(i * 2)];
> +
> + aux_value = (((target_step + 2) << 8) | (target_step + 1)) & AXIOM_U46_AUX_MASK;
> + event_value = (i << 16) | (aux_value);
> + input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> + }
> +
> + input_mt_sync(input_dev);
> + input_sync(input_dev);
> +}
Please forgive me in case I am simply slow to understand, but I really do
not think we can accept this kind of encapsulation. We have multiple calls
to input_mt_sync() and input_sync() spread across different functions, one
of which uses a 'done' flag to make a decision. It's also unclear what 'u41'
and 'u46' represent. The current implementation is too confusing to review
effectively IMO.
What we ultimately want to see here is one homogenous event handler where
MT slots are processed, followed by one call to input_mt_sync(), itself
followed by one call to input_sync() after any additional events (e.g. keys)
are processed. It's certainly OK to break out some processing into helper
functions, but we ultimately want to see one entry point into the input core.
Please consider whether there is a more maintainable way to organize this
processing; it seems more complex than other touchscreen drivers.
> +
> +/*
> + * Validates the crc and demultiplexes the axiom reports to the appropriate
> + * report handler
> + */
> +static void axiom_handle_events(struct axiom_data *ts)
> +{
> + char *report_data = ts->rx_buf;
> + struct device *dev = ts->dev;
> + char usage = report_data[1];
> + u16 crc_report;
> + u16 crc_calc;
> + char len;
> +
> + axiom_i2c_read(ts->client, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
If this read fails due to a HW problem, the rest of this function will act
upon garbage data.
> +
> + if ((report_data[0] & AXIOM_COMMS_OVERFLOW_MASK) != 0)
> + ts->report_overflow_counter++;
> +
> + len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) * 2;
> + if (!len) {
> + dev_err(dev, "Zero length report discarded.\n");
> + return;
Please make the return type of helper functions like axiom_handle_events() of
type int, and consider -ENODATA for this particular condition.
Even though callers to axiom_handle_events() are void at this time, you should
start out with the driver being flexible in case it grows over time.
> + }
> +
> + dev_dbg(dev, "Payload Data %*ph\n", len, report_data);
> +
> + /* Validate the report CRC */
> + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
> + /* Length is in 16 bit words and remove the size of the CRC16 itself */
> + crc_calc = crc16(0, report_data, (len - 2));
> +
> + if (crc_calc != crc_report) {
> + dev_err(dev,
> + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> + crc_report, crc_calc);
> + return;
Return -EINVAL after promoting the return type to int.
> + }
> +
> + switch (usage) {
> + case AXIOM_USAGE_2DCTS_REPORT_ID:
> + axiom_process_u41_report(ts, &report_data[1]);
> + break;
> +
> + case AXIOM_USAGE_2AUX_REPORT_ID:
> + /* This is an aux report (force) */
> + axiom_process_u46_report(ts, &report_data[1]);
> + break;
> +
> + case AXIOM_USAGE_2HB_REPORT_ID:
> + /* This is a heartbeat report */
> + break;
Since 'usage' is read from the HW, we need a default branch for handling
unexpected values.
> + }
> +
> + ts->report_counter++;
This counter appears to be unused.
> +}
> +
> +static void axiom_i2c_poll(struct input_dev *input_dev)
> +{
> + struct axiom_data *ts = input_get_drvdata(input_dev);
> +
> + axiom_handle_events(ts);
> +}
> +
> +static irqreturn_t axiom_irq(int irq, void *dev_id)
> +{
> + struct axiom_data *ts = dev_id;
> +
> + axiom_handle_events(ts);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void axiom_reset(struct gpio_desc *reset_gpio)
> +{
> + gpiod_set_value_cansleep(reset_gpio, 1);
> + usleep_range(1000, 2000);
> + gpiod_set_value_cansleep(reset_gpio, 0);
> + msleep(100);
> +}
> +
> +/* Rebaseline the touchscreen, effectively zero-ing it */
What does it mean to rebaseline the touchscreen? I'm guessing it means
to null out or normalize pressure? Please consider a less colloquialized
function name.
Out of curiousity, what happens if the user's hand happens to be on the
touch surface at the time you call axiom_rebaseline()? Does the device
recover on its own?
> +static int axiom_rebaseline(struct axiom_data *ts)
> +{
> + char buffer[8] = {};
Are you expecting each element to be initialized to zero?
> +
> + buffer[0] = AXIOM_REBASELINE_CMD;
> +
> + return axiom_i2c_write(ts->client, AXIOM_REPORT_USAGE_ID, 0, buffer, sizeof(buffer));
> +}
> +
> +static int axiom_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct input_dev *input_dev;
> + struct axiom_data *ts;
> + int ret;
> + int target;
> +
> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + ts->client = client;
> + i2c_set_clientdata(client, ts);
> + ts->dev = dev;
> +
> + ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
> + if (IS_ERR(ts->irq_gpio))
> + return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get irq GPIO");
> +
> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(ts->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
> +
> + axiom_reset(ts->reset_gpio);
We shouldn't call axiom_reset() if reset_gpio is NULL. Even though the
calls to gpiod_set_value_cansleep() will bail safely, there is no reason
to make the CPU sleep for 100 ms if the device was not actually reset.
> +
> + if (ts->irq_gpio) {
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + axiom_irq, 0, dev_name(dev), ts);
Did you mean to set IRQF_ONESHOT?
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to request threaded IRQ\n");
> + }
This is a kernel panic waiting to happen, as the interrupt handler (which can
post input events) is declared before the input device has been allocated.
Normally you want to set up interrupts last, after all resources have been
initialized and the HW has been configured.
> +
> + ret = axiom_discover(ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed touchscreen discover\n");
> +
> + ret = axiom_rebaseline(ts);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed touchscreen re-baselining\n");
> +
> + input_dev = devm_input_allocate_device(ts->dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + input_dev->name = "TouchNetix aXiom Touchscreen";
> + input_dev->phys = "input/axiom_ts";
> +
> + /* Single Touch */
> + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
You don't need to explicitly declare support for single-contact axes;
input_mt_init_slots() does this for us.
> +
> + /* Multi Touch */
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
> +
> + /* Registers the axiom device as a touchscreen instead of as a mouse pointer */
> + input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
Please check the return value of input_mt_init_slots().
> +
> + input_set_capability(input_dev, EV_KEY, BTN_LEFT);
Why to hard-code the key to BTN_LEFT as opposed to making it configurable via
device tree?
> +
> + /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> + set_bit(EV_REL, input_dev->evbit);
> + set_bit(EV_MSC, input_dev->evbit);
> + /* Declare that we support "RAW" Miscellaneous events */
> + set_bit(MSC_RAW, input_dev->mscbit);
The driver is not posting any REL events. Can you clarify what is represented
by MSC events?
> +
> + if (!ts->irq_gpio) {
> + ret = input_setup_polling(input_dev, axiom_i2c_poll);
> + if (ret)
> + return dev_err_probe(ts->dev, ret, "Unable to set up polling mode\n");
Nit: extraneous space.
> + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> + }
> +
> + ts->input_dev = input_dev;
> + input_set_drvdata(ts->input_dev, ts);
> +
> + /* Ensure that all reports are initialised to not be present. */
> + for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> + ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> +
> + ret = input_register_device(input_dev);
> +
Nit: unnecessary NL.
> + if (ret)
> + return dev_err_probe(ts->dev, ret,
> + "Could not register with Input Sub-system.\n");
Nit: alignment.
> +
> + return 0;
> +}
> +
> +static void axiom_i2c_remove(struct i2c_client *client)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> +
> + input_unregister_device(ts->input_dev);
> +}
This remove callback is unnecessary. So long as input_dev was allocated using
a device-managed function, it will be unregistered automatically.
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> + { "axiom-ax54a" },
> + {},
Nit: no need for a trailing comma after the sentinel, as no line would ever be
added beneath it.
> +};
> +
Nit: unnecessary NL.
> +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> +
> +static const struct of_device_id axiom_i2c_of_match[] = {
> + { .compatible = "touchnetix,axiom-ax54a", },
> + {}
> +};
> +
And here.
> +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> +
> +static struct i2c_driver axiom_i2c_driver = {
> + .driver = {
> + .name = "axiom",
> + .of_match_table = axiom_i2c_of_match,
> + },
> + .id_table = axiom_i2c_id_table,
> + .probe = axiom_i2c_probe,
> + .remove = axiom_i2c_remove,
> +};
> +
> +module_i2c_driver(axiom_i2c_driver);
And here.
> +
> +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
> +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
> +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
> +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("TouchNetix aXiom touchscreen I2C bus driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Kamel Bouhara @ 2023-10-22 19:35 UTC (permalink / raw)
To: Marco Felsch
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, linux-kernel, devicetree,
mark.satterthwaite, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo
In-Reply-To: <20231020120310.vrn6ew3fcg5e545w@pengutronix.de>
On Fri, Oct 20, 2023 at 02:03:10PM +0200, Marco Felsch wrote:
> Hi Kamel,
>
Hi Marco,
> just a rough review.
Thanks !
[...]
> > +#include <linux/crc16.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
>
> Can you please check if all headers are required e.g. sting.h
> seems a bit suspicious here.
Sure, slab.h and string.h are no more required.
>
> > +/*
> > + * Runtime TCP mode: device is executing normal code and is
> > + * accessible via the Touch Controller Mode
> > + */
> > +#define BOOT_TCP 0
> > +/*
> > + * Bootloader BLP mode: device is executing bootloader and is
> > + * accessible via the Boot Loader Protocol.
> > + */
> > +#define BOOT_BLP 1
>
> Both defines are not used.
>
Ack.
> > +#define AXIOM_PROX_LEVEL -128
> > +/*
> > + * Register group u31 has 2 pages for usage table entries.
> > + * (2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE = 85
i> > + */
> > +#define AXIOM_U31_MAX_USAGES 85
>
> The programmer's guid describe the usage as hexadecimal number prefixed
> with an 'u'. The current range is from u00 till uFF, so the max. usages
> should be 0xff.
Based one the above comment, it seems we are computing the byte size of
an usage array. I agree this might require to be more clear though.
>
> > +#define AXIOM_U31_BYTES_PER_USAGE 6
> > +#define AXIOM_U31_PAGE0_LENGTH 0x0C
> > +#define AXIOM_U31_BOOTMODE_MASK BIT(7)
> > +#define AXIOM_U31_FW_INFO_VARIANT_MASK GENMASK(6, 0)
> > +#define AXIOM_U31_FW_INFO_STATUS_MASK BIT(7)
> > +
> > +#define AXIOM_U41_MAX_TARGETS 10
> > +
> > +#define AXIOM_U46_AUX_CHANNELS 4
> > +#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
>
> I'm still not very happy with the decoding, since the so called
> 'protocol' is clear and versioned we can add the all required protocols
> as struct which has far less magic offsets.
Im not sure it will really make a significant difference as we actually
ihave a limited set of registers for the i2c driver, also could you
please clarify what protocol your refering to here ?
>
> > +
> > +#define AXIOM_COMMS_MAX_USAGE_PAGES 3
> > +#define AXIOM_COMMS_PAGE_SIZE 256
> > +#define AXIOM_COMMS_OVERFLOW_MASK BIT(7)
> > +#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(7, 0)
> > +
> > +#define AXIOM_REBASELINE_CMD 0x03
> > +
> > +#define AXIOM_REPORT_USAGE_ID 0x34
> > +#define AXIOM_DEVINFO_USAGE_ID 0x31
> > +#define AXIOM_USAGE_2HB_REPORT_ID 0x01
> > +#define AXIOM_REBASELINE_USAGE_ID 0x02
> > +#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
> > +#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
> > +
> > +#define AXIOM_PAGE_MASK GENMASK(15, 8)
>
> Unused
Ack thx.
[...]
> > +/*
> > + * Holds state of a touch or target when detected prior a touch (eg.
> > + * hover or proximity events).
> > + */
> > +enum axiom_target_state {
> > + TARGET_STATE_NOT_PRESENT = 0,
> > + TARGET_STATE_PROX = 1,
> > + TARGET_STATE_HOVER = 2,
> > + TARGET_STATE_TOUCHING = 3,
> > + TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
> > + TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
>
> STATE_MIN/MAX not used.
Ack.
>
> > +};
> > +
> > +struct u41_target {
> > + enum axiom_target_state state;
> > + u16 x;
> > + u16 y;
> > + s8 z;
> > + bool insert;
> > + bool touch;
> > +};
> > +
> > +struct axiom_target_report {
> > + u8 index;
> > + u8 present;
> > + u16 x;
> > + u16 y;
> > + s8 z;
> > +};
> > +
> > +struct axiom_cmd_header {
> > + u16 target_address;
> > + u16 length:15;
> > + u16 read:1;
> > +} __packed;
> > +
> > +struct axiom_data {
> > + struct axiom_devinfo devinfo;
> > + struct device *dev;
> > + struct gpio_desc *reset_gpio;
> > + struct gpio_desc *irq_gpio;
>
> No need to store the irq_gpio here.
>
Right, thanks.
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + u32 max_report_len;
> > + u32 report_overflow_counter;
> > + u32 report_counter;
> > + char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
> > + struct u41_target targets[AXIOM_U41_MAX_TARGETS];
> > + struct usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> > + bool usage_table_populated;
> > +};
> > +
> > +/*
> > + * aXiom devices are typically configured to report
> > + * touches at a rate of 100Hz (10ms). For systems
> > + * that require polling for reports, 100ms seems like
> > + * an acceptable polling rate.
> > + * When reports are polled, it will be expected to
> > + * occasionally observe the overflow bit being set
> > + * in the reports. This indicates that reports are not
> > + * being read fast enough.
> > + */
> > +#define POLL_INTERVAL_DEFAULT_MS 100
>
> Above you describe that the touch-rate is ~10ms why do we configure it
> 10-times lower here? Also 100ms is huge if you think about user respone
> time.
I am not completely sure aboud this yet, here 100ms is based on my own
*limited* experience, I agree we should stick to the 10ms.
>
> > +/* Translate usage/page/offset triplet into physical address. */
> > +static u16
> > +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> > + char offset)
> > +{
> > + struct axiom_devinfo *device_info;
> > + struct usage_entry *usage_table;
> > + u32 i;
> > +
> > + device_info = &ts->devinfo;
> > + usage_table = ts->usage_table;
> > +
> > + /* At the moment the convention is that u31 is always at physical address 0x0 */
> > + if (!ts->usage_table_populated) {
> > + if (usage == AXIOM_DEVINFO_USAGE_ID)
> > + return ((page << 8) + offset);
> > + else
> > + return 0xffff;
> > + }
> > +
> > + for (i = 0; i < device_info->num_usages; i++) {
> > + if (usage_table[i].id != usage)
> > + continue;
> > +
> > + if (page >= usage_table[i].num_pages) {
> > + dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> > + usage, page, offset);
> > + return 0xffff;
> > + }
> > + }
>
> We can avoid this loop if we store the usage table exactly as it is,
> e.g.:
>
> usage_table[0x31] = u31;
> usage_table[0x41] = u41;
>
Could you please explain your idea ?
> > + return ((usage_table[i].start_page + page) << 8) + offset;
> > +}
> > +
> > +static int
> > +axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > + struct axiom_cmd_header cmd_header;
> > + struct i2c_msg msg[2];
> > + int ret;
> > +
> > + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > + cmd_header.length = cpu_to_le16(len);
> > + cmd_header.read = 1;
> > +
> > + msg[0].addr = client->addr;
> > + msg[0].flags = 0;
> > + msg[0].len = sizeof(cmd_header);
> > + msg[0].buf = (u8 *)&cmd_header;
> > +
> > + msg[1].addr = client->addr;
> > + msg[1].flags = I2C_M_RD;
> > + msg[1].len = len;
> > + msg[1].buf = (char *)buf;
> > +
> > + ret = i2c_transfer(client->adapter, msg, 2);
> > + if (ret != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev,
> > + "Failed reading usage %#x page %#x, error=%d\n",
> > + usage, page, ret);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > + struct axiom_cmd_header cmd_header;
> > + struct i2c_msg msg[2];
> > + int ret;
> > +
> > + cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > + cmd_header.length = cpu_to_le16(len);
> > + cmd_header.read = 0;
> > +
> > + msg[0].addr = client->addr;
> > + msg[0].flags = 0;
> > + msg[0].len = sizeof(cmd_header);
> > + msg[0].buf = (u8 *)&cmd_header;
> > +
> > + msg[1].addr = client->addr;
> > + msg[1].flags = 0;
> > + msg[1].len = len;
> > + msg[1].buf = (char *)buf;
>
> Please check the "comms protocol app note", for write it is not allowed
> to send a stop, so the whole data must be send in one i2c_msg.
>
Well I only have the "Programmer's Guide", I'll have to check that as it
really seems to works as it yet.
> > +
> > + ret = i2c_transfer(client->adapter, msg, 2);
> > + if (ret < 0) {
> > + dev_err(&client->dev,
> > + "Failed to write usage %#x page %#x, error=%d\n", usage,
> > + page, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Decodes and populates the local Usage Table.
> > + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> > + * device.
> > + */
> > +static u32 axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> > +{
> > + u32 usage_id = 0;
> > + u32 max_report_len = 0;
> > + struct axiom_devinfo *device_info;
> > + struct usage_entry *usage_table;
> > +
> > + device_info = &ts->devinfo;
> > + usage_table = ts->usage_table;
> > +
> > + for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> > + u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> > + char id = rx_data[offset + 0];
> > + char start_page = rx_data[offset + 1];
> > + char num_pages = rx_data[offset + 2];
> > + u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> > +
> > + if (!num_pages)
> > + usage_table[usage_id].is_report = true;
> > +
> > + /* Store the entry into the usage table */
> > + usage_table[usage_id].id = id;
> > + usage_table[usage_id].start_page = start_page;
> > + usage_table[usage_id].num_pages = num_pages;
> > +
> > + dev_dbg(ts->dev, "Usage %2u Info: %*ph\n", usage_id,
> > + AXIOM_U31_BYTES_PER_USAGE,
> > + &rx_data[offset]);
> > +
> > + /* Identify the max report length the module will receive */
> > + if (usage_table[usage_id].is_report && max_offset > max_report_len)
> > + max_report_len = max_offset;
> > + }
>
> As said, the sorting can be really easy:
>
> usage_table[0x01] = u01;
> usage_table[0x31] = u31;
>
I still don't get your point here.
> > + ts->usage_table_populated = true;
> > +
> > + return max_report_len;
> > +}
> > +
[...]
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct input_dev *input_dev;
> > + struct axiom_data *ts;
> > + int ret;
> > + int target;
> > +
> > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->client = client;
> > + i2c_set_clientdata(client, ts);
> > + ts->dev = dev;
> > +
> > + ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
> > + if (IS_ERR(ts->irq_gpio))
> > + return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get irq GPIO");
>
> Nope you removed this from the binding.
Yes, will be fixed in v4.
>
> > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(ts->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
>
> Please also add a regulator for the VDDI/VDDA which is required for the
> device to work properly.
>
Right, Im using the AX54 EV board with fixed regulators.
> > + axiom_reset(ts->reset_gpio);
> > +
> > + if (ts->irq_gpio) {
>
> Nope, please drop the ts->irq_gpio check.
Ack.
>
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + axiom_irq, 0, dev_name(dev), ts);
> > + if (ret < 0)
>
> If the threaded_irq does fail you can fallback to the polling mode.
Indeed.
>
> > + return dev_err_probe(dev, ret, "Failed to request threaded IRQ\n");
> > + }
> > +
> > + ret = axiom_discover(ts);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed touchscreen discover\n");
> > +
> > + ret = axiom_rebaseline(ts);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed touchscreen re-baselining\n");
> > +
> > + input_dev = devm_input_allocate_device(ts->dev);
> > + if (!input_dev)
> > + return -ENOMEM;
> > +
> > + input_dev->name = "TouchNetix aXiom Touchscreen";
> > + input_dev->phys = "input/axiom_ts";
> > +
> > + /* Single Touch */
> > + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
> > +
> > + /* Multi Touch */
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
> > +
> > + /* Registers the axiom device as a touchscreen instead of as a mouse pointer */
> > + input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> > +
> > + input_set_capability(input_dev, EV_KEY, BTN_LEFT);
> > +
> > + /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> > + set_bit(EV_REL, input_dev->evbit);
> > + set_bit(EV_MSC, input_dev->evbit);
> > + /* Declare that we support "RAW" Miscellaneous events */
> > + set_bit(MSC_RAW, input_dev->mscbit);
> > +
> > + if (!ts->irq_gpio) {
> > + ret = input_setup_polling(input_dev, axiom_i2c_poll);
> > + if (ret)
> > + return dev_err_probe(ts->dev, ret, "Unable to set up polling mode\n");
> > + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > + }
> > +
> > + ts->input_dev = input_dev;
> > + input_set_drvdata(ts->input_dev, ts);
> > +
> > + /* Ensure that all reports are initialised to not be present. */
> > + for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> > + ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> > +
> > + ret = input_register_device(input_dev);
> > +
> > + if (ret)
> > + return dev_err_probe(ts->dev, ret,
> > + "Could not register with Input Sub-system.\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void axiom_i2c_remove(struct i2c_client *client)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > +
> > + input_unregister_device(ts->input_dev);
>
> This can be part of devm_add_action_or_reset() and we could remove the
> .remove() callback for this driver.
>
Sure, thanks for the tips :)!
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > + { "axiom-ax54a" },
>
> Albeit the datasheet says: "axiom ax54a" I think ax stands for axiom. So
> the name should be "ax54a" only?
Yes this is actually a good point, we can move to ax54a only.
>
> > + {},
>
> Nit: { },
> > +};
> > +
>
> Please drop the unnecessary newline.
>
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_of_match[] = {
> > + { .compatible = "touchnetix,axiom-ax54a", },
> > + {}
>
> same here.
>
> > +};
> > +
>
> same here.
>
> > +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> > +
> > +static struct i2c_driver axiom_i2c_driver = {
> > + .driver = {
> > + .name = "axiom",
> > + .of_match_table = axiom_i2c_of_match,
> > + },
> > + .id_table = axiom_i2c_id_table,
> > + .probe = axiom_i2c_probe,
> > + .remove = axiom_i2c_remove,
> > +};
> > +
>
> same here.
>
OK.
> > +module_i2c_driver(axiom_i2c_driver);
> > +
> > +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
> > +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
> > +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
> > +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("TouchNetix aXiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >
> >
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-10-22 18:49 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Christophe JAILLET, Jonathan Albrieux
In-Reply-To: <ZTVoiklUJaDn5576@nixie71>
Hi Jeff,
Thanks a lot for reviewing this again!
On Sun, Oct 22, 2023 at 01:23:06PM -0500, Jeff LaBundy wrote:
> Hi Stephan,
>
> This looks great, just a few remaining comments.
>
> On Sat, Sep 30, 2023 at 05:32:01PM +0200, Stephan Gerhold wrote:
> > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> >
> > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > with support for multi-touch and capacitive touch keys.
> >
> > The driver is somewhat based on sample code from Himax. However, that
> > code was so extremely confusing that we spent a significant amount of
> > time just trying to understand the packet format and register commands.
> > In this driver they are described with clean structs and defines rather
> > than lots of magic numbers and offset calculations.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 10 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/himax_hx852x.c | 499 +++++++++++++++++++++++++++++++
> > 4 files changed, 517 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90f13281d297..c551c60b0598 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9331,6 +9331,13 @@ S: Maintained
> > F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > F: drivers/input/touchscreen/himax_hx83112b.c
> >
> > +HIMAX HX852X TOUCHSCREEN DRIVER
> > +M: Stephan Gerhold <stephan@gerhold.net>
> > +L: linux-input@vger.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> > +F: drivers/input/touchscreen/himax_hx852x.c
> > +
> > HIPPI
> > M: Jes Sorensen <jes@trained-monkey.org>
> > L: linux-hippi@sunsite.dk
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index e3e2324547b9..8e5667ae5dab 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> > To compile this driver as a module, choose M here : the
> > module will be called hideep_ts.
> >
> > +config TOUCHSCREEN_HIMAX_HX852X
> > + tristate "Himax HX852x(ES) touchscreen"
> > + depends on I2C
> > + help
> > + Say Y here if you have a Himax HX852x(ES) touchscreen.
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called himax_hx852x.
> > +
> > config TOUCHSCREEN_HYCON_HY46XX
> > tristate "Hycon hy46xx touchscreen support"
> > depends on I2C
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..f42a87faa86c 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> > obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> > obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> > diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> > new file mode 100644
> > index 000000000000..98a55be7891d
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/himax_hx852x.c
> > @@ -0,0 +1,499 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Himax HX852x(ES) Touchscreen Driver
> > + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> > + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > + *
> > + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> > + * Copyright (c) 2014 Himax Corporation.
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> > +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> > +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> > + HX852X_WIDTH_SIZE(fingers) + \
> > + sizeof(struct hx852x_touch_info))
> > +
> > +#define HX852X_MAX_FINGERS 12
> > +#define HX852X_MAX_KEY_COUNT 4
> > +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> > +
> > +#define HX852X_TS_SLEEP_IN 0x80
> > +#define HX852X_TS_SLEEP_OUT 0x81
> > +#define HX852X_TS_SENSE_OFF 0x82
> > +#define HX852X_TS_SENSE_ON 0x83
> > +#define HX852X_READ_ONE_EVENT 0x85
> > +#define HX852X_READ_ALL_EVENTS 0x86
> > +#define HX852X_READ_LATEST_EVENT 0x87
> > +#define HX852X_CLEAR_EVENT_STACK 0x88
> > +
> > +#define HX852X_REG_SRAM_SWITCH 0x8c
> > +#define HX852X_REG_SRAM_ADDR 0x8b
> > +#define HX852X_REG_FLASH_RPLACE 0x5a
> > +
> > +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> > +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> > +
> > +struct hx852x {
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + struct touchscreen_properties props;
> > + struct gpio_desc *reset_gpiod;
> > + struct regulator_bulk_data supplies[2];
> > + unsigned int max_fingers;
> > + unsigned int keycount;
> > + unsigned int keycodes[HX852X_MAX_KEY_COUNT];
> > +};
> > +
> > +struct hx852x_config {
> > + u8 rx_num;
> > + u8 tx_num;
> > + u8 max_pt;
> > + u8 padding1[3];
> > + __be16 x_res;
> > + __be16 y_res;
> > + u8 padding2[2];
> > +} __packed __aligned(4);
> > +
> > +struct hx852x_coord {
> > + __be16 x;
> > + __be16 y;
> > +} __packed __aligned(4);
> > +
> > +struct hx852x_touch_info {
> > + u8 finger_num;
> > + __le16 finger_pressed;
> > + u8 padding;
> > +} __packed __aligned(4);
> > +
> > +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> > +{
> > + struct i2c_client *client = hx->client;
> > + int ret;
> > +
> > + struct i2c_msg msg[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = 1,
> > + .buf = &cmd,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .len = len,
> > + .buf = data,
> > + }
>
> Nit: standard practice is to close all array elements with a trailing comma,
> even the final element, in order to avoid an extra line in the diff (i.e.
> } --> },) if another element is added later. Obviously we wouldn't add more
> elements to this array in the future, but it's best to remain consistent.
>
Sure, I'll fix this in v3!
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > + if (ret != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_power_on(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error) {
> > + dev_err(dev, "failed to enable regulators: %d\n", error);
> > + return error;
> > + }
> > +
> > + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> > + msleep(20);
> > + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> > + msleep(20);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_start(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> > + return error;
> > + }
> > + msleep(30);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> > + return error;
> > + }
> > + msleep(20);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_stop(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
> > + return error;
> > + }
> > + msleep(20);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
> > + return error;
> > + }
> > + msleep(30);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_power_off(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + dev_err(dev, "failed to disable regulators: %d\n", error);
> > + return error;
> > +}
> > +
> > +static int hx852x_read_config(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + struct hx852x_config conf;
> > + int x_res, y_res;
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + /* Sensing must be turned on briefly to load the config */
> > + error = hx852x_start(hx);
> > + if (error)
> > + goto err_power_off;
> > +
> > + error = hx852x_stop(hx);
> > + if (error)
> > + goto err_power_off;
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > + HX852X_SRAM_SWITCH_TEST_MODE);
> > + if (error)
> > + goto err_power_off;
> > +
> > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > + HX852X_SRAM_ADDR_CONFIG);
> > + if (error)
> > + goto err_test_mode;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > + if (error)
> > + goto err_test_mode;
> > +
> > + x_res = be16_to_cpu(conf.x_res);
> > + y_res = be16_to_cpu(conf.y_res);
> > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> > + x_res, y_res, hx->max_fingers);
> > +
> > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > + dev_err(dev, "max supported fingers: %u, found: %u\n",
> > + HX852X_MAX_FINGERS, hx->max_fingers);
> > + error = -EINVAL;
> > + goto err_test_mode;
> > + }
> > +
> > + if (x_res && y_res) {
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > + }
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > + if (error)
> > + goto err_power_off;
> > +
> > + return hx852x_power_off(hx);
> > +
> > +err_test_mode:
> > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > +err_power_off:
> > + hx852x_power_off(hx);
> > + return error;
>
> Your new version is an improvement, but maybe we can remove duplicate
> code by introducing some helper variables:
>
> int error, error2 = 0, error3;
>
> /* ... */
>
> err_test_mode:
> error2 = i2c_smbus_write_byte_data(...);
>
> err_power_off:
> error3 = hx852x_power_off(...);
>
> if (error)
> return error;
>
> return error2 ? : error3;
>
> In this case we achieve our goal of attempting to return the device to a
> safe state in both passing and failing cases. In the event of multiple
> errors, we return the first to occur.
>
Right, this would work as well. Personally I think my solution is
slightly easier to read though. In your version my eyes somewhat
"stumble" over the multiple "error" variables and then about the purpose
of the "? : " construction. This is probably highly subjective. :-)
Do you mind if keep this as-is? I don't have a strong opinion about
this, but a slight preference for my current solution.
> > +}
> > +
> > +static int hx852x_handle_events(struct hx852x *hx)
> > +{
> > + /*
> > + * The event packets have variable size, depending on the amount of
> > + * supported fingers (hx->max_fingers). They are laid out as follows:
> > + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> > + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> > + * with padding for 32-bit alignment
> > + * - struct hx852x_touch_info
> > + *
> > + * Load everything into a 32-bit aligned buffer so the coordinates
> > + * can be assigned directly, without using get_unaligned_*().
> > + */
> > + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> > + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> > + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> > + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> > + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> > + unsigned long finger_pressed, key_pressed;
> > + unsigned int i, x, y, w;
> > + int error;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> > + HX852X_BUF_SIZE(hx->max_fingers));
> > + if (error)
> > + return error;
> > +
> > + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> > + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> > +
> > + /* All bits are set when no touch is detected */
> > + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> > + finger_pressed = 0;
> > + if (key_pressed == 0xf)
> > + key_pressed = 0;
> > +
> > + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> > + x = be16_to_cpu(coord[i].x);
> > + y = be16_to_cpu(coord[i].y);
> > + w = width[i];
> > +
> > + input_mt_slot(hx->input_dev, i);
> > + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> > + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> > + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> > + }
> > + input_mt_sync_frame(hx->input_dev);
> > +
> > + for (i = 0; i < hx->keycount; i++)
> > + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> > +
> > + input_sync(hx->input_dev);
> > + return 0;
> > +}
> > +
> > +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> > +{
> > + struct hx852x *hx = ptr;
> > + int error;
> > +
> > + error = hx852x_handle_events(hx);
> > + if (error) {
> > + dev_err_ratelimited(&hx->client->dev, "failed to handle events: %d\n", error);
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int hx852x_input_open(struct input_dev *dev)
> > +{
> > + struct hx852x *hx = input_get_drvdata(dev);
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + error = hx852x_start(hx);
> > + if (error) {
> > + hx852x_power_off(hx);
> > + return error;
> > + }
> > +
> > + enable_irq(hx->client->irq);
> > + return 0;
> > +}
> > +
> > +static void hx852x_input_close(struct input_dev *dev)
> > +{
> > + struct hx852x *hx = input_get_drvdata(dev);
> > +
> > + hx852x_stop(hx);
> > + disable_irq(hx->client->irq);
> > + hx852x_power_off(hx);
> > +}
> > +
> > +static int hx852x_parse_properties(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + hx->keycount = device_property_count_u32(dev, "linux,keycodes");
> > + if (hx->keycount <= 0) {
> > + hx->keycount = 0;
> > + return 0;
> > + }
>
> With negative return values of device_property_count_u32() squashed in this
> way, there is no way to warn the developer that the array is invalid. The
> error message associated with device_property_read_u32_array() below is
> essentially dead code, because the latter is a wrapper around the former,
> and any syntax error that causes one to fail will cause the other to fail.
>
> Since device_property_count_u32() returns -EINVAL in case of a missing array,
> we can do the following. Note there is no need to explicitly set hx->keycount
> to zero in case no keys are specified, since the hx structure is correctly
> allocated using devm_kzalloc().
>
> int count;
>
> count = device_property_count_u32(...);
> if (count == -EINVAL) {
> return 0;
> } else if (count < 0) {
> dev_err(...);
> return count;
> } else if (count > HX852X_MAX_KEY_COUNT) {
> dev_err(...);
> return -EINVAL;
> }
>
> hx->keycount = count;
>
> error = device_property_read_u32_array(...);
> if (error)
> dev_err(...);
>
> return error;
>
Ah right, thanks for spotting this! Will use this in v3.
> > +
> > + if (hx->keycount > HX852X_MAX_KEY_COUNT) {
> > + dev_err(dev, "max supported keys: %u, found: %u\n",
> > + HX852X_MAX_KEY_COUNT, hx->keycount);
> > + return -EINVAL;
> > + }
> > +
> > + error = device_property_read_u32_array(dev, "linux,keycodes",
> > + hx->keycodes, hx->keycount);
> > + if (error)
> > + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> > +
> > + return error;
> > +}
> > +
> > +static int hx852x_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct hx852x *hx;
> > + int error, i;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > + dev_err(dev, "not all required i2c functionality supported\n");
> > + return -ENXIO;
> > + }
> > +
> > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > + if (!hx)
> > + return -ENOMEM;
> > +
> > + hx->client = client;
> > + hx->input_dev = devm_input_allocate_device(dev);
> > + if (!hx->input_dev)
> > + return -ENOMEM;
> > +
> > + hx->input_dev->name = "Himax HX852x";
> > + hx->input_dev->id.bustype = BUS_I2C;
> > + hx->input_dev->open = hx852x_input_open;
> > + hx->input_dev->close = hx852x_input_close;
> > +
> > + i2c_set_clientdata(client, hx);
> > + input_set_drvdata(hx->input_dev, hx);
> > +
> > + hx->supplies[0].supply = "vcca";
> > + hx->supplies[1].supply = "vccd";
> > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to get regulators\n");
> > +
> > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(hx->reset_gpiod))
> > + return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
> > + "failed to get reset gpio\n");
> > +
> > + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> > + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
> > +
> > + error = hx852x_read_config(hx);
> > + if (error)
> > + return error;
> > +
> > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> > + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > +
> > + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> > + error = hx852x_parse_properties(hx);
> > + if (error)
> > + return error;
> > +
> > + hx->input_dev->keycode = hx->keycodes;
> > + hx->input_dev->keycodemax = hx->keycount;
> > + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> > + for (i = 0; i < hx->keycount; i++)
> > + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> > +
> > + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> > + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to init MT slots\n");
> > +
> > + error = input_register_device(hx->input_dev);
> > + if (error)
> > + return dev_err_probe(dev, error, "failed to register input device\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_suspend(struct device *dev)
> > +{
> > + struct hx852x *hx = dev_get_drvdata(dev);
>
> int error = 0;
>
> > +
> > + mutex_lock(&hx->input_dev->mutex);
> > + if (input_device_enabled(hx->input_dev))
> > + hx852x_stop(hx);
>
> error = hx852x_stop(...);
>
> > + mutex_unlock(&hx->input_dev->mutex);
> > +
> > + return 0;
>
> return error;
>
Oops. I meant to change this in v2 but forgot it apparently.
Will fix fix this in v3 as well!
I'll wait for your reply regarding the error handling in
hx852x_read_config(), in case you have a strong opinion about that. :)
Thanks!
Stephan
^ 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