[Aarch64-laptops] [EOAN][UNSTABLE][PATCH] UBUNTU: SAUCE: arm64: dts: qcom: Add support for AArch64 laptops

[Seth Forshee]

On Thu, Aug 08, 2019 at 09:52:29AM -0700, Bjorn Andersson wrote:
> On Thu 08 Aug 08:10 PDT 2019, Seth Forshee wrote:
> 
> > On Tue, Jul 30, 2019 at 03:28:05PM -0400, Dimitri John Ledkov wrote:
> > > From: AArch64 Laptops <aarch64laptops at gmail.com>
> > > 
> > > This is squashed merged of multiple patches from
> > > https://github.com/aarch64-laptops/linux/tree/laptops-ubuntu
> > > 
> > > Note that some of these, with different names are getting upstreamed
> > > targetted for v5.4. Once those are all merged and available, this
> > > patch will become obsolete and will be dropped.
> > 
> > Are they in linux-next or a maintainer tree already? If so I think it
> > would be preferable to cherry pick the individual patches.
> > 
> > Otherwise, adding some device trees seems pretty harmless. But one
> > comment ...
> > 
> > <snip>
> > 
> > > diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> > > index b2c2d01d1637..aa0272497be4 100644
> > > --- a/drivers/regulator/qcom-rpmh-regulator.c
> > > +++ b/drivers/regulator/qcom-rpmh-regulator.c
> > > @@ -207,7 +207,7 @@ static int rpmh_regulator_is_enabled(struct regulator_dev *rdev)
> > >  {
> > >  	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> > >  
> > > -	return vreg->enabled;
> > > +	return vreg->enabled > 0;
> > >  }
> > 
> > This is clearly a bug in the driver. I think it should be a separate
> > patch from the dts files, and it should be able to go into 5.3.
> > 
> 
> The rpmh regulator driver does not have a way to query the state of the
> hardware during initialization, so it has to cache the current state;
> with is initialized to -EINVAL.
> 
> So the upstream driver returns an error here, which according to the
> regulator driver api seems to be the right thing to do. Then, iirc, the
> regulator core interprets this as the regulator being on (!0) and
> requests it to be turned off - which causes the display to be powered
> down.
> 
> So this interaction needs to be investigated further to come up with a
> proper solution.
> 
> 
> PS. Note that the display drivers might very well be loaded way after
> regulator_init_complete(), where the regulator framework will turn off
> any unused regulators. Any lying in the driver (always returning false,
> like I do here) will cause unused regulators to stay on forever.

Hmm, okay. I hadn't actually looked at the documentation, which does say
>0 means the regulator is enabled. But pretty much all the code treats
the return as a boolean. That does complicate things.

Thanks,
Seth