Re: [PATCH 001/001] ASoC: Replace max98090 Device Driver

Re: [PATCH 001/001] ASoC: Replace max98090 Device Driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 10104 bytes --]

On Thu, Dec 13, 2012 at 11:19:29AM -0800, Jerry Wong wrote:

For legibility please submit future revisions as a two stage patch
series, one removing the old driver and another adding the new one.

> +/* codec platform data */
> +struct max98090_pdata {
> +
> +       int irq;

This should be passed in using the normal mechanism provided by the bus,
not custom implemented.

> +       int power_over_performance;

This doesn't look like platform data to me, it looks like something that
should be configured dynamically at runtime.

> +       /* Equalizers for DAC */
> +       struct max98090_eq_cfg *eq_cfg;
> +       unsigned int eq_cfgcnt;

Coefficients should be configured using SND_SOC_BYTES().  Some older
drivers use this method but it's deprecated.

> +       { 0xE0, 0x00 }, /* E0 */
> +       { 0xE1, 0x00 }, /* E1 */
> +       { 0xE2, 0x00 }, /* E2 */

Do these number-only registers actually exist in the published register
map or are they undefined?  If they're undefined and not used by
software there's no need to include them here.

> +       switch (reg) {
> +       case M98090_REG_01_IRQ_STATUS:
> +       case M98090_REG_02_JACK_STATUS:
> +       case M98090_REG_FF_REV_ID:

Including the register number in the constants seems to rather defeat
the point of having symbolic names for the registers; it means people
still have to remember the register numbers.

> +static int max98090_get_enab_tlv(struct snd_kcontrol *kcontrol,
> +                               struct snd_ctl_elem_value *ucontrol)

What's the relevance of _tlv here?  It looks like these are normal get
and sets.

> +static const char * max98090_dsts_text[] =
> +       { "None", "Left", "Right", "Left and Right" };
> +
> +static const struct soc_enum max98090_dsts_enum =
> +       SOC_ENUM_SINGLE(M98090_REG_1A_LVL_SIDETONE, M98090_DSTS_SHIFT,
> +               ARRAY_SIZE(max98090_dsts_text), max98090_dsts_text);


> +static const struct snd_kcontrol_new max98090_snd_controls[] = {

> +       SOC_SINGLE("DMIC MIC Left Enable", M98090_REG_13_MIC_CFG1,
> +               M98090_DIGMICL_SHIFT, M98090_DIGMICL_NUM - 1, 0),
> +       SOC_SINGLE("DMIC MIC Right Enable", M98090_REG_13_MIC_CFG1,
> +               M98090_DIGMICR_SHIFT, M98090_DIGMICR_NUM - 1, 0),

This looks like it should be DAPM.

> +       SOC_ENUM_EXT("External MIC Mux", max98090_extmic_mux_enum,
> +               max98090_extmic_mux_get, max98090_extmic_mux_set),

Likewise.

> +       SOC_ENUM("Digital Sidetone Source", max98090_dsts_enum),

DAPM.

> +       SOC_SINGLE_EXT_TLV("Digital Sidetone Level Volume",
> +               M98090_REG_1A_LVL_SIDETONE, M98090_DVST_SHIFT,
> +               M98090_DVST_NUM - 1, 1, max98090_get_enab_tlv,
> +               max98090_put_enab_tlv, max98090_micboost_tlv),

Level Volume?

> +       SOC_SINGLE_TLV("Digital Gain Volume", M98090_REG_27_DAI_LVL,
> +               M98090_DAI_LVL_DVG_SHIFT, M98090_DAI_LVL_DVG_NUM - 1, 0,
> +               max98090_dvg_tlv),

Gain Volume?

> +       SOC_SINGLE("Digital EQ 3 Band Enable", M98090_REG_41_DSP_EQ_EN,
> +               M98090_EQ3BANDEN_SHIFT, M98090_EQ3BANDEN_NUM - 1, 0),

Switch.

> +       SOC_SINGLE("DAC HP Playback Performance Mode", M98090_REG_43_DAC_CFG,
> +               M98090_DAC_PERFMODE_SHIFT, M98090_DAC_PERFMODE_NUM - 1, 0),
> +       SOC_SINGLE("DAC High Performance Mode", M98090_REG_43_DAC_CFG,
> +               M98090_DACHP_SHIFT, M98090_DACHP_NUM - 1, 0),

These should either be enumerations with real names of Switches if it's
just an on/off (looks like the former).

> +       SOC_SINGLE_TLV("Headphone Left Volume", M98090_REG_2C_LVL_HP_LEFT,
> +               M98090_HPVOLL_SHIFT, M98090_HPVOLL_NUM - 1, 0,
> +               max98090_hp_tlv),
> +       SOC_SINGLE_TLV("Headphone Right Volume", M98090_REG_2D_LVL_HP_RIGHT,
> +               M98090_HPVOLR_SHIFT, M98090_HPVOLR_NUM - 1, 0,
> +               max98090_hp_tlv),

Should be a single stereo control (and similarly for the other output
controls).

> +       SOC_SINGLE("Quick Setup System Clock", M98090_REG_04_QCFG_SYS_CLK,
> +               M98090_CLK_ALL_SHIFT, M98090_CLK_ALL_NUM - 1, 0),

What do these controls do?


> +       if (val >= 1) {
> +               if (w->reg == M98090_REG_10_LVL_MIC1)
> +                       max98090->mic1pre = val - 1; /* Update for volatile */
> +               else
> +                       max98090->mic2pre = val - 1; /* Update for volatile */
> +       }

Use braces consistently within an if statemeent - if you use them for
the outer one use them for the inner one too.

> +static const struct snd_soc_dapm_route max98090_dapm_routes[] = {
> +
> +       {"MIC1 Input", NULL, "MIC1"},
> +       {"MIC2 Input", NULL, "MIC2"},
> +       {"MIC1 Input", NULL, "MICBIAS"},
> +       {"MIC2 Input", NULL, "MICBIAS"},

Unless this chip is *very* unusual I expect the bias is something that's
connected externally and therefore these connections should be being
made by the machine driver.

> +
> +       /* Check for user calculated MI and NI ratios */
> +       for (i = 0; i < ARRAY_SIZE(user_pclk_rates); i++) {
> +               if ((user_pclk_rates[i] == max98090->sysclk) &&
> +                       (user_lrclk_rates[i] == max98090->lrclk)) {
> +                       dev_info(codec->dev,
> +                               "Found user supported PCLK to LRCLK rates\n");
> +                       dev_info(codec->dev, "i %d ni %lld mi %lld\n",
> +                               i, ni_value[i], mi_value[i]);

dev_dbg() would be fine but dev_info() is too loud.

> +/*
> + * This accommodates an inverted logic in the MAX98090 chip
> + * for Bit Clock Invert (BCI). The inverted logic is only seen
> + * for the case of TDM mode. The remaining cases have normal logic.
> + */
> +               if (max98090->tdm_slots > 1) {
> +                       regval ^= M98090_DAI_BCI_MASK;
> +               }

Coding style, the indentation is inconsistent here and no braces for
single line if () statements.

> +               if (max98090->jack_state == M98090_JACK_STATE_HEADSET) {
> +                       /*
> +                        * Set to normal bias level.
> +                        */
> +                       snd_soc_update_bits(codec, M98090_REG_12_MIC_BIAS,
> +                               M98090_MBVSEL_MASK, M98090_MBVSEL_2V4);
> +
> +                       snd_soc_update_bits(codec, M98090_REG_3E_PWR_EN_IN,
> +                               M98090_PWR_MBEN_MASK, M98090_PWR_MBEN_MASK);
> +               }

This looks suspicous, why does the headset state have any bearing here?

> +static int max98090_dai_digital_mute(struct snd_soc_dai *codec_dai, int mute)
> +{
> +       struct snd_soc_codec *codec = codec_dai->codec;
> +       int regval;
> +
> +       dev_info(codec->dev, "max98090_dai_digital_mute\n");

Drop this, it's not conveying any useful information.

> +static void max98090_dmic_switch(struct snd_soc_codec *codec, int state)
> +{

I have no idea what this function is supposed to do...

> +static void max98090_headset_button_event(struct snd_soc_codec *codec)
> +{
> +       dev_info(codec->dev, "max98090_headset_button_event\n");
> +}

Delete this, it doesn't do anything except log.

> +       switch (reg & (M98090_LSNS_MASK | M98090_JKSNS_MASK)) {
> +               case M98090_LSNS_MASK | M98090_JKSNS_MASK:
> +               {
> +                       dev_info(codec->dev, "No Headset Detected\n");
> +
> +                       max98090->jack_state = M98090_JACK_STATE_NO_HEADSET;
> +
> +                       max98090_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +
> +                       snd_soc_dapm_disable_pin(&codec->dapm, "HPL");
> +                       snd_soc_dapm_disable_pin(&codec->dapm, "HPR");
> +                       snd_soc_dapm_force_enable_pin(&codec->dapm, "SPKL");
> +                       snd_soc_dapm_force_enable_pin(&codec->dapm, "SPKR");
> +                       snd_soc_dapm_disable_pin(&codec->dapm, "MIC1");
> +                       snd_soc_dapm_disable_pin(&codec->dapm, "MIC2");
> +                       snd_soc_dapm_force_enable_pin(&codec->dapm, "DMIC1");
> +                       snd_soc_dapm_force_enable_pin(&codec->dapm, "DMIC2");
> +                       max98090_dmic_switch(codec, 1);
> +
> +                       break;
> +               }

This doesn't look obviously sensible...  calling set_bias_level()
outside of DAPM is going to get overridden in pretty short order,
none of the DAPM pin setting looks like a good idea (especially not
forcing on the speakers whenever there's no headset) and there's no DAPM
sync to make any of this take effect.  I'm a bit confused about the
intended effect.  I also can't see any interaction with soc-jack...

What is this code actually intended to do?

> +       dev_info(codec->dev, "***** max98090_interrupt *****\n");
> +

There's lots of these noisy, uninformative prints at info level
throughout the driver.

> +       /* Send work to be scheduled */
> +       if (active & M98090_IRQ_CLD_MASK) {
> +               dev_info(codec->dev, "M98090_IRQ_CLD_MASK\n");
> +       }

This doesn't actually schedule anything...

> +       ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);
> +       if (ret != 0) {
> +               dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> +               return ret;
> +       }

SND_SOC_REGMAP.

> +       if ( (request_threaded_irq(pdata->irq, NULL,
> +               max98090_interrupt, IRQF_TRIGGER_FALLING,
> +               "max98090_interrupt", codec)) < 0) {
> +               dev_info(codec->dev, "request_irq failed\n");
> +       }
> +       /*
> +        * Clear any old interrupts.

Coding style and it's not terribly helpful to hide the return code from
users.

> +       /* Turn on VCM bandgap reference */
> +       snd_soc_write(codec, M98090_REG_42_BIAS_CNTL,
> +               M98090_VCM_MODE_MASK);

This shouldn't be managed at runtime?

> +/*
> + * Driver revision
> + */
> +#define MAX98090_REVISION                      "0.01.00"

The kernel already has perfectly good versioning.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]