[PATCH] dm: button: support remapping phone keys

[Dragan Simic]

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

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,