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

[Péter Ujfalusi]

On Thursday 24 November 2011 17:10:19 Mark Brown wrote:
> 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,

192KHz rate is supported only with one clock configuration:
19.2MHz input clock, and 3.84MHz output clock. With the same clock 
configuration the 96KHz rate is also valid. The difference is that we need to 
use different divider value for 96KHz (0x1) versus 192KHz (0x6).
The Machine driver configures three things: clock source, source clock speed, 
and the desired output speed.
In the omap_dmic_select_divider() the driver configures the dividers, but 
there it does not know if the stream is 96 or 192 KHz. Since for both the 
machine provided configuration is the same I can fix up the divider in case 
the stream is 192KHz, and the machine driver was asking for a clock 
configuration matching with the requirements.
Basically the omap_dmic_select_divider() configures the divider for 96KHz, 
here the driver does the fixup for 192KHz.
I'll update the comment to be more verbose on what is going on here.

> 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)?

So far this is the only configuration need special care.

> 
> > +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.

The fclk selection is using clk_id. I can add clk_id for the dmic_out clock.

> 
> > +	dmic = kzalloc(sizeof(struct omap_dmic), GFP_KERNEL);
> > +	if (!dmic)
> > +		return -ENOMEM;
> 
> devm_ would save you having to clean stuff up later.

Yes, handy. Changed.

> 
> > +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.

Sure.

--
Péter