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));elsesnd_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.