diff mbox series

[v2] gpio: pca953x: do not enable regmap cache when there is no regulator

Message ID 20241128020042.3124957-1-haibo.chen@nxp.com (mailing list archive)
State New
Headers show
Series [v2] gpio: pca953x: do not enable regmap cache when there is no regulator | expand

Commit Message

Bough Chen Nov. 28, 2024, 2 a.m. UTC 
From: Haibo Chen <haibo.chen@nxp.com>

Regmap cache mechanism is enabled in default. Thus, IO expander wouldn't
handle GPIO set really before resuming back.

But there are cases need to toggle gpio in NO_IRQ stage.
e.g. To align with PCIe specification, PERST# signal connected on the IO
expander must be toggled during PCIe RC's NO_IRQ_RESUME.

Do not enable the regmap cache when IO expander doesn't have the regulator
during system PM. That means the power of IO expander would be kept on,
and the GPIOs of the IO expander can be toggled really during system PM.

Fixes: b76574300504 ("gpio: pca953x: Restore registers after suspend/resume cycle")
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
Changes for V2:
  correct the commit head, and add fix tag
  fix some typo in the code comment

---
 drivers/gpio/gpio-pca953x.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

Comments

Mark Brown Nov. 28, 2024, 12:28 p.m. UTC | #1 
On Thu, Nov 28, 2024 at 10:00:42AM +0800, haibo.chen@nxp.com wrote:

> Regmap cache mechanism is enabled in default. Thus, IO expander wouldn't
> handle GPIO set really before resuming back.

> But there are cases need to toggle gpio in NO_IRQ stage.
> e.g. To align with PCIe specification, PERST# signal connected on the IO
> expander must be toggled during PCIe RC's NO_IRQ_RESUME.

> Do not enable the regmap cache when IO expander doesn't have the regulator
> during system PM. That means the power of IO expander would be kept on,
> and the GPIOs of the IO expander can be toggled really during system PM.

Aside from the fact that the device will always have a regulator (power
isn't optional) this is obviously not related to any sequencing needs
that you have on resume.  It might happen to work on your system, but it
will potentially break other systems which do actually need the
registers restoring but don't have a regulator explicitly defined and
will fail to do anything on a system with ordering requirements that do
have one defined.  The fix for this is to make this driver resume early
if it's needed by other things that run in the resume sequence.

> -	reg = devm_regulator_get(dev, "vcc");
> -	if (IS_ERR(reg))
> -		return dev_err_probe(dev, PTR_ERR(reg), "reg get err\n");
> +	reg = devm_regulator_get_optional(dev, "vcc");
> +	if (IS_ERR(reg)) {

This is obviously buggy, the main supply for the device is not going to
be optional.
Bough Chen Nov. 29, 2024, 2:26 a.m. UTC | #2 
> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: 2024年11月28日 20:28
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: linus.walleij@linaro.org; brgl@bgdev.pl; lgirdwood@gmail.com;
> marek.vasut@gmail.com; linux-gpio@vger.kernel.org;
> linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v2] gpio: pca953x: do not enable regmap cache when there
> is no regulator
> 
> On Thu, Nov 28, 2024 at 10:00:42AM +0800, haibo.chen@nxp.com wrote:
> 
> > Regmap cache mechanism is enabled in default. Thus, IO expander
> > wouldn't handle GPIO set really before resuming back.
> 
> > But there are cases need to toggle gpio in NO_IRQ stage.
> > e.g. To align with PCIe specification, PERST# signal connected on the
> > IO expander must be toggled during PCIe RC's NO_IRQ_RESUME.
> 
> > Do not enable the regmap cache when IO expander doesn't have the
> > regulator during system PM. That means the power of IO expander would
> > be kept on, and the GPIOs of the IO expander can be toggled really during
> system PM.
> 
> Aside from the fact that the device will always have a regulator (power isn't
> optional) this is obviously not related to any sequencing needs that you have on
> resume.  It might happen to work on your system, but it will potentially break
> other systems which do actually need the registers restoring but don't have a
> regulator explicitly defined and will fail to do anything on a system with ordering

Yes, you are right. I did not take this case into consideration.

> requirements that do have one defined.  The fix for this is to make this driver
> resume early if it's needed by other things that run in the resume sequence.

I once thought to move current system PM to NOIRQ PM, but seems not all i2c bus controller support i2c operation during NOIRQ PM.
Let me think whether there is a better solution, or do you have any suggestion?
 
> 
> > -	reg = devm_regulator_get(dev, "vcc");
> > -	if (IS_ERR(reg))
> > -		return dev_err_probe(dev, PTR_ERR(reg), "reg get err\n");
> > +	reg = devm_regulator_get_optional(dev, "vcc");
> > +	if (IS_ERR(reg)) {
> 
> This is obviously buggy, the main supply for the device is not going to be optional.

According to Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml, the vcc regulator is optional.

Regards
Haibo Chen
Mark Brown Dec. 2, 2024, 1:19 p.m. UTC | #3 
On Fri, Nov 29, 2024 at 02:26:13AM +0000, Bough Chen wrote:

> I once thought to move current system PM to NOIRQ PM, but seems not all i2c bus controller support i2c operation during NOIRQ PM.
> Let me think whether there is a better solution, or do you have any suggestion?

I'm not sure we have any better option than also moving the I2C
controllers TBH.
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 272febc3230e..2ef1db58a6a1 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1040,9 +1040,15 @@  static int pca953x_get_and_enable_regulator(struct pca953x_chip *chip)
 	struct regulator *reg = chip->regulator;
 	int ret;
 
-	reg = devm_regulator_get(dev, "vcc");
-	if (IS_ERR(reg))
-		return dev_err_probe(dev, PTR_ERR(reg), "reg get err\n");
+	reg = devm_regulator_get_optional(dev, "vcc");
+	if (IS_ERR(reg)) {
+		if (PTR_ERR(reg) == -ENODEV) {
+			chip->regulator = NULL;
+			return 0;
+		} else {
+			return dev_err_probe(dev, PTR_ERR(reg), "reg get err\n");
+		}
+	}
 
 	ret = regulator_enable(reg);
 	if (ret)
@@ -1240,11 +1246,20 @@  static int pca953x_suspend(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 
-	pca953x_save_context(chip);
+	/*
+	 * Only save context when regulator is exist,
+	 * if no regulator, power keeps on, can also
+	 * handle gpio related operation.
+	 * e.g. PCIe RC needs to toggle the RST pin in
+	 * NOIRQ resume stage, so can't open regmap
+	 * cache if there is no regulator.
+	 */
+	if (chip->regulator)
+		pca953x_save_context(chip);
 
 	if (atomic_read(&chip->wakeup_path))
 		device_set_wakeup_path(dev);
-	else
+	else if (chip->regulator)
 		regulator_disable(chip->regulator);
 
 	return 0;
@@ -1255,7 +1270,7 @@  static int pca953x_resume(struct device *dev)
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 	int ret;
 
-	if (!atomic_read(&chip->wakeup_path)) {
+	if (!atomic_read(&chip->wakeup_path) && chip->regulator) {
 		ret = regulator_enable(chip->regulator);
 		if (ret) {
 			dev_err(dev, "Failed to enable regulator: %d\n", ret);
@@ -1263,9 +1278,11 @@  static int pca953x_resume(struct device *dev)
 		}
 	}
 
-	ret = pca953x_restore_context(chip);
-	if (ret)
-		dev_err(dev, "Failed to restore register map: %d\n", ret);
+	if (chip->regulator) {
+		ret = pca953x_restore_context(chip);
+		if (ret)
+			dev_err(dev, "Failed to restore register map: %d\n", ret);
+	}
 
 	return ret;
 }