[alsa-devel] [PATCH 2/4] acpi: allow for an override to set _REV

[Dominik Brodowski]

On Mon, May 18, 2015 at 03:01:32AM +0200, Rafael J. Wysocki wrote:
> On Sunday, May 17, 2015 07:41:44 PM Dominik Brodowski wrote:
> > On Thu, May 14, 2015 at 11:55:33 PM Rafael J. Wysocki wrote:
> > > On Thursday, May 14, 2015 10:36:52 PM Rafael J. Wysocki wrote:
> > > > So the only value that would really make sense here is 5.
> > 
> > > Overall, what about the appended patch instead of your [2-3/4] (modulo the
> > > new command line parameter description which is missing here ATM)?
> > 
> > Well, this approach works as well -- limiting it to an override for just 5
> > seems reasonable; expanding blacklist.c to also cover this case (even though
> > it's not a blacklisting per se) isn't worth any discussion.
> > 
> > Nonetheless, a few specifics:
> > 
> > > +config ACPI_REV_OVERRIDE_POSSIBLE
> > 
> > Why should that be a config option at all? The code savings should be
> > really, really tiny;
> 
> The idea is not about the code savings, but about having a simple way to disable
> the whole thing entirely at one point.
> 
> All of the workarounds under this option *including* the command line switch
> should be temporary.

Hopefully, yes. But I am not convinced about that yet (see below).

> > and especially in the beginning we might see a few
> > machines where testing the override might seem to be a good idea. So I'd
> > favour having the command line optional, and then only specific quirks
> > behind a config option: For the Dell XPS 13 it makes sense to disable the
> > quirk if userspace can manage i2s sound; for other systems, there may not be
> > such hope. And as this is a machine-specific decision, I fear that we have
> > to do CONFIG options for each and every such DMI entry.
> 
> I'm not sure if we need a config option for Dell in particular.  We can simply
> drop the quirk when it is not necessary any more.

The quirk for the Dell XPS 13 (2015), yes. Once userspace catches up (which
may take _years_). But Matthew was mentioning that "[t]he Inspiron 7437
queries _REV and uses it to modify its EC behaviour, and apparently breaks
on Linux without that." If that is indeed the case, we will need a quirk
for that Inspiron for much, much longer than for the Dell XPS 13 (2015).

Therefore, I see a need to distinguish the quirk for the Dell XPS 13 (2015),
the quirk for the Inspiron 7437 and potential quirks for other systems: Some
may depend on _sound_ userspace being up to date (XPS), some may depend on
_other_ parts of userspace being up to date, and some may be needed for
as long as these systems exist (Inspiron 7437?). And if the userspace for
the XPS is up to date, the quirk for that particular issue may not be needed
any more, while other quirks (such as potentially the one for the 7437) may
still be needed -- that is why I see a need for different CONFIG options.

> > > +#ifdef CONFIG_ACPI_REV_OVERRIDE_POSSIBLE
> > > +static bool acpi_rev_override;
> > > +
> > > +int __init acpi_rev_override_setup(char *str)
> > > +{
> > > +	acpi_rev_override = true;
> > > +	return 1;
> > > +}
> > > +__setup("acpi_rev_override", acpi_rev_override_setup);
> > > +#else
> > > +#define acpi_rev_override	false
> > > +#endif
> > 
> > Why __setup, and not module_param? Should mean a smaller diffstat...
> 
> Because it is consistent with the other __setup things done in this file
> (and none of them is a module_param, mind you).

module_param's aren't unusual in drivers/acpi/* and have substantial
advantages IMO, but I also see the issue of consistency -- so if you really
do prefer __setup, I'll use that approach in the next version of the patch.

> > +config ACPI_REV_OVERRIDE_DELL_XPS_13_2015
> 
> I'm still not seeing the point and this is just !@#$%^&*()_+ ugly.

I _know_ it's ugly. But this whole suggested change of _REV behavior after
3+ years is ugly, and its fallout may very well be considerably larger than
is known at the moment: It all depends on whether there are other platforms
out there which need such quirks, and whether these quirks are needed for
different timeframes than the quirk for the Dell XPS 13 (2015).

Best,
	Dominik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150518/3d36d607/attachment-0001.sig>