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

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@gmail.com, pobm@protonmail.com,
	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 14:31:44 +0100	[thread overview]
Message-ID: <20210215143144.060fdbe6@nic.cz> (raw)
In-Reply-To: <20210215004549.135251-2-roderick@gaikai.com>

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

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.

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?

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