On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:This driver is child of spmi bus so, we need SPMI dependency here along with SYSCON.
+config SND_SOC_MSM8916_WCD
+ tristate "Qualcomm MSM8916 WCD"
+ depends on SPMI && MFD_SYSCON
+
Normally users select MFD_SYSCON.
Yep will fix it.@@ -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.
Yes, there are DAI id's which are used in device trees.
+#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.
Yep Will remove it. I guess this is already done in the core..
+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!
I will make sure that I comment it properly in next version.+ 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.
Yep, I will relook at this.
+ /*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.
Yep, sounds good, Will fix it in next version.
+ 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.
Yep, we can move to using bulk* apis.
+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.
Yep. I will re-order the
+ 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 separateYes, the code is written in a way that there is no separate existence hiding the register map split in the read/write wrappers.
existence independent of the broader IP.