Hi Barnabás and Daniel,
I agree with Barnabás in regards to LED naming. Earlier this year for
the new "hid-playstation" driver we ran into issues in regards to the
naming scheme of LEDs moving forward (see some of the archived
threads). I still have to address this for "hid-playstation", but
didn't get to submitting the changes yet due to paternity leave. I did
implement a proof-of-concept, which I can probably clean up over the
next few days.
Basically the LED team would like LEDs to follow the naming scheme
"devicename:color:function" instead of legacy names containing mac
addresses or other strings.
There are 2 issues. One is the "devicename", where there is not
necessarily a clear mapping in particular for device with multiple
functions such as DualShock 4 / DualSense / Joycons. The suggested
name was to use the 'hid device name' something with the bus, device
etcetera numbers in there. I need to check the email thread on what
Benjamin suggested there.
The second issue is the "function" part of the the naming scheme. The
current official naming is like "LED_FUNCTION_DISK" and others as
defined by "include/dt-bindings/leds/common.h". I was proposing to
have a new player type "LED_FUNCTION_PLAYER", which is what the
Nintendo controller would need as well.
I will try to clean up PoC version of my patches and include you on
the thread as well.
Thanks,
Roderick
On Thu, May 20, 2021 at 4:35 PM Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
>
> Hi
>
> please cc the linux-leds mailing list and the appropriate subsystem maintainers
> at least on the relevant patches.
>
>
> 2021. május 21., péntek 0:47 keltezéssel, Daniel J. Ogorchock írta:
>
> > This patch adds led_classdev functionality to the switch controller
> > driver. It adds support for the 4 player LEDs. The Home Button LED still
> > needs to be supported on the pro controllers and right joy-con.
> >
> > Signed-off-by: Daniel J. Ogorchock <djogorchock@xxxxxxxxx>
> > ---
> > drivers/hid/Kconfig | 2 +
> > drivers/hid/hid-nintendo.c | 95 +++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 1f23e84f8bdf3..86a6129d3d89f 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -722,6 +722,8 @@ config HID_MULTITOUCH
> > config HID_NINTENDO
> > tristate "Nintendo Joy-Con and Pro Controller support"
> > depends on HID
> > + depends on NEW_LEDS
> > + depends on LEDS_CLASS
> > help
> > Adds support for the Nintendo Switch Joy-Cons and Pro Controller.
> > All controllers support bluetooth, and the Pro Controller also supports
> > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> > index b6c0e5e36d8b0..c21682b4a2e62 100644
> > --- a/drivers/hid/hid-nintendo.c
> > +++ b/drivers/hid/hid-nintendo.c
> > @@ -25,6 +25,7 @@
> > #include <linux/device.h>
> > #include <linux/hid.h>
> > #include <linux/input.h>
> > +#include <linux/leds.h>
> > #include <linux/module.h>
> > #include <linux/spinlock.h>
> >
> > @@ -183,11 +184,13 @@ struct joycon_input_report {
> > } __packed;
> >
> > #define JC_MAX_RESP_SIZE (sizeof(struct joycon_input_report) + 35)
> > +#define JC_NUM_LEDS 4
>
> I think it'd be better if you could guarantee that `JC_NUM_LEDS` and
> size of the `joycon_player_led_names` won't go out of sync. E.g.
> define `JC_NUM_LEDS` in terms of ARRAY_SIZE(), use static_assert(), etc.
>
>
> >
> > /* Each physical controller is associated with a joycon_ctlr struct */
> > struct joycon_ctlr {
> > struct hid_device *hdev;
> > struct input_dev *input;
> > + struct led_classdev leds[JC_NUM_LEDS];
> > enum joycon_ctlr_state ctlr_state;
> >
> > /* The following members are used for synchronous sends/receives */
> > @@ -553,11 +556,9 @@ static const unsigned int joycon_dpad_inputs_jc[] = {
> > BTN_DPAD_UP, BTN_DPAD_DOWN, BTN_DPAD_LEFT, BTN_DPAD_RIGHT,
> > };
> >
> > -static DEFINE_MUTEX(joycon_input_num_mutex);
> > static int joycon_input_create(struct joycon_ctlr *ctlr)
> > {
> > struct hid_device *hdev;
> > - static int input_num = 1;
> > const char *name;
> > int ret;
> > int i;
> > @@ -643,6 +644,66 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
> > if (ret)
> > return ret;
> >
> > + return 0;
> > +}
> > +
> > +static int joycon_player_led_brightness_set(struct led_classdev *led,
> > + enum led_brightness brightness)
> > +{
> > + struct device *dev = led->dev->parent;
> > + struct hid_device *hdev = to_hid_device(dev);
> > + struct joycon_ctlr *ctlr;
> > + int val = 0;
> > + int i;
> > + int ret;
> > + int num;
> > +
> > + ctlr = hid_get_drvdata(hdev);
> > + if (!ctlr) {
> > + hid_err(hdev, "No controller data\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* determine which player led this is */
> > + for (num = 0; num < JC_NUM_LEDS; num++) {
> > + if (&ctlr->leds[num] == led)
> > + break;
> > + }
> > + if (num >= JC_NUM_LEDS)
> > + return -EINVAL;
> > +
> > + mutex_lock(&ctlr->output_mutex);
> > + for (i = 0; i < JC_NUM_LEDS; i++) {
> > + if (i == num)
> > + val |= brightness << i;
> > + else
> > + val |= ctlr->leds[i].brightness << i;
> > + }
> > + ret = joycon_set_player_leds(ctlr, 0, val);
> > + mutex_unlock(&ctlr->output_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static const char * const joycon_player_led_names[] = {
> > + "player1",
> > + "player2",
> > + "player3",
> > + "player4"
>
> Small thing: after non-sentinel values at the end the comma is usually not omitted.
>
>
> > +};
> > +
> > +static DEFINE_MUTEX(joycon_input_num_mutex);
> > +static int joycon_player_leds_create(struct joycon_ctlr *ctlr)
> > +{
> > + struct hid_device *hdev = ctlr->hdev;
> > + struct device *dev = &hdev->dev;
> > + const char *d_name = dev_name(dev);
> > + struct led_classdev *led;
> > + char *name;
> > + int ret = 0;
> > + int i;
> > + static int input_num = 1;
> > +
> > /* Set the default controller player leds based on controller number */
> > mutex_lock(&joycon_input_num_mutex);
> > mutex_lock(&ctlr->output_mutex);
> > @@ -650,6 +711,29 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
> > if (ret)
> > hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
> > mutex_unlock(&ctlr->output_mutex);
> > +
> > + /* configure the player LEDs */
> > + for (i = 0; i < JC_NUM_LEDS; i++) {
> > + name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", d_name,
>
> This does not seem to be an appropriate name for an LED class device.
> See Documentation/leds/leds-class.rst.
>
>
> > + joycon_player_led_names[i]);
> > + if (!name)
> > + return -ENOMEM;
> > +
> > + led = &ctlr->leds[i];
> > + led->name = name;
> > + led->brightness = ((i + 1) <= input_num) ? LED_ON : LED_OFF;
> > + led->max_brightness = LED_ON;
>
> LED_{ON,OFF,...} constants have been deprecated to the best of my knowledge,
> use integer values as appropriate.
>
>
> > + led->brightness_set_blocking =
> > + joycon_player_led_brightness_set;
> > + led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> > +
> > + ret = devm_led_classdev_register(&hdev->dev, led);
> > + if (ret) {
> > + hid_err(hdev, "Failed registering %s LED\n", led->name);
> > + break;
> > + }
> > + }
> > +
> > if (++input_num > 4)
> > input_num = 1;
> > mutex_unlock(&joycon_input_num_mutex);
> > @@ -813,6 +897,13 @@ static int nintendo_hid_probe(struct hid_device *hdev,
> >
> > mutex_unlock(&ctlr->output_mutex);
> >
> > + /* Initialize the leds */
> > + ret = joycon_player_leds_create(ctlr);
> > + if (ret) {
> > + hid_err(hdev, "Failed to create leds; ret=%d\n", ret);
> > + goto err_close;
> > + }
> > +
> > ret = joycon_input_create(ctlr);
> > if (ret) {
> > hid_err(hdev, "Failed to create input device; ret=%d\n", ret);
> > --
> > 2.31.1
> >
>
>
> Regards,
> Barnabás Pőcze
