Messages in this thread | | | Date | Tue, 16 Feb 2016 19:23:46 +0000 | From | Mark Brown <> | Subject | Re: [RFC v1 5/9] ASoC: msm8x16: add codec intialization setup |
| |
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. [unhandled content-type:application/pgp-signature] | |