From: Bjorn Helgaas <helgaas@kernel.org>
To: Simon Xue <xxm@rock-chips.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
linux-pci@vger.kernel.org, linux-rockchip@lists.infradead.org,
devicetree@vger.kernel.org, robh+dt@kernel.org,
Johan Jonker <jbx6244@gmail.com>,
Heiko Stuebner <heiko@sntech.de>,
Shawn Lin <shawn.lin@rock-chips.com>
Subject: Re: [PATCH v5 2/2] PCI: rockchip: Add DesignWare based PCIe controller
Date: Wed, 24 Mar 2021 11:17:21 -0500 [thread overview]
Message-ID: <20210324161721.GA612661@bjorn-Precision-5520> (raw)
In-Reply-To: <20210222071828.30120-1-xxm@rock-chips.com>
On Mon, Feb 22, 2021 at 03:18:28PM +0800, Simon Xue wrote:
> Add driver for DesignWare based PCIe controller found on RK356X.
> The already existed driver pcie-rockchip-host is only used for
> Rockchip designed IP found on RK3399.
> +static int rockchip_pcie_start_link(struct dw_pcie *pci)
> +{
> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> +
> + /* Reset device */
> + gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
> + msleep(100);
Maybe a spec reference for the 100ms delay so we can match this up
with other places and make sure we aren't repeating delays because we
don't know what they are?
> + gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
> +
> + rockchip_pcie_enable_ltssm(rockchip);
> +
> + return 0;
> +}
> +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip)
> +{
> + struct device *dev = rockchip->pci.dev;
> + int ret;
> +
> + ret = devm_clk_bulk_get_all(dev, &rockchip->clks);
> + if (ret < 0)
> + return ret;
> +
> + rockchip->clk_cnt = ret;
> +
> + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);
> + if (ret)
> + return ret;
> +
> + return 0;
This is the same as
return clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);
> +}
> +static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
> +{
> + struct device *dev = rockchip->pci.dev;
> + int ret;
> +
> + rockchip->rst = devm_reset_control_array_get_exclusive(dev);
> + if (IS_ERR(rockchip->rst))
> + return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> + "failed to get reset lines\n");
> +
> + ret = reset_control_deassert(rockchip->rst);
> +
> + return ret;
return reset_control_deassert(rockchip->rst);
and drop "ret".
> +}
> +static int rockchip_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rockchip_pcie *rockchip;
> + struct pcie_port *pp;
> + int ret;
> +
> + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL);
> + if (!rockchip)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, rockchip);
> +
> + rockchip->pci.dev = dev;
> + rockchip->pci.ops = &dw_pcie_ops;
> +
> + pp = &rockchip->pci.pp;
> + pp->ops = &rockchip_pcie_host_ops;
> +
> + ret = rockchip_pcie_resource_get(pdev, rockchip);
> + if (ret)
> + return ret;
> +
> + /* DON'T MOVE ME: must be enable before PHY init */
> + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
> + if (IS_ERR(rockchip->vpcie3v3))
> + return dev_err_probe(dev, PTR_ERR(rockchip->rst),
> + "failed to get vpcie3v3 regulator\n");
> +
> + if (rockchip->vpcie3v3) {
This assumes devm_regulator_get_optional() returns a valid regulator,
IS_ERR, or NULL. I can't easily tell whether it can actually return
NULL. The regulator_get_optional() comment says it returns either a
regulator or IS_ERR; it doesn't mention NULL.
The other callers in drivers/pci (imx6_pcie_probe(),
histb_pcie_probe(), tegra_pcie_get_slot_regulators()) all explicitly
set the result to NULL in case of error. They should all do it the
same way.
> + ret = regulator_enable(rockchip->vpcie3v3);
> + if (ret) {
> + dev_err(dev, "failed to enable vpcie3v3 regulator\n");
> + return ret;
> + }
> + }
> +
> + ret = rockchip_pcie_phy_init(rockchip);
> + if (ret)
> + goto disable_regulator;
> +
> + ret = rockchip_pcie_reset_control_release(rockchip);
> + if (ret)
> + goto deinit_phy;
> +
> + ret = rockchip_pcie_clk_init(rockchip);
> + if (ret)
> + goto deinit_phy;
> +
> + ret = dw_pcie_host_init(pp);
> + if (ret)
> + goto deinit_clk;
> +
> + return 0;
> +
> +deinit_clk:
> + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +deinit_phy:
> + rockchip_pcie_phy_deinit(rockchip);
> +disable_regulator:
> + if (rockchip->vpcie3v3)
> + regulator_disable(rockchip->vpcie3v3);
> +
> + return ret;
> +}
> +
> +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match);
I don't think this is necessary, is it? I don't see MODULE_DEVICE_TABLE
being used except for modular drivers.
There are a couple other builtin drivers (mobiveil, microchip) that
use it, but I think those are mistakes.
> +static const struct of_device_id rockchip_pcie_of_match[] = {
> + { .compatible = "rockchip,rk3568-pcie", },
> + {},
> +};
> +
> +static struct platform_driver rockchip_pcie_driver = {
> + .driver = {
> + .name = "rockchip-dw-pcie",
> + .of_match_table = rockchip_pcie_of_match,
> + .suppress_bind_attrs = true,
> + },
> + .probe = rockchip_pcie_probe,
> +};
> +
Drop the blank line.
> +builtin_platform_driver(rockchip_pcie_driver);
next prev parent reply other threads:[~2021-03-24 16:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-22 7:17 [PATCH v5 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue
2021-02-22 7:18 ` [PATCH v5 2/2] PCI: " Simon Xue
2021-03-02 0:51 ` xxm
2021-03-05 1:23 ` Kever Yang
2021-03-24 16:17 ` Bjorn Helgaas [this message]
2021-03-24 16:34 ` Bjorn Helgaas
2021-03-02 0:51 ` [PATCH v5 1/2] dt-bindings: " xxm
2021-03-05 23:25 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210324161721.GA612661@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=heiko@sntech.de \
--cc=jbx6244@gmail.com \
--cc=linux-pci@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=robh+dt@kernel.org \
--cc=shawn.lin@rock-chips.com \
--cc=xxm@rock-chips.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).