From: FrancescoFerraro <[email protected]>
Avoids the error messages "pca953x 1-0020: failed reading register"
when resuming from suspend using gpio-key attached to pca9534.
Signed-off-by: Francesco Ferraro <[email protected]>
Signed-off-by: Alifer Moraes <[email protected]>
---
drivers/gpio/gpio-pca953x.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index d2fe76f3f34f..4f35b75dcbb1 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -211,6 +211,7 @@ struct pca953x_chip {
struct regulator *regulator;
const struct pca953x_reg_config *regs;
+ int is_in_suspend;
};
static int pca953x_bank_shift(struct pca953x_chip *chip)
@@ -412,7 +413,8 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *
ret = regmap_bulk_read(chip->regmap, regaddr, value, NBANK(chip));
if (ret < 0) {
- dev_err(&chip->client->dev, "failed reading register\n");
+ if (!chip->is_in_suspend)
+ dev_err(&chip->client->dev, "failed reading register\n");
return ret;
}
@@ -954,6 +956,7 @@ static int pca953x_probe(struct i2c_client *client,
chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL)
return -ENOMEM;
+ chip->is_in_suspend = 0;
pdata = dev_get_platdata(&client->dev);
if (pdata) {
@@ -1161,6 +1164,8 @@ static int pca953x_suspend(struct device *dev)
else
regulator_disable(chip->regulator);
+ chip->is_in_suspend = 1;
+
return 0;
}
@@ -1189,6 +1194,8 @@ static int pca953x_resume(struct device *dev)
return ret;
}
+ chip->is_in_suspend = 0;
+
return 0;
}
#endif
--
2.25.1
On Mon, Mar 7, 2022 at 5:04 PM Alifer Moraes <[email protected]> wrote:
>
> From: FrancescoFerraro <[email protected]>
>
> Avoids the error messages "pca953x 1-0020: failed reading register"
> when resuming from suspend using gpio-key attached to pca9534.
Thanks for your report and fix. My comments below.
First of all, how many of them do you get and why is it a problem?
...
> const struct pca953x_reg_config *regs;
> + int is_in_suspend;
Usually we call it is_suspended or so, check existing code by `git
grep ...`. And it can be boolean.
...
> ret = regmap_bulk_read(chip->regmap, regaddr, value, NBANK(chip));
> if (ret < 0) {
> - dev_err(&chip->client->dev, "failed reading register\n");
> + if (!chip->is_in_suspend)
> + dev_err(&chip->client->dev, "failed reading register\n");
Hmm... Maybe we can simply move it to debug level?
> return ret;
> }
...
> chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> if (chip == NULL)
> return -ENOMEM;
> + chip->is_in_suspend = 0;
Redundant change.
--
With Best Regards,
Andy Shevchenko
Hi All,
> > From: FrancescoFerraro <[email protected]>
> >
> > Avoids the error messages "pca953x 1-0020: failed reading register"
> > when resuming from suspend using gpio-key attached to pca9534.
> Thanks for your report and fix. My comments below.
> First of all, how many of them do you get and why is it a problem?
> ...
The number of occurrences depends on the time required to I2C bus to fully wake-up.
It's not a real problem, but the message may lead to think about a real I2C problem.
> >???????? const struct pca953x_reg_config *regs;
> > +?????? int is_in_suspend;
> Usually we call it is_suspended or so, check existing code by `git
> grep ...`. And it can be boolean.
> ...
Do you mean something like in drivers/gpio/gpio-omap.c ?
> >???????? ret = regmap_bulk_read(chip->regmap, regaddr, value, NBANK(chip));
> >???????? if (ret < 0) {
> > -?????????????? dev_err(&chip->client->dev, "failed reading register\n");
> > +?????????????? if (!chip->is_in_suspend)
> > +?????????????????????? dev_err(&chip->client->dev, "failed reading register\n");
> Hmm... Maybe we can simply move it to debug level?
On the other side, this can be a serious problem, so I'm not sure the level should be changed.
> >???????????????? return ret;
> >???????? }
> ...
> >???????? chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> >???????? if (chip == NULL)
> >???????????????? return -ENOMEM;
> > +?????? chip->is_in_suspend = 0;
> Redundant change.
> --
> With Best Regards,
> Andy Shevchenko
Thanks
Best Regards
Pier
On Mon, Jun 20, 2022 at 4:18 PM Pierluigi Passaro
<[email protected]> wrote:
...
> > > Avoids the error messages "pca953x 1-0020: failed reading register"
> > > when resuming from suspend using gpio-key attached to pca9534.
> > Thanks for your report and fix. My comments below.
> > First of all, how many of them do you get and why is it a problem?
>
> The number of occurrences depends on the time required to I2C bus to fully wake-up.
> It's not a real problem, but the message may lead to think about a real I2C problem.
Wolfram, do we have any mechanisms that guarantees that I2C traffic is
not going on a semi-woken up host controller?
Writing this, I'm in doubt this patch is a fix we want. Wouldn't it
just hide the real issue with some resume ordering?
...
> > > + int is_in_suspend;
> > Usually we call it is_suspended or so, check existing code by `git
> > grep ...`. And it can be boolean.
>
> Do you mean soomething like in drivers/gpio/gpio-omap.c ?
I believe almost any from the list `git grep -nl -w is_suspended` will suffice.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 20, 2022 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Jun 20, 2022 at 4:18 PM Pierluigi Passaro
> <[email protected]> wrote:
> > > > Avoids the error messages "pca953x 1-0020: failed reading register"
> > > > when resuming from suspend using gpio-key attached to pca9534.
> > > Thanks for your report and fix. My comments below.
> > > First of all, how many of them do you get and why is it a problem?
> >
> > The number of occurrences depends on the time required to I2C bus to fully wake-up.
> > It's not a real problem, but the message may lead to think about a real I2C problem.
>
> Wolfram, do we have any mechanisms that guarantees that I2C traffic is
> not going on a semi-woken up host controller?
>
> Writing this, I'm in doubt this patch is a fix we want. Wouldn't it
> just hide the real issue with some resume ordering?
That sounds like a bug in the pm routines in the I2C driver. Why is it
returning from [runtime_]resume() if it is not properly resumed?
Yours,
Linus Walleij