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

[Schulman, James]

On Wed, 14 Nov 2018, Mark Brown wrote:

> 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.
>

Ok we will switch to a _SINGLE control.

>>>> +    }
>>>> +
>>>> +    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?
>

We make the assumption that this is not the case. This handler is modeled 
after our other upstreamed drivers (cs35l35.c). If there is a better 
example please let me know and we can try to be more consistent.A