On Mon, Sep 24, 2018 at 6:17 PM Matan Ziv-Av <matan@xxxxxxxxxxx> wrote: > after a long time, but here is my second version of the LG laptop > support patch. Thank you for an update. Unfortunately more work is needed. My review below. > The dump of ACPI can be found at http://my.svgalib.org/acpi/ . > > From: Matan Ziv-Av <matan@xxxxxxxxxxx> First of all, please send new version separately with proper commit message and versioning (hint: -v parameter to git format-patch). > +Date: September 2018 > +KernelVersion: 4.19 For sure these will be October 2018 4.20 > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * lg_wmi.c - LG Gram ACPI features and hotkeys Driver > + * > + * Copyright (C) 2018 Matan Ziv-Av <matan@xxxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * The idea of SPDX IDs is to get rid of the above boilerplate text. > + */ > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> Should be either one of the above two. > +#include <linux/types.h> > +#include <linux/acpi.h> > +#include <linux/platform_device.h> > +#include <linux/input.h> > +#include <linux/input/sparse-keymap.h> > +#include <acpi/acpi_bus.h> > +#include <acpi/acpi_drivers.h> I bet you don't need this two. > +#include <linux/leds.h> Keep them sorted. > + > +#define LED_DEVICE(_name, max) struct led_classdev _name = { \ > + .name = __stringify(_name), \ > + .max_brightness = max, \ > + .brightness_set = _name##_set, \ > + .brightness_get = _name##_get, \ > +} > +#define WMI_EVENT_GUID0 "E4FB94F9-7F2B-4173-AD1A-CD1D95086248" > +#define WMI_EVENT_GUID1 "023B133E-49D1-4E10-B313-698220140DC2" > +#define WMI_EVENT_GUID2 "37BE1AC0-C3F2-4B1F-BFBE-8FDEAF2814D6" > +#define WMI_EVENT_GUID3 "911BAD44-7DF8-4FBB-9319-BABA1C4B293B" > +#define WMI_METHOD_WMAB "C3A72B38-D3EF-42D3-8CBB-D5A57049F66D" > +#define WMI_METHOD_WMBB "2B4F501A-BD3C-4394-8DCF-00A7D2BC8210" > +#define WMI_EVENT_GUID WMI_EVENT_GUID0 > + > +#define WMAB_METHOD "\\XINI.WMAB" > +#define WMBB_METHOD "\\XINI.WMBB" > +#define SB_GGOV_METHOD "\\_SB.GGOV" > +#define GOV_TLED 0x2020008 > +#define WM_GET 1 > +#define WM_SET 2 > +#define WM_KEY_LIGHT 0x400 > +#define WM_TLED 0x404 > +#define WM_FN_LOCK 0x407 > +#define WM_BATT_LIMIT 0x61 > +#define WM_READER_MODE 0xBF > +#define WM_FAN_MODE 0x33 > +#define WMBB_BATT_LIMIT 0x10C Above contains some indentation issues. > +static const struct key_entry wmi_keymap[] __initconst = { > + /* TODO: Add keymap values once found... */ > + {KE_KEY, 0x70, {KEY_F15} }, /* LG control panel (F1) */ > + {KE_KEY, 0x74, {KEY_F13} }, /* Touchpad toggle (F5) */ > + {KE_KEY, 0xf020000, {KEY_F14} }, /* Read mode (F9) */ > + {KE_KEY, 0x10000000, {KEY_F16} },/* Keyboard backlight (F8) - changes > + * backlight and sends an event > + */ > + {KE_KEY, 0x80, {KEY_RFKILL} }, > + {KE_END, 0} > +}; > + > +static int ggov(u32 arg0) > +{ > + args[0].type = ACPI_TYPE_INTEGER; > + args[0].integer.value = arg0; > + > + status = acpi_get_handle(NULL, (acpi_string) SB_GGOV_METHOD, &handle); > + if (ACPI_FAILURE(status)) { > + pr_err("lg_wmi: Cannot get handle"); If you define pr_fmt() you don't need to repeat the name in each message. > + return -1; Consider to use appropriate -ERRNO instead. > + } > + status = acpi_evaluate_object(handle, NULL, &arg, &buffer); > + if (ACPI_FAILURE(status)) { > + pr_err("lg_wmi: Call failed.\n"); acpi_handle_err() > + return -2; -ERRNO > + } > + > + r = buffer.pointer; > + if (r->type != ACPI_TYPE_INTEGER) { > + kfree(r); > + return -2; -ERRNO > + } > + > + res = r->integer.value; > + kfree(r); > + > + return res; > +} > + > +static union acpi_object *lg_wmab(u32 method, u32 arg1, u32 arg2, > + u32 *retval) > +{ > + status = acpi_get_handle(NULL, (acpi_string) WMAB_METHOD, &handle); > + if (ACPI_FAILURE(status)) { > + pr_err("lg_wmi: Cannot get handle"); pr_fmt() > + return NULL; > + } > + status = acpi_evaluate_object(handle, NULL, &arg, &buffer); > + if (ACPI_FAILURE(status)) { > + pr_err("lg_wmi: Call failed.\n"); acpi_handle_err(); > + return NULL; > + } > + > + return buffer.pointer; > +} > +// *************************************************************************** Please, drop these fancy delimiter lines. > +static void wmi_notify(u32 value, void *context) > +{ > + pr_debug("event guid %li\n", data); If you supply some structure which has a pointer to input device, you may use dev_dbg() and Co for printing in this function. > +} > +static void acpi_notify(struct acpi_device *device, u32 event) > +{ > + struct key_entry *key; > + > + pr_debug("LGEX0815 notify: %d\n", event); acpi_handle_dbg() > + if (wmi_input_dev) { > + key = sparse_keymap_entry_from_scancode(wmi_input_dev, 0x80); > + if (key && key->type == KE_KEY) > + sparse_keymap_report_entry(wmi_input_dev, key, 1, true); > + } > + Redundant blank line. > +} > +static ssize_t fan_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buffer, size_t count) > +{ > + unsigned long value; > + union acpi_object *r; > + u32 m, v1, v2; > + int ret; > + ret = kstrtoul(buffer, 10, &value); > + if (!ret) > + return ret; > + > + if ((value != 0) && (value != 1)) > + return -EINVAL; kstrotobool() > + m = r->integer.value; > + kfree(r); > + v1 = (m&0xffffff0f) | (value<<4); > + v2 = (m&0xfffffff0) | value; I don't see why you need to variables for the above, just reuse v as you did for r. > + r = lg_wmab(WM_FAN_MODE, WM_SET, v1, NULL); > + kfree(r); > + r = lg_wmab(WM_FAN_MODE, WM_SET, v2, NULL); > + kfree(r); > + > + return count; > +} > + unsigned int status; > + Redundant blank line. > + union acpi_object *r; > + unsigned int status; > + Redundant blank line. > + union acpi_object *r; > +static const struct attribute_group dev_attribute_group = { > + .attrs = dev_attributes Keep comma here. > +}; > +static enum led_brightness tpad_led_get(struct led_classdev *cdev) > +{ > + int r; > + > + r = ggov(GOV_TLED); > + return r > 0 ? LED_ON : LED_OFF; Can be one line. > +} > +static void kbd_backlight_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + u32 val; > + union acpi_object *r; > + > + val = 0x22; > + if (brightness <= LED_OFF) > + val = 0; > + if (brightness >= LED_FULL) > + val = 0x24; See below. > + r = lg_wmab(WM_KEY_LIGHT, WM_SET, val, NULL); > + kfree(r); > +} > + > +static enum led_brightness kbd_backlight_get(struct led_classdev *cdev) > +{ > + union acpi_object *r; > + int val; > + > + r = lg_wmab(WM_KEY_LIGHT, WM_GET, 0, NULL); > + Redundant blank line. > + if (!r) > + return LED_OFF; > + switch (r->buffer.pointer[0] & 0x27) { > + case 0x24: > + val = LED_FULL; > + break; > + case 0x22: > + val = LED_HALF; > + break; > + default: > + val = LED_OFF; > + } Looking into this values it looks like BIT(5) defines on/off (or enabled/disabled) state and lowest three bits defines the value of brightness. Correct? If so, can it be rewritten taking above into account? > + > + kfree(r); > + > + return val; > +} > +static struct platform_driver pf_driver = { > + .driver = { > + .name = PLATFORM_NAME, > + } > +}; > + > +static int acpi_add(struct acpi_device *device) > +{ > + int ret = 0; Redundant assignment. > + /* don't run again if already initialized */ > + if (atomic_add_return(1, &pf_users) > 1) > + return 0; Oh, please use just a pf_device for that. Either it's NULL or not which is basically the same. > + > + ret = platform_driver_register(&pf_driver); > + if (ret) > + goto out; > + > + pf_device = platform_device_register_simple(PLATFORM_NAME, > + -1, NULL, 0); -1 has it's own definition. > + if (IS_ERR(pf_device)) { > + ret = PTR_ERR(pf_device); > + pf_device = NULL; > + pr_err("unable to register platform device\n"); > + goto out_platform_device; > + } > + > + ret = sysfs_create_group(&pf_device->dev.kobj, &dev_attribute_group); > + if (ret) > + goto out_platform_registered; > + > + kbd_backlight_registered = !led_classdev_register(&pf_device->dev, > + &kbd_backlight); > + tpad_led_registered = > + !led_classdev_register(&pf_device->dev, &tpad_led); I would rather suggest to provide one variable where bits would define what parts have been registered. Same for WMI part below. > + > + return 0; > +} > + > +static int acpi_remove(struct acpi_device *device) > +{ > + /* deregister only after the last user has gone */ > + if (!atomic_dec_and_test(&pf_users)) > + return -EBUSY; How many of them do you have? > +} > +static const struct acpi_device_id device_ids[] = { > + {"LGEX0815", 0}, > + {"", 0}, No comma for terminator line, please. > +}; > + Redundant blank line. > +MODULE_DEVICE_TABLE(acpi, device_ids); > + > +static struct acpi_driver acpi_driver = { > + .name = "LG", > + .class = "LG", Too short too broad. > + .ids = device_ids, > + .ops = { > + .add = acpi_add, > + .remove = acpi_remove, > + .notify = acpi_notify, > + }, > + .owner = THIS_MODULE, > +}; > +static int __init acpi_init(void) > +{ > + int result = 0; Redundant assignment. > + > + result = acpi_bus_register_driver(&acpi_driver); > + if (result < 0) { > + ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering driver\n")); > + return -ENODEV; > + } > + > + wmi_inited = !wmi_input_setup(); Hmm... So, it's not a fatal error if WMI is not initialized? > + > + return 0; > +} -- With Best Regards, Andy Shevchenko
