On Fri, Feb 13, 2015 at 10:35:42PM -0800, anish wrote:
> On Fri, Feb 13, 2015 at 9:14 PM, Mark Brown <broonie@kernel.org> wrote:

> >> +     switch (reg) {
> >> +     case MAX98925_R000_VBAT_DATA:

> > Putting the register numbers in the define for the register name seems
> > to defeat some of the point of the defines...

> Looked at the other driver and everyone is using the same way.
> Sorry but didn't get your point.

Nobody else has the register numbers as part of the names defined for
the registers which was what my point was.

> >> +     max98925_regmap_update_bits_stereo(max98925,
> >> +                     MAX98925_R01B_DAI_CLK_MODE2,
> >> +                     M98925_DAI_BSEL_MASK, M98925_DAI_BSEL_64);

> > This looks to be a mix of setting slave mode and setting some clocking
> > configuration that ought to be configurable - for example I'd expect
> > clock sources to be configured via set_sysclk().

> Slave mode also should be configured in set_sysclk? I will move
> bclk and lrclk setting to set_sysclk.

Not slave mode but the other bits.

> >> +             max98925_regmap_update_bits_stereo(max98925,
> >> +                     MAX98925_R036_BLOCK_ENABLE,
> >> +                     M98925_BST_EN_MASK |
> >> +                     M98925_ADC_IMON_EN_MASK | M98925_ADC_VMON_EN_MASK,
> >> +                     M98925_BST_EN_MASK |
> >> +                     M98925_ADC_IMON_EN_MASK | M98925_ADC_VMON_EN_MASK);
> >> +
> >> +             max98925_regmap_write_stereo(max98925,
> >> +                     MAX98925_R038_GLOBAL_ENABLE, M98925_EN_MASK);
> >> +     }
> >
> > This doesn't look like it's muting, it looks like it's doing a whole
> > bunch of other things.  The mute operation should mute and only mute
> > (and then only if the device usefully supports it).

> Coming to think of it, you are right. I am wondering if this(turning on voltage)
> and current along with global audio enable bit) should
> be dapm widget as it should always be turned on whenever
> playback starts. I think i will make it a callback of
> SND_SOC_DAPM_DAC_E

If it's really a global enable used in all cases set_bias_level() is
probably the place to do it - it's for chip wide stuff.  If there are
some cases where it's not needed then DAPM widgets (normally a supply
widget) are the best place.