[alsa-devel] [EXTERNAL] Re: [PATCH 1/2] ASoC: cs35l36: Add support for Cirrus CS35L36 Amplifier

[Mark Brown]

On Wed, Nov 14, 2018 at 06:54:02PM +0000, Schulman, James wrote:
> On Tue, 13 Nov 2018, Mark Brown wrote:
> > On Tue, Nov 13, 2018 at 12:49:16PM -0600, James Schulman wrote:

> >> +static const struct snd_kcontrol_new cs35l36_boost_mux[] = {
> >> +    SOC_DAPM_ENUM("Boost Enable", boost_enum),
> >> +};

> > Simple on/off conttrols should be _SINGLE controls with Switch at the
> > end of their name.  It's not super clear why this is in DAPM as a widget
> > at all, the routing around it is:

> > +       {"BOOST Mux", "On", "Channel Mux"},
> > +       {"CLASS H", NULL, "BOOST Mux"},

> > which is a bit perplexing.  Possibly this should be handled in the event
> > for the main amplifier?

> We used _ENUM instead of _SINGLE because we wanted the default value to be
> "On" instead of "Off". We don't want the default assumption to have boost
> turned off. However, we're happy to change this if you would rather have
> consitency with the _SINGLE controls.

I don't follow this at all.  Setting a default value for a control is
not related to the type of the control.

> >> +    /*
> >> +     * Rev B0 has 2 versions
> >> +     * L36 is 10V
> >> +     * L37 is 12V

> > \o/

> Yes we had 2 silicon spins for this revision >_< is there anything about
> this comment you want us to change?

You could more explicitly explain what happened here, revisions and
spins are generally synonymous so it's confusing.

> >> +    }
> >> +
> >> +    return IRQ_HANDLED;

> > This unconditionally reports that it handled the interrupt, even if it
> > didn't.  This will stop the interrupt core handling for faulty interrupt
> > lines working and will disrupt any support that gets added for handling
> > shared threaded interrupts.

> Earlier in the handler, we have a check to see if any unmasked bits are
> set. If not we return IRQ_NONE. I figured this would be enough to ensure
> we don't unconditionally report IRQ_HANDLED.

What if something you didn't expect got unmasked by some bug?

> I think we will add an err_disable_regs jump statement to ensure we don't
> try to use the uninitialized reset_gpio handle. Once we do that, I think
> this will handle -EPROBE_DEFER cleanly?

Yes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20181114/38429b65/attachment.sig>