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

[Carlo Caione]

On Thu, Apr 6, 2017 at 11:11 AM, Benjamin Tissoires
 wrote:
> Hi Carlo,

Hi Benjamin,

[cut]
>> +static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t 
>> buf_size)
>
> Nitpick: should be asus_kbd_set_report()

right :)

[cut]
>> +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())

Yeah, makes sense.

[cut]
>> + /* 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)

ok for the next one but when checking the bit field in kbd_func I
think we still want to return -ENODEV.

>>   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.

yeah, this slipped through

>>   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))

OK

[cut]
>> +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().

I'll do.

Thanks for the quick review.

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

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

[Carlo Caione]

On Thu, Apr 6, 2017 at 11:11 AM, Benjamin Tissoires
 wrote:
> Hi Carlo,

Hi Benjamin,

[cut]
>> +static int asus_kbd_get_report(struct hid_device *hdev, u8 *buf, size_t 
>> buf_size)
>
> Nitpick: should be asus_kbd_set_report()

right :)

[cut]
>> +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())

Yeah, makes sense.

[cut]
>> + /* 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)

ok for the next one but when checking the bit field in kbd_func I
think we still want to return -ENODEV.

>>   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.

yeah, this slipped through

>>   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))

OK

[cut]
>> +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().

I'll do.

Thanks for the quick review.

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

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

[Benjamin Tissoires]

Hi Carlo,

On Apr 05 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

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,
> +   

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

[Benjamin Tissoires]

Hi Carlo,

On Apr 05 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

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);
> +