[PATCH] dm: button: support remapping phone keys

[Quentin Schulz]

Hi Caleb,

On 7/15/24 10:38 AM, Caleb Connolly wrote:
> Hi Quentin,
> 
> On 15/07/2024 10:16, Quentin Schulz wrote:
>> Hi Caleb,
>>
>> On 7/14/24 9:49 PM, 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.
>>>
>>
>> Are those event codes properly defined in the DT already?
>>
>> e.g. 
>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts#L31
>>
>> If so, what prevents us from simply patching the U-Boot DT to change 
>> that code? No additional code required anywhere, no need to maintain a 
>> list of mapping, etc...
> 
> Many platforms in U-Boot are now switching to upstream DT, and 
> SystemReady compliance requires that the bootloader provide a DT to the 
> kernel.
> 

I guess you meant to say "the same DT" to the kernel here?

But yeah, that's what I was afraid of.

> U-Boot doesn't really have a generic way to apply specific adjustments 
> to FDT for U-Boot without them being carried over to the kernel.
> 
> Patching FDT is in and of itself somewhat treacherous without using 
> OF_LIVE, which isn't set up until after bind(), making it unsuitable. 
> It's also vastly slower to patch FDT than to patch a livetree.
> 
> I couldn't conceive of a way to do via patching DT that wouldn't require 
> solving the above problems, I also couldn't think of another example 
> that would justify making this patch more generic or configurable.
> 
> Kind regards,
>>
>> If that isn't possible, .........
>>
>>> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
>>> ---
>>> Cc: u-boot-qcom at groups.io
>>> ---
>>>   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
>>> +

Should this be alphabetically sorted (I don't know if we have any rule 
for Kconfig options?) in the Kconfig file?

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

Does this work for boards without OF_UPSTREAM? Otherwise I suggest to 
use <linux/input.h> instead.

Nit: Missing newline between headers and first function?

>>>   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)
>>> +{
>>> +    switch (code) {
>>> +    case KEY_VOLUMEUP:
>>> +        return KEY_UP;
>>> +    case KEY_VOLUMEDOWN:
>>> +        return KEY_DOWN;
>>> +    case KEY_POWER:
>>> +        return KEY_ENTER;
>>> +    default:
>>> +        return code;
>>> +    }
>>> +}
>>> +
>>
>> ....... I suggest to make this a weak function that can be overridden 
>> by boards (should it maybe be only defined in boards C file?) so that 
>> it's easy for people to come up with their own mapping without having 
>> to deal with two people/the maintainer disagreeing with what should be 
>> the one and true mapping for that key.
>>

I guess this could be done the day we need it to be more generic.

Thanks,
Quentin