Re: [PATCH v3 3/7] PCI: imx6: Fix the regulator dump when link never came up

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

On Tue, Oct 26, 2021 at 02:18:18AM +0000, Richard Zhu wrote:

> > I should probably also say that the check for the regulator looks buggy as well,
> > regulators should only be optional if they can be physically absent which does
> > not seem likely for PCI devices.  If the driver is not doing something to
> > reconfigure the hardware to account for a missing supply this is generally a big
> > warning sign.
> > 
> > I really don't understand why regulator support is so frequently problematic
> > for PCI controllers.  :(

> [Richard Zhu] Hi Mark:
> The _enabled check is used because that this regulator is optional in the HW design.
> To make the codes clean and aligned on different HW boards, the _enabled check is added.

I would be really surprised to see PCI hardware that was able to support
a supply being physically absent, and this use of _is_enabled() is quite
simply not how any of this is supposed to work in the regulator API even
for regulators that can be optional.

> The root cause is that the error return is not handled properly by the controller driver.
> I.MX PCIe controller doesn't support the Hot-Plug, and it would return -110 error
> when PCIe link never came up. Thus, the _probe would be failed in the end.
> The clocks/regulator usage balance should be considered by i.MX PCIe controller, that's all.
> It's not a general case, and the problem is not caused by the regulator support.

Perhaps it's not causing problems in this design but if the supply is
ever shared with anything else then the software will run into trouble.
There will also be problems with the error handling on a system where
the regulator needs to be controlled.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]