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