[PATCH] dm: button: support remapping phone keys

[Dragan Simic]

Hello Caleb,

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

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.

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