diff mbox series

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

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

Commit Message

Roderick Colenbrander Feb. 15, 2021, 12:45 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 | 60 ++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

Comments

Roderick Colenbrander Feb. 15, 2021, 11 p.m. UTC | #1
Hi Marek,

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

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

Thanks,
Roderick
Marek Behún Feb. 16, 2021, 12:33 a.m. UTC | #2
On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

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

Hi,

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

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

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

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

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

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

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

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

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

> Marek

Thanks,
Roderick
Marek Behún Feb. 16, 2021, 2:37 a.m. UTC | #4
On Mon, 15 Feb 2021 17:11:14 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

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

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

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

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

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

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

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

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

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

Cheers,
Benjamin

>
> > If this is what is decided, I guess we should update the Linux gamepad
> > document at some point as well.
> >
> > > Are these LEDs of different colors which are impossible to determine?
> > > The string "hid%d::led1" you mention above does not indicate color.
> >
> > The DualSense LEDs are all white (at least so far?). On controllers
> > from other brands I have seen them be red or green. So could indeed
> > use: "hid%d:white".
>
> Yes, a constant for white color is defined in headers.
>
> > > Marek
> >
> > Thanks,
> > Roderick
>
Marek Behún Feb. 16, 2021, 5:43 p.m. UTC | #6
On Tue, 16 Feb 2021 18:19:49 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

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

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

Marek
diff mbox series

Patch

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