[alsa-devel] [PATCH V4 1/5] sound: asoc: Adding support for STA529 Audio Codec
rajeev
rajeev-dlh.kumar at st.com
Mon Jun 6 09:08:43 CEST 2011
Hi Lars-Peter Clausen
Please find my answer inline below.
On 6/6/2011 11:53 AM, Lars-Peter Clausen wrote:
> On 06/06/2011 07:57 AM, Rajeev Kumar wrote:
>> This patch adds the support for STA529 audio codec.
>> Details of the audio codec can be seen here:
>> http://www.st.com/internet/imag_video/product/159187.jsp
>>
>> Signed-off-by: Rajeev Kumar <rajeev-dlh.kumar at st.com>
>> ---
>> sound/soc/codecs/Kconfig | 5 +
>> sound/soc/codecs/Makefile | 2 +
>> sound/soc/codecs/sta529.c | 374 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 381 insertions(+), 0 deletions(-)
>> create mode 100644 sound/soc/codecs/sta529.c
>>
>> [...]
>> +
>> +struct sta529 {
>> + unsigned int sysclk;
> sysclk is unused
>> + enum snd_soc_control_type control_type;
>> + void *control_data;
> control_data is unused
>
Oops, will be removed.
>> +};
>> +
>> +static const char *pwm_mode_text[] = { "binary", "headphone", "ternary",
>> + "phase-shift"};
>> +static const char *interface_mode_text[] = { "slave", "master"};
>> +
>> +static const struct soc_enum pwm_src_enum =
>> +SOC_ENUM_SINGLE(STA529_FFXCFG1, 4, 4, pwm_mode_text);
>> +
>> +static const struct soc_enum mode_src_enum =
>> +SOC_ENUM_SINGLE(STA529_P2SCFG0, 0, 2, interface_mode_text);
>> +
>> +static const struct snd_kcontrol_new sta529_new_snd_controls[] = {
>> + SOC_ENUM("PWM Select", pwm_src_enum),
>> + SOC_ENUM("MODE Select", mode_src_enum),
>
> The mode should be configured by the dai_drivers set_fmt callback, and not by a
> control.
>
I think giving a interface to the user is better option rather than doing it in set_fmt callback.
>> +};
>> +
>> +static const DECLARE_TLV_DB_SCALE(out_gain_tlv, -9150, 50, 0);
>> +static const DECLARE_TLV_DB_SCALE(master_vol_tlv, -12750, 50, 0);
>> +
>> +static const struct snd_kcontrol_new sta529_snd_controls[] = {
>> + SOC_DOUBLE_R_TLV("Digital Playback Volume", STA529_LVOL, STA529_RVOL, 0,
>> + 127, 0, out_gain_tlv),
>> + SOC_SINGLE_TLV("Master Playback Volume", STA529_MVOL, 0, 127, 1,
>> + master_vol_tlv),
>> +};
>> +
>> +static int sta529_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> + struct snd_soc_codec *codec = rtd->codec;
>> + int pdata = 0;
>> +
>> + switch (params_format(params)) {
>> + case SNDRV_PCM_FORMAT_S16_LE:
>> + pdata = 1;
>> + break;
>> + case SNDRV_PCM_FORMAT_S24_LE:
>> + pdata = 2;
>> + break;
>> + case SNDRV_PCM_FORMAT_S32_LE:
>> + pdata = 3;
>> + break;
>> + }
>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> + snd_soc_update_bits(codec, STA529_S2PCFG1, 0xC0, (pdata << 6));
>> + else
>> + snd_soc_update_bits(codec, STA529_P2SCFG1, 0xC0, (pdata << 6));
>> +
>> + return 0;
>> +}
>> +
>> +static int sta529_set_dai_fmt(struct snd_soc_dai *codec_dai, u32 fmt)
>> +{
>> + struct snd_soc_codec *codec = codec_dai->codec;
>> + u8 mode = 0;
>> +
>> + /* interface format */
>> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>> + case SND_SOC_DAIFMT_LEFT_J:
>> + mode = LEFT_J_DATA_FORMAT;
>> + break;
>> + case SND_SOC_DAIFMT_I2S:
>> + mode = I2S_DATA_FORMAT;
>> + break;
>> + case SND_SOC_DAIFMT_RIGHT_J:
>> + mode = RIGHT_J_DATA_FORMAT;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + mode |= 0x20;
>> + snd_soc_update_bits(codec, STA529_S2PCFG0, 0xE0, mode);
> What about the P2SCFG0 register, I would suspect, that the same data needs to
> be written to it.
>
Yes we need to update this.
>> +
>> + return 0;
>> +}
>> +
>> +static int sta529_mute(struct snd_soc_dai *dai, int mute)
>> +{
>> + struct snd_soc_codec *codec = dai->codec;
>> +
>> + u8 mute_reg = snd_soc_read(codec, STA529_FFXCFG0) & ~CODEC_MUTE_VAL;
>> +
>> + if (mute)
>> + mute_reg |= CODEC_MUTE_VAL;
>> +
>> + snd_soc_update_bits(codec, STA529_FFXCFG0, 0x80, 00);
> I guess, it should be mute_reg instead of 00
>
No, This is value, register is STA529_FFXCFG0
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +sta529_set_bias_level(struct snd_soc_codec *codec,
>> + enum snd_soc_bias_level level)
>> +{
>> + switch (level) {
>> + case SND_SOC_BIAS_ON:
>> + case SND_SOC_BIAS_PREPARE:
>> + snd_soc_update_bits(codec, STA529_FFXCFG0, POWER_CNTLMSAK,
>> + POWER_UP);
>> + snd_soc_update_bits(codec, STA529_MISC, 1, 0x01);
>> + break;
>> + case SND_SOC_BIAS_STANDBY:
>> + case SND_SOC_BIAS_OFF:
>> + snd_soc_update_bits(codec, STA529_FFXCFG0, POWER_CNTLMSAK,
>> + POWER_STDBY);
>> + /* Making FFX output to zero */
>> + snd_soc_update_bits(codec, STA529_FFXCFG0, FFX_MASK,
>> + FFX_OFF);
>> + snd_soc_update_bits(codec, STA529_MISC, 1, 0x00);
>> +
>> + break;
>> + }
>> +
>> + /*
>> + * store the label for powers down audio subsystem for suspend.This is
>> + * used by soc core layer
>> + */
>> + codec->dapm.bias_level = level;
>> + return 0;
>> +
>> +}
>> +
>> +static struct snd_soc_dai_ops sta529_dai_ops = {
> Can be const
It can not be. Please check snd_soc_dai_ops structure in include/sound/soc-dai.h
>> + .hw_params = sta529_hw_params,
>> + .set_fmt = sta529_set_dai_fmt,
>> + .digital_mute = sta529_mute,
>> +};
>> +
>> [..]
>> +
>> +static int sta529_probe(struct snd_soc_codec *codec)
>> +{
>> + struct sta529 *sta529 = snd_soc_codec_get_drvdata(codec);
>> + int ret;
>> +
>> + ret = snd_soc_codec_set_cache_io(codec, 8, 8, sta529->control_type);
> If your codec only supports I2C, you can pass SND_SOC_I2C here directly and get
> rid of the control_type field.
>
ok
>> + if (ret < 0) {
>> + dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
>> + return ret;
>> + }
>> + sta529_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
>> +
>> + snd_soc_add_controls(codec, sta529_snd_controls,
>> + ARRAY_SIZE(sta529_snd_controls));
>> +
>> + snd_soc_add_controls(codec, sta529_new_snd_controls,
>> + ARRAY_SIZE(sta529_new_snd_controls));
> You should use table based controls setup. i.e assign the control table to the
> 'controls' field of your codec_driver.
>
You can do it in either way.
>> + return 0;
>> +}
>> +
>> +/* power down chip */
>> +static int sta529_remove(struct snd_soc_codec *codec)
>> +{
>> + sta529_set_bias_level(codec, SND_SOC_BIAS_OFF);
>> +
>> + return 0;
>> +}
>> +
>> +static int sta529_suspend(struct snd_soc_codec *codec, pm_message_t state)
>> +{
>> + sta529_set_bias_level(codec, SND_SOC_BIAS_OFF);
>> +
>> + return 0;
>> +}
>> +
>> +static int sta529_resume(struct snd_soc_codec *codec)
>> +{
>> + snd_soc_cache_sync(codec);
>> + sta529_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
>> + sta529_set_bias_level(codec, codec->dapm.suspend_bias_level);
>> +
>> + return 0;
>> +}
>> +
>> +struct snd_soc_codec_driver soc_codec_dev_sta529 = {
>> + .probe = sta529_probe,
>> + .remove = sta529_remove,
>> + .set_bias_level = sta529_set_bias_level,
>> + .suspend = sta529_suspend,
>> + .resume = sta529_resume,
>> + .reg_cache_size = STA529_CACHEREGNUM,
>> + .reg_word_size = sizeof(u8),
>> + .reg_cache_default = sta529_reg,
>> +
>> +};
>> +
>> +static __devinit int sta529_i2c_probe(struct i2c_client *i2c,
>> + const struct i2c_device_id *id)
>> +{
>> + struct sta529 *sta529;
>> + int ret;
>> +
>> + if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> + return -EINVAL;
>> +
>> + sta529 = kzalloc(sizeof(struct sta529), GFP_KERNEL);
>> + if (sta529 == NULL)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(i2c, sta529);
>> + sta529->control_data = i2c;
>> + sta529->control_type = SND_SOC_I2C;
>> +
>> + ret = snd_soc_register_codec(&i2c->dev,
>> + &soc_codec_dev_sta529, &sta529_dai, 1);
>> + if (ret < 0)
>> + kfree(sta529);
>> + return ret;
>> +}
>> +
>> +static int sta529_i2c_remove(struct i2c_client *client)
> __devexit
>> +{
>> + snd_soc_unregister_codec(&client->dev);
>> + kfree(i2c_get_clientdata(client));
>> + return 0;
>> +}
>> +
>> [...]
>
> Your driver doesn't use any DAPM. You should at least define input and output
> pins and their routing, but I would expect that there is more that can be done,
> like dynamicly powering unused sections of the codec down, like the DAC or ADC.
> .
>
Right now since my driver has not support for DAPM, so definitions for input and output pins
are not there.Once I will implement DAPM feature in future I will send separate patch for that.
Best Rgds
Rajeev
More information about the Alsa-devel
mailing list