linux-surface / kernel

Linux kernel with modifications for Microsoft Surface devices.
Other
118 stars 33 forks source link

Input: soc_button_array - support AMD variant Surface devices #108

Closed nakato closed 2 years ago

nakato commented 2 years ago

The power button on the AMD variant of the Surface Laptop uses the same MSHW0040 device ID as the 5th and later generation of Surface devices, however they report 0 for their OEM platform revision. As the AMD variant's do not need any special casing, skip calling the _DSM and continue loading the soc_button_array.

While only the power button is used in these laptops, the volume keys are still defined in the ACPI table.

qzed commented 2 years ago

Ah of course it fails there... was wondering when that hack of a check was coming back to bite me. We actually only need to check if the DSM call exists and don't need the value, so we can restructure that a bit. Give me a second and I'll post a suggestion.

nakato commented 2 years ago

I was wondering if acpi_check_dsm would be enough. Looks like the ARM surface will respond to acpi_check_dsm correctly too.

qzed commented 2 years ago

I was wondering if acpi_check_dsm would be enough. Looks like the ARM surface will respond to acpi_check_dsm correctly too.

Yeah, it should be enough. When I wrote that check I wanted to make it a bit more robust with respect to ACPI updates on older devices (like them introducing that DSM call in retrospect). Should have just used acpi_check_dsm in the first place. Anyway, here's the change:

0001-Fix-MSHW0040-device-checks.patch.txt

nakato commented 2 years ago

I'll get those fixed up with that patch.

Should I put you on the commits with a Co-developed-by:? I presume after I post the initial copy here I can get your OK to add your sign off. Once we're happy here I'll work on submitting them upstream with the other patches as well.

Having surfacepro3_button loaded isn't causing problems, so I presume the preferences is two independent patches sent together rather than a single patch?

qzed commented 2 years ago

Should I put you on the commits with a Co-developed-by:? I presume after I post the initial copy here I can get your OK to add your sign off.

Sure, feel free to do that.

Once we're happy here I'll work on submitting them upstream with the other patches as well.

Perfect, thanks!

Having surfacepro3_button loaded isn't causing problems, so I presume the preferences is two independent patches sent together rather than a single patch?

Yeah I think two patches are better. Then they can go through the individual trees by their own if that's the way the maintainers prefer it.

There should also be some fixes tags, specifically:

I'd like to test them in the Surface kernel for one or two versions first though, just to make sure we're not running into any problems on other devices.

nakato commented 2 years ago

There should also be some fixes tags, specifically:

I've added those fixes tags.

I'll hold off on signed off until that's explicit, and mark it ready for review.

qzed commented 2 years ago

Let's test the patches in the surface kernel for two or three versions. Feel free to add my signed-off-by tag after that.

qzed commented 2 years ago

@nakato It's been two weeks now without any error reports. I think the patches should be good to go, feel free to send them in with my signed-off-by tag.

nakato commented 2 years ago

@qzed Ack. I'll work on that this weekend.

nakato commented 2 years ago

Didn't quite get to it that weekend, I've posted the patches for review.

https://lore.kernel.org/lkml/20211109081125.41410-1-nakato@nakato.io/T/#t

qzed commented 2 years ago

Thanks!