Re: [PATCH 1/2] dt-bindings: pinctrl: qcom: Add SM6350 pinctrl bindings

[Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>]

>>> +        #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +        pinctrl@f100000 {
>>> +                compatible = "qcom,sm6350-tlmm";
>>> +                reg = <0x0f100000 0x300000>;
>>> +                interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 211 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 212 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 213 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 214 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>,
>>> +                            <GIC_SPI 216 IRQ_TYPE_LEVEL_HIGH>;
>>> +                gpio-controller;
>>> +                #gpio-cells = <2>;
>>> +                interrupt-controller;
>>> +                #interrupt-cells = <2>;
>>> +                gpio-ranges = <&tlmm 0 0 156>;
>> Shouldn't this be 157?

Yes, it should. Good catch.


>>
>>> +
>>> +                gpio-wo-subnode-state {
>>> +                        pins = "gpio1";
>>> +                        function = "gpio";
>>> +                };
>>> +
>>> +                uart-w-subnodes-state {
>>> +                        rx {
>>> +                                pins = "gpio25";
>>> +                                function = "qup13_f2";
>>> +                                bias-disable;
>>> +                        };
>>> +
>>> +                        tx {
>>> +                                pins = "gpio26";
>>> +                                function = "qup13_f2";
>>> +                                bias-disable;
>>> +                        };
>>> +                };
>>> +        };
>>> +...
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
>>> index 3b37cf102d41..99975122a2ce 100644
>>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
>>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,tlmm-common.yaml
>>> @@ -17,7 +17,7 @@ properties:
>>>     interrupts:
>>>       description:
>>>         Specifies the TLMM summary IRQ
>>> -    maxItems: 1
>>> +    maxItems: 9
>> Is this to support direct connected interrupts?
>>
>> Don't you need to add minItems: 1, to permit the other bindings to not
>> define these? I think that's what Rob's automatic reply complains about
>> at least.
>>
>>
>> PS. Any plans to work up support for direct connected interrupts? I
>> think that and "egpio" is the only downstream delta these days... That
>> said, I don't know if anyone actually uses direct connected interrupts?

I haven't really gotten into that piece yet, trying to get the platform up first..



>
> Using .wakeirq_dual_edge_errata = true, in pinctrl-sm6350.c (msm_pinctrl_soc_data structure) in [1] should help. The direct connect interrupt were added to support dual edge in downstream driver but in upstream setting this flag should help.
>
> This was used in sc7180 but should apply SM6350 too.
>
> That way you don't need other TLMM interrupts to be listed here.
>
> [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/20210820203751.232645-3-konrad.dybcio@xxxxxxxxxxxxxx/
>
> Thanks,
> Maulik
>
Thanks, I'll check and respin a v2 with that.


Konrad