Hi Arvid, On 9/20/22 11:48, Arvid Norlander wrote: > On 2022-09-19 11:27, Hans de Goede wrote: >> Hi, >> >> On 9/11/22 20:49, Arvid Norlander wrote: >>> This is loosly based on a previous staging driver that was removed. See >>> links below for more info on that driver. The original commit ID was >>> 0be013e3dc2ee79ffab8a438bbb4e216837e3d52. >>> >>> However, here a completely different approach is taken to the user space >>> API (which should solve the issues the original driver had). Each PNP0C32 >>> device is a button, and each such button gets a separate input device >>> associated with it (instead of a shared platform input device). >>> >>> The button ID (as read from ACPI method GHID) is provided via a sysfs file >>> "button_id". >>> >>> If the button caused a wakeup it will "latch" the "wakeup_cause" sysfs file >>> to true. This can be reset by a user space process. >>> >>> Link: https://marc.info/?l=linux-acpi&m=120550727131007 >>> Link: https://lkml.org/lkml/2010/5/28/327 >>> Signed-off-by: Arvid Norlander <lkml@xxxxxxxxx> >> >> 2 high level remarks here: >> >> 1. I believe we should strive for having all issues with this driver fixed >> before merging it, at which point it should not sit under drivers/staging >> but rather under drivers/platform/x86 (as an added bonus this can also make >> toshiba_apci's Kconfig bit select it automatically). So for the next version >> please move this to drivers/platform/x86 > > Makes sense, will do. However, there is nothing x86 specific in theory with > this driver. Would it not make more sense to put it under drivers/acpi? Since the spec is from Microsoft I expect it to be a x86 only thing (AFAIK this predates there couple of ARM attempts). Also since this has some tie-in with toshiba-acpi (at least for the laptops you are actually testing this on) keeping it in the same dir as toshiba-acpi seems to make the most sense to me. A lot of ACPI drivers actually live under drivers/platform/x86 for similar reasons. >> 2. This is using struct acpi_driver, but as Rafael (ACPI maintainer) always >> said that is really only for very special cases. The ACPI subsystem should >> instantiate standard platform devices for each PNP0C32 device, you can >> check this under: /sys/bus/devices/platform. And this driver should then >> be convered to a standard platform_driver binding to the platform devices >> instead of being a struct acpi_driver. > > I had a look at this, and it seems like a much more complicated a approach, > for example, there is no dedicated .ops.notify, which means I need to use > acpi_install_notify_handler, and there is no devm_ version of that either. > A lot of other things seem to be ever so slightly more complicated as well. > > What is the motivation behind this being preferred? And are most of the > existing drivers using acpi_driver legacy (e.g. toshiba_acpi)? I'm mostly just parroting (repeating) the party-line here. Not making new drivers an acpi_driver is typically requested by Rafael, the ACPI maintainer. Rafael can you explain why? >From my own view point I guess this has to do with ACPI having changed over time from mostly offering firmware interfaces which mostly talk to the EC, to actually also describing the hw. Now a days of lot of ACPI devices are actually describing real hardware devices, e.g. PCI cards, I2C devices, SPI decices and UART attached devices including things like IO / MMIO addresses, I2C slave addresses, SPI chip selects, GPIOs, IRQs, etc. So the kernel now a days instantiates actual SPI / I2C / UART / PCI / other(platform) devices for all the devices in ACPI, with all the physicial resources attached to the struct platform_device / struct i2c_client / etc. using the kernels standard resource mechanisms. The struct platform_device / struct i2c_client / etc. devices then have a firmware_node pointer (aka companion device) pointing to the ACPI device in case the driver also needs to make some actual ACPI calls on either to query some extra information stored in ACPI, or sometimes to enable / disable special features driven through ACPI methods. The instantiation of these struct platform_device / struct i2c_client / etc. devices is done through a special default/fallback acpi_driver. If you attach your own acpi_driver to an acpi_device then this will not happen. So for any devices which also have some physical part (and not just pure firmware interface) using an acpi_driver is not what you want. At which point I guess we simply just want to avoid it even for pure virtual/fw devices like the PNP0C32 case for consistency. Note this is just my view on this, perhaps Rafael can explain this better? Regards, Hans
