On Mon, 15 Feb 2021 09:51:15 -0800 Roderick Colenbrander <roderick@xxxxxxxxxx> wrote: > On Mon, Feb 15, 2021 at 7:55 AM Marek Behun <marek.behun@xxxxxx> wrote: > > > > On Mon, 15 Feb 2021 07:36:58 -0800 > > Roderick Colenbrander <roderick@xxxxxxxxxx> wrote: > > > > > Hi Marek, > > > > > > On Mon, Feb 15, 2021 at 5:31 AM Marek Behun <marek.behun@xxxxxx> wrote: > > > > > > > > On Sun, 14 Feb 2021 16:45:46 -0800 > > > > Roderick Colenbrander <roderick@xxxxxxxxxx> 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
