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

[Daniel Dadap <ddadap@nvidia.com>]

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@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.


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
>