[alsa-devel] [PATCH v3] ASoC: add RT286 CODEC driver
Mark Brown
broonie at kernel.org
Wed Dec 18 20:20:40 CET 2013
On Wed, Dec 11, 2013 at 09:58:10AM +0800, bardliao at realtek.com wrote:
This is an improvement but there are still quite a few problems here.
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index bc12676..b31615c 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -48,6 +48,7 @@ snd-soc-pcm1792a-codec-objs := pcm1792a.o
> snd-soc-pcm3008-objs := pcm3008.o
> snd-soc-rt5631-objs := rt5631.o
> snd-soc-rt5640-objs := rt5640.o
> +snd-soc-rt286-objs := rt286.o
> snd-soc-sgtl5000-objs := sgtl5000.o
> snd-soc-alc5623-objs := alc5623.o
> snd-soc-alc5632-objs := alc5632.o
Keep this file sorted please.
> +static int rt286_hw_read(void *context, unsigned int reg, unsigned int *value)
To repeat what I said when reviewing the first version of this you
should be using regmap not open coding your register I/O. This looks
totally standard...
> +int rt286_jack_detect(struct snd_soc_codec *codec, bool *hp, bool *mic)
> +{
> + struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> + unsigned int val;
> + int i;
> +EXPORT_SYMBOL_GPL(rt286_jack_detect);
Use the standard jack detection APIs.
> + if (rt286->pdata.irq_en)
> + rt286_index_update_bits(codec,
> + NODE_ID_VENDOR_REGISTERS, 0x33, 0x0002, 0x0002);
Why would the system ever want to supply an interrupt and not use it?
> + /* Set configuration default */
> + rt286_write(codec, AC_VERB_SET_CONFIG_DEFAULT_BYTES_3,
> + NODE_ID_MIC1, 0x00);
> + rt286_write(codec, AC_VERB_SET_CONFIG_DEFAULT_BYTES_3,
> + NODE_ID_DMIC1, 0x00);
> + rt286_write(codec, AC_VERB_SET_CONFIG_DEFAULT_BYTES_3,
> + NODE_ID_DMIC2, 0x00);
Why are the silicon defaults not adequate?
> +static int rt286_hp_event(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_codec *codec = w->codec;
> + unsigned int val;
> +
> + switch (event) {
> + case SND_SOC_DAPM_POST_PMU:
> + val = snd_soc_read(codec, NODE_ID_HP_OUT) & 0x8080;
> + switch (val) {
> + case 0x0:
> + rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,
> + NODE_ID_HP_OUT, 0xb000);
> + break;
> + case 0x8000:
> + rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,
> + NODE_ID_HP_OUT, 0x9000);
> + break;
> + case 0x0080:
> + rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,
> + NODE_ID_HP_OUT, 0xa000);
> + break;
> + }
This is rather unclear. What is it doing?
> +static int rt286_set_dai_sysclk(struct snd_soc_dai *dai,
> + int clk_id, unsigned int freq, int dir)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> + struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> + pr_debug("%s freq=%d\n", __func__, freq);
dev_dbg().
> +static int rt286_set_bias_level(struct snd_soc_codec *codec,
> + enum snd_soc_bias_level level)
> +{
> + switch (level) {
> + case SND_SOC_BIAS_PREPARE:
> + if (SND_SOC_BIAS_STANDBY == codec->dapm.bias_level)
> + rt286_write(codec, AC_VERB_SET_POWER_STATE,
> + NODE_ID_AUDIO_FUNCTION_GROUP, AC_PWRST_D0);
> + break;
> +
> + case SND_SOC_BIAS_OFF:
> + rt286_write(codec, AC_VERB_SET_POWER_STATE,
> + NODE_ID_AUDIO_FUNCTION_GROUP, AC_PWRST_D3);
> + break;
These aren't matched - you power up when leaving standby but only power
down when you go to off. That's a bit odd and probably won't do the
right thing and will leave the CODEC powered up all the time since you
don't set idle_bias_off (though perhaps you should).
> +static int rt286_probe(struct snd_soc_codec *codec)
> +{
> + hw_configure(codec);
Just inline hw_configure(), there's no point having a separate function
with only one use in a tiny function.
> +static int rt286_remove(struct snd_soc_codec *codec)
> +{
> + rt286_set_bias_level(codec, SND_SOC_BIAS_OFF);
> + return 0;
> +}
The framework should power down the device for you.
> +struct snd_soc_dai_driver rt286_dai[] = {
> + {
> + .name = "rt286-aif1",
> + .id = RT286_AIF1,
> + .playback = {
> + .stream_name = "AIF1 Playback",
> + .channels_min = 1,
> + .channels_max = 2,
> + .rates = RT286_STEREO_RATES,
> + .formats = RT286_FORMATS,
> + },
> + .capture = {
> + .stream_name = "AIF1 Capture",
> + .channels_min = 1,
> + .channels_max = 2,
> + .rates = RT286_STEREO_RATES,
> + .formats = RT286_FORMATS,
> + },
> + .ops = &rt286_aif_dai_ops,
> + },
Your code handles up to 16 channels, and looking at the code I suspect
you need symmetric_rates.
> +
> + ret = devm_snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt286,
> + rt286_dai, ARRAY_SIZE(rt286_dai));
> + if (ret < 0)
> + goto err;
> +
> + return 0;
> +err:
> + return ret;
> +}
> +
> +static int rt286_i2c_remove(struct i2c_client *i2c)
> +{
> + snd_soc_unregister_codec(&i2c->dev);
Remove this, the whole point of devm_ is that it cleans up for you.
I'm also not seeing any code for interrupts even though there appears to
be some code to initialise them in the driver.
> +static int __init rt286_modinit(void)
> +{
> + return i2c_add_driver(&rt286_i2c_driver);
> +}
> +
> +module_init(rt286_modinit);
module_i2c_driver().
> +#define NODE_ID_AUDIO_FUNCTION_GROUP 0x01
> +#define NODE_ID_DAC_OUT1 0x02
> +#define NODE_ID_DAC_OUT2 0x03
> +#define NODE_ID_ADC_IN1 0x09
> +#define NODE_ID_ADC_IN2 0x08
> +#define NODE_ID_MIXER_IN 0x0b
> +#define NODE_ID_MIXER_OUT1 0x0c
> +#define NODE_ID_MIXER_OUT2 0x0d
> +#define NODE_ID_DMIC1 0x12
> +#define NODE_ID_DMIC2 0x13
> +#define NODE_ID_SPK_OUT 0x14
> +#define NODE_ID_MIC1 0x18
> +#define NODE_ID_LINE1 0x1a
> +#define NODE_ID_BEEP 0x1d
> +#define NODE_ID_SPDIF 0x1e
> +#define NODE_ID_VENDOR_REGISTERS 0x20
> +#define NODE_ID_HP_OUT 0x21
> +#define NODE_ID_MIXER_IN1 0x22
> +#define NODE_ID_MIXER_IN2 0x23
> +#define TOTAL_NODE_ID 0x23
> +#define CONNECTION_INDEX_MIC1 0X0
> +#define CONNECTION_INDEX_DMIC 0X5
These need namespacing if they're device specific.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20131218/38c5d944/attachment.sig>
More information about the Alsa-devel
mailing list