Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 14 Jan 2021 10:17:11 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     Jeff LaBundy <jeff@labundy.com>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] mfd: iqs62x: Do not poll during ATI
Message-ID: <20210114101711.GM3975472@dell>
References: <1609707369-11297-1-git-send-email-jeff@labundy.com>
 <1609707369-11297-6-git-send-email-jeff@labundy.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1609707369-11297-6-git-send-email-jeff@labundy.com>
Precedence: bulk

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