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

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

Hi,

Thank you for the quick testing.

On 6/17/22 15:07, Stefan Seyfried wrote:
> Hi Hans,
> 
> On 17.06.22 13:07, Hans de Goede wrote:
> 
>> Thank you for providing this info. Can you please give
>> the attached patch series a try, this includes Stefan's 1/2 patch
>> and replaces Stefan's 2/2 patch.
>>
>> This will hopefully fix the double key-presses for you, while
>> also keeping everything working for Stefan without requiring
>> a module option or DMI quirks.
>>
>> Stefan can you also give this series a try please?
> 
> Works for me, almost out of the box.
> I need to enable "report_key_events=1" in the video module, then the panasonic-acpi module starts reporting brightness up/down keys.

Ok, so you need another module option that is not really helpful.

The idea behind the acpi_video_handles_brightness_key_presses() check
is that if the ACPI video bus device is present it is expected to
already report brightness up/down keypresses and we want to avoid
duplicates.

Can you check with evtest or evemu-record that the brightness
events are not already being delivered by the "Video Bus"
input device ?

> Volume and mute keys work without manual changes.

Good.
 
> (I tested against 5.18.2 because that's what was already prepared. That old machine takes quite some time, even to just compile the platform/x86 subdirectory ;-) but I don't think this is relevant. If you think it is, I can also test against latest 5.19-rc code)

Testing against 5.18 is fine .

>> Looking at this has also brought up an unrelated backlight question:
>>
>> Kenneth, since you have acpi-video reporting keypresses you will
>> likely also have an acpi_video (or perhaps a native intel) backlight
>> under /sys/class/backlight and I noticed that panasonic-laptop
>> uncondirionally registers its backlight so you may very well end
>> up with 2 backlight controls under /sys/class/backlight, which
>> we generally try to avoid (so that userspace does not have to
>> guess which one to use).
>>
>> Can you do:
>> ls /sys/class/backlight
> 
> toughbook:~ # ls -l /sys/class/backlight/
> total 0
> lrwxrwxrwx 1 root root 0 Jun 17 14:45 intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
> lrwxrwxrwx 1 root root 0 Jun 17 14:49 panasonic -> ../../devices/virtual/backlight/panasonic
> 
>> and let me know the output?
>>
>> Also if there are 2 backlights there then please do:
>> cat /sys/class/backlight/<name>/max_brightness
>> to find out the range (0-value)
> 
> toughbook:/sys/class/backlight # grep . */max_brightness
> intel_backlight/max_brightness:19531
> panasonic/max_brightness:255
> 
>> and then try if they both work by doing:
>>
>> echo $number > /sys/class/backlight/<name>/brightness
>>
>> with different $number values in the range and see
>> if this actually changes the brightness.
> 
> intel_backlight: does not work
> panasonic: does work

Ok, so that suggests that the ACPI video bus on your
device is defunct, so I guess it also does not report
key-presses (see above) ?

This will also need some work then because we want to move
to there only being 1 (actually working) backlight-class
device. Rather then having multiple and let userspace
guess which one it needs to use.

>> While we are at it, Stefan can you do the same please?
> 
> See above.
> But hey, this is an i855GM graphics chip, I'm happy if it is still working *at all* (for example I need to avoid the xf86-intel driver and use the modesetting driver instead to get a usable sytstem)
> 
> And I'm totally happy if all I have to do in the future is a
> 
> option video report_key_events=1
> 
> modprobe.conf file ;-)

We really don't want people to have to specify module-options just
to have things working.

Stefam, at least for the backlight class-device issue we will need a DMI
quirk, so can you run:

sudo dmidecode > dmidecode.txt

and then attach the output to your next email, or send me a copy
privately ?

Regards,

Hans