On Mon, Nov 28, 2022 at 21:02, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
> Unless it is being done as part of servicing a soft reset interrupt,
> configuring channels on-the-fly (as is the case when writing to the
> ati_trigger attribute) may cause GPIO3 (which reflects the state of
> touch for a selected channel) to be inadvertently asserted.
>
> To solve this problem, follow the vendor's recommendation and write
> all channel configuration as well as the REDO_ATI register field as
> part of a single block write. This ensures the device has been told
> to re-calibrate itself following an I2C stop condition, after which
> sensing resumes and GPIO3 may be asserted.
>
> 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 | 98 ++++++++++++++----------------------
> 1 file changed, 39 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
> index 711e67db71a4..0eb3cff177e5 100644
> --- a/drivers/input/misc/iqs269a.c
> +++ b/drivers/input/misc/iqs269a.c
> @@ -96,8 +96,6 @@
> #define IQS269_MISC_B_TRACKING_UI_ENABLE BIT(4)
> #define IQS269_MISC_B_FILT_STR_SLIDER GENMASK(1, 0)
>
> -#define IQS269_CHx_SETTINGS 0x8C
> -
> #define IQS269_CHx_ENG_A_MEAS_CAP_SIZE BIT(15)
> #define IQS269_CHx_ENG_A_RX_GND_INACTIVE BIT(13)
> #define IQS269_CHx_ENG_A_LOCAL_CAP_SIZE BIT(12)
> @@ -245,6 +243,18 @@ struct iqs269_ver_info {
> u8 padding;
> } __packed;
>
> +struct iqs269_ch_reg {
> + u8 rx_enable;
> + u8 tx_enable;
> + __be16 engine_a;
> + __be16 engine_b;
> + __be16 ati_comp;
> + u8 thresh[3];
> + u8 hyst;
> + u8 assoc_select;
> + u8 assoc_weight;
> +} __packed;
> +
> struct iqs269_sys_reg {
> __be16 general;
> u8 active;
> @@ -266,18 +276,7 @@ struct iqs269_sys_reg {
> u8 timeout_swipe;
> u8 thresh_swipe;
> u8 redo_ati;
> -} __packed;
> -
> -struct iqs269_ch_reg {
> - u8 rx_enable;
> - u8 tx_enable;
> - __be16 engine_a;
> - __be16 engine_b;
> - __be16 ati_comp;
> - u8 thresh[3];
> - u8 hyst;
> - u8 assoc_select;
> - u8 assoc_weight;
> + struct iqs269_ch_reg ch_reg[IQS269_NUM_CH];
> } __packed;
>
> struct iqs269_flags {
> @@ -292,7 +291,6 @@ struct iqs269_private {
> struct regmap *regmap;
> struct mutex lock;
> struct iqs269_switch_desc switches[ARRAY_SIZE(iqs269_events)];
> - struct iqs269_ch_reg ch_reg[IQS269_NUM_CH];
> struct iqs269_sys_reg sys_reg;
> struct input_dev *keypad;
> struct input_dev *slider[IQS269_NUM_SL];
> @@ -307,6 +305,7 @@ struct iqs269_private {
> static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
> unsigned int ch_num, unsigned int mode)
> {
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> u16 engine_a;
>
> if (ch_num >= IQS269_NUM_CH)
> @@ -317,12 +316,12 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
>
> mutex_lock(&iqs269->lock);
>
> - engine_a = be16_to_cpu(iqs269->ch_reg[ch_num].engine_a);
> + engine_a = be16_to_cpu(ch_reg[ch_num].engine_a);
>
> engine_a &= ~IQS269_CHx_ENG_A_ATI_MODE_MASK;
> engine_a |= (mode << IQS269_CHx_ENG_A_ATI_MODE_SHIFT);
>
> - iqs269->ch_reg[ch_num].engine_a = cpu_to_be16(engine_a);
> + ch_reg[ch_num].engine_a = cpu_to_be16(engine_a);
> iqs269->ati_current = false;
>
> mutex_unlock(&iqs269->lock);
> @@ -333,13 +332,14 @@ static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
> static int iqs269_ati_mode_get(struct iqs269_private *iqs269,
> unsigned int ch_num, unsigned int *mode)
> {
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> u16 engine_a;
>
> if (ch_num >= IQS269_NUM_CH)
> return -EINVAL;
>
> mutex_lock(&iqs269->lock);
> - engine_a = be16_to_cpu(iqs269->ch_reg[ch_num].engine_a);
> + engine_a = be16_to_cpu(ch_reg[ch_num].engine_a);
> mutex_unlock(&iqs269->lock);
>
> engine_a &= IQS269_CHx_ENG_A_ATI_MODE_MASK;
> @@ -351,6 +351,7 @@ static int iqs269_ati_mode_get(struct iqs269_private *iqs269,
> static int iqs269_ati_base_set(struct iqs269_private *iqs269,
> unsigned int ch_num, unsigned int base)
> {
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> u16 engine_b;
>
> if (ch_num >= IQS269_NUM_CH)
> @@ -379,12 +380,12 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269,
>
> mutex_lock(&iqs269->lock);
>
> - engine_b = be16_to_cpu(iqs269->ch_reg[ch_num].engine_b);
> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
>
> engine_b &= ~IQS269_CHx_ENG_B_ATI_BASE_MASK;
> engine_b |= base;
>
> - iqs269->ch_reg[ch_num].engine_b = cpu_to_be16(engine_b);
> + ch_reg[ch_num].engine_b = cpu_to_be16(engine_b);
> iqs269->ati_current = false;
>
> mutex_unlock(&iqs269->lock);
> @@ -395,13 +396,14 @@ static int iqs269_ati_base_set(struct iqs269_private *iqs269,
> static int iqs269_ati_base_get(struct iqs269_private *iqs269,
> unsigned int ch_num, unsigned int *base)
> {
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> u16 engine_b;
>
> if (ch_num >= IQS269_NUM_CH)
> return -EINVAL;
>
> mutex_lock(&iqs269->lock);
> - engine_b = be16_to_cpu(iqs269->ch_reg[ch_num].engine_b);
> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
> mutex_unlock(&iqs269->lock);
>
> switch (engine_b & IQS269_CHx_ENG_B_ATI_BASE_MASK) {
> @@ -429,6 +431,7 @@ static int iqs269_ati_base_get(struct iqs269_private *iqs269,
> static int iqs269_ati_target_set(struct iqs269_private *iqs269,
> unsigned int ch_num, unsigned int target)
> {
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> u16 engine_b;
>
> if (ch_num >= IQS269_NUM_CH)
> @@ -439,12 +442,12 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269,
>
> mutex_lock(&iqs269->lock);
>
> - engine_b = be16_to_cpu(iqs269->ch_reg[ch_num].engine_b);
> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
>
> engine_b &= ~IQS269_CHx_ENG_B_ATI_TARGET_MASK;
> engine_b |= target / 32;
>
> - iqs269->ch_reg[ch_num].engine_b = cpu_to_be16(engine_b);
> + ch_reg[ch_num].engine_b = cpu_to_be16(engine_b);
> iqs269->ati_current = false;
>
> mutex_unlock(&iqs269->lock);
> @@ -455,13 +458,14 @@ static int iqs269_ati_target_set(struct iqs269_private *iqs269,
> static int iqs269_ati_target_get(struct iqs269_private *iqs269,
> unsigned int ch_num, unsigned int *target)
> {
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> u16 engine_b;
>
> if (ch_num >= IQS269_NUM_CH)
> return -EINVAL;
>
> mutex_lock(&iqs269->lock);
> - engine_b = be16_to_cpu(iqs269->ch_reg[ch_num].engine_b);
> + engine_b = be16_to_cpu(ch_reg[ch_num].engine_b);
> mutex_unlock(&iqs269->lock);
>
> *target = (engine_b & IQS269_CHx_ENG_B_ATI_TARGET_MASK) * 32;
> @@ -531,13 +535,7 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
> if (fwnode_property_present(ch_node, "azoteq,slider1-select"))
> iqs269->sys_reg.slider_select[1] |= BIT(reg);
>
> - ch_reg = &iqs269->ch_reg[reg];
> -
> - error = regmap_raw_read(iqs269->regmap,
> - IQS269_CHx_SETTINGS + reg * sizeof(*ch_reg) / 2,
> - ch_reg, sizeof(*ch_reg));
> - if (error)
> - return error;
> + ch_reg = &iqs269->sys_reg.ch_reg[reg];
>
> error = iqs269_parse_mask(ch_node, "azoteq,rx-enable",
> &ch_reg->rx_enable);
> @@ -1048,10 +1046,8 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
>
> static int iqs269_dev_init(struct iqs269_private *iqs269)
> {
> - struct iqs269_sys_reg *sys_reg = &iqs269->sys_reg;
> - struct iqs269_ch_reg *ch_reg;
> unsigned int val;
> - int error, i;
> + int error;
>
> mutex_lock(&iqs269->lock);
>
> @@ -1061,27 +1057,8 @@ static int iqs269_dev_init(struct iqs269_private *iqs269)
> if (error)
> goto err_mutex;
>
> - for (i = 0; i < IQS269_NUM_CH; i++) {
> - if (!(sys_reg->active & BIT(i)))
> - continue;
> -
> - ch_reg = &iqs269->ch_reg[i];
> -
> - error = regmap_raw_write(iqs269->regmap,
> - IQS269_CHx_SETTINGS + i *
> - sizeof(*ch_reg) / 2, ch_reg,
> - sizeof(*ch_reg));
> - if (error)
> - goto err_mutex;
> - }
> -
> - /*
> - * The REDO-ATI and ATI channel selection fields must be written in the
> - * same block write, so every field between registers 0x80 through 0x8B
> - * (inclusive) must be written as well.
> - */
> - error = regmap_raw_write(iqs269->regmap, IQS269_SYS_SETTINGS, sys_reg,
> - sizeof(*sys_reg));
> + error = regmap_raw_write(iqs269->regmap, IQS269_SYS_SETTINGS,
> + &iqs269->sys_reg, sizeof(iqs269->sys_reg));
> if (error)
> goto err_mutex;
>
> @@ -1355,6 +1332,7 @@ static ssize_t hall_bin_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct iqs269_private *iqs269 = dev_get_drvdata(dev);
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> struct i2c_client *client = iqs269->client;
> unsigned int val;
> int error;
> @@ -1369,8 +1347,8 @@ static ssize_t hall_bin_show(struct device *dev,
> if (error)
> return error;
>
> - switch (iqs269->ch_reg[IQS269_CHx_HALL_ACTIVE].rx_enable &
> - iqs269->ch_reg[IQS269_CHx_HALL_INACTIVE].rx_enable) {
> + switch (ch_reg[IQS269_CHx_HALL_ACTIVE].rx_enable &
> + ch_reg[IQS269_CHx_HALL_INACTIVE].rx_enable) {
> case IQS269_HALL_PAD_R:
> val &= IQS269_CAL_DATA_A_HALL_BIN_R_MASK;
> val >>= IQS269_CAL_DATA_A_HALL_BIN_R_SHIFT;
> @@ -1450,9 +1428,10 @@ static ssize_t rx_enable_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct iqs269_private *iqs269 = dev_get_drvdata(dev);
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
>
> return scnprintf(buf, PAGE_SIZE, "%u\n",
> - iqs269->ch_reg[iqs269->ch_num].rx_enable);
> + ch_reg[iqs269->ch_num].rx_enable);
> }
>
> static ssize_t rx_enable_store(struct device *dev,
> @@ -1460,6 +1439,7 @@ static ssize_t rx_enable_store(struct device *dev,
> size_t count)
> {
> struct iqs269_private *iqs269 = dev_get_drvdata(dev);
> + struct iqs269_ch_reg *ch_reg = iqs269->sys_reg.ch_reg;
> unsigned int val;
> int error;
>
> @@ -1472,7 +1452,7 @@ static ssize_t rx_enable_store(struct device *dev,
>
> mutex_lock(&iqs269->lock);
>
> - iqs269->ch_reg[iqs269->ch_num].rx_enable = val;
> + ch_reg[iqs269->ch_num].rx_enable = val;
> iqs269->ati_current = false;
>
> mutex_unlock(&iqs269->lock);
> --
> 2.34.1
