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.