Re: [PATCH] arm64: dts: qcom: sm6125-seine: Clean up gpio-keys (volume down)

[Konrad Dybcio]

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