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