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

[Matthias Kaehlcke <mka@chromium.org>]

On Thu, Nov 05, 2020 at 05:05:38PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 5, 2020 at 4:37 PM Matthias Kaehlcke <mka@chromium.org> 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.

You are right, it makes a certain sense to have them paired, I'll change it
even though it leads to a few more entries.

> > +};
> > +
> >  &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 */

ok

> > @@ -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?

Indeed, that was the intention, it didn't blow up into my face during testing
since the downstream tree doesn't have it anymore.