Re: Naming of HID LED devices

["Marek Behún" <marek.behun@nic.cz>]

On Thu, 20 May 2021 22:47:08 -0700
Roderick Colenbrander <thunderbird2k@gmail.com> wrote:

> Hi Benjamin and Marek,
> 
> Earlier this year during review of the hid-playstation driver there
> was a discussion on the naming of LEDs exposed by HID drivers. Moving
> forward the preference from the LED maintainers was to follow the
> naming scheme "device:color:function" instead of the custom names used
> so far by HID drivers.
> 
> I would like to get some guidance on the naming direction not just for
> hid-playstation, but Daniel's hid-nintendo driver for which he posted
> a new revision today has the same problem.
> 
> The original discussion was on "why not use the input device name?"
> (e.g. input15). It was concluded that it wouldn't uniquely identify a
> HID device among reasons.
> 
> One suggested approach by Benjamin was to use the sysfs unique name
> with the bus, vid, pid.. but without ":" or ".":
> > > > 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.  
> 
> Another approach mentioned was to invent some new ID and use a name
> like "hidN":
> > > So you are saying that the fact that userspace cannot take the
> > > number from "hidN" string and simply do a lookup
> > > /sys/bus/hid/devices/hidN is the problem here.
> > >
> > > This is not a problem in my opinion, because userspace can simply
> > > access the parent HID device via
> > > /sys/class/leds/hidN:color:func/parent.  
> >
> > So in that case, there is no real point at keeping this ID in sync
> > with anything else? I would be more willing to accept a patch in HID
> > core that keeps this ID just for HID LEDs, instead of adding just an
> > ID with no meaning to all HID devices.  
> 
> I'm not sure which approach would be prefered. A "hidN" approach would
> have little meaning perhaps, but looks pretty. While the
> "hid-bus_vid_pid_xxxx" has a real meaning, but looks less nice. Unless
> there is another approach as well.
> 
> Then there is the question on how to best generate these names. The
> "hidN" approach could leverage the XXXX id an store it internally
> (though it doesn't have a real meaning). If we only want to allocate
> such an ID for devices with LEDs then some flag would need to be
> passed back to hid-core. Not sure what the best way would be (almost a
> call like hid_hw_start as part of connect_mask unless there is a
> better way).
> 
> A hid-bus string is easier to create. Though even there is a question
> on how to do it. It would be wasteful to store it for each hid_device.
> It could be generated using a helper function out of
> "dev_name(hdev->dev)", though personally I dislike any string
> manipulation kernel side if it can be avoided. I would probably
> suggest to store "XXXX" in each hid_device struct and have users (so
> far would only be hid-nintendo and hid-playstation) generate the
> strings themselves for now. Again also not nice unless a
> "hid_device_name()" helper is desired then...

Since it was some time ago I don't quite remember what the exact
problem was with the suggestion I had about using the ID from the id
variable in hid_add_device() function in hid-core.c.

The code does:

  int hid_add_device(struct hid_device *hdev)
  {
    static atomic_t id = ATOMIC_INIT(0);
    ...
    dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
                 hdev->vendor, hdev->product, atomic_inc_return(&id));
    ...
  }

The id variable is static and atomic, so it is unique for every
hid_device. Why cannot we use this?

Marek