diff mbox series

[11/13] HID: playstation: add DualSense player LEDs support.

Message ID 20201219062336.72568-12-roderick@gaikai.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: new driver for PS5 'DualSense' controller | expand

Commit Message

Roderick Colenbrander Dec. 19, 2020, 6:23 a.m. UTC 
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 | 64 ++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

Comments

Samuel Čavoj Dec. 26, 2020, 7:27 p.m. UTC | #1 
Hi,

I noticed that the `value` argument is not at all used in the
player_led_set_brightness function.

On 18.12.2020 22:23, Roderick Colenbrander wrote:
> +
> +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);
> +	uint8_t player_leds_state = 0;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> +		player_leds_state |= (ds->player_leds[i].brightness << i);

Is it guaranteed at this point, that the led->brightness is set to the
new value? I'm unfamiliar with the led subsystem, but skimming other
drivers I found that they update the device based on the value of the
`value` argument.

> +
> +	spin_lock_irqsave(&ds->base.lock, flags);
> +	ds->player_leds_state = player_leds_state;
> +	ds->update_player_leds = true;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +}

Reading led-core.c, I found that led_set_brightness_{nosleep,sync} do
set the brightness attribute, but the _nopm one does not and it is
exported, although it is not used anywhere other than led-core.c and
led-class.c.

However, I find the usage in led_classdev_suspend and _resume
interesting. In suspend, set_brightness_nopm is called with a value of
0, which should turn off the LED while retaining the value of the
brightness attribute, which is later recalled in _resume. I assume the
intended behaviour is the LED to turn off when suspending and return to
its original state on resume, without overwriting the attribute.

Assuming that, the "value" argument passed to dualsense_player_led_set_brightness
can be different from led->brightness *on purpose* and should be used
instead.

I would write something along these lines:

for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
	if (&ds->player_leds[i] == led) {
		if (value == LED_OFF)
			player_leds_state &= ~(1 << i);
		else
			player_leds_state |= (1 << i);
		break;
	}
}

This is just me hypothesizing though, could anyone clear this up for
me? Thank you very much.

Regards,
Samuel
Roderick Colenbrander Dec. 26, 2020, 11:07 p.m. UTC | #2 
Hi Samuel,

Thanks for your review!

On Sat, Dec 26, 2020 at 11:27 AM Samuel Čavoj <samuel@cavoj.net> wrote:
>
> Hi,
>
> I noticed that the `value` argument is not at all used in the
> player_led_set_brightness function.
>
> On 18.12.2020 22:23, Roderick Colenbrander wrote:
> > +
> > +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);
> > +     uint8_t player_leds_state = 0;
> > +     unsigned long flags;
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> > +             player_leds_state |= (ds->player_leds[i].brightness << i);
>
> Is it guaranteed at this point, that the led->brightness is set to the
> new value? I'm unfamiliar with the led subsystem, but skimming other
> drivers I found that they update the device based on the value of the
> `value` argument.

See comments below. Normally yes the brightness is already updated,
but as mentioned below it might still be best to change the code.

>
> > +
> > +     spin_lock_irqsave(&ds->base.lock, flags);
> > +     ds->player_leds_state = player_leds_state;
> > +     ds->update_player_leds = true;
> > +     spin_unlock_irqrestore(&ds->base.lock, flags);
> > +
> > +     schedule_work(&ds->output_worker);
> > +}
>
> Reading led-core.c, I found that led_set_brightness_{nosleep,sync} do
> set the brightness attribute, but the _nopm one does not and it is
> exported, although it is not used anywhere other than led-core.c and
> led-class.c.
>
> However, I find the usage in led_classdev_suspend and _resume
> interesting. In suspend, set_brightness_nopm is called with a value of
> 0, which should turn off the LED while retaining the value of the
> brightness attribute, which is later recalled in _resume. I assume the
> intended behaviour is the LED to turn off when suspending and return to
> its original state on resume, without overwriting the attribute.
>
> Assuming that, the "value" argument passed to dualsense_player_led_set_brightness
> can be different from led->brightness *on purpose* and should be used
> instead.
>
> I would write something along these lines:
>
> for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
>         if (&ds->player_leds[i] == led) {
>                 if (value == LED_OFF)
>                         player_leds_state &= ~(1 << i);
>                 else
>                         player_leds_state |= (1 << i);
>                 break;
>         }
> }
>
> This is just me hypothesizing though, could anyone clear this up for
> me? Thank you very much.

Thanks for your deep analysis. While writing the code I already found
it strange there were 2 layers of bookkeeping. As you point out this
is likely due to the power management logic.

The LEDs are created by 'ps_led_register' and it is setting:
    led->flags = LED_CORE_SUSPENDRESUME;

So those code paths would indeed be triggered provided
'CONFIG_PM_SLEEP' is set. So it is probably safest for me to change
that for-loop anyway. I had hoped to skip the 'find the LED logic' (it
just feels ugly somehow, but not a big deal). Let me make this quick
change. I will just wait a few more days on potential other feedback
from the list before submitting a 'v2'. So far nothing major, but just
these few small things :)


>
> Regards,
> Samuel

Thanks,
Roderick
Barnabás Pőcze Dec. 27, 2020, 2:27 p.m. UTC | #3 
Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ad8eedd3d2bf..384449e3095d 100644
> [...]
> +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);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
> +		if (&ds->player_leds[i] == led)
> +			return !!(ds->player_leds_state & BIT(i));
> +	}

Is there any reason why

```
  !!(ds->player_leds_state & BIT( led - ds->player_leds ))
```

or something similar is not used? Or am I missing something that prevents this
from working?

Furthermore, I don't quite see the purpose of this function. The LED core
can handle if no brightness_get() callback is provided. And since this
function returns just a cached value, I fail to see how it is different from
the default behaviour of the LED core, which is returning the last brightness
value. Am I missing something?


> +
> +	return LED_OFF;
> +}
> +
> +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);
> +	uint8_t player_leds_state = 0;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> +		player_leds_state |= (ds->player_leds[i].brightness << i);
> +

I believe this could be simplified as well using the fact that
`led - ds->player_leds` gives the index of the LED.


> +	spin_lock_irqsave(&ds->base.lock, flags);
> +	ds->player_leds_state = player_leds_state;
> +	ds->update_player_leds = true;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +}
> [...]


Regards,
Barnabás Pőcze
Roderick Colenbrander Dec. 28, 2020, 10:02 p.m. UTC | #4 
Hi Barnabás,

On Sun, Dec 27, 2020 at 6:27 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index ad8eedd3d2bf..384449e3095d 100644
> > [...]
> > +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);
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
> > +             if (&ds->player_leds[i] == led)
> > +                     return !!(ds->player_leds_state & BIT(i));
> > +     }
>
> Is there any reason why
>
> ```
>   !!(ds->player_leds_state & BIT( led - ds->player_leds ))
> ```
>
> or something similar is not used? Or am I missing something that prevents this
> from working?

I think this pointer math would work and need to give it a try. Hadn't
considered it

> Furthermore, I don't quite see the purpose of this function. The LED core
> can handle if no brightness_get() callback is provided. And since this
> function returns just a cached value, I fail to see how it is different from
> the default behaviour of the LED core, which is returning the last brightness
> value. Am I missing something?

Not all values may get set through sysfs. For example in the next
patch (12/13) the driver sets a default player LED mask value directly
and may set e.g. 0x1f or so. This could use the LED APIs, but the LED
framework doesn't have any group LED support (besides the new
multicolor class) and as such would get scheduled across multiple
output reports.

>
> > +
> > +     return LED_OFF;
> > +}
> > +
> > +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);
> > +     uint8_t player_leds_state = 0;
> > +     unsigned long flags;
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> > +             player_leds_state |= (ds->player_leds[i].brightness << i);
> > +
>
> I believe this could be simplified as well using the fact that
> `led - ds->player_leds` gives the index of the LED.

I will give this a try as well, thanks.

>
> > +     spin_lock_irqsave(&ds->base.lock, flags);
> > +     ds->player_leds_state = player_leds_state;
> > +     ds->update_player_leds = true;
> > +     spin_unlock_irqrestore(&ds->base.lock, flags);
> > +
> > +     schedule_work(&ds->output_worker);
> > +}
> > [...]
>
>
> Regards,
> Barnabás Pőcze

Thanks,
Roderick
Barnabás Pőcze Dec. 29, 2020, 6:49 p.m. UTC | #5 
2020. december 28., hétfő 23:02 keltezéssel, Roderick Colenbrander írta:

> [...]
> > Furthermore, I don't quite see the purpose of this function. The LED core
> > can handle if no brightness_get() callback is provided. And since this
> > function returns just a cached value, I fail to see how it is different from
> > the default behaviour of the LED core, which is returning the last brightness
> > value. Am I missing something?
>
> Not all values may get set through sysfs. For example in the next
> patch (12/13) the driver sets a default player LED mask value directly
> and may set e.g. 0x1f or so. This could use the LED APIs, but the LED
> framework doesn't have any group LED support (besides the new
> multicolor class) and as such would get scheduled across multiple
> output reports.
> [...]

You're right, I've missed that.
diff mbox series

Patch

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ad8eedd3d2bf..384449e3095d 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -90,6 +90,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)
 
@@ -134,6 +135,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. */
@@ -707,6 +713,39 @@  static enum led_brightness dualsense_mute_led_get_brightness(struct led_classdev
 	return ds->mic_muted;
 }
 
+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);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
+		if (&ds->player_leds[i] == led)
+			return !!(ds->player_leds_state & BIT(i));
+	}
+
+	return LED_OFF;
+}
+
+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);
+	uint8_t player_leds_state = 0;
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
+		player_leds_state |= (ds->player_leds[i].brightness << i);
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+	ds->player_leds_state = player_leds_state;
+	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)
 {
@@ -797,6 +836,13 @@  static void dualsense_output_worker(struct work_struct *work)
 		ds->update_lightbar = false;
 	}
 
+	if (ds->update_player_leds) {
+		r->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
+		r->player_leds = ds->player_leds_state;
+
+		ds->update_player_leds = false;
+	}
+
 	if (ds->update_mic_mute) {
 		if (ds->mic_muted) {
 			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
@@ -1042,10 +1088,18 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
 	uint8_t max_output_report_size;
-	int ret;
+	int i, ret;
 
 	struct ps_led_info mute_led_info = { "micmute", dualsense_mute_led_get_brightness, NULL };
 
+	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);
@@ -1126,6 +1180,14 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret < 0)
 		goto err;
 
+	for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) {
+		struct ps_led_info *led_info = &player_leds_info[i];
+
+		ret = ps_led_register((struct ps_device *)ds, &ds->player_leds[i], led_info);
+		if (ret < 0)
+			goto err;
+	}
+
 	return (struct ps_device *)ds;
 
 err: