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

[Daniel Dadap <ddadap@nvidia.com>]

Thanks again, Andy:

On 9/1/21 10:57 AM, Andy Shevchenko wrote:
> On Wed, Sep 1, 2021 at 2:27 AM Daniel Dadap <ddadap@nvidia.com> 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: Aaron Plattner <aplattner@nvidia.com>
> Seems like missed Co-developed-by. Or you are the wrong author. Fix accordingly.


Indeed; as Aaron mentioned, he was the author of a much earlier version 
of this driver which we used internally for testing purposes. He 
actually gave me his blessing to leave his name off the patch even 
before I submitted the original version of the patch for review on this 
list, but I figured it wasn't a big deal to leave him on. Since Aaron 
has suggested taking him off the patch again, I'll just go ahead and do 
that.


>
>> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
> ...
>
>> +#include <linux/backlight.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
> types.h ?
> mod_devicetable.h ?


Evidently, these must already be implicitly picked up by the headers 
which I am explicitly including here. I can see the argument for 
types.h, since it's always possible that future changes to the kernel 
might remove it from the implicit include chain, but I'm not certain why 
you're recommending mod_devicetable.h here. If it's because I use the 
MODULE_DEVICE_TABLE() macro, that seems to be defined in module.h.


>
> ...
>
>> +enum wmaa_get_or_set {
>> +       WMAA_GET = 0,
>> +       WMAA_SET = 1,
>> +       WMAA_GET_MAX = 2
> Is it part of HW protocol? Otherwise drop assignments.


Does ACPI count as HW here? I'd certainly prefer to keep the 
assignments, since the other side of this interface is not in the Linux 
kernel.


>
>> +};
> ...
>
>> + */
>> +enum wmaa_source {
>> +       WMAA_SOURCE_GPU = 1,
>> +       WMAA_SOURCE_EC = 2,
>> +       WMAA_SOURCE_AUX = 3
> Missed comma.


Oops. I am definitely a fan of using commas here, but I removed the 
commas that I had in place for the last elements of these enums, and 
members of static initialized structs, because I was trying to more 
broadly apply feedback from earlier to drop the terminal comma in the 
static initialized device table. I realize now that this feedback was 
meant only for the case of the empty struct terminator element in the 
device table.


>
>> +};
> ...
>
>> +/**
>> + * struct wmaa_args - arguments for the ACPI WMAA method
>> + *
> Redundant blank line.
>
>> + * @set:     Pass in an &enum wmaa_get_or_set value to select between getting or
>> + *           setting a value.
>> + * @val:     In parameter for value to set when operating in %WMAA_SET mode. Not
>> + *           used in %WMAA_GET or %WMAA_GET_MAX mode.
>> + * @ret:     Out parameter returning retrieved value when operating in %WMAA_GET
>> + *           or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode.
>> + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct.
>> + *
>> + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI.
>> + * The value passed in to @val or returned by @ret will be a brightness value
>> + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when
>> + * the WMI method ID is %WMAA_SOURCE.
>> + */
> ...
>
>> +       ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level);
>> +
> Ditto.
>
>> +       if (ret)
>> +               dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n");
> ...
>> +static const struct backlight_ops wmaa_backlight_ops = {
>> +       .update_status = wmaa_backlight_update_status,
>> +       .get_brightness = wmaa_backlight_get_brightness
> Missed comma.
>
>> +};
> ...
>
>> +       if (source != WMAA_SOURCE_EC) {
>> +               /* This driver is only to be used when brightness control is
>> +                * handled by the EC; otherwise, the GPU driver(s) should handle
>> +                * brightness control. */
> /*
>   * Wrong multi-line comment
>   * style. Use this example to fix
>   * it properly.
>   */
>
>> +               return -ENODEV;
>> +       }
>> +       /* Identify this backlight device as a platform device so that it can
>> +        * be prioritized over any exposed GPU-driven raw device(s). */
> Ditto.
> And for any other multi-line comments in this driver.
>
> ...
>
>> +       backlight = devm_backlight_device_register(
>> +               &w->dev, "wmaa_backlight",
>> +               &w->dev, w, &wmaa_backlight_ops, &props);
>> +       if (IS_ERR(backlight))
>> +               return PTR_ERR(backlight);
>> +
>> +       return 0;
> return PTR_ERR_OR_ZERO(backlight);
>
> ...


Thanks; Thomas made the same suggestion.


>
>> +MODULE_AUTHOR("Aaron Plattner <aplattner@nvidia.com>");
>> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>");
> So, Co-developed-by seems missing above.
>
> ...
>
>> +MODULE_LICENSE("GPL v2");
> "GPL" is enough (it's a synonim).


Indeed, Thomas pointed this out as well. I will do some retesting since 
I am changing the backlight driver type from BACKLIGHT_PLATFORM to 
BACKLIGHT_FIRMWARE, then send the updated patch.