From: Hans de Goede <hdegoede@redhat.com>
To: Daniel Dadap <ddadap@nvidia.com>
Cc: "Pali Rohár" <pali@kernel.org>,
"platform-driver-x86@vger.kernel.org"
<platform-driver-x86@vger.kernel.org>,
"mario.limonciello@outlook.com" <mario.limonciello@outlook.com>,
"pobrn@protonmail.com" <pobrn@protonmail.com>,
"andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
"Aaron Plattner" <aplattner@nvidia.com>,
"Thomas Weißschuh" <linux@weissschuh.net>
Subject: Re: [PATCH v6] platform/x86: Add driver for ACPI WMAA EC-based backlight control
Date: Tue, 21 Sep 2021 16:58:31 +0200 [thread overview]
Message-ID: <9cbb4e7a-4af9-32f8-0293-6c2ef7d44ceb@redhat.com> (raw)
In-Reply-To: <A116D719-A0BD-4FA5-A758-E48D6428A8A6@nvidia.com>
Hi,
On 9/21/21 4:29 PM, Daniel Dadap wrote:
>
>> On Sep 21, 2021, at 08:53, Hans de Goede <hdegoede@redhat.com> 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@nvidia.com>
>>>>>>> Reviewed-By: Thomas Weißschuh <linux@weissschuh.net>
>>>>>>
>>>>>> 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.
Regards,
Hans
next prev parent reply other threads:[~2021-09-21 14:58 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <DM6PR19MB2636AB267CD321DE40EF324AFA730@DM6PR19MB2636.namprd19.prod.outlook.com>
[not found] ` <20200731202154.11382-1-ddadap@nvidia.com>
[not found] ` <20200731202154.11382-2-ddadap@nvidia.com>
2020-11-10 9:34 ` [PATCH v2 2/2] platform/x86: wmi: fail wmi_driver_register when no GUID is found Hans de Goede
2020-11-12 18:54 ` Daniel Dadap
2021-08-24 22:04 ` [PATCH v3] platform/x86: Add driver for ACPI WMAA EC-based backlight control Daniel Dadap
2021-08-24 22:47 ` Barnabás Pőcze
2021-08-25 16:36 ` Daniel Dadap
2021-08-25 22:26 ` Daniel Dadap
2021-08-27 16:47 ` Daniel Dadap
2021-08-27 17:12 ` Daniel Dadap
2021-08-25 9:05 ` Andy Shevchenko
2021-08-25 16:47 ` Daniel Dadap
2021-08-25 22:06 ` Thomas Weißschuh
2021-08-25 22:28 ` Daniel Dadap
2021-08-26 13:35 ` Andy Shevchenko
2021-08-26 15:39 ` Daniel Dadap
2021-08-31 22:49 ` [PATCH v4] " Daniel Dadap
2021-09-01 9:25 ` Thomas Weißschuh
2021-09-02 2:12 ` Daniel Dadap
2021-09-02 10:19 ` Thomas Weißschuh
2021-09-02 20:15 ` Daniel Dadap
2021-09-02 20:31 ` Hans de Goede
2021-09-02 21:47 ` [PATCH v5] " Daniel Dadap
2021-09-02 23:20 ` Thomas Weißschuh
2021-09-03 0:22 ` Daniel Dadap
2021-09-03 8:14 ` Andy Shevchenko
2021-09-03 17:55 ` Daniel Dadap
2021-09-03 0:38 ` [PATCH v6] " Daniel Dadap
2021-09-13 9:01 ` Hans de Goede
2021-09-20 13:29 ` Pali Rohár
2021-09-20 13:33 ` Hans de Goede
2021-09-20 13:51 ` Pali Rohár
2021-09-20 17:34 ` Daniel Dadap
2021-09-20 17:55 ` Pali Rohár
2021-09-20 19:38 ` Daniel Dadap
2021-09-20 19:52 ` Pali Rohár
2021-09-21 13:53 ` Hans de Goede
2021-09-21 14:02 ` Pali Rohár
2021-09-21 14:29 ` Daniel Dadap
2021-09-21 14:58 ` Hans de Goede [this message]
2021-09-27 20:23 ` [PATCH 1/2] platform/x86: Remove "WMAA" from identifier names in wmaa-backlight-wmi.c Daniel Dadap
2021-09-27 20:23 ` [PATCH 2/2] platform/x86: Rename wmaa-backlight-wmi to nvidia-wmi-ec-backlight Daniel Dadap
2021-10-11 13:00 ` [PATCH 1/2] platform/x86: Remove "WMAA" from identifier names in wmaa-backlight-wmi.c Hans de Goede
2021-09-27 22:52 ` [PATCH v6] platform/x86: Add driver for ACPI WMAA EC-based backlight control Daniel Dadap
2021-09-02 9:28 ` [PATCH v4] " Andy Shevchenko
2021-09-02 9:33 ` Thomas Weißschuh
2021-09-02 9:36 ` Andy Shevchenko
2021-09-01 15:57 ` Andy Shevchenko
2021-09-01 23:12 ` Aaron Plattner
2021-09-02 2:37 ` Daniel Dadap
2021-09-02 9:35 ` Andy Shevchenko
2021-09-02 20:15 ` Daniel Dadap
2021-09-03 8:10 ` Andy Shevchenko
2021-09-02 15:38 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9cbb4e7a-4af9-32f8-0293-6c2ef7d44ceb@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy.shevchenko@gmail.com \
--cc=aplattner@nvidia.com \
--cc=ddadap@nvidia.com \
--cc=linux@weissschuh.net \
--cc=mario.limonciello@outlook.com \
--cc=pali@kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).