Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver

23.12.2020 23:37, Dmitry Osipenko пишет:

23.12.2020 08:57, Viresh Kumar пишет:

quoted

On 22-12-20, 22:39, Dmitry Osipenko wrote:

quoted

22.12.2020 22:21, Dmitry Osipenko пишет:

quoted

quoted

quoted

+	if (IS_ERR(opp)) {
+		dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n",
+			level, opp);
+		return PTR_ERR(opp);
+	}
+
+	err = dev_pm_opp_set_voltage(&genpd->dev, opp);

IIUC, you implemented this callback because you want to use the voltage triplet
present in the OPP table ?

And so you are setting the regulator ("power") later in this patch ?

yes

quoted

I am not in favor of implementing this routine, as it just adds a wrapper above
the regulator API. What you should be doing rather is get the regulator by
yourself here (instead of depending on the OPP core). And then you can do
dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to
implement a version supporting triplet here though for the same.

And you won't require the sync version of the API as well then.

That's what I initially did for this driver. I don't mind to revert back
to the initial variant in v3, it appeared to me that it will be nicer
and cleaner to have OPP API managing everything here.

I forgot one important detail (why the initial variant wasn't good)..
OPP entries that have unsupportable voltages should be filtered out and
OPP core performs the filtering only if regulator is assigned to the OPP
table.

If regulator is assigned to the OPP table, then we need to use OPP API
for driving the regulator, hence that's why I added
dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().

Perhaps it should be possible to add dev_pm_opp_get_regulator() that

What's wrong with getting the regulator in the driver as well ? Apart from the
OPP core ?

The voltage syncing should be done for each consumer regulator
individually [1].

Secondly, regulator core doesn't work well today if the same regulator
is requested more than one time for the same device.

quoted

quoted

will return the OPP table regulator in order to allow driver to use the
regulator directly. But I'm not sure whether this is a much better
option than the opp_sync_regulators() and opp_set_voltage() APIs.

set_voltage() is still fine as there is some data that the OPP core has, but
sync_regulator() has nothing to do with OPP core.

And this may lead to more wrapper helpers in the OPP core, which I am afraid of.
And so even if it is not the best, I would like the OPP core to provide the data
and not get into this. Ofcourse there is an exception to this, opp_set_rate.

The regulator_sync_voltage() should be invoked only if voltage was
changed previously [1].

The OPP core already has the info about whether voltage was changed and
it provides the necessary locking for both set_voltage() and
sync_regulator(). Perhaps I'll need to duplicate that functionality in
the PD driver, instead of making it all generic and re-usable by other
drivers.

[1]
https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107

I just realized that the locking is missing in the v2 patches, something
to fix in the next revision :)

Still I'm in favor of extending the OPP API with the new common
functions. But if you have a strong opinion about that, then I'll try to
work around it in the PD driver in v3.