Re: [PATCH V4 1/5] sound: asoc: Adding support for STA529 Audio Codec

[Lars-Peter Clausen <lars@metafoo.de>]

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

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

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

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

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

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

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