[PATCH] dm: button: support remapping phone keys

[Caleb Connolly]

On 15/07/2024 09:03, Dragan Simic wrote:
> 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.

Ok, thanks :)

Sorry for the tone, I'm afraid I misread your original email.

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

-- 
// Caleb (they/them)