| Message ID | 20220708093448.42617-4-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adp5588-keys refactor and fw properties support | expand |
On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote: > > Don't continue in code paths after some error is found. It makes no > sense to do any other device configuration if a previous one failed. ... > for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) { > int pull_mask = gpio_data->pullup_dis_mask; > > - ret |= adp5588_write(client, GPIO_PULL1 + i, > + ret = adp5588_write(client, GPIO_PULL1 + i, > (pull_mask >> (8 * i)) & 0xFF); > + if (ret) > + return ret; > } Looks like a good candidate for bitmap_get_value8(pull_mask).
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Friday, July 8, 2022 4:25 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: devicetree <devicetree@vger.kernel.org>; open list:GPIO > SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-input <linux- > input@vger.kernel.org>; Dmitry Torokhov > <dmitry.torokhov@gmail.com>; Bartosz Golaszewski > <brgl@bgdev.pl>; Hennerich, Michael > <Michael.Hennerich@analog.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Linus Walleij > <linus.walleij@linaro.org> > Subject: Re: [PATCH 03/10] input: keyboard: adp5588-keys: bail out on > returned error > > [External] > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote: > > > > Don't continue in code paths after some error is found. It makes no > > sense to do any other device configuration if a previous one failed. > > ... > > > for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) { > > int pull_mask = gpio_data->pullup_dis_mask; > > > > - ret |= adp5588_write(client, GPIO_PULL1 + i, > > + ret = adp5588_write(client, GPIO_PULL1 + i, > > (pull_mask >> (8 * i)) & 0xFF); > > + if (ret) > > + return ret; > > } > > Looks like a good candidate for bitmap_get_value8(pull_mask). > I'm not touching the original way the driver was handling this kind of stuff. I do have in my mind to just convert this driver to use regmap and with it (by using the *bits functions) we can get rid of most of the "plain" bitmaps in the driver. - Nuno Sá
On Fri, Jul 8, 2022 at 4:35 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Friday, July 8, 2022 4:25 PM > > On Fri, Jul 8, 2022 at 11:36 AM Nuno Sá <nuno.sa@analog.com> wrote: ... > > Looks like a good candidate for bitmap_get_value8(pull_mask). > > I'm not touching the original way the driver was handling this > kind of stuff. I do have in my mind to just convert this driver to use > regmap and with it (by using the *bits functions) we can get rid of > most of the "plain" bitmaps in the driver. Works for me!
diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c index dad8862f6c76..10f24283d7a3 100644 --- a/drivers/input/keyboard/adp5588-keys.c +++ b/drivers/input/keyboard/adp5588-keys.c @@ -146,9 +146,13 @@ static int adp5588_gpio_direction_output(struct gpio_chip *chip, ret = adp5588_write(kpad->client, GPIO_DAT_OUT1 + bank, kpad->dat_out[bank]); - ret |= adp5588_write(kpad->client, GPIO_DIR1 + bank, + if (ret) + goto out_unlock; + + ret = adp5588_write(kpad->client, GPIO_DIR1 + bank, kpad->dir[bank]); +out_unlock: mutex_unlock(&kpad->gpio_lock); return ret; @@ -439,42 +443,58 @@ static int adp5588_setup(struct i2c_client *client) int i, ret; ret = adp5588_write(client, KP_GPIO1, KP_SEL(pdata->rows)); - ret |= adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF); - ret |= adp5588_write(client, KP_GPIO3, KP_SEL(pdata->cols) >> 8); + if (ret) + return ret; + + ret = adp5588_write(client, KP_GPIO2, KP_SEL(pdata->cols) & 0xFF); + if (ret) + return ret; + + ret = adp5588_write(client, KP_GPIO3, KP_SEL(pdata->cols) >> 8); + if (ret) + return ret; if (pdata->en_keylock) { - ret |= adp5588_write(client, UNLOCK1, pdata->unlock_key1); - ret |= adp5588_write(client, UNLOCK2, pdata->unlock_key2); - ret |= adp5588_write(client, KEY_LCK_EC_STAT, ADP5588_K_LCK_EN); + ret = adp5588_write(client, UNLOCK1, pdata->unlock_key1); + if (ret) + return ret; + + ret = adp5588_write(client, UNLOCK2, pdata->unlock_key2); + if (ret) + return ret; + + ret = adp5588_write(client, KEY_LCK_EC_STAT, ADP5588_K_LCK_EN); + if (ret) + return ret; } - for (i = 0; i < KEYP_MAX_EVENT; i++) - ret |= adp5588_read(client, Key_EVENTA); + for (i = 0; i < KEYP_MAX_EVENT; i++) { + ret = adp5588_read(client, Key_EVENTA); + if (ret) + return ret; + } if (gpio_data) { for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) { int pull_mask = gpio_data->pullup_dis_mask; - ret |= adp5588_write(client, GPIO_PULL1 + i, + ret = adp5588_write(client, GPIO_PULL1 + i, (pull_mask >> (8 * i)) & 0xFF); + if (ret) + return ret; } } - ret |= adp5588_write(client, INT_STAT, + ret = adp5588_write(client, INT_STAT, ADP5588_CMP2_INT | ADP5588_CMP1_INT | ADP5588_OVR_FLOW_INT | ADP5588_K_LCK_INT | ADP5588_GPI_INT | ADP5588_KE_INT); /* Status is W1C */ + if (ret) + return ret; - ret |= adp5588_write(client, CFG, ADP5588_INT_CFG | + return adp5588_write(client, CFG, ADP5588_INT_CFG | ADP5588_OVR_FLOW_IEN | ADP5588_KE_IEN); - - if (ret < 0) { - dev_err(&client->dev, "Write Error\n"); - return ret; - } - - return 0; } static int adp5588_probe(struct i2c_client *client,
Don't continue in code paths after some error is found. It makes no sense to do any other device configuration if a previous one failed. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/input/keyboard/adp5588-keys.c | 56 ++++++++++++++++++--------- 1 file changed, 38 insertions(+), 18 deletions(-)