diff mbox series

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

Message ID 20210901223037.2964665-4-roderick.colenbrander@sony.com
State Superseded
Headers show
Series HID: playstation: add LED support | expand

Commit Message

Roderick Colenbrander Sept. 1, 2021, 10:30 p.m. UTC
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(-)

Comments

Pavel Machek Sept. 3, 2021, 4:17 p.m. UTC | #1
Hi!

> 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.
> 
>  
> +struct ps_led_info {
...
> +	enum led_brightness (*brightness_get)(struct led_classdev *cdev);
> +	void (*brightness_set)(struct led_classdev *cdev, enum led_brightness);
> +};

Do you need these fields? They are constant in the driver...

> +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));
> +}

You should not need to implement get if all it does is returning cached state.

> +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);
> +}

LED core can handle scheduling the work for you. You should use it, in similar
way you handle the RGB led.

Best regards,
									Pavel
Roderick Colenbrander Sept. 7, 2021, 5:28 a.m. UTC | #2
Hi Pavel,

On Fri, Sep 3, 2021 at 9:25 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > 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.
> >
> >
> > +struct ps_led_info {
> ...
> > +     enum led_brightness (*brightness_get)(struct led_classdev *cdev);
> > +     void (*brightness_set)(struct led_classdev *cdev, enum led_brightness);
> > +};
>
> Do you need these fields? They are constant in the driver...

Currently they are constant, but over time support for some other
devices (e.g. DualShock 4) will be moved into this driver as well and
then these are needed.

>
> > +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));
> > +}
>
> You should not need to implement get if all it does is returning cached state.

The player LEDs are configured at driver loading time with an initial
value by calling 'dualsense_set_player_leds'. This is done behind the
back of the LED framework, so it is driver internal state. The problem
is that we need to set multiple LEDs at once at driver startup and
they share a common HID output report. The LED framework has no 'group
LED support' (outside the new multicolor class), so there is no way to
really synchronize multiple LEDs right now.

> > +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);
> > +}
>
> LED core can handle scheduling the work for you. You should use it, in similar
> way you handle the RGB led.

I'm not entirely sure what you meant. I guess you mean I should use
'cdev->brightness_set_blocking' instead of 'cdev->brightness_set' when
setting up the LED, so LED core can do more things itself? That's the
main difference I saw between my RGB and regular LED code. Both rely
on updating the internal dualsense structure and calling schedule_work
to schedule a HID output report (the output report contains data for
many things outside of just the LEDs). Is that indeed what you meant
or did I miss something (it is getting late here)?

>
> Best regards,
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Regards,
Roderick
diff mbox series

Patch

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);