Re: [PATCH V9 2/2] iio: proximity: aw96103: Add support for aw96103/aw96105 proximity sensor

From: Andy Shevchenko
Date: Tue Sep 03 2024 - 20:31:20 EST

Tue, Aug 27, 2024 at 08:02:29AM +0000, wangshuaijie@xxxxxxxxxx kirjoitti:
> From: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>
> AW96103 is a low power consumption capacitive touch and proximity controller.
> Each channel can be independently config as sensor input, shield output.
>
> Channel Information:
> aw96103: 3-channel
> aw96105: 5-channel

Instead of review the below is the diff that I think makes sense to apply
(in any convenient form, e.g., squash to the existing commit, splitting
and making followups)

diff --git a/drivers/iio/proximity/aw96103.c b/drivers/iio/proximity/aw96103.c
index 89f7e1bde928..eabbb6b08e67 100644
--- a/drivers/iio/proximity/aw96103.c
+++ b/drivers/iio/proximity/aw96103.c
@@ -6,19 +6,24 @@
*
* Copyright (c) 2024 awinic Technology CO., LTD
*/
+#include <linux/array_size.h>
#include <linux/bits.h>
#include <linux/bitfield.h>
+#include <linux/cleanup.h>
#include <linux/delay.h>
#include <linux/firmware.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
-#include <linux/iio/events.h>
-#include <linux/iio/iio.h>
#include <linux/regulator/consumer.h>
#include <linux/regmap.h>
#include <linux/slab.h>
+#include <linux/types.h>
+
#include <asm/unaligned.h>

+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+
#define AW_DATA_PROCESS_FACTOR 1024
#define AW96103_CHIP_ID 0xa961
#define AW96103_BIN_VALID_DATA_OFFSET 64
@@ -94,8 +99,8 @@ enum aw96103_sensor_type {
};

struct aw_channels_info {
- bool used;
unsigned int old_irq_status;
+ bool used;
};

struct aw_chip_info {
@@ -254,8 +259,8 @@ static int aw96103_get_diff_raw(struct aw96103 *aw96103, unsigned int chan,
AW96103_REG_DIFF_CH0 + chan * 4, &data);
if (ret)
return ret;
- *buf = (int)(data / AW_DATA_PROCESS_FACTOR);

+ *buf = data / AW_DATA_PROCESS_FACTOR;
return 0;
}

@@ -302,8 +307,8 @@ static int aw96103_read_out_debounce(struct aw96103 *aw96103,
AW96103_REG_PROXCTRL_CH(chan->channel), &reg_val);
if (ret)
return ret;
- *val = FIELD_GET(AW96103_OUTDEB_MASK, reg_val);

+ *val = FIELD_GET(AW96103_OUTDEB_MASK, reg_val);
return IIO_VAL_INT;
}

@@ -317,8 +322,8 @@ static int aw96103_read_in_debounce(struct aw96103 *aw96103,
AW96103_REG_PROXCTRL_CH(chan->channel), &reg_val);
if (ret)
return ret;
- *val = FIELD_GET(AW96103_INDEB_MASK, reg_val);

+ *val = FIELD_GET(AW96103_INDEB_MASK, reg_val);
return IIO_VAL_INT;
}

@@ -332,8 +337,8 @@ static int aw96103_read_hysteresis(struct aw96103 *aw96103,
AW96103_REG_PROXCTRL_CH(chan->channel), &reg_val);
if (ret)
return ret;
- *val = FIELD_GET(AW96103_THHYST_MASK, reg_val);

+ *val = FIELD_GET(AW96103_THHYST_MASK, reg_val);
return IIO_VAL_INT;
}

@@ -454,11 +459,10 @@ static int aw96103_channel_scan_start(struct aw96103 *aw96103)
aw96103->hostirqen);
}

-static int aw96103_reg_version_comp(struct aw96103 *aw96103,
- struct aw_bin *aw_bin)
+static int aw96103_reg_version_comp(struct aw96103 *aw96103, struct aw_bin *aw_bin)
{
u32 blfilt1_data, fw_ver;
- unsigned char i;
+ unsigned int i;
int ret;

ret = regmap_read(aw96103->regmap, AW96103_REG_FWVER2, &fw_ver);
@@ -474,16 +478,17 @@ static int aw96103_reg_version_comp(struct aw96103 *aw96103,

for (i = 0; i < aw96103->max_channels; i++) {
ret = regmap_read(aw96103->regmap,
- AW96103_REG_BLFILT_CH0 + (AW96103_BLFILT_CH_STEP * i),
+ AW96103_REG_BLFILT_CH0 + AW96103_BLFILT_CH_STEP * i,
&blfilt1_data);
if (ret)
return ret;
+
if (FIELD_GET(AW96103_BLERRTRIG_MASK, blfilt1_data) != 1)
return 0;

return regmap_update_bits(aw96103->regmap,
- AW96103_REG_BLRSTRNG_CH0 + (AW96103_BLFILT_CH_STEP * i),
- AW96103_BLRSTRNG_MASK, 1 << i);
+ AW96103_REG_BLRSTRNG_CH0 + AW96103_BLFILT_CH_STEP * i,
+ AW96103_BLRSTRNG_MASK, BIT(i));
}

return 0;
@@ -493,25 +498,22 @@ static int aw96103_bin_valid_loaded(struct aw96103 *aw96103,
struct aw_bin *aw_bin_data_s)
{
unsigned int start_addr = aw_bin_data_s->valid_data_addr;
- u32 i, reg_data;
+ unsigned int i;
+ u32 reg_data;
u16 reg_addr;
int ret;

- for (i = 0; i < aw_bin_data_s->valid_data_len;
- i += 6, start_addr += 6) {
+ for (i = 0; i < aw_bin_data_s->valid_data_len; i += 6, start_addr += 6) {
reg_addr = get_unaligned_le16(aw_bin_data_s->data + start_addr);
- reg_data = get_unaligned_le32(aw_bin_data_s->data +
- start_addr + 2);
- if ((reg_addr == AW96103_REG_EEDA0) ||
- (reg_addr == AW96103_REG_EEDA1))
+ reg_data = get_unaligned_le32(aw_bin_data_s->data + start_addr + 2);
+ if ((reg_addr == AW96103_REG_EEDA0) || (reg_addr == AW96103_REG_EEDA1))
continue;
if (reg_addr == AW96103_REG_IRQEN) {
aw96103->hostirqen = reg_data;
continue;
}
if (reg_addr == AW96103_REG_SCANCTRL0)
- aw96103->chan_en = FIELD_GET(AW96103_CHAN_EN_MASK,
- reg_data);
+ aw96103->chan_en = FIELD_GET(AW96103_CHAN_EN_MASK, reg_data);

ret = regmap_write(aw96103->regmap, reg_addr, reg_data);
if (ret < 0)
@@ -527,19 +529,21 @@ static int aw96103_bin_valid_loaded(struct aw96103 *aw96103,

static int aw96103_para_loaded(struct aw96103 *aw96103)
{
- int i, ret;
+ unsigned int i;
+ int ret;

for (i = 0; i < ARRAY_SIZE(aw96103_reg_default); i += 2) {
- ret = regmap_write(aw96103->regmap,
- (u16)aw96103_reg_default[i],
- (u32)aw96103_reg_default[i + 1]);
+ u16 offset = aw96103_reg_default[i];
+ u32 value = aw96103_reg_default[i + 1];
+
+ ret = regmap_write(aw96103->regmap, offset, value);
if (ret)
return ret;
- if (aw96103_reg_default[i] == AW96103_REG_IRQEN)
- aw96103->hostirqen = aw96103_reg_default[i + 1];
- else if (aw96103_reg_default[i] == AW96103_REG_SCANCTRL0)
- aw96103->chan_en = FIELD_GET(AW96103_CHAN_EN_MASK,
- aw96103_reg_default[i + 1]);
+
+ if (offset == AW96103_REG_IRQEN)
+ aw96103->hostirqen = value;
+ else if (offset == AW96103_REG_SCANCTRL0)
+ aw96103->chan_en = FIELD_GET(AW96103_CHAN_EN_MASK, value);
}

return aw96103_channel_scan_start(aw96103);
@@ -567,7 +571,8 @@ static int aw96103_cfg_all_loaded(const struct firmware *cont,
static void aw96103_cfg_update(const struct firmware *fw, void *data)
{
struct aw96103 *aw96103 = data;
- int ret, i;
+ unsigned int i;
+ int ret;

if (!fw || !fw->data) {
dev_err(aw96103->dev, "No firmware.\n");
@@ -588,12 +593,9 @@ static void aw96103_cfg_update(const struct firmware *fw, void *data)
}
}

- for (i = 0; i < aw96103->max_channels; i++) {
- if ((aw96103->chan_en >> i) & 0x01)
- aw96103->channels_arr[i].used = true;
- else
+ for (i = 0; i < aw96103->max_channels; i++)
+ aw96103->channels_arr[i].used = aw96103->chan_en & BIT(i);
aw96103->channels_arr[i].used = false;
- }
}

static int aw96103_sw_reset(struct aw96103 *aw96103)
@@ -603,7 +605,7 @@ static int aw96103_sw_reset(struct aw96103 *aw96103)
ret = regmap_write(aw96103->regmap, AW96103_REG_RESET, 0);
/*
* After reset, the initialization process starts to perform and
- * it will last for a bout 20ms.
+ * it will last for about 20ms.
*/
msleep(20);

@@ -611,7 +613,7 @@ static int aw96103_sw_reset(struct aw96103 *aw96103)
}

enum aw96103_irq_trigger_position {
- FAR = 0,
+ FAR = 0x00,
TRIGGER_TH0 = 0x01,
TRIGGER_TH1 = 0x03,
TRIGGER_TH2 = 0x07,
@@ -623,7 +625,8 @@ static irqreturn_t aw96103_irq(int irq, void *data)
unsigned int irq_status, curr_status_val, curr_status;
struct iio_dev *indio_dev = data;
struct aw96103 *aw96103 = iio_priv(indio_dev);
- int ret, i;
+ unsigned int i;
+ int ret;

ret = regmap_read(aw96103->regmap, AW96103_REG_IRQSRC, &irq_status);
if (ret)
@@ -641,10 +644,12 @@ static irqreturn_t aw96103_irq(int irq, void *data)
if (!aw96103->channels_arr[i].used)
continue;

- curr_status = (((curr_status_val >> (24 + i)) & 0x1)) |
- (((curr_status_val >> (16 + i)) & 0x1) << 1) |
- (((curr_status_val >> (8 + i)) & 0x1) << 2) |
- (((curr_status_val >> i) & 0x1) << 3);
+ curr_status = curr_status_val >> i;
+ curr_status = ((curr_status >> 24 + 0) & BIT(0)) |
+ ((curr_status >> 16 + 1) & BIT(0)) |
+ ((curr_status >> 8 + 2) & BIT(0)) |
+ ((curr_status >> 0 + 3) & BIT(0));
+
if (aw96103->channels_arr[i].old_irq_status == curr_status)
continue;

@@ -675,8 +680,7 @@ static irqreturn_t aw96103_irq(int irq, void *data)
return IRQ_HANDLED;
}

-static int aw96103_interrupt_init(struct iio_dev *indio_dev,
- struct i2c_client *i2c)
+static int aw96103_interrupt_init(struct iio_dev *indio_dev, struct i2c_client *i2c)
{
struct aw96103 *aw96103 = iio_priv(indio_dev);
unsigned int irq_status;
@@ -685,9 +689,11 @@ static int aw96103_interrupt_init(struct iio_dev *indio_dev,
ret = regmap_write(aw96103->regmap, AW96103_REG_IRQEN, 0);
if (ret)
return ret;
+
ret = regmap_read(aw96103->regmap, AW96103_REG_IRQSRC, &irq_status);
if (ret)
return ret;
+
ret = devm_request_threaded_irq(aw96103->dev, i2c->irq, NULL,
aw96103_irq, IRQF_ONESHOT,
"aw96103_irq", indio_dev);
@@ -700,26 +706,17 @@ static int aw96103_interrupt_init(struct iio_dev *indio_dev,

static int aw96103_wait_chip_init(struct aw96103 *aw96103)
{
- unsigned int cnt = 20;
u32 reg_data;
int ret;

- while (cnt--) {
- /*
- * The device should generate an initialization completion
- * interrupt within 20ms.
- */
- ret = regmap_read(aw96103->regmap, AW96103_REG_IRQSRC,
- &reg_data);
- if (ret)
- return ret;
-
- if (FIELD_GET(AW96103_INITOVERIRQ_MASK, reg_data))
- return 0;
- fsleep(1000);
- }
-
- return -ETIMEDOUT;
+ /*
+ * The device should generate an initialization completion
+ * interrupt within 20ms.
+ */
+ return regmap_read_poll_timeout(aw96103->regmap, AW96103_REG_IRQSRC,
+ reg_data,
+ FIELD_GET(AW96103_INITOVERIRQ_MASK, reg_data),
+ 1000, 20000);
}

static int aw96103_read_chipid(struct aw96103 *aw96103)

--
With Best Regards,
Andy Shevchenko