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.
...
> >> +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.
> >> +};
...
> >> + backlight = devm_backlight_device_register(
> >> + &w->dev, "wmaa_backlight",
Strange indentation, btw.
> >> + &w->dev, w, &wmaa_backlight_ops, &props);
--
With Best Regards,
Andy Shevchenko
