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.
signature.asc
Description: PGP signature
