| Message ID | 20210215004549.135251-3-roderick@gaikai.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: new driver for PS5 'DualSense' controller | expand |
On Sun, 14 Feb 2021 16:45:47 -0800 Roderick Colenbrander <roderick@gaikai.com> 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
On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > On Sun, 14 Feb 2021 16:45:47 -0800 > Roderick Colenbrander <roderick@gaikai.com> 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? 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 Roderick
On Mon, 15 Feb 2021 10:07:29 -0800 Roderick Colenbrander <roderick@gaikai.com> wrote: > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > Roderick Colenbrander <roderick@gaikai.com> 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? > > 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 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 > > 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 > > Roderick
On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > On Mon, 15 Feb 2021 10:07:29 -0800 > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > Roderick Colenbrander <roderick@gaikai.com> 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? > > > > 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 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 > Roderick
On Tue, 16 Feb 2021 00:33:24 -0800 Roderick Colenbrander <roderick@gaikai.com> wrote: > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Mon, 15 Feb 2021 10:07:29 -0800 > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > > Roderick Colenbrander <roderick@gaikai.com> 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? > > > > > > 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 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. 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
On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Tue, 16 Feb 2021 00:33:24 -0800 > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Mon, 15 Feb 2021 10:07:29 -0800 > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > > > Roderick Colenbrander <roderick@gaikai.com> 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? > > > > > > > > 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 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. > > 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
On Tue, 16 Feb 2021 18:12:39 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Tue, 16 Feb 2021 00:33:24 -0800 > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > On Mon, 15 Feb 2021 10:07:29 -0800 > > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > > > > Roderick Colenbrander <roderick@gaikai.com> 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? > > > > > > > > > > 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 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. > > > > 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
On Tue, 16 Feb 2021 18:21:55 +0100 Marek Behun <marek.behun@nic.cz> wrote: > > 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
On Tue, Feb 16, 2021 at 6:22 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Tue, 16 Feb 2021 18:12:39 +0100 > Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Tue, 16 Feb 2021 00:33:24 -0800 > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > On Mon, 15 Feb 2021 10:07:29 -0800 > > > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > > > > > Roderick Colenbrander <roderick@gaikai.com> 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? > > > > > > > > > > > > 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 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. > > > > > > 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 > > > 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 >
On Tue, 16 Feb 2021 18:42:19 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Tue, Feb 16, 2021 at 6:22 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Tue, 16 Feb 2021 18:12:39 +0100 > > Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > On Tue, 16 Feb 2021 00:33:24 -0800 > > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > > > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > > > On Mon, 15 Feb 2021 10:07:29 -0800 > > > > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > > > > > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > > > > > > > > > On Sun, 14 Feb 2021 16:45:47 -0800 > > > > > > > > Roderick Colenbrander <roderick@gaikai.com> 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? > > > > > > > > > > > > > > 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 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. > > > > > > > > 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
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index cfa29dc17064..446a4d579908 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -857,6 +857,8 @@ config HID_PLAYSTATION tristate "PlayStation HID Driver" depends on HID select CRC32 + select NEW_LEDS + select LEDS_CLASS select LEDS_CLASS_MULTICOLOR select POWER_SUPPLY help diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c index 97c1118ba78f..c436ac8f7a6f 100644 --- a/drivers/hid/hid-playstation.c +++ b/drivers/hid/hid-playstation.c @@ -10,6 +10,7 @@ #include <linux/device.h> #include <linux/hid.h> #include <linux/input/mt.h> +#include <linux/leds.h> #include <linux/led-class-multicolor.h> #include <linux/module.h> @@ -47,6 +48,12 @@ struct ps_calibration_data { int sens_denom; }; +struct ps_led_info { + const char *name; + enum led_brightness (*brightness_get)(struct led_classdev *cdev); + void (*brightness_set)(struct led_classdev *cdev, enum led_brightness); +}; + /* Seed values for DualShock4 / DualSense CRC32 for different report types. */ #define PS_INPUT_CRC32_SEED 0xA1 #define PS_OUTPUT_CRC32_SEED 0xA2 @@ -82,6 +89,7 @@ struct ps_calibration_data { #define DS_BUTTONS1_R3 BIT(7) #define DS_BUTTONS2_PS_HOME BIT(0) #define DS_BUTTONS2_TOUCHPAD BIT(1) +#define DS_BUTTONS2_MIC_MUTE BIT(2) /* Status field of DualSense input report. */ #define DS_STATUS_BATTERY_CAPACITY GENMASK(3, 0) @@ -100,9 +108,12 @@ struct ps_calibration_data { /* Flags for DualSense output report. */ #define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0) #define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1) +#define DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE BIT(0) +#define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1) #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2) #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3) #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1) +#define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4) #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1) /* DualSense hardware limits */ @@ -140,6 +151,12 @@ struct dualsense { uint8_t lightbar_green; uint8_t lightbar_blue; + /* Microphone */ + bool update_mic_mute; + bool mic_muted; + bool last_btn_mic_state; + struct led_classdev mute_led; + struct work_struct output_worker; void *output_report_dmabuf; uint8_t output_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 *bu return 0; } +static int ps_led_register(struct ps_device *ps_dev, struct led_classdev *led, + const struct ps_led_info *led_info) +{ + int ret; + + led->name = devm_kasprintf(&ps_dev->hdev->dev, GFP_KERNEL, + "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); + return ret; + } + + return 0; +} + /* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */ static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev, int (*brightness_set)(struct led_classdev *, enum led_brightness)) @@ -722,6 +765,19 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev, return 0; } +static enum led_brightness dualsense_mute_led_get_brightness(struct led_classdev *led) +{ + struct dualsense *ds = container_of(led, struct dualsense, mute_led); + + return ds->mic_muted; +} + +/* The mute LED is treated as read-only. This set call prevents ENOTSUP errors e.g. on unload. */ +static void dualsense_mute_led_set_brightness(struct led_classdev *led, enum led_brightness value) +{ + +} + static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp, void *buf) { @@ -814,6 +870,23 @@ static void dualsense_output_worker(struct work_struct *work) ds->update_lightbar = false; } + if (ds->update_mic_mute) { + common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE; + common->mute_button_led = ds->mic_muted; + + if (ds->mic_muted) { + /* Disable microphone */ + common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE; + common->power_save_control |= DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE; + } else { + /* Enable microphone */ + common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE; + common->power_save_control &= ~DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE; + } + + ds->update_mic_mute = false; + } + spin_unlock_irqrestore(&ds->base.lock, flags); dualsense_send_output_report(ds, &report); @@ -828,6 +901,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r uint8_t battery_data, battery_capacity, charging_status, value; int battery_status; uint32_t sensor_timestamp; + bool btn_mic_state; unsigned long flags; int i; @@ -883,6 +957,23 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r input_report_key(ds->gamepad, BTN_MODE, ds_report->buttons[2] & DS_BUTTONS2_PS_HOME); input_sync(ds->gamepad); + /* + * The DualSense has an internal microphone, which can be muted through a mute button + * on the device. The driver is expected to read the button state and program the device + * to mute/unmute audio at the hardware level. + */ + btn_mic_state = !!(ds_report->buttons[2] & DS_BUTTONS2_MIC_MUTE); + if (btn_mic_state && !ds->last_btn_mic_state) { + spin_lock_irqsave(&ps_dev->lock, flags); + ds->update_mic_mute = true; + ds->mic_muted = !ds->mic_muted; /* toggle */ + spin_unlock_irqrestore(&ps_dev->lock, flags); + + /* Schedule updating of microphone state at hardware level. */ + schedule_work(&ds->output_worker); + } + ds->last_btn_mic_state = btn_mic_state; + /* Parse and calibrate gyroscope data. */ for (i = 0; i < ARRAY_SIZE(ds_report->gyro); i++) { int raw_data = (short)le16_to_cpu(ds_report->gyro[i]); @@ -1030,6 +1121,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) uint8_t max_output_report_size; int ret; + static const struct ps_led_info mute_led_info = { + "micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness + }; + ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL); if (!ds) return ERR_PTR(-ENOMEM); @@ -1107,6 +1202,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) if (ret) goto err; + ret = ps_led_register(ps_dev, &ds->mute_led, &mute_led_info); + if (ret) + goto err; + return &ds->base; err: