From: Ulf Hansson <ulf.hansson@linaro.org>
To: Daniel Baluta <daniel.baluta@oss.nxp.com>
Cc: Kevin Hilman <khilman@kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Sascha Hauer <kernel@pengutronix.de>,
alsa-devel@alsa-project.org, dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH v3 1/2] PM / domains: Introduce multi PM domains helpers
Date: Tue, 7 Jul 2020 15:54:01 +0200 [thread overview]
Message-ID: <CAPDyKFr8BTkM2W_qbXK1KxtuoZC=uO14_uY9zCWvzfyU0h1a6Q@mail.gmail.com> (raw)
In-Reply-To: <20200624103247.7115-2-daniel.baluta@oss.nxp.com>
On Wed, 24 Jun 2020 at 12:33, Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
>
> From: Daniel Baluta <daniel.baluta@nxp.com>
>
> This patch introduces helpers support for multi PM domains.
>
> API consists of:
>
> 1) dev_multi_pm_attach - powers up all PM domains associated with a given
> device. Because we can attach one PM domain per device, we create
> virtual devices (children of initial device) and associate PM domains
> one per virtual device.
>
> 2) dev_multi_pm_detach - detaches all virtual devices from PM domains
> attached with.
Nit pick:
I suggest to rename the helpers into
dev_pm_domain_attach|detach_multi(), to be more consistent with
existing function names. It's a bit long I admit that, but I prefer
the consistency.
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> ---
> drivers/base/power/common.c | 93 +++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 19 ++++++++
> 2 files changed, 112 insertions(+)
>
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index bbddb267c2e6..b0a4d0109810 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -228,3 +228,96 @@ void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd)
> device_pm_check_callbacks(dev);
> }
> EXPORT_SYMBOL_GPL(dev_pm_domain_set);
> +
> +/**
> + * dev_multi_pm_attach - power up device associated power domains
> + * @dev: The device used to lookup the PM domains
> + *
> + * Parse device's OF node to find all PM domains specifiers. For each power
> + * domain found, create a virtual device and associate it with the
> + * current power domain.
> + *
> + * This function should typically be invoked by a driver during the
> + * probe phase, in the case its device requires power management through
> + * multiple PM domains.
> + *
> + * Returns a pointer to @dev_multi_pm_domain_data if successfully attached PM
> + * domains, NULL when the device doesn't need a PM domain or when single
> + * power-domains exists for it, else an ERR_PTR() in case of
> + * failures.
> + */
> +struct dev_multi_pm_domain_data *dev_multi_pm_attach(struct device *dev)
> +{
> + struct dev_multi_pm_domain_data *mpd, *retp;
> + int num_domains;
> + int i;
> +
> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + if (num_domains < 2)
> + return NULL;
dev_pm_domain_attach_* is typically wrapper functions, allowing
different types of PM domains to be supported. For example,
dev_pm_domain_attach() calls acpi_dev_pm_attach() and
genpd_dev_pm_attach(). While dev_pm_domain_attach_by_id() only calls
genpd_dev_pm_attach_by_id(), as there's no corresponding interface for
the acpi PM domain.
The above said, I don't think another layer should be needed here, but
there is something missing that makes this consistent with the
behaviour of the above mentioned functions.
How about adding a genpd OF helper ("of_genpd_num_domains(struct
device_node *)"), that deals with the above parsing and returns the
number of domains for the device? In this way, if
of_genpd_num_domains() returns an error code or zero, it's easier to
continue to try with other PM domain providers (if/when that is
supported).
> +
> + mpd = devm_kzalloc(dev, sizeof(*mpd), GFP_KERNEL);
> + if (!mpd)
> + return ERR_PTR(-ENOMEM);
> +
> + mpd->dev = dev;
> + mpd->num_domains = num_domains;
> +
> + mpd->virt_devs = devm_kmalloc_array(dev, mpd->num_domains,
> + sizeof(*mpd->virt_devs),
> + GFP_KERNEL);
> + if (!mpd->virt_devs)
> + return ERR_PTR(-ENOMEM);
> +
> + mpd->links = devm_kmalloc_array(dev, mpd->num_domains,
> + sizeof(*mpd->links), GFP_KERNEL);
> + if (!mpd->links)
> + return ERR_PTR(-ENOMEM);
> +
> + for (i = 0; i < mpd->num_domains; i++) {
> + mpd->virt_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(mpd->virt_devs[i])) {
> + retp = (struct dev_multi_pm_domain_data *)
> + mpd->virt_devs[i];
> + goto exit_unroll_pm;
> + }
> + mpd->links[i] = device_link_add(dev, mpd->virt_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
As a suggestion to be a little bit more flexible, perhaps these bits
should be given as an in-parameter instead. Potentially we could then
also treat the in-parameter being zero, as that no device link should
be added.
Although, it's kind of hard to know as the users of this interface
aren't really widely known yet.
> + if (!mpd->links[i]) {
> + retp = ERR_PTR(-ENOMEM);
> + dev_pm_domain_detach(mpd->virt_devs[i], false);
> + goto exit_unroll_pm;
> + }
> + }
> + return mpd;
> +
> +exit_unroll_pm:
> + while (--i >= 0) {
> + device_link_del(mpd->links[i]);
> + dev_pm_domain_detach(mpd->virt_devs[i], false);
> + }
> +
> + return retp;
> +}
> +EXPORT_SYMBOL(dev_multi_pm_attach);
> +
[...]
Kind regards
Uffe
next prev parent reply other threads:[~2020-07-07 13:54 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-24 10:32 [PATCH v3 0/2] Introduce multi PM domains helpers Daniel Baluta
2020-06-24 10:32 ` [PATCH v3 1/2] PM / domains: " Daniel Baluta
2020-07-07 13:54 ` Ulf Hansson [this message]
2020-06-24 10:32 ` [PATCH v3 2/2] ASoC: SOF: Use " Daniel Baluta
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPDyKFr8BTkM2W_qbXK1KxtuoZC=uO14_uY9zCWvzfyU0h1a6Q@mail.gmail.com' \
--to=ulf.hansson@linaro.org \
--cc=alsa-devel@alsa-project.org \
--cc=daniel.baluta@oss.nxp.com \
--cc=kernel@pengutronix.de \
--cc=khilman@kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).