Re: [RFC v1 5/9] ASoC: msm8x16: add codec intialization setup

[Mark Brown]

On Tue, Feb 16, 2016 at 05:33:03PM +0000, Srinivas Kandagatla wrote:

> +	if (micbias1 && micbias2) {
> +		if ((wcd->micbias1_cap_mode == MICBIAS_EXT_BYP_CAP) ||
> +		    (wcd->micbias2_cap_mode == MICBIAS_EXT_BYP_CAP))
> +			snd_soc_update_bits(codec,
> +				MSM8X16_WCD_A_ANALOG_MICB_1_EN,
> +				0x40, (MICBIAS_EXT_BYP_CAP << 6));
> +		else
> +			snd_soc_update_bits(codec,
> +				MSM8X16_WCD_A_ANALOG_MICB_1_EN,
> +				0x40, (MICBIAS_NO_EXT_BYP_CAP << 6));

This is a bit of an exciting combination of magic numbers and #defines.

> +#define VDDIO_VOL_MIN	1800000 /* uV */
> +#define VDDIO_VOL_MAX	1800000 /* uV */

> +		mclk_rate = clk_get_rate(msm8x16_wcd->mclk);
> +
> +		if (mclk_rate == 12288000)
> +			snd_soc_update_bits(codec,
> +				MSM8X16_WCD_A_CDC_TOP_CTL, 0x01, 0x00);
> +		else if (mclk_rate == 9600000)
> +			snd_soc_update_bits(codec,
> +				MSM8X16_WCD_A_CDC_TOP_CTL, 0x01, 0x01);

This looks like it should be a switch statement.

> +static const struct msm8x16_wcd_reg_mask_val
> +	msm8x16_wcd_codec_reg_init_val[] = {

We use these tables...

> +	snd_soc_write(codec, MSM8X16_WCD_A_DIGITAL_PERPH_RESET_CTL4, 0x01);
> +	snd_soc_write(codec, MSM8X16_WCD_A_ANALOG_PERPH_RESET_CTL4, 0x01);
> +
> +	for (reg = 0; reg < ARRAY_SIZE(msm8x16_wcd_reset_reg_defaults); reg++)
> +		if (msm8x16_wcd_reg_readable[reg])
> +			msm8x16_wcd_write(codec,
> +				reg, msm8x16_wcd_reset_reg_defaults[reg]);

...and also sequences of explicit register writes somewhat at random as
far as I can see.  What is going on?

> +	/* delay is required to make sure sound card state updated */
> +	usleep_range(5000, 5100);
> +
> +	for (reg = 0; reg < ARRAY_SIZE(msm8x16_wcd_codec_reg_init_val); reg++)
> +		snd_soc_update_bits(codec,
> +				    msm8x16_wcd_codec_reg_init_val[reg].reg,
> +				    msm8x16_wcd_codec_reg_init_val[reg].mask,
> +				    msm8x16_wcd_codec_reg_init_val[reg].val);

It looks like you need delays in the table.  Perhaps using the regmap
sequence code might be a better fit?

> +	msm8x16_wcd_codec_enable_clock_block(codec, 1);

Separate enable and disable functions if you are going to do this though
with one caller each it's not clear why this is a separate function.

> +static int msm8x16_wcd_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct msm8x16_wcd_chip *chip = dev_get_drvdata(codec->dev);
> +	int err;
> +
> +	regulator_set_voltage(chip->vddio, VDDIO_VOL_MIN, VDDIO_VOL_MAX);

Why are we doing a _set_voltage() here?  We never vary the voltage, we
support only one value and apparently we don't even care if it worked.

> +	err = regulator_enable(chip->vddio);
> +	if (err < 0) {
> +		dev_err(codec->dev, "failed to enable VDDIO regulator\n");
> +		return err;
> +	}
> +
> +	err = regulator_enable(chip->vdd_tx_rx);
> +	if (err < 0) {
> +		dev_err(codec->dev, "failed to enable VDD_TX_RX regulator\n");
> +		return err;
> +	}

These two look like they should be handled with bulk operations (the bit
getting the regulators is in a completely different patch...).  There's
also no disable code.

> +	chip->pmic_rev = snd_soc_read(codec, MSM8X16_WCD_A_DIGITAL_REVISION1);
> +	dev_info(codec->dev, "%s :PMIC REV: %d", __func__, chip->pmic_rev);

Please format the log messages so the look like normal kernel output.

Attachment: signature.asc
Description: PGP signature