On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
>
> On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> > On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> > >
> > > Hi, Ulf,
> > >
> > > On 22.05.2025 14:53, Ulf Hansson wrote:
> > > >
> > > > That said, I think adding a devm_pm_domain_attach() interface would
> > > > make perfect sense. Then we can try to replace
> > > > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > > > devm_pm_domain_attach(). In this way, we should preserve the
> > > > expectation for drivers around devres for PM domains. Even if it would
> > > > change the behaviour for some drivers, it still sounds like the
> > > > correct thing to do in my opinion.
> > >
> > > This looks good to me, as well. I did prototype it on my side and tested on
> > > all my failure cases and it works.
> >
> > That's great! I am happy to help review, if/when you decide to post it.
>
> So you are saying you'd be OK with essentially the following (with
> devm_pm_domain_attach() actually being elsewhere in a real patch and not
> necessarily mimicked by devm_add_action_or_reset()):
Correct!
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index cfccf3ff36e7..1e017bfa5caf 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
> return 0;
> }
>
> +
> +static void platform_pm_domain_detach(void *d)
> +{
> + dev_pm_domain_detach(d, true);
> +}
Well, I would not limit this to the platform bus, even if that is the
most widely used.
Let's add the new generic interface along with
dev_pm_domain_attach|detach* and friends instead.
Then we can convert bus level code (and others), such as the platform
bus to use it, in a step-by-step approach.
> +
> +static int devm_pm_domain_attach(struct device *dev)
> +{
> + int error;
> +
> + error = dev_pm_domain_attach(dev, true);
> + if (error)
> + return error;
> +
> + error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
> + if (error)
> + return error;
> +
> + return 0;
> +}
> +
> static int platform_probe(struct device *_dev)
> {
> struct platform_driver *drv = to_platform_driver(_dev->driver);
> @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
> if (ret < 0)
> return ret;
>
> - ret = dev_pm_domain_attach(_dev, true);
> + ret = devm_pm_domain_attach(_dev);
> if (ret)
> goto out;
>
> - if (drv->probe) {
> + if (drv->probe)
> ret = drv->probe(dev);
> - if (ret)
> - dev_pm_domain_detach(_dev, true);
> - }
>
> out:
> if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
>
> if (drv->remove)
> drv->remove(dev);
> - dev_pm_domain_detach(_dev, true);
> }
>
> static void platform_shutdown(struct device *_dev)
>
>
> If so, then OK, it will work for me as well. This achieves the
> same behavior as with using devres group. The only difference is that if
> we ever need to extend the platform bus to acquire/release more
> resources they will also have to use devm API and not the regular one.
Sounds reasonable to me! Thanks for a nice discussion!
When it comes to the devm_pm_runtime_enable() API, I think we
seriously should consider removing it. Let me have a closer look at
that.
Kind regards
Uffe
