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);
