Re: [alsa-devel] [PATCH] ASoC: codecs: Add support for AK4458 DAC driver
On Wed, Jan 31, 2018 at 03:20:09PM +0200, Cosmin-Gabriel Samoila wrote:
This looks pretty good overall, I've got some issues below but nothing too major:
+static int ak4458_i2c_remove(struct i2c_client *i2c) +{
- ak4458_remove(&i2c->dev);
- pm_runtime_disable(&i2c->dev);
It's weird that the runtime PM handling is here and not in the shared code, and that it only exists in the I2C version. Why is this?
+static const struct soc_enum ak4458_dac_enum[] = { +/*0*/ SOC_ENUM_SINGLE(AK4458_01_CONTROL2, 1,
ARRAY_SIZE(ak4458_dem_select_texts),ak4458_dem_select_texts),+/*1*/ SOC_ENUM_SINGLE(AK4458_0A_CONTROL6, 0,
The fact that you need these comments is why these arrays are a bad idea - just use individually named variables as other drivers do.
+static const struct snd_kcontrol_new ak4458_snd_controls[] = {
- SOC_SINGLE_TLV("AK4458 L1ch Digital Volume",
AK4458_03_LCHATT, 0/*shift*/, 0xFF/*max value*/,0/*invert*/, latt_tlv),- SOC_SINGLE_TLV("AK4458 R1ch Digital Volume",
AK4458_04_RCHATT, 0, 0xFF, 0, ratt_tlv),
It'd be more idiomatic to combine these into stereo pairs than have them as single channel controls.
+static const char * const ak4458_dac_select_texts[] = { "OFF", "ON" };
This looks like the users should be switch controls - what's the goal here?
- rstn = snd_soc_read(codec, AK4458_00_CONTROL1);
- rstn &= ~AK4458_RSTN_MASK;
- if (bit)
rstn |= AK4458_RSTN;- snd_soc_write(codec, AK4458_00_CONTROL1, rstn);
This looks like an open coded snd_soc_update_bits()?
+static int ak4458_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- struct ak4458_priv *ak4458 = snd_soc_codec_get_drvdata(codec);
- u8 format;
- int pcm_width = max(params_physical_width(params), ak4458->slot_width);
+#ifdef AK4458_ACKS_USE_MANUAL_MODE
- u8 dfs1, dfs2;
+#endif
What's this and why is it a compile time option?
- case 32:
if (ak4458->fmt == SND_SOC_DAIFMT_I2S)format |= AK4458_DIF_32BIT_I2S;else if (ak4458->fmt == SND_SOC_DAIFMT_LEFT_J)format |= AK4458_DIF_32BIT_MSB;else if (ak4458->fmt == SND_SOC_DAIFMT_RIGHT_J)format |= AK4458_DIF_32BIT_LSB;else if (ak4458->fmt == SND_SOC_DAIFMT_DSP_B)format |= AK4458_DIF_32BIT_MSB;elsereturn -EINVAL;
This should be a switch statement.
- switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
- case SND_SOC_DAIFMT_I2S:
- case SND_SOC_DAIFMT_LEFT_J:
- case SND_SOC_DAIFMT_RIGHT_J:
ak4458->fmt = fmt & SND_SOC_DAIFMT_FORMAT_MASK;break;- case SND_SOC_DAIFMT_DSP_B:
ak4458->fmt = fmt & SND_SOC_DAIFMT_FORMAT_MASK;break;
All these cases seem to be the same?
+static int ak4458_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *codec_dai)+{
- int ret = 0;
- dev_dbg(codec_dai->dev, "%s(%d)\n", __func__, __LINE__);
- return ret;
+}
Remove empty functions.
- ak4458->mute_gpio = of_get_named_gpio(np, "ak4458,mute_gpio", 0);
- if (gpio_is_valid(ak4458->mute_gpio)) {
Given that this is new code it'd be better to use GPIO descriptors, this will also allow support for non-DT systems - use devm_gpiod_get() and the matching APIs.
Hi Mark,
+static const struct soc_enum ak4458_dac_enum[] = { +/*0*/ SOC_ENUM_SINGLE(AK4458_01_CONTROL2, 1,
ARRAY_SIZE(ak4458_dem_select_texts),ak4458_dem_select_texts),+/*1*/ SOC_ENUM_SINGLE(AK4458_0A_CONTROL6, 0,
The fact that you need these comments is why these arrays are a bad idea
- just use individually named variables as other drivers do.
+static const struct snd_kcontrol_new ak4458_snd_controls[] = {
- SOC_SINGLE_TLV("AK4458 L1ch Digital Volume",
AK4458_03_LCHATT, 0/*shift*/, 0xFF/*maxvalue*/,
0/*invert*/, latt_tlv),- SOC_SINGLE_TLV("AK4458 R1ch Digital Volume",
AK4458_04_RCHATT, 0, 0xFF, 0, ratt_tlv),It'd be more idiomatic to combine these into stereo pairs than have them as single channel controls.
+static const char * const ak4458_dac_select_texts[] = { "OFF", "ON" };
This looks like the users should be switch controls - what's the goal here?
I think this is to allow users to switch off sound for all channels but it seems silly to have 4 controls doing the same thing. We now have two options: - one control to switch off sound for all channels - one controll per DAC
What do you mean by "users should be switch controls" ? Should we use SND_SOC_DAPM_SWITCH instead of SND_SOC_DAPM_MUX?
We are still learing the inner details of ASoC. Thank you for your help.
Cosmin
On Fri, Feb 09, 2018 at 10:22:16AM +0000, Cosmin Samoila wrote:
+static const char * const ak4458_dac_select_texts[] = { "OFF", "ON" };
This looks like the users should be switch controls - what's the goal here?
I think this is to allow users to switch off sound for all channels but it seems silly to have 4 controls doing the same thing. We now have two options:
- one control to switch off sound for all channels
- one controll per DAC
If they're mutes they shouldn't be in DAPM at all, just make them standard controls - they should stack up with the matching volume controls.
What do you mean by "users should be switch controls" ? Should we use SND_SOC_DAPM_SWITCH instead of SND_SOC_DAPM_MUX?
They quite clearly aren't muxes so if they need to be in DAPM probably switches but it's not clear that they need to be in DAPM.
Hi Mark,
+static int ak4458_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct snd_soc_codec *codec = dai->codec;
- struct ak4458_priv *ak4458 =
snd_soc_codec_get_drvdata(codec);
- u8 format;
- int pcm_width = max(params_physical_width(params), ak4458-
slot_width);
+#ifdef AK4458_ACKS_USE_MANUAL_MODE
- u8 dfs1, dfs2;
+#endif
What's this and why is it a compile time option?
This is used to set the codec in Manual Setting Mode, meaning that the default sampling speed is set based on DFS0 and DFS1 bits. In Auto Setting Mode, the MCLK frequency is detected automatically and DFS bits are ignored. This compile time option is used to know when to set the ACKS bit in init_reg function that switches between modes. At this moment we are only using Auto Mode and I can remove it if you think is not necessary.
Thank you, Cosmin
On Tue, Feb 13, 2018 at 09:22:24AM +0000, Cosmin Samoila wrote:
This is used to set the codec in Manual Setting Mode, meaning that the default sampling speed is set based on DFS0 and DFS1 bits. In Auto Setting Mode, the MCLK frequency is detected automatically and DFS bits are ignored.
But why is it a compile time option?
This compile time option is used to know when to set the ACKS bit in init_reg function that switches between modes. At this moment we are only using Auto Mode and I can remove it if you think is not necessary.
Yes, just remove it - if there needs to be some control it should probably be done by the machine driver, or through DT.
participants (2)
-
Cosmin Samoila -
Mark Brown