On Mon, Mar 26, 2018 at 02:26:00PM +0530, Akshu Agrawal wrote:
> This enables/disables and sets auxiliary clock at 25Mhz. It uses
> common clock framework for proper ref counting. This approach will
> save power in comparison to keeping it always On in firmware.
>
> V2: Correcting the pin to OSCOUT1 from OSCOUT2
> V3: Fix error/warnings from kbuild test
> V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK
>
Please consider adding the change history across versions below
the marker --- which is the standard way of doing.
> TEST= aplay -vv <file>
> check register to see clock enabled
> kill aplay
> check register to see clock disabled
>
> Signed-off-by: Akshu Agrawal <akshu.agrawal@xxxxxxx>
> Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
> ---
> sound/soc/amd/acp-da7219-max98357a.c | 144 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 142 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
> index 99c6b5c..3c77803 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c
<snip>
> @@ -98,13 +117,27 @@ static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
> return ret;
> }
>
> + acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
Consider adding the error handling, for example if CONFIG_COMMON_CLK is not
defined for this platform, then clk_get may return error.
> + ret = clk_prepare_enable(acpd7219_clk);
> + if (ret < 0) {
> + dev_err(rtd->dev, "can't enable oscillator clock %d\n", ret);
> + return ret;
One return statement is enough here.
> + }
> +
> return ret;
> }
>
> static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_card *card = rtd->card;
> + struct clk *acpd7219_clk;
> +
> clk_disable_unprepare(da7219_dai_clk);
>
> + acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
Error handling here as well.
> + clk_disable_unprepare(acpd7219_clk);
> +
> return 0;
> }
>
> @@ -237,9 +270,113 @@ static int cz_fe_startup(struct snd_pcm_substream *substream)
> .num_controls = ARRAY_SIZE(cz_mc_controls),
> };
>
<snip>
> +
> +static int register_acpd7219_clocks(struct platform_device *pdev)
> +{
> + struct clk_init_data init = {};
> + struct clk *acp_clks;
> + struct clk_lookup *acp_clks_lookup;
> + struct cz_clock *cz_clock_obj;
> + struct resource *res;
> + struct device dev = pdev->dev;
> +
> + cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL);
> + if (!cz_clock_obj)
> + return -ENOMEM;
> +
> + cz_clock_obj->acp_clks_name = "acpd7219-clks";
> +
> + init.parent_names = NULL;
> + init.num_parents = 0;
> + init.name = cz_clock_obj->acp_clks_name;
> + init.ops = &acpd7219_clks_ops;
> + cz_clock_obj->acp_clks_hw.init = &init;
> +
> + acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw);
> + if (IS_ERR(acp_clks))
> + {
> + dev_err(&dev, "Failed to register DAI clocks: %ld\n",
> + PTR_ERR(acp_clks));
> + return -EINVAL;
> + }
> + cz_clock_obj->acp_clks = acp_clks;
> +
> + acp_clks_lookup = clkdev_create(acp_clks, cz_clock_obj->acp_clks_name,
> + "%s", dev_name(&dev));
> + if (!acp_clks_lookup)
> + dev_warn(&dev, "Failed to create DAI clkdev");
Error not handled on purpose?
> + else
> + cz_clock_obj->acp_clks_lookup = acp_clks_lookup;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get misc io resource.\n");
Shouldn't be clkdev_drop used here to free up?
> + return -EINVAL;
> + }
> + cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev, res->start,
> + resource_size(res));
> + if (!cz_clock_obj->res_base)
Here as well.
clkdev_drop is not called when the driver is removed?
Thanks,
Sriram.
--
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
