| Message ID | 20220307141955.28040-1-alifer.m@variscite.com |
|---|---|
| State | New |
| Headers | show |
| Series | driver: pca953x: avoid error message when resuming | expand |
Hi All, > > From: FrancescoFerraro <francesco.f@variscite.com> > > > > 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 <pierluigi.p@variscite.com> 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.
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