Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Johan Hovold <johan@kernel.org>]

On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:
> Change hand-coded implementation of bulk clocks to use the existing

Let's hope everything is "hand-coded" at least for a few years still
(job security). ;)

Perhaps rephrase using "open-coded"?

> clk_bulk_* API.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++--------------------
>  1 file changed, 16 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 939f19241356..74588438db07 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 {
>  };
>  
>  struct qcom_pcie_resources_1_0_0 {
> -	struct clk *iface;
> -	struct clk *aux;
> -	struct clk *master_bus;
> -	struct clk *slave_bus;
> +	struct clk_bulk_data clks[4];
>  	struct reset_control *core;
>  	struct regulator *vdda;
>  };
> @@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
>  
>  	res->vdda = devm_regulator_get(dev, "vdda");
>  	if (IS_ERR(res->vdda))
>  		return PTR_ERR(res->vdda);
>  
> -	res->iface = devm_clk_get(dev, "iface");
> -	if (IS_ERR(res->iface))
> -		return PTR_ERR(res->iface);
> -
> -	res->aux = devm_clk_get(dev, "aux");
> -	if (IS_ERR(res->aux))
> -		return PTR_ERR(res->aux);
> -
> -	res->master_bus = devm_clk_get(dev, "master_bus");
> -	if (IS_ERR(res->master_bus))
> -		return PTR_ERR(res->master_bus);
> +	res->clks[0].id = "aux";
> +	res->clks[1].id = "iface";
> +	res->clks[2].id = "master_bus";
> +	res->clks[3].id = "slave_bus";
>  
> -	res->slave_bus = devm_clk_get(dev, "slave_bus");
> -	if (IS_ERR(res->slave_bus))
> -		return PTR_ERR(res->slave_bus);
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> +	if (ret < 0)
> +		return ret;

Are you sure there are no dependencies between these clocks and that
they can be enabled and disabled in any order?

Are you also convinced that they will always be enabled and disabled
together (e.g. not controlled individually during suspend)?

> -	ret = clk_prepare_enable(res->aux);
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>  	if (ret) {
> -		dev_err(dev, "cannot prepare/enable aux clock\n");
> +		dev_err(dev, "cannot prepare/enable clocks\n");

The bulk API already logs an error so you can drop the dev_err().

These comments apply also to the other patches.

Johan