On Mon, Feb 15, 2021 at 7:21 PM Marek Behun [off-list ref] wrote:

On Mon, 15 Feb 2021 09:51:15 -0800
Roderick Colenbrander [off-list ref] wrote:

quoted

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.

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