On 06/09/16 12:17, Mark Brown wrote:

On Tue, Sep 06, 2016 at 10:57:40AM +0100, Srinivas Kandagatla wrote:

quoted

+static const struct soc_enum hph_enum =
+SOC_ENUM_SINGLE_VIRT(ARRAY_SIZE(hph_text), hph_text);

Coding style, let's have some indentation please.

I think Lindent did some mess up here, I will take of of such 
occurrences in next version.

quoted

+		if (w->reg == CDC_A_TX_1_EN)
+			snd_soc_update_bits(codec, CDC_D_CDC_CONN_TX1_CTL,
+					    CONN_TX1_SERIAL_TX1_MUX,
+					    CONN_TX1_SERIAL_TX1_ADC_1);
+		else if ((w->reg == CDC_A_TX_2_EN) || (w->reg == CDC_A_TX_3_EN))
+			snd_soc_update_bits(codec, CDC_D_CDC_CONN_TX2_CTL,
+					    CONN_TX2_SERIAL_TX2_MUX,
+					    CONN_TX2_SERIAL_TX2_ADC_2);

This looks like it should be a switch statement.

Yep, will fix this in next version.

quoted

+	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].def);

quoted

+static int msm8916_wcd_analog_enable_spk_pa(struct snd_soc_dapm_widget *w,
+					    struct snd_kcontrol *kcontrol,
+					    int event)

We had some DAPM operations earlier, then the probe now we're back to
DAPM operations.  Seems a bit confused.

I agree, I will reorder these functions to make the code more readable.

quoted

+	//Path domain ???

C style comments please.

Will fix it.

quoted

+static int msm8916_wcd_analog_startup(struct snd_pcm_substream *substream,
+				      struct snd_soc_dai *dai)
+{
+
+	snd_soc_update_bits(dai->codec, CDC_A_MASTER_BIAS_CTL,
+			    MASTER_BIAS_CTL_MASTER_BIAS_EN_MASK |
+			    MASTER_BIAS_CTL_V2L_BUFFER_EN_MASK,
+			    MASTER_BIAS_CTL_MASTER_BIAS_EN_ENABLE |
+			    MASTER_BIAS_CTL_V2L_BUFFER_EN_ENABLE);
+
+	snd_soc_update_bits(dai->codec, CDC_D_CDC_RST_CTL,
+			    RST_CTL_DIG_SW_RST_N_MASK,
+			    RST_CTL_DIG_SW_RST_N_REMOVE_RESET);
+
+	return 0;
+}

I'm not quite sure what this is doing but it looks like something I'd
expect to be in DAPM and I can't see anything that undoes this on
teardown either.

I will fix this in next version, where this thing will be teardown in 
shutdown callback.

quoted

+static int msm8916_wcd_analog_parse_dt(struct device *dev,
+				       struct msm8916_wcd_analog_priv *priv)
+{
+	struct device_node *np = dev->of_node;
+	int ret, i;
+	u32 res[2];
+
+	ret = of_property_read_u32_array(np, "reg", res, 2);
+	if (ret < 0)
+		return ret;

That's weird, why are we reading a reg property directly?  Not that we
ever look at the value we parsed...

Ah.. looks like left over from my previous patches.. will get rid of 
this in next version.

quoted

+static const struct of_device_id msm8916_wcd_analog_spmi_match_table[] = {
+	{.compatible = "qcom,msm8916-wcd-analog-codec"},
+	{}
+};

Coding style, let's have somme spaces please.

Ok.