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

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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
>