Messages in this thread | | | Date | Tue, 14 Jun 2016 16:59:38 +0100 | From | Mark Brown <> | Subject | Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver |
| |
On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:
> +config SND_SOC_MSM8916_WCD > + tristate "Qualcomm MSM8916 WCD" > + depends on SPMI && MFD_SYSCON > +
Normally users select MFD_SYSCON.
> @@ -208,7 +209,6 @@ snd-soc-wm9705-objs := wm9705.o > snd-soc-wm9712-objs := wm9712.o > snd-soc-wm9713-objs := wm9713.o > snd-soc-wm-hubs-objs := wm_hubs.o > - > # Amp > snd-soc-max9877-objs := max9877.o > snd-soc-tpa6130a2-objs := tpa6130a2.o
Spurious whitespace change.
> +#include "msm8916-wcd-registers.h" > +#include "msm8916-wcd.h" > +#include "dt-bindings/sound/msm8916-wcd.h"
What's in here? There weren't any constants in the bindings.
> +struct msm8916_wcd_chip { > + struct regmap *analog_map; > + struct regmap *digital_map; > + unsigned int analog_offset; > + u16 pmic_rev; > + u16 codec_version;
Why is this one device and not two devices? The description indicated that this was two separate bits of silicon.
> +static int msm8916_wcd_write(struct snd_soc_codec *codec, unsigned int reg, > + unsigned int val) > +{ > + int ret = -EINVAL; > + struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev); > + u8 *cache = codec->reg_cache; > + > + if (!msm8916_wcd_reg_readonly[reg]) > + cache[reg] = val;
Why is the driver open coding a cache? Don't do that!
> + case SND_SOC_DAPM_POST_PMU: > + if (w->shift == 5) > + snd_soc_update_bits(codec, LPASS_CDC_RX1_B6_CTL, > + RXn_B6_CTL_MUTE_MASK, 0); > + else if (w->shift == 4) > + snd_soc_update_bits(codec, LPASS_CDC_RX2_B6_CTL, > + RXn_B6_CTL_MUTE_MASK, 0);
Switch statement.
> + widget_name = kstrndup(w->name, 15, GFP_KERNEL); > + if (!widget_name) > + return -ENOMEM; > + temp = widget_name; > + > + dec_name = strsep(&widget_name, " "); > + widget_name = temp; > + if (!dec_name) { > + dev_err(codec->dev, "Invalid decimator = %s\n", w->name); > + ret = -EINVAL; > + goto out; > + } > + > + dec_num = strpbrk(dec_name, "12"); > + if (dec_num == NULL) { > + dev_err(codec->dev, "Invalid Decimator\n"); > + ret = -EINVAL; > + goto out; > + } > + > + ret = kstrtouint(dec_num, 10, &decimator); > + if (ret < 0) { > + dev_err(codec->dev, "Invalid decimator = %s\n", dec_name); > + ret = -EINVAL; > + goto out; > + }
I'm not terribly clear what this is doing, it probably needs some comments explaining what's going on at the very least.
> + /*RX stuff */ > + SND_SOC_DAPM_AIF_IN("I2S RX1", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_AIF_IN("I2S RX2", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0), > + SND_SOC_DAPM_AIF_IN("I2S RX3", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
Use DAPM routes to connect the widgets in, don't name the DAI in the widget.
> + mclk_rate = clk_get_rate(msm8916_wcd->mclk); > + > + if (mclk_rate == 12288000) > + snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL, > + TOP_CTL_DIG_MCLK_FREQ_MASK, > + TOP_CTL_DIG_MCLK_FREQ_F_12_288MHZ); > + > + else if (mclk_rate == 9600000) > + snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL, > + TOP_CTL_DIG_MCLK_FREQ_MASK, > + TOP_CTL_DIG_MCLK_FREQ_F_9_6MHZ);
Switch statement, and this should also handle unexpected rates.
> +static int msm8916_wcd_codec_probe(struct snd_soc_codec *codec) > +{ > + struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev); > + int err, reg; > + > + err = regulator_enable(chip->vddio); > + if (err < 0) { > + dev_err(codec->dev, > + "failed to enable VDDIO regulator (%d)\n", err); > + return err; > + } > + > + err = regulator_enable(chip->vdd_tx_rx); > + if (err < 0) { > + dev_err(codec->dev, > + "failed to enable VDD_TX_RX regulator (%d)\n", err); > + regulator_disable(chip->vddio); > + return err; > + }
Why is this not using regulator_bulk_enable()? I'd also expect to see most if not all of this initial setup stuff in the main device probe.
> + if (TOMBAK_IS_1_0(chip->pmic_rev)) { > + for (reg = 0; reg < ARRAY_SIZE(wcd_reg_defaults); reg++) > + snd_soc_write(codec, wcd_reg_defaults[reg].reg, > + wcd_reg_defaults[reg].val); > + } else { > + for (reg = 0; reg < ARRAY_SIZE(wcd_reg_defaults_2_0); reg++) > + snd_soc_write(codec, wcd_reg_defaults_2_0[reg].reg, > + wcd_reg_defaults_2_0[reg].val); > + }
Please reset the chip properly.
> + ret = clk_prepare_enable(chip->mclk); > + if (ret < 0) { > + dev_err(dev, "failed to enable mclk %d\n", ret); > + return ret; > + }
Runtime PM?
> +static const struct of_device_id msm8916_wcd_match_table[] = { > + {.compatible = "qcom,msm8916-pmic-wcd-codec"}, > + {} > +};
We were peering inside the parent for the register map, why does this appear in the device tree as a separate device? Both the patch description and that code suggest that it doesn't really have a separate existence independent of the broader IP. [unhandled content-type:application/pgp-signature] | |