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
