Niels, Sorry, just got back from holiday and am working my way through email. Is there a V2 for this? Maybe I missed it? If the maintainers have no issue with the duplicate functionalities then I have no further issues and am anxious to see V2 get applied. Regards, Terry >-----Original Message----- >From: Niels Skou Olsen [mailto:nolsen@xxxxxxxxx] >Sent: Thursday, August 18, 2016 2:00 AM >To: Junge, Terry; linux-input@xxxxxxxxxxxxxxx >Cc: jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx; dmitry.torokhov@xxxxxxxxx >Subject: RE: [PATCH] HID: Support telephony devices > >First of all thank you for review and good feedback! > >>Good to get telephony support into the kernel. >> >>My only concerns are the duplicate functionalities of LED_RING::LED_RINGER and LED_MUTE::LED_MICROPHONE. > >By adding RINGER and MICROPHONE we are trying to achieve finer grained >telephony control from user space. We understand your concern about the >user having to handle additional values that may be hard to distinguish. > >RINGER is a functionality that has a different meaning than RING in the HUT >spec. So, if a user space application really wants RINGER (or needs both) >then it should have the option. It is more flexible to make the choice >available in user space, than it is to require vendor specific drivers. Also, >having only RING would preclude applications from using both RING and >RINGER against devices that support both. > >As you point out, RINGER is not an LED usage, so changing the name will >help decrease confusion. But the telephony related usages do not fit well >with the current event types EV_KEY, EV_LED etc. The problem is that we are >trying to extend the existing event types. Maybe we need a new EV_TEL event >type, and a new TEL_ prefix for telephony related values: TEL_RINGER, >TEL_REDIAL etc.? > >> >>Please see below. >> > >Further comments interleaved below. > >>>-----Original Message----- >>>From: linux-input-owner@xxxxxxxxxxxxxxx >>>[mailto:linux-input-owner@xxxxxxxxxxxxxxx] On Behalf Of >>>nolsen@xxxxxxxxx >>>Sent: Wednesday, August 03, 2016 1:52 AM >>>To: linux-input@xxxxxxxxxxxxxxx >>>Cc: jikos@xxxxxxxxxx; benjamin.tissoires@xxxxxxxxxx; >>>dmitry.torokhov@xxxxxxxxx; Niels Skou Olsen >>>Subject: [PATCH] HID: Support telephony devices >>> >>>From: Niels Skou Olsen <nolsen@xxxxxxxxx> >>> >>>Add support for telephony device buttons and LEDs as described in the >>>USB HID Usage Tables specification. >>> >>>This allows user mode applications convenient access to telephony >>>devices such as headsets, speaker phones and desk phones. >>> >>>Signed-off-by: Niels Skou Olsen <nolsen@xxxxxxxxx> >>>--- >>> drivers/hid/hid-debug.c | 39 ++++++++++++++++++++- >>> drivers/hid/hid-input.c | 63 +++++++++++++++++++++++++++++++--- >>> drivers/input/input-leds.c | 5 +++ >>> include/linux/hid.h | 8 ++++- >>> include/linux/mod_devicetable.h | 2 +- >>> include/uapi/linux/input-event-codes.h | 18 +++++++++- >>> 6 files changed, 126 insertions(+), 9 deletions(-) >>> >>>diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c index >>>acfb522..9f109c3 100644 >>>--- a/drivers/hid/hid-debug.c >>>+++ b/drivers/hid/hid-debug.c >>>@@ -129,9 +129,42 @@ static const struct hid_usage_entry hid_usage_table[] = { >>> {0, 0x03, "ScrollLock"}, >>> {0, 0x04, "Compose"}, >>> {0, 0x05, "Kana"}, >>>+{0, 0x09, "Mute"}, >>>+{0, 0x17, "OffHook"}, >>>+{0, 0x18, "Ring"}, >>>+{0, 0x19, "MessageWaiting"}, >>>+{0, 0x20, "Hold"}, >>>+{0, 0x21, "Microphone"}, >>>+{0, 0x27, "Standby"}, > >>+{0, 0x2a, "Online"}, >>>+{0, 0x4c, "SystemSuspend"}, >>>+{0, 0x4d, "ExtPowerConnected"}, >>> {0, 0x4b, "GenericIndicator"}, >>> { 9, 0, "Button" }, >>> { 10, 0, "Ordinal" }, >>>+{ 11, 0, "Telephony" }, >>>+{0, 0x20, "KeyHookSwitch"}, >>>+{0, 0x21, "KeyFlash"}, >>>+{0, 0x23, "KeyHold"}, >>>+{0, 0x24, "KeyRedial"}, >>>+{0, 0x2f, "KeyMicMute"}, >>>+{0, 0x9e, "LedRinger"}, >> >>This is not an LED usage, it's a Locally Generated Tone usage, maybe "RingerTone"? > >Yes, you are right. A V2 patch is forthcoming, once we agree how to proceed. > >>>+{0, 0xb0, "KeyNumeric0"}, >>>+{0, 0xb1, "KeyNumeric1"}, >>>+{0, 0xb2, "KeyNumeric2"}, >>>+{0, 0xb3, "KeyNumeric3"}, >>>+{0, 0xb4, "KeyNumeric4"}, >>>+{0, 0xb5, "KeyNumeric5"}, >>>+{0, 0xb6, "KeyNumeric6"}, >>>+{0, 0xb7, "KeyNumeric7"}, >>>+{0, 0xb8, "KeyNumeric8"}, >>>+{0, 0xb9, "KeyNumeric9"}, >>>+{0, 0xba, "KeyNumericStar"}, >>>+{0, 0xbb, "KeyNumericPound"}, >>>+{0, 0xbc, "KeyNumericA"}, >>>+{0, 0xbd, "KeyNumericB"}, >>>+{0, 0xbe, "KeyNumericC"}, >>>+{0, 0xbf, "KeyNumericD"}, >>> { 12, 0, "Consumer" }, >>> {0, 0x238, "HorizontalWheel"}, >>> { 13, 0, "Digitizers" }, >>>@@ -998,7 +1031,11 @@ static const char *leds[LED_MAX + 1] = { >>> [LED_SCROLLL] = "ScrollLock",[LED_COMPOSE] = "Compose", >>> [LED_KANA] = "Kana",[LED_SLEEP] = "Sleep", >>> [LED_SUSPEND] = "Suspend",[LED_MUTE] = "Mute", >>>-[LED_MISC] = "Misc", >>>+[LED_MISC] = "Misc",[LED_MAIL] = "Mail", >>>+[LED_CHARGING] = "Charging",[LED_OFF_HOOK] = "Off-hook", >>>+[LED_RING] = "Ring",[LED_RINGER] = "Ringer", >> >>LED_RINGER duplicates the functionality of LED_RING to the user. >>Multiple vendor products in the field already respond to the proposed mapping of LED_RING by indicating inbound ring to the user. >>Adding LED_RINGER to the core and mapping it to 0x000b009e will require the user to decide which one to use, LED_RING or >LED_RINGER. >>If needed, the 0x000b009e usage should be mapped by a vendor driver to LED_RING so the user will only have to deal with >LED_RING. >> > >See reply at the top. > >>>+[LED_HOLD] = "Hold",[LED_MICROPHONE] = "Microphone", >> >>LED_MICROPHONE duplicates the functionality of LED_MUTE to the user. >>Multiple vendor products in the field already respond to the core mapping of LED_MUTE by muting the microphone. >>Adding LED_MICROPHONE to the core and mapping it to 0x00080021 will require the user to decide which one to use, LED_MUTE >or LED_MICROPHONE. >>If needed, the 0x00080021 usage should be mapped by a vendor driver to LED_MUTE so the user will only have to deal with >LED_MUTE. >> > >See reply at the top. > >>>+[LED_ONLINE] = "Online" >>> }; >>> >>> static const char *repeats[REP_MAX + 1] = { diff --git >>>a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index >>>bcfaf32..b2a4136 100644 >>>--- a/drivers/hid/hid-input.c >>>+++ b/drivers/hid/hid-input.c >>>@@ -509,9 +509,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel >>> if (field->report_count < 1) >>> goto ignore; >>> >>>-/* only LED usages are supported in output fields */ >>>+/* only LED and TELEPHONY usages are supported in output fields */ >>> if (field->report_type == HID_OUTPUT_REPORT && >>>-(usage->hid & HID_USAGE_PAGE) != HID_UP_LED) { >>>+(usage->hid & HID_USAGE_PAGE) != HID_UP_LED && >>>+(usage->hid & HID_USAGE_PAGE) != HID_UP_TELEPHONY) { >>>+ >>> goto ignore; >>> } >> >>This allowance for telephony page output mapping in order to map LED_RINGER would not be needed if the 0x000b009e mapping >were done by a vendor driver and the vendor driver was allowed access before the currently preemptive LED page test, like this: >> >>if (device->driver->input_mapping) { >>int ret = device->driver->input_mapping(device, hidinput, field, >>usage, &bit, &max); >>if (ret > 0) >>goto mapped; >>if (ret < 0) >>goto ignore; >>} >> >>/* only LED usages are supported in output fields */ >>if (field->report_type == HID_OUTPUT_REPORT && >>(usage->hid & HID_USAGE_PAGE) != HID_UP_LED) { >>goto ignore; >>} >> > >See reply at the top. > >>> >>>@@ -658,11 +660,32 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel >>> case 0x03: map_led (LED_SCROLLL); break; /* "Scroll Lock" */ >>> case 0x04: map_led (LED_COMPOSE); break; /* "Compose" */ >>> case 0x05: map_led (LED_KANA); break; /* "Kana" */ >>>-case 0x27: map_led (LED_SLEEP); break; /* "Stand-By" */ >>>-case 0x4c: map_led (LED_SUSPEND); break; /* "System Suspend" */ >>> case 0x09: map_led (LED_MUTE); break; /* "Mute" */ >>>-case 0x4b: map_led (LED_MISC); break; /* "Generic Indicator" */ >>>+case 0x17: /* "Off hook" */ >>>+map_led(LED_OFF_HOOK); >>>+break; >>>+case 0x18: /* "Ring" */ >>>+map_led(LED_RING); >>>+break; >>> case 0x19: map_led (LED_MAIL); break; /* "Message Waiting" */ >>>+case 0x20: /* "Hold" */ >>>+map_led(LED_HOLD); >>>+break; >>>+case 0x21: /* "Microphone" */ >>>+map_led(LED_MICROPHONE); >>>+break; >> >>As previous comment on duplicate functionality of LED_MICROPHONE and LED_MUTE. >>Usage 0x00080021 could be mapped to LED_MUTE in a vendor driver. > >See reply at the top. > >> >>>+case 0x27: /* "Stand-By" */ >>>+map_led(LED_SLEEP); >>>+break; >>>+case 0x2a: /* "Online" */ >>>+map_led(LED_ONLINE); >>>+break; >>>+case 0x4b: /* "Generic Indicator" */ >>>+map_led(LED_MISC); >>>+break; >>>+case 0x4c: /* "System Suspend" */ >>>+map_led(LED_SUSPEND); >>>+break; >>> case 0x4d: map_led (LED_CHARGING); break; /* "External Power Connected" */ >>> >>> default: goto ignore; >>>@@ -732,7 +755,37 @@ static void hidinput_configure_usage(struct >>>hid_input *hidinput, struct hid_fiel >>> >>> case HID_UP_TELEPHONY: >>> switch (usage->hid & HID_USAGE) { >>>+case 0x07: >>>+map_key_clear(KEY_PROGRAMMABLE); >>>+break; >> >>---- >>>+case 0x1e: >>>+map_led(LED_SPEAKER); >>>+break; >>---- >> >>This is in the wrong place, it should be in the HID_UP_LED case switch statement block. >>Speaker usage 0x1e is in the LED page not the telephony page. >> > >Yes, you are right. A V2 patch is forthcoming, once we agree how to proceed. > >>>+case 0x20: >>>+map_key_clear(KEY_HOOK_SWITCH); >>>+break; >>>+case 0x21: >>>+map_key_clear(KEY_FLASH); >>>+break; >>>+case 0x23: >>>+map_key_clear(KEY_HOLD); >>>+break; >>>+case 0x24: >>>+map_key_clear(KEY_REDIAL); >>>+break; >>>+case 0x2a: >>>+map_key_clear(KEY_LINE); >>>+break; >>>+case 0x2b: >>>+map_key_clear(KEY_SPEAKER_PHONE); >>>+break; >>> case 0x2f: map_key_clear(KEY_MICMUTE);break; >>>+case 0x50: >>>+map_key_clear(KEY_SPEED_DIAL); >>>+break; >>>+case 0x9e: >>>+map_led(LED_RINGER); >>>+break; >> >>As previous comment on duplicate functionality of LED_RINGER and LED_RING. >>Usage 0x000b009e could be mapped to LED_RING in a vendor driver. > >See reply at the top. > >> >>> case 0xb0: map_key_clear(KEY_NUMERIC_0);break; >>> case 0xb1: map_key_clear(KEY_NUMERIC_1);break; >>> case 0xb2: map_key_clear(KEY_NUMERIC_2);break; >>>diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c >>>index 766bf26..cf692df 100644 >>>--- a/drivers/input/input-leds.c >>>+++ b/drivers/input/input-leds.c >>>@@ -36,6 +36,11 @@ static const struct { >>> [LED_MISC]= { "misc" }, >>> [LED_MAIL]= { "mail" }, >>> [LED_CHARGING]= { "charging" }, >>>+[LED_OFF_HOOK]= { "off-hook" }, >>>+[LED_RING]= { "ring" }, >>>+[LED_HOLD]= { "hold" }, >>>+[LED_MICROPHONE] = { "microphone" }, >> >>As previous comments on duplicate functionality of LED_MICROPHONE and LED_MUTE. >>Is LED_RINGER missing here? > >If we change LED_RINGER to something else (maybe TEL_RINGER), then it doesn't belong here. > >> >>>+[LED_ONLINE]= { "online" }, >>> }; >>> >>> struct input_led { >>>diff --git a/include/linux/hid.h b/include/linux/hid.h index >>>75b66ec..0c91c89 100644 >>>--- a/include/linux/hid.h >>>+++ b/include/linux/hid.h >>>@@ -764,7 +764,13 @@ struct hid_ll_driver { >>> >>> /* 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 >= 0x00010000) && (a <= >>>0x00010008)) || (a == 0x00010080) || (a == 0x000c0001) || ((a >>>>= 0x000d0002) && (a <= 0x000d0006))) >>>+#define IS_INPUT_APPLICATION(a) (\ >>>+((a >= 0x00010000) && (a <= 0x00010008)) ||\ >>>+(a == 0x00010080) ||\ >>>+(a == 0x000b0005) ||\ >> >>This should be a range from 0x000b0001 through 0x000b0005 to include Phone (1), Answering Machine (2), Message Controls (3), >Handset (4), as well as Headset (5) collections. > >Agreed. Coming in V2. > >> >>>+(a == 0x000c0001) ||\ >>>+((a >= 0x000d0002) && (a <= 0x000d0006))) >>>+ >>> >>> /* HID core API */ >>> >>>diff --git a/include/linux/mod_devicetable.h >>>b/include/linux/mod_devicetable.h index ed84c07..e342d3f 100644 >>>--- a/include/linux/mod_devicetable.h >>>+++ b/include/linux/mod_devicetable.h >>>@@ -288,7 +288,7 @@ struct pcmcia_device_id { >>> #define INPUT_DEVICE_ID_REL_MAX0x0f >>> #define INPUT_DEVICE_ID_ABS_MAX0x3f >>> #define INPUT_DEVICE_ID_MSC_MAX0x07 >>>-#define INPUT_DEVICE_ID_LED_MAX0x0f >>>+#define INPUT_DEVICE_ID_LED_MAX0x11 >>> #define INPUT_DEVICE_ID_SND_MAX0x07 >>> #define INPUT_DEVICE_ID_FF_MAX0x7f >>> #define INPUT_DEVICE_ID_SW_MAX0x0f >>>diff --git a/include/uapi/linux/input-event-codes.h >>>b/include/uapi/linux/input-event-codes.h >>>index d6d071f..eaa368b 100644 >>>--- a/include/uapi/linux/input-event-codes.h >>>+++ b/include/uapi/linux/input-event-codes.h >>>@@ -593,6 +593,15 @@ >>> >>> #define KEY_ALS_TOGGLE0x230/* Ambient light sensor */ >>> >>>+#define KEY_HOOK_SWITCH0x231 >>>+#define KEY_FLASH0x232 >>>+#define KEY_HOLD0x233 >>>+#define KEY_REDIAL0x234 >>>+#define KEY_PROGRAMMABLE0x235 >>>+#define KEY_LINE0x236 >>>+#define KEY_SPEAKER_PHONE0x237 >>>+#define KEY_SPEED_DIAL0x238 >>>+ >>> #define KEY_BUTTONCONFIG0x240/* AL Button Configuration */ >>> #define KEY_TASKMANAGER0x241/* AL Task/Project Manager */ >>> #define KEY_JOURNAL0x242/* AL Log/Journal/Timecard */ >>>@@ -812,7 +821,14 @@ >>> #define LED_MISC0x08 >>> #define LED_MAIL0x09 >>> #define LED_CHARGING0x0a >>>-#define LED_MAX0x0f >>>+#define LED_OFF_HOOK0x0b >>>+#define LED_RING0x0c >>>+#define LED_RINGER0x0d >> >>As previous comments on duplicate functionality of LED_RINGER and LED_RING. > >See reply at the top. > >> >>>+#define LED_HOLD0x0e >>>+#define LED_MICROPHONE0x0f >> >>As previous comments on duplicate functionality of LED_MICROPHONE and LED_MUTE. > >See reply at the top. > >> >>>+#define LED_SPEAKER0x10 >>>+#define LED_ONLINE0x11 >>>+#define LED_MAX0x11 >>> #define LED_CNT(LED_MAX+1) >>> >>> /* >>>-- >>>2.7.4 >>> >> >>Thanks, >> >>Terry Junge >>Plantronics > >Thanks, > >Niels Skou Olsen >Jabra > >**** GN GROUP NOTICE - AUTOMATICALLY INSERTED**** The information in this e-mail (including attachments, if any) is >considered confidential and is intended only for the recipient(s) listed above. Any review, use, disclosure, distribution or copying of >this e-mail is prohibited except by or on behalf of the intended recipient. If you have received this email in error, please notify me >immediately by reply e-mail, delete this e-mail, and do not disclose its contents to anyone. Any opinions expressed in this e-mail are >those of the individual and not necessarily the GN group. Thank you. ******************** DISCLAIMER END >************************ -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
