On Wed, Aug 25, 2021 at 1:09 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. I tried to avoid repetition of the comments given by others. So, mine below. ... > +config WMAA_BACKLIGHT_WMI > + tristate "ACPI WMAA Backlight Driver" > + depends on ACPI_WMI > + depends on BACKLIGHT_CLASS_DEVICE > + help > + This driver provides a sysfs backlight interface for notebook > + systems which expose the WMAA ACPI method and an associated WMI > + wrapper to drive LCD backlight levels through the system's > + Embedded Controller. Please, add a sentence to tell how the module will be called. There are plenty of examples in the kernel. ... > +struct wmaa_args { > + u32 set; > + u32 val; > + u32 ret; > + u32 ignored[3]; > +}; I guess this structure deserves a kernel doc. ... > +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { > + { .guid_string = WMAA_WMI_GUID }, > + { }, No comma for termination. > +}; ... > +static int wmaa_backlight_get_brightness(struct backlight_device *bd) > +{ > + u32 level; > + int ret; > + > + ret = wmaa_get_brightness(&level); > + WARN_ON(ret != 0); Why? > + return ret == 0 ? level : 0; > +} ... > +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) > +{ > + struct backlight_properties props = {0}; {} is slightly better. > + struct wmi_wmaa_priv *priv; > + u32 source; > + int ret; > + > + priv = devm_kmalloc(&w->dev, sizeof(*priv), GFP_KERNEL); > + if(!priv) > + return -ENOMEM; > + wdev = w; I'm wondering if it's possible to avoid having a global variable. > + ret = wmaa_get_brightness_source(&source); > + if (ret) > + goto done; > + > + if (source != WMAA_SOURCE_EC) { > + ret = -ENODEV; > + goto done; > + } > + > + // Register a backlight handler > + props.type = BACKLIGHT_PLATFORM; > + ret = wmaa_get_max_brightness(&props.max_brightness); > + if (ret) > + goto done; > + > + ret = wmaa_get_brightness(&props.brightness); > + if (ret) > + goto done; > + > + priv->backlight = backlight_device_register("wmaa_backlight", > + NULL, NULL, &wmaa_backlight_ops, &props); > + if (IS_ERR(priv->backlight)) > + return PTR_ERR(priv->backlight); > + > + dev_set_drvdata(&w->dev, priv); > +done: Useless. Return directly. > + return ret; > +} ... > +static struct wmi_driver wmaa_backlight_wmi_driver = { > + .driver = { > + .name = "wmaa-backlight", > + }, > + .probe = wmaa_backlight_wmi_probe, > + .remove = wmaa_backlight_wmi_remove, > + .id_table = wmaa_backlight_wmi_id_table, > +}; > + Redundant blank line. > +module_wmi_driver(wmaa_backlight_wmi_driver); ... > + > +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); Can you move this closer to GUID? But I'm not sure what is the preferred style. Hans? -- With Best Regards, Andy Shevchenko