RE: [PATCH v2] gpio: pca953x: do not enable regmap cache when there is no regulator
From: Bough Chen
Date: Thu Nov 28 2024 - 21:26:25 EST
> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: 2024年11月28日 20:28
> To: Bough Chen <haibo.chen@xxxxxxx>
> Cc: linus.walleij@xxxxxxxxxx; brgl@xxxxxxxx; lgirdwood@xxxxxxxxx;
> marek.vasut@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> 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@xxxxxxx 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