Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200302205700.29746-1-daniel.baluta@oss.nxp.com>
 <20200302205700.29746-2-daniel.baluta@oss.nxp.com> <CAFQqKeU8YF+aZVTafj3ZiPvNUsx3nK-8cdr8eJUm=_9_2TkRQg@mail.gmail.com>
In-Reply-To: <CAFQqKeU8YF+aZVTafj3ZiPvNUsx3nK-8cdr8eJUm=_9_2TkRQg@mail.gmail.com>
From:   Daniel Baluta <daniel.baluta@gmail.com>
Date:   Tue, 3 Mar 2020 15:29:34 +0200
Message-ID: <CAEnQRZBNMRNYOKwqweuZCeOYgMGh9DprRvz2ci3EOQPxqN3fhg@mail.gmail.com>
Subject: Re: [RFC PATCH 1/2] PM / domains: Introduce multi PM domains helpers
To:     "Sridharan, Ranjani" <ranjani.sridharan@intel.com>
Cc:     Daniel Baluta <daniel.baluta@oss.nxp.com>, rjw@rjwysocki.net,
        "Brown, Len" <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Fabio Estevam <festevam@gmail.com>,
        Linux-ALSA <alsa-devel@alsa-project.org>,
        linux-pm@vger.kernel.org, Greg KH <gregkh@linuxfoundation.org>,
        Sascha Hauer <s.hauer@pengutronix.de>, khilman@kernel.org,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Daniel Baluta <daniel.baluta@nxp.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Paul Olaru <paul.olaru@nxp.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        "S.j. Wang" <shengjiu.wang@nxp.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Ranjani,

On Mon, Mar 2, 2020 at 11:24 PM Sridharan, Ranjani
<ranjani.sridharan@intel.com> wrote:

>> + */
>> +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)
>
> Hi Daniel,
>
> Just out of curiosity, should it be an error when num_domains is 1? Is it an error because the expectation is that the caller would use dev_pm_domain_attach() in that case?

NULL here doesn't really mean an error. It means that we don't need to
handle Power domains because as you said the caller
already used dev_pm_domain_attach.

Similar with this:

$ drivers/base/power/domain.c +2504

int genpd_dev_pm_attach(struct device *dev)

         / * Devices with multiple PM domains must be attached separately, as we
           * can only attach one PM domain per device.
          */
        if (of_count_phandle_with_args(dev->of_node, "power-domains",
                                    "#power-domain-cells") != 1)
               return 0;

Will update the description for when this function returns a NULL.

>
>> +               return NULL;
>> +
>> +       mpd = devm_kzalloc(dev, GFP_KERNEL, sizeof(*mpd));
>> +       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];
>
> Should retp be PTR_ERR(mpd->virt_devs[i]) here?

PTR_ERR returns a long but our function needs to return struct
dev_multi_pm_domain_data *.

> Thanks,
> Ranjani