linux-kernel - Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE-0n509bg6RzieOtYuUvicU14D7bmgH-u02F1TB+hBZ+xH4CA@mail.gmail.com>
Date:   Fri, 18 Feb 2022 18:01:55 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Satya Priya <quic_c_skakit@...cinc.com>
Cc:     Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Das Srinagesh <gurus@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_collinsd@...cinc.com,
        quic_subbaram@...cinc.com, quic_jprakash@...cinc.com
Subject: Re: [PATCH V7 5/5] arm64: dts: qcom: sc7280: Add pm8008 support for sc7280-idp

Quoting Satya Priya (2022-02-18 03:01:03)
> Add pm8008_infra and pm8008_regulators support for sc7280 idp.
>
> Signed-off-by: Satya Priya <quic_c_skakit@...cinc.com>
> ---
> Changes in V2:
>  - As per Stephen's comments, replaced '_' with '-' for node names.
>
> Changes in V3:
>  - Changed the regulator node names as l1, l2 etc
>  - Changed "pm8008-regulators" to "regulators"
>  - Changed "qcom,min-dropout-voltage" to "regulator-min-dropout-voltage-microvolt"
>
> Changes in V4:
>  - Moved all common stuff to pm8008.dtsi and added board specific configurations here.
>
> Changes in V5:
>  - Changed the node names as per pm8008.dtsi
>  - Moved supply nodes to chip level (mfd node).
>  - Removed the regulator-mindropout property.
>
> Changes in V6:
>  - No changes.
>
> Changes in V7:
>  - No Changes.
>
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 66 ++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index ecbf2b8..371ad19 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -263,6 +263,62 @@
>         };
>  };
>
> +&i2c1 {

Can we add another phandle?

&pm8008_bus: &i2c1 {

> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       status = "okay";
> +
> +       #include "pm8008.dtsi"
> +};

And then

#include "pm8008.dtsi"

and have the pm8008.dtsi file add itself as a child of &pm8008_bus? Then
we can easily see that pm8008 is a child of pm8008_bus without having to
figure out where the file is included. It also helps avoid polluting the
i2c node with things that shouldn't be there in case we want to include
configuration bits in the pm8008.dtsi file that aren't directly related
to the bus node.

> +
> +&pm8008_infra {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pm8008_active>;
> +};
> +
> +&pm8008_regulators {
> +       vdd_l1_l2-supply = <&vreg_s8b_1p2>;
> +       vdd_l3_l4-supply = <&vreg_s1b_1p8>;
> +       vdd_l5-supply = <&vreg_bob>;
> +       vdd_l6-supply = <&vreg_bob>;
> +       vdd_l7-supply = <&vreg_bob>;
> +};
> +
> +&pm8008_l1 {
> +       regulator-min-microvolt = <950000>;
> +       regulator-max-microvolt = <1300000>;
> +};
> +
> +&pm8008_l2 {
> +       regulator-min-microvolt = <950000>;
> +       regulator-max-microvolt = <1250000>;
> +};
> +
> +&pm8008_l3 {
> +       regulator-min-microvolt = <1650000>;
> +       regulator-max-microvolt = <3000000>;
> +};
> +
> +&pm8008_l4 {
> +       regulator-min-microvolt = <1504000>;
> +       regulator-max-microvolt = <1600000>;
> +};
> +
> +&pm8008_l5 {
> +       regulator-min-microvolt = <2600000>;
> +       regulator-max-microvolt = <3000000>;
> +};
> +
> +&pm8008_l6 {
> +       regulator-min-microvolt = <2600000>;
> +       regulator-max-microvolt = <3000000>;
> +};
> +
> +&pm8008_l7 {
> +       regulator-min-microvolt = <3000000>;
> +       regulator-max-microvolt = <3544000>;
> +};
> +
>  &qfprom {
>         vcc-supply = <&vreg_l1c_1p8>;
>  };
> @@ -375,6 +431,16 @@
>         drive-strength = <2>;
>  };
>
> +&pm8350c_gpios {
> +       pm8008_active: pm8008_active {

No underscore in node names. pm8008_active: pm8008-active {

> +               pins = "gpio4";
> +               function = "normal";
> +               bias-disable;
> +               output-high;

Is this a reset signal? Should the driver be deasserting the reset when
it is ready? That could be the same time the gpio is acquired.

> +               power-source = <0>;
> +       };
> +};