Re: [PATCH 2/5] ASoC: OMAP4: omap-dmic: Initial support for OMAP DMIC

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Tue, Nov 22, 2011 at 04:01:57PM +0200, Peter Ujfalusi wrote:

> +	switch (params_rate(params)) {
> +	case 96000:
> +	case 192000:
> +		break;

Why doesn't the driver need to tell the hardware what sample rate to run
at?

> +	dmic_clk = clk_get(dmic->dev, "dmic_fck");
> +	if (IS_ERR(dmic_clk)) {
> +		dev_err(dmic->dev, "cant get dmic_fck\n");
> +		return -ENODEV;
> +	}

Why aren't we getting and holding a reference to the clock over the
entire lifetime of the driver?

> +	/* disable clock while reparenting */
> +	pm_runtime_put_sync(dmic->dev);
> +	ret = clk_set_parent(dmic_clk, parent_clk);
> +	pm_runtime_get_sync(dmic->dev);

Since we're only allowing reclocking while idle shouldn't the clock
already be disabled?  Seems like it ought to be good for power if
nothing else...

> +static int omap_dmic_set_clkdiv(struct snd_soc_dai *dai,
> +				int div_id, int div)
> +{

DMIC clocking is usually fairly simple so it seems surprising that the
driver isn't able to figure this out for itself.