All of lore.kernel.org
 help / color / mirror / Atom feed
* Naming of HID LED devices
@ 2021-05-21  5:47 Roderick Colenbrander
  2021-05-21 15:57 ` Marek Behún
  2021-05-21 16:04 ` Pavel Machek
  0 siblings, 2 replies; 6+ messages in thread
From: Roderick Colenbrander @ 2021-05-21  5:47 UTC (permalink / raw)
  To: Benjamin Tissoires, marek.behun
  Cc: linux-input, linux-leds, Daniel J. Ogorchock,
	Barnabás Pőcze, Jiri Kosina

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...

Thanks,
Roderick

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Naming of HID LED devices
  2021-05-21  5:47 Naming of HID LED devices Roderick Colenbrander
@ 2021-05-21 15:57 ` Marek Behún
  2021-05-21 16:12   ` Barnabás Pőcze
  2021-05-21 16:04 ` Pavel Machek
  1 sibling, 1 reply; 6+ messages in thread
From: Marek Behún @ 2021-05-21 15:57 UTC (permalink / raw)
  To: Roderick Colenbrander
  Cc: Benjamin Tissoires, linux-input, linux-leds, Daniel J. Ogorchock,
	Barnabás Pőcze, Jiri Kosina

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Naming of HID LED devices
  2021-05-21  5:47 Naming of HID LED devices Roderick Colenbrander
  2021-05-21 15:57 ` Marek Behún
@ 2021-05-21 16:04 ` Pavel Machek
  2021-05-25  5:55   ` Roderick Colenbrander
  1 sibling, 1 reply; 6+ messages in thread
From: Pavel Machek @ 2021-05-21 16:04 UTC (permalink / raw)
  To: Roderick Colenbrander
  Cc: Benjamin Tissoires, marek.behun, linux-input, linux-leds,
	Daniel J. Ogorchock, Barnabás Pőcze, Jiri Kosina

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

Hi!

> 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.

I understand that problem is that one controller is present as
multiple input devices to userspace.

[That is something you might want to fix, BTW. IIRC input protocol is
flexible enough to do that.]

I suggest you simply select one input device (call it primary,
probably the one that contains the master joystick) and use its input
number....

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Naming of HID LED devices
  2021-05-21 15:57 ` Marek Behún
@ 2021-05-21 16:12   ` Barnabás Pőcze
  0 siblings, 0 replies; 6+ messages in thread
From: Barnabás Pőcze @ 2021-05-21 16:12 UTC (permalink / raw)
  To: Marek Behún
  Cc: Roderick Colenbrander, Benjamin Tissoires, linux-input,
	linux-leds, Daniel J. Ogorchock, Jiri Kosina

Hi


2021. május 21., péntek 17:57 keltezéssel, Marek Behún írta:

> 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


One point was put forward by Benjamin in the following email:

https://lore.kernel.org/linux-input/CAO-hwJ+=_fjHgenXvHv45sHgzwiG2z9vGeq7fmMqj2=BeYCF1Q@mail.gmail.com/


> 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.


Regards,
Barnabás Pőcze

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Naming of HID LED devices
  2021-05-21 16:04 ` Pavel Machek
@ 2021-05-25  5:55   ` Roderick Colenbrander
  2021-05-25 20:04     ` Pavel Machek
  0 siblings, 1 reply; 6+ messages in thread
From: Roderick Colenbrander @ 2021-05-25  5:55 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Benjamin Tissoires, marek.behun, linux-input, linux-leds,
	Daniel J. Ogorchock, Barnabás Pőcze, Jiri Kosina

Hi

On Fri, May 21, 2021 at 9:04 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > 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.
>
> I understand that problem is that one controller is present as
> multiple input devices to userspace.
>
> [That is something you might want to fix, BTW. IIRC input protocol is
> flexible enough to do that.]

[That part is actually non-trivial to fix without an overhaul of the
Linux evdev system. Essentially evdev is a bit limiting for some
devices due to conflicts in use of axes or buttons. This is what
prompted creation of multiple input devices I believe. Though various
HID devices are now also receiving multiple input devices
automatically now based on collections or something. Benjamin and Jiri
are the experts there. Anway that's a major other conversation, people
are trying to steer away from...]

>
> I suggest you simply select one input device (call it primary,
> probably the one that contains the master joystick) and use its input
> number....

It is of course an option. Though I recall in the previous discussion,
technically the LED is registered on the HID device and not on the
input device, so it is not entirely correct. There are also cases I
believe where LEDs are directly created for the HID device itself.
Based on a quick search this includes the 'hid-led' driver. Though its
naming is probably fixed as we may not want to break user space (not
sure if anyone is relying on it). There might be other plain HID
device use cases with LEDs.

> Best regards,
>                                                                 Pavel
> --
> http://www.livejournal.com/~pavelmachek

Thanks,
Roderick

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Naming of HID LED devices
  2021-05-25  5:55   ` Roderick Colenbrander
@ 2021-05-25 20:04     ` Pavel Machek
  0 siblings, 0 replies; 6+ messages in thread
From: Pavel Machek @ 2021-05-25 20:04 UTC (permalink / raw)
  To: Roderick Colenbrander
  Cc: Benjamin Tissoires, marek.behun, linux-input, linux-leds,
	Daniel J. Ogorchock, Barnabás Pőcze, Jiri Kosina

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Hi!

> > I suggest you simply select one input device (call it primary,
> > probably the one that contains the master joystick) and use its input
> > number....
> 
> It is of course an option. Though I recall in the previous discussion,
> technically the LED is registered on the HID device and not on the
> input device, so it is not entirely correct. There are also cases I
> believe where LEDs are directly created for the HID device itself.
> Based on a quick search this includes the 'hid-led' driver. Though its
> naming is probably fixed as we may not want to break user space (not
> sure if anyone is relying on it). There might be other plain HID
> device use cases with LEDs.

I'm not that familiar with HID vs. input differences. We already use
inputX naming for keyboard LEDs.

I'd say lets do that.
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-25 20:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-21  5:47 Naming of HID LED devices Roderick Colenbrander
2021-05-21 15:57 ` Marek Behún
2021-05-21 16:12   ` Barnabás Pőcze
2021-05-21 16:04 ` Pavel Machek
2021-05-25  5:55   ` Roderick Colenbrander
2021-05-25 20:04     ` Pavel Machek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.