[PATCH v16 0/3] Input: Add TouchNetix axiom touchscreen driver

[PATCH v16 0/3] Input: Add TouchNetix axiom touchscreen driver

[Kamel Bouhara]

Add a new driver for the TouchNetix's axiom family of touchscreen
controller. This driver only support i2c and can be later adapted for
SPI and USB support.
--
Changes in v16:
 - Fix bug report: https://lore.kernel.org/oe-kbuild-all/202406290709.X77lx18x-lkp@intel.com/
 - Remove leica from list of Maintainers
 - Rebase on v6.10-rc6

Changes in v15:
 - Fix report ABS_MT_TRACKING_ID after input_mt_slot

Changes in v14:
 - Fix is_report/present as booleans
 - Add a comment to describe the boolean variable insert
 - Remove not needed packed attributed from struct axiom_cmd_header
 - Remove duplicate target event checking
 - Only emit ABS_MT_DISTANCE/ABS_MT_PRESSURE
 - Remove input_report_key() as a mean to indicate firs reported contact
 - Remove EV_REL only need for u46 aka debug data
 - Report only MT_TOOL_FINGER instead of MT_TOOL_MAX
 - Remove unused EV_REL
 - Remove emit BTN_TOUCH as a way to indicate first target report
 - Remove unjustified delay after read access
 - Remove falling back to polling mode when no irq acquired in ACPI/DT
 - Remove handle of u46 report as it's not used
 - Remove touchscreen_report_pos() call as it generates extra dummy events
 - Rebase on v6.10-rc5

Changes in v13:
 - Fix CRC16 not selected reported in:https://lore.kernel.org/oe-kbuild-all/202405311035.5QZSREJv-lkp@intel.com/
 - Rebase on v6.10-rc2

Changes in v12:
 - Fix REGMAP_I2C not selected
 - Rebase on v6.10-rc1

Changes in v11:
 - Fix regulators name to match dt-binding
 - Enable regulators before reset is asserted

Changes in v10:
 - Set regulators as required
 - Enable power supply before reset
 - Fix ref count due to regulator requested twice
 - Rebase on v6.9-rc4

Changes in v9:
 - Fix issue reported in https://lore.kernel.org/oe-kbuild-all/202402201157.BKo97uWl-lkp@intel.com/
 - Rebase on v6.8-rc2

Changes in v8:
 - Fix missing call to input_report_slot_state()
 - Fix issue reported in https://lore.kernel.org/oe-kbuild-all/202402020623.8T1Ah513-lkp@intel.com/

Changes in v7:
 - Remove startup time from dt-binding
 - Fix usage table not correctly populated

Changes in v6:
 - Fix missing unevaluatedProperties.in dt-binding
 - Use __le16 to correctly deal with device endianness
 - Use standart kernel types s/char/u8/
 - Use regmap api as driver might support spi later
 - Use get_unaligned_le16() for the sake of clarity
 - Use devm_regulator_enable_optional()

Changes in v5:
 - Fix wrong message constructed in axiom_i2c_read
 - Delay required between i2c reads is >= 250us
 - Do not split report reading in two phases as we'll
   have to wait 500us
 - Use lower-case in properties names
 - Make regulators properties are required in dt-binding
 - Fix bug report: https://lore.kernel.org/lkml/202312051457.y3N1q3sZ-lkp@intel.com/
 - Fix bug report: https://lore.kernel.org/lkml/6f8e3b64-5b21-4a50-8680-063ef7a93bdb@suswa.mountain/

Changes in v4:
 - Cleanup unused headers and macros
 - Use standard kernel type
 - Namespace structures and functions
 - Use packed struct when possible to avoid bitfield operators
 - Fix missing break when address is found in axiom_populate_target_address()
 - Split reads in two steps for the reports, first length then report
   itself so we only read required bytes
 - Get poll-interval from devicetree
 - Add VDDI/VDDA regulators
 - Add a startup delay of 110 ms required after VDDA/VDDI is applied
 - Remove axiom_i2c_write() as it is no more used

Changes in v3:
 - Remove irq-gpios property in dt-binding
 - Use a generic node name
 - Fix issues reported in https://lore.kernel.org/oe-kbuild-all/202310100300.oAC2M62R-lkp@intel.com/

Changes in v2:
 - Add device tree binding documentation
 - Move core functions in axiom_i2c as we only care about i2c support now
 - Use static function when required
 - Use syntax dev_err_probe()
 - Add an hardware based reset

Kamel Bouhara (3):
  dt-bindings: vendor-prefixes: Add TouchNetix AS
  dt-bindings: input: Add TouchNetix axiom touchscreen
  Input: Add TouchNetix axiom i2c touchscreen driver

 .../input/touchscreen/touchnetix,ax54a.yaml   |  62 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   7 +
 drivers/input/touchscreen/Kconfig             |  14 +
 drivers/input/touchscreen/Makefile            |   1 +
 drivers/input/touchscreen/touchnetix_axiom.c  | 616 ++++++++++++++++++
 6 files changed, 702 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
 create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c

--
2.25.1

[PATCH v16 1/3] dt-bindings: vendor-prefixes: Add TouchNetix AS

[Kamel Bouhara]

Add vendor prefix for TouchNetix AS (https://www.touchnetix.com/products/).

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index fbf47f0bacf1..f102b3abf3fc 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1489,6 +1489,8 @@ patternProperties:
     description: Toradex AG
   "^toshiba,.*":
     description: Toshiba Corporation
+  "^touchnetix,.*":
+    description: TouchNetix AS
   "^toumaz,.*":
     description: Toumaz
   "^tpk,.*":
-- 
2.25.1

[PATCH v16 2/3] dt-bindings: input: Add TouchNetix axiom touchscreen

[Kamel Bouhara]

Add the TouchNetix axiom I2C touchscreen device tree bindings
documentation.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../input/touchscreen/touchnetix,ax54a.yaml   | 62 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
new file mode 100644
index 000000000000..66229a4d6f15
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/touchnetix,ax54a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TouchNetix Axiom series touchscreen controller
+
+maintainers:
+  - Kamel Bouhara <kamel.bouhara@bootlin.com>
+
+allOf:
+  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+  - $ref: /schemas/input/input.yaml#
+
+properties:
+  compatible:
+    const: touchnetix,ax54a
+
+  reg:
+    const: 0x66
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  vdda-supply:
+    description: Analog power supply regulator on VDDA pin
+
+  vddi-supply:
+    description: I/O power supply regulator on VDDI pin
+
+required:
+  - compatible
+  - reg
+  - vdda-supply
+  - vddi-supply
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@66 {
+        compatible = "touchnetix,ax54a";
+        reg = <0x66>;
+        interrupt-parent = <&gpio2>;
+        interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+        reset-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+        vdda-supply = <&vdda_reg>;
+        vddi-supply = <&vddi_reg>;
+        poll-interval = <20>;
+      };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index cd2ca0c3158e..2041384d3866 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22740,6 +22740,12 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/toshiba-wmi.c
 
+TOUCHNETIX AXIOM I2C TOUCHSCREEN DRIVER
+M:	Kamel Bouhara <kamel.bouhara@bootlin.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
+
 TPM DEVICE DRIVER
 M:	Peter Huewe <peterhuewe@gmx.de>
 M:	Jarkko Sakkinen <jarkko@kernel.org>
-- 
2.25.1

[PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Kamel Bouhara]

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            |  14 +
 drivers/input/touchscreen/Makefile           |   1 +
 drivers/input/touchscreen/touchnetix_axiom.c | 616 +++++++++++++++++++
 4 files changed, 632 insertions(+)
 create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2041384d3866..ac6b612bfbba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22745,6 +22745,7 @@ M:	Kamel Bouhara <kamel.bouhara@bootlin.com>
 L:	linux-input@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
+F:	drivers/input/touchscreen/touchnetix_axiom.c
 
 TPM DEVICE DRIVER
 M:	Peter Huewe <peterhuewe@gmx.de>
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index c821fe3ee794..1ce8f1c25625 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -834,6 +834,20 @@ config TOUCHSCREEN_MIGOR
 	  To compile this driver as a module, choose M here: the
 	  module will be called migor_ts.
 
+config TOUCHSCREEN_TOUCHNETIX_AXIOM
+	tristate "TouchNetix AXIOM based touchscreen controllers"
+	depends on I2C
+	select CRC16
+	select REGMAP_I2C
+	help
+	  Say Y here if you have a axiom touchscreen connected to
+	  your system.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called axiom.
+
 config TOUCHSCREEN_TOUCHRIGHT
 	tristate "Touchright serial touchscreen"
 	select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index a81cb5aa21a5..6ce7b804adc7 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
 obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI)	+= surface3_spi.o
 obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
+obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM)	+= touchnetix_axiom.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
 obj-$(CONFIG_TOUCHSCREEN_TS4800)	+= ts4800-ts.o
diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
new file mode 100644
index 000000000000..cea52dafc913
--- /dev/null
+++ b/drivers/input/touchscreen/touchnetix_axiom.c
@@ -0,0 +1,616 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TouchNetix axiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Bart Prescott <bartp@baasheep.co.uk>
+ *            Pedro Torruella <pedro.torruella@touchnetix.com>
+ *            Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
+ *            Hannah Rossiter <hannah.rossiter@touchnetix.com>
+ *            Kamel Bouhara <kamel.bouhara@bootlin.com>
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/crc16.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+
+#include <asm/unaligned.h>
+#define AXIOM_PROX_LEVEL		-128
+#define AXIOM_STARTUP_TIME_MS		110
+/*
+ * Register group u31 has 2 pages for usage table entries.
+ */
+#define AXIOM_U31_MAX_USAGES		0xff
+#define AXIOM_U31_BYTES_PER_USAGE	6
+#define AXIOM_U31_PAGE0_LENGTH		0x0C
+#define AXIOM_U31_BOOTMODE_MASK		BIT(7)
+#define AXIOM_U31_DEVID_MASK		GENMASK(14, 0)
+
+#define AXIOM_MAX_REPORT_LEN		0x7f
+
+#define AXIOM_CMD_HEADER_READ_MASK	BIT(15)
+#define AXIOM_U41_MAX_TARGETS		10
+
+#define AXIOM_U46_AUX_CHANNELS		4
+#define AXIOM_U46_AUX_MASK		GENMASK(11, 0)
+
+#define AXIOM_COMMS_MAX_USAGE_PAGES	3
+#define AXIOM_COMMS_PAGE_SIZE		256
+#define AXIOM_COMMS_REPORT_LEN_MASK	GENMASK(6, 0)
+
+#define AXIOM_REPORT_USAGE_ID		0x34
+#define AXIOM_DEVINFO_USAGE_ID		0x31
+#define AXIOM_USAGE_2HB_REPORT_ID	0x01
+#define AXIOM_USAGE_2AUX_REPORT_ID	0x46
+#define AXIOM_USAGE_2DCTS_REPORT_ID	0x41
+
+#define AXIOM_PAGE_OFFSET_MASK		GENMASK(6, 0)
+
+struct axiom_devinfo {
+	__le16 device_id;
+	u8 fw_minor;
+	u8 fw_major;
+	u8 fw_info_extra;
+	u8 tcp_revision;
+	u8 bootloader_fw_minor;
+	u8 bootloader_fw_major;
+	__le16 jedec_id;
+	u8 num_usages;
+} __packed;
+
+/*
+ * Describes parameters of a specific usage, essentially a single element of
+ * the "Usage Table"
+ */
+struct axiom_usage_entry {
+	u8 id;
+	bool is_report;
+	u8 start_page;
+	u8 num_pages;
+};
+
+/*
+ * Represents state of a touch or target when detected prior to a touch (eg.
+ * hover or proximity events).
+ */
+enum axiom_target_state {
+	AXIOM_TARGET_STATE_NOT_PRESENT = 0,
+	AXIOM_TARGET_STATE_PROX = 1,
+	AXIOM_TARGET_STATE_HOVER = 2,
+	AXIOM_TARGET_STATE_TOUCHING = 3,
+};
+
+struct axiom_u41_target {
+	enum axiom_target_state state;
+	u16 x;
+	u16 y;
+	s8 z;
+	/* force target report processing */
+	bool insert;
+	bool touch;
+};
+
+struct axiom_target_report {
+	u8 index;
+	bool present;
+	u16 x;
+	u16 y;
+	s8 z;
+};
+
+union axiom_cmd_header {
+	__le32 preamble;
+	struct {
+		__le16 target_address;
+		__le16 length;
+	} head;
+};
+
+struct axiom_data {
+	struct axiom_devinfo devinfo;
+	struct device *dev;
+	struct gpio_desc *reset_gpio;
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	u32 max_report_len;
+	u8 rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
+	struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
+	struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
+	bool usage_table_populated;
+	struct regmap *regmap;
+	struct touchscreen_properties	prop;
+};
+
+static const struct regmap_config axiom_i2c_regmap_config = {
+	.reg_bits = 32,
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	.val_bits = 8,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+/*
+ * axiom devices are typically configured to report touches at a rate
+ * of 100Hz (10ms) for systems that require polling for reports.
+ * When reports are polled, it will be expected to occasionally
+ * observe the overflow bit being set in the reports.
+ * This indicates that reports are not being read fast enough.
+ */
+#define POLL_INTERVAL_DEFAULT_MS 10
+
+/* Translate usage/page/offset triplet into physical address. */
+static u16 axiom_usage_to_target_address(struct axiom_data *ts, u8 usage, u8 page,
+					 char offset)
+{
+	/* 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;
+	}
+
+	if (page >= ts->usage_table[usage].num_pages) {
+		dev_err(ts->dev, "Invalid usage table! usage: u%02x, page: %02x, offset: %02x\n",
+			usage, page, offset);
+		return 0xffff;
+	}
+
+	return ((ts->usage_table[usage].start_page + page) << 8) + offset;
+}
+
+static int axiom_read(struct axiom_data *ts, u8 usage, u8 page, void *buf, u16 len)
+{
+	union axiom_cmd_header cmd_header;
+	int ret;
+
+	cmd_header.head.target_address =
+		cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
+	cmd_header.head.length = cpu_to_le16(len | AXIOM_CMD_HEADER_READ_MASK);
+
+	ret = regmap_write(ts->regmap, le32_to_cpu(cmd_header.preamble), 0);
+	if (ret) {
+		dev_err(ts->dev, "failed to write preamble, error %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_raw_read(ts->regmap, 0, buf, len);
+	if (ret) {
+		dev_err(ts->dev, "failed to read target address %04x, error %d\n",
+			cmd_header.head.target_address, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * One of the main purposes for reading the usage table is to identify
+ * which usages reside at which target address.
+ * When performing subsequent reads or writes to AXIOM, the target address
+ * is used to specify which usage is being accessed.
+ * Consider the following discovery code which will build up the usage table.
+ */
+static u32 axiom_populate_usage_table(struct axiom_data *ts)
+{
+	struct axiom_usage_entry *usage_table;
+	u8 *rx_data = ts->rx_buf;
+	u32 max_report_len = 0;
+	u32 usage_id;
+	int error;
+
+	usage_table = ts->usage_table;
+
+	/* Read the second page of usage u31 to get the usage table */
+	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
+			   (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
+
+	if (error)
+		return error;
+
+	for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
+		u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
+		u8 id = rx_data[offset + 0];
+		u8 start_page = rx_data[offset + 1];
+		u8 num_pages = rx_data[offset + 2];
+		u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
+
+		usage_table[id].is_report = !num_pages;
+
+		/* Store the entry into the usage table */
+		usage_table[id].id = id;
+		usage_table[id].start_page = start_page;
+		usage_table[id].num_pages = num_pages;
+
+		dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id, AXIOM_U31_BYTES_PER_USAGE,
+			&rx_data[offset]);
+
+		/* Identify the max report length the module will receive */
+		if (usage_table[id].is_report && max_offset > max_report_len)
+			max_report_len = max_offset;
+	}
+
+	ts->usage_table_populated = true;
+
+	return max_report_len;
+}
+
+static int axiom_discover(struct axiom_data *ts)
+{
+	int error;
+
+	/*
+	 * Fetch the first page of usage u31 to get the
+	 * device information and the number of usages
+	 */
+	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 0, &ts->devinfo, AXIOM_U31_PAGE0_LENGTH);
+	if (error)
+		return error;
+
+	dev_dbg(ts->dev, "  Boot Mode      : %s\n",
+		FIELD_GET(AXIOM_U31_BOOTMODE_MASK,
+			  le16_to_cpu(ts->devinfo.device_id)) ? "BLP" : "TCP");
+	dev_dbg(ts->dev, "  Device ID      : %04lx\n",
+		FIELD_GET(AXIOM_U31_DEVID_MASK, le16_to_cpu(ts->devinfo.device_id)));
+	dev_dbg(ts->dev, "  Firmware Rev   : %02x.%02x\n", ts->devinfo.fw_major,
+		ts->devinfo.fw_minor);
+	dev_dbg(ts->dev, "  Bootloader Rev : %02x.%02x\n", ts->devinfo.bootloader_fw_major,
+		ts->devinfo.bootloader_fw_minor);
+	dev_dbg(ts->dev, "  FW Extra Info  : %04x\n", ts->devinfo.fw_info_extra);
+	dev_dbg(ts->dev, "  Silicon        : %04x\n", le16_to_cpu(ts->devinfo.jedec_id));
+	dev_dbg(ts->dev, "  Number usages        : %04x\n", ts->devinfo.num_usages);
+
+	ts->max_report_len = axiom_populate_usage_table(ts);
+	if (!ts->max_report_len || !ts->devinfo.num_usages ||
+	    ts->max_report_len > AXIOM_MAX_REPORT_LEN) {
+		dev_err(ts->dev, "Invalid report length or usages number");
+		return -EINVAL;
+	}
+
+	dev_dbg(ts->dev, "Max Report Length: %u\n", ts->max_report_len);
+
+	return 0;
+}
+
+/*
+ * Support function to axiom_process_u41_report.
+ * Generates input-subsystem events for every target.
+ * After calling this function the caller shall issue
+ * a Sync to the input sub-system.
+ */
+static bool axiom_process_u41_report_target(struct axiom_data *ts,
+					    struct axiom_target_report *target)
+{
+	struct input_dev *input_dev = ts->input_dev;
+	struct axiom_u41_target *target_prev_state;
+	enum axiom_target_state current_state;
+	int id;
+
+	/* Verify the target index */
+	if (target->index >= AXIOM_U41_MAX_TARGETS) {
+		dev_err(ts->dev, "Invalid target index! %u\n", target->index);
+		return false;
+	}
+
+	target_prev_state = &ts->targets[target->index];
+
+	current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
+
+	if (target->present) {
+		if (target->z >= 0)
+			current_state = AXIOM_TARGET_STATE_TOUCHING;
+		else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
+			current_state = AXIOM_TARGET_STATE_HOVER;
+		else if (target->z == AXIOM_PROX_LEVEL)
+			current_state = AXIOM_TARGET_STATE_PROX;
+	}
+
+	id = target->index;
+
+	dev_dbg(ts->dev, "U41 Target T%u, present:%u, x:%u, y:%u, z:%d\n",
+		target->index, target->present,
+		target->x, target->y, target->z);
+
+	switch (current_state) {
+	case AXIOM_TARGET_STATE_NOT_PRESENT:
+	case AXIOM_TARGET_STATE_PROX:
+		if (!target_prev_state->insert)
+			break;
+		target_prev_state->insert = false;
+
+		input_mt_report_slot_inactive(input_dev);
+		/*
+		 * make sure the previous coordinates are
+		 * all off screen when the finger comes back
+		 */
+		target->x = 65535;
+		target->y = 65535;
+		target->z = AXIOM_PROX_LEVEL;
+		break;
+	case AXIOM_TARGET_STATE_HOVER:
+	case AXIOM_TARGET_STATE_TOUCHING:
+		target_prev_state->insert = true;
+		input_mt_slot(ts->input_dev, id);
+		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
+		input_report_abs(input_dev, ABS_MT_TRACKING_ID, id);
+		input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
+		input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
+
+		if (current_state == AXIOM_TARGET_STATE_TOUCHING) {
+			input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
+			input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
+		} else {
+			input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
+			input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	target_prev_state->state = current_state;
+	target_prev_state->x = target->x;
+	target_prev_state->y = target->y;
+	target_prev_state->z = target->z;
+
+	return true;
+}
+
+/*
+ * U41 is the output report of the 2D CTS and contains the status of targets
+ * (including contacts and pre-contacts) along with their X,Y,Z values.
+ * When a target has been removed (no longer detected),
+ * the corresponding X,Y,Z values will be zeroed.
+ */
+static bool axiom_process_u41_report(struct axiom_data *ts, u8 *rx_buf)
+{
+	struct axiom_target_report target;
+	bool update_done = false;
+	u16 target_status;
+	int i;
+
+	target_status = get_unaligned_le16(rx_buf + 1);
+
+	for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
+		u8 *target_step = &rx_buf[i * 4];
+
+		target.index = i;
+		target.present = ((target_status & (1 << i)) != 0) ? true : false;
+		target.x = get_unaligned_le16(target_step + 3);
+		target.y = get_unaligned_le16(target_step + 5);
+		target.z = (s8)(rx_buf[i + 43]);
+		update_done |= axiom_process_u41_report_target(ts, &target);
+	}
+
+	return update_done;
+}
+
+/*
+ * Validates the crc and demultiplexes the axiom reports to the appropriate
+ * report handler
+ */
+static int axiom_handle_events(struct axiom_data *ts)
+{
+	struct input_dev *input_dev = ts->input_dev;
+	u8 *report_data = ts->rx_buf;
+	struct device *dev = ts->dev;
+	u16 crc_report;
+	u8 *crc_bytes;
+	u16 crc_calc;
+	int error;
+	u8 len;
+
+	error = axiom_read(ts, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
+	if (error)
+		return error;
+
+	len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) << 1;
+	if (len <= 2) {
+		dev_err(dev, "Zero length report discarded.\n");
+		return -ENODATA;
+	}
+
+	/* Validate the report CRC */
+	crc_bytes = &report_data[len];
+
+	crc_report = get_unaligned_le16(crc_bytes - 2);
+	/* Length is in 16 bit words and remove the size of the CRC16 itself */
+	crc_calc = crc16(0, report_data, (len - 2));
+
+	if (crc_calc != crc_report) {
+		dev_err(dev,
+			"CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
+			crc_report, crc_calc);
+		return -EINVAL;
+	}
+
+	switch (report_data[1]) {
+	case AXIOM_USAGE_2DCTS_REPORT_ID:
+		if (axiom_process_u41_report(ts, &report_data[1])) {
+			input_mt_sync_frame(input_dev);
+			input_sync(input_dev);
+		}
+		break;
+
+	case AXIOM_USAGE_2AUX_REPORT_ID:
+		/* This is an aux report (force), not used currently */
+		break;
+
+	case AXIOM_USAGE_2HB_REPORT_ID:
+		/* This is a heartbeat report */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void axiom_i2c_poll(struct input_dev *input_dev)
+{
+	struct axiom_data *ts = input_get_drvdata(input_dev);
+
+	axiom_handle_events(ts);
+}
+
+static irqreturn_t axiom_irq(int irq, void *dev_id)
+{
+	struct axiom_data *ts = dev_id;
+
+	axiom_handle_events(ts);
+
+	return IRQ_HANDLED;
+}
+
+static void axiom_reset(struct gpio_desc *reset_gpio)
+{
+	gpiod_set_value_cansleep(reset_gpio, 1);
+	usleep_range(1000, 2000);
+	gpiod_set_value_cansleep(reset_gpio, 0);
+	msleep(AXIOM_STARTUP_TIME_MS);
+}
+
+static int axiom_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct input_dev *input_dev;
+	struct axiom_data *ts;
+	u32 poll_interval;
+	int target;
+	int error;
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, ts);
+	ts->client = client;
+	ts->dev = dev;
+
+	ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
+	error = PTR_ERR_OR_ZERO(ts->regmap);
+	if (error) {
+		dev_err(dev, "Failed to initialize regmap: %d\n", error);
+		return error;
+	}
+
+	error = devm_regulator_get_enable(dev, "vddi");
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "Failed to enable VDDI regulator\n");
+
+	error = devm_regulator_get_enable(dev, "vdda");
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "Failed to enable VDDA regulator\n");
+
+	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");
+
+	/* Make sure the time after a power ON sequence is meet */
+	if (ts->reset_gpio)
+		axiom_reset(ts->reset_gpio);
+	else
+		msleep(AXIOM_STARTUP_TIME_MS);
+
+	error = axiom_discover(ts);
+	if (error)
+		return dev_err_probe(dev, error, "Failed touchscreen discover\n");
+
+	input_dev = devm_input_allocate_device(ts->dev);
+	if (!input_dev)
+		return -ENOMEM;
+
+	input_dev->name = "TouchNetix axiom Touchscreen";
+	input_dev->phys = "input/axiom_ts";
+
+	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
+	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_FINGER, 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);
+
+	touchscreen_parse_properties(input_dev, true, &ts->prop);
+
+	/* Registers the axiom device as a touchscreen instead of a mouse pointer */
+	error = input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
+	if (error)
+		return error;
+
+	/* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
+	set_bit(EV_MSC, input_dev->evbit);
+	/* Declare that we support "RAW" Miscellaneous events */
+	set_bit(MSC_RAW, input_dev->mscbit);
+
+	ts->input_dev = input_dev;
+	input_set_drvdata(ts->input_dev, ts);
+
+	/* Ensure that all reports are initialised to not be present. */
+	for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
+		ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
+
+	if (client->irq) {
+		error = devm_request_threaded_irq(dev, client->irq, NULL,
+						  axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
+		if (error)
+			dev_err_probe(dev, error, "failed to request irq");
+	} else {
+		error = input_setup_polling(input_dev, axiom_i2c_poll);
+		if (error)
+			return dev_err_probe(ts->dev, error, "Unable to set up polling mode\n");
+
+		if (!device_property_read_u32(ts->dev, "poll-interval", &poll_interval))
+			input_set_poll_interval(input_dev, poll_interval);
+		else
+			input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
+	}
+
+	error = input_register_device(input_dev);
+	if (error)
+		return dev_err_probe(ts->dev, error, "failed to register input device\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id axiom_i2c_id_table[] = {
+	{ "ax54a" },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
+
+static const struct of_device_id axiom_i2c_of_match[] = {
+	{ .compatible = "touchnetix,ax54a", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
+
+static struct i2c_driver axiom_i2c_driver = {
+	.driver = {
+		   .name = "axiom",
+		   .of_match_table = axiom_i2c_of_match,
+	},
+	.id_table = axiom_i2c_id_table,
+	.probe = axiom_i2c_probe,
+};
+module_i2c_driver(axiom_i2c_driver);
+
+MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
+MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
+MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
+MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_DESCRIPTION("TouchNetix axiom touchscreen I2C bus driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Marco Felsch]

Hi Kamel,

thank you for the updated version please see my comment inline.

On 24-07-03, 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            |  14 +
>  drivers/input/touchscreen/Makefile           |   1 +
>  drivers/input/touchscreen/touchnetix_axiom.c | 616 +++++++++++++++++++
>  4 files changed, 632 insertions(+)
>  create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2041384d3866..ac6b612bfbba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22745,6 +22745,7 @@ M:	Kamel Bouhara <kamel.bouhara@bootlin.com>
>  L:	linux-input@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
> +F:	drivers/input/touchscreen/touchnetix_axiom.c
>  
>  TPM DEVICE DRIVER
>  M:	Peter Huewe <peterhuewe@gmx.de>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c821fe3ee794..1ce8f1c25625 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -834,6 +834,20 @@ config TOUCHSCREEN_MIGOR
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called migor_ts.
>  
> +config TOUCHSCREEN_TOUCHNETIX_AXIOM
> +	tristate "TouchNetix AXIOM based touchscreen controllers"
> +	depends on I2C
> +	select CRC16
> +	select REGMAP_I2C
> +	help
> +	  Say Y here if you have a axiom touchscreen connected to
> +	  your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called axiom.
> +
>  config TOUCHSCREEN_TOUCHRIGHT
>  	tristate "Touchright serial touchscreen"
>  	select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index a81cb5aa21a5..6ce7b804adc7 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -91,6 +91,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
>  obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI)	+= surface3_spi.o
>  obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
> +obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM)	+= touchnetix_axiom.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
>  obj-$(CONFIG_TOUCHSCREEN_TS4800)	+= ts4800-ts.o
> diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
> new file mode 100644
> index 000000000000..cea52dafc913
> --- /dev/null
> +++ b/drivers/input/touchscreen/touchnetix_axiom.c
> @@ -0,0 +1,616 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TouchNetix axiom Touchscreen Driver
> + *
> + * Copyright (C) 2020-2023 TouchNetix Ltd.
> + *
> + * Author(s): Bart Prescott <bartp@baasheep.co.uk>
> + *            Pedro Torruella <pedro.torruella@touchnetix.com>
> + *            Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> + *            Hannah Rossiter <hannah.rossiter@touchnetix.com>
> + *            Kamel Bouhara <kamel.bouhara@bootlin.com>
> + *
> + */
> +#include <linux/bitfield.h>
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +
> +#include <asm/unaligned.h>
> +#define AXIOM_PROX_LEVEL		-128
> +#define AXIOM_STARTUP_TIME_MS		110
> +/*
> + * Register group u31 has 2 pages for usage table entries.
> + */
> +#define AXIOM_U31_MAX_USAGES		0xff
> +#define AXIOM_U31_BYTES_PER_USAGE	6
> +#define AXIOM_U31_PAGE0_LENGTH		0x0C
> +#define AXIOM_U31_BOOTMODE_MASK		BIT(7)
> +#define AXIOM_U31_DEVID_MASK		GENMASK(14, 0)
> +
> +#define AXIOM_MAX_REPORT_LEN		0x7f
> +
> +#define AXIOM_CMD_HEADER_READ_MASK	BIT(15)
> +#define AXIOM_U41_MAX_TARGETS		10
> +
> +#define AXIOM_U46_AUX_CHANNELS		4
> +#define AXIOM_U46_AUX_MASK		GENMASK(11, 0)
> +
> +#define AXIOM_COMMS_MAX_USAGE_PAGES	3
> +#define AXIOM_COMMS_PAGE_SIZE		256
> +#define AXIOM_COMMS_REPORT_LEN_MASK	GENMASK(6, 0)
> +
> +#define AXIOM_REPORT_USAGE_ID		0x34
> +#define AXIOM_DEVINFO_USAGE_ID		0x31
> +#define AXIOM_USAGE_2HB_REPORT_ID	0x01
> +#define AXIOM_USAGE_2AUX_REPORT_ID	0x46
> +#define AXIOM_USAGE_2DCTS_REPORT_ID	0x41
> +
> +#define AXIOM_PAGE_OFFSET_MASK		GENMASK(6, 0)
> +
> +struct axiom_devinfo {
> +	__le16 device_id;
> +	u8 fw_minor;
> +	u8 fw_major;
> +	u8 fw_info_extra;
> +	u8 tcp_revision;
> +	u8 bootloader_fw_minor;
> +	u8 bootloader_fw_major;
> +	__le16 jedec_id;
> +	u8 num_usages;
> +} __packed;
> +
> +/*
> + * Describes parameters of a specific usage, essentially a single element of
> + * the "Usage Table"
> + */
> +struct axiom_usage_entry {
> +	u8 id;
> +	bool is_report;
> +	u8 start_page;
> +	u8 num_pages;
> +};
> +
> +/*
> + * Represents state of a touch or target when detected prior to a touch (eg.
> + * hover or proximity events).
> + */
> +enum axiom_target_state {
> +	AXIOM_TARGET_STATE_NOT_PRESENT = 0,
> +	AXIOM_TARGET_STATE_PROX = 1,
> +	AXIOM_TARGET_STATE_HOVER = 2,
> +	AXIOM_TARGET_STATE_TOUCHING = 3,
> +};
> +
> +struct axiom_u41_target {
> +	enum axiom_target_state state;
> +	u16 x;
> +	u16 y;
> +	s8 z;
> +	/* force target report processing */
> +	bool insert;
> +	bool touch;
> +};
> +
> +struct axiom_target_report {
> +	u8 index;
> +	bool present;
> +	u16 x;
> +	u16 y;
> +	s8 z;
> +};
> +
> +union axiom_cmd_header {
> +	__le32 preamble;
> +	struct {
> +		__le16 target_address;
> +		__le16 length;
> +	} head;
> +};
> +
> +struct axiom_data {
> +	struct axiom_devinfo devinfo;
> +	struct device *dev;
> +	struct gpio_desc *reset_gpio;
			    ^
The reset is done within the probe so this can be dropped.

> +	struct i2c_client *client;
			    ^
The client is never used. I would either store the client or the device
reference but not both.

> +	struct input_dev *input_dev;
> +	u32 max_report_len;
> +	u8 rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];

Is there a reason for having the rx_buf within the driver priv data?
IMHO it's more error probe since we never set it to zero before we use
the buffer. I would rather move the rx-buffer to the functions which
perform the read.

> +	struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
> +	struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> +	bool usage_table_populated;
		^
This can be an inline helper which checks the max_report_len e.g:

static inline bool axiom_usage_table_populated(struct axiom_data *ts)
{
	return ts->max_report_len;
}

> +	struct regmap *regmap;
> +	struct touchscreen_properties	prop;
> +};
> +
> +static const struct regmap_config axiom_i2c_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.val_bits = 8,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +};
> +
> +/*
> + * axiom devices are typically configured to report touches at a rate
> + * of 100Hz (10ms) for systems that require polling for reports.
> + * When reports are polled, it will be expected to occasionally
> + * observe the overflow bit being set in the reports.
> + * This indicates that reports are not being read fast enough.
> + */
> +#define POLL_INTERVAL_DEFAULT_MS 10
> +
> +/* Translate usage/page/offset triplet into physical address. */
> +static u16 axiom_usage_to_target_address(struct axiom_data *ts, u8 usage, u8 page,
> +					 char offset)
> +{
> +	/* 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;
> +	}
> +
> +	if (page >= ts->usage_table[usage].num_pages) {
> +		dev_err(ts->dev, "Invalid usage table! usage: u%02x, page: %02x, offset: %02x\n",
> +			usage, page, offset);
> +		return 0xffff;
> +	}
> +
> +	return ((ts->usage_table[usage].start_page + page) << 8) + offset;
> +}
> +
> +static int axiom_read(struct axiom_data *ts, u8 usage, u8 page, void *buf, u16 len)
> +{
> +	union axiom_cmd_header cmd_header;
> +	int ret;
> +
> +	cmd_header.head.target_address =
> +		cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
> +	cmd_header.head.length = cpu_to_le16(len | AXIOM_CMD_HEADER_READ_MASK);
> +
> +	ret = regmap_write(ts->regmap, le32_to_cpu(cmd_header.preamble), 0);
> +	if (ret) {
> +		dev_err(ts->dev, "failed to write preamble, error %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_raw_read(ts->regmap, 0, buf, len);
> +	if (ret) {
> +		dev_err(ts->dev, "failed to read target address %04x, error %d\n",
> +			cmd_header.head.target_address, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * One of the main purposes for reading the usage table is to identify
> + * which usages reside at which target address.
> + * When performing subsequent reads or writes to AXIOM, the target address
> + * is used to specify which usage is being accessed.
> + * Consider the following discovery code which will build up the usage table.
> + */
> +static u32 axiom_populate_usage_table(struct axiom_data *ts)
> +{
> +	struct axiom_usage_entry *usage_table;
> +	u8 *rx_data = ts->rx_buf;
> +	u32 max_report_len = 0;
> +	u32 usage_id;
> +	int error;
> +
> +	usage_table = ts->usage_table;
> +
> +	/* Read the second page of usage u31 to get the usage table */
> +	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
> +			   (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> +
> +	if (error)
> +		return error;
> +
> +	for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
> +		u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> +		u8 id = rx_data[offset + 0];
> +		u8 start_page = rx_data[offset + 1];
> +		u8 num_pages = rx_data[offset + 2];
> +		u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> +
> +		usage_table[id].is_report = !num_pages;
> +
> +		/* Store the entry into the usage table */
> +		usage_table[id].id = id;
> +		usage_table[id].start_page = start_page;
> +		usage_table[id].num_pages = num_pages;
> +
> +		dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id, AXIOM_U31_BYTES_PER_USAGE,
> +			&rx_data[offset]);
> +
> +		/* Identify the max report length the module will receive */
> +		if (usage_table[id].is_report && max_offset > max_report_len)
> +			max_report_len = max_offset;
> +	}
> +
> +	ts->usage_table_populated = true;
> +
> +	return max_report_len;
> +}
> +
> +static int axiom_discover(struct axiom_data *ts)
> +{
> +	int error;
> +
> +	/*
> +	 * Fetch the first page of usage u31 to get the
> +	 * device information and the number of usages
> +	 */
> +	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 0, &ts->devinfo, AXIOM_U31_PAGE0_LENGTH);
> +	if (error)
> +		return error;
> +
> +	dev_dbg(ts->dev, "  Boot Mode      : %s\n",
> +		FIELD_GET(AXIOM_U31_BOOTMODE_MASK,
> +			  le16_to_cpu(ts->devinfo.device_id)) ? "BLP" : "TCP");
> +	dev_dbg(ts->dev, "  Device ID      : %04lx\n",
> +		FIELD_GET(AXIOM_U31_DEVID_MASK, le16_to_cpu(ts->devinfo.device_id)));
> +	dev_dbg(ts->dev, "  Firmware Rev   : %02x.%02x\n", ts->devinfo.fw_major,
> +		ts->devinfo.fw_minor);
> +	dev_dbg(ts->dev, "  Bootloader Rev : %02x.%02x\n", ts->devinfo.bootloader_fw_major,
> +		ts->devinfo.bootloader_fw_minor);
> +	dev_dbg(ts->dev, "  FW Extra Info  : %04x\n", ts->devinfo.fw_info_extra);
> +	dev_dbg(ts->dev, "  Silicon        : %04x\n", le16_to_cpu(ts->devinfo.jedec_id));
> +	dev_dbg(ts->dev, "  Number usages        : %04x\n", ts->devinfo.num_usages);
> +
> +	ts->max_report_len = axiom_populate_usage_table(ts);
> +	if (!ts->max_report_len || !ts->devinfo.num_usages ||
> +	    ts->max_report_len > AXIOM_MAX_REPORT_LEN) {
> +		dev_err(ts->dev, "Invalid report length or usages number");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(ts->dev, "Max Report Length: %u\n", ts->max_report_len);
> +
> +	return 0;
> +}
> +
> +/*
> + * Support function to axiom_process_u41_report.
> + * Generates input-subsystem events for every target.
> + * After calling this function the caller shall issue
> + * a Sync to the input sub-system.
> + */
> +static bool axiom_process_u41_report_target(struct axiom_data *ts,
> +					    struct axiom_target_report *target)
> +{
> +	struct input_dev *input_dev = ts->input_dev;
> +	struct axiom_u41_target *target_prev_state;
> +	enum axiom_target_state current_state;
> +	int id;
> +
> +	/* Verify the target index */
> +	if (target->index >= AXIOM_U41_MAX_TARGETS) {
> +		dev_err(ts->dev, "Invalid target index! %u\n", target->index);
> +		return false;
> +	}
> +
> +	target_prev_state = &ts->targets[target->index];
> +
> +	current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
> +
> +	if (target->present) {
> +		if (target->z >= 0)
> +			current_state = AXIOM_TARGET_STATE_TOUCHING;
> +		else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> +			current_state = AXIOM_TARGET_STATE_HOVER;
> +		else if (target->z == AXIOM_PROX_LEVEL)
> +			current_state = AXIOM_TARGET_STATE_PROX;
> +	}
> +
> +	id = target->index;
> +
> +	dev_dbg(ts->dev, "U41 Target T%u, present:%u, x:%u, y:%u, z:%d\n",
> +		target->index, target->present,
> +		target->x, target->y, target->z);
> +
> +	switch (current_state) {
> +	case AXIOM_TARGET_STATE_NOT_PRESENT:
> +	case AXIOM_TARGET_STATE_PROX:
> +		if (!target_prev_state->insert)
> +			break;
> +		target_prev_state->insert = false;
> +
> +		input_mt_report_slot_inactive(input_dev);
> +		/*
> +		 * make sure the previous coordinates are
> +		 * all off screen when the finger comes back
> +		 */
> +		target->x = 65535;
> +		target->y = 65535;
> +		target->z = AXIOM_PROX_LEVEL;
> +		break;
> +	case AXIOM_TARGET_STATE_HOVER:
> +	case AXIOM_TARGET_STATE_TOUCHING:
> +		target_prev_state->insert = true;
> +		input_mt_slot(ts->input_dev, id);
> +		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
> +		input_report_abs(input_dev, ABS_MT_TRACKING_ID, id);
> +		input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> +		input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
> +
> +		if (current_state == AXIOM_TARGET_STATE_TOUCHING) {
> +			input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
> +		} else {
> +			input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	target_prev_state->state = current_state;
> +	target_prev_state->x = target->x;
> +	target_prev_state->y = target->y;
> +	target_prev_state->z = target->z;
> +
> +	return true;
> +}
> +
> +/*
> + * U41 is the output report of the 2D CTS and contains the status of targets
> + * (including contacts and pre-contacts) along with their X,Y,Z values.
> + * When a target has been removed (no longer detected),
> + * the corresponding X,Y,Z values will be zeroed.
> + */
> +static bool axiom_process_u41_report(struct axiom_data *ts, u8 *rx_buf)
> +{
> +	struct axiom_target_report target;
> +	bool update_done = false;
> +	u16 target_status;
> +	int i;
> +
> +	target_status = get_unaligned_le16(rx_buf + 1);
> +
> +	for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> +		u8 *target_step = &rx_buf[i * 4];
> +
> +		target.index = i;
> +		target.present = ((target_status & (1 << i)) != 0) ? true : false;
> +		target.x = get_unaligned_le16(target_step + 3);
> +		target.y = get_unaligned_le16(target_step + 5);
> +		target.z = (s8)(rx_buf[i + 43]);
> +		update_done |= axiom_process_u41_report_target(ts, &target);
> +	}
> +
> +	return update_done;
> +}
> +
> +/*
> + * Validates the crc and demultiplexes the axiom reports to the appropriate
> + * report handler
> + */
> +static int axiom_handle_events(struct axiom_data *ts)
> +{
> +	struct input_dev *input_dev = ts->input_dev;
> +	u8 *report_data = ts->rx_buf;
> +	struct device *dev = ts->dev;
> +	u16 crc_report;
> +	u8 *crc_bytes;
> +	u16 crc_calc;
> +	int error;
> +	u8 len;
> +
> +	error = axiom_read(ts, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
> +	if (error)
> +		return error;
> +
> +	len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) << 1;
> +	if (len <= 2) {
> +		dev_err(dev, "Zero length report discarded.\n");
> +		return -ENODATA;
> +	}
> +
> +	/* Validate the report CRC */
> +	crc_bytes = &report_data[len];
> +
> +	crc_report = get_unaligned_le16(crc_bytes - 2);
> +	/* Length is in 16 bit words and remove the size of the CRC16 itself */
> +	crc_calc = crc16(0, report_data, (len - 2));
> +
> +	if (crc_calc != crc_report) {
> +		dev_err(dev,
> +			"CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> +			crc_report, crc_calc);
> +		return -EINVAL;
> +	}
> +
> +	switch (report_data[1]) {
> +	case AXIOM_USAGE_2DCTS_REPORT_ID:
> +		if (axiom_process_u41_report(ts, &report_data[1])) {
> +			input_mt_sync_frame(input_dev);
> +			input_sync(input_dev);
> +		}
> +		break;
> +
> +	case AXIOM_USAGE_2AUX_REPORT_ID:
> +		/* This is an aux report (force), not used currently */
> +		break;
> +
> +	case AXIOM_USAGE_2HB_REPORT_ID:
> +		/* This is a heartbeat report */
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void axiom_i2c_poll(struct input_dev *input_dev)
> +{
> +	struct axiom_data *ts = input_get_drvdata(input_dev);
> +
> +	axiom_handle_events(ts);
> +}
> +
> +static irqreturn_t axiom_irq(int irq, void *dev_id)
> +{
> +	struct axiom_data *ts = dev_id;
> +
> +	axiom_handle_events(ts);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void axiom_reset(struct gpio_desc *reset_gpio)
> +{
> +	gpiod_set_value_cansleep(reset_gpio, 1);
> +	usleep_range(1000, 2000);
> +	gpiod_set_value_cansleep(reset_gpio, 0);
> +	msleep(AXIOM_STARTUP_TIME_MS);
> +}
> +
> +static int axiom_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct input_dev *input_dev;
> +	struct axiom_data *ts;
> +	u32 poll_interval;
> +	int target;
> +	int error;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, ts);
> +	ts->client = client;
> +	ts->dev = dev;
> +
> +	ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
> +	error = PTR_ERR_OR_ZERO(ts->regmap);
> +	if (error) {
> +		dev_err(dev, "Failed to initialize regmap: %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_regulator_get_enable(dev, "vddi");
> +	if (error)
> +		return dev_err_probe(&client->dev, error,
> +				     "Failed to enable VDDI regulator\n");
> +
> +	error = devm_regulator_get_enable(dev, "vdda");
> +	if (error)
> +		return dev_err_probe(&client->dev, error,
> +				     "Failed to enable VDDA regulator\n");
> +
> +	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");
> +
> +	/* Make sure the time after a power ON sequence is meet */
> +	if (ts->reset_gpio)
> +		axiom_reset(ts->reset_gpio);
> +	else

No else just:

> +		msleep(AXIOM_STARTUP_TIME_MS);

	msleep(AXIOM_STARTUP_TIME_MS);

and drop the msleep within the axiom_reset().

Regards,
  Marco

> +
> +	error = axiom_discover(ts);
> +	if (error)
> +		return dev_err_probe(dev, error, "Failed touchscreen discover\n");
> +
> +	input_dev = devm_input_allocate_device(ts->dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->name = "TouchNetix axiom Touchscreen";
> +	input_dev->phys = "input/axiom_ts";
> +
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> +	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_FINGER, 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);
> +
> +	touchscreen_parse_properties(input_dev, true, &ts->prop);
> +
> +	/* Registers the axiom device as a touchscreen instead of a mouse pointer */
> +	error = input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> +	if (error)
> +		return error;
> +
> +	/* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> +	set_bit(EV_MSC, input_dev->evbit);
> +	/* Declare that we support "RAW" Miscellaneous events */
> +	set_bit(MSC_RAW, input_dev->mscbit);
> +
> +	ts->input_dev = input_dev;
> +	input_set_drvdata(ts->input_dev, ts);
> +
> +	/* Ensure that all reports are initialised to not be present. */
> +	for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> +		ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
> +
> +	if (client->irq) {
> +		error = devm_request_threaded_irq(dev, client->irq, NULL,
> +						  axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
> +		if (error)
> +			dev_err_probe(dev, error, "failed to request irq");
> +	} else {
> +		error = input_setup_polling(input_dev, axiom_i2c_poll);
> +		if (error)
> +			return dev_err_probe(ts->dev, error, "Unable to set up polling mode\n");
> +
> +		if (!device_property_read_u32(ts->dev, "poll-interval", &poll_interval))
> +			input_set_poll_interval(input_dev, poll_interval);
> +		else
> +			input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error)
> +		return dev_err_probe(ts->dev, error, "failed to register input device\n");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> +	{ "ax54a" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> +
> +static const struct of_device_id axiom_i2c_of_match[] = {
> +	{ .compatible = "touchnetix,ax54a", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> +
> +static struct i2c_driver axiom_i2c_driver = {
> +	.driver = {
> +		   .name = "axiom",
> +		   .of_match_table = axiom_i2c_of_match,
> +	},
> +	.id_table = axiom_i2c_id_table,
> +	.probe = axiom_i2c_probe,
> +};
> +module_i2c_driver(axiom_i2c_driver);
> +
> +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
> +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
> +MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
> +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("TouchNetix axiom touchscreen I2C bus driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
> 
> 

Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Jeff LaBundy]

Hi Kamel,

On Wed, Jul 03, 2024 at 04:25:18PM +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>
> ---

This is coming together nicely; just a few trailing comments on
top of Marco's feedback.

>  MAINTAINERS                                  |   1 +
>  drivers/input/touchscreen/Kconfig            |  14 +
>  drivers/input/touchscreen/Makefile           |   1 +
>  drivers/input/touchscreen/touchnetix_axiom.c | 616 +++++++++++++++++++
>  4 files changed, 632 insertions(+)
>  create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2041384d3866..ac6b612bfbba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22745,6 +22745,7 @@ M:	Kamel Bouhara <kamel.bouhara@bootlin.com>
>  L:	linux-input@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
> +F:	drivers/input/touchscreen/touchnetix_axiom.c
>  
>  TPM DEVICE DRIVER
>  M:	Peter Huewe <peterhuewe@gmx.de>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c821fe3ee794..1ce8f1c25625 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -834,6 +834,20 @@ config TOUCHSCREEN_MIGOR
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called migor_ts.
>  
> +config TOUCHSCREEN_TOUCHNETIX_AXIOM
> +	tristate "TouchNetix AXIOM based touchscreen controllers"
> +	depends on I2C
> +	select CRC16
> +	select REGMAP_I2C
> +	help
> +	  Say Y here if you have a axiom touchscreen connected to
> +	  your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called axiom.
> +
>  config TOUCHSCREEN_TOUCHRIGHT
>  	tristate "Touchright serial touchscreen"
>  	select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index a81cb5aa21a5..6ce7b804adc7 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -91,6 +91,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
>  obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI)	+= surface3_spi.o
>  obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
> +obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM)	+= touchnetix_axiom.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
>  obj-$(CONFIG_TOUCHSCREEN_TS4800)	+= ts4800-ts.o
> diff --git a/drivers/input/touchscreen/touchnetix_axiom.c b/drivers/input/touchscreen/touchnetix_axiom.c
> new file mode 100644
> index 000000000000..cea52dafc913
> --- /dev/null
> +++ b/drivers/input/touchscreen/touchnetix_axiom.c
> @@ -0,0 +1,616 @@
> +// 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>
> + *
> + */

Please include bits.h as well.

> +#include <linux/bitfield.h>
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +
> +#include <asm/unaligned.h>
> +#define AXIOM_PROX_LEVEL		-128
> +#define AXIOM_STARTUP_TIME_MS		110
> +/*
> + * Register group u31 has 2 pages for usage table entries.
> + */
> +#define AXIOM_U31_MAX_USAGES		0xff
> +#define AXIOM_U31_BYTES_PER_USAGE	6
> +#define AXIOM_U31_PAGE0_LENGTH		0x0C
> +#define AXIOM_U31_BOOTMODE_MASK		BIT(7)
> +#define AXIOM_U31_DEVID_MASK		GENMASK(14, 0)
> +
> +#define AXIOM_MAX_REPORT_LEN		0x7f
> +
> +#define AXIOM_CMD_HEADER_READ_MASK	BIT(15)
> +#define AXIOM_U41_MAX_TARGETS		10
> +
> +#define AXIOM_U46_AUX_CHANNELS		4
> +#define AXIOM_U46_AUX_MASK		GENMASK(11, 0)
> +
> +#define AXIOM_COMMS_MAX_USAGE_PAGES	3
> +#define AXIOM_COMMS_PAGE_SIZE		256
> +#define AXIOM_COMMS_REPORT_LEN_MASK	GENMASK(6, 0)
> +
> +#define AXIOM_REPORT_USAGE_ID		0x34
> +#define AXIOM_DEVINFO_USAGE_ID		0x31
> +#define AXIOM_USAGE_2HB_REPORT_ID	0x01
> +#define AXIOM_USAGE_2AUX_REPORT_ID	0x46
> +#define AXIOM_USAGE_2DCTS_REPORT_ID	0x41
> +
> +#define AXIOM_PAGE_OFFSET_MASK		GENMASK(6, 0)
> +
> +struct axiom_devinfo {
> +	__le16 device_id;
> +	u8 fw_minor;
> +	u8 fw_major;
> +	u8 fw_info_extra;
> +	u8 tcp_revision;
> +	u8 bootloader_fw_minor;
> +	u8 bootloader_fw_major;
> +	__le16 jedec_id;
> +	u8 num_usages;
> +} __packed;
> +
> +/*
> + * Describes parameters of a specific usage, essentially a single element of
> + * the "Usage Table"
> + */
> +struct axiom_usage_entry {
> +	u8 id;
> +	bool is_report;
> +	u8 start_page;
> +	u8 num_pages;
> +};
> +
> +/*
> + * Represents state of a touch or target when detected prior to a touch (eg.
> + * hover or proximity events).
> + */
> +enum axiom_target_state {
> +	AXIOM_TARGET_STATE_NOT_PRESENT = 0,
> +	AXIOM_TARGET_STATE_PROX = 1,
> +	AXIOM_TARGET_STATE_HOVER = 2,
> +	AXIOM_TARGET_STATE_TOUCHING = 3,
> +};
> +
> +struct axiom_u41_target {
> +	enum axiom_target_state state;
> +	u16 x;
> +	u16 y;
> +	s8 z;
> +	/* force target report processing */
> +	bool insert;
> +	bool touch;
> +};
> +
> +struct axiom_target_report {
> +	u8 index;
> +	bool present;
> +	u16 x;
> +	u16 y;
> +	s8 z;
> +};
> +
> +union axiom_cmd_header {
> +	__le32 preamble;
> +	struct {
> +		__le16 target_address;
> +		__le16 length;
> +	} head;
> +};
> +
> +struct axiom_data {
> +	struct axiom_devinfo devinfo;
> +	struct device *dev;
> +	struct gpio_desc *reset_gpio;
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	u32 max_report_len;
> +	u8 rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
> +	struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
> +	struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
> +	bool usage_table_populated;
> +	struct regmap *regmap;
> +	struct touchscreen_properties	prop;

It looks like a tab snuck in here.

> +};
> +
> +static const struct regmap_config axiom_i2c_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.val_bits = 8,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +};
> +
> +/*
> + * axiom devices are typically configured to report touches at a rate
> + * of 100Hz (10ms) for systems that require polling for reports.
> + * When reports are polled, it will be expected to occasionally
> + * observe the overflow bit being set in the reports.
> + * This indicates that reports are not being read fast enough.

This comment is strange; if reports are not read quickly enough at
the default rate, why not set the default rate at the slowest for
which the overflow bit is not set?

> + */
> +#define POLL_INTERVAL_DEFAULT_MS 10
> +
> +/* Translate usage/page/offset triplet into physical address. */
> +static u16 axiom_usage_to_target_address(struct axiom_data *ts, u8 usage, u8 page,
> +					 char offset)
> +{
> +	/* 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;

Checkpatch normally gripes if an else follows a return; did that not
happen here? You should simplify it either way:

        if (...) {
                if (...)
                        return ...;

                return U16_MAX;
        }

> +	}
> +
> +	if (page >= ts->usage_table[usage].num_pages) {
> +		dev_err(ts->dev, "Invalid usage table! usage: u%02x, page: %02x, offset: %02x\n",
> +			usage, page, offset);
> +		return 0xffff;
> +	}
> +
> +	return ((ts->usage_table[usage].start_page + page) << 8) + offset;
> +}
> +
> +static int axiom_read(struct axiom_data *ts, u8 usage, u8 page, void *buf, u16 len)
> +{
> +	union axiom_cmd_header cmd_header;
> +	int ret;

For consistency, please use 'error' as is done throughout.

> +
> +	cmd_header.head.target_address =
> +		cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
> +	cmd_header.head.length = cpu_to_le16(len | AXIOM_CMD_HEADER_READ_MASK);
> +
> +	ret = regmap_write(ts->regmap, le32_to_cpu(cmd_header.preamble), 0);
> +	if (ret) {
> +		dev_err(ts->dev, "failed to write preamble, error %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_raw_read(ts->regmap, 0, buf, len);
> +	if (ret) {
> +		dev_err(ts->dev, "failed to read target address %04x, error %d\n",
> +			cmd_header.head.target_address, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * One of the main purposes for reading the usage table is to identify
> + * which usages reside at which target address.
> + * When performing subsequent reads or writes to AXIOM, the target address
> + * is used to specify which usage is being accessed.
> + * Consider the following discovery code which will build up the usage table.
> + */
> +static u32 axiom_populate_usage_table(struct axiom_data *ts)
> +{
> +	struct axiom_usage_entry *usage_table;
> +	u8 *rx_data = ts->rx_buf;
> +	u32 max_report_len = 0;
> +	u32 usage_id;
> +	int error;
> +
> +	usage_table = ts->usage_table;
> +
> +	/* Read the second page of usage u31 to get the usage table */
> +	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
> +			   (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> +
> +	if (error)
> +		return error;
> +
> +	for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
> +		u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
> +		u8 id = rx_data[offset + 0];
> +		u8 start_page = rx_data[offset + 1];
> +		u8 num_pages = rx_data[offset + 2];
> +		u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
> +
> +		usage_table[id].is_report = !num_pages;
> +
> +		/* Store the entry into the usage table */
> +		usage_table[id].id = id;
> +		usage_table[id].start_page = start_page;
> +		usage_table[id].num_pages = num_pages;
> +
> +		dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id, AXIOM_U31_BYTES_PER_USAGE,
> +			&rx_data[offset]);
> +
> +		/* Identify the max report length the module will receive */
> +		if (usage_table[id].is_report && max_offset > max_report_len)
> +			max_report_len = max_offset;
> +	}
> +
> +	ts->usage_table_populated = true;
> +
> +	return max_report_len;
> +}
> +
> +static int axiom_discover(struct axiom_data *ts)
> +{
> +	int error;
> +
> +	/*
> +	 * Fetch the first page of usage u31 to get the
> +	 * device information and the number of usages
> +	 */
> +	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 0, &ts->devinfo, AXIOM_U31_PAGE0_LENGTH);

It seems a little safer, and more intuitive, to pass sizeof(ts->devinfo) instead
of AXIOM_U31_PAGE0_LENGTH. To that end, devinfo is only 11 bytes in size, but
you're reading 12 bytes. I'm guessing the compiler is padding axiom_data which is
the only reason the existing implementation seems to work.

> +	if (error)
> +		return error;
> +
> +	dev_dbg(ts->dev, "  Boot Mode      : %s\n",
> +		FIELD_GET(AXIOM_U31_BOOTMODE_MASK,
> +			  le16_to_cpu(ts->devinfo.device_id)) ? "BLP" : "TCP");
> +	dev_dbg(ts->dev, "  Device ID      : %04lx\n",
> +		FIELD_GET(AXIOM_U31_DEVID_MASK, le16_to_cpu(ts->devinfo.device_id)));
> +	dev_dbg(ts->dev, "  Firmware Rev   : %02x.%02x\n", ts->devinfo.fw_major,
> +		ts->devinfo.fw_minor);
> +	dev_dbg(ts->dev, "  Bootloader Rev : %02x.%02x\n", ts->devinfo.bootloader_fw_major,
> +		ts->devinfo.bootloader_fw_minor);
> +	dev_dbg(ts->dev, "  FW Extra Info  : %04x\n", ts->devinfo.fw_info_extra);
> +	dev_dbg(ts->dev, "  Silicon        : %04x\n", le16_to_cpu(ts->devinfo.jedec_id));
> +	dev_dbg(ts->dev, "  Number usages        : %04x\n", ts->devinfo.num_usages);
> +
> +	ts->max_report_len = axiom_populate_usage_table(ts);
> +	if (!ts->max_report_len || !ts->devinfo.num_usages ||
> +	    ts->max_report_len > AXIOM_MAX_REPORT_LEN) {
> +		dev_err(ts->dev, "Invalid report length or usages number");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(ts->dev, "Max Report Length: %u\n", ts->max_report_len);
> +
> +	return 0;
> +}
> +
> +/*
> + * Support function to axiom_process_u41_report.
> + * Generates input-subsystem events for every target.
> + * After calling this function the caller shall issue
> + * a Sync to the input sub-system.
> + */
> +static bool axiom_process_u41_report_target(struct axiom_data *ts,
> +					    struct axiom_target_report *target)
> +{
> +	struct input_dev *input_dev = ts->input_dev;
> +	struct axiom_u41_target *target_prev_state;
> +	enum axiom_target_state current_state;
> +	int id;
> +
> +	/* Verify the target index */
> +	if (target->index >= AXIOM_U41_MAX_TARGETS) {
> +		dev_err(ts->dev, "Invalid target index! %u\n", target->index);
> +		return false;
> +	}
> +
> +	target_prev_state = &ts->targets[target->index];
> +
> +	current_state = AXIOM_TARGET_STATE_NOT_PRESENT;
> +
> +	if (target->present) {
> +		if (target->z >= 0)
> +			current_state = AXIOM_TARGET_STATE_TOUCHING;
> +		else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
> +			current_state = AXIOM_TARGET_STATE_HOVER;
> +		else if (target->z == AXIOM_PROX_LEVEL)
> +			current_state = AXIOM_TARGET_STATE_PROX;
> +	}
> +
> +	id = target->index;
> +
> +	dev_dbg(ts->dev, "U41 Target T%u, present:%u, x:%u, y:%u, z:%d\n",
> +		target->index, target->present,
> +		target->x, target->y, target->z);
> +
> +	switch (current_state) {
> +	case AXIOM_TARGET_STATE_NOT_PRESENT:
> +	case AXIOM_TARGET_STATE_PROX:
> +		if (!target_prev_state->insert)
> +			break;
> +		target_prev_state->insert = false;
> +
> +		input_mt_report_slot_inactive(input_dev);
> +		/*
> +		 * make sure the previous coordinates are
> +		 * all off screen when the finger comes back
> +		 */
> +		target->x = 65535;
> +		target->y = 65535;
> +		target->z = AXIOM_PROX_LEVEL;
> +		break;
> +	case AXIOM_TARGET_STATE_HOVER:
> +	case AXIOM_TARGET_STATE_TOUCHING:
> +		target_prev_state->insert = true;
> +		input_mt_slot(ts->input_dev, id);
> +		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
> +		input_report_abs(input_dev, ABS_MT_TRACKING_ID, id);
> +		input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
> +		input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
> +
> +		if (current_state == AXIOM_TARGET_STATE_TOUCHING) {
> +			input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
> +		} else {
> +			input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
> +			input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	target_prev_state->state = current_state;
> +	target_prev_state->x = target->x;
> +	target_prev_state->y = target->y;
> +	target_prev_state->z = target->z;
> +
> +	return true;
> +}
> +
> +/*
> + * U41 is the output report of the 2D CTS and contains the status of targets
> + * (including contacts and pre-contacts) along with their X,Y,Z values.
> + * When a target has been removed (no longer detected),
> + * the corresponding X,Y,Z values will be zeroed.
> + */
> +static bool axiom_process_u41_report(struct axiom_data *ts, u8 *rx_buf)
> +{
> +	struct axiom_target_report target;
> +	bool update_done = false;
> +	u16 target_status;
> +	int i;
> +
> +	target_status = get_unaligned_le16(rx_buf + 1);
> +
> +	for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
> +		u8 *target_step = &rx_buf[i * 4];
> +
> +		target.index = i;
> +		target.present = ((target_status & (1 << i)) != 0) ? true : false;
> +		target.x = get_unaligned_le16(target_step + 3);
> +		target.y = get_unaligned_le16(target_step + 5);
> +		target.z = (s8)(rx_buf[i + 43]);
> +		update_done |= axiom_process_u41_report_target(ts, &target);
> +	}
> +
> +	return update_done;
> +}
> +
> +/*
> + * Validates the crc and demultiplexes the axiom reports to the appropriate
> + * report handler
> + */
> +static int axiom_handle_events(struct axiom_data *ts)
> +{
> +	struct input_dev *input_dev = ts->input_dev;
> +	u8 *report_data = ts->rx_buf;
> +	struct device *dev = ts->dev;
> +	u16 crc_report;
> +	u8 *crc_bytes;
> +	u16 crc_calc;
> +	int error;
> +	u8 len;
> +
> +	error = axiom_read(ts, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
> +	if (error)
> +		return error;
> +
> +	len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) << 1;
> +	if (len <= 2) {
> +		dev_err(dev, "Zero length report discarded.\n");
> +		return -ENODATA;
> +	}
> +
> +	/* Validate the report CRC */
> +	crc_bytes = &report_data[len];
> +
> +	crc_report = get_unaligned_le16(crc_bytes - 2);
> +	/* Length is in 16 bit words and remove the size of the CRC16 itself */
> +	crc_calc = crc16(0, report_data, (len - 2));
> +
> +	if (crc_calc != crc_report) {
> +		dev_err(dev,
> +			"CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> +			crc_report, crc_calc);
> +		return -EINVAL;
> +	}
> +
> +	switch (report_data[1]) {
> +	case AXIOM_USAGE_2DCTS_REPORT_ID:
> +		if (axiom_process_u41_report(ts, &report_data[1])) {
> +			input_mt_sync_frame(input_dev);
> +			input_sync(input_dev);
> +		}
> +		break;
> +
> +	case AXIOM_USAGE_2AUX_REPORT_ID:
> +		/* This is an aux report (force), not used currently */
> +		break;
> +
> +	case AXIOM_USAGE_2HB_REPORT_ID:
> +		/* This is a heartbeat report */
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void axiom_i2c_poll(struct input_dev *input_dev)
> +{
> +	struct axiom_data *ts = input_get_drvdata(input_dev);
> +
> +	axiom_handle_events(ts);
> +}
> +
> +static irqreturn_t axiom_irq(int irq, void *dev_id)
> +{
> +	struct axiom_data *ts = dev_id;
> +
> +	axiom_handle_events(ts);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void axiom_reset(struct gpio_desc *reset_gpio)
> +{
> +	gpiod_set_value_cansleep(reset_gpio, 1);
> +	usleep_range(1000, 2000);
> +	gpiod_set_value_cansleep(reset_gpio, 0);
> +	msleep(AXIOM_STARTUP_TIME_MS);
> +}
> +
> +static int axiom_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct input_dev *input_dev;
> +	struct axiom_data *ts;
> +	u32 poll_interval;
> +	int target;
> +	int error;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, ts);
> +	ts->client = client;
> +	ts->dev = dev;
> +
> +	ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
> +	error = PTR_ERR_OR_ZERO(ts->regmap);
> +	if (error) {
> +		dev_err(dev, "Failed to initialize regmap: %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_regulator_get_enable(dev, "vddi");
> +	if (error)
> +		return dev_err_probe(&client->dev, error,
> +				     "Failed to enable VDDI regulator\n");
> +
> +	error = devm_regulator_get_enable(dev, "vdda");
> +	if (error)
> +		return dev_err_probe(&client->dev, error,
> +				     "Failed to enable VDDA regulator\n");
> +
> +	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");
> +
> +	/* Make sure the time after a power ON sequence is meet */
> +	if (ts->reset_gpio)
> +		axiom_reset(ts->reset_gpio);
> +	else
> +		msleep(AXIOM_STARTUP_TIME_MS);
> +
> +	error = axiom_discover(ts);
> +	if (error)
> +		return dev_err_probe(dev, error, "Failed touchscreen discover\n");
> +
> +	input_dev = devm_input_allocate_device(ts->dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->name = "TouchNetix axiom Touchscreen";
> +	input_dev->phys = "input/axiom_ts";
> +
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> +	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_FINGER, 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);
> +
> +	touchscreen_parse_properties(input_dev, true, &ts->prop);
> +
> +	/* Registers the axiom device as a touchscreen instead of a mouse pointer */
> +	error = input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
> +	if (error)
> +		return error;
> +
> +	/* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
> +	set_bit(EV_MSC, input_dev->evbit);
> +	/* Declare that we support "RAW" Miscellaneous events */
> +	set_bit(MSC_RAW, input_dev->mscbit);

Neither of these event types are reported; please drop them.

> +
> +	ts->input_dev = input_dev;
> +	input_set_drvdata(ts->input_dev, ts);
> +
> +	/* Ensure that all reports are initialised to not be present. */
> +	for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
> +		ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
> +
> +	if (client->irq) {
> +		error = devm_request_threaded_irq(dev, client->irq, NULL,
> +						  axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
> +		if (error)
> +			dev_err_probe(dev, error, "failed to request irq");
> +	} else {
> +		error = input_setup_polling(input_dev, axiom_i2c_poll);
> +		if (error)
> +			return dev_err_probe(ts->dev, error, "Unable to set up polling mode\n");
> +
> +		if (!device_property_read_u32(ts->dev, "poll-interval", &poll_interval))
> +			input_set_poll_interval(input_dev, poll_interval);
> +		else
> +			input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> +	}
> +
> +	error = input_register_device(input_dev);
> +	if (error)
> +		return dev_err_probe(ts->dev, error, "failed to register input device\n");
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> +	{ "ax54a" },
> +	{ },

Please drop the comma after the sentinel as was done below.

> +};
> +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> +
> +static const struct of_device_id axiom_i2c_of_match[] = {
> +	{ .compatible = "touchnetix,ax54a", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
> +
> +static struct i2c_driver axiom_i2c_driver = {
> +	.driver = {
> +		   .name = "axiom",
> +		   .of_match_table = axiom_i2c_of_match,
> +	},
> +	.id_table = axiom_i2c_id_table,
> +	.probe = axiom_i2c_probe,
> +};
> +module_i2c_driver(axiom_i2c_driver);
> +
> +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

Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Kamel BOUHARA]

Le 2024-07-03 20:22, Marco Felsch a écrit :
> Hi Kamel,
> 
> thank you for the updated version please see my comment inline.

Hi Marco,

Thanks for this new review :) !

[...]

>> +
>> +struct axiom_data {
>> +	struct axiom_devinfo devinfo;
>> +	struct device *dev;
>> +	struct gpio_desc *reset_gpio;
> 			    ^
> The reset is done within the probe so this can be dropped.
> 

Ack, thanks.

>> +	struct i2c_client *client;
> 			    ^
> The client is never used. I would either store the client or the device
> reference but not both.

Sure, nice catch, I'll just keep the device reference here.

> 
>> +	struct input_dev *input_dev;
>> +	u32 max_report_len;
>> +	u8 rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
> 
> Is there a reason for having the rx_buf within the driver priv data?
> IMHO it's more error probe since we never set it to zero before we use
> the buffer. I would rather move the rx-buffer to the functions which
> perform the read.

Ok, there no real/technical reason to not move it to reading functions.

> 
>> +	struct axiom_u41_target targets[AXIOM_U41_MAX_TARGETS];
>> +	struct axiom_usage_entry usage_table[AXIOM_U31_MAX_USAGES];
>> +	bool usage_table_populated;
> 		^
> This can be an inline helper which checks the max_report_len e.g:
> 
> static inline bool axiom_usage_table_populated(struct axiom_data *ts)
> {
> 	return ts->max_report_len;
> }
> 

Ack, just for my curiosity, is this for perfomance or clarity sake ?

>> +	struct regmap *regmap;
>> +	struct touchscreen_properties	prop;
>> +};
>> +

[...]

>> +static int axiom_i2c_probe(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct input_dev *input_dev;
>> +	struct axiom_data *ts;
>> +	u32 poll_interval;
>> +	int target;
>> +	int error;
>> +
>> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
>> +	if (!ts)
>> +		return -ENOMEM;
>> +
>> +	i2c_set_clientdata(client, ts);
>> +	ts->client = client;
>> +	ts->dev = dev;
>> +
>> +	ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
>> +	error = PTR_ERR_OR_ZERO(ts->regmap);
>> +	if (error) {
>> +		dev_err(dev, "Failed to initialize regmap: %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	error = devm_regulator_get_enable(dev, "vddi");
>> +	if (error)
>> +		return dev_err_probe(&client->dev, error,
>> +				     "Failed to enable VDDI regulator\n");
>> +
>> +	error = devm_regulator_get_enable(dev, "vdda");
>> +	if (error)
>> +		return dev_err_probe(&client->dev, error,
>> +				     "Failed to enable VDDA regulator\n");
>> +
>> +	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");
>> +
>> +	/* Make sure the time after a power ON sequence is meet */
>> +	if (ts->reset_gpio)
>> +		axiom_reset(ts->reset_gpio);
>> +	else
> 
> No else just:

Fixed, thanks !

> 
>> +		msleep(AXIOM_STARTUP_TIME_MS);
> 
> 	msleep(AXIOM_STARTUP_TIME_MS);
> 
> and drop the msleep within the axiom_reset().
> 
> Regards,
>   Marco

[...]

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Kamel BOUHARA]

Le 2024-07-07 21:20, Jeff LaBundy a écrit :
> Hi Kamel,
> 

Hello Jeff,

> On Wed, Jul 03, 2024 at 04:25:18PM +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>
>> ---
> 
> This is coming together nicely; just a few trailing comments on
> top of Marco's feedback.
> 
>>  MAINTAINERS                                  |   1 +
>>  drivers/input/touchscreen/Kconfig            |  14 +
>>  drivers/input/touchscreen/Makefile           |   1 +
>>  drivers/input/touchscreen/touchnetix_axiom.c | 616 
>> +++++++++++++++++++
>>  4 files changed, 632 insertions(+)
>>  create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2041384d3866..ac6b612bfbba 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -22745,6 +22745,7 @@ M:	Kamel Bouhara <kamel.bouhara@bootlin.com>
>>  L:	linux-input@vger.kernel.org
>>  S:	Maintained
>>  
>> F:	Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
>> +F:	drivers/input/touchscreen/touchnetix_axiom.c
>> 
>>  TPM DEVICE DRIVER
>>  M:	Peter Huewe <peterhuewe@gmx.de>
>> diff --git a/drivers/input/touchscreen/Kconfig 
>> b/drivers/input/touchscreen/Kconfig
>> index c821fe3ee794..1ce8f1c25625 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -834,6 +834,20 @@ config TOUCHSCREEN_MIGOR
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called migor_ts.
>> 
>> +config TOUCHSCREEN_TOUCHNETIX_AXIOM
>> +	tristate "TouchNetix AXIOM based touchscreen controllers"
>> +	depends on I2C
>> +	select CRC16
>> +	select REGMAP_I2C
>> +	help
>> +	  Say Y here if you have a axiom touchscreen connected to
>> +	  your system.
>> +
>> +	  If unsure, say N.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called axiom.
>> +
>>  config TOUCHSCREEN_TOUCHRIGHT
>>  	tristate "Touchright serial touchscreen"
>>  	select SERIO
>> diff --git a/drivers/input/touchscreen/Makefile 
>> b/drivers/input/touchscreen/Makefile
>> index a81cb5aa21a5..6ce7b804adc7 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -91,6 +91,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40)		+= sur40.o
>>  obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI)	+= surface3_spi.o
>>  obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC)	+= ti_am335x_tsc.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
>> +obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM)	+= touchnetix_axiom.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
>>  obj-$(CONFIG_TOUCHSCREEN_TS4800)	+= ts4800-ts.o
>> diff --git a/drivers/input/touchscreen/touchnetix_axiom.c 
>> b/drivers/input/touchscreen/touchnetix_axiom.c
>> new file mode 100644
>> index 000000000000..cea52dafc913
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/touchnetix_axiom.c
>> @@ -0,0 +1,616 @@
>> +// 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>
>> + *
>> + */
> 
> Please include bits.h as well.
> 

Ack, thanks !

[...]

>> +/*
>> + * axiom devices are typically configured to report touches at a rate
>> + * of 100Hz (10ms) for systems that require polling for reports.
>> + * When reports are polled, it will be expected to occasionally
>> + * observe the overflow bit being set in the reports.
>> + * This indicates that reports are not being read fast enough.
> 
> This comment is strange; if reports are not read quickly enough at
> the default rate, why not set the default rate at the slowest for
> which the overflow bit is not set?
> 

The rate has been set to the give a good user experience regardless of
this overflow bit that isn't even processed here.

>> + */
>> +#define POLL_INTERVAL_DEFAULT_MS 10
>> +
>> +/* Translate usage/page/offset triplet into physical address. */
>> +static u16 axiom_usage_to_target_address(struct axiom_data *ts, u8 
>> usage, u8 page,
>> +					 char offset)
>> +{
>> +	/* 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;
> 
> Checkpatch normally gripes if an else follows a return; did that not
> happen here? You should simplify it either way:
> 
>         if (...) {
>                 if (...)
>                         return ...;
> 
>                 return U16_MAX;
>         }
> 

Fixed thanks !
No, checkpatch didn't raised any issue here with "--strict", am I 
missing something ?

>> +	}
>> +
>> +	if (page >= ts->usage_table[usage].num_pages) {
>> +		dev_err(ts->dev, "Invalid usage table! usage: u%02x, page: %02x, 
>> offset: %02x\n",
>> +			usage, page, offset);
>> +		return 0xffff;
>> +	}
>> +
>> +	return ((ts->usage_table[usage].start_page + page) << 8) + offset;
>> +}
>> +
>> +static int axiom_read(struct axiom_data *ts, u8 usage, u8 page, void 
>> *buf, u16 len)
>> +{
>> +	union axiom_cmd_header cmd_header;
>> +	int ret;
> 
> For consistency, please use 'error' as is done throughout.
> 

Sure, thanks.

>> +
>> +	cmd_header.head.target_address =
>> +		cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
>> +	cmd_header.head.length = cpu_to_le16(len | 
>> AXIOM_CMD_HEADER_READ_MASK);
>> +
>> +	ret = regmap_write(ts->regmap, le32_to_cpu(cmd_header.preamble), 0);
>> +	if (ret) {
>> +		dev_err(ts->dev, "failed to write preamble, error %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regmap_raw_read(ts->regmap, 0, buf, len);
>> +	if (ret) {
>> +		dev_err(ts->dev, "failed to read target address %04x, error %d\n",
>> +			cmd_header.head.target_address, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * One of the main purposes for reading the usage table is to 
>> identify
>> + * which usages reside at which target address.
>> + * When performing subsequent reads or writes to AXIOM, the target 
>> address
>> + * is used to specify which usage is being accessed.
>> + * Consider the following discovery code which will build up the 
>> usage table.
>> + */
>> +static u32 axiom_populate_usage_table(struct axiom_data *ts)
>> +{
>> +	struct axiom_usage_entry *usage_table;
>> +	u8 *rx_data = ts->rx_buf;
>> +	u32 max_report_len = 0;
>> +	u32 usage_id;
>> +	int error;
>> +
>> +	usage_table = ts->usage_table;
>> +
>> +	/* Read the second page of usage u31 to get the usage table */
>> +	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
>> +			   (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
>> +
>> +	if (error)
>> +		return error;
>> +
>> +	for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
>> +		u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
>> +		u8 id = rx_data[offset + 0];
>> +		u8 start_page = rx_data[offset + 1];
>> +		u8 num_pages = rx_data[offset + 2];
>> +		u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 
>> 1) * 2;
>> +
>> +		usage_table[id].is_report = !num_pages;
>> +
>> +		/* Store the entry into the usage table */
>> +		usage_table[id].id = id;
>> +		usage_table[id].start_page = start_page;
>> +		usage_table[id].num_pages = num_pages;
>> +
>> +		dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id, 
>> AXIOM_U31_BYTES_PER_USAGE,
>> +			&rx_data[offset]);
>> +
>> +		/* Identify the max report length the module will receive */
>> +		if (usage_table[id].is_report && max_offset > max_report_len)
>> +			max_report_len = max_offset;
>> +	}
>> +
>> +	ts->usage_table_populated = true;
>> +
>> +	return max_report_len;
>> +}
>> +
>> +static int axiom_discover(struct axiom_data *ts)
>> +{
>> +	int error;
>> +
>> +	/*
>> +	 * Fetch the first page of usage u31 to get the
>> +	 * device information and the number of usages
>> +	 */
>> +	error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 0, &ts->devinfo, 
>> AXIOM_U31_PAGE0_LENGTH);
> 
> It seems a little safer, and more intuitive, to pass 
> sizeof(ts->devinfo) instead
> of AXIOM_U31_PAGE0_LENGTH. To that end, devinfo is only 11 bytes in 
> size, but
> you're reading 12 bytes. I'm guessing the compiler is padding 
> axiom_data which is
> the only reason the existing implementation seems to work.
> 

OK, I tough this actually could be good to keep this as it help knowing 
which register/page
is used to gatter device information here.

I'll however take your suggestion, thanks !

[...]

>> +
>> +	/* Enables the raw data for up to 4 force channels to be sent to the 
>> input subsystem */
>> +	set_bit(EV_MSC, input_dev->evbit);
>> +	/* Declare that we support "RAW" Miscellaneous events */
>> +	set_bit(MSC_RAW, input_dev->mscbit);
> 
> Neither of these event types are reported; please drop them.
> 

Fixed.

>> +
>> +	ts->input_dev = input_dev;
>> +	input_set_drvdata(ts->input_dev, ts);
>> +
>> +	/* Ensure that all reports are initialised to not be present. */
>> +	for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
>> +		ts->targets[target].state = AXIOM_TARGET_STATE_NOT_PRESENT;
>> +
>> +	if (client->irq) {
>> +		error = devm_request_threaded_irq(dev, client->irq, NULL,
>> +						  axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
>> +		if (error)
>> +			dev_err_probe(dev, error, "failed to request irq");
>> +	} else {
>> +		error = input_setup_polling(input_dev, axiom_i2c_poll);
>> +		if (error)
>> +			return dev_err_probe(ts->dev, error, "Unable to set up polling 
>> mode\n");
>> +
>> +		if (!device_property_read_u32(ts->dev, "poll-interval", 
>> &poll_interval))
>> +			input_set_poll_interval(input_dev, poll_interval);
>> +		else
>> +			input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
>> +	}
>> +
>> +	error = input_register_device(input_dev);
>> +	if (error)
>> +		return dev_err_probe(ts->dev, error, "failed to register input 
>> device\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id axiom_i2c_id_table[] = {
>> +	{ "ax54a" },
>> +	{ },
> 
> Please drop the comma after the sentinel as was done below.
> 

Fixed, thanks.

>> +};
>> +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
>> +
>> +static const struct of_device_id axiom_i2c_of_match[] = {
>> +	{ .compatible = "touchnetix,ax54a", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
>> +
>> +static struct i2c_driver axiom_i2c_driver = {
>> +	.driver = {
>> +		   .name = "axiom",
>> +		   .of_match_table = axiom_i2c_of_match,
>> +	},
>> +	.id_table = axiom_i2c_id_table,
>> +	.probe = axiom_i2c_probe,
>> +};
>> +module_i2c_driver(axiom_i2c_driver);
>> +
>> +MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
>> +MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
>> +MODULE_AUTHOR("Mark Satterthwaite 
>> <mark.satterthwaite@touchnetix.com>");
>> +MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
>> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
>> +MODULE_DESCRIPTION("TouchNetix axiom touchscreen I2C bus driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.25.1
>> 
> 
> Kind regards,
> Jeff LaBundy

Thanks for this review !

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Kamel BOUHARA]

[...]

>>> +/*
>>> + * axiom devices are typically configured to report touches at a 
>>> rate
>>> + * of 100Hz (10ms) for systems that require polling for reports.
>>> + * When reports are polled, it will be expected to occasionally
>>> + * observe the overflow bit being set in the reports.
>>> + * This indicates that reports are not being read fast enough.
>> 
>> This comment is strange; if reports are not read quickly enough at
>> the default rate, why not set the default rate at the slowest for
>> which the overflow bit is not set?
>> 
> 
> The rate has been set to the give a good user experience regardless of
> this overflow bit that isn't even processed here.
> 

and therefore I might remove the part refering to this overflow bit in 
the comment.

Regards,
Kamel