| Message ID | 20210112125834.21545-2-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clock and reset improvements for Tegra ALSA drivers | expand |
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>
15.01.2021 18:22, Thierry Reding пишет: ... >> 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? The reason was "to make code look nicer". This was a conscious decision since indeed the clocks order shouldn't matter for this driver.
On Mon, Jan 18, 2021 at 02:31:35AM +0300, Dmitry Osipenko wrote: > 15.01.2021 18:22, Thierry Reding пишет: > ... > >> 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? > > The reason was "to make code look nicer". This was a conscious decision > since indeed the clocks order shouldn't matter for this driver. Yeah, it's probably fine. In case this ends up causing trouble after all we can always change the order back. Thierry
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"; + + err = devm_clk_bulk_get(&pdev->dev, hda->nclocks, hda->clocks); if (err < 0) goto out_free;