[alsa-devel] [PATCH] ASoC: Analog Devices AD193X I2S codec family driver.

[Manuel Lauss]

Hi Mark,

Thanks for looking at it!

> > +/* Does the codec have power?  In case of STANDBY/OFF BIAS levels, the
> > + * hardware access functions below assume the codec is without power
> > + * to avoid futile bus transactions.
> > + */
> > +static int codec_is_powered(struct snd_soc_codec *c)
> > +{
> > +	return (c->bias_level == SND_SOC_BIAS_PREPARE) ||
> > +		(c->bias_level == SND_SOC_BIAS_ON);
> > +}
> 
> It'd be worth updating this comment to mention what you're doing with
> the chip power in the standby state - it's unusual for the codec to be
> held with so little power in standby mode that register writes don't
> work, which is why this is a driver-specific thing.  I guess if DAPM
> support were implemented then this wouldn't be required any more?

The platform I developed this driver for can cut power to the codec
and amplifiers (customer requirement: no audio is playing -- save as
much power as possible, which the hardware engineer interpreted as
cutting all electrical power to the whole audio subsystem).  To be
honest I somewhat expect the I2S codec drivers to be able to cope with
a situation such as this.

The other thing is, I tried to implement finer-grained PM, but to be
honest, after staring at the DAPM defines, it is still a mystery to
me how that stuff is supposed to be hooked up.
The codec allows to control an internal master clock, the DAC, and ADC
clock supply (which depend on master).

 
> > +/* add non dapm controls */
> > +static int ad1939_add_controls(struct snd_soc_codec *codec)
> > +{
> > +	struct ad1939_private *ad = codec->private_data;
> > +	int err, i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ad1939_snd_ctls); i++) {
> > +		if ((i <= 3) && (i >= ad->mixpairs))
> > +			continue;
> 
> This could really use a comment, at least referencing the documentation
> in the header file.

Okay,

 
> > +static int ad1939_hw_params(struct snd_pcm_substream *substream,
> > +			    struct snd_pcm_hw_params *params)
> > +{
> 
> > +	/* sample rate */
> > +	dac0 &= ~(3<<1);	/* 48kHz */
> > +	adc0 &= ~(3<<6);	/* 48kHz */
> 
> Might be worth making these comments (and the similar ones you have
> elsewhere) say "Default: 48kHz" or similar.  I can see what you're doing
> here but on first reading through it was a bit confusing.  Not really 
> important, though.

Will do,

 
> > +	switch (rate) {
> > +	case 32000 ... 48000:
> > +		break;
> > +	case 64000 ... 96000:
> > +		dac0 |= (1<<1);
> > +		adc0 |= (1<<6);
> > +		break;
> > +	case 128000 ... 192000:
> > +		dac0 |= (2<<1);
> > +		adc0 |= (2<<6);
> > +		break;
> 
> This and some of the other code will force the ADC and DAC to have the
> same configuration - is that needed by the codec?  If not then it'd be
> better to check which substream it is operating on and only configure
> the ADC or DAC as appropriate (you could save lots of conditionals by
> only checking when actually writing the configuration out at the end of
> the function).

It's not mandated by the chip but rather my development platform (ADC and
DAC have separate I2S lines; my test hardware has bit- and frameclock of
both tied together).

 
> With the code as it currently stands the codec should impose constraints
> to let user space know that the playback and capture streams have to be
> in the same mode.  There's examples of how to do that in cs4270,
> ssm2602, wm8903 and possibly other drivers.

In my case, the constraints should be imposed by the machine driver ;-)
The codec doesn't really care.

 
> One or the other of these should probably be done prior to merge.
> 
> > +	/* codec clocks master/slave setup */
> > +	dac1 &= ~((1<<4) | (1<<5)); /* LRCK BCK slave */
> > +	adc2 &= ~((1<<3) | (1<<6)); /* LRCK BCK slave */
> > +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +	case SND_SOC_DAIFMT_CBM_CFM:	 /* BCK/LCK master */
> > +		dac1 |= (1<<4) | (1<<5); /* LRCK BCK master */
> > +		adc2 |= (1<<3) | (1<<6); /* LRCK BCK master */
> > +		if (ad->drvflags & AD1939_DRV_ADCDAC_COMMON_BCK) {
> > +			if (ad->drvflags & AD1939_DRV_ADC_BCK_MASTER)
> > +				dac1 &= ~(1<<5); /* DAC BCLK slave */
> > +			else
> > +				adc2 &= ~(1<<6); /* ADC BCLK slave */
> > +		}
> 
> In general if the codec has clocking which can be configured as flexibly
> as this one appears to then it's normally better to allow the machine
> driver to configure the clocking manually via the clock configuration
> APIs.

Agreed; I didn't know I can actually do that with ASoCv1.
 

> This avoids having to have code in the codec driver to support unusual
> machine configurations such as those where the audio clocks are also
> used for other devices and lets machine drivers do things like change
> clock sources dynamically at run time.  For example, a system may only
> enable the PLL for some clock rates and use a crystal source for others,
> saving power.  It also frees you from having to worry about imposing
> constraints arising from things like shared LRCLKs in the codec driver,
> saving quite a bit of complexity.
> 
> This is probably OK for merge as-is but it's very different to how most
> codec drivers work and is going to limit what people can do with the
> driver.
> 
> > +#define AD1939_RATES	\
> > +	(SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | \
> > +	 SNDRV_PCM_RATE_192000)
> 
> Your code appeared to support rather more rates than this?

Theoretically it supports anything coming from the I2S frameclk line.
How can I express that?

 
> > +	/* limit output attenuation controls to requested number */
> > +	ad->mixpairs = setup->mixpairs;
> > +	if ((ad->mixpairs < 1) || (ad->mixpairs > 4))
> > +		ad->mixpairs = 4;
> 
> It's probably worth complaining if someone tries to set ad->mixpairs to
> something more than 4.  It may be better to just not do this and leave
> the controls exposed - why is this being done?

Customer kept asking why alsa shows 4 stereo output mixer controls when
actually only 2 are connected.

 
> > +	/* Bitclock sources for the ADC and DAC I2S interfaces */
> > +	r0 = ad1939_read(codec, AD1939_DACCTL1);
> > +	r1 = ad1939_read(codec, AD1939_ADCCTL2);
> > +	r0 &= ~AD1939_BCLKSRC_DAC_PLL;
> > +	r1 &= ~AD1939_BCLKSRC_ADC_PLL;
> > +	r0 |= setup->dac_adc_clksrc & AD1939_BCLKSRC_DAC_PLL;
> > +	r1 |= setup->dac_adc_clksrc & AD1939_BCLKSRC_ADC_PLL;
> > +	ad1939_write(codec, AD1939_DACCTL1, r0);
> > +	ad1939_write(codec, AD1939_ADCCTL2, r1);
> 
> This definitely makes it look like the chip is flexible enough to
> warrant allowing machine drivers finer grained control.
> 
> > +	/* register pcms */
> > +	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1,
> > +			       SNDRV_DEFAULT_STR1);
> > +	if (unlikely(ret < 0)) {
> 
> Optimist :)

Reality is a harsh mistress, BUT currently she agrees with me ;-)

 
> > +	printk(KERN_ERR CODEC_NAME ": i2c codec init failed\n");
> 
> Since you have an I2C device here this could be dev_err(). 
> > +#define ad1939_i2c_suspend NULL
> > +#define ad1939_i2c_resume NULL
> 
> > +#define ad1939_spi_suspend NULL
> > +#define ad1939_spi_resume NULL
> 
> It'd be better to just remove these - they can be added when someone
> implements them and in the mean time it'll avoid anyone seeing the
> references and assuming suspend and resume are supported.

I'm going to remove them; PM actually works fine as-is.


> > +/* Master PLL clock source, select one */
> > +#define AD1939_PLL_SRC_MCLK		0	/* external clock */
> > +#define AD1939_PLL_SRC_DACLRCK		1	/* get from DAC LRCLK */
> > +#define AD1939_PLL_SRC_ADCLRCK		2	/* get from ADC LRCLK */
> 
> > +/* clock sources for ADC, DAC. Refer to the manual for more information
> > + * (for 192000kHz modes, internal PLL _MUST_ be used). Select one for ADC
> > + * and DAC.
> 
> > +/* MCLK_XO pin configuration */
> > +#define AD1939_MCLKXO_MCLKXI		0	/* mirror MCLK_XI pin */
> > +#define AD1939_MCLKXO_256FS		1
> > +#define AD1939_MCLKXO_512FS		2
> > +#define AD1939_MCLKXO_OFF		3	/* disable MCLK_XO output */
> 
> These are things that I'd definitely expect to see configured via clock
> configuration calls.

I'll look into it.


Thanks again,
	Manuel Lauss