diff mbox series

[2/3] leds: add new LED_FUNCTION_PLAYER for player LEDs for game controllers.

Message ID 20210602061253.5747-3-roderick@gaikai.com
State New
Headers show
Series HID: playstation: add LED support. | expand

Commit Message

Roderick Colenbrander June 2, 2021, 6:12 a.m. UTC
From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Player LEDs are commonly found on game controllers from Nintendo and Sony
to indicate a player ID across a number of LEDs. For example, "Player 2"
might be indicated as "-x--" on a device with 4 LEDs where "x" means on.

This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
player LEDs from the kernel. Until now there was no good standard, which
resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.

Note: management of Player IDs is left to user space, though a kernel
driver may pick a default value.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 include/dt-bindings/leds/common.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jiri Kosina June 24, 2021, 1:25 p.m. UTC | #1
On Tue, 1 Jun 2021, Roderick Colenbrander wrote:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Player LEDs are commonly found on game controllers from Nintendo and Sony
> to indicate a player ID across a number of LEDs. For example, "Player 2"
> might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> 
> This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> player LEDs from the kernel. Until now there was no good standard, which
> resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> 
> Note: management of Player IDs is left to user space, though a kernel
> driver may pick a default value.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  include/dt-bindings/leds/common.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> index 52b619d44ba2..94999c250e4d 100644
> --- a/include/dt-bindings/leds/common.h
> +++ b/include/dt-bindings/leds/common.h
> @@ -60,6 +60,9 @@
>  #define LED_FUNCTION_MICMUTE "micmute"
>  #define LED_FUNCTION_MUTE "mute"
>  
> +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> +#define LED_FUNCTION_PLAYER "player"
> +
>  /* Miscelleaus functions. Use functions above if you can. */
>  #define LED_FUNCTION_ACTIVITY "activity"
>  #define LED_FUNCTION_ALARM "alarm"

Pavel, can I please get your Ack on this one, so that I can take it with 
the rest of the series?

Thanks,
Roderick Colenbrander July 6, 2021, 4:51 a.m. UTC | #2
Hi Pavel,

Any feedback on this patch, which introduces a new player led type,
which is common on game controllers?

Thanks,
Roderick Colenbrander

On Thu, Jun 24, 2021 at 6:26 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Tue, 1 Jun 2021, Roderick Colenbrander wrote:
>
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> >
> > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > player LEDs from the kernel. Until now there was no good standard, which
> > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> >
> > Note: management of Player IDs is left to user space, though a kernel
> > driver may pick a default value.
> >
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > ---
> >  include/dt-bindings/leds/common.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > index 52b619d44ba2..94999c250e4d 100644
> > --- a/include/dt-bindings/leds/common.h
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -60,6 +60,9 @@
> >  #define LED_FUNCTION_MICMUTE "micmute"
> >  #define LED_FUNCTION_MUTE "mute"
> >
> > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > +#define LED_FUNCTION_PLAYER "player"
> > +
> >  /* Miscelleaus functions. Use functions above if you can. */
> >  #define LED_FUNCTION_ACTIVITY "activity"
> >  #define LED_FUNCTION_ALARM "alarm"
>
> Pavel, can I please get your Ack on this one, so that I can take it with
> the rest of the series?
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
Roderick Colenbrander July 22, 2021, 4:22 p.m. UTC | #3
Hi Pavel,

Any update on this topic?

Thanks,
Roderick

On Mon, Jul 5, 2021 at 9:51 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>

> Hi Pavel,

>

> Any feedback on this patch, which introduces a new player led type,

> which is common on game controllers?

>

> Thanks,

> Roderick Colenbrander

>

> On Thu, Jun 24, 2021 at 6:26 AM Jiri Kosina <jikos@kernel.org> wrote:

> >

> > On Tue, 1 Jun 2021, Roderick Colenbrander wrote:

> >

> > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>

> > >

> > > Player LEDs are commonly found on game controllers from Nintendo and Sony

> > > to indicate a player ID across a number of LEDs. For example, "Player 2"

> > > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.

> > >

> > > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate

> > > player LEDs from the kernel. Until now there was no good standard, which

> > > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and

> > > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.

> > >

> > > Note: management of Player IDs is left to user space, though a kernel

> > > driver may pick a default value.

> > >

> > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

> > > ---

> > >  include/dt-bindings/leds/common.h | 3 +++

> > >  1 file changed, 3 insertions(+)

> > >

> > > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h

> > > index 52b619d44ba2..94999c250e4d 100644

> > > --- a/include/dt-bindings/leds/common.h

> > > +++ b/include/dt-bindings/leds/common.h

> > > @@ -60,6 +60,9 @@

> > >  #define LED_FUNCTION_MICMUTE "micmute"

> > >  #define LED_FUNCTION_MUTE "mute"

> > >

> > > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */

> > > +#define LED_FUNCTION_PLAYER "player"

> > > +

> > >  /* Miscelleaus functions. Use functions above if you can. */

> > >  #define LED_FUNCTION_ACTIVITY "activity"

> > >  #define LED_FUNCTION_ALARM "alarm"

> >

> > Pavel, can I please get your Ack on this one, so that I can take it with

> > the rest of the series?

> >

> > Thanks,

> >

> > --

> > Jiri Kosina

> > SUSE Labs

> >
Pavel Machek Aug. 3, 2021, 10:10 p.m. UTC | #4
Hi!

> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > 
> > Player LEDs are commonly found on game controllers from Nintendo and Sony
> > to indicate a player ID across a number of LEDs. For example, "Player 2"
> > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.
> > 
> > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate
> > player LEDs from the kernel. Until now there was no good standard, which
> > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and
> > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.
> > 
> > Note: management of Player IDs is left to user space, though a kernel
> > driver may pick a default value.
> > 
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > ---
> >  include/dt-bindings/leds/common.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
> > index 52b619d44ba2..94999c250e4d 100644
> > --- a/include/dt-bindings/leds/common.h
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -60,6 +60,9 @@
> >  #define LED_FUNCTION_MICMUTE "micmute"
> >  #define LED_FUNCTION_MUTE "mute"
> >  
> > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
> > +#define LED_FUNCTION_PLAYER "player"
> > +
> >  /* Miscelleaus functions. Use functions above if you can. */
> >  #define LED_FUNCTION_ACTIVITY "activity"
> >  #define LED_FUNCTION_ALARM "alarm"
> 
> Pavel, can I please get your Ack on this one, so that I can take it with 
> the rest of the series?

I'm sorry for delays.

But no, player is not suitable function. Function would be "player1"
AFAICT, right?

I'm not sure "function" is suitable here, and we may want to create
documentation like this... where it would be explained which functions
apply to which devices and what they actually mean.

Best regards,
								Pavel

-*- org -*-

It is somehow important to provide consistent interface to the
userland. LED devices have one problem there, and that is naming of
directories in /sys/class/leds. It would be nice if userland would
just know right "name" for given LED function, but situation got more
complex.

Anyway, if backwards compatibility is not an issue, new code should
use one of the "good" names from this list, and you should extend the
list where applicable.

Bad names are listed, too; in case you are writing application that
wants to use particular feature, you should probe for good name, first,
but then try the bad ones, too.

* Keyboards
  
Good: "input*:*:capslock"
Good: "input*:*:scrolllock"
Good: "input*:*:numlock"
Bad: "shift-key-light" (Motorola Droid 4, capslock)

Set of common keyboard LEDs, going back to PC AT or so.

Good: "platform::kbd_backlight"
Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)

Frontlight/backlight of main keyboard.

Bad: "button-backlight" (Motorola Droid 4)

Some phones have touch buttons below screen; it is different from main
keyboard. And this is their backlight.

* Sound subsystem

Good: "platform:*:mute"
Good: "platform:*:micmute"

LEDs on notebook body, indicating that sound input / output is muted.

* System notification

Good: "status-led:{red,green,blue}" (Motorola Droid 4)
Bad: "lp5523:{r,g,b}" (Nokia N900)

Phones usually have multi-color status LED.

* Power management

Good: "platform:*:charging" (allwinner sun50i)

* Screen

Good: ":backlight" (Motorola Droid 4)
Roderick Colenbrander Aug. 3, 2021, 11:29 p.m. UTC | #5
Hi Pavel,

See some quick comments inline.

On Tue, Aug 3, 2021 at 3:39 PM Pavel Machek <pavel@ucw.cz> wrote:
>

> Hi!

>

> > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>

> > >

> > > Player LEDs are commonly found on game controllers from Nintendo and Sony

> > > to indicate a player ID across a number of LEDs. For example, "Player 2"

> > > might be indicated as "-x--" on a device with 4 LEDs where "x" means on.

> > >

> > > This patch introduces a new LED_FUNCTION_PLAYER to properly indicate

> > > player LEDs from the kernel. Until now there was no good standard, which

> > > resulted in inconsistent behavior across xpad, hid-sony, hid-wiimote and

> > > other drivers. Moving forward new drivers should use LED_FUNCTION_PLAYER.

> > >

> > > Note: management of Player IDs is left to user space, though a kernel

> > > driver may pick a default value.

> > >

> > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>

> > > ---

> > >  include/dt-bindings/leds/common.h | 3 +++

> > >  1 file changed, 3 insertions(+)

> > >

> > > diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h

> > > index 52b619d44ba2..94999c250e4d 100644

> > > --- a/include/dt-bindings/leds/common.h

> > > +++ b/include/dt-bindings/leds/common.h

> > > @@ -60,6 +60,9 @@

> > >  #define LED_FUNCTION_MICMUTE "micmute"

> > >  #define LED_FUNCTION_MUTE "mute"

> > >

> > > +/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */

> > > +#define LED_FUNCTION_PLAYER "player"

> > > +

> > >  /* Miscelleaus functions. Use functions above if you can. */

> > >  #define LED_FUNCTION_ACTIVITY "activity"

> > >  #define LED_FUNCTION_ALARM "alarm"

> >

> > Pavel, can I please get your Ack on this one, so that I can take it with

> > the rest of the series?

>

> I'm sorry for delays.

>

> But no, player is not suitable function. Function would be "player1"

> AFAICT, right?


A given gaming device such as Sony or Nintendo controllers have a
multiple of these LEDs, which are meant to be configured with a player
number. A typical device has like 4 of these LEDs, so a single
controller would have: "player-1", "player-2", "player-3" and
"player-4". It is up to userspace to configure the player number (a
driver may set a default number across a number of LEDs).

If player is not the right term (many people know it), what would it be?

>

> I'm not sure "function" is suitable here, and we may want to create

> documentation like this... where it would be explained which functions

> apply to which devices and what they actually mean.

>

> Best regards,

>                                                                 Pavel

>

> -*- org -*-

>

> It is somehow important to provide consistent interface to the

> userland. LED devices have one problem there, and that is naming of

> directories in /sys/class/leds. It would be nice if userland would

> just know right "name" for given LED function, but situation got more

> complex.

>

> Anyway, if backwards compatibility is not an issue, new code should

> use one of the "good" names from this list, and you should extend the

> list where applicable.

>

> Bad names are listed, too; in case you are writing application that

> wants to use particular feature, you should probe for good name, first,

> but then try the bad ones, too.

>

> * Keyboards

>

> Good: "input*:*:capslock"

> Good: "input*:*:scrolllock"

> Good: "input*:*:numlock"

> Bad: "shift-key-light" (Motorola Droid 4, capslock)

>

> Set of common keyboard LEDs, going back to PC AT or so.

>

> Good: "platform::kbd_backlight"

> Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)

> Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)

>

> Frontlight/backlight of main keyboard.

>

> Bad: "button-backlight" (Motorola Droid 4)

>

> Some phones have touch buttons below screen; it is different from main

> keyboard. And this is their backlight.

>

> * Sound subsystem

>

> Good: "platform:*:mute"

> Good: "platform:*:micmute"

>

> LEDs on notebook body, indicating that sound input / output is muted.

>

> * System notification

>

> Good: "status-led:{red,green,blue}" (Motorola Droid 4)

> Bad: "lp5523:{r,g,b}" (Nokia N900)

>

> Phones usually have multi-color status LED.

>

> * Power management

>

> Good: "platform:*:charging" (allwinner sun50i)

>

> * Screen

>

> Good: ":backlight" (Motorola Droid 4)

>

>

> --

> http://www.livejournal.com/~pavelmachek
Jiri Kosina Aug. 31, 2021, 7:09 p.m. UTC | #6
On Fri, 13 Aug 2021, Daniel Ogorchock wrote:

> Hi Pavel,
> 
> Do you have any recommendations on what would be an appropriate
> function string for player indicator LEDs? Would some variant such as:
>   "status-x"
>   "player-status-x"
>   "indicator-x"
>   "player-indicator-x"
> be more suitable? It looks like the string "status" has been used for
> other existing LED names.
> 
> I think we are pretty happy to use whatever naming scheme fits the
> standards of the led subsystem's userspace api for the Nintendo/Sony
> HID drivers, and any future game controller drivers featuring player
> LEDs could conform to that going forward.

Pavel, could you please take a look here, so that we can proceed with the 
patchset?

Thanks,
Pavel Machek Sept. 1, 2021, 5:19 a.m. UTC | #7
Hi!

> > Do you have any recommendations on what would be an appropriate
> > function string for player indicator LEDs? Would some variant such as:
> >   "status-x"
> >   "player-status-x"
> >   "indicator-x"
> >   "player-indicator-x"
> > be more suitable? It looks like the string "status" has been used for
> > other existing LED names.

I guess "player-x" would be suitable.

> > I think we are pretty happy to use whatever naming scheme fits the
> > standards of the led subsystem's userspace api for the Nintendo/Sony
> > HID drivers, and any future game controller drivers featuring player
> > LEDs could conform to that going forward.
> 
> Pavel, could you please take a look here, so that we can proceed with the 
> patchset?

So... leds tree has just been merged:

> git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/
tags/leds-5.15-rc1

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a998a62be9cdb509491731ffe81575aa09943a32

It includes Documentation/leds/well-known-leds.txt file. Could a
section describing proposed naming be added there (both device and
function), with explanations what the LEDs do?

Best regards,
								Pavel
Roderick Colenbrander Sept. 1, 2021, 8:24 p.m. UTC | #8
Hi Pavel,

On Tue, Aug 31, 2021 at 10:19 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > Do you have any recommendations on what would be an appropriate
> > > function string for player indicator LEDs? Would some variant such as:
> > >   "status-x"
> > >   "player-status-x"
> > >   "indicator-x"
> > >   "player-indicator-x"
> > > be more suitable? It looks like the string "status" has been used for
> > > other existing LED names.
>
> I guess "player-x" would be suitable.
>
> > > I think we are pretty happy to use whatever naming scheme fits the
> > > standards of the led subsystem's userspace api for the Nintendo/Sony
> > > HID drivers, and any future game controller drivers featuring player
> > > LEDs could conform to that going forward.
> >
> > Pavel, could you please take a look here, so that we can proceed with the
> > patchset?
>
> So... leds tree has just been merged:
>
> > git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/
> tags/leds-5.15-rc1
>
> has been merged into torvalds/linux.git:
> https://git.kernel.org/torvalds/c/a998a62be9cdb509491731ffe81575aa09943a32
>
> It includes Documentation/leds/well-known-leds.txt file. Could a
> section describing proposed naming be added there (both device and
> function), with explanations what the LEDs do?
>

Sure let me write add a few lines for that file and resubmit. I guess
I should rebase based on Linus his tree then.. let me quickly start on
that. (I'm technically on vacation and far from home, but luckily
caught this and happen to have a break)

Thanks,
Roderick
diff mbox series

Patch

diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 52b619d44ba2..94999c250e4d 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -60,6 +60,9 @@ 
 #define LED_FUNCTION_MICMUTE "micmute"
 #define LED_FUNCTION_MUTE "mute"
 
+/* Used for player LEDs as found on game controllers from e.g. Nintendo, Sony. */
+#define LED_FUNCTION_PLAYER "player"
+
 /* Miscelleaus functions. Use functions above if you can. */
 #define LED_FUNCTION_ACTIVITY "activity"
 #define LED_FUNCTION_ALARM "alarm"