On 9/21/21 9:58 AM, Hans de Goede wrote:
Hi,
On 9/21/21 4:29 PM, Daniel Dadap wrote:
On Sep 21, 2021, at 08:53, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 9/20/21 3:51 PM, Pali Rohár wrote:
On Monday 20 September 2021 15:33:20 Hans de Goede wrote:
Hi Pali,
On 9/20/21 3:29 PM, Pali Rohár wrote:
On Monday 13 September 2021 11:01:50 Hans de Goede wrote:
Hi,
On 9/3/21 2:38 AM, Daniel Dadap wrote:
A number of upcoming notebook computer designs drive the internal
display panel's backlight PWM through the Embedded Controller (EC).
This EC-based backlight control can be plumbed through to an ACPI
"WMAA" method interface, which in turn can be wrapped by WMI with
the GUID handle 603E9613-EF25-4338-A3D0-C46177516DB7.
Add a new driver, aliased to the WMAA WMI GUID, to expose a sysfs
backlight class driver to control backlight levels on systems with
EC-driven backlights.
Signed-off-by: Daniel Dadap <ddadap@xxxxxxxxxx>
Reviewed-By: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Hello Hans!
I would suggest to rename this driver and config option to not include
-AA in its name. WMAA is just internal name of ACPI method, composed
from two parts: "WM" and "AA". Second part "AA" is read from the _WDG
where is the translation table from WMI GUID (in this case 603E9613...)
to ACPI method name. "AA" is just autogenerated identifier by wmi
compiler and because names are ASCII strings, I guess "AA" could mean
the first (autogenerated) method. In the whole driver code you are not
using AA function name, but directly WMI GUID, which also means that
driver is prepared if vendor "recompiles" wmi code in acpi (and compiler
generates another identifier, not AA). Also another argument is that
there can be lot of other laptops which have WMAA ACPI method but they
can have different API or do something totally different. So name WMAA
is in this wmi context very misleading. Rather it should be named by
vendor.
Right, that is a very valid point. I should have spotted this myself.
So what would be a better name wmi-nvidia-backlight.ko I guess ?
(and update the rest to match ?)
It looks like that no vendor driver starts with "wmi-" prefix. "-wmi"
string is used as a suffix. So for consistency it would be better to
choose "nvidia-backlight-wmi.ko".
Right, I should have checked first.
So I just checked and the standard pattern used is:
vendor_wmi_feature
So lets go with nvidia_wmi_backlight.ko
Daniel, can you prepare a patch on top of your merged patch to do
the rename of the Kconfig entry and the module-name please?
Yes, I already had a patch prepared to rename things to nvidia-backlight-wmi; I am waiting to hear back from some folks if there’s a more specific name that might be appropriate (e.g. a name of a particular feature tied to this) or if it should be more generic (e.g., if there is a strong possibility this design may be used in systems with no NVIDIA GPU); while I’m waiting for those answers, I’ll switch to nvidia-wmi-backlight as the assumed option, if there isn’t something more appropriate.
Ok, sounds good, thank you.
The one suggestion I got for improving the name we've discussed here is
to include "ec" in the name, to make it clear that this isn't a driver
for the normal backlight interface that is driven by the NVIDIA GPU. I
intended to send this message before the rename patches that I sent a
few hours ago, but apparently I didn't.
Regards,
Hans
