Re: [PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

[Bjorn Andersson]

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
>