On Fri, Oct 04, 2024 at 02:52:27PM GMT, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> Old device trees for some platforms already define wifi nodes for the WCN
> family of chips since before power sequencing was added upstream.
>
> These nodes don't consume the regulator outputs from the PMU and if we
> allow this driver to bind to one of such "incomplete" nodes, we'll see
> a kernel log error about the indefinite probe deferral.
>
> Let's check the existence of the regulator supply that exists on all WCN
> models before moving forward.
>
> Fixes: 6140d185a43d ("PCI/pwrctl: Add a PCI power control driver for power sequenced devices")
> Reported-by: Johan Hovold <johan@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/Zv565olMDDGHyYVt@xxxxxxxxxxxxxxxxxxxx/
Opens-up: A whole bunch of new issues
> Suggested-by: Stephan Gerhold <stephan.gerhold@xxxxxxxxxx>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
> drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> index a23a4312574b..8ed613655d4a 100644
> --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> @@ -9,6 +9,7 @@
> #include <linux/of.h>
> #include <linux/pci-pwrctl.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/pwrseq/consumer.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -31,6 +32,25 @@ static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> int ret;
>
> + /*
> + * Old device trees for some platforms already define wifi nodes for
> + * the WCN family of chips since before power sequencing was added
> + * upstream.
> + *
> + * These nodes don't consume the regulator outputs from the PMU and
> + * if we allow this driver to bind to one of such "incomplete" nodes,
> + * we'll see a kernel log error about the indefinite probe deferral.
> + *
> + * Let's check the existence of the regulator supply that exists on all
> + * WCN models before moving forward.
> + *
> + * NOTE: If this driver is ever used to support a device other than
> + * a WCN chip, the following lines should become conditional and depend
> + * on the compatible string.
What do you mean "is ever used ... other than WCN chip"?
This driver and the power sequence framework was presented as a
completely generic solution to solve all kinds of PCI power sequence
problems - upon which the WCN case was built.
In fact, if I read this correctly, the second user of the power sequence
implementation (the QPS615, proposed in [1]), would break if this check
is added.
Add to this that your colleagues are pushing people to implement simple
power supplies for M.2-connected devices into this framework - which I
can only assume would trip on this as well (the one supply pin in a M.2.
connector isn't named "vddaon").
[1] https://lore.kernel.org/all/20240803-qps615-v2-3-9560b7c71369@xxxxxxxxxxx/
Regards,
Bjorn
> + */
> + if (!device_property_present(dev, "vddaon-supply"))
> + return -ENODEV;
> +
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
> --
> 2.43.0
>
