| Message ID | 20221202135516.4065670-1-git@aiyionpri.me |
|---|---|
| State | Accepted |
| Delegated to: | David Bauer |
| Headers | show |
| Series | realtek: update GPIO bindings for DGS-1210-10P | expand |
Hi Jan-Niklas, On Fri, 2022-12-02 at 14:55 +0100, Jan-Niklas Burfeind wrote: > add three missing LEDs > - PoE-Max > - Link/Act > - PoE Do the latter two LEDs indicate which LED mode is currently selected (on stock FW)? Users can do with these LEDs what they like, of course, but it would be good to know what the 'default' behaviour is in case somebody ever wanted to add a default trigger. Regarding the changes, I have one comment below. > > add two missing buttons > - mode > - reset > > The last was dropped in > commit 61a3d0075b15 ("realtek: update GPIO bindings in the dts files in dts- > 5.10") > > Signed-off-by: Jan-Niklas Burfeind <git@aiyionpri.me> > --- > Hello everyone, > I just tested the missing GPIO assignments for the DGS-12-10-10P > and verified the three LEDs as well as the reset button are working. > > The mode button should work, as its adress is 481 compared to resets > 484, but I haven't found a way to test it yet. You could change the "linux,code" property to <KEY_RESTART> and verify that it (also) triggers a reboot. > > Thanks > Jan-Niklas Burfeind > > .../dts-5.10/rtl8382_d-link_dgs-1210-10p.dts | 30 ++++++++++++++++--- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts > b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts > index 7ab37aaa9f..16934ede3b 100644 > --- a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts > +++ b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts > @@ -11,12 +11,34 @@ > compatible = "gpio-keys-polled"; > poll-interval = <20>; > > - /* is this pin 30 on the external RTL8231 (&gpio1)? */ > - /*mode { > + mode { > + label = "mode"; > + gpios = <&gpio1 30 GPIO_ACTIVE_LOW>; > + linux,code = <KEY_LIGHTS_TOGGLE>; Strictly speaking, this was intended for turning a (reading) light on or off. See Linux kernel commit 5a1bbf21325bd4f2641f6141fb8c47f6095578dd. You're just adding back what was dropped in commit 61a3d0075b15, but maybe we should try to keep the "turn the lights on/off" semantics for <KEY_LIGHTS_TOGGLE>. The Engenius EW2910P and Panasonic M*eG switch series have similar LED mode buttons, which the authors mapped to <BTN_0> [1, 2]. See target/linux/realtek/dts-5.10/rtl83xx_panasonic_mxxeg-pn28xx0k.dtsi and target/linux/realtek/dts-5.10/rtl8380_engenius_ews2910p.dts As I'm bikeshedding a bit here, does anyone else on the mailing list have an opinion on this? [1] https://github.com/openwrt/openwrt/pull/9896 [2] https://github.com/openwrt/openwrt/pull/4209 Best, Sander
On 12/4/22 20:45, Sander Vanheule wrote: > Hi Jan-Niklas, > > On Fri, 2022-12-02 at 14:55 +0100, Jan-Niklas Burfeind wrote: >> add three missing LEDs >> - PoE-Max >> - Link/Act >> - PoE > > Do the latter two LEDs indicate which LED mode is currently selected (on stock > FW)? Users can do with these LEDs what they like, of course, but it would be > good to know what the 'default' behaviour is in case somebody ever wanted to add > a default trigger. I think they, but am not near the device or its manual; will report back tomorrow. > > Regarding the changes, I have one comment below. > >> >> add two missing buttons >> - mode >> - reset >> >> The last was dropped in >> commit 61a3d0075b15 ("realtek: update GPIO bindings in the dts files in dts- >> 5.10") >> >> Signed-off-by: Jan-Niklas Burfeind <git@aiyionpri.me> >> --- >> Hello everyone, >> I just tested the missing GPIO assignments for the DGS-12-10-10P >> and verified the three LEDs as well as the reset button are working. >> >> The mode button should work, as its adress is 481 compared to resets >> 484, but I haven't found a way to test it yet. > > You could change the "linux,code" property to <KEY_RESTART> and verify that it > (also) triggers a reboot. Will do tomorrow afternoon. Just to be sure, that is not intended to become part of the patch, but only meant as means for me to verify its functionality. Please correct me if wrong; otherwise just ignore. > >> >> Thanks >> Jan-Niklas Burfeind >> >> .../dts-5.10/rtl8382_d-link_dgs-1210-10p.dts | 30 ++++++++++++++++--- >> 1 file changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >> b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >> index 7ab37aaa9f..16934ede3b 100644 >> --- a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >> +++ b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >> @@ -11,12 +11,34 @@ >> compatible = "gpio-keys-polled"; >> poll-interval = <20>; >> >> - /* is this pin 30 on the external RTL8231 (&gpio1)? */ >> - /*mode { >> + mode { >> + label = "mode"; >> + gpios = <&gpio1 30 GPIO_ACTIVE_LOW>; >> + linux,code = <KEY_LIGHTS_TOGGLE>; > > Strictly speaking, this was intended for turning a (reading) light on or off. > See Linux kernel commit 5a1bbf21325bd4f2641f6141fb8c47f6095578dd. You're just > adding back what was dropped in commit 61a3d0075b15, but maybe we should try to > keep the "turn the lights on/off" semantics for <KEY_LIGHTS_TOGGLE>. I tried to match the other variant (dgs-1210-10mp) [3] if we intend to change that in this PR, we should probably fix that as well. The author of that would've been Daniel Groth and you commited. Either way I'm open for suggested changes, if still wanted. > > The Engenius EW2910P and Panasonic M*eG switch series have similar LED mode > buttons, which the authors mapped to <BTN_0> [1, 2]. > > See target/linux/realtek/dts-5.10/rtl83xx_panasonic_mxxeg-pn28xx0k.dtsi > and target/linux/realtek/dts-5.10/rtl8380_engenius_ews2910p.dts > > As I'm bikeshedding a bit here, does anyone else on the mailing list have an > opinion on this? > > [1] https://github.com/openwrt/openwrt/pull/9896 > [2] https://github.com/openwrt/openwrt/pull/4209 > > > Best, > Sander > [3] https://github.com/openwrt/openwrt/blob/master/target/linux/realtek/dts-5.10/rtl8380_d-link_dgs-1210-10mp-f.dts#L34
On 12/5/22 00:02, Jan-Niklas Burfeind wrote: > On 12/4/22 20:45, Sander Vanheule wrote: >> Hi Jan-Niklas, >> >> On Fri, 2022-12-02 at 14:55 +0100, Jan-Niklas Burfeind wrote: >>> add three missing LEDs >>> - PoE-Max >>> - Link/Act >>> - PoE >> >> Do the latter two LEDs indicate which LED mode is currently selected >> (on stock >> FW)? Users can do with these LEDs what they like, of course, but it >> would be >> good to know what the 'default' behaviour is in case somebody ever >> wanted to add >> a default trigger. > > I think they, but am not near the device or its manual; will report back > tomorrow. The manual confirms, the button is for switching the port-LEDs mode between poe and link/act. > >> >> Regarding the changes, I have one comment below. >> >>> >>> add two missing buttons >>> - mode >>> - reset >>> >>> The last was dropped in >>> commit 61a3d0075b15 ("realtek: update GPIO bindings in the dts files >>> in dts- >>> 5.10") >>> >>> Signed-off-by: Jan-Niklas Burfeind <git@aiyionpri.me> >>> --- >>> Hello everyone, >>> I just tested the missing GPIO assignments for the DGS-12-10-10P >>> and verified the three LEDs as well as the reset button are working. >>> >>> The mode button should work, as its adress is 481 compared to resets >>> 484, but I haven't found a way to test it yet. >> >> You could change the "linux,code" property to <KEY_RESTART> and verify >> that it >> (also) triggers a reboot. It does trigger a reboot as well, the buttons assingments are both correct. > > Will do tomorrow afternoon. Just to be sure, that is not intended to > become part of the patch, but only meant as means for me to verify its > functionality. Please correct me if wrong; otherwise just ignore. >> >>> >>> Thanks >>> Jan-Niklas Burfeind >>> >>> .../dts-5.10/rtl8382_d-link_dgs-1210-10p.dts | 30 ++++++++++++++++--- >>> 1 file changed, 26 insertions(+), 4 deletions(-) >>> >>> diff --git >>> a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >>> b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >>> index 7ab37aaa9f..16934ede3b 100644 >>> --- a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >>> +++ b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts >>> @@ -11,12 +11,34 @@ >>> compatible = "gpio-keys-polled"; >>> poll-interval = <20>; >>> - /* is this pin 30 on the external RTL8231 (&gpio1)? */ >>> - /*mode { >>> + mode { >>> + label = "mode"; >>> + gpios = <&gpio1 30 GPIO_ACTIVE_LOW>; >>> + linux,code = <KEY_LIGHTS_TOGGLE>; >> >> Strictly speaking, this was intended for turning a (reading) light on >> or off. >> See Linux kernel commit 5a1bbf21325bd4f2641f6141fb8c47f6095578dd. >> You're just >> adding back what was dropped in commit 61a3d0075b15, but maybe we >> should try to >> keep the "turn the lights on/off" semantics for <KEY_LIGHTS_TOGGLE>. > > I tried to match the other variant (dgs-1210-10mp) [3] if we intend to > change that in this PR, we should probably fix that as well. > The author of that would've been Daniel Groth and you commited. > Either way I'm open for suggested changes, if still wanted. > Let me know, if there anything left, I should add or change in this patch. >> >> The Engenius EW2910P and Panasonic M*eG switch series have similar LED >> mode >> buttons, which the authors mapped to <BTN_0> [1, 2]. >> >> See target/linux/realtek/dts-5.10/rtl83xx_panasonic_mxxeg-pn28xx0k.dtsi >> and target/linux/realtek/dts-5.10/rtl8380_engenius_ews2910p.dts >> >> As I'm bikeshedding a bit here, does anyone else on the mailing list >> have an >> opinion on this? >> >> [1] https://github.com/openwrt/openwrt/pull/9896 >> [2] https://github.com/openwrt/openwrt/pull/4209 >> >> >> Best, >> Sander >> > > [3] > https://github.com/openwrt/openwrt/blob/master/target/linux/realtek/dts-5.10/rtl8380_d-link_dgs-1210-10mp-f.dts#L34 > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts index 7ab37aaa9f..16934ede3b 100644 --- a/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts +++ b/target/linux/realtek/dts-5.10/rtl8382_d-link_dgs-1210-10p.dts @@ -11,12 +11,34 @@ compatible = "gpio-keys-polled"; poll-interval = <20>; - /* is this pin 30 on the external RTL8231 (&gpio1)? */ - /*mode { + mode { + label = "mode"; + gpios = <&gpio1 30 GPIO_ACTIVE_LOW>; + linux,code = <KEY_LIGHTS_TOGGLE>; + }; + + reset { label = "reset"; - gpios = <&gpio0 94 GPIO_ACTIVE_LOW>; + gpios = <&gpio1 33 GPIO_ACTIVE_LOW>; linux,code = <KEY_RESTART>; - };*/ + }; + }; + + leds { + link_act { + label = "green:link_act"; + gpios = <&gpio1 28 GPIO_ACTIVE_LOW>; + }; + + poe { + label = "green:poe"; + gpios = <&gpio1 29 GPIO_ACTIVE_LOW>; + }; + + poe_max { + label = "yellow:poe_max"; + gpios = <&gpio1 27 GPIO_ACTIVE_LOW>; + }; }; gpio1: rtl8231-gpio {
add three missing LEDs - PoE-Max - Link/Act - PoE add two missing buttons - mode - reset The last was dropped in commit 61a3d0075b15 ("realtek: update GPIO bindings in the dts files in dts-5.10") Signed-off-by: Jan-Niklas Burfeind <git@aiyionpri.me> --- Hello everyone, I just tested the missing GPIO assignments for the DGS-12-10-10P and verified the three LEDs as well as the reset button are working. The mode button should work, as its adress is 481 compared to resets 484, but I haven't found a way to test it yet. Thanks Jan-Niklas Burfeind .../dts-5.10/rtl8382_d-link_dgs-1210-10p.dts | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-)