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;
> + else
> + return -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.
Attachment:
signature.asc
Description: PGP signature
