Thread (31 messages) 31 messages, 3 authors, 2021-02-16

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

From: Marek Behun <hidden>
Date: 2021-02-15 18:22:37
Also in: linux-input

On Mon, 15 Feb 2021 09:51:15 -0800
Roderick Colenbrander [off-list ref] wrote:
On Mon, Feb 15, 2021 at 7:55 AM Marek Behun [off-list ref] wrote:
quoted
On Mon, 15 Feb 2021 07:36:58 -0800
Roderick Colenbrander [off-list ref] wrote:
 
quoted
Hi Marek,

On Mon, Feb 15, 2021 at 5:31 AM Marek Behun [off-list ref] wrote:  
quoted
On Sun, 14 Feb 2021 16:45:46 -0800
Roderick Colenbrander [off-list ref] wrote:
 
quoted
+     led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
+                     ps_dev->mac_address);  
...  
quoted
+     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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help