Re: [PATCH 2/2] platform/x86: panasonic-laptop: allow to use all hotkeys

[Hans de Goede <hdegoede@redhat.com>]

Hi Stefan,

On 6/15/22 19:10, Stefan Seyfried wrote:
> Hi Andy,
> 
> On 15.06.22 13:24, Andy Shevchenko wrote:
>> On Wed, Jun 15, 2022 at 1:21 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Sun, Jun 12, 2022 at 3:54 PM <stefan.seyfried@googlemail.com> wrote:
>>
>>> We usually add module options in very bad cases where it's very useful
>>> for debugging or when some devices require the opposite settings while
>>> can't be distinguished automatically. Here I do not see either of such
>>> cases. Hence, I would prefer to see a DMI based quirk as it's done a
>>> lot in the PDx86 drivers. Can you do that?
> 
> I can do that, but... below ;-)
> 
>> Looking into the code of the culprit patch, have you tried to add a
>> debug pr_info() and see what value is in the result? Perhaps you may
>> just sort out by correcting that.
> 
> The driver is working fine, it's just that Kenneth's machine is getting most of the hotkey events (I'd guess all but sleep, hibernate, battery) twice. That's why he disabled the key generation from the panasonic_acpi driver for them. (My guess is that on his CF-W5, they are also coming in via normal keyboard input path). My CF-51 does only generate them via acpi, so if they are not generated, I get nothing.
> Hence the module parameter so that the two known users of this module (Kenneth and me) can adjust this to their needs.
> 
> Now about the DMI match: I can do that.
> But let's face it: the panasonic laptops are pretty rare in the wild, so even if I'm "whitelisting" the CF-51, then probably other models will need the same treatment and we have no real way of finding out which ones, unless people complain. (For example my CF-51 is about 17 years old now and I just pulled it out and updated it to the latest and greatest "because I can". That's also why it has taken me so long to even notice the driver was broken for me. So people not complaining will not mean "nothing is broken" but rather "this code has not many users").
> So even if I add the DMI match (which is a good idea anyhow because then "my" model will work out of the box, while right now I need to add a module parameter or switch it on later), I'd still vote for having a possibility for overriding the DMI results.
> Would that be OK?

Actually I agree with your original assessment that Kenneth's patch
(ed83c9171829) which broke things on your laptop is wrong.

Back then I did not properly realize that it is effectively
disabling event generation for most of the reported event codes.

If anything there should be a DMI match for Kenneth's model and
reporting the events normally should be the default.

Kenneth, can you check with e.g. evemu-record or evtest
where the double events are coming from ?  Obviously one of
the events is coming from the panasonic-laptop driver, but
where is the other event coming from. Is it coming from the
atkbd driver; or ... ?   Maybe from the acpi-video driver
for the brightness keys ?

Regards,

Hans