[PATCH 0/6] mfd: iqs62x: Minor cosmetic and functional improvements

[PATCH 0/6] mfd: iqs62x: Minor cosmetic and functional improvements

[Jeff LaBundy]

This series includes a variety of minor cosmetic and functional
improvements to the core support for the Azoteq IQS620A, IQS621,
IQS622, IQS624 and IQS625 multi-function sensors.

The first three patches are purely cosmetic, while the remaining
three incorporate learnings during recent work with other Azoteq
devices that have similar functions.

Jeff LaBundy (6):
  mfd: iqs62x: Remove superfluous whitespace above fallthroughs
  mfd: iqs62x: Remove unused bit mask
  mfd: iqs62x: Rename regmap_config struct
  mfd: iqs62x: Increase interrupt handler return delay
  mfd: iqs62x: Do not poll during ATI
  mfd: iqs62x: Do not change clock frequency during ATI

 drivers/mfd/iqs62x.c       | 122 ++++++++++++++++++++++++++++++---------------
 include/linux/mfd/iqs62x.h |   6 ++-
 2 files changed, 85 insertions(+), 43 deletions(-)

--
2.7.4

[PATCH 1/6] mfd: iqs62x: Remove superfluous whitespace above fallthroughs

[Jeff LaBundy]

Previously, all instances of the /* fall through */ comment were
preceded by a newline to improve readability.

Now that /* fall through */ comments have been replaced with the
fallthrough pseudo-keyword, the leftover whitespace looks out of
place and can simply be removed.

Fixes: df561f6688fe ("treewide: Use fallthrough pseudo-keyword")
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/mfd/iqs62x.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 761b4ef..ec4c790 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -135,7 +135,6 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
 
 		if (val & IQS620_PROX_SETTINGS_4_SAR_EN)
 			iqs62x->ui_sel = IQS62X_UI_SAR1;
-
 		fallthrough;
 
 	case IQS621_PROD_NUM:
@@ -469,7 +468,6 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
 		switch (event_reg) {
 		case IQS62X_EVENT_UI_LO:
 			event_data.ui_data = get_unaligned_le16(&event_map[i]);
-
 			fallthrough;
 
 		case IQS62X_EVENT_UI_HI:
@@ -490,7 +488,6 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
 
 		case IQS62X_EVENT_HYST:
 			event_map[i] <<= iqs62x->dev_desc->hyst_shift;
-
 			fallthrough;
 
 		case IQS62X_EVENT_WHEEL:
-- 
2.7.4

Re: [PATCH 1/6] mfd: iqs62x: Remove superfluous whitespace above fallthroughs

[Lee Jones]

On Sun, 03 Jan 2021, Jeff LaBundy wrote:

> Previously, all instances of the /* fall through */ comment were
> preceded by a newline to improve readability.
> 
> Now that /* fall through */ comments have been replaced with the
> fallthrough pseudo-keyword, the leftover whitespace looks out of
> place and can simply be removed.
> 
> Fixes: df561f6688fe ("treewide: Use fallthrough pseudo-keyword")
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
>  drivers/mfd/iqs62x.c | 3 ---
>  1 file changed, 3 deletions(-)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/6] mfd: iqs62x: Remove unused bit mask

[Jeff LaBundy]

The register write that performed a mandatory soft reset during
probe was removed during development of the driver, however the
IQS62X_SYS_SETTINGS_SOFT_RESET bit mask was left behind. Remove
it to keep stray macros out of the driver.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/mfd/iqs62x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index ec4c790..ff968dc 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -57,7 +57,6 @@
 #define IQS620_TEMP_CAL_OFFS			0xC4
 
 #define IQS62X_SYS_SETTINGS			0xD0
-#define IQS62X_SYS_SETTINGS_SOFT_RESET		BIT(7)
 #define IQS62X_SYS_SETTINGS_ACK_RESET		BIT(6)
 #define IQS62X_SYS_SETTINGS_EVENT_MODE		BIT(5)
 #define IQS62X_SYS_SETTINGS_CLK_DIV		BIT(4)
-- 
2.7.4

Re: [PATCH 2/6] mfd: iqs62x: Remove unused bit mask

[Lee Jones]

On Sun, 03 Jan 2021, Jeff LaBundy wrote:

> The register write that performed a mandatory soft reset during
> probe was removed during development of the driver, however the
> IQS62X_SYS_SETTINGS_SOFT_RESET bit mask was left behind. Remove
> it to keep stray macros out of the driver.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
>  drivers/mfd/iqs62x.c | 1 -
>  1 file changed, 1 deletion(-)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/6] mfd: iqs62x: Rename regmap_config struct

[Jeff LaBundy]

The regmap member of the driver's private data is called 'regmap',
but the regmap_config struct is called 'iqs62x_map_config'. Rename
the latter to 'iqs62x_regmap_config' for consistency.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/mfd/iqs62x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index ff968dc..7a1ff7c 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -866,7 +866,7 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 	},
 };
 
-static const struct regmap_config iqs62x_map_config = {
+static const struct regmap_config iqs62x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.max_register = IQS62X_MAX_REG,
@@ -892,7 +892,7 @@ static int iqs62x_probe(struct i2c_client *client)
 	INIT_LIST_HEAD(&iqs62x->fw_blk_head);
 	init_completion(&iqs62x->fw_done);
 
-	iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_map_config);
+	iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_regmap_config);
 	if (IS_ERR(iqs62x->regmap)) {
 		ret = PTR_ERR(iqs62x->regmap);
 		dev_err(&client->dev, "Failed to initialize register map: %d\n",
-- 
2.7.4

Re: [PATCH 3/6] mfd: iqs62x: Rename regmap_config struct

[Lee Jones]

On Sun, 03 Jan 2021, Jeff LaBundy wrote:

> The regmap member of the driver's private data is called 'regmap',
> but the regmap_config struct is called 'iqs62x_map_config'. Rename
> the latter to 'iqs62x_regmap_config' for consistency.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
>  drivers/mfd/iqs62x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/6] mfd: iqs62x: Increase interrupt handler return delay

[Jeff LaBundy]

The time the device takes to deassert its RDY output following an
I2C stop condition scales with the core clock frequency.

To prevent level-triggered interrupts from being reasserted after
the interrupt handler returns, increase the time before returning
to account for the worst-case delay (~90 us) plus margin.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/mfd/iqs62x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 7a1ff7c..07c9725 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -533,7 +533,7 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
 	 * ensure the device's RDY output has been deasserted by the time the
 	 * interrupt handler returns.
 	 */
-	usleep_range(50, 100);
+	usleep_range(150, 200);
 
 	return IRQ_HANDLED;
 }
-- 
2.7.4

Re: [PATCH 4/6] mfd: iqs62x: Increase interrupt handler return delay

[Lee Jones]

On Sun, 03 Jan 2021, Jeff LaBundy wrote:

> The time the device takes to deassert its RDY output following an
> I2C stop condition scales with the core clock frequency.
> 
> To prevent level-triggered interrupts from being reasserted after
> the interrupt handler returns, increase the time before returning
> to account for the worst-case delay (~90 us) plus margin.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
>  drivers/mfd/iqs62x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/6] mfd: iqs62x: Do not poll during ATI

[Jeff LaBundy]

After loading firmware, the driver triggers ATI (calibration) with
the newly loaded register configuration in place. Next, the driver
polls a register field to ensure ATI completed in a timely fashion
and that the device is ready to sense.

However, communicating with the device over I2C while ATI is under-
way may induce noise in the device and cause ATI to fail. As such,
the vendor recommends not to poll the device during ATI.

To solve this problem, let the device naturally signal to the host
that ATI is complete by way of an interrupt. A completion prevents
the sub-devices from being registered until this happens.

The former logic that scaled ATI timeout and filter settling delay
is not carried forward with the new implementation, as it produces
overly conservative delays at lower clock rates. Instead, a single
pair of delays that covers all cases is used.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/mfd/iqs62x.c       | 103 ++++++++++++++++++++++++++++++---------------
 include/linux/mfd/iqs62x.h |   6 ++-
 2 files changed, 73 insertions(+), 36 deletions(-)

diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 07c9725..9fdf32f 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -36,7 +36,6 @@
 #define IQS62X_PROD_NUM				0x00
 
 #define IQS62X_SYS_FLAGS			0x10
-#define IQS62X_SYS_FLAGS_IN_ATI			BIT(2)
 
 #define IQS620_HALL_FLAGS			0x16
 #define IQS621_HALL_FLAGS			0x19
@@ -60,6 +59,7 @@
 #define IQS62X_SYS_SETTINGS_ACK_RESET		BIT(6)
 #define IQS62X_SYS_SETTINGS_EVENT_MODE		BIT(5)
 #define IQS62X_SYS_SETTINGS_CLK_DIV		BIT(4)
+#define IQS62X_SYS_SETTINGS_COMM_ATI		BIT(3)
 #define IQS62X_SYS_SETTINGS_REDO_ATI		BIT(1)
 
 #define IQS62X_PWR_SETTINGS			0xD2
@@ -81,9 +81,7 @@
 #define IQS62X_FW_REC_TYPE_MASK			3
 #define IQS62X_FW_REC_TYPE_DATA			4
 
-#define IQS62X_ATI_POLL_SLEEP_US		10000
-#define IQS62X_ATI_POLL_TIMEOUT_US		500000
-#define IQS62X_ATI_STABLE_DELAY_MS		150
+#define IQS62X_FILT_SETTLE_MS			250
 
 struct iqs62x_fw_rec {
 	u8 type;
@@ -111,7 +109,6 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
 	struct iqs62x_fw_blk *fw_blk;
 	unsigned int val;
 	int ret;
-	u8 clk_div = 1;
 
 	list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) {
 		if (fw_blk->mask)
@@ -181,28 +178,32 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
 			return ret;
 	}
 
-	ret = regmap_read(iqs62x->regmap, IQS62X_SYS_SETTINGS, &val);
-	if (ret)
-		return ret;
-
-	if (val & IQS62X_SYS_SETTINGS_CLK_DIV)
-		clk_div = iqs62x->dev_desc->clk_div;
-
-	ret = regmap_write(iqs62x->regmap, IQS62X_SYS_SETTINGS, val |
-			   IQS62X_SYS_SETTINGS_ACK_RESET |
-			   IQS62X_SYS_SETTINGS_EVENT_MODE |
-			   IQS62X_SYS_SETTINGS_REDO_ATI);
-	if (ret)
-		return ret;
-
-	ret = regmap_read_poll_timeout(iqs62x->regmap, IQS62X_SYS_FLAGS, val,
-				       !(val & IQS62X_SYS_FLAGS_IN_ATI),
-				       IQS62X_ATI_POLL_SLEEP_US,
-				       IQS62X_ATI_POLL_TIMEOUT_US * clk_div);
+	/*
+	 * Place the device in streaming mode at first so as not to miss the
+	 * limited number of interrupts that would otherwise occur after ATI
+	 * completes. The device is subsequently placed in event mode by the
+	 * interrupt handler.
+	 *
+	 * In the meantime, mask interrupts during ATI to prevent the device
+	 * from soliciting I2C traffic until the noise-sensitive ATI process
+	 * is complete.
+	 */
+	ret = regmap_update_bits(iqs62x->regmap, IQS62X_SYS_SETTINGS,
+				 IQS62X_SYS_SETTINGS_ACK_RESET |
+				 IQS62X_SYS_SETTINGS_EVENT_MODE |
+				 IQS62X_SYS_SETTINGS_COMM_ATI |
+				 IQS62X_SYS_SETTINGS_REDO_ATI,
+				 IQS62X_SYS_SETTINGS_ACK_RESET |
+				 IQS62X_SYS_SETTINGS_REDO_ATI);
 	if (ret)
 		return ret;
 
-	msleep(IQS62X_ATI_STABLE_DELAY_MS * clk_div);
+	/*
+	 * The following delay gives the device time to deassert its RDY output
+	 * in case a communication window was open while the REDO_ATI field was
+	 * written. This prevents an interrupt from being serviced prematurely.
+	 */
+	usleep_range(5000, 5100);
 
 	return 0;
 }
@@ -433,6 +434,11 @@ const struct iqs62x_event_desc iqs62x_events[IQS62X_NUM_EVENTS] = {
 		.mask	= BIT(7),
 		.val	= BIT(7),
 	},
+	[IQS62X_EVENT_SYS_ATI] = {
+		.reg	= IQS62X_EVENT_SYS,
+		.mask	= BIT(2),
+		.val	= BIT(2),
+	},
 };
 EXPORT_SYMBOL_GPL(iqs62x_events);
 
@@ -521,12 +527,39 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
 				"Failed to re-initialize device: %d\n", ret);
 			return IRQ_NONE;
 		}
+
+		iqs62x->event_cache |= BIT(IQS62X_EVENT_SYS_RESET);
+		reinit_completion(&iqs62x->ati_done);
+	} else if (event_flags & BIT(IQS62X_EVENT_SYS_ATI)) {
+		iqs62x->event_cache |= BIT(IQS62X_EVENT_SYS_ATI);
+		reinit_completion(&iqs62x->ati_done);
+	} else if (!completion_done(&iqs62x->ati_done)) {
+		ret = regmap_update_bits(iqs62x->regmap, IQS62X_SYS_SETTINGS,
+					 IQS62X_SYS_SETTINGS_EVENT_MODE, 0xFF);
+		if (ret) {
+			dev_err(&client->dev,
+				"Failed to enable event mode: %d\n", ret);
+			return IRQ_NONE;
+		}
+
+		msleep(IQS62X_FILT_SETTLE_MS);
+		complete_all(&iqs62x->ati_done);
 	}
 
-	ret = blocking_notifier_call_chain(&iqs62x->nh, event_flags,
-					   &event_data);
-	if (ret & NOTIFY_STOP_MASK)
-		return IRQ_NONE;
+	/*
+	 * Reset and ATI events are not broadcast to the sub-device drivers
+	 * until ATI has completed. Any other events that may have occurred
+	 * during ATI are ignored.
+	 */
+	if (completion_done(&iqs62x->ati_done)) {
+		event_flags |= iqs62x->event_cache;
+		ret = blocking_notifier_call_chain(&iqs62x->nh, event_flags,
+						   &event_data);
+		if (ret & NOTIFY_STOP_MASK)
+			return IRQ_NONE;
+
+		iqs62x->event_cache = 0;
+	}
 
 	/*
 	 * Once the communication window is closed, a small delay is added to
@@ -567,6 +600,12 @@ static void iqs62x_firmware_load(const struct firmware *fw, void *context)
 		goto err_out;
 	}
 
+	if (!wait_for_completion_timeout(&iqs62x->ati_done,
+					 msecs_to_jiffies(2000))) {
+		dev_err(&client->dev, "Failed to complete ATI\n");
+		goto err_out;
+	}
+
 	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 				   iqs62x->dev_desc->sub_devs,
 				   iqs62x->dev_desc->num_sub_devs,
@@ -763,7 +802,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 		.prox_settings	= IQS620_PROX_SETTINGS_4,
 		.hall_flags	= IQS620_HALL_FLAGS,
 
-		.clk_div	= 4,
 		.fw_name	= "iqs620a.bin",
 		.event_regs	= &iqs620a_event_regs[IQS62X_UI_PROX],
 	},
@@ -784,7 +822,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 		.prox_settings	= IQS620_PROX_SETTINGS_4,
 		.hall_flags	= IQS620_HALL_FLAGS,
 
-		.clk_div	= 4,
 		.fw_name	= "iqs620a.bin",
 		.event_regs	= &iqs620a_event_regs[IQS62X_UI_PROX],
 	},
@@ -808,7 +845,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 		.hall_flags	= IQS621_HALL_FLAGS,
 		.hyst_shift	= 5,
 
-		.clk_div	= 2,
 		.fw_name	= "iqs621.bin",
 		.event_regs	= &iqs621_event_regs[IQS62X_UI_PROX],
 	},
@@ -830,7 +866,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 		.als_flags	= IQS622_ALS_FLAGS,
 		.hall_flags	= IQS622_HALL_FLAGS,
 
-		.clk_div	= 2,
 		.fw_name	= "iqs622.bin",
 		.event_regs	= &iqs622_event_regs[IQS62X_UI_PROX],
 	},
@@ -845,7 +880,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 		.interval	= IQS624_INTERVAL_NUM,
 		.interval_div	= 3,
 
-		.clk_div	= 2,
 		.fw_name	= "iqs624.bin",
 		.event_regs	= &iqs624_event_regs[IQS62X_UI_PROX],
 	},
@@ -860,7 +894,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
 		.interval	= IQS625_INTERVAL_NUM,
 		.interval_div	= 10,
 
-		.clk_div	= 2,
 		.fw_name	= "iqs625.bin",
 		.event_regs	= &iqs625_event_regs[IQS62X_UI_PROX],
 	},
@@ -890,6 +923,8 @@ static int iqs62x_probe(struct i2c_client *client)
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&iqs62x->nh);
 	INIT_LIST_HEAD(&iqs62x->fw_blk_head);
+
+	init_completion(&iqs62x->ati_done);
 	init_completion(&iqs62x->fw_done);
 
 	iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_regmap_config);
diff --git a/include/linux/mfd/iqs62x.h b/include/linux/mfd/iqs62x.h
index 043d3b6..0b71173 100644
--- a/include/linux/mfd/iqs62x.h
+++ b/include/linux/mfd/iqs62x.h
@@ -28,7 +28,7 @@
 #define IQS620_GLBL_EVENT_MASK_PMU		BIT(6)
 
 #define IQS62X_NUM_KEYS				16
-#define IQS62X_NUM_EVENTS			(IQS62X_NUM_KEYS + 5)
+#define IQS62X_NUM_EVENTS			(IQS62X_NUM_KEYS + 6)
 
 #define IQS62X_EVENT_SIZE			10
 
@@ -78,6 +78,7 @@ enum iqs62x_event_flag {
 
 	/* everything else */
 	IQS62X_EVENT_SYS_RESET,
+	IQS62X_EVENT_SYS_ATI,
 };
 
 struct iqs62x_event_data {
@@ -119,7 +120,6 @@ struct iqs62x_dev_desc {
 	u8 interval;
 	u8 interval_div;
 
-	u8 clk_div;
 	const char *fw_name;
 	const enum iqs62x_event_reg (*event_regs)[IQS62X_EVENT_SIZE];
 };
@@ -130,8 +130,10 @@ struct iqs62x_core {
 	struct regmap *regmap;
 	struct blocking_notifier_head nh;
 	struct list_head fw_blk_head;
+	struct completion ati_done;
 	struct completion fw_done;
 	enum iqs62x_ui_sel ui_sel;
+	unsigned long event_cache;
 };
 
 extern const struct iqs62x_event_desc iqs62x_events[IQS62X_NUM_EVENTS];
-- 
2.7.4

Re: [PATCH 5/6] mfd: iqs62x: Do not poll during ATI

[Lee Jones]

On Sun, 03 Jan 2021, Jeff LaBundy wrote:

> After loading firmware, the driver triggers ATI (calibration) with
> the newly loaded register configuration in place. Next, the driver
> polls a register field to ensure ATI completed in a timely fashion
> and that the device is ready to sense.
> 
> However, communicating with the device over I2C while ATI is under-

This doesn't line-up with all of the others! ;)

> way may induce noise in the device and cause ATI to fail. As such,
> the vendor recommends not to poll the device during ATI.
> 
> To solve this problem, let the device naturally signal to the host
> that ATI is complete by way of an interrupt. A completion prevents
> the sub-devices from being registered until this happens.
> 
> The former logic that scaled ATI timeout and filter settling delay
> is not carried forward with the new implementation, as it produces
> overly conservative delays at lower clock rates. Instead, a single
> pair of delays that covers all cases is used.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
>  drivers/mfd/iqs62x.c       | 103 ++++++++++++++++++++++++++++++---------------
>  include/linux/mfd/iqs62x.h |   6 ++-
>  2 files changed, 73 insertions(+), 36 deletions(-)

[...]

> @@ -567,6 +600,12 @@ static void iqs62x_firmware_load(const struct firmware *fw, void *context)
>  		goto err_out;
>  	}
>  
> +	if (!wait_for_completion_timeout(&iqs62x->ati_done,
> +					 msecs_to_jiffies(2000))) {
> +		dev_err(&client->dev, "Failed to complete ATI\n");
> +		goto err_out;
> +	}
> +
>  	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
>  				   iqs62x->dev_desc->sub_devs,
>  				   iqs62x->dev_desc->num_sub_devs,
> @@ -763,7 +802,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
>  		.prox_settings	= IQS620_PROX_SETTINGS_4,
>  		.hall_flags	= IQS620_HALL_FLAGS,
>  
> -		.clk_div	= 4,

No unnecessary double-line spacings please.

>  		.fw_name	= "iqs620a.bin",
>  		.event_regs	= &iqs620a_event_regs[IQS62X_UI_PROX],
>  	},
> @@ -784,7 +822,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
>  		.prox_settings	= IQS620_PROX_SETTINGS_4,
>  		.hall_flags	= IQS620_HALL_FLAGS,
>  
> -		.clk_div	= 4,

As above.

>  		.fw_name	= "iqs620a.bin",
>  		.event_regs	= &iqs620a_event_regs[IQS62X_UI_PROX],
>  	},
> @@ -808,7 +845,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
>  		.hall_flags	= IQS621_HALL_FLAGS,
>  		.hyst_shift	= 5,
>  
> -		.clk_div	= 2,

As above.

>  		.fw_name	= "iqs621.bin",
>  		.event_regs	= &iqs621_event_regs[IQS62X_UI_PROX],
>  	},
> @@ -830,7 +866,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
>  		.als_flags	= IQS622_ALS_FLAGS,
>  		.hall_flags	= IQS622_HALL_FLAGS,
>  
> -		.clk_div	= 2,

As above.

>  		.fw_name	= "iqs622.bin",
>  		.event_regs	= &iqs622_event_regs[IQS62X_UI_PROX],
>  	},
> @@ -845,7 +880,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
>  		.interval	= IQS624_INTERVAL_NUM,
>  		.interval_div	= 3,
>  
> -		.clk_div	= 2,

As above.

>  		.fw_name	= "iqs624.bin",
>  		.event_regs	= &iqs624_event_regs[IQS62X_UI_PROX],
>  	},
> @@ -860,7 +894,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
>  		.interval	= IQS625_INTERVAL_NUM,
>  		.interval_div	= 10,
>  
> -		.clk_div	= 2,

As above.

>  		.fw_name	= "iqs625.bin",
>  		.event_regs	= &iqs625_event_regs[IQS62X_UI_PROX],
>  	},
> @@ -890,6 +923,8 @@ static int iqs62x_probe(struct i2c_client *client)
>  
>  	BLOCKING_INIT_NOTIFIER_HEAD(&iqs62x->nh);
>  	INIT_LIST_HEAD(&iqs62x->fw_blk_head);
> +
> +	init_completion(&iqs62x->ati_done);
>  	init_completion(&iqs62x->fw_done);
>  
>  	iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_regmap_config);
> diff --git a/include/linux/mfd/iqs62x.h b/include/linux/mfd/iqs62x.h
> index 043d3b6..0b71173 100644
> --- a/include/linux/mfd/iqs62x.h
> +++ b/include/linux/mfd/iqs62x.h
> @@ -28,7 +28,7 @@
>  #define IQS620_GLBL_EVENT_MASK_PMU		BIT(6)
>  
>  #define IQS62X_NUM_KEYS				16
> -#define IQS62X_NUM_EVENTS			(IQS62X_NUM_KEYS + 5)
> +#define IQS62X_NUM_EVENTS			(IQS62X_NUM_KEYS + 6)
>  
>  #define IQS62X_EVENT_SIZE			10
>  
> @@ -78,6 +78,7 @@ enum iqs62x_event_flag {
>  
>  	/* everything else */
>  	IQS62X_EVENT_SYS_RESET,
> +	IQS62X_EVENT_SYS_ATI,
>  };
>  
>  struct iqs62x_event_data {
> @@ -119,7 +120,6 @@ struct iqs62x_dev_desc {
>  	u8 interval;
>  	u8 interval_div;
>  
> -	u8 clk_div;

As above.

>  	const char *fw_name;
>  	const enum iqs62x_event_reg (*event_regs)[IQS62X_EVENT_SIZE];
>  };
> @@ -130,8 +130,10 @@ struct iqs62x_core {
>  	struct regmap *regmap;
>  	struct blocking_notifier_head nh;
>  	struct list_head fw_blk_head;
> +	struct completion ati_done;
>  	struct completion fw_done;
>  	enum iqs62x_ui_sel ui_sel;
> +	unsigned long event_cache;
>  };
>  
>  extern const struct iqs62x_event_desc iqs62x_events[IQS62X_NUM_EVENTS];

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 5/6] mfd: iqs62x: Do not poll during ATI

[Jeff LaBundy]

Hi Lee,

Thank you for taking a look at the series.

On Thu, Jan 14, 2021 at 10:17:11AM +0000, Lee Jones wrote:
> On Sun, 03 Jan 2021, Jeff LaBundy wrote:
> 
> > After loading firmware, the driver triggers ATI (calibration) with
> > the newly loaded register configuration in place. Next, the driver
> > polls a register field to ensure ATI completed in a timely fashion
> > and that the device is ready to sense.
> > 
> > However, communicating with the device over I2C while ATI is under-
> 
> This doesn't line-up with all of the others! ;)

:)

> 
> > way may induce noise in the device and cause ATI to fail. As such,
> > the vendor recommends not to poll the device during ATI.
> > 
> > To solve this problem, let the device naturally signal to the host
> > that ATI is complete by way of an interrupt. A completion prevents
> > the sub-devices from being registered until this happens.
> > 
> > The former logic that scaled ATI timeout and filter settling delay
> > is not carried forward with the new implementation, as it produces
> > overly conservative delays at lower clock rates. Instead, a single
> > pair of delays that covers all cases is used.
> > 
> > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > ---
> >  drivers/mfd/iqs62x.c       | 103 ++++++++++++++++++++++++++++++---------------
> >  include/linux/mfd/iqs62x.h |   6 ++-
> >  2 files changed, 73 insertions(+), 36 deletions(-)
> 
> [...]
> 
> > @@ -567,6 +600,12 @@ static void iqs62x_firmware_load(const struct firmware *fw, void *context)
> >  		goto err_out;
> >  	}
> >  
> > +	if (!wait_for_completion_timeout(&iqs62x->ati_done,
> > +					 msecs_to_jiffies(2000))) {
> > +		dev_err(&client->dev, "Failed to complete ATI\n");
> > +		goto err_out;
> > +	}
> > +
> >  	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> >  				   iqs62x->dev_desc->sub_devs,
> >  				   iqs62x->dev_desc->num_sub_devs,
> > @@ -763,7 +802,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
> >  		.prox_settings	= IQS620_PROX_SETTINGS_4,
> >  		.hall_flags	= IQS620_HALL_FLAGS,
> >  
> > -		.clk_div	= 4,
> 
> No unnecessary double-line spacings please.

Not a problem, v2 on the way. There are a couple nearby instances of
double-spacing within these same structs so I'll nuke those as well.

[...]

Kind regards,
Jeff LaBundy

[PATCH 6/6] mfd: iqs62x: Do not change clock frequency during ATI

[Jeff LaBundy]

After a reset event, the device automatically triggers ATI at the
default core clock frequency (16 MHz). Soon afterward, the driver
loads firmware which may attempt to lower the frequency.

If this initial ATI cycle is still in progress when the frequency
is changed, however, the device incorrectly reports channels 0, 1
and 2 to be in a state of touch once ATI finally completes.

To solve this problem, wait until ATI is complete before lowering
the frequency. Because this particular ATI cycle occurs following
a reset event, its duration is predictable and a simple delay can
suffice.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
 drivers/mfd/iqs62x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 9fdf32f..3b32542 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -81,6 +81,7 @@
 #define IQS62X_FW_REC_TYPE_MASK			3
 #define IQS62X_FW_REC_TYPE_DATA			4
 
+#define IQS62X_ATI_STARTUP_MS			350
 #define IQS62X_FILT_SETTLE_MS			250
 
 struct iqs62x_fw_rec {
@@ -111,6 +112,14 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
 	int ret;
 
 	list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) {
+		/*
+		 * In case ATI is in progress, wait for it to complete before
+		 * lowering the core clock frequency.
+		 */
+		if (fw_blk->addr == IQS62X_SYS_SETTINGS &&
+		    *fw_blk->data & IQS62X_SYS_SETTINGS_CLK_DIV)
+			msleep(IQS62X_ATI_STARTUP_MS);
+
 		if (fw_blk->mask)
 			ret = regmap_update_bits(iqs62x->regmap, fw_blk->addr,
 						 fw_blk->mask, *fw_blk->data);
-- 
2.7.4

Re: [PATCH 6/6] mfd: iqs62x: Do not change clock frequency during ATI

[Lee Jones]

On Sun, 03 Jan 2021, Jeff LaBundy wrote:

> After a reset event, the device automatically triggers ATI at the
> default core clock frequency (16 MHz). Soon afterward, the driver
> loads firmware which may attempt to lower the frequency.
> 
> If this initial ATI cycle is still in progress when the frequency
> is changed, however, the device incorrectly reports channels 0, 1
> and 2 to be in a state of touch once ATI finally completes.
> 
> To solve this problem, wait until ATI is complete before lowering
> the frequency. Because this particular ATI cycle occurs following
> a reset event, its duration is predictable and a simple delay can
> suffice.
> 
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> ---
>  drivers/mfd/iqs62x.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog