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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

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

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

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.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

On Tue, Feb 07, 2012 at 06:55:49PM +0530, Ashish Chavan wrote:
> 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:

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

The most common case is things like bluetooth SCO connections where if
there's a baseband you want to clock from the baseband but if there's no
baseband (eg, for VoIP calls) you can't do that.  Some devices also need
to change audio interface configuration depening on things like the
number of channels.

Even without any dynamic reconfiguration you also have cases where the
sequence people use to power things up is to set the clocking and then
configure the audio interface format - even if the audio interface
format is always the same you'll still get it being reconfigured the
first time you start up.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Thu, Feb 09, 2012 at 06:12:20PM +0530, Ashish Chavan wrote:

> BTW I am also planning to add SPI support to this driver. Just want to
> know if it should be done via reg-map APIs? If yes, then do I need to
> convert existing I2C stuff too to use reg-map?

Yes, please.  The idea is that all of the I2C and SPI drivers will get
converted over.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]