| Message ID | 20240920-fix-adp5588-err-check-v1-1-81f6e957ef24@analog.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Input: adp5588-keys: fix check on return code | expand |
On Fri, Sep 20, 2024 at 09:22:52AM +0200, Nuno Sa via B4 Relay wrote: > From: Nuno Sa <nuno.sa@analog.com> > > During adp5588_setup(), we read all the events to clear the event FIFO. > However, adp5588_read() just calls i2c_smbus_read_byte_data() which > returns the byte read in case everything goes well. Hence, we need to > explicitly check for a negative error code instead of checking for > something different than 0. > > Fixes: e960309ce318 ("Input: adp5588-keys - bail out on returned error") > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/input/keyboard/adp5588-keys.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c > index b5f4becf5cb6f..d25d63a807f23 100644 > --- a/drivers/input/keyboard/adp5588-keys.c > +++ b/drivers/input/keyboard/adp5588-keys.c > @@ -620,7 +620,7 @@ static int adp5588_setup(struct adp5588_kpad *kpad) > > for (i = 0; i < KEYP_MAX_EVENT; i++) { > ret = adp5588_read(client, KEY_EVENTA); > - if (ret) > + if (ret < 0) > return ret; > } Hmm... There are a bunch of places where we do not check result of adp5588_read(). I wonder if we should: 1. add the checks 2. change it to return error or 0 and return the value through a pointer argument. In the meantime I'll apply this patch. Thanks.
On Fri, 2024-09-20 at 08:17 +0000, Dmitry Torokhov wrote: > On Fri, Sep 20, 2024 at 09:22:52AM +0200, Nuno Sa via B4 Relay wrote: > > From: Nuno Sa <nuno.sa@analog.com> > > > > During adp5588_setup(), we read all the events to clear the event FIFO. > > However, adp5588_read() just calls i2c_smbus_read_byte_data() which > > returns the byte read in case everything goes well. Hence, we need to > > explicitly check for a negative error code instead of checking for > > something different than 0. > > > > Fixes: e960309ce318 ("Input: adp5588-keys - bail out on returned error") > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > drivers/input/keyboard/adp5588-keys.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/input/keyboard/adp5588-keys.c > > b/drivers/input/keyboard/adp5588-keys.c > > index b5f4becf5cb6f..d25d63a807f23 100644 > > --- a/drivers/input/keyboard/adp5588-keys.c > > +++ b/drivers/input/keyboard/adp5588-keys.c > > @@ -620,7 +620,7 @@ static int adp5588_setup(struct adp5588_kpad *kpad) > > > > for (i = 0; i < KEYP_MAX_EVENT; i++) { > > ret = adp5588_read(client, KEY_EVENTA); > > - if (ret) > > + if (ret < 0) > > return ret; > > } > > Hmm... There are a bunch of places where we do not check result of > adp5588_read(). I wonder if we should: > > 1. add the checks > 2. change it to return error or 0 and return the value through a pointer > argument. It does make sense. I can take care of that. And similar work will be needed in the adp5589 driver. I'll include it in the series I'm preparing for the FW properties. - Nuno Sá
diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c index b5f4becf5cb6f..d25d63a807f23 100644 --- a/drivers/input/keyboard/adp5588-keys.c +++ b/drivers/input/keyboard/adp5588-keys.c @@ -620,7 +620,7 @@ static int adp5588_setup(struct adp5588_kpad *kpad) for (i = 0; i < KEYP_MAX_EVENT; i++) { ret = adp5588_read(client, KEY_EVENTA); - if (ret) + if (ret < 0) return ret; }