From: Marek Behun <marek.behun@nic.cz>
To: Roderick Colenbrander <roderick@gaikai.com>
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Pavel Machek <pavel@ucw.cz>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
linux-leds@vger.kernel.org,
Roderick Colenbrander <roderick.colenbrander@sony.com>
Subject: Re: [PATCH v6 1/4] HID: playstation: add DualSense lightbar support
Date: Mon, 15 Feb 2021 19:21:24 +0100 [thread overview]
Message-ID: <20210215192124.7a6c8c9d@nic.cz> (raw)
In-Reply-To: <CANndSKkVAFJzf58CNYw_j0QY7hd4umOMn5Cs6U3JnK6TozWdEQ@mail.gmail.com>
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.
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
next prev parent reply other threads:[~2021-02-15 18:22 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-15 0:45 [PATCH v6 00/4] HID: new driver for PS5 'DualSense' controller Roderick Colenbrander
2021-02-15 0:45 ` [PATCH v6 1/4] HID: playstation: add DualSense lightbar support Roderick Colenbrander
2021-02-15 13:31 ` Marek Behun
2021-02-15 15:36 ` Roderick Colenbrander
2021-02-15 15:55 ` Marek Behun
2021-02-15 17:51 ` Roderick Colenbrander
2021-02-15 18:21 ` Marek Behun [this message]
2021-02-16 17:29 ` Benjamin Tissoires
2021-02-16 17:56 ` Marek Behun
2021-02-16 18:14 ` Benjamin Tissoires
2021-02-16 20:28 ` Marek Behun
2021-02-15 0:45 ` [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense Roderick Colenbrander
2021-02-15 14:40 ` Marek Behun
2021-02-15 18:07 ` Roderick Colenbrander
2021-02-15 18:17 ` Marek Behun
2021-02-16 8:33 ` Roderick Colenbrander
2021-02-16 16:41 ` Marek Behun
2021-02-16 17:12 ` Benjamin Tissoires
2021-02-16 17:21 ` Marek Behun
2021-02-16 17:40 ` Marek Behun
2021-02-16 17:42 ` Benjamin Tissoires
2021-02-16 18:00 ` Marek Behun
2021-02-15 0:45 ` [PATCH v6 3/4] HID: playstation: add DualSense player LEDs support Roderick Colenbrander
2021-02-15 23:00 ` Roderick Colenbrander
2021-02-16 0:33 ` Marek Behun
2021-02-16 1:11 ` Roderick Colenbrander
2021-02-16 2:37 ` Marek Behun
2021-02-16 17:19 ` Benjamin Tissoires
2021-02-16 17:43 ` Marek Behun
2021-02-15 0:45 ` [PATCH v6 4/4] HID: playstation: DualSense set LEDs to default player id Roderick Colenbrander
2021-02-15 14:29 ` [PATCH v6 00/4] HID: new driver for PS5 'DualSense' controller Marek Behun
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=20210215192124.7a6c8c9d@nic.cz \
--to=marek.behun@nic.cz \
--cc=benjamin.tissoires@redhat.com \
--cc=dmitry.torokhov@gmail.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=roderick.colenbrander@sony.com \
--cc=roderick@gaikai.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).