Re: [PATCH v1 1/5] ALSA: hda/tegra: Use clk_bulk helpers

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4187 bytes --]

On Tue, Jan 12, 2021 at 03:58:30PM +0300, Dmitry Osipenko wrote:
> Use clk_bulk helpers to make code cleaner.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  sound/pci/hda/hda_tegra.c | 68 ++++++---------------------------------
>  1 file changed, 9 insertions(+), 59 deletions(-)

Heh... I have a branch samewhere with this same patch. Glad I can cross
that off my list. One thing jumped out at me, see below.

> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
> index 70164d1428d4..4c799661c2f6 100644
> --- a/sound/pci/hda/hda_tegra.c
> +++ b/sound/pci/hda/hda_tegra.c
> @@ -70,9 +70,8 @@
>  struct hda_tegra {
>  	struct azx chip;
>  	struct device *dev;
> -	struct clk *hda_clk;
> -	struct clk *hda2codec_2x_clk;
> -	struct clk *hda2hdmi_clk;
> +	struct clk_bulk_data clocks[3];
> +	unsigned int nclocks;
>  	void __iomem *regs;
>  	struct work_struct probe_work;
>  };
> @@ -113,36 +112,6 @@ static void hda_tegra_init(struct hda_tegra *hda)
>  	writel(v, hda->regs + HDA_IPFS_INTR_MASK);
>  }
>  
> -static int hda_tegra_enable_clocks(struct hda_tegra *data)
> -{
> -	int rc;
> -
> -	rc = clk_prepare_enable(data->hda_clk);
> -	if (rc)
> -		return rc;
> -	rc = clk_prepare_enable(data->hda2codec_2x_clk);
> -	if (rc)
> -		goto disable_hda;
> -	rc = clk_prepare_enable(data->hda2hdmi_clk);
> -	if (rc)
> -		goto disable_codec_2x;
> -
> -	return 0;
> -
> -disable_codec_2x:
> -	clk_disable_unprepare(data->hda2codec_2x_clk);
> -disable_hda:
> -	clk_disable_unprepare(data->hda_clk);
> -	return rc;
> -}
> -
> -static void hda_tegra_disable_clocks(struct hda_tegra *data)
> -{
> -	clk_disable_unprepare(data->hda2hdmi_clk);
> -	clk_disable_unprepare(data->hda2codec_2x_clk);
> -	clk_disable_unprepare(data->hda_clk);
> -}
> -
>  /*
>   * power management
>   */
> @@ -186,7 +155,7 @@ static int __maybe_unused hda_tegra_runtime_suspend(struct device *dev)
>  		azx_stop_chip(chip);
>  		azx_enter_link_reset(chip);
>  	}
> -	hda_tegra_disable_clocks(hda);
> +	clk_bulk_disable_unprepare(hda->nclocks, hda->clocks);
>  
>  	return 0;
>  }
> @@ -198,7 +167,7 @@ static int __maybe_unused hda_tegra_runtime_resume(struct device *dev)
>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
>  	int rc;
>  
> -	rc = hda_tegra_enable_clocks(hda);
> +	rc = clk_bulk_prepare_enable(hda->nclocks, hda->clocks);
>  	if (rc != 0)
>  		return rc;
>  	if (chip && chip->running) {
> @@ -268,29 +237,6 @@ static int hda_tegra_init_chip(struct azx *chip, struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int hda_tegra_init_clk(struct hda_tegra *hda)
> -{
> -	struct device *dev = hda->dev;
> -
> -	hda->hda_clk = devm_clk_get(dev, "hda");
> -	if (IS_ERR(hda->hda_clk)) {
> -		dev_err(dev, "failed to get hda clock\n");
> -		return PTR_ERR(hda->hda_clk);
> -	}
> -	hda->hda2codec_2x_clk = devm_clk_get(dev, "hda2codec_2x");
> -	if (IS_ERR(hda->hda2codec_2x_clk)) {
> -		dev_err(dev, "failed to get hda2codec_2x clock\n");
> -		return PTR_ERR(hda->hda2codec_2x_clk);
> -	}
> -	hda->hda2hdmi_clk = devm_clk_get(dev, "hda2hdmi");
> -	if (IS_ERR(hda->hda2hdmi_clk)) {
> -		dev_err(dev, "failed to get hda2hdmi clock\n");
> -		return PTR_ERR(hda->hda2hdmi_clk);
> -	}
> -
> -	return 0;
> -}
> -
>  static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
>  {
>  	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
> @@ -495,7 +441,11 @@ static int hda_tegra_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	err = hda_tegra_init_clk(hda);
> +	hda->clocks[hda->nclocks++].id = "hda";
> +	hda->clocks[hda->nclocks++].id = "hda2hdmi";
> +	hda->clocks[hda->nclocks++].id = "hda2codec_2x";

Originally the code did this in this order: "hda", "hda2codec_2x" and
"hda2hdmi". I don't expect the exact order to be very relevant, but was
there any particular reason to change it?

In either case, this should be fine:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]