linux-kernel - Re: [PATCH 2/2] pinctrl: qcom: add sdm670 pinctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 15 Sep 2022 22:01:17 -0400
From:   Richard Acayan <mailingradian@...il.com>
To:     Bjorn Andersson <andersson@...nel.org>
Cc:     linux-arm-msm@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        Richard Acayan <mailingradian@...il.com>
Subject: Re: [PATCH 2/2] pinctrl: qcom: add sdm670 pinctrl

> > diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> > index 2961b5eb8e10..7aba4188110c 100644
> > --- a/drivers/pinctrl/qcom/Kconfig
> > +++ b/drivers/pinctrl/qcom/Kconfig
> > @@ -283,6 +283,15 @@ config PINCTRL_SDM660
> >  	 Qualcomm Technologies Inc TLMM block found on the Qualcomm
> >  	 Technologies Inc SDM660 platform.
> >  
> > +config PINCTRL_SDM670
> > +	tristate "Qualcomm Technologies Inc SDM670 pin controller driver"
> > +	depends on (OF || ACPI)
> 
> I believe you can drop ACPI from this?

Yes, I adapted this driver from the SDM845 driver and removed the ACPI
features but forgot to remove the config dependency.

> > +	depends on PINCTRL_MSM
> > +	help
> > +	 This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > +	 Qualcomm Technologies Inc TLMM block found on the Qualcomm
> > +	 Technologies Inc SDM670 platform.
> > +
> >  config PINCTRL_SDM845
> >  	tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
> >  	depends on (OF || ACPI)

> > +/* Every pin is maintained as a single group, and missing or non-existing pin
> > + * would be maintained as dummy group to synchronize pin group index with
> > + * pin descriptor registered with pinctrl core.
> > + * Clients would not be able to request these dummy pin groups.
> 
> The client wouldn't be able to define pinmux/pinconf, but I'm not able
> to spot anything that would prevent a client from referencing the gpio?
> 
> Perhaps I'm missing something?

No, you're not. I kept this comment because I saw it in other pinctrl
drivers and thought it was standard:

    ~/linux $ grep dummy -RC1 drivers/pinctrl/qcom/
    drivers/pinctrl/qcom/pinctrl-qcs404.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-qcs404.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-qcs404.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-qcs404.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-qcs404.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sc7180.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sc7180.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sc7180.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sc7180.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sc7180.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sc7280.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sc7280.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sc7280.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sc7280.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sc7280.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sdx55.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sdx55.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sdx55.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sdx55.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sdx55.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sdx65.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sdx65.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sdx65.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sdx65.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sdx65.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm6115.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm6115.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm6115.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm6115.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm6115.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm8350.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm8350.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm8350.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm8350.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm8350.c- */
    --
    drivers/pinctrl/qcom/pinctrl-qcm2290.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-qcm2290.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-qcm2290.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-qcm2290.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-qcm2290.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm6125.c- * Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm6125.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm6125.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm6125.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm6125.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm6350.c- * Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm6350.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm6350.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm6350.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm6350.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm8150.c- * Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm8150.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm8150.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm8150.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm8150.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm8450.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm8450.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm8450.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm8450.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm8450.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sdm845.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sdm845.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sdm845.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sdm845.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sdm845.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm6375.c- * Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm6375.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm6375.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm6375.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm6375.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sm8250.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sm8250.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sm8250.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sm8250.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sm8250.c- */
    --
    drivers/pinctrl/qcom/pinctrl-sc8180x.c-/* Every pin is maintained as a single group, and missing or non-existing pin
    drivers/pinctrl/qcom/pinctrl-sc8180x.c: * would be maintained as dummy group to synchronize pin group index with
    drivers/pinctrl/qcom/pinctrl-sc8180x.c- * pin descriptor registered with pinctrl core.
    drivers/pinctrl/qcom/pinctrl-sc8180x.c: * Clients would not be able to request these dummy pin groups.
    drivers/pinctrl/qcom/pinctrl-sc8180x.c- */

Since this driver has dummy pingroups, it is a bit confusing to see this
inaccurate information because it is relevant. I'll rewrite the comment so
that it makes sense.

> Otherwise, I think you should be able to specify reserved_gpios in
> sdm670_pinctrl and list the dummy items. This would ensure that the gpio
> code as well treat them as absent.

Yes, as long as I can reserve pins 0, 1, 2, 3, 81, 82, 83, and 84 for the
Pixel 3a. However, I think reserved_gpios overrides the DT schema where it
would be sensible to add it:

drivers/pinctrl/qcom/pinctrl-msm.c:690:

	/* Driver provided reserved list overrides DT and ACPI */

Perhaps I should omit the dummy pingroups from the driver and try to handle
the gpio numbers discrepency on the DT side, like:

    gpio-ranges = <&tlmm 0 0 58>, <&tlmm 65 59 4>, ...

I don't see this being done anywhere else but it should clear up the
debugfs problems I was having.

> > + */
> > +static const struct msm_pingroup sdm670_groups[] = {
> > +	PINGROUP(0, SOUTH, qup0, _, _, _, _, _, _, _, _),
> > +	PINGROUP(1, SOUTH, qup0, _, _, _, _, _, _, _, _),