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.