Re: [PATCH] HID: input: support Microsoft wireless radio control hotkey
On Fri, Dec 7, 2018 at 5:07 AM Chris Chiu wrote: > > On Wed, Dec 5, 2018 at 8:56 AM Chris Chiu wrote: > > > > On Fri, Nov 30, 2018 at 7:18 PM Benjamin Tissoires > > wrote: > > > > > > On Fri, Nov 30, 2018 at 7:46 AM Chris Chiu wrote: > > > > > > > > The ASUS laptops start to support the airplane mode radio management > > > > to replace the original machanism of airplane mode toggle hotkey. > > > > On the ASUS P5440FF, it presents as a HID device connecting via > > > > I2C, name i2c-AMPD0001. When pressing hotkey, the Embedded Controller > > > > send hid report up via I2C and switch the airplane mode indicator > > > > LED based on the status. > > > > > > > > However, it's not working because it fails to be identified as a > > > > hidinput device. It fails in hidinput_connect() due to the macro > > > > IS_INPUT_APPLICATION doesn't identify HID_GD_WIRELESS_RADIO_CTLS > > > > as a legit application code. > > > > > > > > It's easy to add the HID I2C vendor and product id to the quirk > > > > list and apply HID_QUIRK_HIDINPUT_FORCE to make it work. But it > > > > can be more generic to support such kind of application on PC. > > > > > > Sounds good, but while you are at it, can you please: > > > - fix the kbuild warning > > > - rewrite the whole line to use macros, > > > - make the macro prettier by inserting new lines were required > > > - and define the missing macro: > > > 0x000d0006 -> HID_DG_WHITEBOARD > > > > > > Maybe we should keep the ranges definitions with raw values and put a > > > comment on the side with the names. > > > > > > Cheers, > > > Benjamin > > > > > > > Hi Bejamin, > > Thanks for your comment, I've submitted other patches which may > > fulfill your suggestion. > > https://lore.kernel.org/patchwork/patch/1020373/ > > https://lore.kernel.org/patchwork/patch/1020374/ > > > > Please help me review and correct me if there's any modification > > required. Thanks & Regards. > > > > Chris > > > > Gentle ping. Any comment for the new patch? Thanks and Regards. > Sorry for the delay. I broke my system and it took me a few days to get it back on its feet. The v2 is now scheduled for 4.21. Cheers, Benjamin
Re: [PATCH] HID: input: support Microsoft wireless radio control hotkey
On Wed, Dec 5, 2018 at 8:56 AM Chris Chiu wrote: > > On Fri, Nov 30, 2018 at 7:18 PM Benjamin Tissoires > wrote: > > > > On Fri, Nov 30, 2018 at 7:46 AM Chris Chiu wrote: > > > > > > The ASUS laptops start to support the airplane mode radio management > > > to replace the original machanism of airplane mode toggle hotkey. > > > On the ASUS P5440FF, it presents as a HID device connecting via > > > I2C, name i2c-AMPD0001. When pressing hotkey, the Embedded Controller > > > send hid report up via I2C and switch the airplane mode indicator > > > LED based on the status. > > > > > > However, it's not working because it fails to be identified as a > > > hidinput device. It fails in hidinput_connect() due to the macro > > > IS_INPUT_APPLICATION doesn't identify HID_GD_WIRELESS_RADIO_CTLS > > > as a legit application code. > > > > > > It's easy to add the HID I2C vendor and product id to the quirk > > > list and apply HID_QUIRK_HIDINPUT_FORCE to make it work. But it > > > can be more generic to support such kind of application on PC. > > > > Sounds good, but while you are at it, can you please: > > - fix the kbuild warning > > - rewrite the whole line to use macros, > > - make the macro prettier by inserting new lines were required > > - and define the missing macro: > > 0x000d0006 -> HID_DG_WHITEBOARD > > > > Maybe we should keep the ranges definitions with raw values and put a > > comment on the side with the names. > > > > Cheers, > > Benjamin > > > > Hi Bejamin, > Thanks for your comment, I've submitted other patches which may > fulfill your suggestion. > https://lore.kernel.org/patchwork/patch/1020373/ > https://lore.kernel.org/patchwork/patch/1020374/ > > Please help me review and correct me if there's any modification > required. Thanks & Regards. > > Chris > Gentle ping. Any comment for the new patch? Thanks and Regards. Chris > > > > > > Signed-off-by: Chris Chiu > > > --- > > > include/linux/hid.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > > index d44a78362942..f4805f605fed 100644 > > > --- a/include/linux/hid.h > > > +++ b/include/linux/hid.h > > > @@ -836,7 +836,7 @@ static inline bool hid_is_using_ll_driver(struct > > > hid_device *hdev, > > > > > > /* Applications from HID Usage Tables 4/8/99 Version 1.1 */ > > > /* We ignore a few input applications that are not widely used */ > > > -#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= > > > 0x00010008)) || (a == 0x00010080) || (a == 0x000c0001) || ((a >= > > > 0x000d0002) && (a <= 0x000d0006))) > > > +#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= > > > 0x00010008)) || (a == 0x00010080) || || (a == 0x0001000c) || (a == > > > 0x000c0001) || ((a >= 0x000d0002) && (a <= 0x000d0006))) > > > > > > /* HID core API */ > > > > > > -- > > > 2.11.0 > > >
Re: [PATCH] HID: input: support Microsoft wireless radio control hotkey
On Fri, Nov 30, 2018 at 7:18 PM Benjamin Tissoires wrote: > > On Fri, Nov 30, 2018 at 7:46 AM Chris Chiu wrote: > > > > The ASUS laptops start to support the airplane mode radio management > > to replace the original machanism of airplane mode toggle hotkey. > > On the ASUS P5440FF, it presents as a HID device connecting via > > I2C, name i2c-AMPD0001. When pressing hotkey, the Embedded Controller > > send hid report up via I2C and switch the airplane mode indicator > > LED based on the status. > > > > However, it's not working because it fails to be identified as a > > hidinput device. It fails in hidinput_connect() due to the macro > > IS_INPUT_APPLICATION doesn't identify HID_GD_WIRELESS_RADIO_CTLS > > as a legit application code. > > > > It's easy to add the HID I2C vendor and product id to the quirk > > list and apply HID_QUIRK_HIDINPUT_FORCE to make it work. But it > > can be more generic to support such kind of application on PC. > > Sounds good, but while you are at it, can you please: > - fix the kbuild warning > - rewrite the whole line to use macros, > - make the macro prettier by inserting new lines were required > - and define the missing macro: > 0x000d0006 -> HID_DG_WHITEBOARD > > Maybe we should keep the ranges definitions with raw values and put a > comment on the side with the names. > > Cheers, > Benjamin > Hi Bejamin, Thanks for your comment, I've submitted other patches which may fulfill your suggestion. https://lore.kernel.org/patchwork/patch/1020373/ https://lore.kernel.org/patchwork/patch/1020374/ Please help me review and correct me if there's any modification required. Thanks & Regards. Chris > > > > Signed-off-by: Chris Chiu > > --- > > include/linux/hid.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > index d44a78362942..f4805f605fed 100644 > > --- a/include/linux/hid.h > > +++ b/include/linux/hid.h > > @@ -836,7 +836,7 @@ static inline bool hid_is_using_ll_driver(struct > > hid_device *hdev, > > > > /* Applications from HID Usage Tables 4/8/99 Version 1.1 */ > > /* We ignore a few input applications that are not widely used */ > > -#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) > > || (a == 0x00010080) || (a == 0x000c0001) || ((a >= 0x000d0002) && (a <= > > 0x000d0006))) > > +#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) > > || (a == 0x00010080) || || (a == 0x0001000c) || (a == 0x000c0001) || ((a >= > > 0x000d0002) && (a <= 0x000d0006))) > > > > /* HID core API */ > > > > -- > > 2.11.0 > >
Re: [PATCH] HID: input: support Microsoft wireless radio control hotkey
Hi Chris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc4 next-20181130]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Chris-Chiu/HID-input-support-Microsoft-wireless-radio-control-hotkey/20181130-150723
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from drivers/hid/usbhid/hiddev.c:35:0:
drivers/hid/usbhid/hiddev.c: In function 'hiddev_connect':
include/linux/hid.h:839:99: error: expected expression before '||' token
#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008))
|| (a == 0x00010080) || || (a == 0x0001000c) || (a == 0x000c0001) || ((a >=
0x000d0002) && (a <= 0x000d0006)))
^
>> drivers/hid/usbhid/hiddev.c:906:9: note: in expansion of macro
>> 'IS_INPUT_APPLICATION'
!IS_INPUT_APPLICATION(hid->collection[i].usage))
^~~~
vim +/IS_INPUT_APPLICATION +906 drivers/hid/usbhid/hiddev.c
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 891
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 892 /*
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 893 *
This is where hid.c calls us to connect a hid device to the hiddev driver
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 894 */
93c10132 drivers/hid/usbhid/hiddev.c Jiri Slaby 2008-06-27 895 int
hiddev_connect(struct hid_device *hid, unsigned int force)
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 896 {
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 897
struct hiddev *hiddev;
4916b3a5 drivers/usb/input/hiddev.c Jiri Kosina2006-12-08 898
struct usbhid_device *usbhid = hid->driver_data;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 899
int retval;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 900
93c10132 drivers/hid/usbhid/hiddev.c Jiri Slaby 2008-06-27 901
if (!force) {
93c10132 drivers/hid/usbhid/hiddev.c Jiri Slaby 2008-06-27 902
unsigned int i;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 903
for (i = 0; i < hid->maxcollection; i++)
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 904
if (hid->collection[i].type ==
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 905
HID_COLLECTION_APPLICATION &&
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 @906
!IS_INPUT_APPLICATION(hid->collection[i].usage))
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 907
break;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 908
93c10132 drivers/hid/usbhid/hiddev.c Jiri Slaby 2008-06-27 909
if (i == hid->maxcollection)
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 910
return -1;
93c10132 drivers/hid/usbhid/hiddev.c Jiri Slaby 2008-06-27 911
}
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 912
bbdb7daf drivers/usb/input/hiddev.c Oliver Neukum 2006-01-06 913
if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 914
return -1;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 915
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 916
init_waitqueue_head(&hiddev->wait);
826d5982 drivers/usb/input/hiddev.c Dmitry Torokhov2006-07-19 917
INIT_LIST_HEAD(&hiddev->list);
cdcb44e8 drivers/hid/usbhid/hiddev.c Jiri Kosina2007-05-10 918
spin_lock_init(&hiddev->list_lock);
07903407 drivers/hid/usbhid/hiddev.c Oliver Neukum 2008-12-16 919
mutex_init(&hiddev->existancelock);
76052749 drivers/hid/usbhid/hiddev.c Jiri Kosina2009-01-07 920
hid->hiddev = hiddev;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 921
hiddev->hid = hid;
^1da177e drivers/usb/input/hiddev.c Linus Torvalds 2005-04-16 922
hiddev->exist = 1;
07903407 drivers/hid/usbhid/hiddev.c Oliver Neukum 2008-12-16 923
retval = usb_register_dev(usbhid->intf, &hiddev_class);
07903407 drivers/hid/usbhid/hiddev.c Oliver Neukum 2008-12-16 924
if (retval) {
4291ee30 drivers/hid/usbhid/hid
Re: [PATCH] HID: input: support Microsoft wireless radio control hotkey
On Fri, Nov 30, 2018 at 7:46 AM Chris Chiu wrote: > > The ASUS laptops start to support the airplane mode radio management > to replace the original machanism of airplane mode toggle hotkey. > On the ASUS P5440FF, it presents as a HID device connecting via > I2C, name i2c-AMPD0001. When pressing hotkey, the Embedded Controller > send hid report up via I2C and switch the airplane mode indicator > LED based on the status. > > However, it's not working because it fails to be identified as a > hidinput device. It fails in hidinput_connect() due to the macro > IS_INPUT_APPLICATION doesn't identify HID_GD_WIRELESS_RADIO_CTLS > as a legit application code. > > It's easy to add the HID I2C vendor and product id to the quirk > list and apply HID_QUIRK_HIDINPUT_FORCE to make it work. But it > can be more generic to support such kind of application on PC. Sounds good, but while you are at it, can you please: - fix the kbuild warning - rewrite the whole line to use macros, - make the macro prettier by inserting new lines were required - and define the missing macro: 0x000d0006 -> HID_DG_WHITEBOARD Maybe we should keep the ranges definitions with raw values and put a comment on the side with the names. Cheers, Benjamin > > Signed-off-by: Chris Chiu > --- > include/linux/hid.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/hid.h b/include/linux/hid.h > index d44a78362942..f4805f605fed 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -836,7 +836,7 @@ static inline bool hid_is_using_ll_driver(struct > hid_device *hdev, > > /* Applications from HID Usage Tables 4/8/99 Version 1.1 */ > /* We ignore a few input applications that are not widely used */ > -#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) || > (a == 0x00010080) || (a == 0x000c0001) || ((a >= 0x000d0002) && (a <= > 0x000d0006))) > +#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) || > (a == 0x00010080) || || (a == 0x0001000c) || (a == 0x000c0001) || ((a >= > 0x000d0002) && (a <= 0x000d0006))) > > /* HID core API */ > > -- > 2.11.0 >
Re: [PATCH] HID: input: support Microsoft wireless radio control hotkey
Hi Chris, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v4.20-rc4 next-20181130] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Chiu/HID-input-support-Microsoft-wireless-radio-control-hotkey/20181130-150723 config: m68k-sun3_defconfig (attached as .config) compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=m68k All error/warnings (new ones prefixed by >>): In file included from drivers/hid/hid-input.c:32:0: drivers/hid/hid-input.c: In function 'hidinput_connect': >> include/linux/hid.h:839:99: error: expected expression before '||' token #define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) || (a == 0x00010080) || || (a == 0x0001000c) || (a == 0x000c0001) || ((a >= 0x000d0002) && (a <= 0x000d0006))) ^ >> drivers/hid/hid-input.c:1737:9: note: in expansion of macro >> 'IS_INPUT_APPLICATION' if (IS_INPUT_APPLICATION(col->usage)) ^~~~ -- In file included from drivers//hid/hid-input.c:32:0: drivers//hid/hid-input.c: In function 'hidinput_connect': >> include/linux/hid.h:839:99: error: expected expression before '||' token #define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) || (a == 0x00010080) || || (a == 0x0001000c) || (a == 0x000c0001) || ((a >= 0x000d0002) && (a <= 0x000d0006))) ^ drivers//hid/hid-input.c:1737:9: note: in expansion of macro 'IS_INPUT_APPLICATION' if (IS_INPUT_APPLICATION(col->usage)) ^~~~ vim +839 include/linux/hid.h 836 837 /* Applications from HID Usage Tables 4/8/99 Version 1.1 */ 838 /* We ignore a few input applications that are not widely used */ > 839 #define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= > 0x00010008)) || (a == 0x00010080) || || (a == 0x0001000c) || (a == > 0x000c0001) || ((a >= 0x000d0002) && (a <= 0x000d0006))) 840 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH] HID: input: support Microsoft wireless radio control hotkey
The ASUS laptops start to support the airplane mode radio management to replace the original machanism of airplane mode toggle hotkey. On the ASUS P5440FF, it presents as a HID device connecting via I2C, name i2c-AMPD0001. When pressing hotkey, the Embedded Controller send hid report up via I2C and switch the airplane mode indicator LED based on the status. However, it's not working because it fails to be identified as a hidinput device. It fails in hidinput_connect() due to the macro IS_INPUT_APPLICATION doesn't identify HID_GD_WIRELESS_RADIO_CTLS as a legit application code. It's easy to add the HID I2C vendor and product id to the quirk list and apply HID_QUIRK_HIDINPUT_FORCE to make it work. But it can be more generic to support such kind of application on PC. Signed-off-by: Chris Chiu --- include/linux/hid.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/hid.h b/include/linux/hid.h index d44a78362942..f4805f605fed 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -836,7 +836,7 @@ static inline bool hid_is_using_ll_driver(struct hid_device *hdev, /* Applications from HID Usage Tables 4/8/99 Version 1.1 */ /* We ignore a few input applications that are not widely used */ -#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) || (a == 0x00010080) || (a == 0x000c0001) || ((a >= 0x000d0002) && (a <= 0x000d0006))) +#define IS_INPUT_APPLICATION(a) (((a >= 0x0001) && (a <= 0x00010008)) || (a == 0x00010080) || || (a == 0x0001000c) || (a == 0x000c0001) || ((a >= 0x000d0002) && (a <= 0x000d0006))) /* HID core API */ -- 2.11.0
