[PATCH 1/3] HID: input - Add microphone mute LED support

[PATCH 1/3] HID: input - Add microphone mute LED support

[Bernhard Seibold]

Define an input event code for micmute led and enable sending it via HID

Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
 drivers/hid/hid-input.c                | 1 +
 include/uapi/linux/input-event-codes.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index c8b20d44b147..96c595bb14ce 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -928,6 +928,7 @@ 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 0x21:  map_led (LED_MICMUTE);  break;    /*   "Microphone"               */
 		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"                     */
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 022a520e31fc..f9a4f9040c59 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -952,6 +952,7 @@
 #define LED_MISC		0x08
 #define LED_MAIL		0x09
 #define LED_CHARGING		0x0a
+#define LED_MICMUTE		0x0b
 #define LED_MAX			0x0f
 #define LED_CNT			(LED_MAX+1)
 
-- 
2.43.0

[PATCH 2/3] Input: leds - add micmute and triggers

[Bernhard Seibold]

Add microphone mute and add default triggers for mute and micmute

Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
 drivers/input/input-leds.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index 0e935914bc3a..327a7aea4f26 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -18,6 +18,12 @@
 #define VT_TRIGGER(_name)	.trigger = NULL
 #endif
 
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
+#define AUDIO_TRIGGER(_name)	.trigger = _name
+#else
+#define AUDIO_TRIGGER(_name)	.trigger = NULL
+#endif
+
 static const struct {
 	const char *name;
 	const char *trigger;
@@ -29,7 +35,8 @@ static const struct {
 	[LED_KANA]	= { "kana", VT_TRIGGER("kbd-kanalock") },
 	[LED_SLEEP]	= { "sleep" } ,
 	[LED_SUSPEND]	= { "suspend" },
-	[LED_MUTE]	= { "mute" },
+	[LED_MUTE]	= { "mute", AUDIO_TRIGGER("audio-mute") },
+	[LED_MICMUTE]	= { "micmute", AUDIO_TRIGGER("audio-micmute") },
 	[LED_MISC]	= { "misc" },
 	[LED_MAIL]	= { "mail" },
 	[LED_CHARGING]	= { "charging" },
-- 
2.43.0

[PATCH 3/3] HID: lenovo: Remove LEDs from tpkbd

[Bernhard Seibold]

Since both LEDs are now supported by the input-leds module, the custom
LEDs set via a proprietary method are no longer required. In fact they
are duplicates that might interfere with each other.

Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
 drivers/hid/hid-lenovo.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 149a3c74346b..e3eae18fd4ab 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1004,17 +1004,6 @@ static const struct attribute_group lenovo_attr_group_tpkbd = {
 	.attrs = lenovo_attributes_tpkbd,
 };
 
-static void lenovo_led_set_tpkbd(struct hid_device *hdev)
-{
-	struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
-	struct hid_report *report;
-
-	report = hdev->report_enum[HID_OUTPUT_REPORT].report_id_hash[3];
-	report->field[0]->value[0] = (data_pointer->led_state >> 0) & 1;
-	report->field[0]->value[1] = (data_pointer->led_state >> 1) & 1;
-	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
-}
-
 static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
@@ -1034,9 +1023,6 @@ static int lenovo_led_brightness_set(struct led_classdev *led_cdev,
 		data_pointer->led_state |= 1 << led_nr;
 
 	switch (hdev->product) {
-	case USB_DEVICE_ID_LENOVO_TPKBD:
-		lenovo_led_set_tpkbd(hdev);
-		break;
 	case USB_DEVICE_ID_LENOVO_TP10UBKBD:
 	case USB_DEVICE_ID_LENOVO_X1_TAB:
 		ret = lenovo_led_set_tp10ubkbd(hdev, tp10ubkbd_led[led_nr], value);
@@ -1128,10 +1114,6 @@ static int lenovo_probe_tpkbd(struct hid_device *hdev)
 
 	hid_set_drvdata(hdev, data_pointer);
 
-	ret = lenovo_register_leds(hdev);
-	if (ret)
-		goto err;
-
 	lenovo_features_set_tpkbd(hdev);
 
 	return 0;
@@ -1315,9 +1297,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
 
 	sysfs_remove_group(&hdev->dev.kobj,
 			&lenovo_attr_group_tpkbd);
-
-	led_classdev_unregister(&data_pointer->led_micmute);
-	led_classdev_unregister(&data_pointer->led_mute);
 }
 
 static void lenovo_remove_cptkbd(struct hid_device *hdev)
-- 
2.43.0

Re: [PATCH 1/3] HID: input - Add microphone mute LED support

[Dmitry Torokhov]

Hi Bernhard,

On Thu, Jan 04, 2024 at 04:49:39PM +0100, Bernhard Seibold wrote:
> Define an input event code for micmute led and enable sending it via HID
> 
> Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
> ---
>  drivers/hid/hid-input.c                | 1 +
>  include/uapi/linux/input-event-codes.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index c8b20d44b147..96c595bb14ce 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -928,6 +928,7 @@ 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 0x21:  map_led (LED_MICMUTE);  break;    /*   "Microphone"               */
>  		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"                     */
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 022a520e31fc..f9a4f9040c59 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -952,6 +952,7 @@
>  #define LED_MISC		0x08
>  #define LED_MAIL		0x09
>  #define LED_CHARGING		0x0a
> +#define LED_MICMUTE		0x0b

No, please do not add new LEDs to input subsystem, and instead use "new"
LED APIs that are much more flexible. That is why we built the input
leds bridge in direction of input->leds and on the other way around.

The existing input LED definitions are grandfathered because they are
exposed to userspace (via evdev), but we will not be adding new ones.

Thanks.

-- 
Dmitry

Re: Re: [PATCH 1/3] HID: input - Add microphone mute LED support

[Bernhard Seibold]

Hi Dmitri,

On Thu, Jan 04, 2024 at 09:55:42AM -0800, Dmitry Torokhov wrote:
> No, please do not add new LEDs to input subsystem, and instead use "new"
> LED APIs that are much more flexible. That is why we built the input
> leds bridge in direction of input->leds and on the other way around.
> 
> The existing input LED definitions are grandfathered because they are
> exposed to userspace (via evdev), but we will not be adding new ones.
> 
> Thanks.

Sorry, I'm completely new to this subsystem. Did I get it right that
what you're suggesting is to add something that is similar to
hidinput_setup_battery() and create the new LED there directly,
skipping the input subsystem and input-leds completely?

Regards,
Bernhard

Re: Re: [PATCH 1/3] HID: input - Add microphone mute LED support

[Dmitry Torokhov]

On Thu, Jan 04, 2024 at 07:46:32PM +0100, Bernhard Seibold wrote:
> Hi Dmitri,
> 
> On Thu, Jan 04, 2024 at 09:55:42AM -0800, Dmitry Torokhov wrote:
> > No, please do not add new LEDs to input subsystem, and instead use "new"
> > LED APIs that are much more flexible. That is why we built the input
> > leds bridge in direction of input->leds and on the other way around.
> > 
> > The existing input LED definitions are grandfathered because they are
> > exposed to userspace (via evdev), but we will not be adding new ones.
> > 
> > Thanks.
> 
> Sorry, I'm completely new to this subsystem. Did I get it right that
> what you're suggesting is to add something that is similar to
> hidinput_setup_battery() and create the new LED there directly,
> skipping the input subsystem and input-leds completely?

Yes, exactly.

Thanks.

-- 
Dmitry

[PATCH v2 1/2] Input: leds - set default-trigger for mute

[Bernhard Seibold]

Set the default-trigger for the mute led to audio-mute.

Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
 drivers/input/input-leds.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index 0e935914bc3a..b16fc81940f5 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -18,6 +18,12 @@
 #define VT_TRIGGER(_name)	.trigger = NULL
 #endif
 
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
+#define AUDIO_TRIGGER(_name)	.trigger = _name
+#else
+#define AUDIO_TRIGGER(_name)	.trigger = NULL
+#endif
+
 static const struct {
 	const char *name;
 	const char *trigger;
@@ -29,7 +35,7 @@ static const struct {
 	[LED_KANA]	= { "kana", VT_TRIGGER("kbd-kanalock") },
 	[LED_SLEEP]	= { "sleep" } ,
 	[LED_SUSPEND]	= { "suspend" },
-	[LED_MUTE]	= { "mute" },
+	[LED_MUTE]	= { "mute", AUDIO_TRIGGER("audio-mute") },
 	[LED_MISC]	= { "misc" },
 	[LED_MAIL]	= { "mail" },
 	[LED_CHARGING]	= { "charging" },
-- 
2.43.0

[PATCH v2 2/2] HID: input: add support for micmute LED

[Bernhard Seibold]

Since LED support via input-leds is grandfathered, the new LED is added
directly in hid-input.

Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de>
---
 drivers/hid/Kconfig     | 11 +++++
 drivers/hid/hid-input.c | 92 +++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h     |  1 +
 3 files changed, 104 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ce74af79657..71de0af8f460 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -42,6 +42,17 @@ config HID_BATTERY_STRENGTH
 	that support this feature) through power_supply class so that userspace
 	tools, such as upower, can display it.
 
+config HID_LEDS
+	bool "LED support for HID devices"
+	select LEDS_CLASS
+	default y
+	help
+	This option adds support for LEDs on HID devices. Currently, the
+	only supported LED is microphone mute. For all other LEDs,
+	enable CONFIG_INPUT_LEDS.
+
+	If unsure, say Y.
+
 config HIDRAW
 	bool "/dev/hidraw raw HID device support"
 	help
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index c8b20d44b147..32d3e6a2ac44 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/kernel.h>
+#include <linux/leds.h>
 
 #include <linux/hid.h>
 #include <linux/hid-debug.h>
@@ -104,6 +105,9 @@ static const struct usage_priority hidinput_usages_priorities[] = {
 #define map_key_clear(c)	hid_map_usage_clear(hidinput, usage, &bit, \
 		&max, EV_KEY, (c))
 
+#define setup_led(name, trigger) \
+	hidinput_setup_led(device, field, usage_index, name, trigger)
+
 static bool match_scancode(struct hid_usage *usage,
 			   unsigned int cur_idx, unsigned int scancode)
 {
@@ -674,6 +678,88 @@ static bool hidinput_set_battery_charge_status(struct hid_device *dev,
 }
 #endif	/* CONFIG_HID_BATTERY_STRENGTH */
 
+#ifdef CONFIG_HID_LEDS
+
+struct hid_led {
+	struct list_head list;
+	struct led_classdev cdev;
+	struct hid_field *field;
+	unsigned int offset;
+	char *name;
+};
+
+static int hidinput_led_brightness_set(struct led_classdev *cdev,
+		enum led_brightness value)
+{
+	struct device *dev = cdev->dev->parent;
+	struct hid_device *device = to_hid_device(dev);
+	struct hid_led *led = container_of(cdev, struct hid_led, cdev);
+
+	hid_set_field(led->field, led->offset, !!value);
+	schedule_work(&device->led_work);
+
+	return 0;
+}
+
+static void hidinput_setup_led(struct hid_device *device,
+		struct hid_field *field, unsigned int offset,
+		const char *name, const char *trigger)
+{
+	struct hid_led *led;
+	struct device *dev = &device->dev;
+	struct device *idev = &field->hidinput->input->dev;
+
+	led = kzalloc(sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return;
+
+	led->name = kasprintf(GFP_KERNEL, "%s::%s", dev_name(idev), name);
+	if (!led->name) {
+		kfree(led);
+		return;
+	}
+
+	led->cdev.name = led->name;
+	led->cdev.default_trigger = trigger;
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking = hidinput_led_brightness_set;
+	led->field = field;
+	led->offset = offset;
+
+	if (led_classdev_register(dev, &led->cdev)) {
+		kfree(name);
+		kfree(led);
+		return;
+	}
+
+	list_add_tail(&led->list, &device->leds);
+}
+
+static void hidinput_cleanup_leds(struct hid_device *device)
+{
+	struct hid_led *led, *tmp;
+
+	list_for_each_entry_safe(led, tmp, &device->leds, list) {
+		led_classdev_unregister(&led->cdev);
+		kfree(led->name);
+		kfree(led);
+	}
+}
+
+#else  /* !CONFIG_HID_LEDS */
+
+static void hidinput_setup_led(struct hid_device *device,
+		struct hid_field *field, unsigned int offset,
+		const char *name, const char *trigger)
+{
+}
+
+static void hidinput_cleanup_leds(struct hid_device *device)
+{
+}
+
+#endif  /* CONFIG_HID_LEDS */
+
 static bool hidinput_field_in_collection(struct hid_device *device, struct hid_field *field,
 					 unsigned int type, unsigned int usage)
 {
@@ -935,6 +1021,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		case 0x19:  map_led (LED_MAIL);     break;    /*   "Message Waiting"          */
 		case 0x4d:  map_led (LED_CHARGING); break;    /*   "External Power Connected" */
 
+		case 0x21:  /* "Microphone" */
+			setup_led("micmute", "audio-micmute");
+			break;
+
 		default: goto ignore;
 		}
 		break;
@@ -2282,6 +2372,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 	int i, k;
 
 	INIT_LIST_HEAD(&hid->inputs);
+	INIT_LIST_HEAD(&hid->leds);
 	INIT_WORK(&hid->led_work, hidinput_led_worker);
 
 	hid->status &= ~HID_STAT_DUP_DETECTED;
@@ -2380,6 +2471,7 @@ void hidinput_disconnect(struct hid_device *hid)
 {
 	struct hid_input *hidinput, *next;
 
+	hidinput_cleanup_leds(hid);
 	hidinput_cleanup_battery(hid);
 
 	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index bf43f3ff6664..d7cea5476979 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -617,6 +617,7 @@ struct hid_device {							/* device report descriptor */
 	unsigned country;						/* HID country */
 	struct hid_report_enum report_enum[HID_REPORT_TYPES];
 	struct work_struct led_work;					/* delayed LED worker */
+	struct list_head leds;						/* List of associated LEDs */
 
 	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
-- 
2.43.0