| Message ID | 20210215004549.135251-1-roderick@gaikai.com |
|---|---|
| Headers | show |
| Series | HID: new driver for PS5 'DualSense' controller | expand |
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
On Sun, 14 Feb 2021 16:45:45 -0800 Roderick Colenbrander <roderick@gaikai.com> wrote: > From: Roderick Colenbrander <roderick.colenbrander@sony.com> > > Hi, > > Recently Sony released a HID driver for the new PlayStation 5 controller > 'DualSense'. Pavel Machek noticed the driver got staged for "-next" and asked > me to share the LED patches to linux-leds as well. > > The LED patches I'm sharing are patch 6, 9, 11, 12 from the v6 hid-playstation > series as originally posted to linux-input. The driver in its full form can be > found on "hid.git/log/?h=for-5.12/playstation". > Hi, OK I see you described the purpose of these LEDs here, please ignore that one question in my reply to patch 1/4. > Finally, the DualSense has a audio mute LED and a mute button. The mute button is > expected to mute the internal microphone of the DualSense. The mute behavior > is handled driver side and the driver then also programs the LED. From user space > perspective the LED is read-only. The audio mute LED should not be read-only from userspace. Instead a LED trigger should be assigned by default, audio-micmute / audio-mute. With this trigger the LED subsystem will handle setting brightness of the LED according to whether the audio is muted or not. This trigger is currently simple, though. It is system wide - it is impossible to configure it to report only on the state of a specific microphone. But the trigger driver can be extended if this is needed. Marek
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
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
On Mon, 15 Feb 2021 15:00:30 -0800 Roderick Colenbrander <roderick@gaikai.com> wrote: > What is the desired naming for these player LEDs? There is not an > officially designed function based on DT bindings. So far they used > "playstation::mac::ledX". When changing the naming scheme towards > "hid" and removing MAC, they would be: "hid%d::led1" etcetera. Hi, there is one more thing I forgot to mention in the LED name schema: devicename:color:function-functionEnumerator So LED core can for example compose a names in the format: switch0:green:lan-1 switch0:green:lan-2 switch0:green:lan-3 switch0:green:lan-4 In your case I think the most appropriate name would be something like hid0:color:indicator-1 hid0:color:indicator-2 ... Are these LEDs of different colors which are impossible to determine? The string "hid%d::led1" you mention above does not indicate color. Marek
On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Mon, 15 Feb 2021 15:00:30 -0800 > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > What is the desired naming for these player LEDs? There is not an > > officially designed function based on DT bindings. So far they used > > "playstation::mac::ledX". When changing the naming scheme towards > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera. > > Hi, > > there is one more thing I forgot to mention in the LED name schema: > devicename:color:function-functionEnumerator > > So LED core can for example compose a names in the format: > switch0:green:lan-1 > switch0:green:lan-2 > switch0:green:lan-3 > switch0:green:lan-4 > > In your case I think the most appropriate name would be something like > hid0:color:indicator-1 > hid0:color:indicator-2 > ... I am trying to think if indicator is clear enough. Currently devices use a mixture of names, which is obviously bad (wiimote uses p1-p4 at the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses player1-4). I would at least like new drivers to standardize. In particular in Android frameworks we have a need to map these LEDs back to the Java InputDevice. Finding the LEDs has been quite painful so far. If this is what is decided, I guess we should update the Linux gamepad document at some point as well. > Are these LEDs of different colors which are impossible to determine? > The string "hid%d::led1" you mention above does not indicate color. The DualSense LEDs are all white (at least so far?). On controllers from other brands I have seen them be red or green. So could indeed use: "hid%d:white". > Marek Thanks, Roderick
On Mon, 15 Feb 2021 17:11:14 -0800 Roderick Colenbrander <roderick@gaikai.com> wrote: > On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Mon, 15 Feb 2021 15:00:30 -0800 > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > What is the desired naming for these player LEDs? There is not an > > > officially designed function based on DT bindings. So far they used > > > "playstation::mac::ledX". When changing the naming scheme towards > > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera. > > > > Hi, > > > > there is one more thing I forgot to mention in the LED name schema: > > devicename:color:function-functionEnumerator > > > > So LED core can for example compose a names in the format: > > switch0:green:lan-1 > > switch0:green:lan-2 > > switch0:green:lan-3 > > switch0:green:lan-4 > > > > In your case I think the most appropriate name would be something like > > hid0:color:indicator-1 > > hid0:color:indicator-2 > > ... > > I am trying to think if indicator is clear enough. Currently devices > use a mixture of names, which is obviously bad (wiimote uses p1-p4 at > the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses > player1-4). I would at least like new drivers to standardize. In > particular in Android frameworks we have a need to map these LEDs back > to the Java InputDevice. Finding the LEDs has been quite painful so > far. Thinking about it more, function "player" should theoretically be reasonable. Maybe we should try sending a patch for review, adding this funciton to include/dt-bindings/leds/common.h, and see what others think of it... > If this is what is decided, I guess we should update the Linux gamepad > document at some point as well. > > > Are these LEDs of different colors which are impossible to determine? > > The string "hid%d::led1" you mention above does not indicate color. > > The DualSense LEDs are all white (at least so far?). On controllers > from other brands I have seen them be red or green. So could indeed > use: "hid%d:white". Yes, a constant for white color is defined in headers. > > Marek > > Thanks, > Roderick
On Tue, Feb 16, 2021 at 3:37 AM Marek Behun <marek.behun@nic.cz> wrote: > > On Mon, 15 Feb 2021 17:11:14 -0800 > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > On Mon, 15 Feb 2021 15:00:30 -0800 > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > What is the desired naming for these player LEDs? There is not an > > > > officially designed function based on DT bindings. So far they used > > > > "playstation::mac::ledX". When changing the naming scheme towards > > > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera. > > > > > > Hi, > > > > > > there is one more thing I forgot to mention in the LED name schema: > > > devicename:color:function-functionEnumerator > > > > > > So LED core can for example compose a names in the format: > > > switch0:green:lan-1 > > > switch0:green:lan-2 > > > switch0:green:lan-3 > > > switch0:green:lan-4 > > > > > > In your case I think the most appropriate name would be something like > > > hid0:color:indicator-1 > > > hid0:color:indicator-2 > > > ... > > > > I am trying to think if indicator is clear enough. Currently devices > > use a mixture of names, which is obviously bad (wiimote uses p1-p4 at > > the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses > > player1-4). I would at least like new drivers to standardize. In > > particular in Android frameworks we have a need to map these LEDs back > > to the Java InputDevice. Finding the LEDs has been quite painful so > > far. > > Thinking about it more, function "player" should theoretically be > reasonable. Maybe we should try sending a patch for review, adding this > funciton to include/dt-bindings/leds/common.h, and see what others > think of it... FWIW, I am not sure 'player' would be a good fit here. I personally preferred "indicator". My reasons are because those LEDs are basically a matrix of LEDs, and are supposed to be read as a whole. For player 1, you would light up the 3rd LED only. And for player 2, you would light up LEDS #2 and #4. So I would say that in this particular case, "player" would lead to confusion because users would want to set player 1 on the controller and would have to talk to the "player-3" LED. If we keep the more generic "indicator", the one-to-one mapping is removed, and it's clearer that userspace needs an adaptor to convert "players" into "indicator". For the older controllers with a dedicated "player" LED, like the PS3 and the Wiimote, "player" would make sense, yes. Cheers, Benjamin > > > If this is what is decided, I guess we should update the Linux gamepad > > document at some point as well. > > > > > Are these LEDs of different colors which are impossible to determine? > > > The string "hid%d::led1" you mention above does not indicate color. > > > > The DualSense LEDs are all white (at least so far?). On controllers > > from other brands I have seen them be red or green. So could indeed > > use: "hid%d:white". > > Yes, a constant for white color is defined in headers. > > > > Marek > > > > Thanks, > > Roderick >
On 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 >
On Tue, 16 Feb 2021 18:19:49 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Tue, Feb 16, 2021 at 3:37 AM Marek Behun <marek.behun@nic.cz> wrote: > > > > On Mon, 15 Feb 2021 17:11:14 -0800 > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > On Mon, Feb 15, 2021 at 4:33 PM Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > On Mon, 15 Feb 2021 15:00:30 -0800 > > > > Roderick Colenbrander <roderick@gaikai.com> wrote: > > > > > > > > > What is the desired naming for these player LEDs? There is not an > > > > > officially designed function based on DT bindings. So far they used > > > > > "playstation::mac::ledX". When changing the naming scheme towards > > > > > "hid" and removing MAC, they would be: "hid%d::led1" etcetera. > > > > > > > > Hi, > > > > > > > > there is one more thing I forgot to mention in the LED name schema: > > > > devicename:color:function-functionEnumerator > > > > > > > > So LED core can for example compose a names in the format: > > > > switch0:green:lan-1 > > > > switch0:green:lan-2 > > > > switch0:green:lan-3 > > > > switch0:green:lan-4 > > > > > > > > In your case I think the most appropriate name would be something like > > > > hid0:color:indicator-1 > > > > hid0:color:indicator-2 > > > > ... > > > > > > I am trying to think if indicator is clear enough. Currently devices > > > use a mixture of names, which is obviously bad (wiimote uses p1-p4 at > > > the end, sony uses sony1-4 for DualShock 3, hid-nintendo uses > > > player1-4). I would at least like new drivers to standardize. In > > > particular in Android frameworks we have a need to map these LEDs back > > > to the Java InputDevice. Finding the LEDs has been quite painful so > > > far. > > > > Thinking about it more, function "player" should theoretically be > > reasonable. Maybe we should try sending a patch for review, adding this > > funciton to include/dt-bindings/leds/common.h, and see what others > > think of it... > > FWIW, I am not sure 'player' would be a good fit here. I personally > preferred "indicator". > My reasons are because those LEDs are basically a matrix of LEDs, and > are supposed to be read as a whole. For player 1, you would light up > the 3rd LED only. And for player 2, you would light up LEDS #2 and #4. > > So I would say that in this particular case, "player" would lead to > confusion because users would want to set player 1 on the controller > and would have to talk to the "player-3" LED. > > If we keep the more generic "indicator", the one-to-one mapping is > removed, and it's clearer that userspace needs an adaptor to convert > "players" into "indicator". > > For the older controllers with a dedicated "player" LED, like the PS3 > and the Wiimote, "player" would make sense, yes. OK, in that case "indicator" is more reasonable. But all this means that this should simply be discussed more on LED mailing list. We need to wait till other people express their opinions. Marek
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
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
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
From: Roderick Colenbrander <roderick.colenbrander@sony.com> Hi, Recently Sony released a HID driver for the new PlayStation 5 controller 'DualSense'. Pavel Machek noticed the driver got staged for "-next" and asked me to share the LED patches to linux-leds as well. The LED patches I'm sharing are patch 6, 9, 11, 12 from the v6 hid-playstation series as originally posted to linux-input. The driver in its full form can be found on "hid.git/log/?h=for-5.12/playstation". A little bit of background on the DualSense controller. It is a generic HID gamepad (e.g. buttons and sticks) with a number of additional features, such as Motion Sensors, audio, LEDs and others. All features are implemented using HID input and output reports. In regards to LEDs it has 3 types of LEDs. First, it has a RGB lightbar which is used in PS4/PS5 games for various effects e.g. health indicator, police siren, player color and other use cases. This patch series exposes it using the multicolor class, which is great for our use case as the lightbar state is sent using a single HID output report. Previously for our DualShock 4 controller, the driver exposed individual LEDs, which wasn't great due to race conditions as games change LED state frequently. The second LED type, is a row of 5 player indicator LEDs. Such LEDs are common on gamepads to set a player number e.g. 1, 2, 4 etcetera. Each LED is exposed as a single LED. A default player id is set based on the number of connected controllers, which is common behavior within the input system. Finally, the DualSense has a audio mute LED and a mute button. The mute button is expected to mute the internal microphone of the DualSense. The mute behavior is handled driver side and the driver then also programs the LED. From user space perspective the LED is read-only. Let me know if there are any questions or comments. Thanks, Roderick Colenbrander Sony Interactive Entertainment, LLC Roderick Colenbrander (13): HID: playstation: add DualSense lightbar support HID: playstation: add microphone mute support for DualSense. HID: playstation: add DualSense player LEDs support. HID: playstation: DualSense set LEDs to default player id. MAINTAINERS | 6 + drivers/hid/Kconfig | 21 + drivers/hid/Makefile | 1 + drivers/hid/hid-ids.h | 1 + drivers/hid/hid-playstation.c | 1504 +++++++++++++++++++++++++++++++++ 5 files changed, 1533 insertions(+) create mode 100644 drivers/hid/hid-playstation.c