diff mbox series

[v6,1/4] HID: playstation: add DualSense lightbar support

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

Commit Message

Roderick Colenbrander Feb. 15, 2021, 12:45 a.m. UTC
From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Expose the DualSense its RGB lightbar using the new multicolor LED
framework.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |   1 +
 drivers/hid/hid-playstation.c | 118 ++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+)

Comments

Marek Behún Feb. 15, 2021, 1:31 p.m. UTC | #1
On Sun, 14 Feb 2021 16:45:46 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> +	led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> +			ps_dev->mac_address);
...
> +	ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);

The LED subsystem has a predefined schema by which LED names should
look like:
  devicename:color:function
(Not all fields are required, but the order must be preserved. The ':'
 character should be used only as separator of these fields, so not MAC
 addresses in these names, it will confuse userspace parsers.)
See Documentation/leds/leds-class.rst

The devicename part should not be "playstation". It should be something
otherwise recognizable from userspace. For example an mmc indicator has
devicename "mmc0", keyboard capslock LED can have devicename "input0"...

In your case the name should be something like:
  input3:rgb:indicator

Different existing functions are defined in
include/dt-bindings/leds/common.h.

BTW there are extended versions of LED registering functions, suffixed
by "_ext". These accept a struct led_init_data. If a fwnode of the LED
is passed to the registering function via this struct, the LED core
will compose a name for the LED itself. But since your LEDs don't have
device-tree nodes because they are on USB/BlueTooth joysticks, you
either have to compose the name itself like your code is doing now, or
you can propose a patch to the LED core, so that LED core will be able
to compose the LED name even without a device-tree node.

JFI, the function part is (in the future) supposed to somehow define LED
trigger which the system will assign to the LED on probe, but this is
not implemented yet. Currently when the LED has a devicetree node,
the trigger is assigned from the `linux,default-trigger` property, but
the idea is to infer it from the `function` property.

What is this RGB LED supposed to do on the joystick? Is it just for
nice colors? Or should it blink somehow? Can the hardware in the
joystick blink the LED itself? Or maybe fade between colors?

There is for example the pattern LED trigger which changes the LED
brightness by a defined pattern. I am planning to add multicolor
support to this trigger, because our RGB LED controller can offload
such thing to hardware.

Marek
Roderick Colenbrander Feb. 15, 2021, 3:36 p.m. UTC | #2
Hi Marek,

On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Sun, 14 Feb 2021 16:45:46 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > +                     ps_dev->mac_address);
> ...
> > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
>
> The LED subsystem has a predefined schema by which LED names should
> look like:
>   devicename:color:function
> (Not all fields are required, but the order must be preserved. The ':'
>  character should be used only as separator of these fields, so not MAC
>  addresses in these names, it will confuse userspace parsers.)
> See Documentation/leds/leds-class.rst
>
> The devicename part should not be "playstation". It should be something
> otherwise recognizable from userspace. For example an mmc indicator has
> devicename "mmc0", keyboard capslock LED can have devicename "input0"...
>
> In your case the name should be something like:
>   input3:rgb:indicator

Naming is a little bit tricky. The LEDs as well as other sysfs nodes
are added to the 'parent' HID device, not the input devices. In case
of DualSense it is actually implemented as a composite device with
mulitple input devices (gamepad, touchpad and motion sensors) per HID
device. The device name of HID devices seems to be something like:
<bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B

This is I guess why many HID devices in general pick their own names
(and not all have need to have input devices I guess). Though Benjamin
and Jiri know better.

I'm not sure what naming could make sense here. The previous Sony
driver for PlayStation devices used: HID_name "::red" for e.g. red LED
on DualShock 4.

> Different existing functions are defined in
> include/dt-bindings/leds/common.h.
>
> BTW there are extended versions of LED registering functions, suffixed
> by "_ext". These accept a struct led_init_data. If a fwnode of the LED
> is passed to the registering function via this struct, the LED core
> will compose a name for the LED itself. But since your LEDs don't have
> device-tree nodes because they are on USB/BlueTooth joysticks, you
> either have to compose the name itself like your code is doing now, or
> you can propose a patch to the LED core, so that LED core will be able
> to compose the LED name even without a device-tree node.
>
> JFI, the function part is (in the future) supposed to somehow define LED
> trigger which the system will assign to the LED on probe, but this is
> not implemented yet. Currently when the LED has a devicetree node,
> the trigger is assigned from the `linux,default-trigger` property, but
> the idea is to infer it from the `function` property.
>

Thanks for the info. Might be handy in the future.

> What is this RGB LED supposed to do on the joystick? Is it just for
> nice colors? Or should it blink somehow? Can the hardware in the
> joystick blink the LED itself? Or maybe fade between colors?

I mentioned a bit in the other email. These LEDs are under direct
control from PlayStation games. Some may change the color on a per
video frame basis. Use cases include health (green) and when a
character loses health becomes more red/orange and can start flashing.
I have seen games use this for police car and then mixing between blue
and red. Others use it with a static player ID color. The console side
API is literally raw RGB values. There is no hardware blink support.
The previous controllers had it though.

> There is for example the pattern LED trigger which changes the LED
> brightness by a defined pattern. I am planning to add multicolor
> support to this trigger, because our RGB LED controller can offload
> such thing to hardware.
>
> Marek

Thanks,
Roderick
Marek Behún Feb. 15, 2021, 3:55 p.m. UTC | #3
On Mon, 15 Feb 2021 07:36:58 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> Hi Marek,
> 
> On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Sun, 14 Feb 2021 16:45:46 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > +                     ps_dev->mac_address);  
> > ...  
> > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);  
> >
> > The LED subsystem has a predefined schema by which LED names should
> > look like:
> >   devicename:color:function
> > (Not all fields are required, but the order must be preserved. The ':'
> >  character should be used only as separator of these fields, so not MAC
> >  addresses in these names, it will confuse userspace parsers.)
> > See Documentation/leds/leds-class.rst
> >
> > The devicename part should not be "playstation". It should be something
> > otherwise recognizable from userspace. For example an mmc indicator has
> > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> >
> > In your case the name should be something like:
> >   input3:rgb:indicator  
> 
> Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> are added to the 'parent' HID device, not the input devices. In case
> of DualSense it is actually implemented as a composite device with
> mulitple input devices (gamepad, touchpad and motion sensors) per HID
> device. The device name of HID devices seems to be something like:
> <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> 
> This is I guess why many HID devices in general pick their own names
> (and not all have need to have input devices I guess). Though Benjamin
> and Jiri know better.
> 
> I'm not sure what naming could make sense here. The previous Sony
> driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> on DualShock 4.

We have to find a reasonable devicename here. If each joystick registers
multiple input devices, it cannot be "input%d". I suppose there isn't
an API for grouping mulitple input devices toghether into inputgroups.
Maybe it could be in the format "joystick%d".

But I don't think it can be "playstation". Nor can MAC addresses be
there if they contain ':'s.

Marek
Roderick Colenbrander Feb. 15, 2021, 5:51 p.m. UTC | #4
On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 07:36:58 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > Hi Marek,
> >
> > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Sun, 14 Feb 2021 16:45:46 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > > +                     ps_dev->mac_address);
> > > ...
> > > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
> > >
> > > The LED subsystem has a predefined schema by which LED names should
> > > look like:
> > >   devicename:color:function
> > > (Not all fields are required, but the order must be preserved. The ':'
> > >  character should be used only as separator of these fields, so not MAC
> > >  addresses in these names, it will confuse userspace parsers.)
> > > See Documentation/leds/leds-class.rst
> > >
> > > The devicename part should not be "playstation". It should be something
> > > otherwise recognizable from userspace. For example an mmc indicator has
> > > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> > >
> > > In your case the name should be something like:
> > >   input3:rgb:indicator
> >
> > Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> > are added to the 'parent' HID device, not the input devices. In case
> > of DualSense it is actually implemented as a composite device with
> > mulitple input devices (gamepad, touchpad and motion sensors) per HID
> > device. The device name of HID devices seems to be something like:
> > <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> > 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> >
> > This is I guess why many HID devices in general pick their own names
> > (and not all have need to have input devices I guess). Though Benjamin
> > and Jiri know better.
> >
> > I'm not sure what naming could make sense here. The previous Sony
> > driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> > on DualShock 4.
>
> We have to find a reasonable devicename here. If each joystick registers
> multiple input devices, it cannot be "input%d". I suppose there isn't
> an API for grouping mulitple input devices toghether into inputgroups.
> Maybe it could be in the format "joystick%d".

Yeah, there is no inputgroups mechanism.  It could use some type of
joystick name if that's what desired. However, there is no common ID
code. Individual drivers are sometimes calculating their own IDs
(hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
for hid-sony/hid-playstation the use case for tracking IDs is for a
part to prevent duplicate devices as you can connect your device using
both bluetooth and USB. So would be "ps-joystick0"

At the HID layer there does seem to be a unique ID, but it is only
exposed in the name string: This is how the name is constructed:
     dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
             hdev->vendor, hdev->product, atomic_inc_return(&id));

This ID is HID specific, but not all input devices use HID.

I'm not entirely sure what makes sense...


> But I don't think it can be "playstation". Nor can MAC addresses be
> there if they contain ':'s.
>
> Marek
Marek Behún Feb. 15, 2021, 6:21 p.m. UTC | #5
On Mon, 15 Feb 2021 09:51:15 -0800
Roderick Colenbrander <roderick@gaikai.com> wrote:

> On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Mon, 15 Feb 2021 07:36:58 -0800
> > Roderick Colenbrander <roderick@gaikai.com> wrote:
> >  
> > > Hi Marek,
> > >
> > > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:  
> > > >
> > > > On Sun, 14 Feb 2021 16:45:46 -0800
> > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > >  
> > > > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > > > +                     ps_dev->mac_address);  
> > > > ...  
> > > > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);  
> > > >
> > > > The LED subsystem has a predefined schema by which LED names should
> > > > look like:
> > > >   devicename:color:function
> > > > (Not all fields are required, but the order must be preserved. The ':'
> > > >  character should be used only as separator of these fields, so not MAC
> > > >  addresses in these names, it will confuse userspace parsers.)
> > > > See Documentation/leds/leds-class.rst
> > > >
> > > > The devicename part should not be "playstation". It should be something
> > > > otherwise recognizable from userspace. For example an mmc indicator has
> > > > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> > > >
> > > > In your case the name should be something like:
> > > >   input3:rgb:indicator  
> > >
> > > Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> > > are added to the 'parent' HID device, not the input devices. In case
> > > of DualSense it is actually implemented as a composite device with
> > > mulitple input devices (gamepad, touchpad and motion sensors) per HID
> > > device. The device name of HID devices seems to be something like:
> > > <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> > > 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> > >
> > > This is I guess why many HID devices in general pick their own names
> > > (and not all have need to have input devices I guess). Though Benjamin
> > > and Jiri know better.
> > >
> > > I'm not sure what naming could make sense here. The previous Sony
> > > driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> > > on DualShock 4.  
> >
> > We have to find a reasonable devicename here. If each joystick registers
> > multiple input devices, it cannot be "input%d". I suppose there isn't
> > an API for grouping mulitple input devices toghether into inputgroups.
> > Maybe it could be in the format "joystick%d".  
> 
> Yeah, there is no inputgroups mechanism.  It could use some type of
> joystick name if that's what desired. However, there is no common ID
> code. Individual drivers are sometimes calculating their own IDs
> (hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
> for hid-sony/hid-playstation the use case for tracking IDs is for a
> part to prevent duplicate devices as you can connect your device using
> both bluetooth and USB. So would be "ps-joystick0"
> 
> At the HID layer there does seem to be a unique ID, but it is only
> exposed in the name string: This is how the name is constructed:
>      dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
>              hdev->vendor, hdev->product, atomic_inc_return(&id));
> 
> This ID is HID specific, but not all input devices use HID.
> 
> I'm not entirely sure what makes sense...

So all HIDs can be uniqely determined via this atomic_inc_return(&id),
but it is only stored in string form as part of device name.

Send a patch to hid-core to make this atomic_inc_return(&id) also be
stored into struct hid_device as an integer, not only as a part
of the device name string.

Then use "hid%d" as the devicename for this LED, with %d substituted
with this ID.

Marek
Benjamin Tissoires Feb. 16, 2021, 5:29 p.m. UTC | #6
On Mon, Feb 15, 2021 at 7:21 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Mon, 15 Feb 2021 09:51:15 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Mon, 15 Feb 2021 07:36:58 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > Hi Marek,
> > > >
> > > > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > > >
> > > > > On Sun, 14 Feb 2021 16:45:46 -0800
> > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > >
> > > > > > +     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> > > > > > +                     ps_dev->mac_address);
> > > > > ...
> > > > > > +     ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
> > > > >
> > > > > The LED subsystem has a predefined schema by which LED names should
> > > > > look like:
> > > > >   devicename:color:function
> > > > > (Not all fields are required, but the order must be preserved. The ':'
> > > > >  character should be used only as separator of these fields, so not MAC
> > > > >  addresses in these names, it will confuse userspace parsers.)
> > > > > See Documentation/leds/leds-class.rst
> > > > >
> > > > > The devicename part should not be "playstation". It should be something
> > > > > otherwise recognizable from userspace. For example an mmc indicator has
> > > > > devicename "mmc0", keyboard capslock LED can have devicename "input0"...
> > > > >
> > > > > In your case the name should be something like:
> > > > >   input3:rgb:indicator
> > > >
> > > > Naming is a little bit tricky. The LEDs as well as other sysfs nodes
> > > > are added to the 'parent' HID device, not the input devices. In case
> > > > of DualSense it is actually implemented as a composite device with
> > > > mulitple input devices (gamepad, touchpad and motion sensors) per HID
> > > > device. The device name of HID devices seems to be something like:
> > > > <bus>:<vendor_id>:<product_id>:<some other id> e.g. for DualSense USB
> > > > 0003:054C:0CE6.0029 or Bluetooth 0005:054C:0CE6.002B
> > > >
> > > > This is I guess why many HID devices in general pick their own names
> > > > (and not all have need to have input devices I guess). Though Benjamin
> > > > and Jiri know better.
> > > >
> > > > I'm not sure what naming could make sense here. The previous Sony
> > > > driver for PlayStation devices used: HID_name "::red" for e.g. red LED
> > > > on DualShock 4.
> > >
> > > We have to find a reasonable devicename here. If each joystick registers
> > > multiple input devices, it cannot be "input%d". I suppose there isn't
> > > an API for grouping mulitple input devices toghether into inputgroups.
> > > Maybe it could be in the format "joystick%d".
> >
> > Yeah, there is no inputgroups mechanism.  It could use some type of
> > joystick name if that's what desired. However, there is no common ID
> > code. Individual drivers are sometimes calculating their own IDs
> > (hid-nintendo, hid-sony, hid-playstation and xpad I think). At least
> > for hid-sony/hid-playstation the use case for tracking IDs is for a
> > part to prevent duplicate devices as you can connect your device using
> > both bluetooth and USB. So would be "ps-joystick0"
> >
> > At the HID layer there does seem to be a unique ID, but it is only
> > exposed in the name string: This is how the name is constructed:
> >      dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
> >              hdev->vendor, hdev->product, atomic_inc_return(&id));
> >
> > This ID is HID specific, but not all input devices use HID.
> >
> > I'm not entirely sure what makes sense...
>
> So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> but it is only stored in string form as part of device name.

Yes and no. This atomic_inc is only used to allow a sysfs tree,
because you can have several HID devices below the same USB, I2C or
UHID physical device. From the userspace, no-one cares about that ID,
because all HID devices are exported as input, IIO or hidraw nodes.

So using this "id" would not allow for a direct mapping HID device ->
sysfs entry because users will still have to walk through the tree to
find out which is which.

An actual one-to-one mapping would using 'hidrawX' because there is a
one-to-one mapping between /dev/hidrawX for HID devices. However, this
means that we consider the bus to be hidraw which is plain wrong too.

The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
`BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
we standardize LEDs on the HID subsystem to be named
`hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
between the LED and the sysfs, and would also allow users to quickly
filter out the playstation ones.

Cheers,
Benjamin

>
> Send a patch to hid-core to make this atomic_inc_return(&id) also be
> stored into struct hid_device as an integer, not only as a part
> of the device name string.
>
> Then use "hid%d" as the devicename for this LED, with %d substituted
> with this ID.
>
> Marek
>
Marek Behún Feb. 16, 2021, 5:56 p.m. UTC | #7
On Tue, 16 Feb 2021 18:29:46 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> > So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> > but it is only stored in string form as part of device name.  
> 
> Yes and no. This atomic_inc is only used to allow a sysfs tree,
> because you can have several HID devices below the same USB, I2C or
> UHID physical device. From the userspace, no-one cares about that ID,
> because all HID devices are exported as input, IIO or hidraw nodes.
> 
> So using this "id" would not allow for a direct mapping HID device ->
> sysfs entry because users will still have to walk through the tree to
> find out which is which.

So you are saying that the fact that userspace cannot take the number
from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
the problem here.

This is not a problem in my opinion, because userspace can simply
access the parent HID device via /sys/class/leds/hidN:color:func/parent.

In fact we did something similar for LEDs connected to ethernet PHYs.
To summarize:
  - ethernet PHYs are identified by long, sometimes crazy strings like
      d0032004.mdio-mii:01
    or even
      /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
  - for the purposes of having a sane devicename part in LED names, I
    sent this patch
    https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
    which adds a simple incrementing integer ID to each PHY device.
    (The code is not in upstream yet because there is other work needed
     and because I decided that some functionality has to be available
     via a different mechanism, but this part is complete and reviewed.)

> An actual one-to-one mapping would using 'hidrawX' because there is a
> one-to-one mapping between /dev/hidrawX for HID devices. However, this
> means that we consider the bus to be hidraw which is plain wrong too.
> 
> The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
> `BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
> we standardize LEDs on the HID subsystem to be named
> `hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
> between the LED and the sysfs, and would also allow users to quickly
> filter out the playstation ones.

As I wrote in other e-mail some minutes ago, this just means that we
need to wait for other people's opinions. Please do not send this
pull-request with the LED patches until this is resolved.

Marek
Benjamin Tissoires Feb. 16, 2021, 6:14 p.m. UTC | #8
On Tue, Feb 16, 2021 at 6:56 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Tue, 16 Feb 2021 18:29:46 +0100
> Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
> > > So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> > > but it is only stored in string form as part of device name.
> >
> > Yes and no. This atomic_inc is only used to allow a sysfs tree,
> > because you can have several HID devices below the same USB, I2C or
> > UHID physical device. From the userspace, no-one cares about that ID,
> > because all HID devices are exported as input, IIO or hidraw nodes.
> >
> > So using this "id" would not allow for a direct mapping HID device ->
> > sysfs entry because users will still have to walk through the tree to
> > find out which is which.
>
> So you are saying that the fact that userspace cannot take the number
> from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
> the problem here.
>
> This is not a problem in my opinion, because userspace can simply
> access the parent HID device via /sys/class/leds/hidN:color:func/parent.

So in that case, there is no real point at keeping this ID in sync
with anything else? I would be more willing to accept a patch in HID
core that keeps this ID just for HID LEDs, instead of adding just an
ID with no meaning to all HID devices.

Honestly, I think the whole LED class creation API should be
revisited. I guess this is not the first time this problem arises, and
you must be tired of having to chase down users.

If I had to deal with that situation once for all, I would deprecate
the current led class creation API, and add a new API that doesn't
take a free-form string as the name but constrain the name to be
formed by your requirements. This would also send a clear message to
all subsystems because the changes have to be propagated, and then,
all the maintainers would know about this problem. Bonus point, if you
need only "subsystem", "color" and "function", that means that the ID
can be stored internally to the led class and you'll get happy users.

>
> In fact we did something similar for LEDs connected to ethernet PHYs.
> To summarize:
>   - ethernet PHYs are identified by long, sometimes crazy strings like
>       d0032004.mdio-mii:01
>     or even
>       /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
>   - for the purposes of having a sane devicename part in LED names, I
>     sent this patch
>     https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
>     which adds a simple incrementing integer ID to each PHY device.
>     (The code is not in upstream yet because there is other work needed
>      and because I decided that some functionality has to be available
>      via a different mechanism, but this part is complete and reviewed.)
>
> > An actual one-to-one mapping would using 'hidrawX' because there is a
> > one-to-one mapping between /dev/hidrawX for HID devices. However, this
> > means that we consider the bus to be hidraw which is plain wrong too.
> >
> > The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
> > `BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
> > we standardize LEDs on the HID subsystem to be named
> > `hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
> > between the LED and the sysfs, and would also allow users to quickly
> > filter out the playstation ones.
>
> As I wrote in other e-mail some minutes ago, this just means that we
> need to wait for other people's opinions. Please do not send this
> pull-request with the LED patches until this is resolved.
>

Yeah, I just asked Roderick to see if he can revert those patches
while keeping the functionality behind those. I am more concerned
about the micmute button, because we should really offer that feature
to users. The associated LED class has no real benefits for now, so
that code needs a little bit of care instead of a plain revert.

Cheers,
Benjamin
Marek Behún Feb. 16, 2021, 8:28 p.m. UTC | #9
On Tue, 16 Feb 2021 19:14:48 +0100
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> On Tue, Feb 16, 2021 at 6:56 PM Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Tue, 16 Feb 2021 18:29:46 +0100
> > Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >  
> > > > So all HIDs can be uniqely determined via this atomic_inc_return(&id),
> > > > but it is only stored in string form as part of device name.  
> > >
> > > Yes and no. This atomic_inc is only used to allow a sysfs tree,
> > > because you can have several HID devices below the same USB, I2C or
> > > UHID physical device. From the userspace, no-one cares about that ID,
> > > because all HID devices are exported as input, IIO or hidraw nodes.
> > >
> > > So using this "id" would not allow for a direct mapping HID device ->
> > > sysfs entry because users will still have to walk through the tree to
> > > find out which is which.  
> >
> > So you are saying that the fact that userspace cannot take the number
> > from "hidN" string and simply do a lookup /sys/bus/hid/devices/hidN is
> > the problem here.
> >
> > This is not a problem in my opinion, because userspace can simply
> > access the parent HID device via /sys/class/leds/hidN:color:func/parent.  
> 
> So in that case, there is no real point at keeping this ID in sync
> with anything else? I would be more willing to accept a patch in HID
> core that keeps this ID just for HID LEDs, instead of adding just an
> ID with no meaning to all HID devices.

I think there was some misunderstanding.

If there are multiple LEDs on one joystick, all these LEDs should have
the same devicename part of the LED name. Different joysticks should
have different devicename parts of their LEDs names.

As another example think about keyboard LEDs if I have 2 keyboards
  input3:green:numlock
  input3:green:capslock
  input3:green:scrolllock
  input4:green:numlock
  input4:green:capslock
  input4:green:scrolllock

> Honestly, I think the whole LED class creation API should be
> revisited. I guess this is not the first time this problem arises, and
> you must be tired of having to chase down users.

I will not argue with you about this since it is true. The work is
slow though because lack of people and time. I too have some ideas for
the LED subsystem but I also have many other priorities in work. Pavel
has a TODO list in drivers/leds/TODO. The main thing probably is that
it would be great to have more input from other kernel people when
doing something in LEDs, but either not that many people subscribe to
linux-leds mailing list or we should be informing them via different
mechanisms...

> If I had to deal with that situation once for all, I would deprecate
> the current led class creation API, and add a new API that doesn't
> take a free-form string as the name but constrain the name to be
> formed by your requirements. This would also send a clear message to
> all subsystems because the changes have to be propagated, and then,
> all the maintainers would know about this problem. Bonus point, if you
> need only "subsystem", "color" and "function", that means that the ID
> can be stored internally to the led class and you'll get happy users.

As I mentioned in the example above, there can be multiple LEDs for one
devicename...

> >
> > In fact we did something similar for LEDs connected to ethernet PHYs.
> > To summarize:
> >   - ethernet PHYs are identified by long, sometimes crazy strings like
> >       d0032004.mdio-mii:01
> >     or even
> >       /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
> >   - for the purposes of having a sane devicename part in LED names, I
> >     sent this patch
> >     https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2301470.html
> >     which adds a simple incrementing integer ID to each PHY device.
> >     (The code is not in upstream yet because there is other work needed
> >      and because I decided that some functionality has to be available
> >      via a different mechanism, but this part is complete and reviewed.)
> >  
> > > An actual one-to-one mapping would using 'hidrawX' because there is a
> > > one-to-one mapping between /dev/hidrawX for HID devices. However, this
> > > means that we consider the bus to be hidraw which is plain wrong too.
> > >
> > > The unique ID of HID devices (in /sys/bus/hid/devices) is in the form
> > > `BUS:VID:PID.XXXX`. I understand the need to not have colons, so could
> > > we standardize LEDs on the HID subsystem to be named
> > > `hid-bus_vid_pid_xxxx:color:fun(-n)?`? That would allow a mapping
> > > between the LED and the sysfs, and would also allow users to quickly
> > > filter out the playstation ones.  
> >
> > As I wrote in other e-mail some minutes ago, this just means that we
> > need to wait for other people's opinions. Please do not send this
> > pull-request with the LED patches until this is resolved.
> >  
> 
> Yeah, I just asked Roderick to see if he can revert those patches
> while keeping the functionality behind those. I am more concerned
> about the micmute button, because we should really offer that feature
> to users. The associated LED class has no real benefits for now, so
> that code needs a little bit of care instead of a plain revert.

Thank you.

Marek
diff mbox series

Patch

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 54b4eee222f9..cfa29dc17064 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -857,6 +857,7 @@  config HID_PLAYSTATION
 	tristate "PlayStation HID Driver"
 	depends on HID
 	select CRC32
+	select LEDS_CLASS_MULTICOLOR
 	select POWER_SUPPLY
 	help
 	  Provides support for Sony PS5 controllers including support for
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 64193fdeaa0d..97c1118ba78f 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/led-class-multicolor.h>
 #include <linux/module.h>
 
 #include <asm/unaligned.h>
@@ -99,6 +100,10 @@  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_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_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
 
 /* DualSense hardware limits */
 #define DS_ACC_RES_PER_G	8192
@@ -128,6 +133,13 @@  struct dualsense {
 	uint8_t motor_left;
 	uint8_t motor_right;
 
+	/* RGB lightbar */
+	struct led_classdev_mc lightbar;
+	bool update_lightbar;
+	uint8_t lightbar_red;
+	uint8_t lightbar_green;
+	uint8_t lightbar_blue;
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -473,6 +485,45 @@  static int ps_get_report(struct hid_device *hdev, uint8_t report_id, uint8_t *bu
 	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))
+{
+	struct hid_device *hdev = ps_dev->hdev;
+	struct mc_subled *mc_led_info;
+	struct led_classdev *led_cdev;
+	int ret;
+
+	mc_led_info = devm_kmalloc_array(&hdev->dev, 3, sizeof(*mc_led_info),
+					 GFP_KERNEL | __GFP_ZERO);
+	if (!mc_led_info)
+		return -ENOMEM;
+
+	mc_led_info[0].color_index = LED_COLOR_ID_RED;
+	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+
+	lightbar_mc_dev->subled_info = mc_led_info;
+	lightbar_mc_dev->num_colors = 3;
+
+	led_cdev = &lightbar_mc_dev->led_cdev;
+	led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
+			ps_dev->mac_address);
+	if (!led_cdev->name)
+		return -ENOMEM;
+	led_cdev->brightness = 255;
+	led_cdev->max_brightness = 255;
+	led_cdev->brightness_set_blocking = brightness_set;
+
+	ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
+	if (ret < 0) {
+		hid_err(hdev, "Cannot register multicolor LED device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
 		int gyro_range, int gyro_res)
 {
@@ -651,6 +702,26 @@  static int dualsense_get_mac_address(struct dualsense *ds)
 	return ret;
 }
 
+static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
+	enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct dualsense *ds = container_of(mc_cdev, struct dualsense, lightbar);
+	unsigned long flags;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+	ds->update_lightbar = true;
+	ds->lightbar_red = mc_cdev->subled_info[0].brightness;
+	ds->lightbar_green = mc_cdev->subled_info[1].brightness;
+	ds->lightbar_blue = mc_cdev->subled_info[2].brightness;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	schedule_work(&ds->output_worker);
+	return 0;
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -734,6 +805,15 @@  static void dualsense_output_worker(struct work_struct *work)
 		ds->update_rumble = false;
 	}
 
+	if (ds->update_lightbar) {
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE;
+		common->lightbar_red = ds->lightbar_red;
+		common->lightbar_green = ds->lightbar_green;
+		common->lightbar_blue = ds->lightbar_blue;
+
+		ds->update_lightbar = false;
+	}
+
 	spin_unlock_irqrestore(&ds->base.lock, flags);
 
 	dualsense_send_output_report(ds, &report);
@@ -918,6 +998,31 @@  static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
 	return 0;
 }
 
+static int dualsense_reset_leds(struct dualsense *ds)
+{
+	struct dualsense_output_report report;
+	uint8_t *buf;
+
+	buf = kzalloc(sizeof(struct dualsense_output_report_bt), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	dualsense_init_output_report(ds, &report, buf);
+	/*
+	 * On Bluetooth the DualSense outputs an animation on the lightbar
+	 * during startup and maintains a color afterwards. We need to explicitly
+	 * reconfigure the lightbar before we can do any programming later on.
+	 * In USB the lightbar is not on by default, but redoing the setup there
+	 * doesn't hurt.
+	 */
+	report.common->valid_flag2 = DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE;
+	report.common->lightbar_setup = DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT; /* Fade light out. */
+	dualsense_send_output_report(ds, &report);
+
+	kfree(buf);
+	return 0;
+}
+
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
@@ -989,6 +1094,19 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret)
 		goto err;
 
+	/*
+	 * The hardware may have control over the LEDs (e.g. in Bluetooth on startup).
+	 * Reset the LEDs (lightbar, mute, player leds), so we can control them
+	 * from software.
+	 */
+	ret = dualsense_reset_leds(ds);
+	if (ret)
+		goto err;
+
+	ret = ps_lightbar_register(ps_dev, &ds->lightbar, dualsense_lightbar_set_brightness);
+	if (ret)
+		goto err;
+
 	return &ds->base;
 
 err: