From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Hi,
Recently Sony released a HID driver for the new PlayStation 5 controller
'DualSense'. Pavel Machek noticed the driver got staged for "-next" and asked
me to share the LED patches to linux-leds as well.
The LED patches I'm sharing are patch 6, 9, 11, 12 from the v6 hid-playstation
series as originally posted to linux-input. The driver in its full form can be
found on "hid.git/log/?h=for-5.12/playstation".
A little bit of background on the DualSense controller. It is a generic HID
gamepad (e.g. buttons and sticks) with a number of additional features, such
as Motion Sensors, audio, LEDs and others. All features are implemented using HID
input and output reports.
In regards to LEDs it has 3 types of LEDs. First, it has a RGB lightbar
which is used in PS4/PS5 games for various effects e.g. health indicator, police
siren, player color and other use cases. This patch series exposes it
using the multicolor class, which is great for our use case as the lightbar
state is sent using a single HID output report. Previously for our DualShock 4
controller, the driver exposed individual LEDs, which wasn't great due to race
conditions as games change LED state frequently.
The second LED type, is a row of 5 player indicator LEDs. Such LEDs are common
on gamepads to set a player number e.g. 1, 2, 4 etcetera. Each LED is exposed
as a single LED. A default player id is set based on the number of connected
controllers, which is common behavior within the input system.
Finally, the DualSense has a audio mute LED and a mute button. The mute button is
expected to mute the internal microphone of the DualSense. The mute behavior
is handled driver side and the driver then also programs the LED. From user space
perspective the LED is read-only.
Let me know if there are any questions or comments.
Thanks,
Roderick Colenbrander
Sony Interactive Entertainment, LLC
Roderick Colenbrander (13):
HID: playstation: add DualSense lightbar support
HID: playstation: add microphone mute support for DualSense.
HID: playstation: add DualSense player LEDs support.
HID: playstation: DualSense set LEDs to default player id.
MAINTAINERS | 6 +
drivers/hid/Kconfig | 21 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-playstation.c | 1504 +++++++++++++++++++++++++++++++++
5 files changed, 1533 insertions(+)
create mode 100644 drivers/hid/hid-playstation.c
--
2.26.2
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/Kconfig | 2 +
drivers/hid/hid-playstation.c | 99 +++++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+)
@@ -140,6 +151,12 @@ struct dualsense {uint8_tlightbar_green;uint8_tlightbar_blue;+/* Microphone */+boolupdate_mic_mute;+boolmic_muted;+boollast_btn_mic_state;+structled_classdevmute_led;+structwork_structoutput_worker;void*output_report_dmabuf;uint8_toutput_seq;/* Sequence number for output report. */
@@ -485,6 +502,32 @@ static int ps_get_report(struct hid_device *hdev, uint8_t report_id, uint8_t *bureturn0;}+staticintps_led_register(structps_device*ps_dev,structled_classdev*led,+conststructps_led_info*led_info)+{+intret;++led->name=devm_kasprintf(&ps_dev->hdev->dev,GFP_KERNEL,+"playstation::%pMR::%s",ps_dev->mac_address,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);+returnret;+}++return0;+}+/* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */staticintps_lightbar_register(structps_device*ps_dev,structled_classdev_mc*lightbar_mc_dev,int(*brightness_set)(structled_classdev*,enumled_brightness))
@@ -722,6 +765,19 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,return0;}+staticenumled_brightnessdualsense_mute_led_get_brightness(structled_classdev*led)+{+structdualsense*ds=container_of(led,structdualsense,mute_led);++returnds->mic_muted;+}++/* The mute LED is treated as read-only. This set call prevents ENOTSUP errors e.g. on unload. */+staticvoiddualsense_mute_led_set_brightness(structled_classdev*led,enumled_brightnessvalue)+{++}+staticvoiddualsense_init_output_report(structdualsense*ds,structdualsense_output_report*rp,void*buf){
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(-)
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Add a ID allocator to assign player ids to ps_device instances.
Utilize the player id to set a default color on the DualSense its
player LED strip.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
drivers/hid/hid-playstation.c | 70 ++++++++++++++++++++++++++++++++++-
1 file changed, 69 insertions(+), 1 deletion(-)
@@ -22,6 +23,8 @@staticDEFINE_MUTEX(ps_devices_lock);staticLIST_HEAD(ps_devices_list);+staticDEFINE_IDA(ps_player_id_allocator);+#define HID_PLAYSTATION_VERSION_PATCH 0x8000/* Base class for playstation devices. */
@@ -321,6 +326,24 @@ static int ps_devices_list_remove(struct ps_device *dev)return0;}+staticintps_device_set_player_id(structps_device*dev)+{+intret=ida_alloc(&ps_player_id_allocator,GFP_KERNEL);++if(ret<0)+returnret;++dev->player_id=ret;+return0;+}++staticvoidps_device_release_player_id(structps_device*dev)+{+ida_free(&ps_player_id_allocator,dev->player_id);++dev->player_id=U32_MAX;+}+staticstructinput_dev*ps_allocate_input_dev(structhid_device*hdev,constchar*name_suffix){structinput_dev*input_dev;
@@ -1156,6 +1179,29 @@ static int dualsense_reset_leds(struct dualsense *ds)return0;}+staticvoiddualsense_set_player_leds(structdualsense*ds)+{+/*+*TheDualSensecontrollerhasarowof5LEDsusedforplayerids.+*BehavioronthePlayStation5consoleistocentertheplayerid+*acrosstheLEDs,soe.g.player1wouldbe"--x--"withxbeing'on'.+*Followasimilarmappinghere.+*/+staticconstintplayer_ids[5]={+BIT(2),+BIT(3)|BIT(1),+BIT(4)|BIT(2)|BIT(0),+BIT(4)|BIT(3)|BIT(1)|BIT(0),+BIT(4)|BIT(3)|BIT(2)|BIT(1)|BIT(0)+};++uint8_tplayer_id=ds->base.player_id%ARRAY_SIZE(player_ids);++ds->update_player_leds=true;+ds->player_leds_state=player_ids[player_id];+schedule_work(&ds->output_worker);+}+staticstructps_device*dualsense_create(structhid_device*hdev){structdualsense*ds;
@@ -1264,6 +1310,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)gotoerr;}+ret=ps_device_set_player_id(ps_dev);+if(ret){+hid_err(hdev,"Failed to assign player id for DualSense: %d\n",ret);+gotoerr;+}++/* Set player LEDs to our player id. */+dualsense_set_player_leds(ds);+return&ds->base;err:
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
The LED subsystem has a predefined schema by which LED names should
look like:
devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
character should be used only as separator of these fields, so not MAC
addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst
The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...
In your case the name should be something like:
input3:rgb:indicator
Different existing functions are defined in
include/dt-bindings/leds/common.h.
BTW there are extended versions of LED registering functions, suffixed
by "_ext". These accept a struct led_init_data. If a fwnode of the LED
is passed to the registering function via this struct, the LED core
will compose a name for the LED itself. But since your LEDs don't have
device-tree nodes because they are on USB/BlueTooth joysticks, you
either have to compose the name itself like your code is doing now, or
you can propose a patch to the LED core, so that LED core will be able
to compose the LED name even without a device-tree node.
JFI, the function part is (in the future) supposed to somehow define LED
trigger which the system will assign to the LED on probe, but this is
not implemented yet. Currently when the LED has a devicetree node,
the trigger is assigned from the `linux,default-trigger` property, but
the idea is to infer it from the `function` property.
What is this RGB LED supposed to do on the joystick? Is it just for
nice colors? Or should it blink somehow? Can the hardware in the
joystick blink the LED itself? Or maybe fade between colors?
There is for example the pattern LED trigger which changes the LED
brightness by a defined pattern. I am planning to add multicolor
support to this trigger, because our RGB LED controller can offload
such thing to hardware.
Marek
From: Marek Behun <hidden> Date: 2021-02-15 14:30:25
On Sun, 14 Feb 2021 16:45:45 -0800
Roderick Colenbrander [off-list ref] wrote:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
Hi,
Recently Sony released a HID driver for the new PlayStation 5 controller
'DualSense'. Pavel Machek noticed the driver got staged for "-next" and asked
me to share the LED patches to linux-leds as well.
The LED patches I'm sharing are patch 6, 9, 11, 12 from the v6 hid-playstation
series as originally posted to linux-input. The driver in its full form can be
found on "hid.git/log/?h=for-5.12/playstation".
Hi,
OK I see you described the purpose of these LEDs here, please ignore
that one question in my reply to patch 1/4.
Finally, the DualSense has a audio mute LED and a mute button. The mute button is
expected to mute the internal microphone of the DualSense. The mute behavior
is handled driver side and the driver then also programs the LED. From user space
perspective the LED is read-only.
The audio mute LED should not be read-only from userspace. Instead a
LED trigger should be assigned by default, audio-micmute / audio-mute.
With this trigger the LED subsystem will handle setting brightness of
the LED according to whether the audio is muted or not.
This trigger is currently simple, though. It is system wide - it
is impossible to configure it to report only on the state of a
specific microphone. But the trigger driver can be extended if this is needed.
Marek
From: Marek Behun <hidden> Date: 2021-02-15 14:42:00
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
If it is, look at the audio-micmute LED trigger.
If you can't use the audio-micmute trigger because the microphone isn't
supported via Linux, I still think the LED should the LED should be
read-write. You can then register a LED private trigger. The driver should
change the state of the LED according to the microphone mute state only
if these trigger is enabled.
Marek
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
The LED subsystem has a predefined schema by which LED names should
look like:
devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
character should be used only as separator of these fields, so not MAC
addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst
The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...
In your case the name should be something like:
input3:rgb:indicator
Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.
I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.
Different existing functions are defined in
include/dt-bindings/leds/common.h.
BTW there are extended versions of LED registering functions, suffixed
by "_ext". These accept a struct led_init_data. If a fwnode of the LED
is passed to the registering function via this struct, the LED core
will compose a name for the LED itself. But since your LEDs don't have
device-tree nodes because they are on USB/BlueTooth joysticks, you
either have to compose the name itself like your code is doing now, or
you can propose a patch to the LED core, so that LED core will be able
to compose the LED name even without a device-tree node.
JFI, the function part is (in the future) supposed to somehow define LED
trigger which the system will assign to the LED on probe, but this is
not implemented yet. Currently when the LED has a devicetree node,
the trigger is assigned from the `linux,default-trigger` property, but
the idea is to infer it from the `function` property.
Thanks for the info. Might be handy in the future.
What is this RGB LED supposed to do on the joystick? Is it just for
nice colors? Or should it blink somehow? Can the hardware in the
joystick blink the LED itself? Or maybe fade between colors?
I mentioned a bit in the other email. These LEDs are under direct
control from PlayStation games. Some may change the color on a per
video frame basis. Use cases include health (green) and when a
character loses health becomes more red/orange and can start flashing.
I have seen games use this for police car and then mixing between blue
and red. Others use it with a static player ID color. The console side
API is literally raw RGB values. There is no hardware blink support.
The previous controllers had it though.
There is for example the pattern LED trigger which changes the LED
brightness by a defined pattern. I am planning to add multicolor
support to this trigger, because our RGB LED controller can offload
such thing to hardware.
Marek
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
The LED subsystem has a predefined schema by which LED names should
look like:
devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
character should be used only as separator of these fields, so not MAC
addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst
The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...
In your case the name should be something like:
input3:rgb:indicator
Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.
I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.
We have to find a reasonable devicename here. If each joystick registers
multiple input devices, it cannot be "input%d". I suppose there isn't
an API for grouping mulitple input devices toghether into inputgroups.
Maybe it could be in the format "joystick%d".
But I don't think it can be "playstation". Nor can MAC addresses be
there if they contain ':'s.
Marek
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
The LED subsystem has a predefined schema by which LED names should
look like:
devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
character should be used only as separator of these fields, so not MAC
addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst
The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...
In your case the name should be something like:
input3:rgb:indicator
Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.
I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.
We have to find a reasonable devicename here. If each joystick registers
multiple input devices, it cannot be "input%d". I suppose there isn't
an API for grouping mulitple input devices toghether into inputgroups.
Maybe it could be in the format "joystick%d".
Yeah, there is no inputgroups mechanism. It could use some type of
joystick name if that's what desired. However, there is no common ID
code. Individual drivers are sometimes calculating their own IDs
(hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
for hid-sony/hid-playstation the use case for tracking IDs is for a
part to prevent duplicate devices as you can connect your device using
both bluetooth and USB. So would be "ps-joystick0"
At the HID layer there does seem to be a unique ID, but it is only
exposed in the name string: This is how the name is constructed:
dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
hdev->vendor, hdev->product, atomic_inc_return(&id));
This ID is HID specific, but not all input devices use HID.
I'm not entirely sure what makes sense...
But I don't think it can be "playstation". Nor can MAC addresses be
there if they contain ':'s.
Marek
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If you can't use the audio-micmute trigger because the microphone isn't
supported via Linux, I still think the LED should the LED should be
read-write. You can then register a LED private trigger. The driver should
change the state of the LED according to the microphone mute state only
if these trigger is enabled.
Marek
From: Marek Behun <hidden> Date: 2021-02-15 18:20:00
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs. Before registering
the LED, assign default_trigger member so that this trigger is enabled
during registration.
This is why we have support for private LED triggers.
Marek
quoted
If you can't use the audio-micmute trigger because the microphone isn't
supported via Linux, I still think the LED should the LED should be
read-write. You can then register a LED private trigger. The driver should
change the state of the LED according to the microphone mute state only
if these trigger is enabled.
Marek
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
The LED subsystem has a predefined schema by which LED names should
look like:
devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
character should be used only as separator of these fields, so not MAC
addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst
The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...
In your case the name should be something like:
input3:rgb:indicator
Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.
I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.
We have to find a reasonable devicename here. If each joystick registers
multiple input devices, it cannot be "input%d". I suppose there isn't
an API for grouping mulitple input devices toghether into inputgroups.
Maybe it could be in the format "joystick%d".
Yeah, there is no inputgroups mechanism. It could use some type of
joystick name if that's what desired. However, there is no common ID
code. Individual drivers are sometimes calculating their own IDs
(hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
for hid-sony/hid-playstation the use case for tracking IDs is for a
part to prevent duplicate devices as you can connect your device using
both bluetooth and USB. So would be "ps-joystick0"
At the HID layer there does seem to be a unique ID, but it is only
exposed in the name string: This is how the name is constructed:
dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
hdev->vendor, hdev->product, atomic_inc_return(&id));
This ID is HID specific, but not all input devices use HID.
I'm not entirely sure what makes sense...
So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.
Send a patch to hid-core to make this atomic_inc_return(&id) also be
stored into struct hid_device as an integer, not only as a part
of the device name string.
Then use "hid%d" as the devicename for this LED, with %d substituted
with this ID.
Marek
Hi Marek,
On Sun, Feb 14, 2021 at 4:46 PM Roderick Colenbrander
[off-list ref] wrote:
quoted hunk
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(-)
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
From: Marek Behun <hidden> Date: 2021-02-16 00:34:39
On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander [off-list ref] 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 [off-list ref] wrote:
On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
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".
From: Marek Behun <hidden> Date: 2021-02-16 02:38:28
On Mon, 15 Feb 2021 17:11:14 -0800
Roderick Colenbrander [off-list ref] wrote:
On Mon, Feb 15, 2021 at 4:33 PM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
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.
quoted
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.
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun [off-list ref] wrote:
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).
Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs.
Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?
If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)
I am tempted to wait with the trigger code as I really don't understand it.
Before registering
the LED, assign default_trigger member so that this trigger is enabled
during registration.
This is why we have support for private LED triggers.
Marek
From: Marek Behun <hidden> Date: 2021-02-16 16:42:46
On Tue, 16 Feb 2021 00:33:24 -0800
Roderick Colenbrander [off-list ref] wrote:
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).
Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?
quoted
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs.
Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?
If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)
I am tempted to wait with the trigger code as I really don't understand it.
Simple triggers are just normal triggers but with some simplifying code
to avoid code repetition. Ignore them for now.
When a trigger is set to a LED via sysfs, the trigger .activate()
method is called and the led_cdev.trigger is set to point to that
trigger.
It is then up to the code inside the trigger's .activate() method to
initialize mechanisms that will control the LED.
For netdev trigger a delayed_work is scheduled periodically, and in each
execution of that work's callback the netdevice's stats are compared to
the last ones. If the new stats are greater, the trigger code blinks the
LED.
So in your case it is pretty simple to implement, because you already
have the necessary code to manipulate the LED brightness automatically
according to whether button was pressed. You are setting
ds->update_mic_mute = true;
in dualsense_parse_report() and then manipulate the LED in
dualsense_output_worker().
Just add another boolean member into struct dualsense:
bool control_mute_led;
and change the code in dualsense_output_worker() to only change the
mute_led brightness is this new member is true.
Add this code to your driver:
static struct led_hw_trigger_type ps_micmute_trigger_type;
When registering the LED in ps_led_register(), also set
led->trigger_type = &ps_micmute_trigger_type;
Add this functions:
static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
/* make the worker control mute LED according to mute button */
ds->control_mute_led = true;
/* make sure the mute LED shows the current mute button state */
ds->update_mic_mute = true;
schedule_work(&ds->output_worker);
return 0;
}
static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
ds->control_mute_led = false;
}
static struct led_trigger ps_micmute_trigger = {
.name = "playstation-micmute",
.activate = ps_micmute_trig_activate,
.deactivate = ps_micmute_trig_deactivate,
.trigger_type = &ps_micmute_trigger_type,
};
Add this code to ps_init():
int ret;
ret = led_trigger_register(&ps_micmute_trigger);
if (ret)
return ret;
And to ps_exit():
led_trigger_unregister(&ps_micmute_trigger);
All this will make sure that the driver will manipulate the mute
LED state only when the playstation-micmute trigger is active on the
LED.
Moreover if you want this driver to be active on the LED by default,
set this prior to registering the LED
led->default_trigger = "playstation-micmute";
Finally add code to dualsense_mute_led_set_brightness() to make
userspace/[other LED triggers] able to set mute LED brightness.
The purpose of the .trigger_type member of struct led_classdev and
struct led_trigger is that if this member is set for a trigger, this
trigger will only be available for LEDs that have the same trigger_type.
Marek
From: Benjamin Tissoires <hidden> Date: 2021-02-16 17:14:32
On Tue, Feb 16, 2021 at 5:41 PM Marek Behun [off-list ref] wrote:
On Tue, 16 Feb 2021 00:33:24 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).
Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?
quoted
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs.
Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?
If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)
I am tempted to wait with the trigger code as I really don't understand it.
Simple triggers are just normal triggers but with some simplifying code
to avoid code repetition. Ignore them for now.
When a trigger is set to a LED via sysfs, the trigger .activate()
method is called and the led_cdev.trigger is set to point to that
trigger.
It is then up to the code inside the trigger's .activate() method to
initialize mechanisms that will control the LED.
For netdev trigger a delayed_work is scheduled periodically, and in each
execution of that work's callback the netdevice's stats are compared to
the last ones. If the new stats are greater, the trigger code blinks the
LED.
So in your case it is pretty simple to implement, because you already
have the necessary code to manipulate the LED brightness automatically
according to whether button was pressed. You are setting
ds->update_mic_mute = true;
in dualsense_parse_report() and then manipulate the LED in
dualsense_output_worker().
Just add another boolean member into struct dualsense:
bool control_mute_led;
and change the code in dualsense_output_worker() to only change the
mute_led brightness is this new member is true.
Add this code to your driver:
static struct led_hw_trigger_type ps_micmute_trigger_type;
When registering the LED in ps_led_register(), also set
led->trigger_type = &ps_micmute_trigger_type;
Add this functions:
static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
/* make the worker control mute LED according to mute button */
ds->control_mute_led = true;
/* make sure the mute LED shows the current mute button state */
ds->update_mic_mute = true;
schedule_work(&ds->output_worker);
return 0;
}
static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
ds->control_mute_led = false;
}
static struct led_trigger ps_micmute_trigger = {
.name = "playstation-micmute",
.activate = ps_micmute_trig_activate,
.deactivate = ps_micmute_trig_deactivate,
.trigger_type = &ps_micmute_trigger_type,
};
Add this code to ps_init():
int ret;
ret = led_trigger_register(&ps_micmute_trigger);
if (ret)
return ret;
And to ps_exit():
led_trigger_unregister(&ps_micmute_trigger);
All this will make sure that the driver will manipulate the mute
LED state only when the playstation-micmute trigger is active on the
LED.
Moreover if you want this driver to be active on the LED by default,
set this prior to registering the LED
led->default_trigger = "playstation-micmute";
Finally add code to dualsense_mute_led_set_brightness() to make
userspace/[other LED triggers] able to set mute LED brightness.
The purpose of the .trigger_type member of struct led_classdev and
struct led_trigger is that if this member is set for a trigger, this
trigger will only be available for LEDs that have the same trigger_type.
Thanks Marek for the in-depth 101 on LED triggers :)
However, I am not sure we want to enable LED triggers for the micmute
on the controller itself. In the early discussions with Roderick, I
already suggested the use of the LED triggers, and the problem was
that they are shared system-wide. This is good for many use cases, but
in that particular case, the user expects the mic *of the controller*
to be muted, not everyone's controller's mics.This is a behavior
inherited from the Playstation 5 which would be hard to sell to owners
of the controllers.
So if read-only LEDs are not an option, how about we simply ditch the
LED for micmute in the current code, and have a simple callback
executed by the driver to light up or not the LED when the player
presses the key. Or just revert entirely this commit in the currently
staged series. We can then figure out a better way in the next future
to handle that part.
BTW, AFAICT, the only problem we have left (if we put that micmute
issue aside) is about the naming convention. If we fix the naming
shortly, would you have any concerns if we still push that code to
Linus in 5.12-rc0?
Cheers,
Benjamin
From: Benjamin Tissoires <hidden> Date: 2021-02-16 17:21:36
On Tue, Feb 16, 2021 at 3:37 AM Marek Behun [off-list ref] wrote:
On Mon, 15 Feb 2021 17:11:14 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 4:33 PM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
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
quoted
If this is what is decided, I guess we should update the Linux gamepad
document at some point as well.
quoted
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.
From: Marek Behun <hidden> Date: 2021-02-16 17:23:03
On Tue, 16 Feb 2021 18:12:39 +0100
Benjamin Tissoires [off-list ref] wrote:
On Tue, Feb 16, 2021 at 5:41 PM Marek Behun [off-list ref] wrote:
quoted
On Tue, 16 Feb 2021 00:33:24 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).
Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?
quoted
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs.
Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?
If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)
I am tempted to wait with the trigger code as I really don't understand it.
Simple triggers are just normal triggers but with some simplifying code
to avoid code repetition. Ignore them for now.
When a trigger is set to a LED via sysfs, the trigger .activate()
method is called and the led_cdev.trigger is set to point to that
trigger.
It is then up to the code inside the trigger's .activate() method to
initialize mechanisms that will control the LED.
For netdev trigger a delayed_work is scheduled periodically, and in each
execution of that work's callback the netdevice's stats are compared to
the last ones. If the new stats are greater, the trigger code blinks the
LED.
So in your case it is pretty simple to implement, because you already
have the necessary code to manipulate the LED brightness automatically
according to whether button was pressed. You are setting
ds->update_mic_mute = true;
in dualsense_parse_report() and then manipulate the LED in
dualsense_output_worker().
Just add another boolean member into struct dualsense:
bool control_mute_led;
and change the code in dualsense_output_worker() to only change the
mute_led brightness is this new member is true.
Add this code to your driver:
static struct led_hw_trigger_type ps_micmute_trigger_type;
When registering the LED in ps_led_register(), also set
led->trigger_type = &ps_micmute_trigger_type;
Add this functions:
static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
/* make the worker control mute LED according to mute button */
ds->control_mute_led = true;
/* make sure the mute LED shows the current mute button state */
ds->update_mic_mute = true;
schedule_work(&ds->output_worker);
return 0;
}
static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
ds->control_mute_led = false;
}
static struct led_trigger ps_micmute_trigger = {
.name = "playstation-micmute",
.activate = ps_micmute_trig_activate,
.deactivate = ps_micmute_trig_deactivate,
.trigger_type = &ps_micmute_trigger_type,
};
Add this code to ps_init():
int ret;
ret = led_trigger_register(&ps_micmute_trigger);
if (ret)
return ret;
And to ps_exit():
led_trigger_unregister(&ps_micmute_trigger);
All this will make sure that the driver will manipulate the mute
LED state only when the playstation-micmute trigger is active on the
LED.
Moreover if you want this driver to be active on the LED by default,
set this prior to registering the LED
led->default_trigger = "playstation-micmute";
Finally add code to dualsense_mute_led_set_brightness() to make
userspace/[other LED triggers] able to set mute LED brightness.
The purpose of the .trigger_type member of struct led_classdev and
struct led_trigger is that if this member is set for a trigger, this
trigger will only be available for LEDs that have the same trigger_type.
Thanks Marek for the in-depth 101 on LED triggers :)
However, I am not sure we want to enable LED triggers for the micmute
on the controller itself. In the early discussions with Roderick, I
already suggested the use of the LED triggers, and the problem was
that they are shared system-wide. This is good for many use cases, but
in that particular case, the user expects the mic *of the controller*
to be muted, not everyone's controller's mics.This is a behavior
inherited from the Playstation 5 which would be hard to sell to owners
of the controllers.
They are not system wide if private LED trigger API is used, as I
explained and as the example code does it in my previous reply. This
trigger will only be available for PlayStation micmute LEDs.
So if read-only LEDs are not an option, how about we simply ditch the
LED for micmute in the current code, and have a simple callback
executed by the driver to light up or not the LED when the player
presses the key. Or just revert entirely this commit in the currently
staged series. We can then figure out a better way in the next future
to handle that part.
This is irrelevant if you take into account the information I wrote
above.
BTW, AFAICT, the only problem we have left (if we put that micmute
issue aside) is about the naming convention. If we fix the naming
shortly, would you have any concerns if we still push that code to
Linus in 5.12-rc0?
Yes, if naming is corrected I have no issue with this. LED triggers can
be sent to 5.13.
Marek
+ ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
The LED subsystem has a predefined schema by which LED names should
look like:
devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
character should be used only as separator of these fields, so not MAC
addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst
The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...
In your case the name should be something like:
input3:rgb:indicator
Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.
I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.
We have to find a reasonable devicename here. If each joystick registers
multiple input devices, it cannot be "input%d". I suppose there isn't
an API for grouping mulitple input devices toghether into inputgroups.
Maybe it could be in the format "joystick%d".
Yeah, there is no inputgroups mechanism. It could use some type of
joystick name if that's what desired. However, there is no common ID
code. Individual drivers are sometimes calculating their own IDs
(hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
for hid-sony/hid-playstation the use case for tracking IDs is for a
part to prevent duplicate devices as you can connect your device using
both bluetooth and USB. So would be "ps-joystick0"
At the HID layer there does seem to be a unique ID, but it is only
exposed in the name string: This is how the name is constructed:
dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
hdev->vendor, hdev->product, atomic_inc_return(&id));
This ID is HID specific, but not all input devices use HID.
I'm not entirely sure what makes sense...
So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.
Yes and no. This atomic_inc is only used to allow a sysfs tree,
because you can have several HID devices below the same USB, I2C or
UHID physical device. From the userspace, no-one cares about that ID,
because all HID devices are exported as input, IIO or hidraw nodes.
So using this "id" would not allow for a direct mapping HID device ->
sysfs entry because users will still have to walk through the tree to
find out which is which.
An actual one-to-one mapping would using 'hidrawX' because there is a
one-to-one mapping between /dev/hidrawX for HID devices. However, this
means that we consider the bus to be hidraw which is plain wrong too.
The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
`BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
we standardize LEDs on the HID subsystem to be named
`hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
between the LED and the sysfs, and would also allow users to quickly
filter out the playstation ones.
Cheers,
Benjamin
Send a patch to hid-core to make this atomic_inc_return(&id) also be
stored into struct hid_device as an integer, not only as a part
of the device name string.
Then use "hid%d" as the devicename for this LED, with %d substituted
with this ID.
Marek
From: Marek Behun <hidden> Date: 2021-02-16 17:41:15
On Tue, 16 Feb 2021 18:21:55 +0100
Marek Behun [off-list ref] wrote:
quoted
BTW, AFAICT, the only problem we have left (if we put that micmute
issue aside) is about the naming convention. If we fix the naming
shortly, would you have any concerns if we still push that code to
Linus in 5.12-rc0?
So I had a call with Pavel Machek, who is maintainer of the LED
subsystem, and he thinks that you should wait with the LED patches
until they are acked/reviewed by others. So if you want to send a
pull-request to Linus, you probably should remove the LED patches for
now. They can be added later when they are reviewed.
Yes, if naming is corrected I have no issue with this. LED triggers can
be sent to 5.13.
By that I mean that in my opinion the best naming scheme here would be:
hid%d:COLOR:micmute
for micmute LED (there is LED_FUNCTION_MICMUTE constant defined in
include/dt-bindings/leds/common.h)
and
hid%d:COLOR:player-%d
for player LEDs.
This will need adding LED_FUNCTION_PLAYER constant, which will need
sending a patch that will need to be reviewed by LED people (and
applied by Pavel). I talked with Pavel and he thinks that if there are
other HID drivers (for joysticks) that also have this kind of LEDs
(identifiing players), than such a patch would be reasonable (and those
LEDs should use the constant). Otherwise LED_FUNCTION_INDICATOR should
be probably used.
Marek
From: Benjamin Tissoires <hidden> Date: 2021-02-16 17:44:21
On Tue, Feb 16, 2021 at 6:22 PM Marek Behun [off-list ref] wrote:
On Tue, 16 Feb 2021 18:12:39 +0100
Benjamin Tissoires [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 5:41 PM Marek Behun [off-list ref] wrote:
quoted
On Tue, 16 Feb 2021 00:33:24 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).
Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?
quoted
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs.
Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?
If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)
I am tempted to wait with the trigger code as I really don't understand it.
Simple triggers are just normal triggers but with some simplifying code
to avoid code repetition. Ignore them for now.
When a trigger is set to a LED via sysfs, the trigger .activate()
method is called and the led_cdev.trigger is set to point to that
trigger.
It is then up to the code inside the trigger's .activate() method to
initialize mechanisms that will control the LED.
For netdev trigger a delayed_work is scheduled periodically, and in each
execution of that work's callback the netdevice's stats are compared to
the last ones. If the new stats are greater, the trigger code blinks the
LED.
So in your case it is pretty simple to implement, because you already
have the necessary code to manipulate the LED brightness automatically
according to whether button was pressed. You are setting
ds->update_mic_mute = true;
in dualsense_parse_report() and then manipulate the LED in
dualsense_output_worker().
Just add another boolean member into struct dualsense:
bool control_mute_led;
and change the code in dualsense_output_worker() to only change the
mute_led brightness is this new member is true.
Add this code to your driver:
static struct led_hw_trigger_type ps_micmute_trigger_type;
When registering the LED in ps_led_register(), also set
led->trigger_type = &ps_micmute_trigger_type;
Add this functions:
static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
/* make the worker control mute LED according to mute button */
ds->control_mute_led = true;
/* make sure the mute LED shows the current mute button state */
ds->update_mic_mute = true;
schedule_work(&ds->output_worker);
return 0;
}
static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
ds->control_mute_led = false;
}
static struct led_trigger ps_micmute_trigger = {
.name = "playstation-micmute",
.activate = ps_micmute_trig_activate,
.deactivate = ps_micmute_trig_deactivate,
.trigger_type = &ps_micmute_trigger_type,
};
Add this code to ps_init():
int ret;
ret = led_trigger_register(&ps_micmute_trigger);
if (ret)
return ret;
And to ps_exit():
led_trigger_unregister(&ps_micmute_trigger);
All this will make sure that the driver will manipulate the mute
LED state only when the playstation-micmute trigger is active on the
LED.
Moreover if you want this driver to be active on the LED by default,
set this prior to registering the LED
led->default_trigger = "playstation-micmute";
Finally add code to dualsense_mute_led_set_brightness() to make
userspace/[other LED triggers] able to set mute LED brightness.
The purpose of the .trigger_type member of struct led_classdev and
struct led_trigger is that if this member is set for a trigger, this
trigger will only be available for LEDs that have the same trigger_type.
Thanks Marek for the in-depth 101 on LED triggers :)
However, I am not sure we want to enable LED triggers for the micmute
on the controller itself. In the early discussions with Roderick, I
already suggested the use of the LED triggers, and the problem was
that they are shared system-wide. This is good for many use cases, but
in that particular case, the user expects the mic *of the controller*
to be muted, not everyone's controller's mics.This is a behavior
inherited from the Playstation 5 which would be hard to sell to owners
of the controllers.
They are not system wide if private LED trigger API is used, as I
explained and as the example code does it in my previous reply. This
trigger will only be available for PlayStation micmute LEDs.
My initial reply was the following:
"""
Unless I am reading this all wrong, the "private" is private for the
driver, not the device itself. If I have 2 controllers connected, and
both are set to "playstation-micmute", if one controller sends the
trigger because the button is pressed, both controllers would receive
the trigger, no?
"""
But after re-reading your explanations it seems you are not asking the
trigger to be actually "triggered". So basically, the private trigger
is just a way for the driver to handle its own business and allow
other triggers to be set. I always thought that whenever we declare a
trigger, we *had* to use it, but actually using the trigger just as a
way to confer information is smart.
Cheers,
Benjamin
quoted
So if read-only LEDs are not an option, how about we simply ditch the
LED for micmute in the current code, and have a simple callback
executed by the driver to light up or not the LED when the player
presses the key. Or just revert entirely this commit in the currently
staged series. We can then figure out a better way in the next future
to handle that part.
This is irrelevant if you take into account the information I wrote
above.
quoted
BTW, AFAICT, the only problem we have left (if we put that micmute
issue aside) is about the naming convention. If we fix the naming
shortly, would you have any concerns if we still push that code to
Linus in 5.12-rc0?
Yes, if naming is corrected I have no issue with this. LED triggers can
be sent to 5.13.
Marek
From: Marek Behun <hidden> Date: 2021-02-16 17:45:11
On Tue, 16 Feb 2021 18:19:49 +0100
Benjamin Tissoires [off-list ref] wrote:
On Tue, Feb 16, 2021 at 3:37 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 17:11:14 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 4:33 PM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 15:00:30 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
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
From: Marek Behun <hidden> Date: 2021-02-16 17:57:05
On Tue, 16 Feb 2021 18:29:46 +0100
Benjamin Tissoires [off-list ref] wrote:
quoted
So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.
Yes and no. This atomic_inc is only used to allow a sysfs tree,
because you can have several HID devices below the same USB, I2C or
UHID physical device. From the userspace, no-one cares about that ID,
because all HID devices are exported as input, IIO or hidraw nodes.
So using this "id" would not allow for a direct mapping HID device ->
sysfs entry because users will still have to walk through the tree to
find out which is which.
So you are saying that the fact that userspace cannot take the number
from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
the problem here.
This is not a problem in my opinion, because userspace can simply
access the parent HID device via /sys/class/leds/hidN:color:func/parent.
In fact we did something similar for LEDs connected to ethernet PHYs.
To summarize:
- ethernet PHYs are identified by long, sometimes crazy strings like
d0032004.mdio-mii:01
or even
/soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
- for the purposes of having a sane devicename part in LED names, I
sent this patch
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
which adds a simple incrementing integer ID to each PHY device.
(The code is not in upstream yet because there is other work needed
and because I decided that some functionality has to be available
via a different mechanism, but this part is complete and reviewed.)
An actual one-to-one mapping would using 'hidrawX' because there is a
one-to-one mapping between /dev/hidrawX for HID devices. However, this
means that we consider the bus to be hidraw which is plain wrong too.
The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
`BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
we standardize LEDs on the HID subsystem to be named
`hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
between the LED and the sysfs, and would also allow users to quickly
filter out the playstation ones.
As I wrote in other e-mail some minutes ago, this just means that we
need to wait for other people's opinions. Please do not send this
pull-request with the LED patches until this is resolved.
Marek
From: Marek Behun <hidden> Date: 2021-02-16 18:00:56
On Tue, 16 Feb 2021 18:42:19 +0100
Benjamin Tissoires [off-list ref] wrote:
On Tue, Feb 16, 2021 at 6:22 PM Marek Behun [off-list ref] wrote:
quoted
On Tue, 16 Feb 2021 18:12:39 +0100
Benjamin Tissoires [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 5:41 PM Marek Behun [off-list ref] wrote:
quoted
On Tue, 16 Feb 2021 00:33:24 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 10:07:29 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun [off-list ref] wrote:
quoted
On Sun, 14 Feb 2021 16:45:47 -0800
Roderick Colenbrander [off-list ref] wrote:
quoted
From: Roderick Colenbrander <roderick.colenbrander@sony.com>
The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.
This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.
Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Is the microphone supported via Linux? I.e. is there an audio driver
for it?
Yes and no. The microphone is supported using USB, not yet using
Bluetooth (uses a custom protocol). Actually there are various other
audio features in the DualSense (headphone jack, speaker, volume
controls,..) and they all work using custom protocols. We were
planning to defer this work through future patches as the features are
very complicated and need a deep analysis on how to realize them. For
example audio controls work through HID, but for USB the audio driver
is a generic hda audio device I think. Bluetooth is a custom protocol
and will be yet a different audio driver somewhere.
quoted
If it is, look at the audio-micmute LED trigger.
I'm not sure if the expected behavior for the DualSense is similar to
the standard audio mute use cases. My understanding of these triggers
(please correct me if I'm wrong) is for e.g. an audio driver or user
space to send a signal to anything registering for a particular
trigger. In this case a global micmute. Is that, right?
In our case for PlayStation games, there are often multiple
controllers connected and each user has their own microphone in their
controller. All can function at the same time (different from a
standard PC use case). That's why I'm wondering if this makes sense.I
know we are on Linux, but for Sony we want to properly support such
use cases.
If there aren't audio drivers yet for this, simply have this driver
also register a private LED trigger (with name "joystick-audiomute"
or something similar), and when registering the LED, set the
trigger_type member. Look at trigger_type in include/linux/leds.h, and
in LED Documentation.
Sorry for some more questions. I have been trying to understand
triggers all night. The concept is just so strange and foreign to me.
I understand it is in the end just a string and one use case is
in-kernel IPC and you can configure them from user space as well, but
I just don't get it. I understand you can use a trigger to in the end
program your LED in a automatic manner. I just don't understand how
the concepts fit together and how to implement it (maybe I will update
the docs later on... they are a bit sparse for if you don't know this
area).
Regarding registering a private trigger. I see include/linux/leds.h
have a comment about trigger_type and how it should be set for private
triggers on led_classdev. I haven't been able to find any example
usages of this within the kernel. It doesn't seem to be used in the
kernel, maybe it is just around for future use? I also seem to need to
implement my own activate/deactive callbacks for the trigger. These I
would use to program the LED brightness I guess. Though I see various
trigger drivers (drivers/leds/triggers), but not all of them have
activate/deactivate callbacks. Mostly simple drivers, but not sure why
they don't need them. What else is the point of a trigger?
quoted
When this trigger is enabled for your LED, have your code switch LED
state like it does now. When there is no trigger enabled, the userspace
will be able to set brightness of this LED via sysfs.
Right now I manage the button mute state directly from the input
handler (dualsense_parse_report) when the button is pressed and then
schedule an output report to toggle the LED and program the DualSense
to mute its audio (the PlayStation works very similar). I would need
to use led_trigger_event then here?
If I then understand it right, I need to modify my "brightness_set"
handler and check if there is a trigger (based on
led_classdev->activated??). If there is none, then userspace can
change the LED state. Internally when I change the LED state, I will
also program the hardware to mute as well. (they are tied together)
I am tempted to wait with the trigger code as I really don't understand it.
Simple triggers are just normal triggers but with some simplifying code
to avoid code repetition. Ignore them for now.
When a trigger is set to a LED via sysfs, the trigger .activate()
method is called and the led_cdev.trigger is set to point to that
trigger.
It is then up to the code inside the trigger's .activate() method to
initialize mechanisms that will control the LED.
For netdev trigger a delayed_work is scheduled periodically, and in each
execution of that work's callback the netdevice's stats are compared to
the last ones. If the new stats are greater, the trigger code blinks the
LED.
So in your case it is pretty simple to implement, because you already
have the necessary code to manipulate the LED brightness automatically
according to whether button was pressed. You are setting
ds->update_mic_mute = true;
in dualsense_parse_report() and then manipulate the LED in
dualsense_output_worker().
Just add another boolean member into struct dualsense:
bool control_mute_led;
and change the code in dualsense_output_worker() to only change the
mute_led brightness is this new member is true.
Add this code to your driver:
static struct led_hw_trigger_type ps_micmute_trigger_type;
When registering the LED in ps_led_register(), also set
led->trigger_type = &ps_micmute_trigger_type;
Add this functions:
static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
/* make the worker control mute LED according to mute button */
ds->control_mute_led = true;
/* make sure the mute LED shows the current mute button state */
ds->update_mic_mute = true;
schedule_work(&ds->output_worker);
return 0;
}
static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
{
struct dualsense *ds = container_of(...);
ds->control_mute_led = false;
}
static struct led_trigger ps_micmute_trigger = {
.name = "playstation-micmute",
.activate = ps_micmute_trig_activate,
.deactivate = ps_micmute_trig_deactivate,
.trigger_type = &ps_micmute_trigger_type,
};
Add this code to ps_init():
int ret;
ret = led_trigger_register(&ps_micmute_trigger);
if (ret)
return ret;
And to ps_exit():
led_trigger_unregister(&ps_micmute_trigger);
All this will make sure that the driver will manipulate the mute
LED state only when the playstation-micmute trigger is active on the
LED.
Moreover if you want this driver to be active on the LED by default,
set this prior to registering the LED
led->default_trigger = "playstation-micmute";
Finally add code to dualsense_mute_led_set_brightness() to make
userspace/[other LED triggers] able to set mute LED brightness.
The purpose of the .trigger_type member of struct led_classdev and
struct led_trigger is that if this member is set for a trigger, this
trigger will only be available for LEDs that have the same trigger_type.
Thanks Marek for the in-depth 101 on LED triggers :)
However, I am not sure we want to enable LED triggers for the micmute
on the controller itself. In the early discussions with Roderick, I
already suggested the use of the LED triggers, and the problem was
that they are shared system-wide. This is good for many use cases, but
in that particular case, the user expects the mic *of the controller*
to be muted, not everyone's controller's mics.This is a behavior
inherited from the Playstation 5 which would be hard to sell to owners
of the controllers.
They are not system wide if private LED trigger API is used, as I
explained and as the example code does it in my previous reply. This
trigger will only be available for PlayStation micmute LEDs.
My initial reply was the following:
"""
Unless I am reading this all wrong, the "private" is private for the
driver, not the device itself. If I have 2 controllers connected, and
both are set to "playstation-micmute", if one controller sends the
trigger because the button is pressed, both controllers would receive
the trigger, no?
"""
But after re-reading your explanations it seems you are not asking the
trigger to be actually "triggered". So basically, the private trigger
is just a way for the driver to handle its own business and allow
other triggers to be set. I always thought that whenever we declare a
trigger, we *had* to use it, but actually using the trigger just as a
way to confer information is smart.
Not only private triggers work this way. All but simple triggers do.
If you use the "netdev" trigger to 2 differnet LEDs, you can set it to
blink LED 1 on activity on eth0 device, and LED 2 to blink on activity
on eth1 device. So clearly they are triggering just the LEDs that are
configured to be triggered, in such a way that was desired for each LED.
Marek
From: Benjamin Tissoires <hidden> Date: 2021-02-16 18:16:47
On Tue, Feb 16, 2021 at 6:56 PM Marek Behun [off-list ref] wrote:
On Tue, 16 Feb 2021 18:29:46 +0100
Benjamin Tissoires [off-list ref] wrote:
quoted
quoted
So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.
Yes and no. This atomic_inc is only used to allow a sysfs tree,
because you can have several HID devices below the same USB, I2C or
UHID physical device. From the userspace, no-one cares about that ID,
because all HID devices are exported as input, IIO or hidraw nodes.
So using this "id" would not allow for a direct mapping HID device ->
sysfs entry because users will still have to walk through the tree to
find out which is which.
So you are saying that the fact that userspace cannot take the number
from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
the problem here.
This is not a problem in my opinion, because userspace can simply
access the parent HID device via /sys/class/leds/hidN:color:func/parent.
So in that case, there is no real point at keeping this ID in sync
with anything else? I would be more willing to accept a patch in HID
core that keeps this ID just for HID LEDs, instead of adding just an
ID with no meaning to all HID devices.
Honestly, I think the whole LED class creation API should be
revisited. I guess this is not the first time this problem arises, and
you must be tired of having to chase down users.
If I had to deal with that situation once for all, I would deprecate
the current led class creation API, and add a new API that doesn't
take a free-form string as the name but constrain the name to be
formed by your requirements. This would also send a clear message to
all subsystems because the changes have to be propagated, and then,
all the maintainers would know about this problem. Bonus point, if you
need only "subsystem", "color" and "function", that means that the ID
can be stored internally to the led class and you'll get happy users.
In fact we did something similar for LEDs connected to ethernet PHYs.
To summarize:
- ethernet PHYs are identified by long, sometimes crazy strings like
d0032004.mdio-mii:01
or even
/soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
- for the purposes of having a sane devicename part in LED names, I
sent this patch
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
which adds a simple incrementing integer ID to each PHY device.
(The code is not in upstream yet because there is other work needed
and because I decided that some functionality has to be available
via a different mechanism, but this part is complete and reviewed.)
quoted
An actual one-to-one mapping would using 'hidrawX' because there is a
one-to-one mapping between /dev/hidrawX for HID devices. However, this
means that we consider the bus to be hidraw which is plain wrong too.
The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
`BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
we standardize LEDs on the HID subsystem to be named
`hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
between the LED and the sysfs, and would also allow users to quickly
filter out the playstation ones.
As I wrote in other e-mail some minutes ago, this just means that we
need to wait for other people's opinions. Please do not send this
pull-request with the LED patches until this is resolved.
Yeah, I just asked Roderick to see if he can revert those patches
while keeping the functionality behind those. I am more concerned
about the micmute button, because we should really offer that feature
to users. The associated LED class has no real benefits for now, so
that code needs a little bit of care instead of a plain revert.
Cheers,
Benjamin
From: Marek Behun <hidden> Date: 2021-02-16 20:28:47
On Tue, 16 Feb 2021 19:14:48 +0100
Benjamin Tissoires [off-list ref] wrote:
On Tue, Feb 16, 2021 at 6:56 PM Marek Behun [off-list ref] wrote:
quoted
On Tue, 16 Feb 2021 18:29:46 +0100
Benjamin Tissoires [off-list ref] wrote:
quoted
quoted
So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.
Yes and no. This atomic_inc is only used to allow a sysfs tree,
because you can have several HID devices below the same USB, I2C or
UHID physical device. From the userspace, no-one cares about that ID,
because all HID devices are exported as input, IIO or hidraw nodes.
So using this "id" would not allow for a direct mapping HID device ->
sysfs entry because users will still have to walk through the tree to
find out which is which.
So you are saying that the fact that userspace cannot take the number
from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
the problem here.
This is not a problem in my opinion, because userspace can simply
access the parent HID device via /sys/class/leds/hidN:color:func/parent.
So in that case, there is no real point at keeping this ID in sync
with anything else? I would be more willing to accept a patch in HID
core that keeps this ID just for HID LEDs, instead of adding just an
ID with no meaning to all HID devices.
I think there was some misunderstanding.
If there are multiple LEDs on one joystick, all these LEDs should have
the same devicename part of the LED name. Different joysticks should
have different devicename parts of their LEDs names.
As another example think about keyboard LEDs if I have 2 keyboards
input3:green:numlock
input3:green:capslock
input3:green:scrolllock
input4:green:numlock
input4:green:capslock
input4:green:scrolllock
Honestly, I think the whole LED class creation API should be
revisited. I guess this is not the first time this problem arises, and
you must be tired of having to chase down users.
I will not argue with you about this since it is true. The work is
slow though because lack of people and time. I too have some ideas for
the LED subsystem but I also have many other priorities in work. Pavel
has a TODO list in drivers/leds/TODO. The main thing probably is that
it would be great to have more input from other kernel people when
doing something in LEDs, but either not that many people subscribe to
linux-leds mailing list or we should be informing them via different
mechanisms...
If I had to deal with that situation once for all, I would deprecate
the current led class creation API, and add a new API that doesn't
take a free-form string as the name but constrain the name to be
formed by your requirements. This would also send a clear message to
all subsystems because the changes have to be propagated, and then,
all the maintainers would know about this problem. Bonus point, if you
need only "subsystem", "color" and "function", that means that the ID
can be stored internally to the led class and you'll get happy users.
As I mentioned in the example above, there can be multiple LEDs for one
devicename...
quoted
In fact we did something similar for LEDs connected to ethernet PHYs.
To summarize:
- ethernet PHYs are identified by long, sometimes crazy strings like
d0032004.mdio-mii:01
or even
/soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
- for the purposes of having a sane devicename part in LED names, I
sent this patch
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
which adds a simple incrementing integer ID to each PHY device.
(The code is not in upstream yet because there is other work needed
and because I decided that some functionality has to be available
via a different mechanism, but this part is complete and reviewed.)
quoted
An actual one-to-one mapping would using 'hidrawX' because there is a
one-to-one mapping between /dev/hidrawX for HID devices. However, this
means that we consider the bus to be hidraw which is plain wrong too.
The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
`BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
we standardize LEDs on the HID subsystem to be named
`hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
between the LED and the sysfs, and would also allow users to quickly
filter out the playstation ones.
As I wrote in other e-mail some minutes ago, this just means that we
need to wait for other people's opinions. Please do not send this
pull-request with the LED patches until this is resolved.
Yeah, I just asked Roderick to see if he can revert those patches
while keeping the functionality behind those. I am more concerned
about the micmute button, because we should really offer that feature
to users. The associated LED class has no real benefits for now, so
that code needs a little bit of care instead of a plain revert.