On Tue, Sep 2, 2008 at 4:26 AM, Mark Brown <broonie@xxxxxxxxxxxxx> wrote:
> On Mon, Sep 01, 2008 at 02:57:57PM -0700, sakoman@xxxxxxxxx wrote:
>> +
>> +config SND_SOC_TWL4030
>> + tristate
>> + depends on SND_SOC && I2C
>> +
>
> This should also be added to SND_SOC_ALL_CODECS to ensure testing
> coverage.
Hmm . . . I get no hits grep-ing for SND_SOC_ALL_CODECS. Could this
be something we are missing in the linux-omap git?
>> +struct twl4030_priv {
>> + unsigned int dummy;
>> +};
>
> If this is empty it should be removable?
I plan to support further codec features in future patches and have a
strong suspicion that private data may be required. Is it preferred
to add the infrastructure now, or wait till it is required in the
future? I opted for the former, but don't really care either way.
>> +static int twl4030_set_bias_level(struct snd_soc_codec *codec,
>> + enum snd_soc_bias_level level)
>> +{
>> +
>> + printk(KERN_INFO "TWL4030 Audio Codec set bias level\n");
>
> This printk() should be removed - it's just debug output.
You are correct. I will remove this.
>> + switch (level) {
>> + case SND_SOC_BIAS_ON:
>> + break;
>> + case SND_SOC_BIAS_PREPARE:
>> + break;
>> + case SND_SOC_BIAS_STANDBY:
>> + break;
>> + case SND_SOC_BIAS_OFF:
>> + break;
>> + }
>
> Should you be calling power_up() and power_down() for standby and off
> here?
I was saving power management for another patch after I have time to
actually measure the effects. That said, I see no reason to not add
the power up/down calls to the places you indicate. I will do a quick
test and add to my resubmission.
>> +static int twl4030_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params)
>> +{
>
>> + /* turn off codec before changing format */
>> + mode = twl4030_read_reg_cache(codec, REG_CODEC_MODE);
>> + mode &= ~CODECPDZ;
>> + twl4030_write(codec, REG_CODEC_MODE, mode);
>
> The comments about powering up and down the codec could probably use a
> little clarification given the fact that there are also power_down() and
> power_up() functions.
Good point. This is just one of those chip quirks that require the
CODECPDZ bit to be 0 when modes are changed rather than any
requirement for a full power up/power down sequence.
>> +static int twl4030_mute(struct snd_soc_dai *dai, int mute)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> +
>> + u8 ldac_reg = twl4030_read_reg_cache(codec, REG_ARXL2PGA);
>> + u8 rdac_reg = twl4030_read_reg_cache(codec, REG_ARXR2PGA);
>> +
>> + if (mute) {
>> + twl4030_write(codec, REG_ARXL2PGA, 0x00);
>> + twl4030_write(codec, REG_ARXR2PGA, 0x00);
>> + twl4030_write_reg_cache(codec, REG_ARXL2PGA, ldac_reg);
>> + twl4030_write_reg_cache(codec, REG_ARXR2PGA, rdac_reg);
>> + } else {
>> + twl4030_write(codec, REG_ARXL2PGA, ldac_reg);
>> + twl4030_write(codec, REG_ARXR2PGA, rdac_reg);
>> + }
>
> It may be better to make these user visible controls - usually the mute
> provided here is specifically for the DAC but these look like controls
> for the PGA. The intention here is to avoid artifacts from the DAC when
> the input clock stops and start - if there aren't any then user space
> may well appreciate the control. Either way is fine.
I'm not hearing any clicks & pops, so I will leave this in.
>> +static int twl4030_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> + struct snd_soc_device *socdev = platform_get_drvdata(pdev);
>> + struct snd_soc_codec *codec = socdev->codec;
>> +
>> + printk(KERN_INFO "TWL4030 Audio Codec suspend\n");
>> + twl4030_set_bias_level(codec, SND_SOC_BIAS_OFF);
>> +
>> + return 0;
>> +}
>
> Merging the power down into set_bias_level() would mean that this would
> do a controlled power down - at present it will be a no-op.
>
> Similarly, adding the power up to the standby configuration would ensure
> a controlled power up of the codec when resuming. Looking at the power
> up sequence I'm not sure that simply writing back the cache will work
> well?
Agreed. I will make this change.
> +}
>
>> + twl4030_init_chip();
>> + twl4030_power_up(codec, APLL_RATE_44100 | OPT_MODE);
>
> It looks like the driver is assuming a particular clock rate going into
> the codec - does the chip require a fixed clock or is the driver only
> supporting one configuration? Either is fine.
There is a fixed clock.
Thanks for the comments!
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
