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

[Jiri Kosina]

On Wed, 12 Apr 2017, Carlo Caione wrote:

> > I'll drop the cancel_work_sync() call while doing it, ok?
> 
> Sure, go for it.

Applied to hid.git#for-4.12/asus. Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Wed, 12 Apr 2017, Carlo Caione wrote:

> > I'll drop the cancel_work_sync() call while doing it, ok?
> 
> Sure, go for it.

Applied to hid.git#for-4.12/asus. Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Carlo Caione]

On Wed, Apr 12, 2017 at 10:12 PM, Jiri Kosina  wrote:
> On Wed, 12 Apr 2017, Carlo Caione wrote:
>
>> >> + ret = devm_led_classdev_register(>dev, 
>> >> >kbd_backlight->cdev);
>> >> + if (ret < 0) {
>> >> + /* No need to have this still around */
>> >> + devm_kfree(>dev, drvdata->kbd_backlight);
>> >> + cancel_work_sync(>kbd_backlight->work);
>> >
>> > Small nitpick, you don't need to call cancel_work_sync() here, nobody
>> > could have called a worker. But OTOH, it doesn't hurt.
>> >
>> > Reviewed-by: Benjamin Tissoires 
>>
>> Thanks Benjamin. Who should pick this up?
>
> I am going to.
>
> I'll drop the cancel_work_sync() call while doing it, ok?

Sure, go for it.

Thank you,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

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

[Carlo Caione]

On Wed, Apr 12, 2017 at 10:12 PM, Jiri Kosina  wrote:
> On Wed, 12 Apr 2017, Carlo Caione wrote:
>
>> >> + ret = devm_led_classdev_register(>dev, 
>> >> >kbd_backlight->cdev);
>> >> + if (ret < 0) {
>> >> + /* No need to have this still around */
>> >> + devm_kfree(>dev, drvdata->kbd_backlight);
>> >> + cancel_work_sync(>kbd_backlight->work);
>> >
>> > Small nitpick, you don't need to call cancel_work_sync() here, nobody
>> > could have called a worker. But OTOH, it doesn't hurt.
>> >
>> > Reviewed-by: Benjamin Tissoires 
>>
>> Thanks Benjamin. Who should pick this up?
>
> I am going to.
>
> I'll drop the cancel_work_sync() call while doing it, ok?

Sure, go for it.

Thank you,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

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

[Jiri Kosina]

On Wed, 12 Apr 2017, Carlo Caione wrote:

> >> + ret = devm_led_classdev_register(>dev, 
> >> >kbd_backlight->cdev);
> >> + if (ret < 0) {
> >> + /* No need to have this still around */
> >> + devm_kfree(>dev, drvdata->kbd_backlight);
> >> + cancel_work_sync(>kbd_backlight->work);
> >
> > Small nitpick, you don't need to call cancel_work_sync() here, nobody
> > could have called a worker. But OTOH, it doesn't hurt.
> >
> > Reviewed-by: Benjamin Tissoires 
> 
> Thanks Benjamin. Who should pick this up?

I am going to.

I'll drop the cancel_work_sync() call while doing it, ok?

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Wed, 12 Apr 2017, Carlo Caione wrote:

> >> + ret = devm_led_classdev_register(>dev, 
> >> >kbd_backlight->cdev);
> >> + if (ret < 0) {
> >> + /* No need to have this still around */
> >> + devm_kfree(>dev, drvdata->kbd_backlight);
> >> + cancel_work_sync(>kbd_backlight->work);
> >
> > Small nitpick, you don't need to call cancel_work_sync() here, nobody
> > could have called a worker. But OTOH, it doesn't hurt.
> >
> > Reviewed-by: Benjamin Tissoires 
> 
> Thanks Benjamin. Who should pick this up?

I am going to.

I'll drop the cancel_work_sync() call while doing it, ok?

-- 
Jiri Kosina
SUSE Labs

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

[Carlo Caione]

On Mon, Apr 10, 2017 at 8:39 AM, Benjamin Tissoires
 wrote:
> On Apr 06 2017 or thereabouts, Carlo Caione wrote:

>> + ret = devm_led_classdev_register(>dev, 
>> >kbd_backlight->cdev);
>> + if (ret < 0) {
>> + /* No need to have this still around */
>> + devm_kfree(>dev, drvdata->kbd_backlight);
>> + cancel_work_sync(>kbd_backlight->work);
>
> Small nitpick, you don't need to call cancel_work_sync() here, nobody
> could have called a worker. But OTOH, it doesn't hurt.
>
> Reviewed-by: Benjamin Tissoires 

Thanks Benjamin. Who should pick this up?

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

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

[Carlo Caione]

On Mon, Apr 10, 2017 at 8:39 AM, Benjamin Tissoires
 wrote:
> On Apr 06 2017 or thereabouts, Carlo Caione wrote:

>> + ret = devm_led_classdev_register(>dev, 
>> >kbd_backlight->cdev);
>> + if (ret < 0) {
>> + /* No need to have this still around */
>> + devm_kfree(>dev, drvdata->kbd_backlight);
>> + cancel_work_sync(>kbd_backlight->work);
>
> Small nitpick, you don't need to call cancel_work_sync() here, nobody
> could have called a worker. But OTOH, it doesn't hurt.
>
> Reviewed-by: Benjamin Tissoires 

Thanks Benjamin. Who should pick this up?

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

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

[Benjamin Tissoires]

On Apr 06 2017 or thereabouts, Carlo Caione wrote:
> From: Carlo Caione 
> 
> 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 
> ---
> 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
> 
> v3:
>  - s/asus_kbd_get_report/asus_kbd_set_report
>  - Added cancel_work_sync()
>  - Return error from asus_kbd_register_leds()
>  - Do not leak drvdata->kbd_backlight on devm_led_classdev_register()
>failure
>  - Coding style
> ---
>  drivers/hid/Kconfig|   1 +
>  drivers/hid/hid-asus.c | 184 
> -
>  2 files changed, 184 insertions(+), 1 deletion(-)
> 
> 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..e78c7de 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,149 @@ static int asus_raw_event(struct hid_device *hdev,
>   return 0;
>  }
>  
> +static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t 
> buf_size)
> +{
> + 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_set_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_set_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, 

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

[Benjamin Tissoires]

On Apr 06 2017 or thereabouts, Carlo Caione wrote:
> From: Carlo Caione 
> 
> 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 
> ---
> 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
> 
> v3:
>  - s/asus_kbd_get_report/asus_kbd_set_report
>  - Added cancel_work_sync()
>  - Return error from asus_kbd_register_leds()
>  - Do not leak drvdata->kbd_backlight on devm_led_classdev_register()
>failure
>  - Coding style
> ---
>  drivers/hid/Kconfig|   1 +
>  drivers/hid/hid-asus.c | 184 
> -
>  2 files changed, 184 insertions(+), 1 deletion(-)
> 
> 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..e78c7de 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,149 @@ static int asus_raw_event(struct hid_device *hdev,
>   return 0;
>  }
>  
> +static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t 
> buf_size)
> +{
> + 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_set_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_set_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,
> +