No subject

The code logic here is different from snd_soc_update_bits,
and therefore snd_soc_update_bits can not be used for this
like you suggested.

> > +
> > +static const unsigned int ex_mode_table[] =3D {
> > +       0x00,           /* disabled */
> > +       (0<<4)|3,       /* 100Hz */
> > +       (1<<4)|0,       /* 400Hz */
> > +       (2<<4)|0,       /* 600Hz */
> > +       (3<<4)|0,       /* 800Hz */
> > +       (4<<4)|0,       /* 1000Hz */
> > +       (1<<4)|1,       /* 200-400Hz */
> > +       (2<<4)|2,       /* 400-600Hz */
> > +       (3<<4)|2,       /* 400-800Hz */
> > +};
>=20
> You should add value muxes like we have for DAPM.

Please clarify what you mean by referencing the specific=20
code usage case in the dapm source module.

>=20
> > +static const char *max98088_extmic[] =3D {
> > +       "Off",
> > +       "MIC1",
> > +       "MIC2",
> > +};
>=20
> > +static const struct soc_enum max98088_extmic_enum[] =3D {
> > +       SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(max98088_extmic), max98088_extmi=
c),
> > +};
>=20
> This looks like it should be in DAPM.

This has nothing to do with power management. =20
I guess you were confused by the word "Off" in the options list.
The "Off" case means the input for external mic is not used,
i.e. disconnected.  It does not power up or down anything.

>=20
> > +static int max98088_mic1pre_set(struct snd_kcontrol *kcontrol,
> > +                               struct snd_ctl_elem_value *ucontrol)
> > +{
> > +       struct snd_soc_codec *codec =3D snd_kcontrol_chip(kcontrol);
> > +       struct max98088_priv *max98088 =3D snd_soc_codec_get_drvdata(co=
dec);
> > +       unsigned int *mode =3D &max98088->mic1pre;
> > +       int sel =3D ucontrol->value.integer.value[0];
> > +
> > +       if (sel >=3D ARRAY_SIZE(max98088_micpre))
> > +               return -EINVAL;
> > +
> > +       *mode =3D ucontrol->value.integer.value[0];
> > +       return 0;
> > +}
>=20
> I'd expect that some action would be taken when the value is set here.
> All this does is set a variable, changing the control will have no
> effect.

The register field works like this:
00 =3D preamp disabled
01 =3D 0dB
10 =3D 20dB
11 =3D 30dB
So the gain and the power setting are in the same bit field.
DAPM owns the power control.  Here the user settings are stored,
and they get picked up by DAPM.

>=20
> > +static int max98088_hp_event(struct snd_soc_dapm_widget *w,
> > +                            struct snd_kcontrol *kcontrol, int event)
> > +{
> > +       struct snd_soc_codec *codec =3D w->codec;
> > +       u16 status;
> > +
> > +       BUG_ON(event !=3D SND_SOC_DAPM_PRE_PMD);
> > +
> > +       /* powering down headphone gracefully */
> > +       status =3D snd_soc_read(codec, M98088_REG_4D_PWR_EN_OUT);
> > +       if ((status & M98088_HPEN) =3D=3D M98088_HPEN) {
> > +               max98088_hw_write(codec, M98088_REG_4D_PWR_EN_OUT,
> > +                       (status & ~M98088_HPEN));
> > +       }
> > +       schedule_timeout(msecs_to_jiffies(20));
>=20
> This looks rather like it should just be a post event implementing a
> timeout?

This needs to work as a pre event.

>=20
> > +       if (rate !=3D cdata->rate) {
> > +               /* set DAI1 SR1 value for the DSP; FREQ1:0=3Danyclock *=
/
> > +               if (rate_value(rate, &regval))
> > +                       return -EINVAL;
> > +
> > +               snd_soc_write(codec, M98088_REG_11_DAI1_CLKMODE, regval=
);
> > +               cdata->rate =3D rate;
> > +       }
>=20
> Just use snd_soc_update_bits() to suppress unneeded writes.  Many of the
> operaations have this issue.

The full value for this register is accounted for here and so there
is no need to use snd_soc_update_bits for this case.

>=20
> > +               & M98088_DAI_MAS) {
> > +               if (max98088->sysclk =3D=3D 0)
> > +                       return -EINVAL;
>=20
> You should print an error here so users can tell what went wrong.

Yes thank you.

>=20
> > +       case SND_SOC_BIAS_STANDBY:
> > +               max98088_sync_cache(codec);
> > +               snd_soc_update_bits(codec, M98088_REG_4C_PWR_EN_IN,
> > +                               M98088_MBEN, M98088_MBEN);
> > +               break;
>=20
> Do you really want to sync the cache *every* time you go into standby?

The sync_cache function itself will just return if the
codec->cache_sync flag is cleared from the first time it ran.
You do the exact same thing in your codec driver...
What is the change that you are suggesting?

>=20
> > +module_init(max98088_init);
>=20
> Normally this would be next to the function it references.

Is this a new formatting style of the kernel now all across,
or is this a personal preference?
Not a problem, I can change it.  Just that I find a huge number=20
of drivers in the kernel having the module_init and module_exit=20
together at the very end.