Re: [PATCH v5 5/7] PCI: spacemit: Add SpacemiT PCIe host driver

[Alex Elder]

On 11/8/25 7:00 AM, Christophe JAILLET wrote:
> Le 07/11/2025 à 20:15, Alex Elder a écrit :
> > Introduce a driver for the PCIe host controller found in the SpacemiT
> > K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
> > The driver supports three PCIe ports that operate at PCIe gen2 transfer
> > rates (5 GT/sec).  The first port uses a combo PHY, which may be
> > configured for use for USB 3 instead.
> >
> > Signed-off-by: Alex Elder <elder@xxxxxxxxxxxx>
> > ---
>
> ...
>
> > +static int k1_pcie_probe(struct platform_device *pdev)
> > +{
> > +    struct device *dev = &pdev->dev;
> > +    struct k1_pcie *k1;
> > +    int ret;
> > +
> > +    k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
> > +    if (!k1)
> > +        return -ENOMEM;
> > +
> > +    k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
> > +                               SYSCON_APMU, 1,
> > +                               &k1->pmu_off);
> > +    if (IS_ERR(k1->pmu))
> > +        return dev_err_probe(dev, PTR_ERR(k1->pmu),
> > +                     "failed to lookup PMU registers\n");
> > +
> > +    k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
> > +    if (!k1->link)
>
> if (IS_ERR(k1->link)) ?

Yes, you're right.  I'll fix this in the next version.


> > +        return dev_err_probe(dev, -ENOMEM,
> > +                     "failed to map \"link\" registers\n");
>
> Message with -ENOMEM are ignored, so a direct return -ENOMEM is less 
> verbose and will bhave the same. See [1].
>
> But in this case, I think it should be PTR_ERR(k1->link).

Yes, that's what it will be.

> [1]: https://elixir.bootlin.com/linux/v6.18-rc2/source/drivers/base/ 
> core.c#L5015
>
> > +
> > +    k1->pci.dev = dev;
> > +    k1->pci.ops = &k1_pcie_ops;
> > +    dw_pcie_cap_set(&k1->pci, REQ_RES);
> > +
> > +    k1->pci.pp.ops = &k1_pcie_host_ops;
> > +
> > +    /* Hold the PHY in reset until we start the link */
> > +    regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> > +            APP_HOLD_PHY_RST);
> > +
> > +    ret = devm_regulator_get_enable(dev, "vpcie3v3");
> > +    if (ret)
> > +        return dev_err_probe(dev, ret,
> > +                     "failed to get \"vpcie3v3\" supply\n");
> > +
> > +    pm_runtime_set_active(dev);
> > +    pm_runtime_no_callbacks(dev);
> > +    devm_pm_runtime_enable(dev);
> > +
> > +    platform_set_drvdata(pdev, k1);
> > +
> > +    ret = k1_pcie_parse_port(k1);
> > +    if (ret)
> > +        return dev_err_probe(dev, ret, "failed to parse root port\n");
> > +
> > +    ret = dw_pcie_host_init(&k1->pci.pp);
> > +    if (ret)
> > +        return dev_err_probe(dev, ret, "failed to initialize host\n");
> > +
> > +    return 0;
> > +}
>
> ...
>
> > +static const struct of_device_id k1_pcie_of_match_table[] = {
> > +    { .compatible = "spacemit,k1-pcie", },
> > +    { },
>
> Unneeded trainling comma after a terminator.

Unneeded, but not erroneous.  In any case, I'll drop it based
on your preference.

Thank you for your review.

					-Alex

> > +};
> > +
> > +static struct platform_driver k1_pcie_driver = {
> > +    .probe    = k1_pcie_probe,
> > +    .remove    = k1_pcie_remove,
> > +    .driver = {
> > +        .name            = "spacemit-k1-pcie",
> > +        .of_match_table        = k1_pcie_of_match_table,
> > +        .probe_type        = PROBE_PREFER_ASYNCHRONOUS,
> > +    },
> > +};
> > +module_platform_driver(k1_pcie_driver);