Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Richard Acayan <mailingradian@gmail.com>
To:     Bjorn Andersson <andersson@kernel.org>
Cc:     linux-arm-msm@vger.kernel.org, Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht,
        Richard Acayan <mailingradian@gmail.com>
Subject: Re: [PATCH 2/2] pinctrl: qcom: add sdm670 pinctrl
Date:   Thu, 15 Sep 2022 22:01:17 -0400
Message-Id: <20220916020117.227480-1-mailingradian@gmail.com>
In-Reply-To: <20220914023900.z64wugbq7p2gfb32@builder.lan>
References: 
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

> > diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfi=
g=0D
> > index 2961b5eb8e10..7aba4188110c 100644=0D
> > --- a/drivers/pinctrl/qcom/Kconfig=0D
> > +++ b/drivers/pinctrl/qcom/Kconfig=0D
> > @@ -283,6 +283,15 @@ config PINCTRL_SDM660=0D
> >  	 Qualcomm Technologies Inc TLMM block found on the Qualcomm=0D
> >  	 Technologies Inc SDM660 platform.=0D
> >  =0D
> > +config PINCTRL_SDM670=0D
> > +	tristate "Qualcomm Technologies Inc SDM670 pin controller driver"=0D
> > +	depends on (OF || ACPI)=0D
> =0D
> I believe you can drop ACPI from this?=0D
=0D
Yes, I adapted this driver from the SDM845 driver and removed the ACPI=0D
features but forgot to remove the config dependency.=0D
=0D
> > +	depends on PINCTRL_MSM=0D
> > +	help=0D
> > +	 This is the pinctrl, pinmux, pinconf and gpiolib driver for the=0D
> > +	 Qualcomm Technologies Inc TLMM block found on the Qualcomm=0D
> > +	 Technologies Inc SDM670 platform.=0D
> > +=0D
> >  config PINCTRL_SDM845=0D
> >  	tristate "Qualcomm Technologies Inc SDM845 pin controller driver"=0D
> >  	depends on (OF || ACPI)=0D
=0D
> > +/* Every pin is maintained as a single group, and missing or non-exist=
ing pin=0D
> > + * would be maintained as dummy group to synchronize pin group index w=
ith=0D
> > + * pin descriptor registered with pinctrl core.=0D
> > + * Clients would not be able to request these dummy pin groups.=0D
> =0D
> The client wouldn't be able to define pinmux/pinconf, but I'm not able=0D
> to spot anything that would prevent a client from referencing the gpio?=0D
> =0D
> Perhaps I'm missing something?=0D
=0D
No, you're not. I kept this comment because I saw it in other pinctrl=0D
drivers and thought it was standard:=0D
=0D
    ~/linux $ grep dummy -RC1 drivers/pinctrl/qcom/=0D
    drivers/pinctrl/qcom/pinctrl-qcs404.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-qcs404.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-qcs404.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-qcs404.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-qcs404.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sc7180.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sc7180.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sc7180.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sc7180.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sc7180.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sc7280.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sc7280.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sc7280.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sc7280.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sc7280.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sdx55.c-/* Every pin is maintained as a si=
ngle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sdx55.c: * would be maintained as dummy gr=
oup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sdx55.c- * pin descriptor registered with =
pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sdx55.c: * Clients would not be able to re=
quest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sdx55.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sdx65.c-/* Every pin is maintained as a si=
ngle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sdx65.c: * would be maintained as dummy gr=
oup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sdx65.c- * pin descriptor registered with =
pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sdx65.c: * Clients would not be able to re=
quest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sdx65.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm6115.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm6115.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm6115.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm6115.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm6115.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm8350.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm8350.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm8350.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm8350.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm8350.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-qcm2290.c-/* Every pin is maintained as a =
single group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-qcm2290.c: * would be maintained as dummy =
group to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-qcm2290.c- * pin descriptor registered wit=
h pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-qcm2290.c: * Clients would not be able to =
request these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-qcm2290.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm6125.c- * Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm6125.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm6125.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm6125.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm6125.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm6350.c- * Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm6350.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm6350.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm6350.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm6350.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm8150.c- * Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm8150.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm8150.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm8150.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm8150.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm8450.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm8450.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm8450.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm8450.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm8450.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sdm845.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sdm845.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sdm845.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sdm845.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sdm845.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm6375.c- * Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm6375.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm6375.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm6375.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm6375.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sm8250.c-/* Every pin is maintained as a s=
ingle group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sm8250.c: * would be maintained as dummy g=
roup to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sm8250.c- * pin descriptor registered with=
 pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sm8250.c: * Clients would not be able to r=
equest these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sm8250.c- */=0D
    --=0D
    drivers/pinctrl/qcom/pinctrl-sc8180x.c-/* Every pin is maintained as a =
single group, and missing or non-existing pin=0D
    drivers/pinctrl/qcom/pinctrl-sc8180x.c: * would be maintained as dummy =
group to synchronize pin group index with=0D
    drivers/pinctrl/qcom/pinctrl-sc8180x.c- * pin descriptor registered wit=
h pinctrl core.=0D
    drivers/pinctrl/qcom/pinctrl-sc8180x.c: * Clients would not be able to =
request these dummy pin groups.=0D
    drivers/pinctrl/qcom/pinctrl-sc8180x.c- */=0D
=0D
Since this driver has dummy pingroups, it is a bit confusing to see this=0D
inaccurate information because it is relevant. I'll rewrite the comment so=
=0D
that it makes sense.=0D
=0D
> Otherwise, I think you should be able to specify reserved_gpios in=0D
> sdm670_pinctrl and list the dummy items. This would ensure that the gpio=
=0D
> code as well treat them as absent.=0D
=0D
Yes, as long as I can reserve pins 0, 1, 2, 3, 81, 82, 83, and 84 for the=0D
Pixel 3a. However, I think reserved_gpios overrides the DT schema where it=
=0D
would be sensible to add it:=0D
=0D
drivers/pinctrl/qcom/pinctrl-msm.c:690:=0D
=0D
	/* Driver provided reserved list overrides DT and ACPI */=0D
=0D
Perhaps I should omit the dummy pingroups from the driver and try to handle=
=0D
the gpio numbers discrepency on the DT side, like:=0D
=0D
    gpio-ranges =3D <&tlmm 0 0 58>, <&tlmm 65 59 4>, ...=0D
=0D
I don't see this being done anywhere else but it should clear up the=0D
debugfs problems I was having.=0D
=0D
> > + */=0D
> > +static const struct msm_pingroup sdm670_groups[] =3D {=0D
> > +	PINGROUP(0, SOUTH, qup0, _, _, _, _, _, _, _, _),=0D
> > +	PINGROUP(1, SOUTH, qup0, _, _, _, _, _, _, _, _),=0D