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@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.
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@xxxxxxxxxx>
...
+#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@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).
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.
