On 9/2/21 4:35 AM, Andy Shevchenko wrote:
On Thu, Sep 2, 2021 at 5:37 AM Daniel Dadap<ddadap@xxxxxxxxxx> wrote:
On 9/1/21 10:57 AM, Andy Shevchenko wrote:
On Wed, Sep 1, 2021 at 2:27 AM Daniel Dadap<ddadap@xxxxxxxxxx> wrote:
...
+#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.
No, it's because of the ID table type which leaves there.
Basically you have to find a compromise between what to include and
what to not. At least those I have mentioned are kinda core stuff that
usually should be included explicitly in the modules like this driver.
Okay, thanks. I suppose I should have <linux/acpi.h> as well: I
originally had that on there, then removed it when I realized that it's
included implicitly via <linux/wmi.h>, but I do use some ACPI types and
macros that are defined in that header.
...
+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.
Yes, that counts as FW and as you noticed out of the Linux kernel realm.
+};
...
+ */
+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.
Not only, the _MAX in the above enum is correct in leaving commas out.
No, I think it does need a comma, unless I'm misunderstanding why you're
saying it doesn't. WMAA_GET_MAX here isn't saying "this is the final
element of the enum which is also a count of the 'real' enum values";
it's saying "retrieve the maximum valid brightness level from the
firmware". I renamed the enumerant to WMAA_GET_MAX_LEVEL to avoid
aliasing with the common "_MAX" convention for the final value defined
in an enum.
+};
...
+ backlight = devm_backlight_device_register(
+ &w->dev, "wmaa_backlight",
Strange indentation, btw.
Indeed. I'm not totally sure why I did it that way, maybe because the
long variable and function name didn't leave much space for arguments to
indent normally? I shortened the variable name and reindented it.
+ &w->dev, w, &wmaa_backlight_ops, &props);
