Add OPP and SoC core voltage scaling support to the Tegra SDHCI driver.
This is required for enabling system-wide DVFS on older Tegra SoCs.
Tested-by: Peter Geis <[email protected]>
Tested-by: Nicolas Chauvet <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/mmc/host/Kconfig | 1 +
drivers/mmc/host/sdhci-tegra.c | 70 ++++++++++++++++++++++++++++++++--
2 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 310e546e5898..7d719c81b917 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -293,6 +293,7 @@ config MMC_SDHCI_TEGRA
depends on MMC_SDHCI_PLTFM
select MMC_SDHCI_IO_ACCESSORS
select MMC_CQHCI
+ select PM_OPP
help
This selects the Tegra SD/MMC controller. If you have a Tegra
platform with SD or MMC devices, say Y or M here.
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index ed12aacb1c73..964709a3ccd6 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -14,6 +14,7 @@
#include <linux/io.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pm_opp.h>
#include <linux/pinctrl/consumer.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
@@ -754,10 +755,15 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
+ struct device *dev = mmc_dev(host->mmc);
unsigned long host_clk;
- if (!clock)
- return sdhci_set_clock(host, clock);
+ /* disable clock and then remove OPP performance/voltage vote */
+ if (!clock) {
+ sdhci_set_clock(host, clock);
+ dev_pm_opp_set_rate(dev, clock);
+ return;
+ }
/*
* In DDR50/52 modes the Tegra SDHCI controllers require the SDHCI
@@ -772,7 +778,7 @@ static void tegra_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
* from clk_get_rate() is used.
*/
host_clk = tegra_host->ddr_signaling ? clock * 2 : clock;
- clk_set_rate(pltfm_host->clk, host_clk);
+ dev_pm_opp_set_rate(dev, host_clk);
tegra_host->curr_clk_rate = host_clk;
if (tegra_host->ddr_signaling)
host->max_clk = host_clk;
@@ -1558,6 +1564,60 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
return ret;
}
+static void sdhci_tegra_deinit_opp_table(void *data)
+{
+ struct device *dev = data;
+ struct opp_table *opp_table;
+
+ opp_table = dev_pm_opp_get_opp_table(dev);
+ dev_pm_opp_of_remove_table(dev);
+ dev_pm_opp_put_regulators(opp_table);
+ dev_pm_opp_put_opp_table(opp_table);
+}
+
+static int devm_sdhci_tegra_init_opp_table(struct device *dev)
+{
+ struct opp_table *opp_table;
+ const char *rname = "core";
+ int err;
+
+ /* voltage scaling is optional */
+ if (device_property_present(dev, "core-supply"))
+ opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
+ else
+ opp_table = dev_pm_opp_get_opp_table(dev);
+
+ if (IS_ERR(opp_table))
+ return dev_err_probe(dev, PTR_ERR(opp_table),
+ "failed to prepare OPP table\n");
+
+ /*
+ * OPP table presence is optional and we want the set_rate() of OPP
+ * API to work similarly to clk_set_rate() if table is missing in a
+ * device-tree. The add_table() errors out if OPP is missing in DT.
+ */
+ if (device_property_present(dev, "operating-points-v2")) {
+ err = dev_pm_opp_of_add_table(dev);
+ if (err) {
+ dev_err(dev, "failed to add OPP table: %d\n", err);
+ goto put_table;
+ }
+ }
+
+ err = devm_add_action(dev, sdhci_tegra_deinit_opp_table, dev);
+ if (err)
+ goto remove_table;
+
+ return 0;
+
+remove_table:
+ dev_pm_opp_of_remove_table(dev);
+put_table:
+ dev_pm_opp_put_regulators(opp_table);
+
+ return err;
+}
+
static int sdhci_tegra_probe(struct platform_device *pdev)
{
const struct of_device_id *match;
@@ -1621,6 +1681,10 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
goto err_power_req;
}
+ rc = devm_sdhci_tegra_init_opp_table(&pdev->dev);
+ if (rc)
+ goto err_parse_dt;
+
/*
* Tegra210 has a separate SDMMC_LEGACY_TM clock used for host
* timeout clock and SW can choose TMCLK or SDCLK for hardware
--
2.27.0
On Thu, Nov 5, 2020 at 5:15 AM Dmitry Osipenko <[email protected]> wrote:
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> +static void sdhci_tegra_deinit_opp_table(void *data)
> +{
> + struct device *dev = data;
> + struct opp_table *opp_table;
> +
> + opp_table = dev_pm_opp_get_opp_table(dev);
So you need to get an OPP table to put one :)
You need to save the pointer returned by dev_pm_opp_set_regulators() instead.
> + dev_pm_opp_of_remove_table(dev);
> + dev_pm_opp_put_regulators(opp_table);
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +
> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
> +{
> + struct opp_table *opp_table;
> + const char *rname = "core";
> + int err;
> +
> + /* voltage scaling is optional */
> + if (device_property_present(dev, "core-supply"))
> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> + else
> + opp_table = dev_pm_opp_get_opp_table(dev);
Nice. I didn't think that someone will end up abusing this API and so made it
available for all, but someone just did that. I will fix that in the OPP core.
Any idea why you are doing what you are doing here ?
> +
> + if (IS_ERR(opp_table))
> + return dev_err_probe(dev, PTR_ERR(opp_table),
> + "failed to prepare OPP table\n");
> +
> + /*
> + * OPP table presence is optional and we want the set_rate() of OPP
> + * API to work similarly to clk_set_rate() if table is missing in a
> + * device-tree. The add_table() errors out if OPP is missing in DT.
> + */
> + if (device_property_present(dev, "operating-points-v2")) {
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + dev_err(dev, "failed to add OPP table: %d\n", err);
> + goto put_table;
> + }
> + }
> +
> + err = devm_add_action(dev, sdhci_tegra_deinit_opp_table, dev);
> + if (err)
> + goto remove_table;
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(dev);
> +put_table:
> + dev_pm_opp_put_regulators(opp_table);
> +
> + return err;
> +}
> +
> static int sdhci_tegra_probe(struct platform_device *pdev)
> {
> const struct of_device_id *match;
> @@ -1621,6 +1681,10 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
> goto err_power_req;
> }
>
> + rc = devm_sdhci_tegra_init_opp_table(&pdev->dev);
> + if (rc)
> + goto err_parse_dt;
> +
> /*
> * Tegra210 has a separate SDMMC_LEGACY_TM clock used for host
> * timeout clock and SW can choose TMCLK or SDCLK for hardware
> --
> 2.27.0
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
05.11.2020 12:58, Viresh Kumar пишет:
>> +static void sdhci_tegra_deinit_opp_table(void *data)
>> +{
>> + struct device *dev = data;
>> + struct opp_table *opp_table;
>> +
>> + opp_table = dev_pm_opp_get_opp_table(dev);
> So you need to get an OPP table to put one :)
> You need to save the pointer returned by dev_pm_opp_set_regulators() instead.
This is intentional because why do we need to save the pointer if we're
not using it and we know that we could get this pointer using OPP API?
This is exactly the same what I did for the CPUFreq driver [1] :)
[1]
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/cpufreq/tegra20-cpufreq.c#L97
>> + dev_pm_opp_of_remove_table(dev);
>> + dev_pm_opp_put_regulators(opp_table);
>> + dev_pm_opp_put_opp_table(opp_table);
>> +}
>> +
>> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
>> +{
>> + struct opp_table *opp_table;
>> + const char *rname = "core";
>> + int err;
>> +
>> + /* voltage scaling is optional */
>> + if (device_property_present(dev, "core-supply"))
>> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
>> + else
>
>> + opp_table = dev_pm_opp_get_opp_table(dev);
> Nice. I didn't think that someone will end up abusing this API and so made it
> available for all, but someone just did that. I will fix that in the OPP core.
The dev_pm_opp_put_regulators() handles the case where regulator is
missing by acting as dev_pm_opp_get_opp_table(), but the
dev_pm_opp_set_regulators() doesn't do it. Hence I don't think this is
an abuse, but the OPP API drawback.
> Any idea why you are doing what you are doing here ?
Two reasons:
1. Voltage regulator is optional, but dev_pm_opp_set_regulators()
doesn't support optional regulators.
2. We need to balance the opp_table refcount in order to use OPP API
without polluting code with if(have_regulator), hence the
dev_pm_opp_get_opp_table() is needed for taking the opp_table reference
to have the same refcount as in the case of the dev_pm_opp_set_regulators().
I guess we could make dev_pm_opp_set_regulators(dev, count) to accept
regulators count=0 and then act as dev_pm_opp_get_opp_table(dev), will
it be acceptable?
On 05-11-20, 17:18, Dmitry Osipenko wrote:
> 05.11.2020 12:58, Viresh Kumar пишет:
> >> +static void sdhci_tegra_deinit_opp_table(void *data)
> >> +{
> >> + struct device *dev = data;
> >> + struct opp_table *opp_table;
> >> +
> >> + opp_table = dev_pm_opp_get_opp_table(dev);
> > So you need to get an OPP table to put one :)
> > You need to save the pointer returned by dev_pm_opp_set_regulators() instead.
>
> This is intentional because why do we need to save the pointer if we're
> not using it and we know that we could get this pointer using OPP API?
Because it is highly inefficient and it doesn't follow the rules set
by the OPP core. Hypothetically speaking, the OPP core is free to
allocate the OPP table structure as much as it wants, and if you don't
use the value returned back to you earlier (think of it as a cookie
assigned to your driver), then it will eventually lead to memory leak.
> This is exactly the same what I did for the CPUFreq driver [1] :)
I will strongly suggest you to save the pointer here and do the same
in the cpufreq driver as well.
> >> +static int devm_sdhci_tegra_init_opp_table(struct device *dev)
> >> +{
> >> + struct opp_table *opp_table;
> >> + const char *rname = "core";
> >> + int err;
> >> +
> >> + /* voltage scaling is optional */
> >> + if (device_property_present(dev, "core-supply"))
> >> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> >> + else
> >
> >> + opp_table = dev_pm_opp_get_opp_table(dev);
To make it further clear, this will end up allocating an OPP table for
you, which it shouldn't have.
> > Nice. I didn't think that someone will end up abusing this API and so made it
> > available for all, but someone just did that. I will fix that in the OPP core.
To be fair, I allowed the cpufreq-dt driver to abuse it too, which I
am going to fix shortly.
> The dev_pm_opp_put_regulators() handles the case where regulator is
> missing by acting as dev_pm_opp_get_opp_table(), but the
> dev_pm_opp_set_regulators() doesn't do it. Hence I don't think this is
> an abuse, but the OPP API drawback.
I am not sure what you meant here. Normally you are required to call
dev_pm_opp_put_regulators() only if you have called
dev_pm_opp_set_regulators() earlier. And the refcount stays in
balance.
> > Any idea why you are doing what you are doing here ?
>
> Two reasons:
>
> 1. Voltage regulator is optional, but dev_pm_opp_set_regulators()
> doesn't support optional regulators.
>
> 2. We need to balance the opp_table refcount in order to use OPP API
> without polluting code with if(have_regulator), hence the
> dev_pm_opp_get_opp_table() is needed for taking the opp_table reference
> to have the same refcount as in the case of the dev_pm_opp_set_regulators().
I am going to send a patchset shortly after which this call to
dev_pm_opp_get_opp_table() will fail, if it is called before adding
the OPP table.
> I guess we could make dev_pm_opp_set_regulators(dev, count) to accept
> regulators count=0 and then act as dev_pm_opp_get_opp_table(dev), will
> it be acceptable?
Setting regulators for count as 0 doesn't sound good to me.
But, I understand that you don't want to have that if (have_regulator)
check, and it is a fair request. What I will instead do is, allow all
dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
table and fail silently. And so you won't be required to have this
unwanted check. But you will be required to save the pointer returned
back by dev_pm_opp_set_regulators(), which is the right thing to do
anyways.
--
viresh
06.11.2020 09:15, Viresh Kumar пишет:
> Setting regulators for count as 0 doesn't sound good to me.
>
> But, I understand that you don't want to have that if (have_regulator)
> check, and it is a fair request. What I will instead do is, allow all
> dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
> table and fail silently. And so you won't be required to have this
> unwanted check. But you will be required to save the pointer returned
> back by dev_pm_opp_set_regulators(), which is the right thing to do
> anyways.
Perhaps even a better variant could be to add a devm versions of the OPP
API functions, then drivers won't need to care about storing the
opp_table pointer if it's unused by drivers.
On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko <[email protected]> wrote:
>
> 06.11.2020 09:15, Viresh Kumar пишет:
> > Setting regulators for count as 0 doesn't sound good to me.
> >
> > But, I understand that you don't want to have that if (have_regulator)
> > check, and it is a fair request. What I will instead do is, allow all
> > dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
> > table and fail silently. And so you won't be required to have this
> > unwanted check. But you will be required to save the pointer returned
> > back by dev_pm_opp_set_regulators(), which is the right thing to do
> > anyways.
>
> Perhaps even a better variant could be to add a devm versions of the OPP
> API functions, then drivers won't need to care about storing the
> opp_table pointer if it's unused by drivers.
I think so. The consumer may not be so concerned about the status of
these OPP tables.
If the driver needs to manage the release, it needs to add a pointer
to their driver global structure.
Maybe it's worth having these devm interfaces for opp.
Yangtao
On 06-11-20, 21:41, Frank Lee wrote:
> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko <[email protected]> wrote:
> >
> > 06.11.2020 09:15, Viresh Kumar пишет:
> > > Setting regulators for count as 0 doesn't sound good to me.
> > >
> > > But, I understand that you don't want to have that if (have_regulator)
> > > check, and it is a fair request. What I will instead do is, allow all
> > > dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
> > > table and fail silently. And so you won't be required to have this
> > > unwanted check. But you will be required to save the pointer returned
> > > back by dev_pm_opp_set_regulators(), which is the right thing to do
> > > anyways.
> >
> > Perhaps even a better variant could be to add a devm versions of the OPP
> > API functions, then drivers won't need to care about storing the
> > opp_table pointer if it's unused by drivers.
>
> I think so. The consumer may not be so concerned about the status of
> these OPP tables.
> If the driver needs to manage the release, it needs to add a pointer
> to their driver global structure.
>
> Maybe it's worth having these devm interfaces for opp.
Sure if there are enough users of this, I am all for it. I was fine
with the patches you sent, just that there were not a lot of users of
it and so I pushed them back. If we find that we have more users of it
now, we can surely get that back.
--
viresh
09.11.2020 08:00, Viresh Kumar пишет:
> On 06-11-20, 21:41, Frank Lee wrote:
>> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko <[email protected]> wrote:
>>>
>>> 06.11.2020 09:15, Viresh Kumar пишет:
>>>> Setting regulators for count as 0 doesn't sound good to me.
>>>>
>>>> But, I understand that you don't want to have that if (have_regulator)
>>>> check, and it is a fair request. What I will instead do is, allow all
>>>> dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
>>>> table and fail silently. And so you won't be required to have this
>>>> unwanted check. But you will be required to save the pointer returned
>>>> back by dev_pm_opp_set_regulators(), which is the right thing to do
>>>> anyways.
>>>
>>> Perhaps even a better variant could be to add a devm versions of the OPP
>>> API functions, then drivers won't need to care about storing the
>>> opp_table pointer if it's unused by drivers.
>>
>> I think so. The consumer may not be so concerned about the status of
>> these OPP tables.
>> If the driver needs to manage the release, it needs to add a pointer
>> to their driver global structure.
>>
>> Maybe it's worth having these devm interfaces for opp.
>
> Sure if there are enough users of this, I am all for it. I was fine
> with the patches you sent, just that there were not a lot of users of
> it and so I pushed them back. If we find that we have more users of it
> now, we can surely get that back.
>
There was already attempt to add the devm? Could you please give me a
link to the patches?
I already prepared a patch which adds the devm helpers. It helps to keep
code cleaner and readable.
On 09-11-20, 08:08, Dmitry Osipenko wrote:
> 09.11.2020 08:00, Viresh Kumar пишет:
> > On 06-11-20, 21:41, Frank Lee wrote:
> >> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko <[email protected]> wrote:
> >>>
> >>> 06.11.2020 09:15, Viresh Kumar пишет:
> >>>> Setting regulators for count as 0 doesn't sound good to me.
> >>>>
> >>>> But, I understand that you don't want to have that if (have_regulator)
> >>>> check, and it is a fair request. What I will instead do is, allow all
> >>>> dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
> >>>> table and fail silently. And so you won't be required to have this
> >>>> unwanted check. But you will be required to save the pointer returned
> >>>> back by dev_pm_opp_set_regulators(), which is the right thing to do
> >>>> anyways.
> >>>
> >>> Perhaps even a better variant could be to add a devm versions of the OPP
> >>> API functions, then drivers won't need to care about storing the
> >>> opp_table pointer if it's unused by drivers.
> >>
> >> I think so. The consumer may not be so concerned about the status of
> >> these OPP tables.
> >> If the driver needs to manage the release, it needs to add a pointer
> >> to their driver global structure.
> >>
> >> Maybe it's worth having these devm interfaces for opp.
> >
> > Sure if there are enough users of this, I am all for it. I was fine
> > with the patches you sent, just that there were not a lot of users of
> > it and so I pushed them back. If we find that we have more users of it
> > now, we can surely get that back.
> >
>
> There was already attempt to add the devm? Could you please give me a
> link to the patches?
>
> I already prepared a patch which adds the devm helpers. It helps to keep
> code cleaner and readable.
https://lore.kernel.org/lkml/[email protected]/
--
viresh
09.11.2020 08:10, Viresh Kumar пишет:
> On 09-11-20, 08:08, Dmitry Osipenko wrote:
>> 09.11.2020 08:00, Viresh Kumar пишет:
>>> On 06-11-20, 21:41, Frank Lee wrote:
>>>> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko <[email protected]> wrote:
>>>>>
>>>>> 06.11.2020 09:15, Viresh Kumar пишет:
>>>>>> Setting regulators for count as 0 doesn't sound good to me.
>>>>>>
>>>>>> But, I understand that you don't want to have that if (have_regulator)
>>>>>> check, and it is a fair request. What I will instead do is, allow all
>>>>>> dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
>>>>>> table and fail silently. And so you won't be required to have this
>>>>>> unwanted check. But you will be required to save the pointer returned
>>>>>> back by dev_pm_opp_set_regulators(), which is the right thing to do
>>>>>> anyways.
>>>>>
>>>>> Perhaps even a better variant could be to add a devm versions of the OPP
>>>>> API functions, then drivers won't need to care about storing the
>>>>> opp_table pointer if it's unused by drivers.
>>>>
>>>> I think so. The consumer may not be so concerned about the status of
>>>> these OPP tables.
>>>> If the driver needs to manage the release, it needs to add a pointer
>>>> to their driver global structure.
>>>>
>>>> Maybe it's worth having these devm interfaces for opp.
>>>
>>> Sure if there are enough users of this, I am all for it. I was fine
>>> with the patches you sent, just that there were not a lot of users of
>>> it and so I pushed them back. If we find that we have more users of it
>>> now, we can surely get that back.
>>>
>>
>> There was already attempt to add the devm? Could you please give me a
>> link to the patches?
>>
>> I already prepared a patch which adds the devm helpers. It helps to keep
>> code cleaner and readable.
>
> https://lore.kernel.org/lkml/[email protected]/
>
Thanks, I made it in a different way by simply adding helpers to the
pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
add new kernel symbols.
static inline int devm_pm_opp_of_add_table(struct device *dev)
{
int err;
err = dev_pm_opp_of_add_table(dev);
if (err)
return err;
err = devm_add_action_or_reset(dev, (void*)dev_pm_opp_remove_table,
dev);
if (err)
return err;
return 0;
}
On 09-11-20, 08:19, Dmitry Osipenko wrote:
> Thanks, I made it in a different way by simply adding helpers to the
> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> add new kernel symbols.
I will prefer to add it in core.c itself, and yes
devm_add_action_or_reset() looks better. But I am still not sure for
which helpers do we need the devm_*() variants, as this is only useful
for non-CPU devices. But if we have users that we can add right now,
why not.
> static inline int devm_pm_opp_of_add_table(struct device *dev)
> {
> int err;
>
> err = dev_pm_opp_of_add_table(dev);
> if (err)
> return err;
>
> err = devm_add_action_or_reset(dev, (void*)dev_pm_opp_remove_table,
> dev);
> if (err)
> return err;
>
> return 0;
> }
--
viresh
09.11.2020 08:35, Viresh Kumar пишет:
> On 09-11-20, 08:19, Dmitry Osipenko wrote:
>> Thanks, I made it in a different way by simply adding helpers to the
>> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
>> add new kernel symbols.
>
> I will prefer to add it in core.c itself, and yes
> devm_add_action_or_reset() looks better. But I am still not sure for
> which helpers do we need the devm_*() variants, as this is only useful
> for non-CPU devices. But if we have users that we can add right now,
> why not.
All current non-CPU drivers (devfreq, mmc, memory, etc) can benefit from it.
For Tegra drivers we need these variants:
devm_pm_opp_set_supported_hw()
devm_pm_opp_set_regulators() [if we won't use GENPD]
devm_pm_opp_set_clkname()
devm_pm_opp_of_add_table()
On 09-11-20, 08:44, Dmitry Osipenko wrote:
> 09.11.2020 08:35, Viresh Kumar пишет:
> > On 09-11-20, 08:19, Dmitry Osipenko wrote:
> >> Thanks, I made it in a different way by simply adding helpers to the
> >> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> >> add new kernel symbols.
> >
> > I will prefer to add it in core.c itself, and yes
> > devm_add_action_or_reset() looks better. But I am still not sure for
> > which helpers do we need the devm_*() variants, as this is only useful
> > for non-CPU devices. But if we have users that we can add right now,
> > why not.
>
> All current non-CPU drivers (devfreq, mmc, memory, etc) can benefit from it.
>
> For Tegra drivers we need these variants:
>
> devm_pm_opp_set_supported_hw()
> devm_pm_opp_set_regulators() [if we won't use GENPD]
> devm_pm_opp_set_clkname()
> devm_pm_opp_of_add_table()
I tried to look earlier for the stuff already merged in and didn't
find a lot of stuff where the devm_* could be used, maybe I missed
some of it.
Frank, would you like to refresh your series based on suggestions from
Dmitry and make other drivers adapt to the new APIs ?
--
viresh
On Mon, Nov 9, 2020 at 1:53 PM Viresh Kumar <[email protected]> wrote:
>
> On 09-11-20, 08:44, Dmitry Osipenko wrote:
> > 09.11.2020 08:35, Viresh Kumar пишет:
> > > On 09-11-20, 08:19, Dmitry Osipenko wrote:
> > >> Thanks, I made it in a different way by simply adding helpers to the
> > >> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> > >> add new kernel symbols.
> > >
> > > I will prefer to add it in core.c itself, and yes
> > > devm_add_action_or_reset() looks better. But I am still not sure for
> > > which helpers do we need the devm_*() variants, as this is only useful
> > > for non-CPU devices. But if we have users that we can add right now,
> > > why not.
> >
> > All current non-CPU drivers (devfreq, mmc, memory, etc) can benefit from it.
> >
> > For Tegra drivers we need these variants:
> >
> > devm_pm_opp_set_supported_hw()
> > devm_pm_opp_set_regulators() [if we won't use GENPD]
> > devm_pm_opp_set_clkname()
> > devm_pm_opp_of_add_table()
>
> I tried to look earlier for the stuff already merged in and didn't
> find a lot of stuff where the devm_* could be used, maybe I missed
> some of it.
>
> Frank, would you like to refresh your series based on suggestions from
> Dmitry and make other drivers adapt to the new APIs ?
I am glad to do this.：）
Yangtao
On Mon, 9 Nov 2020 at 16:51, Frank Lee <[email protected]> wrote:
> On Mon, Nov 9, 2020 at 1:53 PM Viresh Kumar <[email protected]> wrote:
> > > devm_pm_opp_set_supported_hw()
> > > devm_pm_opp_set_regulators() [if we won't use GENPD]
> > > devm_pm_opp_set_clkname()
> > > devm_pm_opp_of_add_table()
> >
> > I tried to look earlier for the stuff already merged in and didn't
> > find a lot of stuff where the devm_* could be used, maybe I missed
> > some of it.
> >
> > Frank, would you like to refresh your series based on suggestions from
> > Dmitry and make other drivers adapt to the new APIs ?
>
> I am glad to do this.：）
Frank,
Dmitry has submitted a series with a patch that does stuff like this since you
never resent your patches.
http://lore.kernel.org/lkml/[email protected]
Since you were the first one to get to this, I would still like to
give you a chance
to get these patches merged under your authorship, otherwise I would be going
to pick patches from Dmitry.
--
viresh