| Message ID | 20221217100455.52593-1-marijn.suijten@somainline.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sm6125-seine: Clean up gpio-keys (volume down) | expand |
On 17.12.2022 11:04, Marijn Suijten wrote: > - Remove autorepeat (leave key repetition to userspace); > - Remove unneeded status = "okay" (this is the default); > - Allow the interrupt line for this button to be disabled; > - Use a full, descriptive node name; > - Set proper bias on the GPIO via pinctrl; > - Sort properties. > > Fixes: 82e1783890b7 ("arm64: dts: qcom: sm6125: Add support for Sony Xperia 10II") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > .../qcom/sm6125-sony-xperia-seine-pdx201.dts | 20 ++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts b/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts > index 1b9e40d3d269..8c7c9331fd21 100644 > --- a/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts > +++ b/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts > @@ -41,17 +41,17 @@ extcon_usb: extcon-usb { > }; > > gpio-keys { > - status = "okay"; > compatible = "gpio-keys"; > - autorepeat; > + pinctrl-0 = <&gpio_keys_state>; > + pinctrl-names = "default"; > > - key-vol-dn { > + key-volume-down { > label = "Volume Down"; > gpios = <&tlmm 47 GPIO_ACTIVE_LOW>; > - linux,input-type = <1>; > linux,code = <KEY_VOLUMEDOWN>; > - gpio-key,wakeup; > debounce-interval = <15>; > + linux,can-disable; > + gpio-key,wakeup; > }; > }; > > @@ -270,6 +270,16 @@ &sdhc_1 { > > &tlmm { > gpio-reserved-ranges = <22 2>, <28 6>; > + > + gpio_keys_state: gpio-keys-state { > + key-volume-down-pins { I see no need for defining a wrapper node. The other changes look good! Konrad > + pins = "gpio47"; > + function = "gpio"; > + drive-strength = <2>; > + bias-disable; > + input-enable; > + }; > + }; > }; > > &usb3 {
On 2022-12-17 16:04:17, Konrad Dybcio wrote: > On 17.12.2022 11:04, Marijn Suijten wrote: > > [..] > > @@ -270,6 +270,16 @@ &sdhc_1 { > > > > &tlmm { > > gpio-reserved-ranges = <22 2>, <28 6>; > > + > > + gpio_keys_state: gpio-keys-state { > > + key-volume-down-pins { > I see no need for defining a wrapper node. > The other changes look good! I did the same for sm6350-lena, which we should flatten out then too. For these uses I'm not sure when it's clearer/better to use: thing@x { pinctrl-0 = <&thing_state>; ... }; thing_state: thing-state { specific-pin { ... }; other-specific-pin ... ... }; Or separate out the pins with their own state and instead use: thing@x { pinctrl-0 = <&specific_pin1_state &specific_pin2_state>; ... }; If I had to guess the former groups related pins together (as we finally do now for SDC...) which should all be toggled at once. In this specific gpio-keys case, irrespective of whether it has one or more keys, the pins aren't related apart from representing keys, and should thus better be individual pinctrl nodes and individually referenced in pinctrl-X. Did I sympathize that correctly? (side-note: the SDC pinctrl groups typically get extended with a card-detect pin in board DTS or in some likely-erroneous cases directly in SoC DTSI. This may also count as unrelated pins being grouped together only because that is how the hardware/DTS node consumes them, but it is rather concise/readable/convenient though...) - Marijn
On 18.12.2022 11:18, Marijn Suijten wrote: > On 2022-12-17 16:04:17, Konrad Dybcio wrote: >> On 17.12.2022 11:04, Marijn Suijten wrote: >>> [..] >>> @@ -270,6 +270,16 @@ &sdhc_1 { >>> >>> &tlmm { >>> gpio-reserved-ranges = <22 2>, <28 6>; >>> + >>> + gpio_keys_state: gpio-keys-state { >>> + key-volume-down-pins { >> I see no need for defining a wrapper node. >> The other changes look good! > > I did the same for sm6350-lena, which we should flatten out then too. > > For these uses I'm not sure when it's clearer/better to use: > > thing@x { > pinctrl-0 = <&thing_state>; > ... > }; > > thing_state: thing-state { > specific-pin { > ... > }; > > other-specific-pin ... > ... > }; > > Or separate out the pins with their own state and instead use: > > thing@x { > pinctrl-0 = <&specific_pin1_state &specific_pin2_state>; > ... > }; > > If I had to guess the former groups related pins together (as we finally > do now for SDC...) which should all be toggled at once. In this > specific gpio-keys case, irrespective of whether it has one or more > keys, the pins aren't related apart from representing keys, and should > thus better be individual pinctrl nodes and individually referenced in > pinctrl-X. > > Did I sympathize that correctly? I think so. > > (side-note: the SDC pinctrl groups typically get extended with a > card-detect pin in board DTS or in some likely-erroneous cases directly > in SoC DTSI. This may also count as unrelated pins being grouped > together only because that is how the hardware/DTS node consumes them, > but it is rather concise/readable/convenient though...) 8450 has: pinctrl-0 = <&sdc2_default_state &sdc2_card_det_n>; which seems like a sane application of what you described. Konrad > > - Marijn
On 2022-12-19 11:00:10, Konrad Dybcio wrote: > > > On 18.12.2022 11:18, Marijn Suijten wrote: > > On 2022-12-17 16:04:17, Konrad Dybcio wrote: > >> On 17.12.2022 11:04, Marijn Suijten wrote: > >>> [..] > >>> @@ -270,6 +270,16 @@ &sdhc_1 { > >>> > >>> &tlmm { > >>> gpio-reserved-ranges = <22 2>, <28 6>; > >>> + > >>> + gpio_keys_state: gpio-keys-state { > >>> + key-volume-down-pins { > >> I see no need for defining a wrapper node. > >> The other changes look good! > > > > I did the same for sm6350-lena, which we should flatten out then too. > > > > For these uses I'm not sure when it's clearer/better to use: > > > > thing@x { > > pinctrl-0 = <&thing_state>; > > ... > > }; > > > > thing_state: thing-state { > > specific-pin { > > ... > > }; > > > > other-specific-pin ... > > ... > > }; > > > > Or separate out the pins with their own state and instead use: > > > > thing@x { > > pinctrl-0 = <&specific_pin1_state &specific_pin2_state>; > > ... > > }; > > > > If I had to guess the former groups related pins together (as we finally > > do now for SDC...) which should all be toggled at once. In this > > specific gpio-keys case, irrespective of whether it has one or more > > keys, the pins aren't related apart from representing keys, and should > > thus better be individual pinctrl nodes and individually referenced in > > pinctrl-X. > > > > Did I sympathize that correctly? > I think so. Ack, will respin like this for V2. > > (side-note: the SDC pinctrl groups typically get extended with a > > card-detect pin in board DTS or in some likely-erroneous cases directly > > in SoC DTSI. This may also count as unrelated pins being grouped > > together only because that is how the hardware/DTS node consumes them, > > but it is rather concise/readable/convenient though...) > 8450 has: > > pinctrl-0 = <&sdc2_default_state &sdc2_card_det_n>; > > which seems like a sane application of what you described. Glad to hear we (I and sm8450 dts writer(s)) came to the same conclusion independently. Not sure if it's worth retroactively cleaning up existing DTS, but feel free to. There are still DT's out there that define all pins individually, too... - Marijn
diff --git a/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts b/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts index 1b9e40d3d269..8c7c9331fd21 100644 --- a/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts +++ b/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts @@ -41,17 +41,17 @@ extcon_usb: extcon-usb { }; gpio-keys { - status = "okay"; compatible = "gpio-keys"; - autorepeat; + pinctrl-0 = <&gpio_keys_state>; + pinctrl-names = "default"; - key-vol-dn { + key-volume-down { label = "Volume Down"; gpios = <&tlmm 47 GPIO_ACTIVE_LOW>; - linux,input-type = <1>; linux,code = <KEY_VOLUMEDOWN>; - gpio-key,wakeup; debounce-interval = <15>; + linux,can-disable; + gpio-key,wakeup; }; }; @@ -270,6 +270,16 @@ &sdhc_1 { &tlmm { gpio-reserved-ranges = <22 2>, <28 6>; + + gpio_keys_state: gpio-keys-state { + key-volume-down-pins { + pins = "gpio47"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + input-enable; + }; + }; }; &usb3 {
- Remove autorepeat (leave key repetition to userspace); - Remove unneeded status = "okay" (this is the default); - Allow the interrupt line for this button to be disabled; - Use a full, descriptive node name; - Set proper bias on the GPIO via pinctrl; - Sort properties. Fixes: 82e1783890b7 ("arm64: dts: qcom: sm6125: Add support for Sony Xperia 10II") Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- .../qcom/sm6125-sony-xperia-seine-pdx201.dts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)