Message-ID: <20190906105523.GR23391@sirena.co.uk>
Date:   Fri, 6 Sep 2019 11:55:23 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Steven Price <steven.price@....com>
Cc:     David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

On Fri, Sep 06, 2019 at 11:00:53AM +0100, Steven Price wrote:
> On 05/09/2019 17:34, Mark Brown wrote:

> > that information, I'd recommend eliminating individual OPPs if some are
> > supported or just never doing any regulator configuration if none can be
> > set.

> The problem on the Hikey960 is that the voltage control is not done by
> Linux. At the moment I have a DT with a set of operating-points:

Like I said above:

| > > that information, I'd recommend eliminating individual OPPs if some are
| > > supported or just never doing any regulator configuration if none can be
| > > set.

> But while Linux can set the frequency (via the mailbox interface) the
> voltages are not set by Linux but are implicit by choosing a frequency.
> At the moment my DT has a clock but no regulator and with the code in
> drm-next this works.

If you're just not getting any voltages for the OPPs then your
code shouldn't be trying to set voltages in the first place,
regulator or not.

> Your change swapping devm_regulator_get_optional() to
> devm_regulator_get() breaks this because that will return a dummy
> regulator which will reject any regulator_set_voltage() calls.

OTOH the current code won't work on a system which does specify a
regulator but doesn't have voltages configured in the OPP table
or where the regulator constraints for the board haven't been
configured to allow the voltage to be varied (perhaps the voltage
bit hasn't been worked out, perhaps the voltages were just added
to the OPPs in the SoC DT and the board constraints weren't
updated to allow voltage variation).

> I don't currently see how I can write a DT configuration for the
> Hikey960 which would work with the devm_regulator_get() call.

Like I've been saying you can discover if you can configure
individual voltages and use that information to either suppress
the OPPs concerned or just skip setting voltages for them, my
suggestion is to suppress OPPs only if you can set some of them.
At the very least if you don't have a voltage at all on a given
OPP you should skip the set.

> > However you're probably better off hiding all this stuff with the
> > generic OPP code rather than open coding it - this already has much
> > better handling for this, it supports voltage ranges rather than single
> > voltages and optional regulators already.  I'm not 100% clear why this
> > is open coded TBH but I might be missing something, if there's some
> > restriction preventing the generic code being used it seems like those
> > sohuld be fixed.

> To be honest I've no idea how to use the generic OPP code to do this. I
> suspect the original open coding was cargo-culted from another driver:
> the comments in the function look like they were lifted from
> drivers/devfreq/rk3399_dmc.c. Any help tidying this up would be appreciated.

Yes, there's a lot of cargo culting of bad regulator API usage in
the DRM subsystem for some reason, I keep having to do these
periodic sweeps and there's always stuff in there.  I think a lot
of it comes from BSP code that just gets dropped in without
review and then cut'n'pasted but I've not figured out why DRM is
so badly affected.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists