On 2024-07-15 09:15, Caleb Connolly wrote:
On 15/07/2024 09:03, Dragan Simic wrote:
On 2024-07-15 08:24, Caleb Connolly 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.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?
Hmm, if I had to choose between BUTTON_REMAP_PHONE_KEYS and
BUTTON_REMAP_MOBILE_KEYS, I'd still choose BUTTON_REMAP_PHONE_KEYS.
As you're against BUTTON_REMAP_REDUCED_KEYS, for which you provided
valid arguments, I'm still fine with your original word choice.
In other words, there are no reasons for the word choice to hold
this nice patch back from becoming accepted.
Ok, thanks :)
Sorry for the tone, I'm afraid I misread your original email.
No worries. My intention was never to split hairs, or to hold the
patch back from becoming accepted.
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.
I see, but I just tried to make it a bit more future-proof by using
more general terms. However, as I already wrote above, I'm fine with
keeping the patch in its original form.
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,
