Re: [PATCH v2 2/2] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub

[Doug Anderson]

Hi,

On Thu, Nov 5, 2020 at 4:37 PM Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote:
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
> index 0a281c24841c..6603f2102233 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
> @@ -58,10 +58,23 @@ ap_ts: touchscreen@10 {
>         };
>  };
>
> +&pp3300_hub {
> +       /* pp3300_l7c is used to power the USB hub */
> +       /delete-property/regulator-always-on;
> +};
> +
> +&pp3300_l7c {
> +       regulator-always-on;

Personally I always end up pairing "always-on" and "boot-on", but that
might just be superstition from many kernel versions ago when there
were weird quirks.  The way you have it now you will sometimes have
"boot-on" but not "always-on".  Probably what you have is fine,
though.


> +};
> +
>  &sdhc_2 {
>         status = "okay";
>  };
>
> +&usb_hub {
> +        vdd-supply = <&pp3300_l7c>;
> +};
> +
>  /* PINCTRL - board-specific pinctrl */
>
>  &tlmm {
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index bf875589d364..50e733412a7f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -174,6 +174,25 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
>                 vin-supply = <&pp3300_a>;
>         };
>
> +       pp3300_hub: pp3300-hub {
> +               compatible = "regulator-fixed";
> +               regulator-name = "pp3300_hub";
> +
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +
> +               gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
> +               enable-active-high;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&en_pp3300_hub>;
> +
> +               /* AP turns on with en_pp3300_hub; always on for AP */

Delete the above comment.  It's obvious based on the properties in
this node.  Other similar comments are useful because they describe
how the _EC_ turns on regulators and why a regulator that has an
enable still looks like an "always-on" regulator to the AP (because
it's always on whenever the AP is on).

If you want to add a comment, you could say:

/* Always on until we have a way to specify it can go off in suspend */


> @@ -469,7 +488,6 @@ ppvar_l6c: ldo6 {
>                         regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>                 };
>
> -               pp3300_hub:
>                 pp3300_l7c: ldo7 {
>                         regulator-min-microvolt = <3304000>;
>                         regulator-max-microvolt = <3304000>;

Shouldn't you delete the "regulator-always-on;" from ldo7 since you're
adding it for all the older revs?