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

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

[Roderick Colenbrander]

Hi,

This is a resubmit of the previous patch series with a few changes:
- Added LED_FUNCTION_PLAYER1 to LED_FUNCTION_PLAYER5 defines.
- Removed LED_FUNCTION_PLAYER define.
- Changed ps_led_register use led_classdev->brightness_set_blocking.

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.

 Documentation/leds/well-known-leds.txt |  14 +++
 drivers/hid/hid-playstation.c          | 157 ++++++++++++++++++++++++-
 include/dt-bindings/leds/common.h      |   7 ++
 3 files changed, 177 insertions(+), 1 deletion(-)

-- 
2.31.1

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

[Roderick Colenbrander]

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 v3 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Roderick Colenbrander]

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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.

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>
---
 Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
 include/dt-bindings/leds/common.h      |  7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt
index 4a8b9dc4bf52..2160382c86be 100644
--- a/Documentation/leds/well-known-leds.txt
+++ b/Documentation/leds/well-known-leds.txt
@@ -16,6 +16,20 @@ but then try the legacy ones, too.
 
 Notice there's a list of functions in include/dt-bindings/leds/common.h .
 
+* Gamepads and joysticks
+
+Game controllers may feature LEDs to indicate a player number. This is commonly
+used on game consoles in which multiple controllers can be connected to a system.
+The "player LEDs" are then programmed with a pattern to indicate a particular
+player. For example, a game controller with 4 LEDs, may be programmed with "x---"
+to indicate player 1, "-x--" to indicate player 2 etcetera where "x" means on.
+Input drivers can utilize the LED class to expose the individual player LEDs
+of a game controller using the function "player".
+Note: tracking and management of Player IDs is the responsibility of user space,
+though drivers may pick a default value.
+
+Good: "input*:*:player-{1,2,3,4,5}
+
 * Keyboards
   
 Good: "input*:*:capslock"
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 52b619d44ba2..3be89a7c20a9 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -60,6 +60,13 @@
 #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_PLAYER1 "player-1"
+#define LED_FUNCTION_PLAYER2 "player-2"
+#define LED_FUNCTION_PLAYER3 "player-3"
+#define LED_FUNCTION_PLAYER4 "player-4"
+#define LED_FUNCTION_PLAYER5 "player-5"
+
 /* Miscelleaus functions. Use functions above if you can. */
 #define LED_FUNCTION_ACTIVITY "activity"
 #define LED_FUNCTION_ALARM "alarm"
-- 
2.31.1

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

[Roderick Colenbrander]

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..5cdfa71d1563 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);
+	int (*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_blocking = 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 int 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_PLAYER1, "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER2, "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER3, "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER4, "white", dualsense_player_led_get_brightness,
+				dualsense_player_led_set_brightness },
+		{ LED_FUNCTION_PLAYER5, "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 v3 2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

[Jiri Kosina]

On Wed, 8 Sep 2021, Roderick Colenbrander wrote:

> 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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.
> 
> 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>
> ---
>  Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
>  include/dt-bindings/leds/common.h      |  7 +++++++
>  2 files changed, 21 insertions(+)

Pavel, could you please eventually Ack this, so that I can take it 
together with the rest?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Pavel Machek]

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

Hi!

> > 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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.
> > 
> > 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>
> > ---
> >  Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
> >  include/dt-bindings/leds/common.h      |  7 +++++++
> >  2 files changed, 21 insertions(+)
> 
> Pavel, could you please eventually Ack this, so that I can take it 
> together with the rest?

I'm willing to take Documentation/leds/well-known-leds.txt part
through LED tree.

I don't like the common.h change; either avoid the define or put it
into your local header.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Roderick Colenbrander]

Hi Pavel,

On Mon, Sep 27, 2021 at 7:12 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > 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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.
> > >
> > > 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>
> > > ---
> > >  Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
> > >  include/dt-bindings/leds/common.h      |  7 +++++++
> > >  2 files changed, 21 insertions(+)
> >
> > Pavel, could you please eventually Ack this, so that I can take it
> > together with the rest?
>
> I'm willing to take Documentation/leds/well-known-leds.txt part
> through LED tree.
>
> I don't like the common.h change; either avoid the define or put it
> into your local header.

If the LED_FUNCTION_PLAYER* defines don't belong in common with the
other LED_FUNCTION* ones, where should it go? The hid-nintendo driver
intends to use the same defines, so defining it local to each driver
isn't right. Not sure if there is a great place in the input system
either (you would then have to move scrolllock and all those other LED
definitions too.)

Thanks,
Roderick


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

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

[Jiri Kosina]

On Mon, 27 Sep 2021, Roderick Colenbrander wrote:

> > > > 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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.
> > > >
> > > > 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>
> > > > ---
> > > >  Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
> > > >  include/dt-bindings/leds/common.h      |  7 +++++++
> > > >  2 files changed, 21 insertions(+)
> > >
> > > Pavel, could you please eventually Ack this, so that I can take it
> > > together with the rest?
> >
> > I'm willing to take Documentation/leds/well-known-leds.txt part
> > through LED tree.
> >
> > I don't like the common.h change; either avoid the define or put it
> > into your local header.
> 
> If the LED_FUNCTION_PLAYER* defines don't belong in common with the
> other LED_FUNCTION* ones, where should it go? The hid-nintendo driver
> intends to use the same defines, so defining it local to each driver
> isn't right. Not sure if there is a great place in the input system
> either (you would then have to move scrolllock and all those other LED
> definitions too.)

Pavel, ping please? This has been lingering really for a bit too long.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Pavel Machek]

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

Hi!

> > > > 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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.
> > > >
> > > > 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>
> > > > ---
> > > >  Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
> > > >  include/dt-bindings/leds/common.h      |  7 +++++++
> > > >  2 files changed, 21 insertions(+)
> > >
> > > Pavel, could you please eventually Ack this, so that I can take it
> > > together with the rest?
> >
> > I'm willing to take Documentation/leds/well-known-leds.txt part
> > through LED tree.
> >
> > I don't like the common.h change; either avoid the define or put it
> > into your local header.
> 
> If the LED_FUNCTION_PLAYER* defines don't belong in common with the
> other LED_FUNCTION* ones, where should it go? The hid-nintendo driver
> intends to use the same defines, so defining it local to each driver
> isn't right. Not sure if there is a great place in the input system
> either (you would then have to move scrolllock and all those other LED
> definitions too.)

Ok, so let's put it in the common place. I'll take this patch through
LED tree if you resubmit it. You still may want to use local defines
so you can apply the other patches without waiting.

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

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

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

[Roderick Colenbrander]

On Wed, Oct 13, 2021 at 12:48 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > > 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 LED_FUNCTION_PLAYER1-5 defines 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_PLAYERx.
> > > > >
> > > > > 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>
> > > > > ---
> > > > >  Documentation/leds/well-known-leds.txt | 14 ++++++++++++++
> > > > >  include/dt-bindings/leds/common.h      |  7 +++++++
> > > > >  2 files changed, 21 insertions(+)
> > > >
> > > > Pavel, could you please eventually Ack this, so that I can take it
> > > > together with the rest?
> > >
> > > I'm willing to take Documentation/leds/well-known-leds.txt part
> > > through LED tree.
> > >
> > > I don't like the common.h change; either avoid the define or put it
> > > into your local header.
> >
> > If the LED_FUNCTION_PLAYER* defines don't belong in common with the
> > other LED_FUNCTION* ones, where should it go? The hid-nintendo driver
> > intends to use the same defines, so defining it local to each driver
> > isn't right. Not sure if there is a great place in the input system
> > either (you would then have to move scrolllock and all those other LED
> > definitions too.)
>
> Ok, so let's put it in the common place. I'll take this patch through
> LED tree if you resubmit it. You still may want to use local defines
> so you can apply the other patches without waiting.
>

Thanks for your reply. If we want to decouple the series between LEDs
and HID, I don't mind. Not sure what is preferred by Benjamin/Jiri
either a temporary local define (#ifndef LED_FUNCTION_PLAYER1 #define
LED_FUNCTION_PLAYER1 "player-1"..) or just temporary hard coding the
string. Neither is that nice. It is only a few lines of code, so since
defines are the long-term way maybe a local define is okay.

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

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

[Jiri Kosina]

On Wed, 13 Oct 2021, Pavel Machek wrote:

> Ok, so let's put it in the common place. I'll take this patch through
> LED tree if you resubmit it. You still may want to use local defines
> so you can apply the other patches without waiting.

Pavel, why complicate it so much? Given how trivial the patch is, the 
easiest way is what's usually done in such cases (where substantial patch 
depends on a tiny trivial change elsewhere) -- take it through HID tree 
with your Reviewed-by / Acked-by:.

Do you see any issue with that?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Mon, 18 Oct 2021, Jiri Kosina wrote:

> > Ok, so let's put it in the common place. I'll take this patch through
> > LED tree if you resubmit it. You still may want to use local defines
> > so you can apply the other patches without waiting.
> 
> Pavel, why complicate it so much? Given how trivial the patch is, the 
> easiest way is what's usually done in such cases (where substantial patch 
> depends on a tiny trivial change elsewhere) -- take it through HID tree 
> with your Reviewed-by / Acked-by:.
> 
> Do you see any issue with that?

Pavel, another week has passed. I am considering just including the 
trivial LED #define additions and take them through hid.git unless I hear 
from you today.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Pavel Machek]

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

On Fri 2021-10-22 08:42:06, Jiri Kosina wrote:
> On Mon, 18 Oct 2021, Jiri Kosina wrote:
> 
> > > Ok, so let's put it in the common place. I'll take this patch through
> > > LED tree if you resubmit it. You still may want to use local defines
> > > so you can apply the other patches without waiting.
> > 
> > Pavel, why complicate it so much? Given how trivial the patch is, the 
> > easiest way is what's usually done in such cases (where substantial patch 
> > depends on a tiny trivial change elsewhere) -- take it through HID tree 
> > with your Reviewed-by / Acked-by:.
> > 
> > Do you see any issue with that?
> 
> Pavel, another week has passed. I am considering just including the 
> trivial LED #define additions and take them through hid.git unless I hear 
> from you today.

I'd prefer not to deal with rejects / common immutable branches / etc.

You don't _need_ the defines at all, and you don't need them in the
common place. Just merge the patch without the defines. I'll merge the
defines. That seems like least complex solution to me.
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

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

[Jiri Kosina]

On Fri, 22 Oct 2021, Pavel Machek wrote:

> > > > Ok, so let's put it in the common place. I'll take this patch through
> > > > LED tree if you resubmit it. You still may want to use local defines
> > > > so you can apply the other patches without waiting.
> > > 
> > > Pavel, why complicate it so much? Given how trivial the patch is, the 
> > > easiest way is what's usually done in such cases (where substantial patch 
> > > depends on a tiny trivial change elsewhere) -- take it through HID tree 
> > > with your Reviewed-by / Acked-by:.
> > > 
> > > Do you see any issue with that?
> > 
> > Pavel, another week has passed. I am considering just including the 
> > trivial LED #define additions and take them through hid.git unless I hear 
> > from you today.
> 
> I'd prefer not to deal with rejects / common immutable branches / etc.

I am not proposing common immutable branch; and if there are going to be 
trivial cotext conflicts because of that, those will be sorted out by 
Linus without you even noticing.

> You don't _need_ the defines at all

As I've already pointed to you in several threads, we have quite a lot of 
code queued that does depend on the defines.

> and you don't need them in the common place.

I compltely fail to see the point of having them teporarily local before 
you manage to finally do something about the trivial addition to proper 
shared header.

> Just merge the patch without the defines. I'll merge the defines. That 
> seems like least complex solution to me.

That would cause my tree not to build.

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Fri, 22 Oct 2021, Jiri Kosina wrote:

> > > > > Ok, so let's put it in the common place. I'll take this patch through
> > > > > LED tree if you resubmit it. You still may want to use local defines
> > > > > so you can apply the other patches without waiting.
> > > > 
> > > > Pavel, why complicate it so much? Given how trivial the patch is, the 
> > > > easiest way is what's usually done in such cases (where substantial patch 
> > > > depends on a tiny trivial change elsewhere) -- take it through HID tree 
> > > > with your Reviewed-by / Acked-by:.
> > > > 
> > > > Do you see any issue with that?
> > > 
> > > Pavel, another week has passed. I am considering just including the 
> > > trivial LED #define additions and take them through hid.git unless I hear 
> > > from you today.
> > 
> > I'd prefer not to deal with rejects / common immutable branches / etc.
> 
> I am not proposing common immutable branch; and if there are going to be 
> trivial cotext conflicts because of that, those will be sorted out by 
> Linus without you even noticing.
> 
> > You don't _need_ the defines at all
> 
> As I've already pointed to you in several threads, we have quite a lot of 
> code queued that does depend on the defines.
> 
> > and you don't need them in the common place.
> 
> I compltely fail to see the point of having them teporarily local before 
> you manage to finally do something about the trivial addition to proper 
> shared header.
> 
> > Just merge the patch without the defines. I'll merge the defines. That 
> > seems like least complex solution to me.
> 
> That would cause my tree not to build.

In other words: could you please elaborate what exact issue are you trying 
to avoid by not providing your Acked-by: and letting it go through hid.git 
with all the rest of the code depending on it?

-- 
Jiri Kosina
SUSE Labs

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

[Pavel Machek]

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

Hi!

> > > > Pavel, another week has passed. I am considering just including the 
> > > > trivial LED #define additions and take them through hid.git unless I hear 
> > > > from you today.
> > > 
> > > I'd prefer not to deal with rejects / common immutable branches / etc.
> > 
> > I am not proposing common immutable branch; and if there are going to be 
> > trivial cotext conflicts because of that, those will be sorted out by 
> > Linus without you even noticing.
> > 
> > > You don't _need_ the defines at all
> > 
> > As I've already pointed to you in several threads, we have quite a lot of 
> > code queued that does depend on the defines.
> > 
> > > and you don't need them in the common place.
> > 
> > I compltely fail to see the point of having them teporarily local before 
> > you manage to finally do something about the trivial addition to proper 
> > shared header.
> > 
> > > Just merge the patch without the defines. I'll merge the defines. That 
> > > seems like least complex solution to me.
> > 
> > That would cause my tree not to build.
> 
> In other words: could you please elaborate what exact issue are you trying 
> to avoid by not providing your Acked-by: and letting it go through hid.git 
> with all the rest of the code depending on it?

I'm trying to avoid merge conflict.

I believe open-coding string for a while is acceptable price to pay
for that, and I'd prefer that solution.

If you can promise that no conflicts or other problems will happen for
either me or Linus... go ahead and merge the patch.

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

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

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

[Jiri Kosina]

On Mon, 25 Oct 2021, Pavel Machek wrote:

> > In other words: could you please elaborate what exact issue are you trying 
> > to avoid by not providing your Acked-by: and letting it go through hid.git 
> > with all the rest of the code depending on it?
> 
> I'm trying to avoid merge conflict.

What's wrong with this kind of conflict?

That's what linux-next is for; if there is a conflict, we'll be notified 
and if needed and we could indicate this to Linus during merge window. The 
trivial ones he resolves without any issues. And we'll know exactly what 
kind of conflict (if any at all) is there beforehand from linux-next.

> I believe open-coding string for a while is acceptable price to pay for 
> that, and I'd prefer that solution.

I don't. It's a mess. If you'd then for some reason change your mind on 
the last minute and did the numbering differently in your tree, it will go 
by unnoticed, while when the real conflict happens, it'll be a clear sign 
that there is a thing to resolve.

Conflict is not a bad thing per se that needs to be avoided at all costs.

Conflict clearly shows the dependency between the trees and is trivially 
resolved.

> If you can promise that no conflicts or other problems will happen for 
> either me or Linus... 

Linus doesn't care about this kind of hypothetical conflict if there is a 
reason for it, and he resolves them on a daily basis, just check the git 
history.

> go ahead and merge the patch.

Can I take this as your Acked-by: then, so that I can finally apply it and 
get the needed linux-next coverage before merge window opens?

-- 
Jiri Kosina
SUSE Labs

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

[Pavel Machek]

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

Hi!

> > go ahead and merge the patch.
> 
> Can I take this as your Acked-by: then, so that I can finally apply it and 
> get the needed linux-next coverage before merge window opens?

Yes.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Jiri Kosina]

On Tue, 26 Oct 2021, Pavel Machek wrote:

> > Can I take this as your Acked-by: then, so that I can finally apply it and 
> > get the needed linux-next coverage before merge window opens?
> 
> Yes.

Thanks. The whole series is now in hid.git#for-5.16/playstation

-- 
Jiri Kosina
SUSE Labs