Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
> On 2/25/20 11:54 AM, Andy Shevchenko wrote:
> > On Tue, Feb 25, 2020 at 11:27:52AM +0100, Hans de Goede wrote:

...

> > In general I'm fine with this, but looking to the history of your changes I'm
> > afraid that in future it will require more than one pin to be listed or
> > something like this.
> 
> The only models which need this so far are the weird HP X2 models which
> use an external embedded controller with the tablet version of BYT / CHT
> which is just al sorts of hacked together. Also see:
> 
> https://lore.kernel.org/stable/20200223153208.312005-1-hdegoede@xxxxxxxxxx/T/#u
> 
> For other parts of the same device which also rather "hacked together"
> HP made these models really really interesting...
> 
> With that said I cannot guarantee that we won't need something similar
> for some other botched-up device.

...

> > > +static int ignore_wake = IGNORE_WAKE_AUTO;
> > > +module_param(ignore_wake, int, 0444);
> > > +MODULE_PARM_DESC(ignore_wake,
> > > +	"Ignore ACPI wake flag: x=ignore-for-pin-x, -1=auto, -2=all, -3=none");
> > 
> > Perhaps we may take list of pins or a bitmap (see bitmap list parsers API).
> 
> I guess you mean bitmap_parse_user / bitmap_print_to_pagebuf, the problem
> is that for a more generic solution we need a wat to specify the
> GPIO controller + the pin, so we would get a list of <name>,<pin> pairs
> and then need to parse that, e.g. :
> 
> 	gpiolib_acpi.ignore_wake=INT33FC:00,0x1c;INT33FC:01;0x12
> 
> I agree that if we really want to future proof this that then this is
> the way we should go. This does mean adding a bunch of extra code for
> parsing this, but I guess that would be better then my current hack.
> 
> Please let me know if you prefer going this route then I will respin
> the patches to work this way.

Let's do it as a list of pairs, but in slightly different format (I see some
potential to derive a generic parser, based on users described in
Documentation/admin-guide/kernel-parameters.txt), i.e.

	ignore_wake=pin:controller[,pin:controller[,...]]

...

> > > -MODULE_PARM_DESC(honor_wakeup,
> > > -		 "Honor the ACPI wake-capable flag: 0=no, 1=yes, -1=auto");

> > Isn't it now a part of ABI? I don't think we may remove it, though we may ignore it.
> > Or do something else.
> > 
> > (One of the reasons why I hate module parameters)
> 
> I personally do not subscribe to the module parameters are part of the kernel ABI
> crowd. I do not think Linus has ever stated something like that ?  For long existing
> often used module parameters treating them as such makes a ton of sense, but this
> one is quite new and AFAIK almost nobody is using it. So my vote would be to just
> drop it. If we get push back we can easily restore it in some form.

I'm fine with dropping it, perhaps add a phrase to the commit message about
(safe) removal.

-- 
With Best Regards,
Andy Shevchenko