acpi_system76: Support for acpi-controlled buttons on System76

[pouria]

Summary

Add acpi_system76 for handling acpi-controlled buttons
on System76 Laptops.

This is my first attempt to break from network only codes :)
So please review carefully since it's my first ACPI driver.
I've tested it on my laptop and checked the dsdt.dsl of:
https://github.com/system76/firmware-open

I may add battary thresholds in future. But that needs custom ACPI
objects so I prefer to keep this simple for now.

Not sure who to add in reviewers list.

Test Plan

I have serw13 and it works perfectly:

% sysctl hw.acpi.s76.keyboard_backlight=0
hw.acpi.s76.keyboard_backlight: 255 -> 0
% sysctl hw.acpi.s76.keyboard_backlight=255
hw.acpi.s76.keyboard_backlight: 0 -> 255
% sysctl hw.acpi.s76.keyboard_color=0
hw.acpi.s76.keyboard_color: 65535 -> 0
% sysctl hw.acpi.s76.keyboard_color=123123
hw.acpi.s76.keyboard_color: 0 -> 123123

input validation

sysctl: hw.acpi.s76.keyboard_backlight=256: Invalid argument
% sysctl hw.acpi.s76.keyboard_backlight=-1
hw.acpi.s76.keyboard_backlight: 255
sysctl: hw.acpi.s76.keyboard_backlight=-1: Invalid argument

change color + backlight using keyboard and check if sysctl is updated.

% sysctl hw.acpi.s76
hw.acpi.s76.keyboard_color: 16776960
hw.acpi.s76.keyboard_backlight: 48

[pouria]

Fix minor style.9 issues

[wulf]

We have backlight(9) interface since FreeBSD 13

[pouria]

In D55694#1274781, @wulf wrote:

We have backlight(9) interface since FreeBSD 13

I think there's a misunderstanding here.
The identifier named keyboard_backlight in my code refers to one of the System76's ACPI-exposed controls, it's not related to the backlight(9) interface.
I can rename it if that's clearer.

This implementation targets System76 ACPI (as noted in the revision title). System76 exposes several controls in their ACPI tree (e.g., DGPU/IGPU switching, charge thresholds, a vendor-specific airplane mode).
See their ACPI EC code here:
https://github.com/system76/coreboot/blob/05dda8aa05f988f3d38720b46804c9cf2590855a/src/ec/system76/ec/acpi/

In my current patch I handle keyboard color (which is not the job of backlight(9)) and keyboard brightness which is not registered under /dev/backlight on my laptop.
I probably didn't make that clear in the summary.
could you take another look at the code?

[imp]

Driver looks fairly straightforward. One question, but looking at other drivers after I wrote it, maybe the answer is 'that's just the pattern' which is fine.

sys/dev/acpi_support/acpi_system76.c

248

This is held for a lot longer than I'd think it would need to be. I see that it's likely because it makes the error path simpler. Will this cause problems?

[pouria]

Address @imp comment.

[pouria]

Just one question. I'm not familiar with the usual number of reviewer approval in ACPI driver changes here.
Can I commit this after your approval?
This driver simply works on my laptop and I want to go further and implement other functionalities provided by "\\_SB.S76D".

sys/dev/acpi_support/acpi_system76.c

248

No, but you're right. I was refactoring this function multiple times and forgot to reduce the locking duration.
Also I removed the unnecessary goto below.

[wulf]

In D55694#1274782, @pouria wrote:

could you take another look at the code?

backlight(9) interface is replacement or addition to sysctl interface. It has no relation to ACPI as it is user interface rather than hardware one. See e.g. sys/dev/acpi_support/acpi_asus_wmi.c

[wulf]

Adding of EVDEV interface to notyfy userland software would be good idea too. See sys/dev/acpi_support/acpi_asus_wmi.c again

[pouria]

In D55694#1274835, @wulf wrote:

In D55694#1274782, @pouria wrote:

could you take another look at the code?

backlight(9) interface is replacement or addition to sysctl interface. It has no relation to ACPI as it is user interface rather than hardware one. See e.g. sys/dev/acpi_support/acpi_asus_wmi.c

So you think I should use backlight for keyboard_backlight?
how about keyboard_color?
The keyboard_color has full RGB coverage and accept 24-bit value as its input, so I don't think registering it in backlight be a good idea.

In D55694#1274836, @wulf wrote:

Adding of EVDEV interface to notyfy userland software would be good idea too. See sys/dev/acpi_support/acpi_asus_wmi.c again

Thank you, I'll look into that.

[wulf]

In D55694#1274837, @pouria wrote:

The keyboard_color has full RGB coverage and accept 24-bit value as its input, so I don't think registering it in backlight be a good idea.

No one requires you to drop sysctl interface for the sake of backlight. Just expose hw.acpi.s76.keyboard_backlight sysctl through backlight(9) as well. This greatly helps userland software to find right backlight with scandir of /dev/backlight

It would be good to add RGB color support to backlight(9) itself, but that is other story.

[pouria]

In D55694#1274838, @wulf wrote:

In D55694#1274837, @pouria wrote:

The keyboard_color has full RGB coverage and accept 24-bit value as its input, so I don't think registering it in backlight be a good idea.

No one requires you to drop sysctl interface for the sake of backlight. Just expose hw.acpi.s76.keyboard_backlight sysctl through backlight(9) as well. This greatly helps userland software to find right backlight with scandir of /dev/backlight

It would be good to add RGB color support to backlight(9) itself, but that is other story.

Thank you, I'll work on it.

[wulf]

sys/dev/acpi_support/acpi_system76.c

42

You may add ACPI_PNP_INFO(system76_ids) line to get acpi_system76.ko autoloaded with devd on boot. See e.g. sys/dev/acpi_support/acpi_wmi.c