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

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

On Thu, Nov 24, 2011 at 03:54:46PM +0200, Peter Ujfalusi wrote:

> +	/*
> +	 * 192KHz rate is only supported with 19.2MHz/3.84MHz clock
> +	 * configuration. Fix the dmic clock divider for 192KHz
> +	 */
> +	if (params_rate(params) == 192000) {
> +		if (dmic->fclk_freq == 19200000 && dmic->clk_div == 0x1) {
> +			dmic->clk_div = 0x6;
> +		} else {
> +			dev_err(dmic->dev,
> +				"invalid clock configuration for 192KHz\n");
> +			return -EINVAL;
> +		}
> +	}

That's a bit wierd, magic numbers there and it'll mean we're trashing
the user's configuration.  Why not just return an error if the clock
setup is broken, or alternatively aren't there other invalid
configurations we should be trapping and fixing up (looking at the
divider setting code it seems unlikely that if the user doesn't
configure things we're not going to have a valid divider setup)?

> +static int omap_dmic_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
> +				    unsigned int freq, int dir)
> +{
> +	struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
> +
> +	if (dir == SND_SOC_CLOCK_IN)
> +		return omap_dmic_select_fclk(dmic, clk_id, freq);
> +	else if (dir == SND_SOC_CLOCK_OUT)
> +		return omap_dmic_select_divider(dmic, freq);

Might be better to specify using clk_id in case the next revision of the
IP has more clocks or something.

> +	dmic = kzalloc(sizeof(struct omap_dmic), GFP_KERNEL);
> +	if (!dmic)
> +		return -ENOMEM;

devm_ would save you having to clean stuff up later.

> +static int __init snd_omap_dmic_init(void)
> +{
> +	return platform_driver_register(&asoc_dmic_driver);
> +}
> +module_init(snd_omap_dmic_init);
> +
> +static void __exit snd_omap_dmic_exit(void)
> +{
> +	platform_driver_unregister(&asoc_dmic_driver);
> +}
> +module_exit(snd_omap_dmic_exit);

module_platform_driver.