On 06-11-20, 21:41, Frank Lee wrote:
On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko digetx@gmail.com 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.
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 digetx@gmail.com 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 digetx@gmail.com 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/20201012135517.19468-1-frank@allwinnertech.com/
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 digetx@gmail.com 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/20201012135517.19468-1-frank@allwinnertech.com/
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; }
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 ?
On Mon, Nov 9, 2020 at 1:53 PM Viresh Kumar viresh.kumar@linaro.org 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 tiny.windzz@gmail.com wrote:
On Mon, Nov 9, 2020 at 1:53 PM Viresh Kumar viresh.kumar@linaro.org 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/20201217180638.22748-14-digetx@gmail.com
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
dri-devel@lists.freedesktop.org
-
Dmitry Osipenko -
Frank Lee -
Viresh Kumar