> -----Original Message----- > From: Manivannan Sadhasivam [mailto:manivannan.sadhasivam@xxxxxxxxxx] > Sent: 27 October 2023 19:19 > To: Shradha Todi <shradha.t@xxxxxxxxxxx> > Cc: jingoohan1@xxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; > robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx; > alim.akhtar@xxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; pankaj.dubey@xxxxxxxxxxx > Subject: Re: [PATCH] PCI: exynos: Adapt to clk_bulk_* APIs > > On Mon, Oct 09, 2023 at 11:52:16AM +0530, Shradha Todi wrote: > > There is no need to hardcode the clock info in the driver as driver > > can rely on the devicetree to supply the clocks required for the > > functioning of the peripheral. Get rid of the static clock info and > > obtain the platform supplied clocks. The total number of clocks > > supplied is obtained using the devm_clk_bulk_get_all() API and used > > for the rest of the clk_bulk_* APIs. > > > > Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx> > > --- > > drivers/pci/controller/dwc/pci-exynos.c | 46 > > ++++++------------------- > > 1 file changed, 11 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-exynos.c > > b/drivers/pci/controller/dwc/pci-exynos.c > > index 9e42cfcd99cc..023cf41fccd7 100644 > > --- a/drivers/pci/controller/dwc/pci-exynos.c > > +++ b/drivers/pci/controller/dwc/pci-exynos.c > > @@ -54,8 +54,8 @@ > > struct exynos_pcie { > > struct dw_pcie pci; > > void __iomem *elbi_base; > > - struct clk *clk; > > - struct clk *bus_clk; > > + struct clk_bulk_data *clks; > > + int clk_cnt; > > struct phy *phy; > > struct regulator_bulk_data supplies[2]; > > }; > > @@ -65,30 +65,18 @@ static int exynos_pcie_init_clk_resources(struct > exynos_pcie *ep) > > struct device *dev = ep->pci.dev; > > int ret; > > > > - ret = clk_prepare_enable(ep->clk); > > - if (ret) { > > - dev_err(dev, "cannot enable pcie rc clock"); > > + ret = devm_clk_bulk_get_all(dev, &ep->clks); > > + if (ret < 0) > > Please use !(ret) here and below to be consistent with the driver. > In this case, only negative values indicate failure. Hence we cannot use (!ret) here. > > return ret; > > - } > > > > - ret = clk_prepare_enable(ep->bus_clk); > > - if (ret) { > > - dev_err(dev, "cannot enable pcie bus clock"); > > - goto err_bus_clk; > > - } > > + ep->clk_cnt = ret; > > Since clk_cnt is "int", you can just use it directly instead of "ret". > Thanks for this suggestion! Will take care in v2. > > > > - return 0; > > - > > -err_bus_clk: > > - clk_disable_unprepare(ep->clk); > > - > > - return ret; > > + return clk_bulk_prepare_enable(ep->clk_cnt, ep->clks); > > } > > > > static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep) > > { > > - clk_disable_unprepare(ep->bus_clk); > > - clk_disable_unprepare(ep->clk); > > + clk_bulk_disable_unprepare(ep->clk_cnt, ep->clks); > > } > > > > static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg) > > @@ -332,17 +320,9 @@ static int exynos_pcie_probe(struct > platform_device *pdev) > > if (IS_ERR(ep->elbi_base)) > > return PTR_ERR(ep->elbi_base); > > > > - ep->clk = devm_clk_get(dev, "pcie"); > > - if (IS_ERR(ep->clk)) { > > - dev_err(dev, "Failed to get pcie rc clock\n"); > > - return PTR_ERR(ep->clk); > > - } > > - > > - ep->bus_clk = devm_clk_get(dev, "pcie_bus"); > > - if (IS_ERR(ep->bus_clk)) { > > - dev_err(dev, "Failed to get pcie bus clock\n"); > > - return PTR_ERR(ep->bus_clk); > > - } > > + ret = exynos_pcie_init_clk_resources(ep); > > + if (ret < 0) > > + return ret; > > > > ep->supplies[0].supply = "vdd18"; > > ep->supplies[1].supply = "vdd10"; > > @@ -351,10 +331,6 @@ static int exynos_pcie_probe(struct > platform_device *pdev) > > if (ret) > > return ret; > > > > - ret = exynos_pcie_init_clk_resources(ep); > > - if (ret) > > - return ret; > > - > > ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep- > >supplies); > > if (ret) > > You need to disable_unprepare() clocks in error path here and above. > Thanks for pointing out! Will take care in v2. > - Mani > > -- > மணிவண்ணன் சதாசிவம்
