acpi_apple: Apple platform ACPI driver,

[guest-seuros]

Summary

handles wake-on-LAN arming and lid state on the PCH
PMC/GMUX platform instance, and panel backlight
control on the PNLF instance, registering with
backlight(9).

[guest-seuros]

@adrian , i had this driver locally tested across all the fleet. It attach in the full range 2006-2020, the code can extended in Apple ARM but i removed that portion .

It also brings Wake-on-LAN support to many Macs. this is not always exposed or available under macOS or even documented.

PS: Display control is currently not working on every iMac. We either need to fix the DRM driver to support it, or I need to figure out how much of that functionality belongs in this driver.

PS2: I made this a separate driver because none of this functionality is actually part of the SMC.

[adrian]

interesting; does this provide backlight control without DRM loaded?

[guest-seuros]

Not all models.
Mono GPU mostly work (especially intel)
Dual GPU could work depending on which GPU is active.

This is why i wrote : future apple_gmux driver.

We need to know which GPU is driving the screen to poke it. That a much bigger project as open pandora box in testing all models and modes.

[ngie]

Most of my comments deal with maintainability of the driver post-merge.

share/man/man4/Makefile

17

This should only be installed on arm64/amd64 [1]. Please see _acpi_asus.4 as an example for how to conditionally build/install this manpage.

1. As much as @adrian is trying to bring back PowerPC64, I don't see the value in resurrecting powerpc64 Apple hardware.

sys/dev/apple_acpi/apple_acpi.h

35–41

sys/dev/apple_acpi/apple_backlight.c

125

Not a huge fan of hardcoded values like this. Yes, I know why 4 and 8 are hardcoded, but I'm almost inclined (at some level) give them names or something.

136

Why 100? Should this be tunable somehow?

138

What does 0x01 represent? Seems like a candidate for a named constant.

141

Again, why 100?

152

Why 100? Should this be tunable somehow?

154

What does 0x01 represent? Seems like a candidate for a named constant.

208

Why 200? Should this be a tunable constant?

287

We might have a macro that does this casting magic better today under sys/..., like __DECONST does.

452–457

Why these magic numbers? What do they represent? Why aren't they named constants?

sys/modules/acpi/Makefile

3

• What about arm64?
• Why build on i386?

[guest-seuros]

I will address few of those comments. Waiting on the reply from the others.

share/man/man4/Makefile

17

If Adrian wants to drop ppc or if the code is not working there. I would like to keep amd64 for now only, then move to arm64 once we extend the driver.

sys/dev/apple_acpi/apple_backlight.c

125

This is just splitting the 32-bit value into 4 bytes. The 8 is the number of bits per byte, so each loop shifts to the next byte, and & 0xff keeps only that byte.

136

There is 0 reason to add more code to let users tune it.

I will need to know when someone will want to that.

100 is just a round number, i could retry with 50 or 30 if you want. But i dont want to add tunable without a real possible scenario.

152

Same as the other comment. 100 a round number to wait for it, i did not want to write 79 , 96, or any value look emperical.

50 will work too, 100 is to be sure and simpler.

When i see : timeout(1), there is less question than if i see timeout(0.438) ...

154

0x01 is just a bit mask. I only care about bit 0 here.

gwr: 10110101
mask: 00000001

---

result: 00000001 -> bit 0 is set

Is there a better way to do it ? a helper ?

208

We could! but that value worked across all the apple fleet.

200 is just a round number. this look complete in 0.02 seconds max, depend hardware and it state.

adding the tunable will look like we don't test the code.

sys/modules/acpi/Makefile

3

I can remove i386, but the driver works with it.

I removed arm64 , because the handling is also removed from here. I want to add it in another diff.

No ppc either, @adrian can test and relax the build.

[guest-seuros]

clarify some magic numbers, dropped support for 32bit.

[ngie]

re: magic numbers for iterations/attempts -- part of the reason why it matters is that a) there's a point where fixed timeouts generally fail. b) unintelligently waiting via sleeps instead of using another form of synchronization usually results in less performant code that's also more difficult to maintain over the long run. I know you're doing this work with a lot of empirical testing on lab devices, but those devices might not be representative of reality in terms of other [more stock] devices.

[ngie]

These sources look like they would be well-homed in sys/dev/acpi_support/apple_acpi, BTW.

sys/dev/apple_acpi/wake_on_lan.c

27

Using this variable to track ownership and teardown smells a bit fishy... I don't usually play in this space, so I don't know the absolute best solution to this, but at first glance a) using an atomic(9) type seems like a good idea to ensure things are being properly tracked, b) you should confirm that the value is not <0 or clamp the value at 0 before cleaning up resources, and c) this problem seems like it could be solved via another relationship between this driver and its consumers.
How do other ACPI drivers handle this?

sys/modules/acpi/Makefile

3

I can remove i386, but the driver works with it.

I removed arm64 , because the handling is also removed from here. I want to add it in another diff.

No ppc either, @adrian can test and relax the build.

I'm pretty sure ppc64 doesn't make sense here. I remember ACPI being a solidly Intel-architecture concept back when ppc64 Macs were being produced, as compared to now where it's a mix of ACPI, FDT, and UEFI.