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

[Roderick Colenbrander <roderick@gaikai.com>]

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.

> Different existing functions are defined in
> include/dt-bindings/leds/common.h.
>
> BTW there are extended versions of LED registering functions, suffixed
> by "_ext". These accept a struct led_init_data. If a fwnode of the LED
> is passed to the registering function via this struct, the LED core
> will compose a name for the LED itself. But since your LEDs don't have
> device-tree nodes because they are on USB/BlueTooth joysticks, you
> either have to compose the name itself like your code is doing now, or
> you can propose a patch to the LED core, so that LED core will be able
> to compose the LED name even without a device-tree node.
>
> JFI, the function part is (in the future) supposed to somehow define LED
> trigger which the system will assign to the LED on probe, but this is
> not implemented yet. Currently when the LED has a devicetree node,
> the trigger is assigned from the `linux,default-trigger` property, but
> the idea is to infer it from the `function` property.
>

Thanks for the info. Might be handy in the future.

> What is this RGB LED supposed to do on the joystick? Is it just for
> nice colors? Or should it blink somehow? Can the hardware in the
> joystick blink the LED itself? Or maybe fade between colors?

I mentioned a bit in the other email. These LEDs are under direct
control from PlayStation games. Some may change the color on a per
video frame basis. Use cases include health (green) and when a
character loses health becomes more red/orange and can start flashing.
I have seen games use this for police car and then mixing between blue
and red. Others use it with a static player ID color. The console side
API is literally raw RGB values. There is no hardware blink support.
The previous controllers had it though.

> There is for example the pattern LED trigger which changes the LED
> brightness by a defined pattern. I am planning to add multicolor
> support to this trigger, because our RGB LED controller can offload
> such thing to hardware.
>
> Marek

Thanks,
Roderick