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? > > 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)? > > Please address these 2 things as well as the remarks from Barnabás and > then send out a version 2. Then I will do a more detailed review of > version 2 once posted. > > Regards, > > Hans > <snip>
