diff mbox series

[v6,2/4] HID: playstation: add microphone mute support for DualSense.

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

Commit Message

Roderick Colenbrander Feb. 15, 2021, 12:45 a.m. UTC
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(+)

Comments

Marek Behún Feb. 15, 2021, 2:40 p.m. UTC | #1
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
Roderick Colenbrander Feb. 15, 2021, 6:07 p.m. UTC | #2
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
Marek Behún Feb. 15, 2021, 6:17 p.m. UTC | #3
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
Roderick Colenbrander Feb. 16, 2021, 8:33 a.m. UTC | #4
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
Marek Behún Feb. 16, 2021, 4:41 p.m. UTC | #5
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
Benjamin Tissoires Feb. 16, 2021, 5:12 p.m. UTC | #6
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
Marek Behún Feb. 16, 2021, 5:21 p.m. UTC | #7
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
Marek Behún Feb. 16, 2021, 5:40 p.m. UTC | #8
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
Benjamin Tissoires Feb. 16, 2021, 5:42 p.m. UTC | #9
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
>
Marek Behún Feb. 16, 2021, 6 p.m. UTC | #10
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 mbox series

Patch

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: