Adding my casual opinion to this naming decision:
On Sun, Jul 14, 2024 at 11:24 PM Caleb Connolly
<caleb.conno...@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.conno...@linaro.org>
> >> ---
> >> Cc: u-boot-q...@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 <dsi...@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
