Re: [PATCH v6] platform/x86: Add driver for ACPI WMAA EC-based backlight control

From: Hans de Goede <hdegoede@redhat.com>
To: "Pali Rohár" <pali@kernel.org>
Cc: "Daniel Dadap" <ddadap@nvidia.com>,
	platform-driver-x86@vger.kernel.org,
	mario.limonciello@outlook.com, pobrn@protonmail.com,
	andy.shevchenko@gmail.com, 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 15:53:29 +0200	[thread overview]
Message-ID: <57a958e2-93d7-3eea-ec7b-9496acd6aae5@redhat.com> (raw)
In-Reply-To: <20210920135106.y6mi57bcmurczmka@pali>

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?

Regards,

Hans