Re: [PATCH 8/8] ASoC: Ux500: Add machine-driver

[Ola Lilja <ola.o.lilja@stericsson.com>]

On 04/23/2012 09:05 PM, Mark Brown wrote:

> On Fri, Apr 20, 2012 at 11:33:29AM +0200, Ola Lilja wrote:
> 
>> +snd-soc-ux500-mach-objs := u8500.o ux500_ab8500.o
>> +obj-$(CONFIG_SND_SOC_UX500_AB8500) += snd-soc-ux500-mach.o
> 
> This split into multiple files *really* doesn't seem like it adds
> anything but complexity, the small amount of reuse just doesn't seem
> worth it.


We will add more codecs to be matched up the same machine-driver and I found it
useful to have this split. It just separates the callbacks related to each codec
added in the dai-link-struct. I would like to keep this division if that is OK.

> 
>> +	/* Setup codec depending on driver-mode */
>> +	driver_mode = (channels == 8) ?
>> +		DRIVERMODE_CODEC_ONLY : DRIVERMODE_NORMAL;
>> +	dev_dbg(dev, "%s: Driver-mode: %s.\n", __func__,
>> +		(driver_mode == DRIVERMODE_NORMAL) ? "NORMAL" : "CODEC_ONLY");
>> +
>> +	ab8500_audio_set_bit_delay(codec_dai, 1);
> 
> What's this configuring?  I didn't notice it on the CODEC driver as the
> function wasn't exported IIRC.


The bit delay is the number of bit-clocks from the framesync to the first data-bit.
For the AB8500-chip it is set by the bit AB8500_DIGIFCONF2_IF0DEL.
I would have put this in the set_dai_fmt but I have not found a bit that is
controlling this.

> 
>> +	} else {
>> +		ab8500_audio_set_word_length(codec_dai, 20);
> 
> This should be done by using the TDM slot API - the slot length is one
> of the parameters.
> 
>> +	status = snd_soc_add_codec_controls(codec, ux500_ab8500_ctrls,
>> +			ARRAY_SIZE(ux500_ab8500_ctrls));
> 
> Do this from the driver.


OK.

> 
>> +	status = snd_soc_dapm_enable_pin(&codec->dapm, "Headset Left");
>> +	status |= snd_soc_dapm_enable_pin(&codec->dapm, "Headset Right");
> 
> No need to do this, everything defaults on.


Ah, then I have to put back all the disables instead, which I removed before
sending the patch =)