Re: [PATCH v3] ASoC: da7210: Add support for PLL and SRM

[Ashish Chavan <ashish.chavan@kpitcummins.com>]

On Wed, 2012-02-01 at 11:50 +0000, Mark Brown wrote:
> On Wed, Feb 01, 2012 at 04:55:29PM +0530, Ashish Chavan wrote:
> 
> > +static const u32 da7210_pll_div[][COL_CNT] = {
> > +	/* for MASTER mode, fs = 44.1Khz */
> > +	{ 12000000, 0xE8, 0x6C, 0x2, },		/* MCLK=12Mhz */
> 
> Even better than defines to index into the array would be an array of
> structs with named members...
> 

OK, will do that.

> > +	if (da7210->mclk_rate) {
> > +		/* PLL mode, disable PLL bypass */
> > +		snd_soc_update_bits(codec, DA7210_PLL_DIV3, DA7210_PLL_BYP, 0);
> > +	} else {
> > +		/* PLL bypass mode, enable PLL bypass */
> > +		snd_soc_update_bits(codec, DA7210_PLL_DIV3, DA7210_PLL_BYP,
> > +							    DA7210_PLL_BYP);
> > +	}
> 
> This is really weird - if anything it looks the wrong way round.
> mclk_rate is what's set by set_sysclk() so if the user has configured
> MCLK we will never use it even if it's at a suitable rate but if the
> user hasn't configured one we rely on it directly.  If anything I'd
> expect this code to enable the PLL only if the MCLK is not a suitable
> rate.
> 

Actually it worked fine for me because of the way platform driver is
written. It basically uses static configurations for PLL and PLL bypass
modes. It calls set_sysclk() only in case of PLL. So the codec driver
more or less uses mckl_rate as a flag to see if PLL is enabled. I know
it's not clean and may be I can update it to comply with rest of the
stuff.

> The normal pattern is that set_sysclk() specifies the clock the bulk of
> the device will be using, when used with the PLL that'd be the PLL
> output not the PLL input.  Alternatively just specify the MCLK always
> and let the driver figure out when to use the PLL.

Yes, got the point.

> 
> > +	if (da7210->master) {
> > +		/* In PLL master mode, use master mode PLL dividers */
> > +		switch (fout) {
> > +		case 2822400:
> > +			row_idx = MASTER_2822400_DIV_OFFSET;
> > +			break;
> > +		case 3072000:
> > +			row_idx = MASTER_3072000_DIV_OFFSET;
> > +			break;
> > +		default:
> > +			dev_err(codec_dai->dev,
> > +				"Unsupported PLL output frequency %d\n", fout);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		/* In PLL slave mode, use SRM mode PLL dividers */
> > +		row_idx = SLAVE_SRM_DIV_OFFSET;
> > +	}
> 
> You need checks elsewhere to make sure that the user doesn't try to
> reconfigure master/slave while the PLL is active.

AFAIK master/slave configuration is static(depending on the board
configuration) and done via set_fmt() from hw_params() of platform
driver. Can you please point me to an example where dynamic/runtime
setting of I2S master/slave mode is supported?