[PATCH 0/4] Input: adp5588-keys: refactor adp5588_read()

[PATCH 0/4] Input: adp5588-keys: refactor adp5588_read()

[Nuno Sa]

Hi Dmitry,

As suggested by you in [1], here it goes a small series to make sure we
check for error codes in adp5588_read() plus a following patch
refactoring the function.

In addition a couple more commits doing a simple conversion to
dev_err_probe() and a sanity check for IRQ presence (when the keymap is
to be used).

[1]: https://lore.kernel.org/linux-input/Zu0vq0ogr2HzXWv7@google.com/

---
Nuno Sa (4):
      Input: adp5588-keys: bail on returned error
      Input: adp5588-keys: refactor adp5589_read()
      Input: adp5588-keys: error out if no IRQ is given
      Input: adp5588-keys: make use of dev_err_probe()

 drivers/input/keyboard/adp5588-keys.c | 151 +++++++++++++++++++---------------
 1 file changed, 86 insertions(+), 65 deletions(-)
---
base-commit: c684771630e64bc39bddffeb65dd8a6612a6b249
change-id: 20241002-fix-adp5588-read-refactor-a85c88af4062
--

Thanks!
- Nuno Sá

[PATCH 0/4] Input: adp5588-keys: refactor adp5588_read()

[Nuno Sa via B4 Relay]

Hi Dmitry,

As suggested by you in [1], here it goes a small series to make sure we
check for error codes in adp5588_read() plus a following patch
refactoring the function.

In addition a couple more commits doing a simple conversion to
dev_err_probe() and a sanity check for IRQ presence (when the keymap is
to be used).

[1]: https://lore.kernel.org/linux-input/Zu0vq0ogr2HzXWv7@google.com/

---
Nuno Sa (4):
      Input: adp5588-keys: bail on returned error
      Input: adp5588-keys: refactor adp5589_read()
      Input: adp5588-keys: error out if no IRQ is given
      Input: adp5588-keys: make use of dev_err_probe()

 drivers/input/keyboard/adp5588-keys.c | 151 +++++++++++++++++++---------------
 1 file changed, 86 insertions(+), 65 deletions(-)
---
base-commit: c684771630e64bc39bddffeb65dd8a6612a6b249
change-id: 20241002-fix-adp5588-read-refactor-a85c88af4062
--

Thanks!
- Nuno Sá

[PATCH 1/4] Input: adp5588-keys: bail on returned error

[Nuno Sa]

Bail out in case adp5588_read() return an error code. Not doing so is
asking for problems.

While at it, did a small refactor in adp5588_gpio_get_value() to make it
easier to handle checking the return value.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 39 ++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index d25d63a807f23..9ac68baf01179 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -225,9 +225,11 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned int off)
 	guard(mutex)(&kpad->gpio_lock);
 
 	if (kpad->dir[bank] & bit)
-		val = kpad->dat_out[bank];
-	else
-		val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
+		return !!(kpad->dat_out[bank] & bit);
+
+	val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
+	if (val < 0)
+		return val;
 
 	return !!(val & bit);
 }
@@ -455,8 +457,16 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 		kpad->dat_out[i] = adp5588_read(kpad->client,
 						GPIO_DAT_OUT1 + i);
+		if (kpad->dat_out[i] < 0)
+			return kpad->dat_out[i];
+
 		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
+		if (kpad->dir[i] < 0)
+			return kpad->dir[i];
+
 		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
+		if (kpad->pull_dis[i] < 0)
+			return kpad->pull_dis[i];
 	}
 
 	return 0;
@@ -519,9 +529,14 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key = adp5588_read(kpad->client, KEY_EVENTA + i);
-		int key_val = key & KEY_EV_MASK;
-		int key_press = key & KEY_EV_PRESSED;
+		int key, key_val, key_press;
+
+		key = adp5588_read(kpad->client, KEY_EVENTA + i);
+		if (key < 0)
+			return;
+
+		key_val = key & KEY_EV_MASK;
+		key_press = key & KEY_EV_PRESSED;
 
 		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
 			/* gpio line used as IRQ source */
@@ -572,18 +587,22 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 	}
 
 	status = adp5588_read(client, INT_STAT);
+	if (status < 0)
+		return IRQ_HANDLED;
 
 	if (status & ADP5588_OVR_FLOW_INT)	/* Unlikely and should never happen */
 		dev_err(&client->dev, "Event Overflow Error\n");
 
 	if (status & ADP5588_KE_INT) {
 		ev_cnt = adp5588_read(client, KEY_LCK_EC_STAT) & ADP5588_KEC;
-		if (ev_cnt) {
-			adp5588_report_events(kpad, ev_cnt);
-			input_sync(kpad->input);
-		}
+		if (ev_cnt <= 0)
+			goto out_irq;
+
+		adp5588_report_events(kpad, ev_cnt);
+		input_sync(kpad->input);
 	}
 
+out_irq:
 	adp5588_write(client, INT_STAT, status); /* Status is W1C */
 
 	return IRQ_HANDLED;

-- 
2.46.1

[PATCH 1/4] Input: adp5588-keys: bail on returned error

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Bail out in case adp5588_read() return an error code. Not doing so is
asking for problems.

While at it, did a small refactor in adp5588_gpio_get_value() to make it
easier to handle checking the return value.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 39 ++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index d25d63a807f23..9ac68baf01179 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -225,9 +225,11 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned int off)
 	guard(mutex)(&kpad->gpio_lock);
 
 	if (kpad->dir[bank] & bit)
-		val = kpad->dat_out[bank];
-	else
-		val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
+		return !!(kpad->dat_out[bank] & bit);
+
+	val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
+	if (val < 0)
+		return val;
 
 	return !!(val & bit);
 }
@@ -455,8 +457,16 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 		kpad->dat_out[i] = adp5588_read(kpad->client,
 						GPIO_DAT_OUT1 + i);
+		if (kpad->dat_out[i] < 0)
+			return kpad->dat_out[i];
+
 		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
+		if (kpad->dir[i] < 0)
+			return kpad->dir[i];
+
 		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
+		if (kpad->pull_dis[i] < 0)
+			return kpad->pull_dis[i];
 	}
 
 	return 0;
@@ -519,9 +529,14 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key = adp5588_read(kpad->client, KEY_EVENTA + i);
-		int key_val = key & KEY_EV_MASK;
-		int key_press = key & KEY_EV_PRESSED;
+		int key, key_val, key_press;
+
+		key = adp5588_read(kpad->client, KEY_EVENTA + i);
+		if (key < 0)
+			return;
+
+		key_val = key & KEY_EV_MASK;
+		key_press = key & KEY_EV_PRESSED;
 
 		if (key_val >= GPI_PIN_BASE && key_val <= GPI_PIN_END) {
 			/* gpio line used as IRQ source */
@@ -572,18 +587,22 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 	}
 
 	status = adp5588_read(client, INT_STAT);
+	if (status < 0)
+		return IRQ_HANDLED;
 
 	if (status & ADP5588_OVR_FLOW_INT)	/* Unlikely and should never happen */
 		dev_err(&client->dev, "Event Overflow Error\n");
 
 	if (status & ADP5588_KE_INT) {
 		ev_cnt = adp5588_read(client, KEY_LCK_EC_STAT) & ADP5588_KEC;
-		if (ev_cnt) {
-			adp5588_report_events(kpad, ev_cnt);
-			input_sync(kpad->input);
-		}
+		if (ev_cnt <= 0)
+			goto out_irq;
+
+		adp5588_report_events(kpad, ev_cnt);
+		input_sync(kpad->input);
 	}
 
+out_irq:
 	adp5588_write(client, INT_STAT, status); /* Status is W1C */
 
 	return IRQ_HANDLED;

-- 
2.46.1

[PATCH 2/4] Input: adp5588-keys: refactor adp5589_read()

[Nuno Sa]

adp5588_read() now either returns 0 or an error code. The value to read
is passed as an argument to the function. Therefore we don't mix
register values with error codes.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 75 ++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 9ac68baf0117..11a70ee18482 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -200,14 +200,17 @@ struct adp5588_kpad {
 	u8 pull_dis[3];
 };
 
-static int adp5588_read(struct i2c_client *client, u8 reg)
+static int adp5588_read(struct i2c_client *client, u8 reg, u8 *val)
 {
 	int ret = i2c_smbus_read_byte_data(client, reg);
 
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "Read Error\n");
+		return ret;
+	}
 
-	return ret;
+	*val = ret;
+	return 0;
 }
 
 static int adp5588_write(struct i2c_client *client, u8 reg, u8 val)
@@ -220,16 +223,17 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned int off)
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
 	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
 	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
-	int val;
+	int error;
+	u8 val;
 
 	guard(mutex)(&kpad->gpio_lock);
 
 	if (kpad->dir[bank] & bit)
 		return !!(kpad->dat_out[bank] & bit);
 
-	val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
-	if (val < 0)
-		return val;
+	error = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank, &val);
+	if (error)
+		return error;
 
 	return !!(val & bit);
 }
@@ -455,18 +459,20 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	}
 
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
-		kpad->dat_out[i] = adp5588_read(kpad->client,
-						GPIO_DAT_OUT1 + i);
-		if (kpad->dat_out[i] < 0)
-			return kpad->dat_out[i];
+		error = adp5588_read(kpad->client,
+				     GPIO_DAT_OUT1 + i, &kpad->dat_out[i]);
+		if (error)
+			return error;
 
-		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
-		if (kpad->dir[i] < 0)
-			return kpad->dir[i];
+		error = adp5588_read(kpad->client, GPIO_DIR1 + i,
+				     &kpad->dir[i]);
+		if (error)
+			return error;
 
-		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
-		if (kpad->pull_dis[i] < 0)
-			return kpad->pull_dis[i];
+		error = adp5588_read(kpad->client, GPIO_PULL1 + i,
+				     &kpad->pull_dis[i]);
+		if (error)
+			return error;
 	}
 
 	return 0;
@@ -529,10 +535,11 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key, key_val, key_press;
+		u8 key, key_val, key_press;
+		int error;
 
-		key = adp5588_read(kpad->client, KEY_EVENTA + i);
-		if (key < 0)
+		error = adp5588_read(kpad->client, KEY_EVENTA + i, &key);
+		if (error)
 			return;
 
 		key_val = key & KEY_EV_MASK;
@@ -571,7 +578,8 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 	struct i2c_client *client = kpad->client;
 	ktime_t target_time, now;
 	unsigned long delay;
-	int status, ev_cnt;
+	u8 status, ev_cnt;
+	int error;
 
 	/*
 	 * Readout needs to wait for at least 25ms after the notification
@@ -586,19 +594,19 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 		}
 	}
 
-	status = adp5588_read(client, INT_STAT);
-	if (status < 0)
+	error = adp5588_read(client, INT_STAT, &status);
+	if (error)
 		return IRQ_HANDLED;
 
 	if (status & ADP5588_OVR_FLOW_INT)	/* Unlikely and should never happen */
 		dev_err(&client->dev, "Event Overflow Error\n");
 
 	if (status & ADP5588_KE_INT) {
-		ev_cnt = adp5588_read(client, KEY_LCK_EC_STAT) & ADP5588_KEC;
-		if (ev_cnt <= 0)
+		error = adp5588_read(client, KEY_LCK_EC_STAT, &ev_cnt);
+		if (error || !(ev_cnt & ADP5588_KEC))
 			goto out_irq;
 
-		adp5588_report_events(kpad, ev_cnt);
+		adp5588_report_events(kpad, ev_cnt & ADP5588_KEC);
 		input_sync(kpad->input);
 	}
 
@@ -612,6 +620,7 @@ static int adp5588_setup(struct adp5588_kpad *kpad)
 {
 	struct i2c_client *client = kpad->client;
 	int i, ret;
+	u8 dummy;
 
 	ret = adp5588_write(client, KP_GPIO1, KP_SEL(kpad->rows));
 	if (ret)
@@ -638,8 +647,8 @@ static int adp5588_setup(struct adp5588_kpad *kpad)
 	}
 
 	for (i = 0; i < KEYP_MAX_EVENT; i++) {
-		ret = adp5588_read(client, KEY_EVENTA);
-		if (ret < 0)
+		ret = adp5588_read(client, KEY_EVENTA, &dummy);
+		if (ret)
 			return ret;
 	}
 
@@ -743,8 +752,8 @@ static int adp5588_probe(struct i2c_client *client)
 	struct input_dev *input;
 	struct gpio_desc *gpio;
 	unsigned int revid;
-	int ret;
 	int error;
+	u8 id;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -781,11 +790,11 @@ static int adp5588_probe(struct i2c_client *client)
 		fsleep(60);
 	}
 
-	ret = adp5588_read(client, DEV_ID);
-	if (ret < 0)
-		return ret;
+	error = adp5588_read(client, DEV_ID, &id);
+	if (error)
+		return error;
 
-	revid = ret & ADP5588_DEVICE_ID_MASK;
+	revid = id & ADP5588_DEVICE_ID_MASK;
 	if (WA_DELAYED_READOUT_REVID(revid))
 		kpad->delay = msecs_to_jiffies(WA_DELAYED_READOUT_TIME);
 

-- 
2.46.1

[PATCH 2/4] Input: adp5588-keys: refactor adp5589_read()

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

adp5588_read() now either returns 0 or an error code. The value to read
is passed as an argument to the function. Therefore we don't mix
register values with error codes.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 75 ++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 9ac68baf0117..11a70ee18482 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -200,14 +200,17 @@ struct adp5588_kpad {
 	u8 pull_dis[3];
 };
 
-static int adp5588_read(struct i2c_client *client, u8 reg)
+static int adp5588_read(struct i2c_client *client, u8 reg, u8 *val)
 {
 	int ret = i2c_smbus_read_byte_data(client, reg);
 
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "Read Error\n");
+		return ret;
+	}
 
-	return ret;
+	*val = ret;
+	return 0;
 }
 
 static int adp5588_write(struct i2c_client *client, u8 reg, u8 val)
@@ -220,16 +223,17 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned int off)
 	struct adp5588_kpad *kpad = gpiochip_get_data(chip);
 	unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
 	unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
-	int val;
+	int error;
+	u8 val;
 
 	guard(mutex)(&kpad->gpio_lock);
 
 	if (kpad->dir[bank] & bit)
 		return !!(kpad->dat_out[bank] & bit);
 
-	val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
-	if (val < 0)
-		return val;
+	error = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank, &val);
+	if (error)
+		return error;
 
 	return !!(val & bit);
 }
@@ -455,18 +459,20 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	}
 
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
-		kpad->dat_out[i] = adp5588_read(kpad->client,
-						GPIO_DAT_OUT1 + i);
-		if (kpad->dat_out[i] < 0)
-			return kpad->dat_out[i];
+		error = adp5588_read(kpad->client,
+				     GPIO_DAT_OUT1 + i, &kpad->dat_out[i]);
+		if (error)
+			return error;
 
-		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
-		if (kpad->dir[i] < 0)
-			return kpad->dir[i];
+		error = adp5588_read(kpad->client, GPIO_DIR1 + i,
+				     &kpad->dir[i]);
+		if (error)
+			return error;
 
-		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
-		if (kpad->pull_dis[i] < 0)
-			return kpad->pull_dis[i];
+		error = adp5588_read(kpad->client, GPIO_PULL1 + i,
+				     &kpad->pull_dis[i]);
+		if (error)
+			return error;
 	}
 
 	return 0;
@@ -529,10 +535,11 @@ static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
 	int i;
 
 	for (i = 0; i < ev_cnt; i++) {
-		int key, key_val, key_press;
+		u8 key, key_val, key_press;
+		int error;
 
-		key = adp5588_read(kpad->client, KEY_EVENTA + i);
-		if (key < 0)
+		error = adp5588_read(kpad->client, KEY_EVENTA + i, &key);
+		if (error)
 			return;
 
 		key_val = key & KEY_EV_MASK;
@@ -571,7 +578,8 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 	struct i2c_client *client = kpad->client;
 	ktime_t target_time, now;
 	unsigned long delay;
-	int status, ev_cnt;
+	u8 status, ev_cnt;
+	int error;
 
 	/*
 	 * Readout needs to wait for at least 25ms after the notification
@@ -586,19 +594,19 @@ static irqreturn_t adp5588_thread_irq(int irq, void *handle)
 		}
 	}
 
-	status = adp5588_read(client, INT_STAT);
-	if (status < 0)
+	error = adp5588_read(client, INT_STAT, &status);
+	if (error)
 		return IRQ_HANDLED;
 
 	if (status & ADP5588_OVR_FLOW_INT)	/* Unlikely and should never happen */
 		dev_err(&client->dev, "Event Overflow Error\n");
 
 	if (status & ADP5588_KE_INT) {
-		ev_cnt = adp5588_read(client, KEY_LCK_EC_STAT) & ADP5588_KEC;
-		if (ev_cnt <= 0)
+		error = adp5588_read(client, KEY_LCK_EC_STAT, &ev_cnt);
+		if (error || !(ev_cnt & ADP5588_KEC))
 			goto out_irq;
 
-		adp5588_report_events(kpad, ev_cnt);
+		adp5588_report_events(kpad, ev_cnt & ADP5588_KEC);
 		input_sync(kpad->input);
 	}
 
@@ -612,6 +620,7 @@ static int adp5588_setup(struct adp5588_kpad *kpad)
 {
 	struct i2c_client *client = kpad->client;
 	int i, ret;
+	u8 dummy;
 
 	ret = adp5588_write(client, KP_GPIO1, KP_SEL(kpad->rows));
 	if (ret)
@@ -638,8 +647,8 @@ static int adp5588_setup(struct adp5588_kpad *kpad)
 	}
 
 	for (i = 0; i < KEYP_MAX_EVENT; i++) {
-		ret = adp5588_read(client, KEY_EVENTA);
-		if (ret < 0)
+		ret = adp5588_read(client, KEY_EVENTA, &dummy);
+		if (ret)
 			return ret;
 	}
 
@@ -743,8 +752,8 @@ static int adp5588_probe(struct i2c_client *client)
 	struct input_dev *input;
 	struct gpio_desc *gpio;
 	unsigned int revid;
-	int ret;
 	int error;
+	u8 id;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -781,11 +790,11 @@ static int adp5588_probe(struct i2c_client *client)
 		fsleep(60);
 	}
 
-	ret = adp5588_read(client, DEV_ID);
-	if (ret < 0)
-		return ret;
+	error = adp5588_read(client, DEV_ID, &id);
+	if (error)
+		return error;
 
-	revid = ret & ADP5588_DEVICE_ID_MASK;
+	revid = id & ADP5588_DEVICE_ID_MASK;
 	if (WA_DELAYED_READOUT_REVID(revid))
 		kpad->delay = msecs_to_jiffies(WA_DELAYED_READOUT_TIME);
 

-- 
2.46.1

[PATCH 3/4] Input: adp5588-keys: error out if no IRQ is given

[Nuno Sa]

If the keypad is configured, it also depends on the presence of an
interrupt. With
commit dc748812fca0 ("Input: adp5588-keys - add support for pure gpio"),
having an interrupt is no longer mandatory so better check for it when
it is indeed mandatory.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 11a70ee18482..0152e4fa088c 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -680,6 +680,11 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
+	if (!client->irq) {
+		dev_err(&client->dev, "Keypad configured but no IRQ present\n");
+		return -EINVAL;
+	}
+
 	ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
 					     &kpad->cols);
 	if (ret)

-- 
2.46.1

[PATCH 3/4] Input: adp5588-keys: error out if no IRQ is given

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

If the keypad is configured, it also depends on the presence of an
interrupt. With
commit dc748812fca0 ("Input: adp5588-keys - add support for pure gpio"),
having an interrupt is no longer mandatory so better check for it when
it is indeed mandatory.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 11a70ee18482..0152e4fa088c 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -680,6 +680,11 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
+	if (!client->irq) {
+		dev_err(&client->dev, "Keypad configured but no IRQ present\n");
+		return -EINVAL;
+	}
+
 	ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
 					     &kpad->cols);
 	if (ret)

-- 
2.46.1

[PATCH 4/4] Input: adp5588-keys: make use of dev_err_probe()

[Nuno Sa]

Simplify the probe error path by using dev_err_probe().

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 70 +++++++++++++++--------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 0152e4fa088c..60a7cb040af7 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -439,10 +439,9 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	kpad->gc.owner = THIS_MODULE;
 
 	if (device_property_present(dev, "interrupt-controller")) {
-		if (!kpad->client->irq) {
-			dev_err(dev, "Unable to serve as interrupt controller without interrupt");
-			return -EINVAL;
-		}
+		if (!kpad->client->irq)
+			return dev_err_probe(dev, -EINVAL,
+					     "Unable to serve as interrupt controller without interrupt");
 
 		girq = &kpad->gc.irq;
 		gpio_irq_chip_set_chip(girq, &adp5588_irq_chip);
@@ -453,10 +452,8 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	mutex_init(&kpad->gpio_lock);
 
 	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
-	if (error) {
-		dev_err(dev, "gpiochip_add failed: %d\n", error);
-		return error;
-	}
+	if (error)
+		return dev_err_probe(dev, error, "gpiochip_add failed\n");
 
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 		error = adp5588_read(kpad->client,
@@ -680,21 +677,19 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
-	if (!client->irq) {
-		dev_err(&client->dev, "Keypad configured but no IRQ present\n");
-		return -EINVAL;
-	}
+	if (!client->irq)
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Keypad configured but no IRQ present\n");
 
 	ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
 					     &kpad->cols);
 	if (ret)
 		return ret;
 
-	if (kpad->rows > ADP5588_ROWS_MAX || kpad->cols > ADP5588_COLS_MAX) {
-		dev_err(&client->dev, "Invalid nr of rows(%u) or cols(%u)\n",
-			kpad->rows, kpad->cols);
-		return -EINVAL;
-	}
+	if (kpad->rows > ADP5588_ROWS_MAX || kpad->cols > ADP5588_COLS_MAX)
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Invalid nr of rows(%u) or cols(%u)\n",
+				     kpad->rows, kpad->cols);
 
 	ret = matrix_keypad_build_keymap(NULL, NULL, kpad->rows, kpad->cols,
 					 kpad->keycode, kpad->input);
@@ -714,11 +709,10 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
-	if (kpad->nkeys_unlock > ARRAY_SIZE(kpad->unlock_keys)) {
-		dev_err(&client->dev, "number of unlock keys(%d) > (%zu)\n",
-			kpad->nkeys_unlock, ARRAY_SIZE(kpad->unlock_keys));
-		return -EINVAL;
-	}
+	if (kpad->nkeys_unlock > ARRAY_SIZE(kpad->unlock_keys))
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "number of unlock keys(%d) > (%zu)\n",
+				     kpad->nkeys_unlock, ARRAY_SIZE(kpad->unlock_keys));
 
 	ret = device_property_read_u32_array(&client->dev, "adi,unlock-keys",
 					     kpad->unlock_keys,
@@ -735,11 +729,10 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		 * part of keypad matrix to be used and if a reliable way of
 		 * using GPIs is found, this condition can be removed/lightened.
 		 */
-		if (kpad->unlock_keys[i] >= kpad->cols * kpad->rows) {
-			dev_err(&client->dev, "Invalid unlock key(%d)\n",
-				kpad->unlock_keys[i]);
-			return -EINVAL;
-		}
+		if (kpad->unlock_keys[i] >= kpad->cols * kpad->rows)
+			return dev_err_probe(&client->dev, -EINVAL,
+					     "Invalid unlock key(%d)\n",
+					     kpad->unlock_keys[i]);
 
 		/*
 		 * Firmware properties keys start from 0 but on the device they
@@ -761,10 +754,9 @@ static int adp5588_probe(struct i2c_client *client)
 	u8 id;
 
 	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_BYTE_DATA)) {
-		dev_err(&client->dev, "SMBUS Byte Data not Supported\n");
-		return -EIO;
-	}
+				     I2C_FUNC_SMBUS_BYTE_DATA))
+		return dev_err_probe(&client->dev, -EIO,
+				     "SMBUS Byte Data not Supported\n");
 
 	kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
 	if (!kpad)
@@ -814,11 +806,9 @@ static int adp5588_probe(struct i2c_client *client)
 	input->id.version = revid;
 
 	error = input_register_device(input);
-	if (error) {
-		dev_err(&client->dev, "unable to register input device: %d\n",
-			error);
-		return error;
-	}
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "unable to register input device\n");
 
 	error = adp5588_setup(kpad);
 	if (error)
@@ -833,11 +823,9 @@ static int adp5588_probe(struct i2c_client *client)
 						  adp5588_hard_irq, adp5588_thread_irq,
 						  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 						  client->dev.driver->name, kpad);
-		if (error) {
-			dev_err(&client->dev, "failed to request irq %d: %d\n",
-				client->irq, error);
-			return error;
-		}
+		if (error)
+			return dev_err_probe(&client->dev, error,
+					     "failed to request irq %d\n", client->irq);
 	}
 
 	dev_info(&client->dev, "Rev.%d controller\n", revid);

-- 
2.46.1

[PATCH 4/4] Input: adp5588-keys: make use of dev_err_probe()

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Simplify the probe error path by using dev_err_probe().

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/input/keyboard/adp5588-keys.c | 70 +++++++++++++++--------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index 0152e4fa088c..60a7cb040af7 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -439,10 +439,9 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	kpad->gc.owner = THIS_MODULE;
 
 	if (device_property_present(dev, "interrupt-controller")) {
-		if (!kpad->client->irq) {
-			dev_err(dev, "Unable to serve as interrupt controller without interrupt");
-			return -EINVAL;
-		}
+		if (!kpad->client->irq)
+			return dev_err_probe(dev, -EINVAL,
+					     "Unable to serve as interrupt controller without interrupt");
 
 		girq = &kpad->gc.irq;
 		gpio_irq_chip_set_chip(girq, &adp5588_irq_chip);
@@ -453,10 +452,8 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
 	mutex_init(&kpad->gpio_lock);
 
 	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
-	if (error) {
-		dev_err(dev, "gpiochip_add failed: %d\n", error);
-		return error;
-	}
+	if (error)
+		return dev_err_probe(dev, error, "gpiochip_add failed\n");
 
 	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
 		error = adp5588_read(kpad->client,
@@ -680,21 +677,19 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
-	if (!client->irq) {
-		dev_err(&client->dev, "Keypad configured but no IRQ present\n");
-		return -EINVAL;
-	}
+	if (!client->irq)
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Keypad configured but no IRQ present\n");
 
 	ret = matrix_keypad_parse_properties(&client->dev, &kpad->rows,
 					     &kpad->cols);
 	if (ret)
 		return ret;
 
-	if (kpad->rows > ADP5588_ROWS_MAX || kpad->cols > ADP5588_COLS_MAX) {
-		dev_err(&client->dev, "Invalid nr of rows(%u) or cols(%u)\n",
-			kpad->rows, kpad->cols);
-		return -EINVAL;
-	}
+	if (kpad->rows > ADP5588_ROWS_MAX || kpad->cols > ADP5588_COLS_MAX)
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Invalid nr of rows(%u) or cols(%u)\n",
+				     kpad->rows, kpad->cols);
 
 	ret = matrix_keypad_build_keymap(NULL, NULL, kpad->rows, kpad->cols,
 					 kpad->keycode, kpad->input);
@@ -714,11 +709,10 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		return 0;
 	}
 
-	if (kpad->nkeys_unlock > ARRAY_SIZE(kpad->unlock_keys)) {
-		dev_err(&client->dev, "number of unlock keys(%d) > (%zu)\n",
-			kpad->nkeys_unlock, ARRAY_SIZE(kpad->unlock_keys));
-		return -EINVAL;
-	}
+	if (kpad->nkeys_unlock > ARRAY_SIZE(kpad->unlock_keys))
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "number of unlock keys(%d) > (%zu)\n",
+				     kpad->nkeys_unlock, ARRAY_SIZE(kpad->unlock_keys));
 
 	ret = device_property_read_u32_array(&client->dev, "adi,unlock-keys",
 					     kpad->unlock_keys,
@@ -735,11 +729,10 @@ static int adp5588_fw_parse(struct adp5588_kpad *kpad)
 		 * part of keypad matrix to be used and if a reliable way of
 		 * using GPIs is found, this condition can be removed/lightened.
 		 */
-		if (kpad->unlock_keys[i] >= kpad->cols * kpad->rows) {
-			dev_err(&client->dev, "Invalid unlock key(%d)\n",
-				kpad->unlock_keys[i]);
-			return -EINVAL;
-		}
+		if (kpad->unlock_keys[i] >= kpad->cols * kpad->rows)
+			return dev_err_probe(&client->dev, -EINVAL,
+					     "Invalid unlock key(%d)\n",
+					     kpad->unlock_keys[i]);
 
 		/*
 		 * Firmware properties keys start from 0 but on the device they
@@ -761,10 +754,9 @@ static int adp5588_probe(struct i2c_client *client)
 	u8 id;
 
 	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_BYTE_DATA)) {
-		dev_err(&client->dev, "SMBUS Byte Data not Supported\n");
-		return -EIO;
-	}
+				     I2C_FUNC_SMBUS_BYTE_DATA))
+		return dev_err_probe(&client->dev, -EIO,
+				     "SMBUS Byte Data not Supported\n");
 
 	kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
 	if (!kpad)
@@ -814,11 +806,9 @@ static int adp5588_probe(struct i2c_client *client)
 	input->id.version = revid;
 
 	error = input_register_device(input);
-	if (error) {
-		dev_err(&client->dev, "unable to register input device: %d\n",
-			error);
-		return error;
-	}
+	if (error)
+		return dev_err_probe(&client->dev, error,
+				     "unable to register input device\n");
 
 	error = adp5588_setup(kpad);
 	if (error)
@@ -833,11 +823,9 @@ static int adp5588_probe(struct i2c_client *client)
 						  adp5588_hard_irq, adp5588_thread_irq,
 						  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 						  client->dev.driver->name, kpad);
-		if (error) {
-			dev_err(&client->dev, "failed to request irq %d: %d\n",
-				client->irq, error);
-			return error;
-		}
+		if (error)
+			return dev_err_probe(&client->dev, error,
+					     "failed to request irq %d\n", client->irq);
 	}
 
 	dev_info(&client->dev, "Rev.%d controller\n", revid);

-- 
2.46.1

Re: [PATCH 1/4] Input: adp5588-keys: bail on returned error

[Dmitry Torokhov]

Hi Nuno,

On Wed, Oct 02, 2024 at 12:51:50PM +0200, Nuno Sa via B4 Relay wrote:
> @@ -455,8 +457,16 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
>  	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
>  		kpad->dat_out[i] = adp5588_read(kpad->client,
>  						GPIO_DAT_OUT1 + i);
> +		if (kpad->dat_out[i] < 0)
> +			return kpad->dat_out[i];
> +
>  		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
> +		if (kpad->dir[i] < 0)
> +			return kpad->dir[i];
> +
>  		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
> +		if (kpad->pull_dis[i] < 0)
> +			return kpad->pull_dis[i];


Unfortunately all these are u8 so they will never be negative. You need
to do the adp5588_read() refactor first and then (or maybe together) add
error checking.

Thanks.

-- 
Dmitry

Re: [PATCH 1/4] Input: adp5588-keys: bail on returned error

[Nuno Sá]

On Wed, 2024-10-02 at 04:09 -0700, Dmitry Torokhov wrote:
> Hi Nuno,
> 
> On Wed, Oct 02, 2024 at 12:51:50PM +0200, Nuno Sa via B4 Relay wrote:
> > @@ -455,8 +457,16 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
> >  	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
> >  		kpad->dat_out[i] = adp5588_read(kpad->client,
> >  						GPIO_DAT_OUT1 + i);
> > +		if (kpad->dat_out[i] < 0)
> > +			return kpad->dat_out[i];
> > +
> >  		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
> > +		if (kpad->dir[i] < 0)
> > +			return kpad->dir[i];
> > +
> >  		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
> > +		if (kpad->pull_dis[i] < 0)
> > +			return kpad->pull_dis[i];
> 
> 
> Unfortunately all these are u8 so they will never be negative. You need
> to do the adp5588_read() refactor first and then (or maybe together) add
> error checking.
> 

Ahh crap... Completely missed that. Yeah, will see what looks better... Thanks for
catching this.

BTW, this is also wrong in the adp5589 series.

- Nuno Sá

Re: [PATCH 1/4] Input: adp5588-keys: bail on returned error

[Dmitry Torokhov]

On Wed, Oct 02, 2024 at 01:23:18PM +0200, Nuno Sá wrote:
> On Wed, 2024-10-02 at 04:09 -0700, Dmitry Torokhov wrote:
> > Hi Nuno,
> > 
> > On Wed, Oct 02, 2024 at 12:51:50PM +0200, Nuno Sa via B4 Relay wrote:
> > > @@ -455,8 +457,16 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
> > >  	for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
> > >  		kpad->dat_out[i] = adp5588_read(kpad->client,
> > >  						GPIO_DAT_OUT1 + i);
> > > +		if (kpad->dat_out[i] < 0)
> > > +			return kpad->dat_out[i];
> > > +
> > >  		kpad->dir[i] = adp5588_read(kpad->client, GPIO_DIR1 + i);
> > > +		if (kpad->dir[i] < 0)
> > > +			return kpad->dir[i];
> > > +
> > >  		kpad->pull_dis[i] = adp5588_read(kpad->client, GPIO_PULL1 + i);
> > > +		if (kpad->pull_dis[i] < 0)
> > > +			return kpad->pull_dis[i];
> > 
> > 
> > Unfortunately all these are u8 so they will never be negative. You need
> > to do the adp5588_read() refactor first and then (or maybe together) add
> > error checking.
> > 
> 
> Ahh crap... Completely missed that. Yeah, will see what looks better... Thanks for
> catching this.
> 
> BTW, this is also wrong in the adp5589 series.

I didn't get that far there ;)

Thanks.

-- 
Dmitry