| Message ID | 20240714194948.1271135-1-caleb.connolly@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | dm: button: support remapping phone keys | expand |
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@linaro.org> > --- > Cc: u-boot-qcom@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@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" 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. > 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,
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@linaro.org> >> --- >> Cc: u-boot-qcom@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@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,
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@linaro.org> >>> --- >>> Cc: u-boot-qcom@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@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,
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@linaro.org> >>>> --- >>>> Cc: u-boot-qcom@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@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,
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@linaro.org> >>>>> --- >>>>> Cc: u-boot-qcom@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@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,
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... If that isn't possible, ......... > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > Cc: u-boot-qcom@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 > + > 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) > +{ > + 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. Cheers, Quentin
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. 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@linaro.org> >> --- >> Cc: u-boot-qcom@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 >> + >> 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) >> +{ >> + 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. > > Cheers, > Quentin
Hi Quentin, >> +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. This is intentionally not a board specific feature. It is a generic option that does precisely one thing: remap the keys on a phone to be useful for navigating boot menus. If some folks have a strong disagreement about e.g. what KEY_CAMERA should be (for the devices that have it) then I would rather propose making this all configurable via an environment variable, or better yet introducing a new bootloader,code property in devicetree to define what a key should do in a bootloader. > > Cheers, > Quentin
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@linaro.org> >>> --- >>> Cc: u-boot-qcom@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
Adding my casual opinion to this naming decision: On Sun, Jul 14, 2024 at 11:24 PM Caleb Connolly <caleb.connolly@linaro.org> wrote: > > 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@linaro.org> > >> --- > >> Cc: u-boot-qcom@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@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? The distinction is about what it is not, rather than what it is. It is not a full keyboard layout but there may be some limited keyboard or button events. That is not necessarily a mobile device, nor a phone, or any other specific device. Certainly "reduced" makes sense but may not translate well for all people who would need to interact with this code. What I have known over the existence of keyboards is that "media keys" are describing functionality that is not typical for a keyboard input device. The other thought is "menu" keys which is more UX-derived but confusingly may be assumed to be arrow or pagination keys that do exist on a full keyboard layout. Most keyboards sold (circa 2024) now include these media keys and are listed as such, so would be familiar. > > > > 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) -E
Hello Shattow, On 2024-07-15 20:02, E Shattow wrote: > Adding my casual opinion to this naming decision: > > On Sun, Jul 14, 2024 at 11:24 PM Caleb Connolly > <caleb.connolly@linaro.org> 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@linaro.org> >> >> --- >> >> Cc: u-boot-qcom@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@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? > > The distinction is about what it is not, rather than what it is. It is > not a full keyboard layout but there may be some limited keyboard or > button events. That is not necessarily a mobile device, nor a phone, > or any other specific device. Certainly "reduced" makes sense but may > not translate well for all people who would need to interact with this > code. > > What I have known over the existence of keyboards is that "media keys" > are describing functionality that is not typical for a keyboard input > device. The other thought is "menu" keys which is more UX-derived but > confusingly may be assumed to be arrow or pagination keys that do > exist on a full keyboard layout. Most keyboards sold (circa 2024) now > include these media keys and are listed as such, so would be familiar. Yeah, BUTTON_REMAP_MEDIA_KEYS already crossed my mind, because of the failiarity with the modern PC keyboards that have become the standard. Though, is a power key still a media key? IDK. :) >> > 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) > > -E
Hi all, There's been some interesting discussion around this (thanks everyone who has chipped in). To summarise, it seems like a more elegant solution to this problem would be to introduce a new devicetree property "bootph,keys" (or u-boot,keys?) to denote the keycodes which should be send when a key is pressed in the bootloader stage, rather than the OS. This does seem like an elegant solution, but it would also require changing all the devicetree files for all the devices where this matters. Considering the lack of other usecases for this, I'm not sure if a generic solution like that is really worth it compared to what I'm proposing here. I also don't think that accepting this patch is mutually exclusive with implementing a "proper" generic solution later on. Does anyone have further thoughts? Can we go ahead with this patch? I'd be happy to re-send it, and go ahead with Quentin's suggestion to make the funtion weak so that boards can override it (though maybe that could be done in a future patch if there is some usecase for it). Thanks and regards, On 14/07/2024 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@linaro.org> > --- > Cc: u-boot-qcom@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 > + > 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) > +{ > + 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,
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 + 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) +{ + 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,
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@linaro.org> --- Cc: u-boot-qcom@groups.io --- drivers/button/Kconfig | 11 +++++++++++ drivers/button/button-uclass.c | 22 +++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-)