[PATCH] dm: button: support remapping phone keys

[Dragan Simic]

Hello Shattow,

On 2024-07-15 20:02, E Shattow wrote:
> 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:
>> On 14/07/2024 22:47, Dragan Simic wrote:
>> > 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.

Yeah, BUTTON_REMAP_MEDIA_KEYS already crossed my mind, because of the
failiarity with the modern PC keyboards that have become the standard.
Though, is a power key still a media key?  IDK. :)

>> > 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