All of lore.kernel.org
 help / color / mirror / Atom feed
From: Roderick Colenbrander <thunderbird2k@gmail.com>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>, marek.behun@nic.cz
Cc: linux-input <linux-input@vger.kernel.org>,
	linux-leds@vger.kernel.org,
	"Daniel J. Ogorchock" <djogorchock@gmail.com>,
	"Barnabás Pőcze" <pobrn@protonmail.com>,
	"Jiri Kosina" <jikos@kernel.org>
Subject: Naming of HID LED devices
Date: Thu, 20 May 2021 22:47:08 -0700	[thread overview]
Message-ID: <CAEc3jaCfS=DPQiSjh+_aVePbUXHe-M7WH1t+JtSLwqu0Vktnxw@mail.gmail.com> (raw)

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

             reply	other threads:[~2021-05-21  5:47 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-21  5:47 Roderick Colenbrander [this message]
2021-05-21 15:57 ` Naming of HID LED devices 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEc3jaCfS=DPQiSjh+_aVePbUXHe-M7WH1t+JtSLwqu0Vktnxw@mail.gmail.com' \
    --to=thunderbird2k@gmail.com \
    --cc=benjamin.tissoires@redhat.com \
    --cc=djogorchock@gmail.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=marek.behun@nic.cz \
    --cc=pobrn@protonmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.