[alsa-devel] [PATCH] ASoC: Add max98088 CODEC driver

[Peter Hsiang]

On Wed, Sep 29, 2010, Mark Brown wrote:
> On Wed, Sep 29, 2010 at 02:42:32PM -0700, Peter Hsiang wrote:
> > On Wed, Sep 29, 2010, Mark Brown wrote:
> > > On Tue, Sep 28, 2010 at 07:34:34PM -0700, Peter Hsiang wrote:
> 
> > > You should add value muxes like we have for DAPM.
> 
> > Please clarify what you mean by referencing the specific
> > code usage case in the dapm source module.
> 
> You're looking for the non-DAPM equivalent of SND_SOC_DAPM_VALUE_MUX().

This is a simple table lookup of a register value from the index
number given by SOC_ENUM, the same way it's been done in other drivers.

I found a "case snd_soc_dapm_value_mux:" in dapm_set_path_status()
Is this what you are referring to?
How is the code there relevant to this?

> 
> > > > +       /* powering down headphone gracefully */
> > > > +       status = snd_soc_read(codec, M98088_REG_4D_PWR_EN_OUT);
> > > > +       if ((status & M98088_HPEN) == M98088_HPEN) {
> > > > +               max98088_hw_write(codec, M98088_REG_4D_PWR_EN_OUT,
> > > > +                       (status & ~M98088_HPEN));
> > > > +       }
> > > > +       schedule_timeout(msecs_to_jiffies(20));
> 
> > > This looks rather like it should just be a post event implementing a
> > > timeout?
> 
> > This needs to work as a pre event.
> 
> Again, why is this?

When powering down the headphone, the way that DAPM works is it
likes to power off one item at a time, for example, the left channel,
then right channel.  The headphone hardware likes to see the 
headphone bits L and R be powered down together, for optimum result.
This works best with the pre method.  Powering up one channel at a 
time later is fine, when DAPM resumes.

> 
> > > > +       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;
> 
> > > 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?
> 
> The cache syncs should be part of some operation which would make it
> useful to sync the cache rather than just located at some point in the
> driver without any particular reason.  For example, with the drivers
> I've worked on the cache is synced after we enable the supplies for the
> device since the CODEC may have been powered off and therefore lost any
> register settings that might have been done.  If the cache sync is not
> associated with any such event then it's at best redundant and at worst
> the driver will loose some robustness since it becomes unclear if the
> events which cause a cache sync to be required are joined up with the
> triggering of a sync.

I see :)

> 
> > > > +module_init(max98088_init);
> 
> > > 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?
> 
> It's a global style for the kernel, though not enforced with 100%
> success.

Haha, got it. I will help you with this statistic from now on.