[PATCH] dm: button: support remapping phone keys

[E Shattow]

Adding my casual opinion to this naming decision:

On Sun, Jul 14, 2024 at 11:24 PM Caleb Connolly
<caleb.connolly at linaro.org> wrote:
>
> Hi Dragan,
>
> On 14/07/2024 22:47, Dragan Simic wrote:
> > Hello Caleb,
> >
> > On 2024-07-14 21:49, Caleb Connolly wrote:
> >> We don't have audio support in U-Boot, but we do have boot menus. Add an
> >> option to re-map the volume and power buttons to up/down/enter so that
> >> in situations where these are the only available buttons (such as on
> >> mobile phones) it's still possible to navigate menus built in U-Boot or
> >> an external EFI app like GRUB or systemd-boot.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >> Cc: u-boot-qcom at groups.io
> >
> > Very nice, thanks for this patch!  Looking good to me, with a few
> > suggestions available below.  Anyway, please feel free to add:
> >
> > Reviewed-by: Dragan Simic <dsimic at manjaro.org>
> >
> >> ---
> >>  drivers/button/Kconfig         | 11 +++++++++++
> >>  drivers/button/button-uclass.c | 22 +++++++++++++++++++++-
> >>  2 files changed, 32 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/button/Kconfig b/drivers/button/Kconfig
> >> index 3918b05ae03e..6cae16fcc8bf 100644
> >> --- a/drivers/button/Kconfig
> >> +++ b/drivers/button/Kconfig
> >> @@ -8,8 +8,19 @@ config BUTTON
> >>        U-Boot provides a uclass API to implement this feature. Button
> >> drivers
> >>        can provide access to board-specific buttons. Use of the device
> >> tree
> >>        for configuration is encouraged.
> >>
> >> +config BUTTON_REMAP_PHONE_KEYS
> >> +    bool "Remap phone keys for navigation"
> >> +    depends on BUTTON
> >> +    help
> >> +      Enable remapping of phone keys to navigation keys. This is
> >> useful for
> >> +      devices with phone keys that are not used in U-Boot. The phone
> >> keys
> >> +      are remapped to the following navigation keys:
> >> +      - Volume up: Up
> >> +      - Volume down: Down
> >> +      - Power: Enter
> >> +
> >
> > Frankly, "phone keys" sounds a bit strange to me, maybe because there
> > are also tablets that have the same style of reduced-set keys.  Thus,
> > I'd suggest that the following language is used:
> >
> > - "BUTTON_REMAP_PHONE_KEYS" instead of "BUTTON_REMAP_REDUCED_KEYS"
> > - "reduced smartphone-style keys" instead of "phone keys"
>
> I would have assumed that anyone working on a tablet would immediately
> guess what this option does and that it might be useful given the name.
> I would argue that seeing "BUTTON_REMAP_REDUCED_KEYS" instead makes it
> harder to guess what this option does.
>
> I think of it not as "this option remaps the keys on your phone" but as
> "this option remaps the keys that phones have", as in, the volume and
> power buttons.
>
> If you'd prefer, maybe we can meet somewhere in the middle with "mobile"?
>

> how's BUTTON_REMAP_MOBILE_KEYS?

The distinction is about what it is not, rather than what it is. It is
not a full keyboard layout but there may be some limited keyboard or
button events. That is not necessarily a mobile device, nor a phone,
or any other specific device. Certainly "reduced" makes sense but may
not translate well for all people who would need to interact with this
code.

What I have known over the existence of keyboards is that "media keys"
are describing functionality that is not typical for a keyboard input
device. The other thought is "menu" keys which is more UX-derived but
confusingly may be assumed to be arrow or pagination keys that do
exist on a full keyboard layout. Most keyboards sold (circa 2024) now
include these media keys and are listed as such, so would be familiar.

> >
> > Using "reduced" would also allow us to have this remapping logic more
> > easily extended to also cover some other buttons found on some other
> > devices with reduced-set keys.
>
> If such a device exists and gains support in U-Boot, the switch/case
> could be extended, or a new option added if it doesn't make sense to
> lump everything together. Without knowing about such a device I think
> it's impossible to make a judgement here.
>
> >
> >>  config BUTTON_ADC
> >>      bool "Button adc"
> >>      depends on BUTTON
> >>      depends on ADC
> >> diff --git a/drivers/button/button-uclass.c
> >> b/drivers/button/button-uclass.c
> >> index cda243389df3..729983d58701 100644
> >> --- a/drivers/button/button-uclass.c
> >> +++ b/drivers/button/button-uclass.c
> >> @@ -9,8 +9,9 @@
> >>
> >>  #include <button.h>
> >>  #include <dm.h>
> >>  #include <dm/uclass-internal.h>
> >> +#include <dt-bindings/input/linux-event-codes.h>
> >>
> >>  int button_get_by_label(const char *label, struct udevice **devp)
> >>  {
> >>      struct udevice *dev;
> >> @@ -36,16 +37,35 @@ enum button_state_t button_get_state(struct
> >> udevice *dev)
> >>
> >>      return ops->get_state(dev);
> >>  }
> >>
> >> +static int button_remap_phone_keys(int code)
> >
> > Pretty much the same suggestion as above applies here.
> >
> >> +{
> >> +    switch (code) {
> >> +    case KEY_VOLUMEUP:
> >> +        return KEY_UP;
> >> +    case KEY_VOLUMEDOWN:
> >> +        return KEY_DOWN;
> >> +    case KEY_POWER:
> >> +        return KEY_ENTER;
> >> +    default:
> >> +        return code;
> >> +    }
> >> +}
> >> +
> >>  int button_get_code(struct udevice *dev)
> >>  {
> >>      struct button_ops *ops = button_get_ops(dev);
> >> +    int code;
> >>
> >>      if (!ops->get_code)
> >>          return -ENOSYS;
> >>
> >> -    return ops->get_code(dev);
> >> +    code = ops->get_code(dev);
> >> +    if (CONFIG_IS_ENABLED(BUTTON_REMAP_PHONE_KEYS))
> >> +        return button_remap_phone_keys(code);
> >> +    else
> >> +        return code;
> >>  }
> >>
> >>  UCLASS_DRIVER(button) = {
> >>      .id        = UCLASS_BUTTON,
>
> --
> // Caleb (they/them)

-E