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

[Bough Chen <haibo.chen@xxxxxxx>]

> -----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