[PATCH] dm: button: support remapping phone keys

[Caleb Connolly]

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