On Wed, Sep 1, 2021 at 2:27 AM Daniel Dadap <ddadap@xxxxxxxxxx> 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@xxxxxxxxxx>
Seems like missed Co-developed-by. Or you are the wrong author. Fix accordingly.
> Signed-off-by: Daniel Dadap <ddadap@xxxxxxxxxx>
...
> +#include <linux/backlight.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
types.h ?
mod_devicetable.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.
> +};
...
> + */
> +enum wmaa_source {
> + WMAA_SOURCE_GPU = 1,
> + WMAA_SOURCE_EC = 2,
> + WMAA_SOURCE_AUX = 3
Missed comma.
> +};
...
> +/**
> + * 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);
...
> +MODULE_AUTHOR("Aaron Plattner <aplattner@xxxxxxxxxx>");
> +MODULE_AUTHOR("Daniel Dadap <ddadap@xxxxxxxxxx>");
So, Co-developed-by seems missing above.
...
> +MODULE_LICENSE("GPL v2");
"GPL" is enough (it's a synonim).
--
With Best Regards,
Andy Shevchenko
