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.conno...@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.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.
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
