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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

Tue, Aug 27, 2024 at 08:02:29AM +0000, wangshuaijie@awinic.com kirjoitti:
> From: shuaijie wang <wangshuaijie@awinic.com>
> 
> 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