[alsa-devel] [PATCH v4] ASoC: AMD: Enable/Disable auxiliary clock via common clock framework

[Sriram Periyasamy]

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 at amd.com>
> Acked-by: Alex Deucher <alexander.deucher at amd.com>
> ---
>  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.
--