[PATCH 0/3] HID: playstation: add LED support.

[PATCH 0/3] HID: playstation: add LED support.

[Roderick Colenbrander]

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

Hi,

Earlier this year Sony published a new driver 'hid-playstation' to support
our new DualSense controller. During the review process, the maintainers
of the LED subsystem expressed that they wanted to be more strict about LED
naming moving forward. There was little time to make the necessary changes
for Linux 5.12 at the time, so LED support was taken out for Linux 5.12.

This patch set adds support for the DualSense lightbar and player LEDs
based on previous patches, but using a new naming scheme.

The DualSense lightbar is exposed using a multi-color LED and is named
after the 'main' input device and uses a name like "inputX:rgb:indicator".

The player LEDs didn't have a suitable "function" type defined by the LED
subsystem. This patch series introduces a new "LED_FUNCTION_PLAYER" type
for these LEDs. This type is then used for 'hid-playstation' and long-term
will be used by 'hid-nintendo' as well. The overall naming takes the form:
"inputX:white:player-1" for the first player LED on a DualSense controller.

Regards,

Roderick Colenbrander
Sony Interactive Entertainment, LLC

Roderick Colenbrander (3):
  HID: playstation: expose DualSense lightbar through a multi-color LED.
  leds: add new LED_FUNCTION_PLAYER for player LEDs for game
    controllers.
  HID: playstation: expose DualSense player LEDs through LED class.

 drivers/hid/hid-playstation.c     | 157 +++++++++++++++++++++++++++++-
 include/dt-bindings/leds/common.h |   3 +
 2 files changed, 159 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/3] HID: playstation: expose DualSense lightbar through a multi-color LED.

[Roderick Colenbrander]

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

The DualSense lightbar has so far been supported, but it was not yet
adjustable from user space. This patch exposes it through a multi-color
LED.

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

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ab7c82c2e886..ff2fc315a89d 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -11,6 +11,8 @@
 #include <linux/hid.h>
 #include <linux/idr.h>
 #include <linux/input/mt.h>
+#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
 #include <linux/module.h>
 
 #include <asm/unaligned.h>
@@ -38,6 +40,7 @@ struct ps_device {
 	uint8_t battery_capacity;
 	int battery_status;
 
+	const char *input_dev_name; /* Name of primary input device. */
 	uint8_t mac_address[6]; /* Note: stored in little endian order. */
 	uint32_t hw_version;
 	uint32_t fw_version;
@@ -147,6 +150,7 @@ struct dualsense {
 	uint8_t motor_right;
 
 	/* RGB lightbar */
+	struct led_classdev_mc lightbar;
 	bool update_lightbar;
 	uint8_t lightbar_red;
 	uint8_t lightbar_green;
@@ -288,6 +292,8 @@ static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
 	{0, 0},
 };
 
+static void dualsense_set_lightbar(struct dualsense *ds, uint8_t red, uint8_t green, uint8_t blue);
+
 /*
  * Add a new ps_device to ps_devices if it doesn't exist.
  * Return error on duplicate device, which can happen if the same
@@ -525,6 +531,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, "%s:rgb:indicator",
+			ps_dev->input_dev_name);
+	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)
 {
@@ -761,6 +806,22 @@ 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);
+	uint8_t red, green, blue;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+	red = mc_cdev->subled_info[0].brightness;
+	green = mc_cdev->subled_info[1].brightness;
+	blue = mc_cdev->subled_info[2].brightness;
+
+	dualsense_set_lightbar(ds, red, green, blue);
+	return 0;
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -1106,10 +1167,14 @@ static int dualsense_reset_leds(struct dualsense *ds)
 
 static void dualsense_set_lightbar(struct dualsense *ds, uint8_t red, uint8_t green, uint8_t blue)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&ds->base.lock, flags);
 	ds->update_lightbar = true;
 	ds->lightbar_red = red;
 	ds->lightbar_green = green;
 	ds->lightbar_blue = blue;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
 
 	schedule_work(&ds->output_worker);
 }
@@ -1196,6 +1261,8 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		ret = PTR_ERR(ds->gamepad);
 		goto err;
 	}
+	/* Use gamepad input device name as primary device name for e.g. LEDs */
+	ps_dev->input_dev_name = dev_name(&ds->gamepad->dev);
 
 	ds->sensors = ps_sensors_create(hdev, DS_ACC_RANGE, DS_ACC_RES_PER_G,
 			DS_GYRO_RANGE, DS_GYRO_RES_PER_DEG_S);
@@ -1223,6 +1290,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret)
 		goto err;
 
+	ret = ps_lightbar_register(ps_dev, &ds->lightbar, dualsense_lightbar_set_brightness);
+	if (ret)
+		goto err;
+
+	/* Set default lightbar color. */
 	dualsense_set_lightbar(ds, 0, 0, 128); /* blue */
 
 	ret = ps_device_set_player_id(ps_dev);
-- 
2.31.1

[PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Roderick Colenbrander]

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

Player LEDs are commonly found on game controllers from Nintendo and Sony
to indicate a player ID across a number of LEDs. For example, "Player 2"
might be indicated as "-x--" on a device with 4 LEDs where "x" means on.

This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
player LEDs from the kernel. Until now there was no good standard, which
resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.

Note: management of Player IDs is left to user space, though a kernel
driver may pick a default value.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 include/dt-bindings/leds/common.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 52b619d44ba2..94999c250e4d 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -60,6 +60,9 @@
 #define LED_FUNCTION_MICMUTE "micmute"
 #define LED_FUNCTION_MUTE "mute"
 
+/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
+#define LED_FUNCTION_PLAYER "player"
+
 /* Miscelleaus functions. Use functions above if you can. */
 #define LED_FUNCTION_ACTIVITY "activity"
 #define LED_FUNCTION_ALARM "alarm"
-- 
2.31.1

[PATCH 3/3] HID: playstation: expose DualSense player LEDs through LED class.

[Roderick Colenbrander]

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

The DualSense player LEDs were so far not adjustable from user-space.
This patch exposes each LED individually through the LED class. Each
LED uses the new 'player' function resulting in a name like:
'inputX:white:player-1' for the first LED.

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

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ff2fc315a89d..9b96239bba5f 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -56,6 +56,13 @@ struct ps_calibration_data {
 	int sens_denom;
 };
 
+struct ps_led_info {
+	const char *name;
+	const char *color;
+	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
@@ -531,6 +538,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,
+			"%s:%s:%s", ps_dev->input_dev_name, led_info->color, 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))
@@ -822,6 +855,35 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
 	return 0;
 }
 
+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)
 {
@@ -1207,7 +1269,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 player_leds_info[] = {
+		{ LED_FUNCTION_PLAYER "-1", "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER "-2", "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER "-3", "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER "-4", "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER "-5", "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness }
+	};
 
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
 	if (!ds)
@@ -1297,6 +1372,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	/* Set default lightbar color. */
 	dualsense_set_lightbar(ds, 0, 0, 128); /* blue */
 
+	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;
+	}
+
 	ret = ps_device_set_player_id(ps_dev);
 	if (ret) {
 		hid_err(hdev, "Failed to assign player id for DualSense: %d\n", ret);
-- 
2.31.1

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Jiri Kosina]

On Tue, 1 Jun 2021, Roderick Colenbrander wrote:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Player LEDs are commonly found on game controllers from Nintendo and Sony
> to indicate a player ID across a number of LEDs. For example, "Player 2"
> might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> 
> This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> player LEDs from the kernel. Until now there was no good standard, which
> resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> 
> Note: management of Player IDs is left to user space, though a kernel
> driver may pick a default value.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  include/dt-bindings/leds/common.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> index 52b619d44ba2..94999c250e4d 100644
> --- a/include/dt-bindings/leds/common.h
> +++ b/include/dt-bindings/leds/common.h
> @@ -60,6 +60,9 @@
>  #define LED_FUNCTION_MICMUTE "micmute"
>  #define LED_FUNCTION_MUTE "mute"
>  
> +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> +#define LED_FUNCTION_PLAYER "player"
> +
>  /* Miscelleaus functions. Use functions above if you can. */
>  #define LED_FUNCTION_ACTIVITY "activity"
>  #define LED_FUNCTION_ALARM "alarm"

Pavel, can I please get your Ack on this one, so that I can take it with 
the rest of the series?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Roderick Colenbrander]

Hi Pavel,

Any feedback on this patch, which introduces a new player led type,
which is common on game controllers?

Thanks,
Roderick Colenbrander

On Thu, Jun 24, 2021 at 6:26 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Tue, 1 Jun 2021, Roderick Colenbrander wrote:
>
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> >
> > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > player LEDs from the kernel. Until now there was no good standard, which
> > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> >
> > Note: management of Player IDs is left to user space, though a kernel
> > driver may pick a default value.
> >
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > ---
> >  include/dt-bindings/leds/common.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > index 52b619d44ba2..94999c250e4d 100644
> > --- a/include/dt-bindings/leds/common.h
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -60,6 +60,9 @@
> >  #define LED_FUNCTION_MICMUTE "micmute"
> >  #define LED_FUNCTION_MUTE "mute"
> >
> > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > +#define LED_FUNCTION_PLAYER "player"
> > +
> >  /* Miscelleaus functions. Use functions above if you can. */
> >  #define LED_FUNCTION_ACTIVITY "activity"
> >  #define LED_FUNCTION_ALARM "alarm"
>
> Pavel, can I please get your Ack on this one, so that I can take it with
> the rest of the series?
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Roderick Colenbrander]

Hi Pavel,

Any update on this topic?

Thanks,
Roderick

On Mon, Jul 5, 2021 at 9:51 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> Hi Pavel,
>
> Any feedback on this patch, which introduces a new player led type,
> which is common on game controllers?
>
> Thanks,
> Roderick Colenbrander
>
> On Thu, Jun 24, 2021 at 6:26 AM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Tue, 1 Jun 2021, Roderick Colenbrander wrote:
> >
> > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > >
> > > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> > >
> > > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > > player LEDs from the kernel. Until now there was no good standard, which
> > > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> > >
> > > Note: management of Player IDs is left to user space, though a kernel
> > > driver may pick a default value.
> > >
> > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > ---
> > >  include/dt-bindings/leds/common.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > > index 52b619d44ba2..94999c250e4d 100644
> > > --- a/include/dt-bindings/leds/common.h
> > > +++ b/include/dt-bindings/leds/common.h
> > > @@ -60,6 +60,9 @@
> > >  #define LED_FUNCTION_MICMUTE "micmute"
> > >  #define LED_FUNCTION_MUTE "mute"
> > >
> > > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > > +#define LED_FUNCTION_PLAYER "player"
> > > +
> > >  /* Miscelleaus functions. Use functions above if you can. */
> > >  #define LED_FUNCTION_ACTIVITY "activity"
> > >  #define LED_FUNCTION_ALARM "alarm"
> >
> > Pavel, can I please get your Ack on this one, so that I can take it with
> > the rest of the series?
> >
> > Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]

Hi!

> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > 
> > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> > 
> > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > player LEDs from the kernel. Until now there was no good standard, which
> > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> > 
> > Note: management of Player IDs is left to user space, though a kernel
> > driver may pick a default value.
> > 
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > ---
> >  include/dt-bindings/leds/common.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > index 52b619d44ba2..94999c250e4d 100644
> > --- a/include/dt-bindings/leds/common.h
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -60,6 +60,9 @@
> >  #define LED_FUNCTION_MICMUTE "micmute"
> >  #define LED_FUNCTION_MUTE "mute"
> >  
> > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > +#define LED_FUNCTION_PLAYER "player"
> > +
> >  /* Miscelleaus functions. Use functions above if you can. */
> >  #define LED_FUNCTION_ACTIVITY "activity"
> >  #define LED_FUNCTION_ALARM "alarm"
> 
> Pavel, can I please get your Ack on this one, so that I can take it with 
> the rest of the series?

I'm sorry for delays.

But no, player is not suitable function. Function would be "player1"
AFAICT, right?

I'm not sure "function" is suitable here, and we may want to create
documentation like this... where it would be explained which functions
apply to which devices and what they actually mean.

Best regards,
								Pavel

-*- org -*-

It is somehow important to provide consistent interface to the
userland. LED devices have one problem there, and that is naming of
directories in /sys/class/leds. It would be nice if userland would
just know right "name" for given LED function, but situation got more
complex.

Anyway, if backwards compatibility is not an issue, new code should
use one of the "good" names from this list, and you should extend the
list where applicable.

Bad names are listed, too; in case you are writing application that
wants to use particular feature, you should probe for good name, first,
but then try the bad ones, too.

* Keyboards
  
Good: "input*:*:capslock"
Good: "input*:*:scrolllock"
Good: "input*:*:numlock"
Bad: "shift-key-light" (Motorola Droid 4, capslock)

Set of common keyboard LEDs, going back to PC AT or so.

Good: "platform::kbd_backlight"
Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)

Frontlight/backlight of main keyboard.

Bad: "button-backlight" (Motorola Droid 4)

Some phones have touch buttons below screen; it is different from main
keyboard. And this is their backlight.

* Sound subsystem

Good: "platform:*:mute"
Good: "platform:*:micmute"

LEDs on notebook body, indicating that sound input / output is muted.

* System notification

Good: "status-led:{red,green,blue}" (Motorola Droid 4)
Bad: "lp5523:{r,g,b}" (Nokia N900)

Phones usually have multi-color status LED.

* Power management

Good: "platform:*:charging" (allwinner sun50i)

* Screen

Good: ":backlight" (Motorola Droid 4)


-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Roderick Colenbrander]

Hi Pavel,

See some quick comments inline.

On Tue, Aug 3, 2021 at 3:39 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > >
> > > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> > >
> > > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > > player LEDs from the kernel. Until now there was no good standard, which
> > > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> > >
> > > Note: management of Player IDs is left to user space, though a kernel
> > > driver may pick a default value.
> > >
> > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > ---
> > >  include/dt-bindings/leds/common.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > > index 52b619d44ba2..94999c250e4d 100644
> > > --- a/include/dt-bindings/leds/common.h
> > > +++ b/include/dt-bindings/leds/common.h
> > > @@ -60,6 +60,9 @@
> > >  #define LED_FUNCTION_MICMUTE "micmute"
> > >  #define LED_FUNCTION_MUTE "mute"
> > >
> > > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > > +#define LED_FUNCTION_PLAYER "player"
> > > +
> > >  /* Miscelleaus functions. Use functions above if you can. */
> > >  #define LED_FUNCTION_ACTIVITY "activity"
> > >  #define LED_FUNCTION_ALARM "alarm"
> >
> > Pavel, can I please get your Ack on this one, so that I can take it with
> > the rest of the series?
>
> I'm sorry for delays.
>
> But no, player is not suitable function. Function would be "player1"
> AFAICT, right?

A given gaming device such as Sony or Nintendo controllers have a
multiple of these LEDs, which are meant to be configured with a player
number. A typical device has like 4 of these LEDs, so a single
controller would have: "player-1", "player-2", "player-3" and
"player-4". It is up to userspace to configure the player number (a
driver may set a default number across a number of LEDs).

If player is not the right term (many people know it), what would it be?

>
> I'm not sure "function" is suitable here, and we may want to create
> documentation like this... where it would be explained which functions
> apply to which devices and what they actually mean.
>
> Best regards,
>                                                                 Pavel
>
> -*- org -*-
>
> It is somehow important to provide consistent interface to the
> userland. LED devices have one problem there, and that is naming of
> directories in /sys/class/leds. It would be nice if userland would
> just know right "name" for given LED function, but situation got more
> complex.
>
> Anyway, if backwards compatibility is not an issue, new code should
> use one of the "good" names from this list, and you should extend the
> list where applicable.
>
> Bad names are listed, too; in case you are writing application that
> wants to use particular feature, you should probe for good name, first,
> but then try the bad ones, too.
>
> * Keyboards
>
> Good: "input*:*:capslock"
> Good: "input*:*:scrolllock"
> Good: "input*:*:numlock"
> Bad: "shift-key-light" (Motorola Droid 4, capslock)
>
> Set of common keyboard LEDs, going back to PC AT or so.
>
> Good: "platform::kbd_backlight"
> Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
> Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)
>
> Frontlight/backlight of main keyboard.
>
> Bad: "button-backlight" (Motorola Droid 4)
>
> Some phones have touch buttons below screen; it is different from main
> keyboard. And this is their backlight.
>
> * Sound subsystem
>
> Good: "platform:*:mute"
> Good: "platform:*:micmute"
>
> LEDs on notebook body, indicating that sound input / output is muted.
>
> * System notification
>
> Good: "status-led:{red,green,blue}" (Motorola Droid 4)
> Bad: "lp5523:{r,g,b}" (Nokia N900)
>
> Phones usually have multi-color status LED.
>
> * Power management
>
> Good: "platform:*:charging" (allwinner sun50i)
>
> * Screen
>
> Good: ":backlight" (Motorola Droid 4)
>
>
> --
> http://www.livejournal.com/~pavelmachek

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Daniel Ogorchock]

Hi Pavel,

Do you have any recommendations on what would be an appropriate
function string for player indicator LEDs? Would some variant such as:
  "status-x"
  "player-status-x"
  "indicator-x"
  "player-indicator-x"
be more suitable? It looks like the string "status" has been used for
other existing LED names.

I think we are pretty happy to use whatever naming scheme fits the
standards of the led subsystem's userspace api for the Nintendo/Sony
HID drivers, and any future game controller drivers featuring player
LEDs could conform to that going forward.

Thanks,
Daniel

On Tue, Aug 3, 2021 at 7:30 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> Hi Pavel,
>
> See some quick comments inline.
>
> On Tue, Aug 3, 2021 at 3:39 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > >
> > > > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > > > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > > > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> > > >
> > > > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > > > player LEDs from the kernel. Until now there was no good standard, which
> > > > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > > > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> > > >
> > > > Note: management of Player IDs is left to user space, though a kernel
> > > > driver may pick a default value.
> > > >
> > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > ---
> > > >  include/dt-bindings/leds/common.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > > > index 52b619d44ba2..94999c250e4d 100644
> > > > --- a/include/dt-bindings/leds/common.h
> > > > +++ b/include/dt-bindings/leds/common.h
> > > > @@ -60,6 +60,9 @@
> > > >  #define LED_FUNCTION_MICMUTE "micmute"
> > > >  #define LED_FUNCTION_MUTE "mute"
> > > >
> > > > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > > > +#define LED_FUNCTION_PLAYER "player"
> > > > +
> > > >  /* Miscelleaus functions. Use functions above if you can. */
> > > >  #define LED_FUNCTION_ACTIVITY "activity"
> > > >  #define LED_FUNCTION_ALARM "alarm"
> > >
> > > Pavel, can I please get your Ack on this one, so that I can take it with
> > > the rest of the series?
> >
> > I'm sorry for delays.
> >
> > But no, player is not suitable function. Function would be "player1"
> > AFAICT, right?
>
> A given gaming device such as Sony or Nintendo controllers have a
> multiple of these LEDs, which are meant to be configured with a player
> number. A typical device has like 4 of these LEDs, so a single
> controller would have: "player-1", "player-2", "player-3" and
> "player-4". It is up to userspace to configure the player number (a
> driver may set a default number across a number of LEDs).
>
> If player is not the right term (many people know it), what would it be?
>
> >
> > I'm not sure "function" is suitable here, and we may want to create
> > documentation like this... where it would be explained which functions
> > apply to which devices and what they actually mean.
> >
> > Best regards,
> >                                                                 Pavel
> >
> > -*- org -*-
> >
> > It is somehow important to provide consistent interface to the
> > userland. LED devices have one problem there, and that is naming of
> > directories in /sys/class/leds. It would be nice if userland would
> > just know right "name" for given LED function, but situation got more
> > complex.
> >
> > Anyway, if backwards compatibility is not an issue, new code should
> > use one of the "good" names from this list, and you should extend the
> > list where applicable.
> >
> > Bad names are listed, too; in case you are writing application that
> > wants to use particular feature, you should probe for good name, first,
> > but then try the bad ones, too.
> >
> > * Keyboards
> >
> > Good: "input*:*:capslock"
> > Good: "input*:*:scrolllock"
> > Good: "input*:*:numlock"
> > Bad: "shift-key-light" (Motorola Droid 4, capslock)
> >
> > Set of common keyboard LEDs, going back to PC AT or so.
> >
> > Good: "platform::kbd_backlight"
> > Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
> > Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)
> >
> > Frontlight/backlight of main keyboard.
> >
> > Bad: "button-backlight" (Motorola Droid 4)
> >
> > Some phones have touch buttons below screen; it is different from main
> > keyboard. And this is their backlight.
> >
> > * Sound subsystem
> >
> > Good: "platform:*:mute"
> > Good: "platform:*:micmute"
> >
> > LEDs on notebook body, indicating that sound input / output is muted.
> >
> > * System notification
> >
> > Good: "status-led:{red,green,blue}" (Motorola Droid 4)
> > Bad: "lp5523:{r,g,b}" (Nokia N900)
> >
> > Phones usually have multi-color status LED.
> >
> > * Power management
> >
> > Good: "platform:*:charging" (allwinner sun50i)
> >
> > * Screen
> >
> > Good: ":backlight" (Motorola Droid 4)
> >
> >
> > --
> > http://www.livejournal.com/~pavelmachek



-- 
Daniel Ogorchock

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Jiri Kosina]

On Fri, 13 Aug 2021, Daniel Ogorchock wrote:

> Hi Pavel,
> 
> Do you have any recommendations on what would be an appropriate
> function string for player indicator LEDs? Would some variant such as:
>   "status-x"
>   "player-status-x"
>   "indicator-x"
>   "player-indicator-x"
> be more suitable? It looks like the string "status" has been used for
> other existing LED names.
> 
> I think we are pretty happy to use whatever naming scheme fits the
> standards of the led subsystem's userspace api for the Nintendo/Sony
> HID drivers, and any future game controller drivers featuring player
> LEDs could conform to that going forward.

Pavel, could you please take a look here, so that we can proceed with the 
patchset?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

Hi!

> > Do you have any recommendations on what would be an appropriate
> > function string for player indicator LEDs? Would some variant such as:
> >   "status-x"
> >   "player-status-x"
> >   "indicator-x"
> >   "player-indicator-x"
> > be more suitable? It looks like the string "status" has been used for
> > other existing LED names.

I guess "player-x" would be suitable.

> > I think we are pretty happy to use whatever naming scheme fits the
> > standards of the led subsystem's userspace api for the Nintendo/Sony
> > HID drivers, and any future game controller drivers featuring player
> > LEDs could conform to that going forward.
> 
> Pavel, could you please take a look here, so that we can proceed with the 
> patchset?

So... leds tree has just been merged:

> git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/
tags/leds-5.15-rc1

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a998a62be9cdb509491731ffe81575aa09943a32

It includes Documentation/leds/well-known-leds.txt file. Could a
section describing proposed naming be added there (both device and
function), with explanations what the LEDs do?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Roderick Colenbrander]

Hi Pavel,

On Tue, Aug 31, 2021 at 10:19 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > Do you have any recommendations on what would be an appropriate
> > > function string for player indicator LEDs? Would some variant such as:
> > >   "status-x"
> > >   "player-status-x"
> > >   "indicator-x"
> > >   "player-indicator-x"
> > > be more suitable? It looks like the string "status" has been used for
> > > other existing LED names.
>
> I guess "player-x" would be suitable.
>
> > > I think we are pretty happy to use whatever naming scheme fits the
> > > standards of the led subsystem's userspace api for the Nintendo/Sony
> > > HID drivers, and any future game controller drivers featuring player
> > > LEDs could conform to that going forward.
> >
> > Pavel, could you please take a look here, so that we can proceed with the
> > patchset?
>
> So... leds tree has just been merged:
>
> > git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/
> tags/leds-5.15-rc1
>
> has been merged into torvalds/linux.git:
> https://git.kernel.org/torvalds/c/a998a62be9cdb509491731ffe81575aa09943a32
>
> It includes Documentation/leds/well-known-leds.txt file. Could a
> section describing proposed naming be added there (both device and
> function), with explanations what the LEDs do?
>

Sure let me write add a few lines for that file and resubmit. I guess
I should rebase based on Linus his tree then.. let me quickly start on
that. (I'm technically on vacation and far from home, but luckily
caught this and happen to have a break)

Thanks,
Roderick