On 22-12-20, 22:39, Dmitry Osipenko wrote: > 22.12.2020 22:21, Dmitry Osipenko пишет: > >>> + 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 > > > >> 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 ? > 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. -- viresh