* [PATCH 0/4] Input: adp5588-keys: refactor adp5588_read()
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
0 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa via B4 Relay @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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á
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/4] Input: adp5588-keys: bail on returned error
2024-10-02 10:51 ` Nuno Sa via B4 Relay
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
-1 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 1/4] Input: adp5588-keys: bail on returned error
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
0 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa via B4 Relay @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] Input: adp5588-keys: bail on returned error
2024-10-02 10:51 ` Nuno Sa via B4 Relay
(?)
@ 2024-10-02 11:09 ` Dmitry Torokhov
2024-10-02 11:23 ` Nuno Sá
-1 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2024-10-02 11:09 UTC (permalink / raw)
To: nuno.sa; +Cc: Michael Hennerich, linux-input
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] Input: adp5588-keys: bail on returned error
2024-10-02 11:09 ` Dmitry Torokhov
@ 2024-10-02 11:23 ` Nuno Sá
2024-10-02 11:28 ` Dmitry Torokhov
0 siblings, 1 reply; 13+ messages in thread
From: Nuno Sá @ 2024-10-02 11:23 UTC (permalink / raw)
To: Dmitry Torokhov, nuno.sa; +Cc: Michael Hennerich, linux-input
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á
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] Input: adp5588-keys: bail on returned error
2024-10-02 11:23 ` Nuno Sá
@ 2024-10-02 11:28 ` Dmitry Torokhov
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2024-10-02 11:28 UTC (permalink / raw)
To: Nuno Sá; +Cc: nuno.sa, Michael Hennerich, linux-input
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] Input: adp5588-keys: refactor adp5589_read()
2024-10-02 10:51 ` Nuno Sa via B4 Relay
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
-1 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/4] Input: adp5588-keys: refactor adp5589_read()
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
0 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa via B4 Relay @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] Input: adp5588-keys: error out if no IRQ is given
2024-10-02 10:51 ` Nuno Sa via B4 Relay
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
-1 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/4] Input: adp5588-keys: error out if no IRQ is given
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
0 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa via B4 Relay @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] Input: adp5588-keys: make use of dev_err_probe()
2024-10-02 10:51 ` Nuno Sa via B4 Relay
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
-1 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/4] Input: adp5588-keys: make use of dev_err_probe()
@ 2024-10-02 10:51 ` Nuno Sa via B4 Relay
0 siblings, 0 replies; 13+ messages in thread
From: Nuno Sa via B4 Relay @ 2024-10-02 10:51 UTC (permalink / raw)
To: Michael Hennerich, Dmitry Torokhov; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 13+ messages in thread