[PATCH v6 00/4] HID: new driver for PS5 'DualSense' controller

[PATCH v6 00/4] HID: new driver for PS5 'DualSense' controller

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Hi,

Recently Sony released a HID driver for the new PlayStation 5 controller
'DualSense'. Pavel Machek noticed the driver got staged for "-next" and asked
me to share the LED patches to linux-leds as well.

The LED patches I'm sharing are patch 6, 9, 11, 12 from the v6 hid-playstation
series as originally posted to linux-input. The driver in its full form can be
found on "hid.git/log/?h=for-5.12/playstation".

A little bit of background on the DualSense controller. It is a generic HID
gamepad (e.g. buttons and sticks) with a number of additional features, such
as Motion Sensors, audio, LEDs and others.  All features are implemented using HID
input and output reports.

In regards to LEDs it has 3 types of LEDs. First, it has a RGB lightbar
which is used in PS4/PS5 games for various effects e.g. health indicator, police
siren, player color and other use cases. This patch series exposes it
using the multicolor class, which is great for our use case as the lightbar
state is sent using a single HID output report. Previously for our DualShock 4
controller, the driver exposed individual LEDs, which wasn't great due to race
conditions as games change LED state frequently.

The second LED type, is a row of 5 player indicator LEDs. Such LEDs are common
on gamepads to set a player number e.g. 1, 2, 4 etcetera. Each LED is exposed
as a single LED. A default player id is set based on the number of connected
controllers, which is common behavior within the input system.

Finally, the DualSense has a audio mute LED and a mute button. The mute button is
expected to mute the internal microphone of the DualSense. The mute behavior
is handled driver side and the driver then also programs the LED. From user space
perspective the LED is read-only.

Let me know if there are any questions or comments.

Thanks,

Roderick Colenbrander
Sony Interactive Entertainment, LLC

Roderick Colenbrander (13):
  HID: playstation: add DualSense lightbar support
  HID: playstation: add microphone mute support for DualSense.
  HID: playstation: add DualSense player LEDs support.
  HID: playstation: DualSense set LEDs to default player id.

 MAINTAINERS                   |    6 +
 drivers/hid/Kconfig           |   21 +
 drivers/hid/Makefile          |    1 +
 drivers/hid/hid-ids.h         |    1 +
 drivers/hid/hid-playstation.c | 1504 +++++++++++++++++++++++++++++++++
 5 files changed, 1533 insertions(+)
 create mode 100644 drivers/hid/hid-playstation.c

-- 
2.26.2

[PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Expose the DualSense its RGB lightbar using the new multicolor LED
framework.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |   1 +
 drivers/hid/hid-playstation.c | 118 ++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 54b4eee222f9..cfa29dc17064 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -857,6 +857,7 @@ config HID_PLAYSTATION
 	tristate "PlayStation HID Driver"
 	depends on HID
 	select CRC32
+	select LEDS_CLASS_MULTICOLOR
 	select POWER_SUPPLY
 	help
 	  Provides support for Sony PS5 controllers including support for
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 64193fdeaa0d..97c1118ba78f 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input/mt.h>
+#include <linux/led-class-multicolor.h>
 #include <linux/module.h>
 
 #include <asm/unaligned.h>
@@ -99,6 +100,10 @@ struct ps_calibration_data {
 /* Flags for DualSense output report. */
 #define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0)
 #define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1)
+#define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
+#define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
+#define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
+#define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
 
 /* DualSense hardware limits */
 #define DS_ACC_RES_PER_G	8192
@@ -128,6 +133,13 @@ struct dualsense {
 	uint8_t motor_left;
 	uint8_t motor_right;
 
+	/* RGB lightbar */
+	struct led_classdev_mc lightbar;
+	bool update_lightbar;
+	uint8_t lightbar_red;
+	uint8_t lightbar_green;
+	uint8_t lightbar_blue;
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -473,6 +485,45 @@ static int ps_get_report(struct hid_device *hdev, uint8_t report_id, uint8_t *bu
 	return 0;
 }
 
+/* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
+static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev,
+	int (*brightness_set)(struct led_classdev *, enum led_brightness))
+{
+	struct hid_device *hdev = ps_dev->hdev;
+	struct mc_subled *mc_led_info;
+	struct led_classdev *led_cdev;
+	int ret;
+
+	mc_led_info = devm_kmalloc_array(&hdev->dev, 3, sizeof(*mc_led_info),
+					 GFP_KERNEL | __GFP_ZERO);
+	if (!mc_led_info)
+		return -ENOMEM;
+
+	mc_led_info[0].color_index = LED_COLOR_ID_RED;
+	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+
+	lightbar_mc_dev->subled_info = mc_led_info;
+	lightbar_mc_dev->num_colors = 3;
+
+	led_cdev = &lightbar_mc_dev->led_cdev;
+	led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
+			ps_dev->mac_address);
+	if (!led_cdev->name)
+		return -ENOMEM;
+	led_cdev->brightness = 255;
+	led_cdev->max_brightness = 255;
+	led_cdev->brightness_set_blocking = brightness_set;
+
+	ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
+	if (ret < 0) {
+		hid_err(hdev, "Cannot register multicolor LED device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
 		int gyro_range, int gyro_res)
 {
@@ -651,6 +702,26 @@ static int dualsense_get_mac_address(struct dualsense *ds)
 	return ret;
 }
 
+static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
+	enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct dualsense *ds = container_of(mc_cdev, struct dualsense, lightbar);
+	unsigned long flags;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+	ds->update_lightbar = true;
+	ds->lightbar_red = mc_cdev->subled_info[0].brightness;
+	ds->lightbar_green = mc_cdev->subled_info[1].brightness;
+	ds->lightbar_blue = mc_cdev->subled_info[2].brightness;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	schedule_work(&ds->output_worker);
+	return 0;
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -734,6 +805,15 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_rumble = false;
 	}
 
+	if (ds->update_lightbar) {
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE;
+		common->lightbar_red = ds->lightbar_red;
+		common->lightbar_green = ds->lightbar_green;
+		common->lightbar_blue = ds->lightbar_blue;
+
+		ds->update_lightbar = false;
+	}
+
 	spin_unlock_irqrestore(&ds->base.lock, flags);
 
 	dualsense_send_output_report(ds, &report);
@@ -918,6 +998,31 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
 	return 0;
 }
 
+static int dualsense_reset_leds(struct dualsense *ds)
+{
+	struct dualsense_output_report report;
+	uint8_t *buf;
+
+	buf = kzalloc(sizeof(struct dualsense_output_report_bt), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	dualsense_init_output_report(ds, &report, buf);
+	/*
+	 * On Bluetooth the DualSense outputs an animation on the lightbar
+	 * during startup and maintains a color afterwards. We need to explicitly
+	 * reconfigure the lightbar before we can do any programming later on.
+	 * In USB the lightbar is not on by default, but redoing the setup there
+	 * doesn't hurt.
+	 */
+	report.common->valid_flag2 = DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE;
+	report.common->lightbar_setup = DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT; /* Fade light out. */
+	dualsense_send_output_report(ds, &report);
+
+	kfree(buf);
+	return 0;
+}
+
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
@@ -989,6 +1094,19 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret)
 		goto err;
 
+	/*
+	 * The hardware may have control over the LEDs (e.g. in Bluetooth on startup).
+	 * Reset the LEDs (lightbar, mute, player leds), so we can control them
+	 * from software.
+	 */
+	ret = dualsense_reset_leds(ds);
+	if (ret)
+		goto err;
+
+	ret = ps_lightbar_register(ps_dev, &ds->lightbar, dualsense_lightbar_set_brightness);
+	if (ret)
+		goto err;
+
 	return &ds->base;
 
 err:
-- 
2.26.2

[PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.

This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |  2 +
 drivers/hid/hid-playstation.c | 99 +++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index cfa29dc17064..446a4d579908 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -857,6 +857,8 @@ config HID_PLAYSTATION
 	tristate "PlayStation HID Driver"
 	depends on HID
 	select CRC32
+	select NEW_LEDS
+	select LEDS_CLASS
 	select LEDS_CLASS_MULTICOLOR
 	select POWER_SUPPLY
 	help
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 97c1118ba78f..c436ac8f7a6f 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input/mt.h>
+#include <linux/leds.h>
 #include <linux/led-class-multicolor.h>
 #include <linux/module.h>
 
@@ -47,6 +48,12 @@ struct ps_calibration_data {
 	int sens_denom;
 };
 
+struct ps_led_info {
+	const char *name;
+	enum led_brightness (*brightness_get)(struct led_classdev *cdev);
+	void (*brightness_set)(struct led_classdev *cdev, enum led_brightness);
+};
+
 /* Seed values for DualShock4 / DualSense CRC32 for different report types. */
 #define PS_INPUT_CRC32_SEED	0xA1
 #define PS_OUTPUT_CRC32_SEED	0xA2
@@ -82,6 +89,7 @@ struct ps_calibration_data {
 #define DS_BUTTONS1_R3		BIT(7)
 #define DS_BUTTONS2_PS_HOME	BIT(0)
 #define DS_BUTTONS2_TOUCHPAD	BIT(1)
+#define DS_BUTTONS2_MIC_MUTE	BIT(2)
 
 /* Status field of DualSense input report. */
 #define DS_STATUS_BATTERY_CAPACITY	GENMASK(3, 0)
@@ -100,9 +108,12 @@ struct ps_calibration_data {
 /* Flags for DualSense output report. */
 #define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0)
 #define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1)
+#define DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE BIT(0)
+#define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1)
 #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
 #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
 #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
+#define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4)
 #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
 
 /* DualSense hardware limits */
@@ -140,6 +151,12 @@ struct dualsense {
 	uint8_t lightbar_green;
 	uint8_t lightbar_blue;
 
+	/* Microphone */
+	bool update_mic_mute;
+	bool mic_muted;
+	bool last_btn_mic_state;
+	struct led_classdev mute_led;
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -485,6 +502,32 @@ static int ps_get_report(struct hid_device *hdev, uint8_t report_id, uint8_t *bu
 	return 0;
 }
 
+static int ps_led_register(struct ps_device *ps_dev, struct led_classdev *led,
+		const struct ps_led_info *led_info)
+{
+	int ret;
+
+	led->name = devm_kasprintf(&ps_dev->hdev->dev, GFP_KERNEL,
+			"playstation::%pMR::%s", ps_dev->mac_address, led_info->name);
+
+	if (!led->name)
+		return -ENOMEM;
+
+	led->brightness = 0;
+	led->max_brightness = 1;
+	led->flags = LED_CORE_SUSPENDRESUME;
+	led->brightness_get = led_info->brightness_get;
+	led->brightness_set = led_info->brightness_set;
+
+	ret = devm_led_classdev_register(&ps_dev->hdev->dev, led);
+	if (ret) {
+		hid_err(ps_dev->hdev, "Failed to register LED %s: %d\n", led_info->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 /* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
 static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev,
 	int (*brightness_set)(struct led_classdev *, enum led_brightness))
@@ -722,6 +765,19 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
 	return 0;
 }
 
+static enum led_brightness dualsense_mute_led_get_brightness(struct led_classdev *led)
+{
+	struct dualsense *ds = container_of(led, struct dualsense, mute_led);
+
+	return ds->mic_muted;
+}
+
+/* The mute LED is treated as read-only. This set call prevents ENOTSUP errors e.g. on unload. */
+static void dualsense_mute_led_set_brightness(struct led_classdev *led, enum led_brightness value)
+{
+
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -814,6 +870,23 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_lightbar = false;
 	}
 
+	if (ds->update_mic_mute) {
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
+		common->mute_button_led = ds->mic_muted;
+
+		if (ds->mic_muted) {
+			/* Disable microphone */
+			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE;
+			common->power_save_control |= DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE;
+		} else {
+			/* Enable microphone */
+			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE;
+			common->power_save_control &= ~DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE;
+		}
+
+		ds->update_mic_mute = false;
+	}
+
 	spin_unlock_irqrestore(&ds->base.lock, flags);
 
 	dualsense_send_output_report(ds, &report);
@@ -828,6 +901,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	uint8_t battery_data, battery_capacity, charging_status, value;
 	int battery_status;
 	uint32_t sensor_timestamp;
+	bool btn_mic_state;
 	unsigned long flags;
 	int i;
 
@@ -883,6 +957,23 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds->gamepad);
 
+	/*
+	 * The DualSense has an internal microphone, which can be muted through a mute button
+	 * on the device. The driver is expected to read the button state and program the device
+	 * to mute/unmute audio at the hardware level.
+	 */
+	btn_mic_state = !!(ds_report->buttons[2] & DS_BUTTONS2_MIC_MUTE);
+	if (btn_mic_state && !ds->last_btn_mic_state) {
+		spin_lock_irqsave(&ps_dev->lock, flags);
+		ds->update_mic_mute = true;
+		ds->mic_muted = !ds->mic_muted; /* toggle */
+		spin_unlock_irqrestore(&ps_dev->lock, flags);
+
+		/* Schedule updating of microphone state at hardware level. */
+		schedule_work(&ds->output_worker);
+	}
+	ds->last_btn_mic_state = btn_mic_state;
+
 	/* Parse and calibrate gyroscope data. */
 	for (i = 0; i < ARRAY_SIZE(ds_report->gyro); i++) {
 		int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
@@ -1030,6 +1121,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	uint8_t max_output_report_size;
 	int ret;
 
+	static const struct ps_led_info mute_led_info = {
+		"micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness
+	};
+
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
 	if (!ds)
 		return ERR_PTR(-ENOMEM);
@@ -1107,6 +1202,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret)
 		goto err;
 
+	ret = ps_led_register(ps_dev, &ds->mute_led, &mute_led_info);
+	if (ret)
+		goto err;
+
 	return &ds->base;
 
 err:
-- 
2.26.2

[PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The DualSense features 5 player LEDs below its touchpad, which are
meant as player id indications. This patch exposes the player LEDs
as individual LEDs.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 60 ++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index c436ac8f7a6f..2d96785c397d 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -112,6 +112,7 @@ struct ps_led_info {
 #define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1)
 #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
 #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
+#define DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE BIT(4)
 #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
 #define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4)
 #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
@@ -157,6 +158,11 @@ struct dualsense {
 	bool last_btn_mic_state;
 	struct led_classdev mute_led;
 
+	/* Player leds */
+	bool update_player_leds;
+	uint8_t player_leds_state;
+	struct led_classdev player_leds[5];
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -778,6 +784,35 @@ static void dualsense_mute_led_set_brightness(struct led_classdev *led, enum led
 
 }
 
+static enum led_brightness dualsense_player_led_get_brightness(struct led_classdev *led)
+{
+	struct hid_device *hdev = to_hid_device(led->dev->parent);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+
+	return !!(ds->player_leds_state & BIT(led - ds->player_leds));
+}
+
+static void dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
+{
+	struct hid_device *hdev = to_hid_device(led->dev->parent);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+	unsigned long flags;
+	unsigned int led_index;
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+
+	led_index = led - ds->player_leds;
+	if (value == LED_OFF)
+		ds->player_leds_state &= ~BIT(led_index);
+	else
+		ds->player_leds_state |= BIT(led_index);
+
+	ds->update_player_leds = true;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	schedule_work(&ds->output_worker);
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -870,6 +905,13 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_lightbar = false;
 	}
 
+	if (ds->update_player_leds) {
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
+		common->player_leds = ds->player_leds_state;
+
+		ds->update_player_leds = false;
+	}
+
 	if (ds->update_mic_mute) {
 		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
 		common->mute_button_led = ds->mic_muted;
@@ -1119,12 +1161,20 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	struct dualsense *ds;
 	struct ps_device *ps_dev;
 	uint8_t max_output_report_size;
-	int ret;
+	int i, ret;
 
 	static const struct ps_led_info mute_led_info = {
 		"micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness
 	};
 
+	static const struct ps_led_info player_leds_info[] = {
+		{ "led1", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led2", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led3", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led4", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led5", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness }
+	};
+
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
 	if (!ds)
 		return ERR_PTR(-ENOMEM);
@@ -1206,6 +1256,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret)
 		goto err;
 
+	for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) {
+		const struct ps_led_info *led_info = &player_leds_info[i];
+
+		ret = ps_led_register(ps_dev, &ds->player_leds[i], led_info);
+		if (ret < 0)
+			goto err;
+	}
+
 	return &ds->base;
 
 err:
-- 
2.26.2

[PATCH v6 4/4] HID: playstation: DualSense set LEDs to default player id.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Add a ID allocator to assign player ids to ps_device instances.
Utilize the player id to set a default color on the DualSense its
player LED strip.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 70 ++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 2d96785c397d..973c1fe61e8a 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -9,6 +9,7 @@
 #include <linux/crc32.h>
 #include <linux/device.h>
 #include <linux/hid.h>
+#include <linux/idr.h>
 #include <linux/input/mt.h>
 #include <linux/leds.h>
 #include <linux/led-class-multicolor.h>
@@ -22,6 +23,8 @@
 static DEFINE_MUTEX(ps_devices_lock);
 static LIST_HEAD(ps_devices_list);
 
+static DEFINE_IDA(ps_player_id_allocator);
+
 #define HID_PLAYSTATION_VERSION_PATCH 0x8000
 
 /* Base class for playstation devices. */
@@ -30,6 +33,8 @@ struct ps_device {
 	struct hid_device *hdev;
 	spinlock_t lock;
 
+	uint32_t player_id;
+
 	struct power_supply_desc battery_desc;
 	struct power_supply *battery;
 	uint8_t battery_capacity;
@@ -321,6 +326,24 @@ static int ps_devices_list_remove(struct ps_device *dev)
 	return 0;
 }
 
+static int ps_device_set_player_id(struct ps_device *dev)
+{
+	int ret = ida_alloc(&ps_player_id_allocator, GFP_KERNEL);
+
+	if (ret < 0)
+		return ret;
+
+	dev->player_id = ret;
+	return 0;
+}
+
+static void ps_device_release_player_id(struct ps_device *dev)
+{
+	ida_free(&ps_player_id_allocator, dev->player_id);
+
+	dev->player_id = U32_MAX;
+}
+
 static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
 {
 	struct input_dev *input_dev;
@@ -1156,6 +1179,29 @@ static int dualsense_reset_leds(struct dualsense *ds)
 	return 0;
 }
 
+static void dualsense_set_player_leds(struct dualsense *ds)
+{
+	/*
+	 * The DualSense controller has a row of 5 LEDs used for player ids.
+	 * Behavior on the PlayStation 5 console is to center the player id
+	 * across the LEDs, so e.g. player 1 would be "--x--" with x being 'on'.
+	 * Follow a similar mapping here.
+	 */
+	static const int player_ids[5] = {
+		BIT(2),
+		BIT(3) | BIT(1),
+		BIT(4) | BIT(2) | BIT(0),
+		BIT(4) | BIT(3) | BIT(1) | BIT(0),
+		BIT(4) | BIT(3) | BIT(2) | BIT(1) | BIT(0)
+	};
+
+	uint8_t player_id = ds->base.player_id % ARRAY_SIZE(player_ids);
+
+	ds->update_player_leds = true;
+	ds->player_leds_state = player_ids[player_id];
+	schedule_work(&ds->output_worker);
+}
+
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
@@ -1264,6 +1310,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 			goto err;
 	}
 
+	ret = ps_device_set_player_id(ps_dev);
+	if (ret) {
+		hid_err(hdev, "Failed to assign player id for DualSense: %d\n", ret);
+		goto err;
+	}
+
+	/* Set player LEDs to our player id. */
+	dualsense_set_player_leds(ds);
+
 	return &ds->base;
 
 err:
@@ -1328,6 +1383,7 @@ static void ps_remove(struct hid_device *hdev)
 	struct ps_device *dev = hid_get_drvdata(hdev);
 
 	ps_devices_list_remove(dev);
+	ps_device_release_player_id(dev);
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
@@ -1348,7 +1404,19 @@ static struct hid_driver ps_driver = {
 	.raw_event	= ps_raw_event,
 };
 
-module_hid_driver(ps_driver);
+static int __init ps_init(void)
+{
+	return hid_register_driver(&ps_driver);
+}
+
+static void __exit ps_exit(void)
+{
+	hid_unregister_driver(&ps_driver);
+	ida_destroy(&ps_player_id_allocator);
+}
+
+module_init(ps_init);
+module_exit(ps_exit);
 
 MODULE_AUTHOR("Sony Interactive Entertainment");
 MODULE_DESCRIPTION("HID Driver for PlayStation peripherals.");
-- 
2.26.2

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Marek Behun]

On Sun, 14 Feb 2021 16:45:46 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> +	led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> +			ps_dev->mac_address);
...
> +	ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);

The LED subsystem has a predefined schema by which LED names should
look like:
  devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
 character should be used only as separator of these fields, so not MAC
 addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst

The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...

In your case the name should be something like:
  input3:rgb:indicator

Different existing functions are defined in
include/dt-bindings/leds/common.h.

BTW there are extended versions of LED registering functions, suffixed
by "_ext". These accept a struct led_init_data. If a fwnode of the LED
is passed to the registering function via this struct, the LED core
will compose a name for the LED itself. But since your LEDs don't have
device-tree nodes because they are on USB/BlueTooth joysticks, you
either have to compose the name itself like your code is doing now, or
you can propose a patch to the LED core, so that LED core will be able
to compose the LED name even without a device-tree node.

JFI, the function part is (in the future) supposed to somehow define LED
trigger which the system will assign to the LED on probe, but this is
not implemented yet. Currently when the LED has a devicetree node,
the trigger is assigned from the `linux,default-trigger` property, but
the idea is to infer it from the `function` property.

What is this RGB LED supposed to do on the joystick? Is it just for
nice colors? Or should it blink somehow? Can the hardware in the
joystick blink the LED itself? Or maybe fade between colors?

There is for example the pattern LED trigger which changes the LED
brightness by a defined pattern. I am planning to add multicolor
support to this trigger, because our RGB LED controller can offload
such thing to hardware.

Marek

Re: [PATCH v6 00/4] HID: new driver for PS5 'DualSense' controller

[Marek Behun]

On Sun, 14 Feb 2021 16:45:45 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Hi,
> 
> Recently Sony released a HID driver for the new PlayStation 5 controller
> 'DualSense'. Pavel Machek noticed the driver got staged for "-next" and asked
> me to share the LED patches to linux-leds as well.
> 
> The LED patches I'm sharing are patch 6, 9, 11, 12 from the v6 hid-playstation
> series as originally posted to linux-input. The driver in its full form can be
> found on "hid.git/log/?h=for-5.12/playstation".
> 

Hi,

OK I see you described the purpose of these LEDs here, please ignore
that one question in my reply to patch 1/4.

> Finally, the DualSense has a audio mute LED and a mute button. The mute button is
> expected to mute the internal microphone of the DualSense. The mute behavior
> is handled driver side and the driver then also programs the LED. From user space
> perspective the LED is read-only.

The audio mute LED should not be read-only from userspace. Instead a
LED trigger should be assigned by default, audio-micmute / audio-mute.

With this trigger the LED subsystem will handle setting brightness of
the LED according to whether the audio is muted or not.

This trigger is currently simple, though. It is system wide - it
is impossible to configure it to report only on the state of a
specific microphone. But the trigger driver can be extended if this is needed.

Marek

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Marek Behun]

On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> The DualSense controller has a built-in microphone exposed as an
> audio device over USB (or HID using Bluetooth). A dedicated
> button on the controller handles mute, but software has to configure
> the device to mute the audio stream.
> 
> This patch captures the mute button and schedules an output report
> to mute/unmute the audio stream as well as toggle the mute LED.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

Is the microphone supported via Linux? I.e. is there an audio driver
for it?

If it is, look at the audio-micmute LED trigger.

If you can't use the audio-micmute trigger because the microphone isn't
supported via Linux, I still think the LED should the LED should be
read-write. You can then register a LED private trigger. The driver should
change the state of the LED according to the microphone mute state only
if these trigger is enabled.

Marek

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Roderick Colenbrander]

Hi Marek,

On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Sun, 14 Feb 2021 16:45:46 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > +                     ps_dev->mac_address);
> ...
> > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
>
> The LED subsystem has a predefined schema by which LED names should
> look like:
>   devicename:color:function
> (Not all fields are required, but the order must be preserved. The ':'
>  character should be used only as separator of these fields, so not MAC
>  addresses in these names, it will confuse userspace parsers.)
> See Documentation/leds/leds-class.rst
>
> The devicename part should not be "playstation". It should be something
> otherwise recognizable from userspace. For example an mmc indicator has
> devicename "mmc0", keyboard capslock LED can have devicename "input0"...
>
> In your case the name should be something like:
>   input3:rgb:indicator

Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B

This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.

I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.

> Different existing functions are defined in
> include/dt-bindings/leds/common.h.
>
> BTW there are extended versions of LED registering functions, suffixed
> by "_ext". These accept a struct led_init_data. If a fwnode of the LED
> is passed to the registering function via this struct, the LED core
> will compose a name for the LED itself. But since your LEDs don't have
> device-tree nodes because they are on USB/BlueTooth joysticks, you
> either have to compose the name itself like your code is doing now, or
> you can propose a patch to the LED core, so that LED core will be able
> to compose the LED name even without a device-tree node.
>
> JFI, the function part is (in the future) supposed to somehow define LED
> trigger which the system will assign to the LED on probe, but this is
> not implemented yet. Currently when the LED has a devicetree node,
> the trigger is assigned from the `linux,default-trigger` property, but
> the idea is to infer it from the `function` property.
>

Thanks for the info. Might be handy in the future.

> What is this RGB LED supposed to do on the joystick? Is it just for
> nice colors? Or should it blink somehow? Can the hardware in the
> joystick blink the LED itself? Or maybe fade between colors?

I mentioned a bit in the other email. These LEDs are under direct
control from PlayStation games. Some may change the color on a per
video frame basis. Use cases include health (green) and when a
character loses health becomes more red/orange and can start flashing.
I have seen games use this for police car and then mixing between blue
and red. Others use it with a static player ID color. The console side
API is literally raw RGB values. There is no hardware blink support.
The previous controllers had it though.

> There is for example the pattern LED trigger which changes the LED
> brightness by a defined pattern. I am planning to add multicolor
> support to this trigger, because our RGB LED controller can offload
> such thing to hardware.
>
> Marek

Thanks,
Roderick

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Marek Behun]

On Mon, 15 Feb 2021 07:36:58 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> Hi Marek,
> 
> On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Sun, 14 Feb 2021 16:45:46 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > +                     ps_dev->mac_address);  
> > ...  
> > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);  
> >
> > The LED subsystem has a predefined schema by which LED names should
> > look like:
> >   devicename:color:function
> > (Not all fields are required, but the order must be preserved. The ':'
> >  character should be used only as separator of these fields, so not MAC
> >  addresses in these names, it will confuse userspace parsers.)
> > See Documentation/leds/leds-class.rst
> >
> > The devicename part should not be "playstation". It should be something
> > otherwise recognizable from userspace. For example an mmc indicator has
> > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> >
> > In your case the name should be something like:
> >   input3:rgb:indicator  
> 
> Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> are added to the 'parent' HID device, not the input devices. In case
> of DualSense it is actually implemented as a composite device with
> mulitple input devices (gamepad, touchpad and motion sensors) per HID
> device. The device name of HID devices seems to be something like:
> <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> 
> This is I guess why many HID devices in general pick their own names
> (and not all have need to have input devices I guess). Though Benjamin
> and Jiri know better.
> 
> I'm not sure what naming could make sense here. The previous Sony
> driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> on DualShock 4.

We have to find a reasonable devicename here. If each joystick registers
multiple input devices, it cannot be "input%d". I suppose there isn't
an API for grouping mulitple input devices toghether into inputgroups.
Maybe it could be in the format "joystick%d".

But I don't think it can be "playstation". Nor can MAC addresses be
there if they contain ':'s.

Marek

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Roderick Colenbrander]

On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 07:36:58 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > Hi Marek,
> >
> > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Sun, 14 Feb 2021 16:45:46 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > > +                     ps_dev->mac_address);
> > > ...
> > > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
> > >
> > > The LED subsystem has a predefined schema by which LED names should
> > > look like:
> > >   devicename:color:function
> > > (Not all fields are required, but the order must be preserved. The ':'
> > >  character should be used only as separator of these fields, so not MAC
> > >  addresses in these names, it will confuse userspace parsers.)
> > > See Documentation/leds/leds-class.rst
> > >
> > > The devicename part should not be "playstation". It should be something
> > > otherwise recognizable from userspace. For example an mmc indicator has
> > > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> > >
> > > In your case the name should be something like:
> > >   input3:rgb:indicator
> >
> > Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> > are added to the 'parent' HID device, not the input devices. In case
> > of DualSense it is actually implemented as a composite device with
> > mulitple input devices (gamepad, touchpad and motion sensors) per HID
> > device. The device name of HID devices seems to be something like:
> > <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> > 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> >
> > This is I guess why many HID devices in general pick their own names
> > (and not all have need to have input devices I guess). Though Benjamin
> > and Jiri know better.
> >
> > I'm not sure what naming could make sense here. The previous Sony
> > driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> > on DualShock 4.
>
> We have to find a reasonable devicename here. If each joystick registers
> multiple input devices, it cannot be "input%d". I suppose there isn't
> an API for grouping mulitple input devices toghether into inputgroups.
> Maybe it could be in the format "joystick%d".

Yeah, there is no inputgroups mechanism.  It could use some type of
joystick name if that's what desired. However, there is no common ID
code. Individual drivers are sometimes calculating their own IDs
(hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
for hid-sony/hid-playstation the use case for tracking IDs is for a
part to prevent duplicate devices as you can connect your device using
both bluetooth and USB. So would be "ps-joystick0"

At the HID layer there does seem to be a unique ID, but it is only
exposed in the name string: This is how the name is constructed:
     dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
             hdev->vendor, hdev->product, atomic_inc_return(&id));

This ID is HID specific, but not all input devices use HID.

I'm not entirely sure what makes sense...


> But I don't think it can be "playstation". Nor can MAC addresses be
> there if they contain ':'s.
>
> Marek

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Roderick Colenbrander]

On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Sun, 14 Feb 2021 16:45:47 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > The DualSense controller has a built-in microphone exposed as an
> > audio device over USB (or HID using Bluetooth). A dedicated
> > button on the controller handles mute, but software has to configure
> > the device to mute the audio stream.
> >
> > This patch captures the mute button and schedules an output report
> > to mute/unmute the audio stream as well as toggle the mute LED.
> >
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Is the microphone supported via Linux? I.e. is there an audio driver
> for it?

Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.

> If it is, look at the audio-micmute LED trigger.
>

I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?

In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.

> If you can't use the audio-micmute trigger because the microphone isn't
> supported via Linux, I still think the LED should the LED should be
> read-write. You can then register a LED private trigger. The driver should
> change the state of the LED according to the microphone mute state only
> if these trigger is enabled.
>
> Marek

Roderick

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Marek Behun]

On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Sun, 14 Feb 2021 16:45:47 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > >
> > > The DualSense controller has a built-in microphone exposed as an
> > > audio device over USB (or HID using Bluetooth). A dedicated
> > > button on the controller handles mute, but software has to configure
> > > the device to mute the audio stream.
> > >
> > > This patch captures the mute button and schedules an output report
> > > to mute/unmute the audio stream as well as toggle the mute LED.
> > >
> > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>  
> >
> > Is the microphone supported via Linux? I.e. is there an audio driver
> > for it?  
> 
> Yes and no. The microphone is supported using USB, not yet using
> Bluetooth (uses a custom protocol). Actually there are various other
> audio features in the DualSense (headphone jack, speaker, volume
> controls,..) and they all work using custom protocols. We were
> planning to defer this work through future patches as the features are
> very complicated and need a deep analysis on how to realize them. For
> example audio controls work through HID, but for USB the audio driver
> is a generic hda audio device I think. Bluetooth is a custom protocol
> and will be yet a different audio driver somewhere.
> 
> > If it is, look at the audio-micmute LED trigger.
> >  
> 
> I'm not sure if the expected behavior for the DualSense is similar to
> the standard audio mute use cases. My understanding of these triggers
> (please correct me if I'm wrong) is for e.g. an audio driver or user
> space to send a signal to anything registering for a particular
> trigger. In this case a global micmute. Is that, right?
> 
> In our case for PlayStation games, there are often multiple
> controllers connected and each user has their own microphone in their
> controller. All can function at the same time (different from a
> standard PC use case). That's why I'm wondering if this makes sense.I
> know we are on Linux, but for Sony we want to properly support such
> use cases.

If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.

When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs. Before registering
the LED, assign default_trigger member so that this trigger is enabled
during registration.

This is why we have support for private LED triggers.

Marek

> > If you can't use the audio-micmute trigger because the microphone isn't
> > supported via Linux, I still think the LED should the LED should be
> > read-write. You can then register a LED private trigger. The driver should
> > change the state of the LED according to the microphone mute state only
> > if these trigger is enabled.
> >
> > Marek  
> 
> Roderick

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Marek Behun]

On Mon, 15 Feb 2021 09:51:15 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Mon, 15 Feb 2021 07:36:58 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > Hi Marek,
> > >
> > > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > >
> > > > On Sun, 14 Feb 2021 16:45:46 -0800
> > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > >  
> > > > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > > > +                     ps_dev->mac_address);  
> > > > ...  
> > > > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);  
> > > >
> > > > The LED subsystem has a predefined schema by which LED names should
> > > > look like:
> > > >   devicename:color:function
> > > > (Not all fields are required, but the order must be preserved. The ':'
> > > >  character should be used only as separator of these fields, so not MAC
> > > >  addresses in these names, it will confuse userspace parsers.)
> > > > See Documentation/leds/leds-class.rst
> > > >
> > > > The devicename part should not be "playstation". It should be something
> > > > otherwise recognizable from userspace. For example an mmc indicator has
> > > > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> > > >
> > > > In your case the name should be something like:
> > > >   input3:rgb:indicator  
> > >
> > > Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> > > are added to the 'parent' HID device, not the input devices. In case
> > > of DualSense it is actually implemented as a composite device with
> > > mulitple input devices (gamepad, touchpad and motion sensors) per HID
> > > device. The device name of HID devices seems to be something like:
> > > <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> > > 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> > >
> > > This is I guess why many HID devices in general pick their own names
> > > (and not all have need to have input devices I guess). Though Benjamin
> > > and Jiri know better.
> > >
> > > I'm not sure what naming could make sense here. The previous Sony
> > > driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> > > on DualShock 4.  
> >
> > We have to find a reasonable devicename here. If each joystick registers
> > multiple input devices, it cannot be "input%d". I suppose there isn't
> > an API for grouping mulitple input devices toghether into inputgroups.
> > Maybe it could be in the format "joystick%d".  
> 
> Yeah, there is no inputgroups mechanism.  It could use some type of
> joystick name if that's what desired. However, there is no common ID
> code. Individual drivers are sometimes calculating their own IDs
> (hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
> for hid-sony/hid-playstation the use case for tracking IDs is for a
> part to prevent duplicate devices as you can connect your device using
> both bluetooth and USB. So would be "ps-joystick0"
> 
> At the HID layer there does seem to be a unique ID, but it is only
> exposed in the name string: This is how the name is constructed:
>      dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
>              hdev->vendor, hdev->product, atomic_inc_return(&id));
> 
> This ID is HID specific, but not all input devices use HID.
> 
> I'm not entirely sure what makes sense...

So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.

Send a patch to hid-core to make this atomic_inc_return(&id) also be
stored into struct hid_device as an integer, not only as a part
of the device name string.

Then use "hid%d" as the devicename for this LED, with %d substituted
with this ID.

Marek

Re: [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Roderick Colenbrander]

Hi Marek,

On Sun, Feb 14, 2021 at 4:46 PM Roderick Colenbrander
<roderick@gaikai.com> wrote:
>
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> The DualSense features 5 player LEDs below its touchpad, which are
> meant as player id indications. This patch exposes the player LEDs
> as individual LEDs.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  drivers/hid/hid-playstation.c | 60 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index c436ac8f7a6f..2d96785c397d 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -112,6 +112,7 @@ struct ps_led_info {
>  #define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1)
>  #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
>  #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
> +#define DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE BIT(4)
>  #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
>  #define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4)
>  #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
> @@ -157,6 +158,11 @@ struct dualsense {
>         bool last_btn_mic_state;
>         struct led_classdev mute_led;
>
> +       /* Player leds */
> +       bool update_player_leds;
> +       uint8_t player_leds_state;
> +       struct led_classdev player_leds[5];
> +
>         struct work_struct output_worker;
>         void *output_report_dmabuf;
>         uint8_t output_seq; /* Sequence number for output report. */
> @@ -778,6 +784,35 @@ static void dualsense_mute_led_set_brightness(struct led_classdev *led, enum led
>
>  }
>
> +static enum led_brightness dualsense_player_led_get_brightness(struct led_classdev *led)
> +{
> +       struct hid_device *hdev = to_hid_device(led->dev->parent);
> +       struct dualsense *ds = hid_get_drvdata(hdev);
> +
> +       return !!(ds->player_leds_state & BIT(led - ds->player_leds));
> +}
> +
> +static void dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
> +{
> +       struct hid_device *hdev = to_hid_device(led->dev->parent);
> +       struct dualsense *ds = hid_get_drvdata(hdev);
> +       unsigned long flags;
> +       unsigned int led_index;
> +
> +       spin_lock_irqsave(&ds->base.lock, flags);
> +
> +       led_index = led - ds->player_leds;
> +       if (value == LED_OFF)
> +               ds->player_leds_state &= ~BIT(led_index);
> +       else
> +               ds->player_leds_state |= BIT(led_index);
> +
> +       ds->update_player_leds = true;
> +       spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +       schedule_work(&ds->output_worker);
> +}
> +
>  static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
>                 void *buf)
>  {
> @@ -870,6 +905,13 @@ static void dualsense_output_worker(struct work_struct *work)
>                 ds->update_lightbar = false;
>         }
>
> +       if (ds->update_player_leds) {
> +               common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
> +               common->player_leds = ds->player_leds_state;
> +
> +               ds->update_player_leds = false;
> +       }
> +
>         if (ds->update_mic_mute) {
>                 common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
>                 common->mute_button_led = ds->mic_muted;
> @@ -1119,12 +1161,20 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         struct dualsense *ds;
>         struct ps_device *ps_dev;
>         uint8_t max_output_report_size;
> -       int ret;
> +       int i, ret;
>
>         static const struct ps_led_info mute_led_info = {
>                 "micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness
>         };
>
> +       static const struct ps_led_info player_leds_info[] = {
> +               { "led1", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +               { "led2", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +               { "led3", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +               { "led4", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +               { "led5", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness }
> +       };
> +
>         ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
>         if (!ds)
>                 return ERR_PTR(-ENOMEM);
> @@ -1206,6 +1256,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         if (ret)
>                 goto err;
>
> +       for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) {
> +               const struct ps_led_info *led_info = &player_leds_info[i];
> +
> +               ret = ps_led_register(ps_dev, &ds->player_leds[i], led_info);
> +               if (ret < 0)
> +                       goto err;
> +       }
> +
>         return &ds->base;
>
>  err:
> --
> 2.26.2
>

What is the desired naming for these player LEDs? There is not an
officially designed function based on DT bindings. So far they used
"playstation::mac::ledX". When changing the naming scheme towards
"hid" and removing MAC, they would be: "hid%d::led1" etcetera.

Thanks,
Roderick

Re: [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Marek Behun]

On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> What is the desired naming for these player LEDs? There is not an
> officially designed function based on DT bindings. So far they used
> "playstation::mac::ledX". When changing the naming scheme towards
> "hid" and removing MAC, they would be: "hid%d::led1" etcetera.

Hi,

there is one more thing I forgot to mention in the LED name schema:
  devicename:color:function-functionEnumerator

So LED core can for example compose a names in the format:
  switch0:green:lan-1
  switch0:green:lan-2
  switch0:green:lan-3
  switch0:green:lan-4

In your case I think the most appropriate name would be something like
  hid0:color:indicator-1
  hid0:color:indicator-2
  ...

Are these LEDs of different colors which are impossible to determine?
The string "hid%d::led1" you mention above does not indicate color.

Marek

Re: [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Roderick Colenbrander]

On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 15:00:30 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > What is the desired naming for these player LEDs? There is not an
> > officially designed function based on DT bindings. So far they used
> > "playstation::mac::ledX". When changing the naming scheme towards
> > "hid" and removing MAC, they would be: "hid%d::led1" etcetera.
>
> Hi,
>
> there is one more thing I forgot to mention in the LED name schema:
>   devicename:color:function-functionEnumerator
>
> So LED core can for example compose a names in the format:
>   switch0:green:lan-1
>   switch0:green:lan-2
>   switch0:green:lan-3
>   switch0:green:lan-4
>
> In your case I think the most appropriate name would be something like
>   hid0:color:indicator-1
>   hid0:color:indicator-2
>   ...

I am trying to think if indicator is clear enough. Currently devices
use a mixture of names, which is obviously bad (wiimote uses p1-p4 at
the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses
player1-4). I would at least like new drivers to standardize. In
particular in Android frameworks we have a need to map these LEDs back
to the Java InputDevice. Finding the LEDs has been quite painful so
far.

If this is what is decided, I guess we should update the Linux gamepad
document at some point as well.

> Are these LEDs of different colors which are impossible to determine?
> The string "hid%d::led1" you mention above does not indicate color.

The DualSense LEDs are all white (at least so far?). On controllers
from other brands I have seen them be red or green. So could indeed
use: "hid%d:white".

> Marek

Thanks,
Roderick

Re: [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Marek Behun]

On Mon, 15 Feb 2021 17:11:14 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Mon, 15 Feb 2021 15:00:30 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > What is the desired naming for these player LEDs? There is not an
> > > officially designed function based on DT bindings. So far they used
> > > "playstation::mac::ledX". When changing the naming scheme towards
> > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera.  
> >
> > Hi,
> >
> > there is one more thing I forgot to mention in the LED name schema:
> >   devicename:color:function-functionEnumerator
> >
> > So LED core can for example compose a names in the format:
> >   switch0:green:lan-1
> >   switch0:green:lan-2
> >   switch0:green:lan-3
> >   switch0:green:lan-4
> >
> > In your case I think the most appropriate name would be something like
> >   hid0:color:indicator-1
> >   hid0:color:indicator-2
> >   ...  
> 
> I am trying to think if indicator is clear enough. Currently devices
> use a mixture of names, which is obviously bad (wiimote uses p1-p4 at
> the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses
> player1-4). I would at least like new drivers to standardize. In
> particular in Android frameworks we have a need to map these LEDs back
> to the Java InputDevice. Finding the LEDs has been quite painful so
> far.

Thinking about it more, function "player" should theoretically be
reasonable. Maybe we should try sending a patch for review, adding this
funciton to include/dt-bindings/leds/common.h, and see what others
think of it...

> If this is what is decided, I guess we should update the Linux gamepad
> document at some point as well.
> 
> > Are these LEDs of different colors which are impossible to determine?
> > The string "hid%d::led1" you mention above does not indicate color.  
> 
> The DualSense LEDs are all white (at least so far?). On controllers
> from other brands I have seen them be red or green. So could indeed
> use: "hid%d:white".

Yes, a constant for white color is defined in headers.

> > Marek  
> 
> Thanks,
> Roderick

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Roderick Colenbrander]

On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 10:07:29 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > >
> > > > The DualSense controller has a built-in microphone exposed as an
> > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > button on the controller handles mute, but software has to configure
> > > > the device to mute the audio stream.
> > > >
> > > > This patch captures the mute button and schedules an output report
> > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > >
> > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > >
> > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > for it?
> >
> > Yes and no. The microphone is supported using USB, not yet using
> > Bluetooth (uses a custom protocol). Actually there are various other
> > audio features in the DualSense (headphone jack, speaker, volume
> > controls,..) and they all work using custom protocols. We were
> > planning to defer this work through future patches as the features are
> > very complicated and need a deep analysis on how to realize them. For
> > example audio controls work through HID, but for USB the audio driver
> > is a generic hda audio device I think. Bluetooth is a custom protocol
> > and will be yet a different audio driver somewhere.
> >
> > > If it is, look at the audio-micmute LED trigger.
> > >
> >
> > I'm not sure if the expected behavior for the DualSense is similar to
> > the standard audio mute use cases. My understanding of these triggers
> > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > space to send a signal to anything registering for a particular
> > trigger. In this case a global micmute. Is that, right?
> >
> > In our case for PlayStation games, there are often multiple
> > controllers connected and each user has their own microphone in their
> > controller. All can function at the same time (different from a
> > standard PC use case). That's why I'm wondering if this makes sense.I
> > know we are on Linux, but for Sony we want to properly support such
> > use cases.
>
> If there aren't audio drivers yet for this, simply have this driver
> also register a private LED trigger (with name "joystick-audiomute"
> or something similar), and when registering the LED, set the
> trigger_type member. Look at trigger_type in include/linux/leds.h, and
> in LED Documentation.

Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).

Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?

> When this trigger is enabled for your LED, have your code switch LED
> state like it does now. When there is no trigger enabled, the userspace
> will be able to set brightness of this LED via sysfs.

Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?

If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)

I am tempted to wait with the trigger code as I really don't understand it.

> Before registering
> the LED, assign default_trigger member so that this trigger is enabled
> during registration.
>
> This is why we have support for private LED triggers.
>
> Marek
>

Roderick

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Marek Behun]

On Tue, 16 Feb 2021 00:33:24 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Mon, 15 Feb 2021 10:07:29 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > >
> > > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > >  
> > > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > >
> > > > > The DualSense controller has a built-in microphone exposed as an
> > > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > > button on the controller handles mute, but software has to configure
> > > > > the device to mute the audio stream.
> > > > >
> > > > > This patch captures the mute button and schedules an output report
> > > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > > >
> > > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>  
> > > >
> > > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > > for it?  
> > >
> > > Yes and no. The microphone is supported using USB, not yet using
> > > Bluetooth (uses a custom protocol). Actually there are various other
> > > audio features in the DualSense (headphone jack, speaker, volume
> > > controls,..) and they all work using custom protocols. We were
> > > planning to defer this work through future patches as the features are
> > > very complicated and need a deep analysis on how to realize them. For
> > > example audio controls work through HID, but for USB the audio driver
> > > is a generic hda audio device I think. Bluetooth is a custom protocol
> > > and will be yet a different audio driver somewhere.
> > >  
> > > > If it is, look at the audio-micmute LED trigger.
> > > >  
> > >
> > > I'm not sure if the expected behavior for the DualSense is similar to
> > > the standard audio mute use cases. My understanding of these triggers
> > > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > > space to send a signal to anything registering for a particular
> > > trigger. In this case a global micmute. Is that, right?
> > >
> > > In our case for PlayStation games, there are often multiple
> > > controllers connected and each user has their own microphone in their
> > > controller. All can function at the same time (different from a
> > > standard PC use case). That's why I'm wondering if this makes sense.I
> > > know we are on Linux, but for Sony we want to properly support such
> > > use cases.  
> >
> > If there aren't audio drivers yet for this, simply have this driver
> > also register a private LED trigger (with name "joystick-audiomute"
> > or something similar), and when registering the LED, set the
> > trigger_type member. Look at trigger_type in include/linux/leds.h, and
> > in LED Documentation.  
> 
> Sorry for some more questions. I have been trying to understand
> triggers all night. The concept is just so strange and foreign to me.
> I understand it is in the end just a string and one use case is
> in-kernel IPC and you can configure them from user space as well, but
> I just don't get it. I understand you can use a trigger to in the end
> program your LED in a automatic manner. I just don't understand how
> the concepts fit together and how to implement it (maybe I will update
> the docs later on... they are a bit sparse for if you don't know this
> area).
> 
> Regarding registering a private trigger. I see include/linux/leds.h
> have a comment about trigger_type and how it should be set for private
> triggers on led_classdev. I haven't been able to find any example
> usages of this within the kernel. It doesn't seem to be used in the
> kernel, maybe it is just around for future use? I also seem to need to
> implement my own activate/deactive callbacks for the trigger. These I
> would use to program the LED brightness I guess. Though I see various
> trigger drivers (drivers/leds/triggers), but not all of them have
> activate/deactivate callbacks. Mostly simple drivers, but not sure why
> they don't need them. What else is the point of a trigger?
> 
> > When this trigger is enabled for your LED, have your code switch LED
> > state like it does now. When there is no trigger enabled, the userspace
> > will be able to set brightness of this LED via sysfs.  
> 
> Right now I manage the button mute state directly from the input
> handler (dualsense_parse_report) when the button is pressed and then
> schedule an output report to toggle the LED and program the DualSense
> to mute its audio (the PlayStation works very similar). I would need
> to use led_trigger_event then here?
> 
> If I then understand it right, I need to modify my "brightness_set"
> handler and check if there is a trigger (based on
> led_classdev->activated??). If there is none, then userspace can
> change the LED state. Internally when I change the LED state, I will
> also program the hardware to mute as well. (they are tied together)
> 
> I am tempted to wait with the trigger code as I really don't understand it.

Simple triggers are just normal triggers but with some simplifying code
to avoid code repetition. Ignore them for now.

When a trigger is set to a LED via sysfs, the trigger .activate()
method is called and the led_cdev.trigger is set to point to that
trigger.

It is then up to the code inside the trigger's .activate() method to
initialize mechanisms that will control the LED.

For netdev trigger a delayed_work is scheduled periodically, and in each
execution of that work's callback the netdevice's stats are compared to
the last ones. If the new stats are greater, the trigger code blinks the
LED.

So in your case it is pretty simple to implement, because you already
have the necessary code to manipulate the LED brightness automatically
according to whether button was pressed. You are setting
  ds->update_mic_mute = true;
in dualsense_parse_report() and then manipulate the LED in
dualsense_output_worker().

Just add another boolean member into struct dualsense:
  bool control_mute_led;
and change the code in dualsense_output_worker() to only change the
mute_led brightness is this new member is true.

Add this code to your driver:

  static struct led_hw_trigger_type ps_micmute_trigger_type;

When registering the LED in ps_led_register(), also set
  led->trigger_type = &ps_micmute_trigger_type;

Add this functions:
  static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
  {
    struct dualsense *ds = container_of(...);

    /* make the worker control mute LED according to mute button */
    ds->control_mute_led = true;

    /* make sure the mute LED shows the current mute button state */
    ds->update_mic_mute = true;
    schedule_work(&ds->output_worker);

    return 0;
  }

  static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
  {
    struct dualsense *ds = container_of(...);

    ds->control_mute_led = false;
  }

  static struct led_trigger ps_micmute_trigger = {
    .name = "playstation-micmute",
    .activate = ps_micmute_trig_activate,
    .deactivate = ps_micmute_trig_deactivate,
    .trigger_type = &ps_micmute_trigger_type,
  };

Add this code to ps_init():
  int ret;

  ret = led_trigger_register(&ps_micmute_trigger);
  if (ret)
    return ret;

And to ps_exit():
  led_trigger_unregister(&ps_micmute_trigger);

All this will make sure that the driver will manipulate the mute
LED state only when the playstation-micmute trigger is active on the
LED.

Moreover if you want this driver to be active on the LED by default,
set this prior to registering the LED
  led->default_trigger = "playstation-micmute";

Finally add code to dualsense_mute_led_set_brightness() to make
userspace/[other LED triggers] able to set mute LED brightness.

The purpose of the .trigger_type member of struct led_classdev and
struct led_trigger is that if this member is set for a trigger, this
trigger will only be available for LEDs that have the same trigger_type.

Marek

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Benjamin Tissoires]

On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Tue, 16 Feb 2021 00:33:24 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Mon, 15 Feb 2021 10:07:29 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > > >
> > > > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > >
> > > > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > > >
> > > > > > The DualSense controller has a built-in microphone exposed as an
> > > > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > > > button on the controller handles mute, but software has to configure
> > > > > > the device to mute the audio stream.
> > > > > >
> > > > > > This patch captures the mute button and schedules an output report
> > > > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > > > >
> > > > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > >
> > > > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > > > for it?
> > > >
> > > > Yes and no. The microphone is supported using USB, not yet using
> > > > Bluetooth (uses a custom protocol). Actually there are various other
> > > > audio features in the DualSense (headphone jack, speaker, volume
> > > > controls,..) and they all work using custom protocols. We were
> > > > planning to defer this work through future patches as the features are
> > > > very complicated and need a deep analysis on how to realize them. For
> > > > example audio controls work through HID, but for USB the audio driver
> > > > is a generic hda audio device I think. Bluetooth is a custom protocol
> > > > and will be yet a different audio driver somewhere.
> > > >
> > > > > If it is, look at the audio-micmute LED trigger.
> > > > >
> > > >
> > > > I'm not sure if the expected behavior for the DualSense is similar to
> > > > the standard audio mute use cases. My understanding of these triggers
> > > > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > > > space to send a signal to anything registering for a particular
> > > > trigger. In this case a global micmute. Is that, right?
> > > >
> > > > In our case for PlayStation games, there are often multiple
> > > > controllers connected and each user has their own microphone in their
> > > > controller. All can function at the same time (different from a
> > > > standard PC use case). That's why I'm wondering if this makes sense.I
> > > > know we are on Linux, but for Sony we want to properly support such
> > > > use cases.
> > >
> > > If there aren't audio drivers yet for this, simply have this driver
> > > also register a private LED trigger (with name "joystick-audiomute"
> > > or something similar), and when registering the LED, set the
> > > trigger_type member. Look at trigger_type in include/linux/leds.h, and
> > > in LED Documentation.
> >
> > Sorry for some more questions. I have been trying to understand
> > triggers all night. The concept is just so strange and foreign to me.
> > I understand it is in the end just a string and one use case is
> > in-kernel IPC and you can configure them from user space as well, but
> > I just don't get it. I understand you can use a trigger to in the end
> > program your LED in a automatic manner. I just don't understand how
> > the concepts fit together and how to implement it (maybe I will update
> > the docs later on... they are a bit sparse for if you don't know this
> > area).
> >
> > Regarding registering a private trigger. I see include/linux/leds.h
> > have a comment about trigger_type and how it should be set for private
> > triggers on led_classdev. I haven't been able to find any example
> > usages of this within the kernel. It doesn't seem to be used in the
> > kernel, maybe it is just around for future use? I also seem to need to
> > implement my own activate/deactive callbacks for the trigger. These I
> > would use to program the LED brightness I guess. Though I see various
> > trigger drivers (drivers/leds/triggers), but not all of them have
> > activate/deactivate callbacks. Mostly simple drivers, but not sure why
> > they don't need them. What else is the point of a trigger?
> >
> > > When this trigger is enabled for your LED, have your code switch LED
> > > state like it does now. When there is no trigger enabled, the userspace
> > > will be able to set brightness of this LED via sysfs.
> >
> > Right now I manage the button mute state directly from the input
> > handler (dualsense_parse_report) when the button is pressed and then
> > schedule an output report to toggle the LED and program the DualSense
> > to mute its audio (the PlayStation works very similar). I would need
> > to use led_trigger_event then here?
> >
> > If I then understand it right, I need to modify my "brightness_set"
> > handler and check if there is a trigger (based on
> > led_classdev->activated??). If there is none, then userspace can
> > change the LED state. Internally when I change the LED state, I will
> > also program the hardware to mute as well. (they are tied together)
> >
> > I am tempted to wait with the trigger code as I really don't understand it.
>
> Simple triggers are just normal triggers but with some simplifying code
> to avoid code repetition. Ignore them for now.
>
> When a trigger is set to a LED via sysfs, the trigger .activate()
> method is called and the led_cdev.trigger is set to point to that
> trigger.
>
> It is then up to the code inside the trigger's .activate() method to
> initialize mechanisms that will control the LED.
>
> For netdev trigger a delayed_work is scheduled periodically, and in each
> execution of that work's callback the netdevice's stats are compared to
> the last ones. If the new stats are greater, the trigger code blinks the
> LED.
>
> So in your case it is pretty simple to implement, because you already
> have the necessary code to manipulate the LED brightness automatically
> according to whether button was pressed. You are setting
>   ds->update_mic_mute = true;
> in dualsense_parse_report() and then manipulate the LED in
> dualsense_output_worker().
>
> Just add another boolean member into struct dualsense:
>   bool control_mute_led;
> and change the code in dualsense_output_worker() to only change the
> mute_led brightness is this new member is true.
>
> Add this code to your driver:
>
>   static struct led_hw_trigger_type ps_micmute_trigger_type;
>
> When registering the LED in ps_led_register(), also set
>   led->trigger_type = &ps_micmute_trigger_type;
>
> Add this functions:
>   static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
>   {
>     struct dualsense *ds = container_of(...);
>
>     /* make the worker control mute LED according to mute button */
>     ds->control_mute_led = true;
>
>     /* make sure the mute LED shows the current mute button state */
>     ds->update_mic_mute = true;
>     schedule_work(&ds->output_worker);
>
>     return 0;
>   }
>
>   static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
>   {
>     struct dualsense *ds = container_of(...);
>
>     ds->control_mute_led = false;
>   }
>
>   static struct led_trigger ps_micmute_trigger = {
>     .name = "playstation-micmute",
>     .activate = ps_micmute_trig_activate,
>     .deactivate = ps_micmute_trig_deactivate,
>     .trigger_type = &ps_micmute_trigger_type,
>   };
>
> Add this code to ps_init():
>   int ret;
>
>   ret = led_trigger_register(&ps_micmute_trigger);
>   if (ret)
>     return ret;
>
> And to ps_exit():
>   led_trigger_unregister(&ps_micmute_trigger);
>
> All this will make sure that the driver will manipulate the mute
> LED state only when the playstation-micmute trigger is active on the
> LED.
>
> Moreover if you want this driver to be active on the LED by default,
> set this prior to registering the LED
>   led->default_trigger = "playstation-micmute";
>
> Finally add code to dualsense_mute_led_set_brightness() to make
> userspace/[other LED triggers] able to set mute LED brightness.
>
> The purpose of the .trigger_type member of struct led_classdev and
> struct led_trigger is that if this member is set for a trigger, this
> trigger will only be available for LEDs that have the same trigger_type.
>

Thanks Marek for the in-depth 101 on LED triggers :)

However, I am not sure we want to enable LED triggers for the micmute
on the controller itself. In the early discussions with Roderick, I
already suggested the use of the LED triggers, and the problem was
that they are shared system-wide. This is good for many use cases, but
in that particular case, the user expects the mic *of the controller*
to be muted, not everyone's controller's mics.This is a behavior
inherited from the Playstation 5 which would be hard to sell to owners
of the controllers.

So if read-only LEDs are not an option, how about we simply ditch the
LED for micmute in the current code, and have a simple callback
executed by the driver to light up or not the LED when the player
presses the key. Or just revert entirely this commit in the currently
staged series. We can then figure out a better way in the next future
to handle that part.

BTW, AFAICT, the only problem we have left (if we put that micmute
issue aside) is about the naming convention. If we fix the naming
shortly, would you have any concerns if we still push that code to
Linus in 5.12-rc0?

Cheers,
Benjamin

Re: [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Benjamin Tissoires]

On Tue, Feb 16, 2021 at 3:37 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 17:11:14 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Mon, 15 Feb 2021 15:00:30 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > What is the desired naming for these player LEDs? There is not an
> > > > officially designed function based on DT bindings. So far they used
> > > > "playstation::mac::ledX". When changing the naming scheme towards
> > > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera.
> > >
> > > Hi,
> > >
> > > there is one more thing I forgot to mention in the LED name schema:
> > >   devicename:color:function-functionEnumerator
> > >
> > > So LED core can for example compose a names in the format:
> > >   switch0:green:lan-1
> > >   switch0:green:lan-2
> > >   switch0:green:lan-3
> > >   switch0:green:lan-4
> > >
> > > In your case I think the most appropriate name would be something like
> > >   hid0:color:indicator-1
> > >   hid0:color:indicator-2
> > >   ...
> >
> > I am trying to think if indicator is clear enough. Currently devices
> > use a mixture of names, which is obviously bad (wiimote uses p1-p4 at
> > the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses
> > player1-4). I would at least like new drivers to standardize. In
> > particular in Android frameworks we have a need to map these LEDs back
> > to the Java InputDevice. Finding the LEDs has been quite painful so
> > far.
>
> Thinking about it more, function "player" should theoretically be
> reasonable. Maybe we should try sending a patch for review, adding this
> funciton to include/dt-bindings/leds/common.h, and see what others
> think of it...

FWIW, I am not sure 'player' would be a good fit here. I personally
preferred "indicator".
My reasons are because those LEDs are basically a matrix of LEDs, and
are supposed to be read as a whole. For player 1, you would light up
the 3rd LED only. And for player 2, you would light up LEDS #2 and #4.

So I would say that in this particular case, "player" would lead to
confusion because users would want to set player 1 on the controller
and would have to talk to the "player-3" LED.

If we keep the more generic "indicator", the one-to-one mapping is
removed, and it's clearer that userspace needs an adaptor to convert
"players" into "indicator".

For the older controllers with a dedicated "player" LED, like the PS3
and the Wiimote, "player" would make sense, yes.

Cheers,
Benjamin

>
> > If this is what is decided, I guess we should update the Linux gamepad
> > document at some point as well.
> >
> > > Are these LEDs of different colors which are impossible to determine?
> > > The string "hid%d::led1" you mention above does not indicate color.
> >
> > The DualSense LEDs are all white (at least so far?). On controllers
> > from other brands I have seen them be red or green. So could indeed
> > use: "hid%d:white".
>
> Yes, a constant for white color is defined in headers.
>
> > > Marek
> >
> > Thanks,
> > Roderick
>

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Marek Behun]

On Tue, 16 Feb 2021 18:12:39 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Tue, 16 Feb 2021 00:33:24 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > >
> > > > On Mon, 15 Feb 2021 10:07:29 -0800
> > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > >  
> > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > > > >
> > > > > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > > >  
> > > > > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > > > >
> > > > > > > The DualSense controller has a built-in microphone exposed as an
> > > > > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > > > > button on the controller handles mute, but software has to configure
> > > > > > > the device to mute the audio stream.
> > > > > > >
> > > > > > > This patch captures the mute button and schedules an output report
> > > > > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > > > > >
> > > > > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>  
> > > > > >
> > > > > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > > > > for it?  
> > > > >
> > > > > Yes and no. The microphone is supported using USB, not yet using
> > > > > Bluetooth (uses a custom protocol). Actually there are various other
> > > > > audio features in the DualSense (headphone jack, speaker, volume
> > > > > controls,..) and they all work using custom protocols. We were
> > > > > planning to defer this work through future patches as the features are
> > > > > very complicated and need a deep analysis on how to realize them. For
> > > > > example audio controls work through HID, but for USB the audio driver
> > > > > is a generic hda audio device I think. Bluetooth is a custom protocol
> > > > > and will be yet a different audio driver somewhere.
> > > > >  
> > > > > > If it is, look at the audio-micmute LED trigger.
> > > > > >  
> > > > >
> > > > > I'm not sure if the expected behavior for the DualSense is similar to
> > > > > the standard audio mute use cases. My understanding of these triggers
> > > > > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > > > > space to send a signal to anything registering for a particular
> > > > > trigger. In this case a global micmute. Is that, right?
> > > > >
> > > > > In our case for PlayStation games, there are often multiple
> > > > > controllers connected and each user has their own microphone in their
> > > > > controller. All can function at the same time (different from a
> > > > > standard PC use case). That's why I'm wondering if this makes sense.I
> > > > > know we are on Linux, but for Sony we want to properly support such
> > > > > use cases.  
> > > >
> > > > If there aren't audio drivers yet for this, simply have this driver
> > > > also register a private LED trigger (with name "joystick-audiomute"
> > > > or something similar), and when registering the LED, set the
> > > > trigger_type member. Look at trigger_type in include/linux/leds.h, and
> > > > in LED Documentation.  
> > >
> > > Sorry for some more questions. I have been trying to understand
> > > triggers all night. The concept is just so strange and foreign to me.
> > > I understand it is in the end just a string and one use case is
> > > in-kernel IPC and you can configure them from user space as well, but
> > > I just don't get it. I understand you can use a trigger to in the end
> > > program your LED in a automatic manner. I just don't understand how
> > > the concepts fit together and how to implement it (maybe I will update
> > > the docs later on... they are a bit sparse for if you don't know this
> > > area).
> > >
> > > Regarding registering a private trigger. I see include/linux/leds.h
> > > have a comment about trigger_type and how it should be set for private
> > > triggers on led_classdev. I haven't been able to find any example
> > > usages of this within the kernel. It doesn't seem to be used in the
> > > kernel, maybe it is just around for future use? I also seem to need to
> > > implement my own activate/deactive callbacks for the trigger. These I
> > > would use to program the LED brightness I guess. Though I see various
> > > trigger drivers (drivers/leds/triggers), but not all of them have
> > > activate/deactivate callbacks. Mostly simple drivers, but not sure why
> > > they don't need them. What else is the point of a trigger?
> > >  
> > > > When this trigger is enabled for your LED, have your code switch LED
> > > > state like it does now. When there is no trigger enabled, the userspace
> > > > will be able to set brightness of this LED via sysfs.  
> > >
> > > Right now I manage the button mute state directly from the input
> > > handler (dualsense_parse_report) when the button is pressed and then
> > > schedule an output report to toggle the LED and program the DualSense
> > > to mute its audio (the PlayStation works very similar). I would need
> > > to use led_trigger_event then here?
> > >
> > > If I then understand it right, I need to modify my "brightness_set"
> > > handler and check if there is a trigger (based on
> > > led_classdev->activated??). If there is none, then userspace can
> > > change the LED state. Internally when I change the LED state, I will
> > > also program the hardware to mute as well. (they are tied together)
> > >
> > > I am tempted to wait with the trigger code as I really don't understand it.  
> >
> > Simple triggers are just normal triggers but with some simplifying code
> > to avoid code repetition. Ignore them for now.
> >
> > When a trigger is set to a LED via sysfs, the trigger .activate()
> > method is called and the led_cdev.trigger is set to point to that
> > trigger.
> >
> > It is then up to the code inside the trigger's .activate() method to
> > initialize mechanisms that will control the LED.
> >
> > For netdev trigger a delayed_work is scheduled periodically, and in each
> > execution of that work's callback the netdevice's stats are compared to
> > the last ones. If the new stats are greater, the trigger code blinks the
> > LED.
> >
> > So in your case it is pretty simple to implement, because you already
> > have the necessary code to manipulate the LED brightness automatically
> > according to whether button was pressed. You are setting
> >   ds->update_mic_mute = true;
> > in dualsense_parse_report() and then manipulate the LED in
> > dualsense_output_worker().
> >
> > Just add another boolean member into struct dualsense:
> >   bool control_mute_led;
> > and change the code in dualsense_output_worker() to only change the
> > mute_led brightness is this new member is true.
> >
> > Add this code to your driver:
> >
> >   static struct led_hw_trigger_type ps_micmute_trigger_type;
> >
> > When registering the LED in ps_led_register(), also set
> >   led->trigger_type = &ps_micmute_trigger_type;
> >
> > Add this functions:
> >   static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
> >   {
> >     struct dualsense *ds = container_of(...);
> >
> >     /* make the worker control mute LED according to mute button */
> >     ds->control_mute_led = true;
> >
> >     /* make sure the mute LED shows the current mute button state */
> >     ds->update_mic_mute = true;
> >     schedule_work(&ds->output_worker);
> >
> >     return 0;
> >   }
> >
> >   static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
> >   {
> >     struct dualsense *ds = container_of(...);
> >
> >     ds->control_mute_led = false;
> >   }
> >
> >   static struct led_trigger ps_micmute_trigger = {
> >     .name = "playstation-micmute",
> >     .activate = ps_micmute_trig_activate,
> >     .deactivate = ps_micmute_trig_deactivate,
> >     .trigger_type = &ps_micmute_trigger_type,
> >   };
> >
> > Add this code to ps_init():
> >   int ret;
> >
> >   ret = led_trigger_register(&ps_micmute_trigger);
> >   if (ret)
> >     return ret;
> >
> > And to ps_exit():
> >   led_trigger_unregister(&ps_micmute_trigger);
> >
> > All this will make sure that the driver will manipulate the mute
> > LED state only when the playstation-micmute trigger is active on the
> > LED.
> >
> > Moreover if you want this driver to be active on the LED by default,
> > set this prior to registering the LED
> >   led->default_trigger = "playstation-micmute";
> >
> > Finally add code to dualsense_mute_led_set_brightness() to make
> > userspace/[other LED triggers] able to set mute LED brightness.
> >
> > The purpose of the .trigger_type member of struct led_classdev and
> > struct led_trigger is that if this member is set for a trigger, this
> > trigger will only be available for LEDs that have the same trigger_type.
> >  
> 
> Thanks Marek for the in-depth 101 on LED triggers :)
> 
> However, I am not sure we want to enable LED triggers for the micmute
> on the controller itself. In the early discussions with Roderick, I
> already suggested the use of the LED triggers, and the problem was
> that they are shared system-wide. This is good for many use cases, but
> in that particular case, the user expects the mic *of the controller*
> to be muted, not everyone's controller's mics.This is a behavior
> inherited from the Playstation 5 which would be hard to sell to owners
> of the controllers.

They are not system wide if private LED trigger API is used, as I
explained and as the example code does it in my previous reply. This
trigger will only be available for PlayStation micmute LEDs.

> So if read-only LEDs are not an option, how about we simply ditch the
> LED for micmute in the current code, and have a simple callback
> executed by the driver to light up or not the LED when the player
> presses the key. Or just revert entirely this commit in the currently
> staged series. We can then figure out a better way in the next future
> to handle that part.

This is irrelevant if you take into account the information I wrote
above.

> BTW, AFAICT, the only problem we have left (if we put that micmute
> issue aside) is about the naming convention. If we fix the naming
> shortly, would you have any concerns if we still push that code to
> Linus in 5.12-rc0?

Yes, if naming is corrected I have no issue with this. LED triggers can
be sent to 5.13.

Marek

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Benjamin Tissoires]

On Mon, Feb 15, 2021 at 7:21 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 09:51:15 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Mon, 15 Feb 2021 07:36:58 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > Hi Marek,
> > > >
> > > > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > > >
> > > > > On Sun, 14 Feb 2021 16:45:46 -0800
> > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > >
> > > > > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > > > > +                     ps_dev->mac_address);
> > > > > ...
> > > > > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
> > > > >
> > > > > The LED subsystem has a predefined schema by which LED names should
> > > > > look like:
> > > > >   devicename:color:function
> > > > > (Not all fields are required, but the order must be preserved. The ':'
> > > > >  character should be used only as separator of these fields, so not MAC
> > > > >  addresses in these names, it will confuse userspace parsers.)
> > > > > See Documentation/leds/leds-class.rst
> > > > >
> > > > > The devicename part should not be "playstation". It should be something
> > > > > otherwise recognizable from userspace. For example an mmc indicator has
> > > > > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> > > > >
> > > > > In your case the name should be something like:
> > > > >   input3:rgb:indicator
> > > >
> > > > Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> > > > are added to the 'parent' HID device, not the input devices. In case
> > > > of DualSense it is actually implemented as a composite device with
> > > > mulitple input devices (gamepad, touchpad and motion sensors) per HID
> > > > device. The device name of HID devices seems to be something like:
> > > > <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> > > > 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> > > >
> > > > This is I guess why many HID devices in general pick their own names
> > > > (and not all have need to have input devices I guess). Though Benjamin
> > > > and Jiri know better.
> > > >
> > > > I'm not sure what naming could make sense here. The previous Sony
> > > > driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> > > > on DualShock 4.
> > >
> > > We have to find a reasonable devicename here. If each joystick registers
> > > multiple input devices, it cannot be "input%d". I suppose there isn't
> > > an API for grouping mulitple input devices toghether into inputgroups.
> > > Maybe it could be in the format "joystick%d".
> >
> > Yeah, there is no inputgroups mechanism.  It could use some type of
> > joystick name if that's what desired. However, there is no common ID
> > code. Individual drivers are sometimes calculating their own IDs
> > (hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
> > for hid-sony/hid-playstation the use case for tracking IDs is for a
> > part to prevent duplicate devices as you can connect your device using
> > both bluetooth and USB. So would be "ps-joystick0"
> >
> > At the HID layer there does seem to be a unique ID, but it is only
> > exposed in the name string: This is how the name is constructed:
> >      dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
> >              hdev->vendor, hdev->product, atomic_inc_return(&id));
> >
> > This ID is HID specific, but not all input devices use HID.
> >
> > I'm not entirely sure what makes sense...
>
> So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> but it is only stored in string form as part of device name.

Yes and no. This atomic_inc is only used to allow a sysfs tree,
because you can have several HID devices below the same USB, I2C or
UHID physical device. From the userspace, no-one cares about that ID,
because all HID devices are exported as input, IIO or hidraw nodes.

So using this "id" would not allow for a direct mapping HID device ->
sysfs entry because users will still have to walk through the tree to
find out which is which.

An actual one-to-one mapping would using 'hidrawX' because there is a
one-to-one mapping between /dev/hidrawX for HID devices. However, this
means that we consider the bus to be hidraw which is plain wrong too.

The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
`BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
we standardize LEDs on the HID subsystem to be named
`hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
between the LED and the sysfs, and would also allow users to quickly
filter out the playstation ones.

Cheers,
Benjamin

>
> Send a patch to hid-core to make this atomic_inc_return(&id) also be
> stored into struct hid_device as an integer, not only as a part
> of the device name string.
>
> Then use "hid%d" as the devicename for this LED, with %d substituted
> with this ID.
>
> Marek
>

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Marek Behun]

On Tue, 16 Feb 2021 18:21:55 +0100
Marek Behun <marek.behun@nic.cz> wrote:

> > BTW, AFAICT, the only problem we have left (if we put that micmute
> > issue aside) is about the naming convention. If we fix the naming
> > shortly, would you have any concerns if we still push that code to
> > Linus in 5.12-rc0?  

So I had a call with Pavel Machek, who is maintainer of the LED
subsystem, and he thinks that you should wait with the LED patches
until they are acked/reviewed by others. So if you want to send a
pull-request to Linus, you probably should remove the LED patches for
now. They can be added later when they are reviewed.

> Yes, if naming is corrected I have no issue with this. LED triggers can
> be sent to 5.13.

By that I mean that in my opinion the best naming scheme here would be:
  hid%d:COLOR:micmute
for micmute LED (there is LED_FUNCTION_MICMUTE constant defined in
include/dt-bindings/leds/common.h)
and
  hid%d:COLOR:player-%d
for player LEDs.

This will need adding LED_FUNCTION_PLAYER constant, which will need
sending a patch that will need to be reviewed by LED people (and
applied by Pavel). I talked with Pavel and he thinks that if there are
other HID drivers (for joysticks) that also have this kind of LEDs
(identifiing players), than such a patch would be reasonable (and those
LEDs should use the constant). Otherwise LED_FUNCTION_INDICATOR should
be probably used.

Marek

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Benjamin Tissoires]

On Tue, Feb 16, 2021 at 6:22 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Tue, 16 Feb 2021 18:12:39 +0100
> Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
> > On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Tue, 16 Feb 2021 00:33:24 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > > >
> > > > > On Mon, 15 Feb 2021 10:07:29 -0800
> > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > >
> > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > > > > >
> > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > > > >
> > > > > > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > > > > >
> > > > > > > > The DualSense controller has a built-in microphone exposed as an
> > > > > > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > > > > > button on the controller handles mute, but software has to configure
> > > > > > > > the device to mute the audio stream.
> > > > > > > >
> > > > > > > > This patch captures the mute button and schedules an output report
> > > > > > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > > > > > >
> > > > > > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > > > >
> > > > > > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > > > > > for it?
> > > > > >
> > > > > > Yes and no. The microphone is supported using USB, not yet using
> > > > > > Bluetooth (uses a custom protocol). Actually there are various other
> > > > > > audio features in the DualSense (headphone jack, speaker, volume
> > > > > > controls,..) and they all work using custom protocols. We were
> > > > > > planning to defer this work through future patches as the features are
> > > > > > very complicated and need a deep analysis on how to realize them. For
> > > > > > example audio controls work through HID, but for USB the audio driver
> > > > > > is a generic hda audio device I think. Bluetooth is a custom protocol
> > > > > > and will be yet a different audio driver somewhere.
> > > > > >
> > > > > > > If it is, look at the audio-micmute LED trigger.
> > > > > > >
> > > > > >
> > > > > > I'm not sure if the expected behavior for the DualSense is similar to
> > > > > > the standard audio mute use cases. My understanding of these triggers
> > > > > > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > > > > > space to send a signal to anything registering for a particular
> > > > > > trigger. In this case a global micmute. Is that, right?
> > > > > >
> > > > > > In our case for PlayStation games, there are often multiple
> > > > > > controllers connected and each user has their own microphone in their
> > > > > > controller. All can function at the same time (different from a
> > > > > > standard PC use case). That's why I'm wondering if this makes sense.I
> > > > > > know we are on Linux, but for Sony we want to properly support such
> > > > > > use cases.
> > > > >
> > > > > If there aren't audio drivers yet for this, simply have this driver
> > > > > also register a private LED trigger (with name "joystick-audiomute"
> > > > > or something similar), and when registering the LED, set the
> > > > > trigger_type member. Look at trigger_type in include/linux/leds.h, and
> > > > > in LED Documentation.
> > > >
> > > > Sorry for some more questions. I have been trying to understand
> > > > triggers all night. The concept is just so strange and foreign to me.
> > > > I understand it is in the end just a string and one use case is
> > > > in-kernel IPC and you can configure them from user space as well, but
> > > > I just don't get it. I understand you can use a trigger to in the end
> > > > program your LED in a automatic manner. I just don't understand how
> > > > the concepts fit together and how to implement it (maybe I will update
> > > > the docs later on... they are a bit sparse for if you don't know this
> > > > area).
> > > >
> > > > Regarding registering a private trigger. I see include/linux/leds.h
> > > > have a comment about trigger_type and how it should be set for private
> > > > triggers on led_classdev. I haven't been able to find any example
> > > > usages of this within the kernel. It doesn't seem to be used in the
> > > > kernel, maybe it is just around for future use? I also seem to need to
> > > > implement my own activate/deactive callbacks for the trigger. These I
> > > > would use to program the LED brightness I guess. Though I see various
> > > > trigger drivers (drivers/leds/triggers), but not all of them have
> > > > activate/deactivate callbacks. Mostly simple drivers, but not sure why
> > > > they don't need them. What else is the point of a trigger?
> > > >
> > > > > When this trigger is enabled for your LED, have your code switch LED
> > > > > state like it does now. When there is no trigger enabled, the userspace
> > > > > will be able to set brightness of this LED via sysfs.
> > > >
> > > > Right now I manage the button mute state directly from the input
> > > > handler (dualsense_parse_report) when the button is pressed and then
> > > > schedule an output report to toggle the LED and program the DualSense
> > > > to mute its audio (the PlayStation works very similar). I would need
> > > > to use led_trigger_event then here?
> > > >
> > > > If I then understand it right, I need to modify my "brightness_set"
> > > > handler and check if there is a trigger (based on
> > > > led_classdev->activated??). If there is none, then userspace can
> > > > change the LED state. Internally when I change the LED state, I will
> > > > also program the hardware to mute as well. (they are tied together)
> > > >
> > > > I am tempted to wait with the trigger code as I really don't understand it.
> > >
> > > Simple triggers are just normal triggers but with some simplifying code
> > > to avoid code repetition. Ignore them for now.
> > >
> > > When a trigger is set to a LED via sysfs, the trigger .activate()
> > > method is called and the led_cdev.trigger is set to point to that
> > > trigger.
> > >
> > > It is then up to the code inside the trigger's .activate() method to
> > > initialize mechanisms that will control the LED.
> > >
> > > For netdev trigger a delayed_work is scheduled periodically, and in each
> > > execution of that work's callback the netdevice's stats are compared to
> > > the last ones. If the new stats are greater, the trigger code blinks the
> > > LED.
> > >
> > > So in your case it is pretty simple to implement, because you already
> > > have the necessary code to manipulate the LED brightness automatically
> > > according to whether button was pressed. You are setting
> > >   ds->update_mic_mute = true;
> > > in dualsense_parse_report() and then manipulate the LED in
> > > dualsense_output_worker().
> > >
> > > Just add another boolean member into struct dualsense:
> > >   bool control_mute_led;
> > > and change the code in dualsense_output_worker() to only change the
> > > mute_led brightness is this new member is true.
> > >
> > > Add this code to your driver:
> > >
> > >   static struct led_hw_trigger_type ps_micmute_trigger_type;
> > >
> > > When registering the LED in ps_led_register(), also set
> > >   led->trigger_type = &ps_micmute_trigger_type;
> > >
> > > Add this functions:
> > >   static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
> > >   {
> > >     struct dualsense *ds = container_of(...);
> > >
> > >     /* make the worker control mute LED according to mute button */
> > >     ds->control_mute_led = true;
> > >
> > >     /* make sure the mute LED shows the current mute button state */
> > >     ds->update_mic_mute = true;
> > >     schedule_work(&ds->output_worker);
> > >
> > >     return 0;
> > >   }
> > >
> > >   static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
> > >   {
> > >     struct dualsense *ds = container_of(...);
> > >
> > >     ds->control_mute_led = false;
> > >   }
> > >
> > >   static struct led_trigger ps_micmute_trigger = {
> > >     .name = "playstation-micmute",
> > >     .activate = ps_micmute_trig_activate,
> > >     .deactivate = ps_micmute_trig_deactivate,
> > >     .trigger_type = &ps_micmute_trigger_type,
> > >   };
> > >
> > > Add this code to ps_init():
> > >   int ret;
> > >
> > >   ret = led_trigger_register(&ps_micmute_trigger);
> > >   if (ret)
> > >     return ret;
> > >
> > > And to ps_exit():
> > >   led_trigger_unregister(&ps_micmute_trigger);
> > >
> > > All this will make sure that the driver will manipulate the mute
> > > LED state only when the playstation-micmute trigger is active on the
> > > LED.
> > >
> > > Moreover if you want this driver to be active on the LED by default,
> > > set this prior to registering the LED
> > >   led->default_trigger = "playstation-micmute";
> > >
> > > Finally add code to dualsense_mute_led_set_brightness() to make
> > > userspace/[other LED triggers] able to set mute LED brightness.
> > >
> > > The purpose of the .trigger_type member of struct led_classdev and
> > > struct led_trigger is that if this member is set for a trigger, this
> > > trigger will only be available for LEDs that have the same trigger_type.
> > >
> >
> > Thanks Marek for the in-depth 101 on LED triggers :)
> >
> > However, I am not sure we want to enable LED triggers for the micmute
> > on the controller itself. In the early discussions with Roderick, I
> > already suggested the use of the LED triggers, and the problem was
> > that they are shared system-wide. This is good for many use cases, but
> > in that particular case, the user expects the mic *of the controller*
> > to be muted, not everyone's controller's mics.This is a behavior
> > inherited from the Playstation 5 which would be hard to sell to owners
> > of the controllers.
>
> They are not system wide if private LED trigger API is used, as I
> explained and as the example code does it in my previous reply. This
> trigger will only be available for PlayStation micmute LEDs.

My initial reply was the following:
"""
Unless I am reading this all wrong, the "private" is private for the
driver, not the device itself. If I have 2 controllers connected, and
both are set to "playstation-micmute", if one controller sends the
trigger because the button is pressed, both controllers would receive
the trigger, no?
"""

But after re-reading your explanations it seems you are not asking the
trigger to be actually "triggered". So basically, the private trigger
is just a way for the driver to handle its own business and allow
other triggers to be set. I always thought that whenever we declare a
trigger, we *had* to use it, but actually using the trigger just as a
way to confer information is smart.

Cheers,
Benjamin

>
> > So if read-only LEDs are not an option, how about we simply ditch the
> > LED for micmute in the current code, and have a simple callback
> > executed by the driver to light up or not the LED when the player
> > presses the key. Or just revert entirely this commit in the currently
> > staged series. We can then figure out a better way in the next future
> > to handle that part.
>
> This is irrelevant if you take into account the information I wrote
> above.
>
> > BTW, AFAICT, the only problem we have left (if we put that micmute
> > issue aside) is about the naming convention. If we fix the naming
> > shortly, would you have any concerns if we still push that code to
> > Linus in 5.12-rc0?
>
> Yes, if naming is corrected I have no issue with this. LED triggers can
> be sent to 5.13.
>
> Marek
>

Re: [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support.

[Marek Behun]

On Tue, 16 Feb 2021 18:19:49 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> On Tue, Feb 16, 2021 at 3:37 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Mon, 15 Feb 2021 17:11:14 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote:  
> > > >
> > > > On Mon, 15 Feb 2021 15:00:30 -0800
> > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > >  
> > > > > What is the desired naming for these player LEDs? There is not an
> > > > > officially designed function based on DT bindings. So far they used
> > > > > "playstation::mac::ledX". When changing the naming scheme towards
> > > > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera.  
> > > >
> > > > Hi,
> > > >
> > > > there is one more thing I forgot to mention in the LED name schema:
> > > >   devicename:color:function-functionEnumerator
> > > >
> > > > So LED core can for example compose a names in the format:
> > > >   switch0:green:lan-1
> > > >   switch0:green:lan-2
> > > >   switch0:green:lan-3
> > > >   switch0:green:lan-4
> > > >
> > > > In your case I think the most appropriate name would be something like
> > > >   hid0:color:indicator-1
> > > >   hid0:color:indicator-2
> > > >   ...  
> > >
> > > I am trying to think if indicator is clear enough. Currently devices
> > > use a mixture of names, which is obviously bad (wiimote uses p1-p4 at
> > > the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses
> > > player1-4). I would at least like new drivers to standardize. In
> > > particular in Android frameworks we have a need to map these LEDs back
> > > to the Java InputDevice. Finding the LEDs has been quite painful so
> > > far.  
> >
> > Thinking about it more, function "player" should theoretically be
> > reasonable. Maybe we should try sending a patch for review, adding this
> > funciton to include/dt-bindings/leds/common.h, and see what others
> > think of it...  
> 
> FWIW, I am not sure 'player' would be a good fit here. I personally
> preferred "indicator".
> My reasons are because those LEDs are basically a matrix of LEDs, and
> are supposed to be read as a whole. For player 1, you would light up
> the 3rd LED only. And for player 2, you would light up LEDS #2 and #4.
> 
> So I would say that in this particular case, "player" would lead to
> confusion because users would want to set player 1 on the controller
> and would have to talk to the "player-3" LED.
> 
> If we keep the more generic "indicator", the one-to-one mapping is
> removed, and it's clearer that userspace needs an adaptor to convert
> "players" into "indicator".
> 
> For the older controllers with a dedicated "player" LED, like the PS3
> and the Wiimote, "player" would make sense, yes.

OK, in that case "indicator" is more reasonable. But all this means that
this should simply be discussed more on LED mailing list. We need to
wait till other people express their opinions.

Marek

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Marek Behun]

On Tue, 16 Feb 2021 18:29:46 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> > So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> > but it is only stored in string form as part of device name.  
> 
> Yes and no. This atomic_inc is only used to allow a sysfs tree,
> because you can have several HID devices below the same USB, I2C or
> UHID physical device. From the userspace, no-one cares about that ID,
> because all HID devices are exported as input, IIO or hidraw nodes.
> 
> So using this "id" would not allow for a direct mapping HID device ->
> sysfs entry because users will still have to walk through the tree to
> find out which is which.

So you are saying that the fact that userspace cannot take the number
from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
the problem here.

This is not a problem in my opinion, because userspace can simply
access the parent HID device via /sys/class/leds/hidN:color:func/parent.

In fact we did something similar for LEDs connected to ethernet PHYs.
To summarize:
  - ethernet PHYs are identified by long, sometimes crazy strings like
      d0032004.mdio-mii:01
    or even
      /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
  - for the purposes of having a sane devicename part in LED names, I
    sent this patch
    https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
    which adds a simple incrementing integer ID to each PHY device.
    (The code is not in upstream yet because there is other work needed
     and because I decided that some functionality has to be available
     via a different mechanism, but this part is complete and reviewed.)

> An actual one-to-one mapping would using 'hidrawX' because there is a
> one-to-one mapping between /dev/hidrawX for HID devices. However, this
> means that we consider the bus to be hidraw which is plain wrong too.
> 
> The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
> `BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
> we standardize LEDs on the HID subsystem to be named
> `hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
> between the LED and the sysfs, and would also allow users to quickly
> filter out the playstation ones.

As I wrote in other e-mail some minutes ago, this just means that we
need to wait for other people's opinions. Please do not send this
pull-request with the LED patches until this is resolved.

Marek

Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Marek Behun]

On Tue, 16 Feb 2021 18:42:19 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> On Tue, Feb 16, 2021 at 6:22 PM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Tue, 16 Feb 2021 18:12:39 +0100
> > Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >  
> > > On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote:  
> > > >
> > > > On Tue, 16 Feb 2021 00:33:24 -0800
> > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > >  
> > > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > > > >
> > > > > > On Mon, 15 Feb 2021 10:07:29 -0800
> > > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > > >  
> > > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > > > > > >
> > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > > > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > > > > >  
> > > > > > > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > > > > > >
> > > > > > > > > The DualSense controller has a built-in microphone exposed as an
> > > > > > > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > > > > > > button on the controller handles mute, but software has to configure
> > > > > > > > > the device to mute the audio stream.
> > > > > > > > >
> > > > > > > > > This patch captures the mute button and schedules an output report
> > > > > > > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>  
> > > > > > > >
> > > > > > > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > > > > > > for it?  
> > > > > > >
> > > > > > > Yes and no. The microphone is supported using USB, not yet using
> > > > > > > Bluetooth (uses a custom protocol). Actually there are various other
> > > > > > > audio features in the DualSense (headphone jack, speaker, volume
> > > > > > > controls,..) and they all work using custom protocols. We were
> > > > > > > planning to defer this work through future patches as the features are
> > > > > > > very complicated and need a deep analysis on how to realize them. For
> > > > > > > example audio controls work through HID, but for USB the audio driver
> > > > > > > is a generic hda audio device I think. Bluetooth is a custom protocol
> > > > > > > and will be yet a different audio driver somewhere.
> > > > > > >  
> > > > > > > > If it is, look at the audio-micmute LED trigger.
> > > > > > > >  
> > > > > > >
> > > > > > > I'm not sure if the expected behavior for the DualSense is similar to
> > > > > > > the standard audio mute use cases. My understanding of these triggers
> > > > > > > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > > > > > > space to send a signal to anything registering for a particular
> > > > > > > trigger. In this case a global micmute. Is that, right?
> > > > > > >
> > > > > > > In our case for PlayStation games, there are often multiple
> > > > > > > controllers connected and each user has their own microphone in their
> > > > > > > controller. All can function at the same time (different from a
> > > > > > > standard PC use case). That's why I'm wondering if this makes sense.I
> > > > > > > know we are on Linux, but for Sony we want to properly support such
> > > > > > > use cases.  
> > > > > >
> > > > > > If there aren't audio drivers yet for this, simply have this driver
> > > > > > also register a private LED trigger (with name "joystick-audiomute"
> > > > > > or something similar), and when registering the LED, set the
> > > > > > trigger_type member. Look at trigger_type in include/linux/leds.h, and
> > > > > > in LED Documentation.  
> > > > >
> > > > > Sorry for some more questions. I have been trying to understand
> > > > > triggers all night. The concept is just so strange and foreign to me.
> > > > > I understand it is in the end just a string and one use case is
> > > > > in-kernel IPC and you can configure them from user space as well, but
> > > > > I just don't get it. I understand you can use a trigger to in the end
> > > > > program your LED in a automatic manner. I just don't understand how
> > > > > the concepts fit together and how to implement it (maybe I will update
> > > > > the docs later on... they are a bit sparse for if you don't know this
> > > > > area).
> > > > >
> > > > > Regarding registering a private trigger. I see include/linux/leds.h
> > > > > have a comment about trigger_type and how it should be set for private
> > > > > triggers on led_classdev. I haven't been able to find any example
> > > > > usages of this within the kernel. It doesn't seem to be used in the
> > > > > kernel, maybe it is just around for future use? I also seem to need to
> > > > > implement my own activate/deactive callbacks for the trigger. These I
> > > > > would use to program the LED brightness I guess. Though I see various
> > > > > trigger drivers (drivers/leds/triggers), but not all of them have
> > > > > activate/deactivate callbacks. Mostly simple drivers, but not sure why
> > > > > they don't need them. What else is the point of a trigger?
> > > > >  
> > > > > > When this trigger is enabled for your LED, have your code switch LED
> > > > > > state like it does now. When there is no trigger enabled, the userspace
> > > > > > will be able to set brightness of this LED via sysfs.  
> > > > >
> > > > > Right now I manage the button mute state directly from the input
> > > > > handler (dualsense_parse_report) when the button is pressed and then
> > > > > schedule an output report to toggle the LED and program the DualSense
> > > > > to mute its audio (the PlayStation works very similar). I would need
> > > > > to use led_trigger_event then here?
> > > > >
> > > > > If I then understand it right, I need to modify my "brightness_set"
> > > > > handler and check if there is a trigger (based on
> > > > > led_classdev->activated??). If there is none, then userspace can
> > > > > change the LED state. Internally when I change the LED state, I will
> > > > > also program the hardware to mute as well. (they are tied together)
> > > > >
> > > > > I am tempted to wait with the trigger code as I really don't understand it.  
> > > >
> > > > Simple triggers are just normal triggers but with some simplifying code
> > > > to avoid code repetition. Ignore them for now.
> > > >
> > > > When a trigger is set to a LED via sysfs, the trigger .activate()
> > > > method is called and the led_cdev.trigger is set to point to that
> > > > trigger.
> > > >
> > > > It is then up to the code inside the trigger's .activate() method to
> > > > initialize mechanisms that will control the LED.
> > > >
> > > > For netdev trigger a delayed_work is scheduled periodically, and in each
> > > > execution of that work's callback the netdevice's stats are compared to
> > > > the last ones. If the new stats are greater, the trigger code blinks the
> > > > LED.
> > > >
> > > > So in your case it is pretty simple to implement, because you already
> > > > have the necessary code to manipulate the LED brightness automatically
> > > > according to whether button was pressed. You are setting
> > > >   ds->update_mic_mute = true;
> > > > in dualsense_parse_report() and then manipulate the LED in
> > > > dualsense_output_worker().
> > > >
> > > > Just add another boolean member into struct dualsense:
> > > >   bool control_mute_led;
> > > > and change the code in dualsense_output_worker() to only change the
> > > > mute_led brightness is this new member is true.
> > > >
> > > > Add this code to your driver:
> > > >
> > > >   static struct led_hw_trigger_type ps_micmute_trigger_type;
> > > >
> > > > When registering the LED in ps_led_register(), also set
> > > >   led->trigger_type = &ps_micmute_trigger_type;
> > > >
> > > > Add this functions:
> > > >   static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
> > > >   {
> > > >     struct dualsense *ds = container_of(...);
> > > >
> > > >     /* make the worker control mute LED according to mute button */
> > > >     ds->control_mute_led = true;
> > > >
> > > >     /* make sure the mute LED shows the current mute button state */
> > > >     ds->update_mic_mute = true;
> > > >     schedule_work(&ds->output_worker);
> > > >
> > > >     return 0;
> > > >   }
> > > >
> > > >   static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
> > > >   {
> > > >     struct dualsense *ds = container_of(...);
> > > >
> > > >     ds->control_mute_led = false;
> > > >   }
> > > >
> > > >   static struct led_trigger ps_micmute_trigger = {
> > > >     .name = "playstation-micmute",
> > > >     .activate = ps_micmute_trig_activate,
> > > >     .deactivate = ps_micmute_trig_deactivate,
> > > >     .trigger_type = &ps_micmute_trigger_type,
> > > >   };
> > > >
> > > > Add this code to ps_init():
> > > >   int ret;
> > > >
> > > >   ret = led_trigger_register(&ps_micmute_trigger);
> > > >   if (ret)
> > > >     return ret;
> > > >
> > > > And to ps_exit():
> > > >   led_trigger_unregister(&ps_micmute_trigger);
> > > >
> > > > All this will make sure that the driver will manipulate the mute
> > > > LED state only when the playstation-micmute trigger is active on the
> > > > LED.
> > > >
> > > > Moreover if you want this driver to be active on the LED by default,
> > > > set this prior to registering the LED
> > > >   led->default_trigger = "playstation-micmute";
> > > >
> > > > Finally add code to dualsense_mute_led_set_brightness() to make
> > > > userspace/[other LED triggers] able to set mute LED brightness.
> > > >
> > > > The purpose of the .trigger_type member of struct led_classdev and
> > > > struct led_trigger is that if this member is set for a trigger, this
> > > > trigger will only be available for LEDs that have the same trigger_type.
> > > >  
> > >
> > > Thanks Marek for the in-depth 101 on LED triggers :)
> > >
> > > However, I am not sure we want to enable LED triggers for the micmute
> > > on the controller itself. In the early discussions with Roderick, I
> > > already suggested the use of the LED triggers, and the problem was
> > > that they are shared system-wide. This is good for many use cases, but
> > > in that particular case, the user expects the mic *of the controller*
> > > to be muted, not everyone's controller's mics.This is a behavior
> > > inherited from the Playstation 5 which would be hard to sell to owners
> > > of the controllers.  
> >
> > They are not system wide if private LED trigger API is used, as I
> > explained and as the example code does it in my previous reply. This
> > trigger will only be available for PlayStation micmute LEDs.  
> 
> My initial reply was the following:
> """
> Unless I am reading this all wrong, the "private" is private for the
> driver, not the device itself. If I have 2 controllers connected, and
> both are set to "playstation-micmute", if one controller sends the
> trigger because the button is pressed, both controllers would receive
> the trigger, no?
> """
> 
> But after re-reading your explanations it seems you are not asking the
> trigger to be actually "triggered". So basically, the private trigger
> is just a way for the driver to handle its own business and allow
> other triggers to be set. I always thought that whenever we declare a
> trigger, we *had* to use it, but actually using the trigger just as a
> way to confer information is smart.

Not only private triggers work this way. All but simple triggers do.

If you use the "netdev" trigger to 2 differnet LEDs, you can set it to
blink LED 1 on activity on eth0 device, and LED 2 to blink on activity
on eth1 device. So clearly they are triggering just the LEDs that are
configured to be triggered, in such a way that was desired for each LED.

Marek

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Benjamin Tissoires]

On Tue, Feb 16, 2021 at 6:56 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Tue, 16 Feb 2021 18:29:46 +0100
> Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
> > > So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> > > but it is only stored in string form as part of device name.
> >
> > Yes and no. This atomic_inc is only used to allow a sysfs tree,
> > because you can have several HID devices below the same USB, I2C or
> > UHID physical device. From the userspace, no-one cares about that ID,
> > because all HID devices are exported as input, IIO or hidraw nodes.
> >
> > So using this "id" would not allow for a direct mapping HID device ->
> > sysfs entry because users will still have to walk through the tree to
> > find out which is which.
>
> So you are saying that the fact that userspace cannot take the number
> from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
> the problem here.
>
> This is not a problem in my opinion, because userspace can simply
> access the parent HID device via /sys/class/leds/hidN:color:func/parent.

So in that case, there is no real point at keeping this ID in sync
with anything else? I would be more willing to accept a patch in HID
core that keeps this ID just for HID LEDs, instead of adding just an
ID with no meaning to all HID devices.

Honestly, I think the whole LED class creation API should be
revisited. I guess this is not the first time this problem arises, and
you must be tired of having to chase down users.

If I had to deal with that situation once for all, I would deprecate
the current led class creation API, and add a new API that doesn't
take a free-form string as the name but constrain the name to be
formed by your requirements. This would also send a clear message to
all subsystems because the changes have to be propagated, and then,
all the maintainers would know about this problem. Bonus point, if you
need only "subsystem", "color" and "function", that means that the ID
can be stored internally to the led class and you'll get happy users.

>
> In fact we did something similar for LEDs connected to ethernet PHYs.
> To summarize:
>   - ethernet PHYs are identified by long, sometimes crazy strings like
>       d0032004.mdio-mii:01
>     or even
>       /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
>   - for the purposes of having a sane devicename part in LED names, I
>     sent this patch
>     https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
>     which adds a simple incrementing integer ID to each PHY device.
>     (The code is not in upstream yet because there is other work needed
>      and because I decided that some functionality has to be available
>      via a different mechanism, but this part is complete and reviewed.)
>
> > An actual one-to-one mapping would using 'hidrawX' because there is a
> > one-to-one mapping between /dev/hidrawX for HID devices. However, this
> > means that we consider the bus to be hidraw which is plain wrong too.
> >
> > The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
> > `BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
> > we standardize LEDs on the HID subsystem to be named
> > `hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
> > between the LED and the sysfs, and would also allow users to quickly
> > filter out the playstation ones.
>
> As I wrote in other e-mail some minutes ago, this just means that we
> need to wait for other people's opinions. Please do not send this
> pull-request with the LED patches until this is resolved.
>

Yeah, I just asked Roderick to see if he can revert those patches
while keeping the functionality behind those. I am more concerned
about the micmute button, because we should really offer that feature
to users. The associated LED class has no real benefits for now, so
that code needs a little bit of care instead of a plain revert.

Cheers,
Benjamin

Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support

[Marek Behun]

On Tue, 16 Feb 2021 19:14:48 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> On Tue, Feb 16, 2021 at 6:56 PM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Tue, 16 Feb 2021 18:29:46 +0100
> > Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >  
> > > > So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> > > > but it is only stored in string form as part of device name.  
> > >
> > > Yes and no. This atomic_inc is only used to allow a sysfs tree,
> > > because you can have several HID devices below the same USB, I2C or
> > > UHID physical device. From the userspace, no-one cares about that ID,
> > > because all HID devices are exported as input, IIO or hidraw nodes.
> > >
> > > So using this "id" would not allow for a direct mapping HID device ->
> > > sysfs entry because users will still have to walk through the tree to
> > > find out which is which.  
> >
> > So you are saying that the fact that userspace cannot take the number
> > from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
> > the problem here.
> >
> > This is not a problem in my opinion, because userspace can simply
> > access the parent HID device via /sys/class/leds/hidN:color:func/parent.  
> 
> So in that case, there is no real point at keeping this ID in sync
> with anything else? I would be more willing to accept a patch in HID
> core that keeps this ID just for HID LEDs, instead of adding just an
> ID with no meaning to all HID devices.

I think there was some misunderstanding.

If there are multiple LEDs on one joystick, all these LEDs should have
the same devicename part of the LED name. Different joysticks should
have different devicename parts of their LEDs names.

As another example think about keyboard LEDs if I have 2 keyboards
  input3:green:numlock
  input3:green:capslock
  input3:green:scrolllock
  input4:green:numlock
  input4:green:capslock
  input4:green:scrolllock

> Honestly, I think the whole LED class creation API should be
> revisited. I guess this is not the first time this problem arises, and
> you must be tired of having to chase down users.

I will not argue with you about this since it is true. The work is
slow though because lack of people and time. I too have some ideas for
the LED subsystem but I also have many other priorities in work. Pavel
has a TODO list in drivers/leds/TODO. The main thing probably is that
it would be great to have more input from other kernel people when
doing something in LEDs, but either not that many people subscribe to
linux-leds mailing list or we should be informing them via different
mechanisms...

> If I had to deal with that situation once for all, I would deprecate
> the current led class creation API, and add a new API that doesn't
> take a free-form string as the name but constrain the name to be
> formed by your requirements. This would also send a clear message to
> all subsystems because the changes have to be propagated, and then,
> all the maintainers would know about this problem. Bonus point, if you
> need only "subsystem", "color" and "function", that means that the ID
> can be stored internally to the led class and you'll get happy users.

As I mentioned in the example above, there can be multiple LEDs for one
devicename...

> >
> > In fact we did something similar for LEDs connected to ethernet PHYs.
> > To summarize:
> >   - ethernet PHYs are identified by long, sometimes crazy strings like
> >       d0032004.mdio-mii:01
> >     or even
> >       /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
> >   - for the purposes of having a sane devicename part in LED names, I
> >     sent this patch
> >     https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
> >     which adds a simple incrementing integer ID to each PHY device.
> >     (The code is not in upstream yet because there is other work needed
> >      and because I decided that some functionality has to be available
> >      via a different mechanism, but this part is complete and reviewed.)
> >  
> > > An actual one-to-one mapping would using 'hidrawX' because there is a
> > > one-to-one mapping between /dev/hidrawX for HID devices. However, this
> > > means that we consider the bus to be hidraw which is plain wrong too.
> > >
> > > The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
> > > `BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
> > > we standardize LEDs on the HID subsystem to be named
> > > `hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
> > > between the LED and the sysfs, and would also allow users to quickly
> > > filter out the playstation ones.  
> >
> > As I wrote in other e-mail some minutes ago, this just means that we
> > need to wait for other people's opinions. Please do not send this
> > pull-request with the LED patches until this is resolved.
> >  
> 
> Yeah, I just asked Roderick to see if he can revert those patches
> while keeping the functionality behind those. I am more concerned
> about the micmute button, because we should really offer that feature
> to users. The associated LED class has no real benefits for now, so
> that code needs a little bit of care instead of a plain revert.

Thank you.

Marek