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:indicatorNaming 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