Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> For full support we need:
> 
> 1) invoke special SMBIOS call for some machine
> 
> 2) patch dell-wmi.c to do not drop some events for some machines
> 
> Part 2) needs to touch only dell-wmi.c code, this is obvious. But I
> thought that part 1) will be done in dell-laptop.c code where are all
> others SMBIOS calls. Reason is just because dell-wmi.c is doing WMI
> stuff. Do you want to include this is part 1) also to dell-wmi.c file?

Yes, that was my idea.  That SMBIOS call is basically WMI initialization
code.  Consider the case of Vostro V131 - there is no WMI on that
machine without that SMBIOS call and there is no need to issue that
SMBIOS call if you don't want to use WMI.  Given that dell-wmi does not
depend on dell-laptop, consider the extreme case where the user selects
CONFIG_DELL_LAPTOP=n and CONFIG_DELL_WMI=m, expecting WMI to work.  If
we put the WMI-enabling SMBIOS call in dell-laptop, it won't.  Also,
once my Dell SMBIOS API rework series gets applied, adding the relevant
call to dell-wmi will be easy.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> For full support we need:
> 
> 1) invoke special SMBIOS call for some machine
> 
> 2) patch dell-wmi.c to do not drop some events for some machines
> 
> Part 2) needs to touch only dell-wmi.c code, this is obvious. But I
> thought that part 1) will be done in dell-laptop.c code where are all
> others SMBIOS calls. Reason is just because dell-wmi.c is doing WMI
> stuff. Do you want to include this is part 1) also to dell-wmi.c file?

Yes, that was my idea.  That SMBIOS call is basically WMI initialization
code.  Consider the case of Vostro V131 - there is no WMI on that
machine without that SMBIOS call and there is no need to issue that
SMBIOS call if you don't want to use WMI.  Given that dell-wmi does not
depend on dell-laptop, consider the extreme case where the user selects
CONFIG_DELL_LAPTOP=n and CONFIG_DELL_WMI=m, expecting WMI to work.  If
we put the WMI-enabling SMBIOS call in dell-laptop, it won't.  Also,
once my Dell SMBIOS API rework series gets applied, adding the relevant
call to dell-wmi will be easy.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 21 January 2016 15:56:12 Michał Kępień wrote:
> > > > Michał, can you prepare new (v3) version of this patch? Now required
> > > > acpi video changes are included and so dell-wmi changes should go to...
> > > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > > 
> > > I keep this on my to-do list, but the updated patch will depend on the
> > > final version of the SMBIOS API rework, so I guess there is little point
> > > in posting it now as that API is subject to change.  But rest assured
> > > that as soon as the final version of the API rework series (which I have
> > > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > > this patch - after all, it is the very reason I am working on the API
> > > rework.
> > 
> > There is still need to patch dell-wmi.c? And this change does not depend
> > on another SMBIOS change (in dell-laptop), right?
> 
> Well, back in December, you wrote [1]:
> 
> > This patch is not enough for enabling 0xe025 key on that Vostro machine.
> > Some extra SMBIOS call is needed, without them ACPI will not send WMI
> > keypress event.
> > 
> > (...)
> > 
> > Maybe now it could make sense to unify Dell SMBIOS API in kernel and
> > move common functions to one place and let drivers to use just common
> > functions. According to older Dell ACPI WMI documentation in DMI is bit
> > which specify if BIOS support SMBIOS via WMI or not.
> > 
> > At least I think this one patch should not be included into kernel until
> > there will be full support for 0xe025 key (adding that SMBIOS call).
> 
> From the above I understood that first you want to unify the Dell SMBIOS
> API used throughout the kernel (that's currently in progress), so that
> it can then be used in dell-wmi as well to perform the SMBIOS call
> needed on the Vostro V131.
> 
> If you want me to just rework the patch so that it doesn't introduce a
> quirk structure, I recalled another reason to use it after all: there
> are other Dell laptops which require the special SMI for enabling WMI
> events, but report the Dell Instant Launch Hotkey using a different WMI
> event code [2].  So I'd say that two separate issues should be fixed
> using DMI matching:
> 
>   * whether the special SMBIOS call is required,
>   * whether the 0x0e25 event should be translated into a keypress.
> 
> A quirk structure looks like an elegant way to deal with this.
> 
> Could you please advise how you would like me to proceed with this?
> 
> [1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12
> 

Hm...

For full support we need:

1) invoke special SMBIOS call for some machine

2) patch dell-wmi.c to do not drop some events for some machines

Part 2) needs to touch only dell-wmi.c code, this is obvious. But I
thought that part 1) will be done in dell-laptop.c code where are all
others SMBIOS calls. Reason is just because dell-wmi.c is doing WMI
stuff. Do you want to include this is part 1) also to dell-wmi.c file?

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > > Michał, can you prepare new (v3) version of this patch? Now required
> > > acpi video changes are included and so dell-wmi changes should go to...
> > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > 
> > I keep this on my to-do list, but the updated patch will depend on the
> > final version of the SMBIOS API rework, so I guess there is little point
> > in posting it now as that API is subject to change.  But rest assured
> > that as soon as the final version of the API rework series (which I have
> > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > this patch - after all, it is the very reason I am working on the API
> > rework.
> 
> There is still need to patch dell-wmi.c? And this change does not depend
> on another SMBIOS change (in dell-laptop), right?

Well, back in December, you wrote [1]:

> This patch is not enough for enabling 0xe025 key on that Vostro machine.
> Some extra SMBIOS call is needed, without them ACPI will not send WMI
> keypress event.
> 
> (...)
> 
> Maybe now it could make sense to unify Dell SMBIOS API in kernel and
> move common functions to one place and let drivers to use just common
> functions. According to older Dell ACPI WMI documentation in DMI is bit
> which specify if BIOS support SMBIOS via WMI or not.
> 
> At least I think this one patch should not be included into kernel until
> there will be full support for 0xe025 key (adding that SMBIOS call).

>From the above I understood that first you want to unify the Dell SMBIOS
API used throughout the kernel (that's currently in progress), so that
it can then be used in dell-wmi as well to perform the SMBIOS call
needed on the Vostro V131.

If you want me to just rework the patch so that it doesn't introduce a
quirk structure, I recalled another reason to use it after all: there
are other Dell laptops which require the special SMI for enabling WMI
events, but report the Dell Instant Launch Hotkey using a different WMI
event code [2].  So I'd say that two separate issues should be fixed
using DMI matching:

  * whether the special SMBIOS call is required,
  * whether the 0x0e25 event should be translated into a keypress.

A quirk structure looks like an elegant way to deal with this.

Could you please advise how you would like me to proceed with this?

[1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 21 January 2016 11:52:03 Michał Kępień wrote:
> > Michał, can you prepare new (v3) version of this patch? Now required
> > acpi video changes are included and so dell-wmi changes should go to...
> > To finally fix this keypress bug on Dell Vostro V131 machine.
> 
> I keep this on my to-do list, but the updated patch will depend on the
> final version of the SMBIOS API rework, so I guess there is little point
> in posting it now as that API is subject to change.  But rest assured
> that as soon as the final version of the API rework series (which I have
> yet to prepare, of course) gets applied by Darren, I will post a v3 of
> this patch - after all, it is the very reason I am working on the API
> rework.

There is still need to patch dell-wmi.c? And this change does not depend
on another SMBIOS change (in dell-laptop), right?

I still have in my mind some changes for dell-wmi.c code but I'm waiting
until all other changes for dell-wmi.c will be merged to prevent
conflicts and needed rebase.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> Michał, can you prepare new (v3) version of this patch? Now required
> acpi video changes are included and so dell-wmi changes should go to...
> To finally fix this keypress bug on Dell Vostro V131 machine.

I keep this on my to-do list, but the updated patch will depend on the
final version of the SMBIOS API rework, so I guess there is little point
in posting it now as that API is subject to change.  But rest assured
that as soon as the final version of the API rework series (which I have
yet to prepare, of course) gets applied by Darren, I will post a v3 of
this patch - after all, it is the very reason I am working on the API
rework.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

Michał, can you prepare new (v3) version of this patch? Now required
acpi video changes are included and so dell-wmi changes should go to...
To finally fix this keypress bug on Dell Vostro V131 machine.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> Michał, can you prepare new (v3) version of this patch? Now required
> acpi video changes are included and so dell-wmi changes should go to...
> To finally fix this keypress bug on Dell Vostro V131 machine.

I keep this on my to-do list, but the updated patch will depend on the
final version of the SMBIOS API rework, so I guess there is little point
in posting it now as that API is subject to change.  But rest assured
that as soon as the final version of the API rework series (which I have
yet to prepare, of course) gets applied by Darren, I will post a v3 of
this patch - after all, it is the very reason I am working on the API
rework.

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 21 January 2016 11:52:03 Michał Kępień wrote:
> > Michał, can you prepare new (v3) version of this patch? Now required
> > acpi video changes are included and so dell-wmi changes should go to...
> > To finally fix this keypress bug on Dell Vostro V131 machine.
> 
> I keep this on my to-do list, but the updated patch will depend on the
> final version of the SMBIOS API rework, so I guess there is little point
> in posting it now as that API is subject to change.  But rest assured
> that as soon as the final version of the API rework series (which I have
> yet to prepare, of course) gets applied by Darren, I will post a v3 of
> this patch - after all, it is the very reason I am working on the API
> rework.

There is still need to patch dell-wmi.c? And this change does not depend
on another SMBIOS change (in dell-laptop), right?

I still have in my mind some changes for dell-wmi.c code but I'm waiting
until all other changes for dell-wmi.c will be merged to prevent
conflicts and needed rebase.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

Michał, can you prepare new (v3) version of this patch? Now required
acpi video changes are included and so dell-wmi changes should go to...
To finally fix this keypress bug on Dell Vostro V131 machine.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > > Michał, can you prepare new (v3) version of this patch? Now required
> > > acpi video changes are included and so dell-wmi changes should go to...
> > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > 
> > I keep this on my to-do list, but the updated patch will depend on the
> > final version of the SMBIOS API rework, so I guess there is little point
> > in posting it now as that API is subject to change.  But rest assured
> > that as soon as the final version of the API rework series (which I have
> > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > this patch - after all, it is the very reason I am working on the API
> > rework.
> 
> There is still need to patch dell-wmi.c? And this change does not depend
> on another SMBIOS change (in dell-laptop), right?

Well, back in December, you wrote [1]:

> This patch is not enough for enabling 0xe025 key on that Vostro machine.
> Some extra SMBIOS call is needed, without them ACPI will not send WMI
> keypress event.
> 
> (...)
> 
> Maybe now it could make sense to unify Dell SMBIOS API in kernel and
> move common functions to one place and let drivers to use just common
> functions. According to older Dell ACPI WMI documentation in DMI is bit
> which specify if BIOS support SMBIOS via WMI or not.
> 
> At least I think this one patch should not be included into kernel until
> there will be full support for 0xe025 key (adding that SMBIOS call).

>From the above I understood that first you want to unify the Dell SMBIOS
API used throughout the kernel (that's currently in progress), so that
it can then be used in dell-wmi as well to perform the SMBIOS call
needed on the Vostro V131.

If you want me to just rework the patch so that it doesn't introduce a
quirk structure, I recalled another reason to use it after all: there
are other Dell laptops which require the special SMI for enabling WMI
events, but report the Dell Instant Launch Hotkey using a different WMI
event code [2].  So I'd say that two separate issues should be fixed
using DMI matching:

  * whether the special SMBIOS call is required,
  * whether the 0x0e25 event should be translated into a keypress.

A quirk structure looks like an elegant way to deal with this.

Could you please advise how you would like me to proceed with this?

[1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12

-- 
Best regards,
Michał Kępień

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 21 January 2016 15:56:12 Michał Kępień wrote:
> > > > Michał, can you prepare new (v3) version of this patch? Now required
> > > > acpi video changes are included and so dell-wmi changes should go to...
> > > > To finally fix this keypress bug on Dell Vostro V131 machine.
> > > 
> > > I keep this on my to-do list, but the updated patch will depend on the
> > > final version of the SMBIOS API rework, so I guess there is little point
> > > in posting it now as that API is subject to change.  But rest assured
> > > that as soon as the final version of the API rework series (which I have
> > > yet to prepare, of course) gets applied by Darren, I will post a v3 of
> > > this patch - after all, it is the very reason I am working on the API
> > > rework.
> > 
> > There is still need to patch dell-wmi.c? And this change does not depend
> > on another SMBIOS change (in dell-laptop), right?
> 
> Well, back in December, you wrote [1]:
> 
> > This patch is not enough for enabling 0xe025 key on that Vostro machine.
> > Some extra SMBIOS call is needed, without them ACPI will not send WMI
> > keypress event.
> > 
> > (...)
> > 
> > Maybe now it could make sense to unify Dell SMBIOS API in kernel and
> > move common functions to one place and let drivers to use just common
> > functions. According to older Dell ACPI WMI documentation in DMI is bit
> > which specify if BIOS support SMBIOS via WMI or not.
> > 
> > At least I think this one patch should not be included into kernel until
> > there will be full support for 0xe025 key (adding that SMBIOS call).
> 
> From the above I understood that first you want to unify the Dell SMBIOS
> API used throughout the kernel (that's currently in progress), so that
> it can then be used in dell-wmi as well to perform the SMBIOS call
> needed on the Vostro V131.
> 
> If you want me to just rework the patch so that it doesn't introduce a
> quirk structure, I recalled another reason to use it after all: there
> are other Dell laptops which require the special SMI for enabling WMI
> events, but report the Dell Instant Launch Hotkey using a different WMI
> event code [2].  So I'd say that two separate issues should be fixed
> using DMI matching:
> 
>   * whether the special SMBIOS call is required,
>   * whether the 0x0e25 event should be translated into a keypress.
> 
> A quirk structure looks like an elegant way to deal with this.
> 
> Could you please advise how you would like me to proceed with this?
> 
> [1] http://www.spinics.net/lists/platform-driver-x86/msg07845.html
> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1205791/comments/12
> 

Hm...

For full support we need:

1) invoke special SMBIOS call for some machine

2) patch dell-wmi.c to do not drop some events for some machines

Part 2) needs to touch only dell-wmi.c code, this is obvious. But I
thought that part 1) will be done in dell-laptop.c code where are all
others SMBIOS calls. Reason is just because dell-wmi.c is doing WMI
stuff. Do you want to include this is part 1) also to dell-wmi.c file?

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Andy Lutomirski]

On Fri, Dec 4, 2015 at 4:55 AM, Michał Kępień  wrote:
>> > On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
>> > Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
>> > is generated.  As there seems to be no ACPI method or SMI call to
>> > determine without possible side effects whether a given machine is
>> > capable of simulating a keypress when this hotkey is pressed, DMI
>> > matching is used to whitelist the models for which an input event should
>> > be generated when WMI event 0xe025 is received.
>> >
>> > Signed-off-by: Michał Kępień 
>> > ---
>> > Changes from v1:
>> >
>> >   - Use DMI matching instead of a module parameter
>> >   - Change flag name to improve readability
>> >
>> > Darren, I am aware this patch conflicts with Andy's WMI rework series
>> > posted yesterday, so please just let me know if you want me to rebase.
>>
>> Thanks for the heads' up. Andy's series is longer and likely going to need 
>> more
>> review from more people, so I don't necessarily want to hold this one up on 
>> it -
>> unless it makes sense to include it in that series so they can evolve 
>> together.
>> I'll leave that to you and Andy to decide.
>
> As merging my patch with Andy's work is just a matter of moving code
> around, not actually changing it, personally I don't feel this patch
> should be somehow linked to the WMI rework.  Andy, please let me know if
> you disagree, I don't really have any strong feelings about this.
>'

I think it's just a matter of which lands first.  It shouldn't be a big deal.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> This patch is not enough for enabling 0xe025 key on that Vostro machine.
> Some extra SMBIOS call is needed, without them ACPI will not send WMI
> keypress event.

Indeed.  But have you read the last e-mail I wrote before submitting the
original patch [1]?  Brightness control on the V131 is already broken
"out of the box" with newer kernels (flickering upon brightness change),
but if we do what you're suggesting and include the SMI call in the
kernel, we'll break it even more, to the point where pressing one of the
brightness control keys might not result in any brightness change at
all.  Sure, we can fix that by overriding an arbitrary ACPI method.  Oh,
wait, did I say "fix"?

I posted the patch without the SMI call because that way if you want to
use the Dell Instant Launch hotkey, you just fire up a userspace script
(which uses libsmbios and takes care of overriding the ACPI method) and
chances are you will end up with a fully functional system.  Of course
you need to understand that using this script is not an elegant solution
and that it might break something else, but it's your choice, not the
kernel's.  And the patch itself does not change kernel's default
behavior, so we're not risking breaking any other models out there.

> At least I think this one patch should not be included into kernel until
> there will be full support for 0xe025 key (adding that SMBIOS call).

Again, fully supporting the Dell Instant Launch hotkey makes brightness
control even more broken than it has to be.  In other words, everything
is terrible.

The only real solution to all these issues is a BIOS fix and I'm pretty
sure it's not happening.

[1] http://www.spinics.net/lists/platform-driver-x86/msg07679.html

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> > Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> > is generated.  As there seems to be no ACPI method or SMI call to
> > determine without possible side effects whether a given machine is
> > capable of simulating a keypress when this hotkey is pressed, DMI
> > matching is used to whitelist the models for which an input event should
> > be generated when WMI event 0xe025 is received.
> > 
> > Signed-off-by: Michał Kępień 
> > ---
> > Changes from v1:
> > 
> >   - Use DMI matching instead of a module parameter
> >   - Change flag name to improve readability
> > 
> > Darren, I am aware this patch conflicts with Andy's WMI rework series
> > posted yesterday, so please just let me know if you want me to rebase.
> 
> Thanks for the heads' up. Andy's series is longer and likely going to need 
> more
> review from more people, so I don't necessarily want to hold this one up on 
> it -
> unless it makes sense to include it in that series so they can evolve 
> together.
> I'll leave that to you and Andy to decide.

As merging my patch with Andy's work is just a matter of moving code
around, not actually changing it, personally I don't feel this patch
should be somehow linked to the WMI rework.  Andy, please let me know if
you disagree, I don't really have any strong feelings about this.

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -47,6 +47,38 @@ static int acpi_video;
> >  
> >  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
> >  
> > +struct quirk_entry {
> > +   bool process_dell_instant_launch;
> > +};
> > +
> > +static struct quirk_entry *quirks;
> > +
> > +static struct quirk_entry quirk_unknown = {
> > +};
> > +
> > +static struct quirk_entry quirk_dell_vostro_v131 = {
> > +   .process_dell_instant_launch = true,
> > +};
> > +
> > +static int __init dmi_matched(const struct dmi_system_id *dmi)
> > +{
> > +   quirks = dmi->driver_data;
> > +   return 1;
> > +}
> > +
> > +static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
> > +   {
> > +   .callback = dmi_matched,
> > +   .ident = "Dell Vostro V131",
> > +   .matches = {
> > +   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +   DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> > +   },
> > +   .driver_data = _dell_vostro_v131,
> > +   },
> > +   { }
> > +};
> > +
> 
> It is not possible to simplify this part of code? Currently we only need
> boolean variable: ignore 0xe025 event or not. I think that whole quirk
> structure is not needed yet (and I would be happy if never in future).

Of course it is possible.  I just got the feeling that using a quirk
structure is the way to go for this subsystem as it currently contains 7
drivers using something like this.  Moreover, I saw that in some commits
initially adding a quirk structure to a driver (2d8b90be, a979e2e2) that
structure contained just one field.  

So, between KISS and following the beaten path, I chose the latter.  If
you still think this patch should be reworked to use a single global
boolean instead, please let me know and I'll prepare a v3.

> > @@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
> > return -ENODEV;
> > }
> >  
> > +   quirks = _unknown;
> 
> Unknown sounds like something is not know or we do not know what it is.
> But here we know exactly what is needed (= ignore 0xe025 key).

Again, not my idea, I just thought it would be wise to follow what seems
to be an established pattern:

$ git grep 'quirk.*unknown' drivers/platform/x86/

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 03 December 2015 17:16:06 Darren Hart wrote:
> This looks fine to me, and if Pali will ack it, I'll move it from for-review 
> to
> testing and Andy will need to update patch 14/14 to accomodate - unless you 
> guys
> decide to include this in his.

This patch is not enough for enabling 0xe025 key on that Vostro machine.
Some extra SMBIOS call is needed, without them ACPI will not send WMI
keypress event.

Dell SMBIOS call can be done either via WMI or via SMI inb/outb
instructions which implements dcdbas.ko driver. Module dell-laptop.ko
uses only SMBIOS API for all functionality and uses dcdbas.ko.

There is another driver which uses SMBIOS API, but use WMI calls. It is
dell-led.ko in leds subsystem.

So now I do not know where to put that needed SMBIOS call for enabling
0xe025 hotkey event and also I do not know if it is better to use WMI or
dcdbas.ko.

Maybe now it could make sense to unify Dell SMBIOS API in kernel and
move common functions to one place and let drivers to use just common
functions. According to older Dell ACPI WMI documentation in DMI is bit
which specify if BIOS support SMBIOS via WMI or not.

At least I think this one patch should not be included into kernel until
there will be full support for 0xe025 key (adding that SMBIOS call).

-- 
Pali Rohár
pali.ro...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Tuesday 01 December 2015 20:51:34 Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated.  As there seems to be no ACPI method or SMI call to
> determine without possible side effects whether a given machine is
> capable of simulating a keypress when this hotkey is pressed, DMI
> matching is used to whitelist the models for which an input event should
> be generated when WMI event 0xe025 is received.
> 
> Signed-off-by: Michał Kępień 
> ---
> Changes from v1:
> 
>   - Use DMI matching instead of a module parameter
>   - Change flag name to improve readability
> 
> Darren, I am aware this patch conflicts with Andy's WMI rework series
> posted yesterday, so please just let me know if you want me to rebase.
> 
>  drivers/platform/x86/dell-wmi.c |   39 
> ++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 8cb0f57..27c8f49 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -47,6 +47,38 @@ static int acpi_video;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  
> +struct quirk_entry {
> + bool process_dell_instant_launch;
> +};
> +
> +static struct quirk_entry *quirks;
> +
> +static struct quirk_entry quirk_unknown = {
> +};
> +
> +static struct quirk_entry quirk_dell_vostro_v131 = {
> + .process_dell_instant_launch = true,
> +};
> +
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> + quirks = dmi->driver_data;
> + return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Vostro V131",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> + },
> + .driver_data = _dell_vostro_v131,
> + },
> + { }
> +};
> +

It is not possible to simplify this part of code? Currently we only need
boolean variable: ignore 0xe025 event or not. I think that whole quirk
structure is not needed yet (and I would be happy if never in future).

>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -87,7 +119,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] 
> __initconst = {
>   { KE_IGNORE, 0xe020, { KEY_MUTE } },
>  
>   /* Shortcut and audio panel keys */
> - { KE_IGNORE, 0xe025, { KEY_RESERVED } },
> + { KE_KEY, 0xe025, { KEY_PROG4 } },
>   { KE_IGNORE, 0xe026, { KEY_RESERVED } },
>  
>   { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> @@ -183,6 +215,9 @@ static void dell_wmi_process_key(int reported_key)
>key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
>   return;
>  
> + if (key->keycode == KEY_PROG4 && !quirks->process_dell_instant_launch)
> + return;
> +
>   sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>  }
>  
> @@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
>   return -ENODEV;
>   }
>  
> + quirks = _unknown;

Unknown sounds like something is not know or we do not know what it is.
But here we know exactly what is needed (= ignore 0xe025 key).

> + dmi_check_system(dell_wmi_quirks);
>   dmi_walk(find_hk_type, NULL);
>   acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
>  

-- 
Pali Rohár
pali.ro...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Tuesday 01 December 2015 20:51:34 Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated.  As there seems to be no ACPI method or SMI call to
> determine without possible side effects whether a given machine is
> capable of simulating a keypress when this hotkey is pressed, DMI
> matching is used to whitelist the models for which an input event should
> be generated when WMI event 0xe025 is received.
> 
> Signed-off-by: Michał Kępień 
> ---
> Changes from v1:
> 
>   - Use DMI matching instead of a module parameter
>   - Change flag name to improve readability
> 
> Darren, I am aware this patch conflicts with Andy's WMI rework series
> posted yesterday, so please just let me know if you want me to rebase.
> 
>  drivers/platform/x86/dell-wmi.c |   39 
> ++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 8cb0f57..27c8f49 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -47,6 +47,38 @@ static int acpi_video;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  
> +struct quirk_entry {
> + bool process_dell_instant_launch;
> +};
> +
> +static struct quirk_entry *quirks;
> +
> +static struct quirk_entry quirk_unknown = {
> +};
> +
> +static struct quirk_entry quirk_dell_vostro_v131 = {
> + .process_dell_instant_launch = true,
> +};
> +
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> + quirks = dmi->driver_data;
> + return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Vostro V131",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> + },
> + .driver_data = _dell_vostro_v131,
> + },
> + { }
> +};
> +

It is not possible to simplify this part of code? Currently we only need
boolean variable: ignore 0xe025 event or not. I think that whole quirk
structure is not needed yet (and I would be happy if never in future).

>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -87,7 +119,7 @@ static const struct key_entry dell_wmi_legacy_keymap[] 
> __initconst = {
>   { KE_IGNORE, 0xe020, { KEY_MUTE } },
>  
>   /* Shortcut and audio panel keys */
> - { KE_IGNORE, 0xe025, { KEY_RESERVED } },
> + { KE_KEY, 0xe025, { KEY_PROG4 } },
>   { KE_IGNORE, 0xe026, { KEY_RESERVED } },
>  
>   { KE_IGNORE, 0xe02e, { KEY_VOLUMEDOWN } },
> @@ -183,6 +215,9 @@ static void dell_wmi_process_key(int reported_key)
>key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
>   return;
>  
> + if (key->keycode == KEY_PROG4 && !quirks->process_dell_instant_launch)
> + return;
> +
>   sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>  }
>  
> @@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
>   return -ENODEV;
>   }
>  
> + quirks = _unknown;

Unknown sounds like something is not know or we do not know what it is.
But here we know exactly what is needed (= ignore 0xe025 key).

> + dmi_check_system(dell_wmi_quirks);
>   dmi_walk(find_hk_type, NULL);
>   acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
>  

-- 
Pali Rohár
pali.ro...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Pali Rohár]

On Thursday 03 December 2015 17:16:06 Darren Hart wrote:
> This looks fine to me, and if Pali will ack it, I'll move it from for-review 
> to
> testing and Andy will need to update patch 14/14 to accomodate - unless you 
> guys
> decide to include this in his.

This patch is not enough for enabling 0xe025 key on that Vostro machine.
Some extra SMBIOS call is needed, without them ACPI will not send WMI
keypress event.

Dell SMBIOS call can be done either via WMI or via SMI inb/outb
instructions which implements dcdbas.ko driver. Module dell-laptop.ko
uses only SMBIOS API for all functionality and uses dcdbas.ko.

There is another driver which uses SMBIOS API, but use WMI calls. It is
dell-led.ko in leds subsystem.

So now I do not know where to put that needed SMBIOS call for enabling
0xe025 hotkey event and also I do not know if it is better to use WMI or
dcdbas.ko.

Maybe now it could make sense to unify Dell SMBIOS API in kernel and
move common functions to one place and let drivers to use just common
functions. According to older Dell ACPI WMI documentation in DMI is bit
which specify if BIOS support SMBIOS via WMI or not.

At least I think this one patch should not be included into kernel until
there will be full support for 0xe025 key (adding that SMBIOS call).

-- 
Pali Rohár
pali.ro...@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Andy Lutomirski]

On Fri, Dec 4, 2015 at 4:55 AM, Michał Kępień  wrote:
>> > On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
>> > Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
>> > is generated.  As there seems to be no ACPI method or SMI call to
>> > determine without possible side effects whether a given machine is
>> > capable of simulating a keypress when this hotkey is pressed, DMI
>> > matching is used to whitelist the models for which an input event should
>> > be generated when WMI event 0xe025 is received.
>> >
>> > Signed-off-by: Michał Kępień 
>> > ---
>> > Changes from v1:
>> >
>> >   - Use DMI matching instead of a module parameter
>> >   - Change flag name to improve readability
>> >
>> > Darren, I am aware this patch conflicts with Andy's WMI rework series
>> > posted yesterday, so please just let me know if you want me to rebase.
>>
>> Thanks for the heads' up. Andy's series is longer and likely going to need 
>> more
>> review from more people, so I don't necessarily want to hold this one up on 
>> it -
>> unless it makes sense to include it in that series so they can evolve 
>> together.
>> I'll leave that to you and Andy to decide.
>
> As merging my patch with Andy's work is just a matter of moving code
> around, not actually changing it, personally I don't feel this patch
> should be somehow linked to the WMI rework.  Andy, please let me know if
> you disagree, I don't really have any strong feelings about this.
>'

I think it's just a matter of which lands first.  It shouldn't be a big deal.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -47,6 +47,38 @@ static int acpi_video;
> >  
> >  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
> >  
> > +struct quirk_entry {
> > +   bool process_dell_instant_launch;
> > +};
> > +
> > +static struct quirk_entry *quirks;
> > +
> > +static struct quirk_entry quirk_unknown = {
> > +};
> > +
> > +static struct quirk_entry quirk_dell_vostro_v131 = {
> > +   .process_dell_instant_launch = true,
> > +};
> > +
> > +static int __init dmi_matched(const struct dmi_system_id *dmi)
> > +{
> > +   quirks = dmi->driver_data;
> > +   return 1;
> > +}
> > +
> > +static const struct dmi_system_id dell_wmi_quirks[] __initconst = {
> > +   {
> > +   .callback = dmi_matched,
> > +   .ident = "Dell Vostro V131",
> > +   .matches = {
> > +   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +   DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> > +   },
> > +   .driver_data = _dell_vostro_v131,
> > +   },
> > +   { }
> > +};
> > +
> 
> It is not possible to simplify this part of code? Currently we only need
> boolean variable: ignore 0xe025 event or not. I think that whole quirk
> structure is not needed yet (and I would be happy if never in future).

Of course it is possible.  I just got the feeling that using a quirk
structure is the way to go for this subsystem as it currently contains 7
drivers using something like this.  Moreover, I saw that in some commits
initially adding a quirk structure to a driver (2d8b90be, a979e2e2) that
structure contained just one field.  

So, between KISS and following the beaten path, I chose the latter.  If
you still think this patch should be reworked to use a single global
boolean instead, please let me know and I'll prepare a v3.

> > @@ -432,6 +467,8 @@ static int __init dell_wmi_init(void)
> > return -ENODEV;
> > }
> >  
> > +   quirks = _unknown;
> 
> Unknown sounds like something is not know or we do not know what it is.
> But here we know exactly what is needed (= ignore 0xe025 key).

Again, not my idea, I just thought it would be wise to follow what seems
to be an established pattern:

$ git grep 'quirk.*unknown' drivers/platform/x86/

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> This patch is not enough for enabling 0xe025 key on that Vostro machine.
> Some extra SMBIOS call is needed, without them ACPI will not send WMI
> keypress event.

Indeed.  But have you read the last e-mail I wrote before submitting the
original patch [1]?  Brightness control on the V131 is already broken
"out of the box" with newer kernels (flickering upon brightness change),
but if we do what you're suggesting and include the SMI call in the
kernel, we'll break it even more, to the point where pressing one of the
brightness control keys might not result in any brightness change at
all.  Sure, we can fix that by overriding an arbitrary ACPI method.  Oh,
wait, did I say "fix"?

I posted the patch without the SMI call because that way if you want to
use the Dell Instant Launch hotkey, you just fire up a userspace script
(which uses libsmbios and takes care of overriding the ACPI method) and
chances are you will end up with a fully functional system.  Of course
you need to understand that using this script is not an elegant solution
and that it might break something else, but it's your choice, not the
kernel's.  And the patch itself does not change kernel's default
behavior, so we're not risking breaking any other models out there.

> At least I think this one patch should not be included into kernel until
> there will be full support for 0xe025 key (adding that SMBIOS call).

Again, fully supporting the Dell Instant Launch hotkey makes brightness
control even more broken than it has to be.  In other words, everything
is terrible.

The only real solution to all these issues is a BIOS fix and I'm pretty
sure it's not happening.

[1] http://www.spinics.net/lists/platform-driver-x86/msg07679.html

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Michał Kępień]

> > On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> > Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> > is generated.  As there seems to be no ACPI method or SMI call to
> > determine without possible side effects whether a given machine is
> > capable of simulating a keypress when this hotkey is pressed, DMI
> > matching is used to whitelist the models for which an input event should
> > be generated when WMI event 0xe025 is received.
> > 
> > Signed-off-by: Michał Kępień 
> > ---
> > Changes from v1:
> > 
> >   - Use DMI matching instead of a module parameter
> >   - Change flag name to improve readability
> > 
> > Darren, I am aware this patch conflicts with Andy's WMI rework series
> > posted yesterday, so please just let me know if you want me to rebase.
> 
> Thanks for the heads' up. Andy's series is longer and likely going to need 
> more
> review from more people, so I don't necessarily want to hold this one up on 
> it -
> unless it makes sense to include it in that series so they can evolve 
> together.
> I'll leave that to you and Andy to decide.

As merging my patch with Andy's work is just a matter of moving code
around, not actually changing it, personally I don't feel this patch
should be somehow linked to the WMI rework.  Andy, please let me know if
you disagree, I don't really have any strong feelings about this.

-- 
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Darren Hart]

On Tue, Dec 01, 2015 at 08:51:34PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated.  As there seems to be no ACPI method or SMI call to
> determine without possible side effects whether a given machine is
> capable of simulating a keypress when this hotkey is pressed, DMI
> matching is used to whitelist the models for which an input event should
> be generated when WMI event 0xe025 is received.
> 
> Signed-off-by: Michał Kępień 
> ---
> Changes from v1:
> 
>   - Use DMI matching instead of a module parameter
>   - Change flag name to improve readability
> 
> Darren, I am aware this patch conflicts with Andy's WMI rework series
> posted yesterday, so please just let me know if you want me to rebase.

Thanks for the heads' up. Andy's series is longer and likely going to need more
review from more people, so I don't necessarily want to hold this one up on it -
unless it makes sense to include it in that series so they can evolve together.
I'll leave that to you and Andy to decide.

This looks fine to me, and if Pali will ack it, I'll move it from for-review to
testing and Andy will need to update patch 14/14 to accomodate - unless you guys
decide to include this in his.

For now, this is queued to for-review.

Thanks!

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dell-wmi: process Dell Instant Launch hotkey on Dell Vostro V131

[Darren Hart]

On Tue, Dec 01, 2015 at 08:51:34PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), pressing the Dell Instant
> Launch hotkey does not raise an i8042 interrupt - only WMI event 0xe025
> is generated.  As there seems to be no ACPI method or SMI call to
> determine without possible side effects whether a given machine is
> capable of simulating a keypress when this hotkey is pressed, DMI
> matching is used to whitelist the models for which an input event should
> be generated when WMI event 0xe025 is received.
> 
> Signed-off-by: Michał Kępień 
> ---
> Changes from v1:
> 
>   - Use DMI matching instead of a module parameter
>   - Change flag name to improve readability
> 
> Darren, I am aware this patch conflicts with Andy's WMI rework series
> posted yesterday, so please just let me know if you want me to rebase.

Thanks for the heads' up. Andy's series is longer and likely going to need more
review from more people, so I don't necessarily want to hold this one up on it -
unless it makes sense to include it in that series so they can evolve together.
I'll leave that to you and Andy to decide.

This looks fine to me, and if Pali will ack it, I'll move it from for-review to
testing and Andy will need to update patch 14/14 to accomodate - unless you guys
decide to include this in his.

For now, this is queued to for-review.

Thanks!

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/