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

[Daniel Ogorchock <djogorchock@gmail.com>]

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.

Thanks,
Daniel

On Tue, Aug 3, 2021 at 7:30 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> 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



-- 
Daniel Ogorchock