* [PATCH v6 1/2] dt-bindings: input: touchscreen: iqs5xx: Add bindings
@ 2019-02-10 22:39 Jeff LaBundy
2019-02-10 22:39 ` [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525 Jeff LaBundy
0 siblings, 1 reply; 5+ messages in thread
From: Jeff LaBundy @ 2019-02-10 22:39 UTC (permalink / raw)
To: dmitry.torokhov, devicetree
Cc: linux-input, rydberg, robh+dt, mark.rutland, Jeff LaBundy
This patch adds binding documentation for the Azoteq IQS550/572/525
family of trackpad/touchscreen controllers.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v6:
- None
Changes in v5:
- None
Changes in v4:
- None
Changes in v3:
- Added Reviewed-by trailer
Changes in v2:
- Separated each valid "compatible" property with a line break
- Specified the polarity of the RDY and NRST pins
- Replaced duplicate definitions of common touchscreen properties with a
reference to touchscreen.txt
- Specified the example node as "touchscreen@74"
.../bindings/input/touchscreen/iqs5xx.txt | 109 +++++++++++++++++++++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
2 files changed, 110 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt
diff --git a/Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt b/Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt
new file mode 100644
index 0000000..25ee2a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/iqs5xx.txt
@@ -0,0 +1,109 @@
+Azoteq IQS550/572/525 Trackpad/Touchscreen Controller
+
+Required properties:
+
+- compatible : Must be equal to one of the following:
+ "azoteq,iqs550"
+ "azoteq,iqs572"
+ "azoteq,iqs525"
+
+- reg : I2C slave address for the device.
+
+- interrupts : GPIO to which the device's active-high RDY
+ output is connected (see [0]).
+
+- reset-gpios : GPIO to which the device's active-low NRST
+ input is connected (see [1]). Hardware reset
+ access is required in order to to facilitate
+ in-system firmware replacement.
+
+Optional properties:
+
+- azoteq,exp-ver-major : Major field of the export file version number
+ (0 through 255 inclusive). This property must
+ be specified as a '/bits/ 8' value.
+
+- azoteq,exp-ver-minor : Minor field of the export file version number
+ (0 through 255 inclusive). This property must
+ be specified as a '/bits/ 8' value.
+
+ The export file version number is a customer-
+ assigned parameter embedded in the device's
+ firmware at the time it is exported from the
+ manufacturer's PC-based configuration tool.
+
+ If the export file version number stored in
+ the device's nonvolatile memory does not match
+ the number specified here, the driver replaces
+ the firmware on the device with that which is
+ stored on the local filesystem (for which the
+ export file version number must match).
+
+ If both major and minor fields are omitted or
+ specified as zero, the driver does not query
+ the device's export file version number.
+
+- touchscreen-min-x : See [2].
+
+- touchscreen-min-y : See [2].
+
+- touchscreen-size-x : See [2]. If this property is omitted, the
+ maximum x-coordinate is specified by the
+ device's "X Resolution" register.
+
+- touchscreen-size-y : See [2]. If this property is omitted, the
+ maximum y-coordinate is specified by the
+ device's "Y Resolution" register.
+
+- touchscreen-max-pressure : See [2]. Pressure is expressed as the sum of
+ the deltas across all channels impacted by a
+ touch event. A channel's delta is calculated
+ as its count value minus a reference, where
+ the count value is inversely proportional to
+ the channel's capacitance.
+
+- touchscreen-fuzz-x : See [2].
+
+- touchscreen-fuzz-y : See [2].
+
+- touchscreen-fuzz-pressure : See [2].
+
+- touchscreen-inverted-x : See [2]. Inversion is applied relative to that
+ which may already be specified by the device's
+ FLIP_X and FLIP_Y register fields.
+
+- touchscreen-inverted-y : See [2]. Inversion is applied relative to that
+ which may already be specified by the device's
+ FLIP_X and FLIP_Y register fields.
+
+- touchscreen-swapped-x-y : See [2]. Swapping is applied relative to that
+ which may already be specified by the device's
+ SWITCH_XY_AXIS register fields.
+
+[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+[1]: Documentation/devicetree/bindings/gpio/gpio.txt
+[2]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
+
+Example:
+
+ &i2c1 {
+ /* ... */
+
+ touchscreen@74 {
+ compatible = "azoteq,iqs550";
+ reg = <0x74>;
+ interrupt-parent = <&gpio>;
+ interrupts = <17 1>;
+ reset-gpios = <&gpio 27 0>;
+
+ azoteq,exp-ver-major = /bits/ 8 <1>;
+ azoteq,exp-ver-minor = /bits/ 8 <0>;
+
+ touchscreen-size-x = <640>;
+ touchscreen-size-y = <480>;
+
+ touchscreen-max-pressure = <16000>;
+ };
+
+ /* ... */
+ };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 4b1a2a8..51f9954 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -53,6 +53,7 @@ avic Shanghai AVIC Optoelectronics Co., Ltd.
avnet Avnet, Inc.
axentia Axentia Technologies AB
axis Axis Communications AB
+azoteq Azoteq (Pty) Ltd
bananapi BIPAI KEJI LIMITED
bhf Beckhoff Automation GmbH & Co. KG
bitmain Bitmain Technologies
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525
2019-02-10 22:39 [PATCH v6 1/2] dt-bindings: input: touchscreen: iqs5xx: Add bindings Jeff LaBundy
@ 2019-02-10 22:39 ` Jeff LaBundy
2019-02-18 6:40 ` Dmitry Torokhov
0 siblings, 1 reply; 5+ messages in thread
From: Jeff LaBundy @ 2019-02-10 22:39 UTC (permalink / raw)
To: dmitry.torokhov, devicetree
Cc: linux-input, rydberg, robh+dt, mark.rutland, Jeff LaBundy
This patch adds support for the Azoteq IQS550/572/525 family of
trackpad/touchscreen controllers.
The driver has been tested with an IQS550EV02 evaluation board. A
demonstration of the driver's capabilities is available here:
https://youtu.be/sRNNx4XZBts
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v6:
- None
Changes in v5:
- Demoted the first instance of dev_err in iqs5xx_update_firmware to dev_info
- Updated the commit message to describe the test setup and provide a link to
a demonstration of the driver's capabilities
Changes in v4:
- Replaced 'ret' with 'error' where applicable
- Made use of packed structures to parse device ID and touch data registers
as well as firmware records
- Overhauled iqs5xx_update_firmware to make use of existing helper functions
and to be robust against erroneous firmware files
- Refactored and fortified the bootloader steps in iqs5xx_update_firmware by
allowing additional retries upon failure
- Optimized register read/write functions and added wrappers for single-byte
transactions
- Updated the error handling in iqs5xx_set_state and removed the superfluous
'disabled' flag
- General improvements to error messages returned via dev_err
Changes in v3:
- None
Changes in v2:
- Rebased on top of updated iqs5xx.txt
- Refactored the axis configuration logic in iqs5xx_dev_init to adhere to the
behavior defined in touchscreen.txt
drivers/input/touchscreen/Kconfig | 10 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/iqs5xx.c | 1015 ++++++++++++++++++++++++++++++++++++
3 files changed, 1026 insertions(+)
create mode 100644 drivers/input/touchscreen/iqs5xx.c
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 6c16aae..d8337ed 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1311,4 +1311,14 @@ config TOUCHSCREEN_ROHM_BU21023
To compile this driver as a module, choose M here: the
module will be called bu21023_ts.
+config TOUCHSCREEN_IQS5XX
+ tristate "Azoteq IQS550/572/525 trackpad/touchscreen controller"
+ depends on I2C
+ help
+ Say Y to enable support for the Azoteq IQS550/572/525
+ family of trackpad/touchscreen controllers.
+
+ To compile this driver as a module, choose M here: the
+ module will be called iqs5xx.
+
endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index fcc7605..084a596 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -110,3 +110,4 @@ obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50) += colibri-vf50-ts.o
obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o
obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
+obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
new file mode 100644
index 0000000..80d6da1
--- /dev/null
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -0,0 +1,1015 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Azoteq IQS550/572/525 Trackpad/Touchscreen Controller
+ *
+ * Copyright (C) 2018
+ * Author: Jeff LaBundy <jeff@labundy.com>
+ *
+ * Note: these devices require firmware exported from a PC-based configuration
+ * tool made available by the manufacturer. The driver expects the firmware to
+ * be named according to the device (e.g. iqs550.hex for IQS550).
+ *
+ * Link to PC-based configuration tool and data sheet: http://www.azoteq.com/
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/firmware.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/of_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <asm/unaligned.h>
+
+#define IQS5XX_NUM_RETRIES 10
+#define IQS5XX_NUM_POINTS 256
+#define IQS5XX_NUM_CONTACTS 5
+#define IQS5XX_WR_BYTES_MAX 2
+
+#define IQS5XX_PROD_NUM_IQS550 40
+#define IQS5XX_PROD_NUM_IQS572 58
+#define IQS5XX_PROD_NUM_IQS525 52
+#define IQS5XX_PROJ_NUM_A000 0
+#define IQS5XX_PROJ_NUM_B000 15
+#define IQS5XX_MAJOR_VER_SPEC 2
+
+#define IQS5XX_RESUME 0x00
+#define IQS5XX_SUSPEND 0x01
+
+#define IQS5XX_SW_INPUT_EVENT 0x10
+#define IQS5XX_SETUP_COMPLETE 0x40
+#define IQS5XX_EVENT_MODE 0x01
+#define IQS5XX_TP_EVENT 0x04
+
+#define IQS5XX_FLIP_X 0x01
+#define IQS5XX_FLIP_Y 0x02
+#define IQS5XX_SWITCH_XY_AXIS 0x04
+
+#define IQS5XX_PROD_NUM 0x0000
+#define IQS5XX_ABS_X 0x0016
+#define IQS5XX_ABS_Y 0x0018
+#define IQS5XX_SYS_CTRL0 0x0431
+#define IQS5XX_SYS_CTRL1 0x0432
+#define IQS5XX_SYS_CFG0 0x058E
+#define IQS5XX_SYS_CFG1 0x058F
+#define IQS5XX_TOTAL_RX 0x063D
+#define IQS5XX_TOTAL_TX 0x063E
+#define IQS5XX_XY_CFG0 0x0669
+#define IQS5XX_X_RES 0x066E
+#define IQS5XX_Y_RES 0x0670
+#define IQS5XX_EXP_VER 0x0677
+#define IQS5XX_CHKSM 0x83C0
+#define IQS5XX_APP 0x8400
+#define IQS5XX_CSTM 0xBE00
+#define IQS5XX_PMAP_END 0xBFFF
+#define IQS5XX_END_COMM 0xEEEE
+
+#define IQS5XX_CHKSM_LEN (IQS5XX_APP - IQS5XX_CHKSM)
+#define IQS5XX_APP_LEN (IQS5XX_CSTM - IQS5XX_APP)
+#define IQS5XX_CSTM_LEN (IQS5XX_PMAP_END + 1 - IQS5XX_CSTM)
+#define IQS5XX_PMAP_LEN (IQS5XX_PMAP_END + 1 - IQS5XX_CHKSM)
+
+#define IQS5XX_FW_NAME_LEN 11
+#define IQS5XX_REC_HDR_LEN 4
+#define IQS5XX_REC_LEN_MAX 256
+#define IQS5XX_REC_TYPE_DATA 0x00
+#define IQS5XX_REC_TYPE_EOF 0x01
+
+#define IQS5XX_BL_ADDR_MASK 0x40
+#define IQS5XX_BL_CMD_VER 0x00
+#define IQS5XX_BL_CMD_READ 0x01
+#define IQS5XX_BL_CMD_EXEC 0x02
+#define IQS5XX_BL_CMD_CRC 0x03
+#define IQS5XX_BL_BLK_LEN_MAX 64
+#define IQS5XX_BL_ID 0x0200
+#define IQS5XX_BL_CRC_PASS 0x00
+#define IQS5XX_BL_CRC_FAIL 0x01
+#define IQS5XX_BL_ATTEMPTS 3
+
+struct iqs5xx_private {
+ struct i2c_client *client;
+ struct input_dev *input;
+ struct gpio_desc *reset_gpio;
+};
+
+struct iqs5xx_dev_id_info {
+ __be16 prod_num;
+ __be16 proj_num;
+ u8 major_ver;
+ u8 minor_ver;
+ u8 bl_status;
+} __packed;
+
+struct iqs5xx_ihex_rec {
+ char start;
+ char len[2];
+ char addr[4];
+ char type[2];
+ char data[2];
+} __packed;
+
+struct iqs5xx_touch_data {
+ __be16 abs_x;
+ __be16 abs_y;
+ __be16 strength;
+ u8 area;
+} __packed;
+
+static int iqs5xx_read_burst(struct i2c_client *client,
+ u16 reg, u8 *val8, u8 len)
+{
+ __be16 reg_buf = cpu_to_be16(reg);
+ int ret, i;
+ struct i2c_msg msg[] = {
+ {
+ .addr = client->addr,
+ .flags = 0,
+ .len = sizeof(reg_buf),
+ .buf = (u8 *)®_buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = val8,
+ },
+ };
+
+ /*
+ * The first addressing attempt outside of a communication window fails
+ * and must be retried, after which the device clock stretches until it
+ * is available.
+ */
+ for (i = 0; i < IQS5XX_NUM_RETRIES; i++) {
+ ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+ if (ret == ARRAY_SIZE(msg))
+ return 0;
+
+ usleep_range(200, 300);
+ }
+
+ if (ret >= 0)
+ ret = -EIO;
+
+ dev_err(&client->dev, "Failed to read from address 0x%04X: %d\n",
+ reg, ret);
+
+ return ret;
+}
+
+static int iqs5xx_read_word(struct i2c_client *client, u16 reg, u16 *val)
+{
+ __be16 val_buf;
+ int error;
+
+ error = iqs5xx_read_burst(client, reg, (u8 *)&val_buf, sizeof(val_buf));
+ if (error)
+ return error;
+
+ *val = be16_to_cpu(val_buf);
+
+ return 0;
+}
+
+static int iqs5xx_read_byte(struct i2c_client *client, u16 reg, u8 *val8)
+{
+ return iqs5xx_read_burst(client, reg, val8, sizeof(*val8));
+}
+
+static int iqs5xx_write_burst(struct i2c_client *client,
+ u16 reg, u8 *val8, u8 len)
+{
+ int ret, i;
+ u8 mlen = sizeof(reg) + len;
+ u8 mbuf[sizeof(reg) + IQS5XX_WR_BYTES_MAX];
+
+ if (len > IQS5XX_WR_BYTES_MAX)
+ return -EINVAL;
+
+ put_unaligned_be16(reg, mbuf);
+ memcpy(mbuf + sizeof(reg), val8, len);
+
+ /*
+ * The first addressing attempt outside of a communication window fails
+ * and must be retried, after which the device clock stretches until it
+ * is available.
+ */
+ for (i = 0; i < IQS5XX_NUM_RETRIES; i++) {
+ ret = i2c_master_send(client, (char *)mbuf, mlen);
+ if (ret == mlen)
+ return 0;
+
+ usleep_range(200, 300);
+ }
+
+ if (ret >= 0)
+ ret = -EIO;
+
+ dev_err(&client->dev, "Failed to write to address 0x%04X: %d\n",
+ reg, ret);
+
+ return ret;
+}
+
+static int iqs5xx_write_word(struct i2c_client *client, u16 reg, u16 val)
+{
+ __be16 val_buf = cpu_to_be16(val);
+
+ return iqs5xx_write_burst(client, reg, (u8 *)&val_buf, sizeof(val_buf));
+}
+
+static int iqs5xx_write_byte(struct i2c_client *client, u16 reg, u8 val8)
+{
+ return iqs5xx_write_burst(client, reg, &val8, sizeof(val8));
+}
+
+static void iqs5xx_reset(struct i2c_client *client)
+{
+ struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
+
+ gpiod_set_value_cansleep(iqs5xx->reset_gpio, 0);
+ usleep_range(200, 300);
+
+ gpiod_set_value_cansleep(iqs5xx->reset_gpio, 1);
+}
+
+static int iqs5xx_bl_cmd(struct i2c_client *client, u8 bl_cmd, u16 bl_addr)
+{
+ struct i2c_msg msg;
+ int ret;
+ u8 mbuf[sizeof(bl_cmd) + sizeof(bl_addr)];
+
+ msg.addr = client->addr ^ IQS5XX_BL_ADDR_MASK;
+ msg.flags = 0;
+ msg.len = sizeof(bl_cmd);
+ msg.buf = mbuf;
+
+ *mbuf = bl_cmd;
+
+ switch (bl_cmd) {
+ case IQS5XX_BL_CMD_VER:
+ case IQS5XX_BL_CMD_CRC:
+ case IQS5XX_BL_CMD_EXEC:
+ break;
+ case IQS5XX_BL_CMD_READ:
+ msg.len += sizeof(bl_addr);
+ put_unaligned_be16(bl_addr, mbuf + sizeof(bl_cmd));
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret != 1)
+ goto msg_fail;
+
+ switch (bl_cmd) {
+ case IQS5XX_BL_CMD_VER:
+ msg.len = sizeof(u16);
+ break;
+ case IQS5XX_BL_CMD_CRC:
+ msg.len = sizeof(u8);
+ /*
+ * This delay saves the bus controller the trouble of having to
+ * tolerate a relatively long clock-stretching period while the
+ * CRC is calculated.
+ */
+ msleep(50);
+ break;
+ case IQS5XX_BL_CMD_EXEC:
+ usleep_range(10000, 10100);
+ /* intentionally fall through */
+ default:
+ return 0;
+ }
+
+ msg.flags = I2C_M_RD;
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret != 1)
+ goto msg_fail;
+
+ if (bl_cmd == IQS5XX_BL_CMD_VER
+ && get_unaligned_be16(mbuf) != IQS5XX_BL_ID) {
+ dev_err(&client->dev, "Unexpected bootloader ID: 0x%04X\n",
+ get_unaligned_be16(mbuf));
+ return -EINVAL;
+ }
+
+ if (bl_cmd == IQS5XX_BL_CMD_CRC && *mbuf != IQS5XX_BL_CRC_PASS) {
+ dev_err(&client->dev, "Bootloader CRC failed\n");
+ return -EIO;
+ }
+
+ return 0;
+
+msg_fail:
+ if (ret >= 0)
+ ret = -EIO;
+
+ if (bl_cmd != IQS5XX_BL_CMD_VER)
+ dev_err(&client->dev,
+ "Unsuccessful bootloader command 0x%02X: %d\n",
+ bl_cmd, ret);
+
+ return ret;
+}
+
+static int iqs5xx_bl_write(struct i2c_client *client,
+ u16 bl_addr, u8 *pmap_data, u16 pmap_len)
+{
+ struct i2c_msg msg;
+ int ret, i;
+ u8 mbuf[sizeof(bl_addr) + IQS5XX_BL_BLK_LEN_MAX];
+
+ if (pmap_len % IQS5XX_BL_BLK_LEN_MAX)
+ return -EINVAL;
+
+ msg.addr = client->addr ^ IQS5XX_BL_ADDR_MASK;
+ msg.flags = 0;
+ msg.len = ARRAY_SIZE(mbuf);
+ msg.buf = mbuf;
+
+ for (i = 0; i < pmap_len; i += IQS5XX_BL_BLK_LEN_MAX) {
+ put_unaligned_be16(bl_addr + i, mbuf);
+ memcpy(mbuf + sizeof(bl_addr),
+ pmap_data + i, IQS5XX_BL_BLK_LEN_MAX);
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret != 1)
+ goto msg_fail;
+
+ usleep_range(10000, 10100);
+ }
+
+ return 0;
+
+msg_fail:
+ if (ret >= 0)
+ ret = -EIO;
+
+ dev_err(&client->dev, "Failed to write block at address 0x%04X: %d\n",
+ bl_addr + i, ret);
+
+ return ret;
+}
+
+static int iqs5xx_bl_verify(struct i2c_client *client,
+ u16 bl_addr, u8 *pmap_data, u16 pmap_len)
+{
+ struct i2c_msg msg;
+ int ret, i;
+ u8 bl_data[IQS5XX_BL_BLK_LEN_MAX];
+
+ if (pmap_len % IQS5XX_BL_BLK_LEN_MAX)
+ return -EINVAL;
+
+ msg.addr = client->addr ^ IQS5XX_BL_ADDR_MASK;
+ msg.flags = I2C_M_RD;
+ msg.len = IQS5XX_BL_BLK_LEN_MAX;
+ msg.buf = bl_data;
+
+ for (i = 0; i < pmap_len; i += IQS5XX_BL_BLK_LEN_MAX) {
+ ret = iqs5xx_bl_cmd(client, IQS5XX_BL_CMD_READ, bl_addr + i);
+ if (ret)
+ return ret;
+
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ if (ret != 1)
+ goto msg_fail;
+
+ if (memcmp(bl_data, pmap_data + i, IQS5XX_BL_BLK_LEN_MAX)) {
+ dev_err(&client->dev,
+ "Failed to verify block at address 0x%04X\n",
+ bl_addr + i);
+ return -EIO;
+ }
+ }
+
+ return 0;
+
+msg_fail:
+ if (ret >= 0)
+ ret = -EIO;
+
+ dev_err(&client->dev, "Failed to read block at address 0x%04X: %d\n",
+ bl_addr + i, ret);
+
+ return ret;
+}
+
+static int iqs5xx_update_firmware(struct i2c_client *client)
+{
+ struct iqs5xx_ihex_rec *rec;
+ const struct firmware *fw;
+ char fw_name[IQS5XX_FW_NAME_LEN];
+ int error, i, j;
+ u32 pos = 0;
+ u16 rec_num = 1;
+ u16 rec_addr;
+ u8 rec_len, rec_type, rec_chksm, chksm;
+ u8 rec_hdr[IQS5XX_REC_HDR_LEN];
+ u8 rec_data[IQS5XX_REC_LEN_MAX];
+ u8 *pmap = kzalloc(IQS5XX_PMAP_LEN, GFP_KERNEL);
+
+ if (!pmap)
+ return -ENOMEM;
+
+ snprintf(fw_name, ARRAY_SIZE(fw_name), "%s.hex", client->name);
+ dev_info(&client->dev, "Updating device from firmware %s...\n",
+ fw_name);
+
+ error = request_firmware(&fw, fw_name, &client->dev);
+ if (error) {
+ dev_err(&client->dev, "Failed to request firmware %s: %d\n",
+ fw_name, error);
+ goto err_kfree;
+ }
+
+ do {
+ if (pos + sizeof(*rec) > fw->size) {
+ dev_err(&client->dev, "Insufficient firmware size\n");
+ error = -EINVAL;
+ goto err_rls_fw;
+ }
+ rec = (struct iqs5xx_ihex_rec *)(fw->data + pos);
+ pos += sizeof(*rec);
+
+ if (rec->start != ':') {
+ dev_err(&client->dev, "Invalid start at record %u\n",
+ rec_num);
+ error = -EINVAL;
+ goto err_rls_fw;
+ }
+
+ error = hex2bin(rec_hdr, rec->len, ARRAY_SIZE(rec_hdr));
+ if (error) {
+ dev_err(&client->dev, "Invalid header at record %u\n",
+ rec_num);
+ goto err_rls_fw;
+ }
+
+ rec_len = *rec_hdr;
+ rec_addr = get_unaligned_be16(rec_hdr + sizeof(rec_len));
+ rec_type = *(rec_hdr + sizeof(rec_len) + sizeof(rec_addr));
+
+ if (pos + rec_len * 2 > fw->size) {
+ dev_err(&client->dev, "Insufficient firmware size\n");
+ error = -EINVAL;
+ goto err_rls_fw;
+ }
+ pos += (rec_len * 2);
+
+ error = hex2bin(rec_data, rec->data, rec_len);
+ if (error) {
+ dev_err(&client->dev, "Invalid data at record %u\n",
+ rec_num);
+ goto err_rls_fw;
+ }
+
+ error = hex2bin(&rec_chksm, rec->data + rec_len * 2, 1);
+ if (error) {
+ dev_err(&client->dev, "Invalid checksum at record %u\n",
+ rec_num);
+ goto err_rls_fw;
+ }
+
+ chksm = 0;
+ for (i = 0; i < ARRAY_SIZE(rec_hdr); i++)
+ chksm += rec_hdr[i];
+ for (i = 0; i < rec_len; i++)
+ chksm += rec_data[i];
+ chksm = ~chksm + 1;
+
+ /*
+ * The checksum field for records corresponding to user-exported
+ * settings does not appear to be recalculated when the firmware
+ * is exported, so skip checksum verification for that region.
+ */
+ if (chksm != rec_chksm && rec_addr < IQS5XX_CSTM) {
+ dev_err(&client->dev,
+ "Incorrect checksum at record %u\n",
+ rec_num);
+ error = -EINVAL;
+ goto err_rls_fw;
+ }
+
+ switch (rec_type) {
+ case IQS5XX_REC_TYPE_DATA:
+ if (rec_addr < IQS5XX_CHKSM
+ || rec_addr > IQS5XX_PMAP_END) {
+ dev_err(&client->dev,
+ "Invalid address at record %u\n",
+ rec_num);
+ error = -EINVAL;
+ goto err_rls_fw;
+ }
+ memcpy(pmap + rec_addr - IQS5XX_CHKSM,
+ rec_data, rec_len);
+ break;
+ case IQS5XX_REC_TYPE_EOF:
+ break;
+ default:
+ dev_err(&client->dev, "Invalid type at record %u\n",
+ rec_num);
+ error = -EINVAL;
+ goto err_rls_fw;
+ }
+
+ rec_num++;
+ while (pos < fw->size) {
+ if (*(fw->data + pos) == ':')
+ break;
+ pos++;
+ }
+ } while (rec_type != IQS5XX_REC_TYPE_EOF);
+
+ /*
+ * The device opens a bootloader polling window for 2 ms following the
+ * release of reset. If the host cannot establish communication during
+ * this time frame, it must cycle reset again.
+ */
+ for (i = 0; i < IQS5XX_BL_ATTEMPTS; i++) {
+ iqs5xx_reset(client);
+
+ for (j = 0; j < IQS5XX_NUM_RETRIES; j++) {
+ error = iqs5xx_bl_cmd(client, IQS5XX_BL_CMD_VER, 0);
+ if (error == -EINVAL || !error)
+ break;
+ }
+
+ if (error == -EINVAL)
+ continue;
+
+ if (j == IQS5XX_NUM_RETRIES) {
+ dev_err(&client->dev,
+ "Failed to enter bootloader: %d\n",
+ error);
+ continue;
+ }
+
+ error = iqs5xx_bl_write(client, IQS5XX_CHKSM,
+ pmap, IQS5XX_PMAP_LEN);
+ if (error)
+ continue;
+
+ error = iqs5xx_bl_cmd(client, IQS5XX_BL_CMD_CRC, 0);
+ if (error)
+ continue;
+
+ error = iqs5xx_bl_verify(client, IQS5XX_CSTM,
+ pmap + IQS5XX_CHKSM_LEN + IQS5XX_APP_LEN,
+ IQS5XX_CSTM_LEN);
+ if (error)
+ continue;
+
+ error = iqs5xx_bl_cmd(client, IQS5XX_BL_CMD_EXEC, 0);
+ if (!error)
+ goto err_rls_fw;
+ }
+
+ dev_err(&client->dev, "Aborting update after %d attempts\n", i);
+
+err_rls_fw:
+ release_firmware(fw);
+
+err_kfree:
+ kfree(pmap);
+
+ return error;
+}
+
+static int iqs5xx_dev_id(struct i2c_client *client, __be16 exp_ver_spec)
+{
+ struct iqs5xx_dev_id_info *dev_id_info;
+ int error;
+ u16 val;
+ u8 buf[sizeof(*dev_id_info) + 1];
+
+ /*
+ * Failure to read the device's ID registers could represent corrupted
+ * firmware. In that case, -EAGAIN is returned to prompt the caller to
+ * enter the bootloader. If the device is truly absent, the bootloader
+ * is inaccessible and the caller fails as well.
+ */
+ buf[0] = 0;
+ error = iqs5xx_read_burst(client,
+ IQS5XX_PROD_NUM, &buf[1], sizeof(*dev_id_info));
+ if (error)
+ return -EAGAIN;
+
+ /*
+ * A000 and B000 devices use 8-bit and 16-bit addressing, respectively.
+ * Querying an A000 device's version information with 16-bit addressing
+ * gives the appearance that the data is shifted by one byte; a nonzero
+ * leading array element suggests this could be the case (in which case
+ * the missing zero is prepended).
+ */
+ dev_id_info = (struct iqs5xx_dev_id_info *)&buf[(buf[1] > 0) ? 0 : 1];
+
+ switch (be16_to_cpu(dev_id_info->prod_num)) {
+ case IQS5XX_PROD_NUM_IQS550:
+ case IQS5XX_PROD_NUM_IQS572:
+ case IQS5XX_PROD_NUM_IQS525:
+ break;
+ default:
+ dev_err(&client->dev, "Unrecognized product number: %u\n",
+ be16_to_cpu(dev_id_info->prod_num));
+ return -EINVAL;
+ }
+
+ switch (be16_to_cpu(dev_id_info->proj_num)) {
+ case IQS5XX_PROJ_NUM_A000:
+ dev_err(&client->dev, "Found legacy A000 firmware\n");
+ return -EAGAIN;
+ case IQS5XX_PROJ_NUM_B000:
+ break;
+ default:
+ dev_err(&client->dev, "Unrecognized project number: %u\n",
+ be16_to_cpu(dev_id_info->proj_num));
+ return -EINVAL;
+ }
+
+ if (dev_id_info->major_ver != IQS5XX_MAJOR_VER_SPEC) {
+ dev_err(&client->dev, "Unexpected major version: %u\n",
+ dev_id_info->major_ver);
+ return -EAGAIN;
+ }
+
+ if (be16_to_cpu(exp_ver_spec)) {
+ error = iqs5xx_read_word(client, IQS5XX_EXP_VER, &val);
+ if (error)
+ return error;
+
+ if (val != be16_to_cpu(exp_ver_spec)) {
+ dev_err(&client->dev,
+ "Unexpected export version: %u.%u\n",
+ (val & 0xFF00) >> 8, val & 0x00FF);
+ return -EAGAIN;
+ }
+ }
+
+ return 0;
+}
+
+static int iqs5xx_dev_init(struct i2c_client *client)
+{
+ struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
+ struct touchscreen_properties prop;
+ int error;
+ u16 max_x, max_x_hw;
+ u16 max_y, max_y_hw;
+ u8 val8;
+
+ error = iqs5xx_read_byte(client, IQS5XX_TOTAL_RX, &val8);
+ if (error)
+ return error;
+ max_x_hw = (val8 - 1) * IQS5XX_NUM_POINTS;
+
+ error = iqs5xx_read_byte(client, IQS5XX_TOTAL_TX, &val8);
+ if (error)
+ return error;
+ max_y_hw = (val8 - 1) * IQS5XX_NUM_POINTS;
+
+ error = iqs5xx_read_byte(client, IQS5XX_XY_CFG0, &val8);
+ if (error)
+ return error;
+
+ if (val8 & IQS5XX_SWITCH_XY_AXIS)
+ swap(max_x_hw, max_y_hw);
+
+ touchscreen_parse_properties(iqs5xx->input, true, &prop);
+
+ if (prop.swap_x_y)
+ val8 ^= IQS5XX_SWITCH_XY_AXIS;
+
+ if (prop.invert_x)
+ val8 ^= prop.swap_x_y ? IQS5XX_FLIP_Y : IQS5XX_FLIP_X;
+
+ if (prop.invert_y)
+ val8 ^= prop.swap_x_y ? IQS5XX_FLIP_X : IQS5XX_FLIP_Y;
+
+ error = iqs5xx_write_byte(client, IQS5XX_XY_CFG0, val8);
+ if (error)
+ return error;
+
+ if (prop.max_x > max_x_hw) {
+ dev_err(&client->dev, "Invalid maximum x-coordinate: %u > %u\n",
+ prop.max_x, max_x_hw);
+ return -EINVAL;
+ } else if (prop.max_x == 0) {
+ error = iqs5xx_read_word(client, IQS5XX_X_RES, &max_x);
+ if (error)
+ return error;
+
+ input_abs_set_max(iqs5xx->input, prop.swap_x_y ?
+ ABS_MT_POSITION_Y : ABS_MT_POSITION_X, max_x);
+ } else {
+ max_x = (u16)prop.max_x;
+ }
+
+ if (prop.max_y > max_y_hw) {
+ dev_err(&client->dev, "Invalid maximum y-coordinate: %u > %u\n",
+ prop.max_y, max_y_hw);
+ return -EINVAL;
+ } else if (prop.max_y == 0) {
+ error = iqs5xx_read_word(client, IQS5XX_Y_RES, &max_y);
+ if (error)
+ return error;
+
+ input_abs_set_max(iqs5xx->input, prop.swap_x_y ?
+ ABS_MT_POSITION_X : ABS_MT_POSITION_Y, max_y);
+ } else {
+ max_y = (u16)prop.max_y;
+ }
+
+ /*
+ * Write horizontal and vertical resolution to the device in case its
+ * original defaults were overridden or swapped as per the properties
+ * specified in the device tree.
+ */
+ error = iqs5xx_write_word(client,
+ prop.swap_x_y ? IQS5XX_Y_RES : IQS5XX_X_RES, max_x);
+ if (error)
+ return error;
+
+ error = iqs5xx_write_word(client,
+ prop.swap_x_y ? IQS5XX_X_RES : IQS5XX_Y_RES, max_y);
+ if (error)
+ return error;
+
+ error = iqs5xx_read_byte(client, IQS5XX_SYS_CFG0, &val8);
+ if (error)
+ return error;
+
+ val8 |= IQS5XX_SETUP_COMPLETE;
+ val8 &= ~IQS5XX_SW_INPUT_EVENT;
+ error = iqs5xx_write_byte(client, IQS5XX_SYS_CFG0, val8);
+ if (error)
+ return error;
+
+ val8 = IQS5XX_TP_EVENT | IQS5XX_EVENT_MODE;
+ error = iqs5xx_write_byte(client, IQS5XX_SYS_CFG1, val8);
+ if (error)
+ return error;
+
+ error = iqs5xx_write_byte(client, IQS5XX_END_COMM, 0);
+ if (error)
+ return error;
+
+ /*
+ * The return from this function is delayed so as to avoid a relatively
+ * long clock-stretching period that occurs during open and close call-
+ * backs made when the input device is subsequently registered.
+ */
+ msleep(100);
+
+ return 0;
+}
+
+static irqreturn_t iqs5xx_irq(int irq, void *data)
+{
+ struct iqs5xx_private *iqs5xx = (struct iqs5xx_private *)data;
+ struct iqs5xx_touch_data *touch_data;
+ struct i2c_client *client = iqs5xx->client;
+ struct input_dev *input = iqs5xx->input;
+ int error, i;
+ u8 buf[sizeof(*touch_data) * IQS5XX_NUM_CONTACTS];
+
+ error = iqs5xx_read_burst(client, IQS5XX_ABS_X, buf, ARRAY_SIZE(buf));
+ if (error)
+ return IRQ_NONE;
+
+ for (i = 0, touch_data = (struct iqs5xx_touch_data *)buf;
+ i < IQS5XX_NUM_CONTACTS; i++, touch_data++) {
+ input_mt_slot(input, i);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER,
+ be16_to_cpu(touch_data->strength) > 0);
+
+ if (!be16_to_cpu(touch_data->strength))
+ continue;
+
+ input_report_abs(input, ABS_MT_POSITION_X,
+ be16_to_cpu(touch_data->abs_x));
+ input_report_abs(input, ABS_MT_POSITION_Y,
+ be16_to_cpu(touch_data->abs_y));
+ input_report_abs(input, ABS_MT_PRESSURE,
+ be16_to_cpu(touch_data->strength));
+ }
+
+ input_mt_sync_frame(input);
+ input_sync(input);
+
+ error = iqs5xx_write_byte(client, IQS5XX_END_COMM, 0);
+ if (error)
+ return IRQ_NONE;
+
+ return IRQ_HANDLED;
+}
+
+static int iqs5xx_set_state(struct i2c_client *client, u8 state)
+{
+ int error1, error2;
+
+ /*
+ * Addressing the device outside of a communication window prompts it
+ * to assert the RDY output, so disable the interrupt line to prevent
+ * the handler from servicing a false interrupt.
+ */
+ disable_irq(client->irq);
+
+ error1 = iqs5xx_write_byte(client, IQS5XX_SYS_CTRL1, state);
+ error2 = iqs5xx_write_byte(client, IQS5XX_END_COMM, 0);
+
+ usleep_range(200, 300);
+ enable_irq(client->irq);
+
+ if (error1)
+ return error1;
+
+ return error2;
+}
+
+static int iqs5xx_open(struct input_dev *input)
+{
+ struct iqs5xx_private *iqs5xx = input_get_drvdata(input);
+
+ return iqs5xx_set_state(iqs5xx->client, IQS5XX_RESUME);
+}
+
+static void iqs5xx_close(struct input_dev *input)
+{
+ struct iqs5xx_private *iqs5xx = input_get_drvdata(input);
+
+ iqs5xx_set_state(iqs5xx->client, IQS5XX_SUSPEND);
+}
+
+static int __maybe_unused iqs5xx_suspend(struct device *dev)
+{
+ struct iqs5xx_private *iqs5xx = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&iqs5xx->input->mutex);
+
+ if (iqs5xx->input->users)
+ error = iqs5xx_set_state(iqs5xx->client, IQS5XX_SUSPEND);
+
+ mutex_unlock(&iqs5xx->input->mutex);
+
+ return error;
+}
+
+static int __maybe_unused iqs5xx_resume(struct device *dev)
+{
+ struct iqs5xx_private *iqs5xx = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&iqs5xx->input->mutex);
+
+ if (iqs5xx->input->users)
+ error = iqs5xx_set_state(iqs5xx->client, IQS5XX_RESUME);
+
+ mutex_unlock(&iqs5xx->input->mutex);
+
+ return error;
+}
+
+static SIMPLE_DEV_PM_OPS(iqs5xx_pm, iqs5xx_suspend, iqs5xx_resume);
+
+static int iqs5xx_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct iqs5xx_private *iqs5xx;
+ struct input_dev *input;
+ int error;
+ __be16 exp_ver_spec = cpu_to_be16(0);
+
+ iqs5xx = devm_kzalloc(&client->dev, sizeof(*iqs5xx), GFP_KERNEL);
+ if (!iqs5xx)
+ return -ENOMEM;
+
+ dev_set_drvdata(&client->dev, iqs5xx);
+
+ i2c_set_clientdata(client, iqs5xx);
+ iqs5xx->client = client;
+
+ iqs5xx->reset_gpio = devm_gpiod_get(&client->dev,
+ "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(iqs5xx->reset_gpio)) {
+ error = PTR_ERR(iqs5xx->reset_gpio);
+ dev_err(&client->dev, "Failed to request GPIO: %d\n", error);
+ return error;
+ }
+
+ device_property_read_u8(&client->dev,
+ "azoteq,exp-ver-major", (u8 *)&exp_ver_spec);
+ device_property_read_u8(&client->dev,
+ "azoteq,exp-ver-minor", (u8 *)&exp_ver_spec + 1);
+
+ iqs5xx_reset(client);
+ usleep_range(10000, 10100);
+
+ error = iqs5xx_dev_id(client, exp_ver_spec);
+ switch (error) {
+ case 0:
+ break;
+ case -EAGAIN:
+ error = iqs5xx_update_firmware(client);
+ if (error)
+ return error;
+
+ error = iqs5xx_dev_id(client, exp_ver_spec);
+ if (error) {
+ dev_err(&client->dev,
+ "Aborting device initialization\n");
+ return -EINVAL;
+ }
+ break;
+ default:
+ return error;
+ }
+
+ input = devm_input_allocate_device(&client->dev);
+ if (!input)
+ return -ENOMEM;
+
+ input_set_drvdata(input, iqs5xx);
+ iqs5xx->input = input;
+
+ input->name = client->name;
+ input->id.bustype = BUS_I2C;
+ input->open = iqs5xx_open;
+ input->close = iqs5xx_close;
+
+ input_set_capability(input, EV_ABS, ABS_MT_POSITION_X);
+ input_set_capability(input, EV_ABS, ABS_MT_POSITION_Y);
+ input_set_capability(input, EV_ABS, ABS_MT_PRESSURE);
+
+ error = iqs5xx_dev_init(client);
+ if (error)
+ return error;
+
+ error = input_mt_init_slots(input,
+ IQS5XX_NUM_CONTACTS, INPUT_MT_DIRECT);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to initialize slots: %d\n", error);
+ return error;
+ }
+
+ error = devm_request_threaded_irq(&client->dev, client->irq,
+ NULL, iqs5xx_irq,
+ IRQF_ONESHOT | IRQF_TRIGGER_RISING,
+ client->name, iqs5xx);
+ if (error) {
+ dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
+ return error;
+ }
+
+ error = input_register_device(input);
+ if (error) {
+ dev_err(&client->dev, "Failed to register device: %d\n", error);
+ return error;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id iqs5xx_id[] = {
+ { "iqs550", 0 },
+ { "iqs572", 1 },
+ { "iqs525", 2 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, iqs5xx_id);
+
+static const struct of_device_id iqs5xx_of_match[] = {
+ { .compatible = "azoteq,iqs550" },
+ { .compatible = "azoteq,iqs572" },
+ { .compatible = "azoteq,iqs525" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, iqs5xx_of_match);
+
+static struct i2c_driver iqs5xx_i2c_driver = {
+ .driver = {
+ .name = "iqs5xx",
+ .of_match_table = iqs5xx_of_match,
+ .pm = &iqs5xx_pm,
+ },
+ .id_table = iqs5xx_id,
+ .probe = iqs5xx_probe,
+};
+module_i2c_driver(iqs5xx_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <jeff@labundy.com>");
+MODULE_DESCRIPTION("Azoteq IQS550/572/525 Trackpad/Touchscreen Controller");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525
2019-02-10 22:39 ` [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525 Jeff LaBundy
@ 2019-02-18 6:40 ` Dmitry Torokhov
2019-02-19 2:05 ` Jeff LaBundy
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Torokhov @ 2019-02-18 6:40 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: devicetree, linux-input, rydberg, robh+dt, mark.rutland
Hi Jeff,
On Sun, Feb 10, 2019 at 04:39:54PM -0600, Jeff LaBundy wrote:
> This patch adds support for the Azoteq IQS550/572/525 family of
> trackpad/touchscreen controllers.
>
> The driver has been tested with an IQS550EV02 evaluation board. A
> demonstration of the driver's capabilities is available here:
>
> https://youtu.be/sRNNx4XZBts
A few comments:
- we have request_ihex_firmware that requests and validates biary
representation of ihex into the kernel. There is not really a good
reason to parse text ihex in kernel.
There is idex2bin in ./firmware to convert it.
- I would recommend against doing this whole business of automatic
uprevving and updating firmware on probe. The controller mees to have
persistent firmware, so just check whether the device comes up in
bootloader or normal mode, and if it is in bootloader mode simply
forego creating and registering input device and wait for the
userspace to update firmware ad fix it up.
Not having to deal with firmware on probe will also allow compiling
the driver into the kernel (vs being a module) as firmware loading is
not normally available until after root filesystem is mounted.
- firmware update is usually keyed off device model, not DT name.
> +static int iqs5xx_read_burst(struct i2c_client *client,
> + u16 reg, u8 *val8, u8 len)
If you make val8 void * you won't need to cast.
> +
> +static int iqs5xx_write_burst(struct i2c_client *client,
> + u16 reg, u8 *val8, u8 len)
const void *.
> + error = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL, iqs5xx_irq,
> + IRQF_ONESHOT | IRQF_TRIGGER_RISING,
Please do not specify IRQ trigger, let it come from device tree, so only
use IRQF_ONESHOT.
By the way, I'd recommend using level interrupts so you never miss the
edge.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525
2019-02-18 6:40 ` Dmitry Torokhov
@ 2019-02-19 2:05 ` Jeff LaBundy
2019-02-19 8:13 ` Dmitry Torokhov
0 siblings, 1 reply; 5+ messages in thread
From: Jeff LaBundy @ 2019-02-19 2:05 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: devicetree, linux-input, rydberg, robh+dt, mark.rutland
Hi Dmitry,
Many thanks for your kind review. All of your comments sound great
to me; would just like to provide some background behind the first
one before spinning a v7 (comments below).
On Sun, Feb 17, 2019 at 10:40:54PM -0800, Dmitry Torokhov wrote:
> Hi Jeff,
>
> On Sun, Feb 10, 2019 at 04:39:54PM -0600, Jeff LaBundy wrote:
> > This patch adds support for the Azoteq IQS550/572/525 family of
> > trackpad/touchscreen controllers.
> >
> > The driver has been tested with an IQS550EV02 evaluation board. A
> > demonstration of the driver's capabilities is available here:
> >
> > https://youtu.be/sRNNx4XZBts
>
> A few comments:
>
> - we have request_ihex_firmware that requests and validates biary
> representation of ihex into the kernel. There is not really a good
> reason to parse text ihex in kernel.
> There is idex2bin in ./firmware to convert it.
Agreed on all counts; using request_ihex_firmware was my original
intent. However the vendor's ihex format is slightly nonstandard in
that the checksum field is not calculated for every record, and the
EOF record contains a nonzero address (0xFFFF), both of which cause
ihex2bin to produce an error.
This means the vendor's firmware would have to be passed through a
second (custom) fixup tool before being converted yet again with
ihex2bin. Therefore I (reluctantly) made the decision to handle the
vendor's semi-custom ihex format in the driver directly.
If that's a deal breaker, I'm happy to adopt request_ihex_firmware
and contact the vendor to suggest that their configuration tool is
updated to export true ihex files. However, there is no guarantee
as to when/if that would happen, and it seems I would need to offer
said fixup tool in the meantime (which I can certainly do).
Let me know if that background changes your mind, or if you would
still like to see this changed (either way works for me).
> - I would recommend against doing this whole business of automatic
> uprevving and updating firmware on probe. The controller mees to have
> persistent firmware, so just check whether the device comes up in
> bootloader or normal mode, and if it is in bootloader mode simply
> forego creating and registering input device and wait for the
> userspace to update firmware ad fix it up.
> Not having to deal with firmware on probe will also allow compiling
> the driver into the kernel (vs being a module) as firmware loading is
> not normally available until after root filesystem is mounted.
Agreed on all counts; I like that idea much better.
> - firmware update is usually keyed off device model, not DT name.
Agreed; I wasn't sold on my original method.
My new plan is to let user space specify the name of the firmware, as
the vendor's configuration tool appends some customer-specific revision
information to the exported file's name that customers may opt to keep.
> > +static int iqs5xx_read_burst(struct i2c_client *client,
> > + u16 reg, u8 *val8, u8 len)
>
> If you make val8 void * you won't need to cast.
Thanks for the tip; will do.
> > +
> > +static int iqs5xx_write_burst(struct i2c_client *client,
> > + u16 reg, u8 *val8, u8 len)
>
> const void *.
Thanks for the tip; will do.
> > + error = devm_request_threaded_irq(&client->dev, client->irq,
> > + NULL, iqs5xx_irq,
> > + IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>
> Please do not specify IRQ trigger, let it come from device tree, so only
> use IRQF_ONESHOT.
Agreed; will do.
> By the way, I'd recommend using level interrupts so you never miss the
> edge.
Sounds good; I will update the example in the bindings doc to reflect.
> Thanks.
>
> --
> Dmitry
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525
2019-02-19 2:05 ` Jeff LaBundy
@ 2019-02-19 8:13 ` Dmitry Torokhov
0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Torokhov @ 2019-02-19 8:13 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: devicetree, linux-input, rydberg, robh+dt, mark.rutland
On Mon, Feb 18, 2019 at 08:05:49PM -0600, Jeff LaBundy wrote:
> Hi Dmitry,
>
> Many thanks for your kind review. All of your comments sound great
> to me; would just like to provide some background behind the first
> one before spinning a v7 (comments below).
>
> On Sun, Feb 17, 2019 at 10:40:54PM -0800, Dmitry Torokhov wrote:
> > Hi Jeff,
> >
> > On Sun, Feb 10, 2019 at 04:39:54PM -0600, Jeff LaBundy wrote:
> > > This patch adds support for the Azoteq IQS550/572/525 family of
> > > trackpad/touchscreen controllers.
> > >
> > > The driver has been tested with an IQS550EV02 evaluation board. A
> > > demonstration of the driver's capabilities is available here:
> > >
> > > https://youtu.be/sRNNx4XZBts
> >
> > A few comments:
> >
> > - we have request_ihex_firmware that requests and validates biary
> > representation of ihex into the kernel. There is not really a good
> > reason to parse text ihex in kernel.
> > There is idex2bin in ./firmware to convert it.
>
> Agreed on all counts; using request_ihex_firmware was my original
> intent. However the vendor's ihex format is slightly nonstandard in
> that the checksum field is not calculated for every record, and the
> EOF record contains a nonzero address (0xFFFF), both of which cause
> ihex2bin to produce an error.
>
> This means the vendor's firmware would have to be passed through a
> second (custom) fixup tool before being converted yet again with
> ihex2bin. Therefore I (reluctantly) made the decision to handle the
> vendor's semi-custom ihex format in the driver directly.
>
> If that's a deal breaker, I'm happy to adopt request_ihex_firmware
> and contact the vendor to suggest that their configuration tool is
> updated to export true ihex files. However, there is no guarantee
> as to when/if that would happen, and it seems I would need to offer
> said fixup tool in the meantime (which I can certainly do).
>
> Let me know if that background changes your mind, or if you would
> still like to see this changed (either way works for me).
OK, I guess we can keep the parser in the driver then. Can you please
call out in the comments that standard parser is not suitable?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-19 8:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-10 22:39 [PATCH v6 1/2] dt-bindings: input: touchscreen: iqs5xx: Add bindings Jeff LaBundy
2019-02-10 22:39 ` [PATCH v6 2/2] input: touchscreen: Add support for Azoteq IQS550/572/525 Jeff LaBundy
2019-02-18 6:40 ` Dmitry Torokhov
2019-02-19 2:05 ` Jeff LaBundy
2019-02-19 8:13 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.