On Thu, Sep 05, 2019 at 10:37:53AM +0100, Steven Price wrote: > Ah, I didn't realise that regulator_get() will return a dummy regulator > if none is provided in the DT. In theory that seems like a nicer > solution to my two commits. However there's still a problem - the dummy > regulator returned from regulator_get() reports errors when > regulator_set_voltage() is called. So I get errors like this: > [ 299.861165] panfrost e82c0000.mali: Cannot set voltage 1100000 uV > [ 299.867294] devfreq devfreq0: dvfs failed with (-22) error > (And therefore the frequency isn't being changed) > Ideally we want a dummy regulator that will silently ignore any > regulator_set_voltage() calls. Is that safe? You can't rely on being able to change voltages even if there's a physical regulator available, system constraints or the results of sharing the regulator with other users may prevent changes. I guess at the minute the code is assuming that if you can't vary the regulator it's fixed at the maximum voltage and that it's safe to run at a lower clock with a higher voltage (some devices don't like doing that). If the device always starts up at full speed I guess that's OK. It's certainly in general a bad idea to do this in general, we can't tell how important it is to the consumer that they actually get the voltage that they asked for - for some applications like this it's just adding to the power saving it's likely fine but for others it might break things. If you're happy to change the frequency without the ability to vary the voltage you can query what's supported through the API (the simplest interface is regulator_is_supported_voltage()). You should do the regulator API queries at initialization time since they can be a bit expensive, the usual pattern would be to go through your OPP table and disable states where you can't support the voltage but you *could* also flag states where you just don't set the voltage. That seems especially reasonable if no voltages in the range the device supports can be set. I do note that the current code requires exactly specified voltages with no variation which doesn't match the behaviour you say you're OK with here, what you're describing sounds like the driver should be specifying a voltage range from the hardware specified maximum down to whatever the minimum the OPP supports rather than exactly the OPP voltage. As things are you might also run into voltages that can't be hit exactly (eg, in the Exynos 5433 case in mainline a regulator that only offers steps of 2mV will error out trying to set several of the OPPs).
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
