On Mon, Nov 28, 2022 at 21:03, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
> After initial start-up, 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 device from successfully probing until this happens.
>
> As an added benefit, initial switch states are now reported in the
> interrupt handler at the same time ATI status is checked. As such,
> duplicate code that reports initial switch states has been removed
> from iqs269_input_init().
>
> 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 the lower clock rate.
>
> Rather, a single timeout that covers both clock rates is used. The
> filter settling delay does not happen to be necessary and has been
> removed as well.
>
> Fixes: 04e49867fad1 ("Input: add support for Azoteq IQS269A")
> Signed-off-by: Jeff LaBundy <jeff@xxxxxxxxxxx>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@xxxxxxxxxxxx>
> ---
> drivers/input/misc/iqs269a.c | 97 +++++++++++++++++-------------------
> 1 file changed, 46 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index eca680bf8c20..4e7b46d30052 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -9,6 +9,7 @@
> * axial sliders presented by the device.
> */
>
> +#include <linux/completion.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -144,10 +145,6 @@
> #define IQS269_NUM_CH 8
> #define IQS269_NUM_SL 2
>
> -#define IQS269_ATI_POLL_SLEEP_US (iqs269->delay_mult * 10000)
> -#define IQS269_ATI_POLL_TIMEOUT_US (iqs269->delay_mult * 500000)
> -#define IQS269_ATI_STABLE_DELAY_MS (iqs269->delay_mult * 150)
> -
> #define iqs269_irq_wait() usleep_range(200, 250)
>
> enum iqs269_local_cap_size {
> @@ -289,10 +286,10 @@ struct iqs269_private {
> struct mutex lock;
> struct iqs269_switch_desc switches[ARRAY_SIZE(iqs269_events)];
> struct iqs269_sys_reg sys_reg;
> + struct completion ati_done;
> struct input_dev *keypad;
> struct input_dev *slider[IQS269_NUM_SL];
> unsigned int keycode[ARRAY_SIZE(iqs269_events) * IQS269_NUM_CH];
> - unsigned int delay_mult;
> unsigned int ch_num;
> bool hall_enable;
> bool ati_current;
> @@ -979,13 +976,8 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
>
> general = be16_to_cpu(sys_reg->general);
>
> - if (device_property_present(&client->dev, "azoteq,clk-div")) {
> + if (device_property_present(&client->dev, "azoteq,clk-div"))
> general |= IQS269_SYS_SETTINGS_CLK_DIV;
> - iqs269->delay_mult = 4;
> - } else {
> - general &= ~IQS269_SYS_SETTINGS_CLK_DIV;
> - iqs269->delay_mult = 1;
> - }
>
> /*
> * Configure the device to automatically switch between normal and low-
> @@ -1042,7 +1034,6 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
>
> static int iqs269_dev_init(struct iqs269_private *iqs269)
> {
> - unsigned int val;
> int error;
>
> mutex_lock(&iqs269->lock);
> @@ -1058,14 +1049,12 @@ static int iqs269_dev_init(struct iqs269_private *iqs269)
> if (error)
> goto err_mutex;
>
> - error = regmap_read_poll_timeout(iqs269->regmap, IQS269_SYS_FLAGS, val,
> - !(val & IQS269_SYS_FLAGS_IN_ATI),
> - IQS269_ATI_POLL_SLEEP_US,
> - IQS269_ATI_POLL_TIMEOUT_US);
> - if (error)
> - goto err_mutex;
> + /*
> + * The following delay gives the device time to deassert its RDY output
> + * so as to prevent an interrupt from being serviced prematurely.
> + */
> + usleep_range(2000, 2100);
>
> - msleep(IQS269_ATI_STABLE_DELAY_MS);
> iqs269->ati_current = true;
>
> err_mutex:
> @@ -1077,10 +1066,8 @@ static int iqs269_dev_init(struct iqs269_private *iqs269)
> static int iqs269_input_init(struct iqs269_private *iqs269)
> {
> struct i2c_client *client = iqs269->client;
> - struct iqs269_flags flags;
> unsigned int sw_code, keycode;
> int error, i, j;
> - u8 dir_mask, state;
>
> iqs269->keypad = devm_input_allocate_device(&client->dev);
> if (!iqs269->keypad)
> @@ -1093,23 +1080,7 @@ static int iqs269_input_init(struct iqs269_private *iqs269)
> iqs269->keypad->name = "iqs269a_keypad";
> iqs269->keypad->id.bustype = BUS_I2C;
>
> - if (iqs269->hall_enable) {
> - error = regmap_raw_read(iqs269->regmap, IQS269_SYS_FLAGS,
> - &flags, sizeof(flags));
> - if (error) {
> - dev_err(&client->dev,
> - "Failed to read initial status: %d\n", error);
> - return error;
> - }
> - }
> -
> for (i = 0; i < ARRAY_SIZE(iqs269_events); i++) {
> - dir_mask = flags.states[IQS269_ST_OFFS_DIR];
> - if (!iqs269_events[i].dir_up)
> - dir_mask = ~dir_mask;
> -
> - state = flags.states[iqs269_events[i].st_offs] & dir_mask;
> -
> sw_code = iqs269->switches[i].code;
>
> for (j = 0; j < IQS269_NUM_CH; j++) {
> @@ -1122,13 +1093,9 @@ static int iqs269_input_init(struct iqs269_private *iqs269)
> switch (j) {
> case IQS269_CHx_HALL_ACTIVE:
> if (iqs269->hall_enable &&
> - iqs269->switches[i].enabled) {
> + iqs269->switches[i].enabled)
> input_set_capability(iqs269->keypad,
> EV_SW, sw_code);
> - input_report_switch(iqs269->keypad,
> - sw_code,
> - state & BIT(j));
> - }
> fallthrough;
>
> case IQS269_CHx_HALL_INACTIVE:
> @@ -1144,14 +1111,6 @@ static int iqs269_input_init(struct iqs269_private *iqs269)
> }
> }
>
> - input_sync(iqs269->keypad);
> -
> - error = input_register_device(iqs269->keypad);
> - if (error) {
> - dev_err(&client->dev, "Failed to register keypad: %d\n", error);
> - return error;
> - }
> -
> for (i = 0; i < IQS269_NUM_SL; i++) {
> if (!iqs269->sys_reg.slider_select[i])
> continue;
> @@ -1211,6 +1170,9 @@ static int iqs269_report(struct iqs269_private *iqs269)
> return error;
> }
>
> + if (be16_to_cpu(flags.system) & IQS269_SYS_FLAGS_IN_ATI)
> + return 0;
> +
> error = regmap_raw_read(iqs269->regmap, IQS269_SLIDER_X, slider_x,
> sizeof(slider_x));
> if (error) {
> @@ -1273,6 +1235,12 @@ static int iqs269_report(struct iqs269_private *iqs269)
>
> input_sync(iqs269->keypad);
>
> + /*
> + * The following completion signals that ATI has finished, any initial
> + * switch states have been reported and the keypad can be registered.
> + */
> + complete_all(&iqs269->ati_done);
> +
> return 0;
> }
>
> @@ -1304,6 +1272,9 @@ static ssize_t counts_show(struct device *dev,
> if (!iqs269->ati_current || iqs269->hall_enable)
> return -EPERM;
>
> + if (!completion_done(&iqs269->ati_done))
> + return -EBUSY;
> +
> /*
> * Unsolicited I2C communication prompts the device to assert its RDY
> * pin, so disable the interrupt line until the operation is finished
> @@ -1560,7 +1531,9 @@ static ssize_t ati_trigger_show(struct device *dev,
> {
> struct iqs269_private *iqs269 = dev_get_drvdata(dev);
>
> - return scnprintf(buf, PAGE_SIZE, "%u\n", iqs269->ati_current);
> + return scnprintf(buf, PAGE_SIZE, "%u\n",
> + iqs269->ati_current &&
> + completion_done(&iqs269->ati_done));
> }
>
> static ssize_t ati_trigger_store(struct device *dev,
> @@ -1580,6 +1553,7 @@ static ssize_t ati_trigger_store(struct device *dev,
> return count;
>
> disable_irq(client->irq);
> + reinit_completion(&iqs269->ati_done);
>
> error = iqs269_dev_init(iqs269);
>
> @@ -1589,6 +1563,10 @@ static ssize_t ati_trigger_store(struct device *dev,
> if (error)
> return error;
>
> + if (!wait_for_completion_timeout(&iqs269->ati_done,
> + msecs_to_jiffies(2000)))
> + return -ETIMEDOUT;
> +
> return count;
> }
>
> @@ -1647,6 +1625,7 @@ static int iqs269_probe(struct i2c_client *client)
> }
>
> mutex_init(&iqs269->lock);
> + init_completion(&iqs269->ati_done);
>
> error = regmap_raw_read(iqs269->regmap, IQS269_VER_INFO, &ver_info,
> sizeof(ver_info));
> @@ -1682,6 +1661,22 @@ static int iqs269_probe(struct i2c_client *client)
> return error;
> }
>
> + if (!wait_for_completion_timeout(&iqs269->ati_done,
> + msecs_to_jiffies(2000))) {
> + dev_err(&client->dev, "Failed to complete ATI\n");
> + return -ETIMEDOUT;
> + }
> +
> + /*
> + * The keypad may include one or more switches and is not registered
> + * until ATI is complete and the initial switch states are read.
> + */
> + error = input_register_device(iqs269->keypad);
> + if (error) {
> + dev_err(&client->dev, "Failed to register keypad: %d\n", error);
> + return error;
> + }
> +
> error = devm_device_add_group(&client->dev, &iqs269_attr_group);
> if (error)
> dev_err(&client->dev, "Failed to add attributes: %d\n", error);
> --
> 2.34.1
