Re: [PATCH v2] HID: asus: support backlight on USB keyboards

From: Benjamin Tissoires
Date: Thu Apr 06 2017 - 05:12:30 EST

Next message: Greg Kroah-Hartman: "[PATCH 4.10 15/81] xfs: update ctime and mtime on clone destinatation inodes"
Previous message: Juergen Gross: "Re: [PATCH] x86/xen/time: set ->min_delta_ticks and ->max_delta_ticks"
In reply to: Carlo Caione: "[PATCH v2] HID: asus: support backlight on USB keyboards"
Next in thread: Carlo Caione: "Re: [PATCH v2] HID: asus: support backlight on USB keyboards"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Carlo,

On Apr 05 2017 or thereabouts, Carlo Caione wrote:
> From: Carlo Caione <carlo@xxxxxxxxxxxx>
>
> The latest USB keyboards shipped on several ASUS laptop models
> (including ROG laptop models such as GL702VMK) have the keyboards
> backlight controlled by the keyboard firmware.
>
> The firmware implements at least 3 different commands:
> - Init command (to use when the system starts)
> - Configuration command (to get keyboard status/information)
> - Backlight level control (to change the level of the keyboard light)
>
> With this patch we create the usual 'asus::kbd_backlight' led class
> entry to control the keyboard backlight.
>
> Signed-off-by: Carlo Caione <carlo@xxxxxxxxxxxx>
> ---
> Changelog:
>
> v2:
> - Code refactoring
> - s/sysfs/led class
> - No error messages on kzalloc/kmemdup failing
> - Fixed race condition when removing device
> - Used devm_* when possible
> - Added LEDS_CLASS dependency

Looks much better now. Thanks for the quick respin. I still have few
issues to tackle, but we should be good at the next version IMO.

Cheers,
Benjamin

> ---
> drivers/hid/Kconfig | 1 +
> drivers/hid/hid-asus.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 173 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index eb1846a..dec5f3e 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -136,6 +136,7 @@ config HID_APPLEIR
>
> config HID_ASUS
> tristate "Asus"
> + depends on LEDS_CLASS
> ---help---
> Support for Asus notebook built-in keyboard and touchpad via i2c, and
> the Asus Republic of Gamers laptop keyboard special keys.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index bacba97..845e68a 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -40,8 +40,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>
> #define FEATURE_REPORT_ID 0x0d
> #define INPUT_REPORT_ID 0x5d
> +#define FEATURE_KBD_REPORT_ID 0x5a
>
> #define INPUT_REPORT_SIZE 28
> +#define FEATURE_KBD_REPORT_SIZE 16
> +
> +#define SUPPORT_KBD_BACKLIGHT BIT(0)
>
> #define MAX_CONTACTS 5
>
> @@ -64,6 +68,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define QUIRK_SKIP_INPUT_MAPPING BIT(2)
> #define QUIRK_IS_MULTITOUCH BIT(3)
> #define QUIRK_NO_CONSUMER_USAGES BIT(4)
> +#define QUIRK_USE_KBD_BACKLIGHT BIT(5)
>
> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> QUIRK_NO_INIT_REPORTS | \
> @@ -74,9 +79,19 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>
> #define TRKID_SGN ((TRKID_MAX + 1) >> 1)
>
> +struct asus_kbd_leds {
> + struct led_classdev cdev;
> + struct hid_device *hdev;
> + struct work_struct work;
> + unsigned int brightness;
> + bool removed;
> +};
> +
> struct asus_drvdata {
> unsigned long quirks;
> struct input_dev *input;
> + struct asus_kbd_leds *kbd_backlight;
> + bool enable_backlight;
> };
>
> static void asus_report_contact_down(struct input_dev *input,
> @@ -171,6 +186,139 @@ static int asus_raw_event(struct hid_device *hdev,
> return 0;
> }
>
> +static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t buf_size)

Nitpick: should be asus_kbd_set_report()

> +{
> + unsigned char *dmabuf;
> + int ret;
> +
> + dmabuf = kmemdup(buf, buf_size, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
> + buf_size, HID_FEATURE_REPORT,
> + HID_REQ_SET_REPORT);
> + kfree(dmabuf);
> +
> + return ret;
> +}
> +
> +static int asus_kbd_init(struct hid_device *hdev)
> +{
> + u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
> + 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> + int ret;
> +
> + ret = asus_kbd_get_report(hdev, buf, sizeof(buf));
> + if (ret < 0)
> + hid_err(hdev, "Asus failed to send init command: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int asus_kbd_get_functions(struct hid_device *hdev,
> + unsigned char *kbd_func)
> +{
> + u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x05, 0x20, 0x31, 0x00, 0x08 };
> + u8 *readbuf;
> + int ret;
> +
> + ret = asus_kbd_get_report(hdev, buf, sizeof(buf));
> + if (ret < 0) {
> + hid_err(hdev, "Asus failed to send configuration command: %d\n", ret);
> + return ret;
> + }
> +
> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
> + if (!readbuf)
> + return -ENOMEM;
> +
> + ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> + if (ret < 0) {
> + hid_err(hdev, "Asus failed to request functions: %d\n", ret);
> + kfree(readbuf);
> + return ret;
> + }
> +
> + *kbd_func = readbuf[6];
> +
> + kfree(readbuf);
> + return ret;
> +}
> +
> +static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
> + cdev);
> + if (led->brightness == brightness)
> + return;
> +
> + led->brightness = brightness;
> + schedule_work(&led->work);
> +}
> +
> +static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
> +{
> + struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
> + cdev);
> +
> + return led->brightness;
> +}
> +
> +static void asus_kbd_backlight_work(struct work_struct *work)
> +{
> + struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
> + u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
> + int ret;
> +

You should probably protect leds->removed by a mutex here to avoid
having it set to false right after the test. (or not, see asus_remove())

> + if (led->removed)
> + return;
> +
> + buf[4] = led->brightness;
> +
> + ret = asus_kbd_get_report(led->hdev, buf, sizeof(buf));
> + if (ret < 0)
> + hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> +}
> +
> +static int asus_kbd_register_leds(struct hid_device *hdev)
> +{
> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + unsigned char kbd_func;
> +
> + /* Initialize keyboard */
> + if (asus_kbd_init(hdev) < 0)
> + return -ENODEV;

Don't hide the returned error code please. I know it'll be dropped in
the end, but better not hiding it with an other one. (same for the next
2)

> +
> + /* Get keyboard functions */
> + if (asus_kbd_get_functions(hdev, &kbd_func) < 0)
> + return -ENODEV;
> +
> + /* Check for backlight support */
> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> + return -ENODEV;
> +
> + drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> + sizeof(struct asus_kbd_leds),
> + GFP_KERNEL);
> + if (!drvdata->kbd_backlight)
> + return -ENOMEM;
> +
> + drvdata->kbd_backlight->removed = false;
> + drvdata->kbd_backlight->brightness = 0;
> + drvdata->kbd_backlight->hdev = hdev;
> + drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
> + drvdata->kbd_backlight->cdev.max_brightness = 3;
> + drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
> + drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
> + INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
> +
> + return devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
> +}
> +
> static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> {
> struct input_dev *input = hi->input;
> @@ -178,7 +326,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>
> if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
> int ret;
> -

Leftover from the previous patch, but please don't remove this empty
line, it's the coding style policy.

> input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
> input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
> input_set_abs_params(input, ABS_TOOL_WIDTH, 0, MAX_TOUCH_MAJOR, 0, 0);
> @@ -198,6 +345,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>
> drvdata->input = input;
>
> + if (drvdata->enable_backlight)
> + if (asus_kbd_register_leds(hdev))

could be changed into:
if (drvdata->enable_backlight && asus_kbd_register_leds(hdev))

> + hid_warn(hdev, "Failed to initialize backlight.\n");
> +
> return 0;
> }
>
> @@ -248,6 +399,16 @@ static int asus_input_mapping(struct hid_device *hdev,
> * as some make the keyboard appear as a pointer device. */
> return -1;
> }
> +
> + /*
> + * Check and enable backlight only on devices with UsagePage ==
> + * 0xff31 to avoid initializing the keyboard firmware multiple
> + * times on devices with multiple HID descriptors but same
> + * PID/VID.
> + */
> + if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
> + drvdata->enable_backlight = true;
> +
> return 1;
> }
>
> @@ -358,6 +519,14 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> +static void asus_remove(struct hid_device *hdev)
> +{
> + struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> + if (drvdata->kbd_backlight)
> + drvdata->kbd_backlight->removed = true;

Add a cancel_work_sync() here too to terminate currently working
workers. Bonus point, you don't need the mutex if you call
cancel_work_sync().

> +}
> +
> static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> @@ -379,7 +548,7 @@ static const struct hid_device_id asus_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1) },
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> - USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2) },
> + USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2), QUIRK_USE_KBD_BACKLIGHT },
> { }
> };
> MODULE_DEVICE_TABLE(hid, asus_devices);
> @@ -389,6 +558,7 @@ static struct hid_driver asus_driver = {
> .id_table = asus_devices,
> .report_fixup = asus_report_fixup,
> .probe = asus_probe,
> + .remove = asus_remove,
> .input_mapping = asus_input_mapping,
> .input_configured = asus_input_configured,
> #ifdef CONFIG_PM
> --
> 2.9.3
>

Next message: Greg Kroah-Hartman: "[PATCH 4.10 15/81] xfs: update ctime and mtime on clone destinatation inodes"
Previous message: Juergen Gross: "Re: [PATCH] x86/xen/time: set ->min_delta_ticks and ->max_delta_ticks"
In reply to: Carlo Caione: "[PATCH v2] HID: asus: support backlight on USB keyboards"
Next in thread: Carlo Caione: "Re: [PATCH v2] HID: asus: support backlight on USB keyboards"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]