Date:   Tue, 19 Oct 2021 12:19:45 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Meng Li <Meng.Li@...driver.com>
Cc:     Magnus Damm <magnus.damm@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Marek Vasut <marek.vasut+renesas@...il.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-pci <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] pci: pcie-rcar: add regulators support

Hi Meng,

On Tue, Oct 19, 2021 at 11:59 AM Meng Li <Meng.Li@...driver.com> wrote:
> From: Andrey Gusakov <andrey.gusakov@...entembedded.com>
>
> Add PCIe regulators for KingFisher board.
>
> Signed-off-by: Meng Li <Meng.Li@...driver.com>

Thanks for your patch!

>  arch/arm64/boot/dts/renesas/ulcb-kf.dtsi | 47 +++++++++++++++++
>  drivers/pci/controller/pcie-rcar-host.c  | 64 ++++++++++++++++++++++++

Please split patches touching both DT and driver sources.

> --- a/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
> +++ b/arch/arm64/boot/dts/renesas/ulcb-kf.dtsi

> @@ -259,6 +303,9 @@
>
>  &pciec1 {
>         status = "okay";
> +
> +       pcie3v3-supply = <&mpcie_3v3>;
> +       pcie1v8-supply = <&mpcie_1v8>;

This needs an update to the R-Car PCIe DT bindings first.

> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c

> @@ -893,6 +896,36 @@ static const struct of_device_id rcar_pcie_of_match[] = {
>         {},
>  };
>
> +static int rcar_pcie_set_vpcie(struct rcar_pcie_host *host)
> +{
> +       struct device *dev = host->pcie.dev;
> +       int err;
> +
> +       if (!IS_ERR(host->pcie3v3)) {
> +               err = regulator_enable(host->pcie3v3);

This will crash if host->pcie3v3 is NULL (optional regulator not
present).  Probably you just want to check for non-NULL (see below).

> +               if (err) {
> +                       dev_err(dev, "fail to enable vpcie3v3 regulator\n");
> +                       goto err_out;
> +               }
> +       }
> +
> +       if (!IS_ERR(host->pcie1v8)) {
> +               err = regulator_enable(host->pcie1v8);

Likewise.

> +               if (err) {
> +                       dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> +                       goto err_disable_3v3;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_disable_3v3:
> +       if (!IS_ERR(host->pcie3v3))

Likewise.

> +               regulator_disable(host->pcie3v3);
> +err_out:
> +       return err;
> +}
> +
>  static int rcar_pcie_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -911,6 +944,31 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>         pcie->dev = dev;
>         platform_set_drvdata(pdev, host);
>
> +       host->pcie3v3 = devm_regulator_get_optional(dev, "pcie3v3");
> +       if (IS_ERR(host->pcie3v3)) {
> +               if (PTR_ERR(host->pcie3v3) == -EPROBE_DEFER) {
> +                       pci_free_host_bridge(bridge);

Please drop this.  As the bridge was allocated using
devm_pci_alloc_host_bridge(), freeing it manually will cause a
double free.

> +                       return -EPROBE_DEFER;
> +               }
> +               dev_info(dev, "no pcie3v3 regulator found\n");

devm_regulator_get_optional() returns NULL if the regulator was not
found.  Hence if IS_ERR() is true, this indicates a real error, which
you should handle:

    if (IS_ERR(host->pcie3v3))
            return PTR_ERR(host->pcie3v3);

> +       }
> +
> +       host->pcie1v8 = devm_regulator_get_optional(dev, "pcie1v8");
> +       if (IS_ERR(host->pcie1v8)) {
> +               if (PTR_ERR(host->pcie1v8) == -EPROBE_DEFER) {
> +                       pci_free_host_bridge(bridge);
> +                       return -EPROBE_DEFER;
> +               }
> +               dev_info(dev, "no pcie1v8 regulator found\n");

Likewise.

> +       }
> +
> +       err = rcar_pcie_set_vpcie(host);
> +       if (err) {
> +               dev_err(dev, "failed to set pcie regulators\n");
> +               pci_free_host_bridge(bridge);

Please drop this to avoid double free.

> +               return err;
> +       }
> +
>         pm_runtime_enable(pcie->dev);
>         err = pm_runtime_get_sync(pcie->dev);
>         if (err < 0) {
> @@ -985,6 +1043,12 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>         irq_dispose_mapping(host->msi.irq1);
>
>  err_pm_put:
> +       if(!IS_ERR(host->pcie3v3))

if (host->pcie3v3)

> +               if (regulator_is_enabled(host->pcie3v3))

If you get here, the regulator should be enabled?

> +                       regulator_disable(host->pcie3v3);
> +       if(!IS_ERR(host->pcie1v8))

if (host->pcie1v8)

> +               if (regulator_is_enabled(host->pcie1v8))
> +                       regulator_disable(host->pcie1v8);

Please move this below the call to pm_runtime_disable(), to preserve
symmetry.

>         pm_runtime_put(dev);
>         pm_runtime_disable(dev);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists