linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thara Gopinath <thara.gopinath@linaro.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Eduardo Valentin <edubezval@gmail.com>,
	Zhang Rui <rui.zhang@intel.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Andy Gross <agross@kernel.org>,
	Amit Kucheria <amit.kucheria@verdurent.com>,
	Mark Rutland <mark.rutland@arm.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux PM <linux-pm@vger.kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [Patch v4 4/7] thermal: Add generic power domain warming device driver.
Date: Fri, 13 Mar 2020 11:02:58 -0400	[thread overview]
Message-ID: <decacd00-9c41-a664-6329-dca494fd7302@linaro.org> (raw)
In-Reply-To: <CAPDyKFotwoiDkF6Ru39rOv+GxF7dgXG6tc0oQHMim8yKB2hRvQ@mail.gmail.com>

Hi Ulf,

Thanks for the reviews. Will send out v5 soon.

On 3/13/20 9:13 AM, Ulf Hansson wrote:
> [...]
> 
>>>> +static struct thermal_cooling_device_ops pd_warming_device_ops = {
>>>> +       .get_max_state  = ::pd_wdev_get_max_state,
>>>> +       .get_cur_state  = pd_wdev_get_cur_state,
>>>> +       .set_cur_state  = pd_wdev_set_cur_state,
>>>> +};
>>>> +
>>>> +struct thermal_cooling_device *
>>>> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id)
>>>
>>> Maybe rename this to: thermal_of_pd_warming_register()
>>
>> How about pd_of_warming_register? It is consistent with other cooling
>> device register like cpuidle_of_cooling_register and
>> cpufreq_of_cooling_register.
> 
> Well, we actually have the following:
> of_devfreq_cooling_register()
> of_cpufreq_cooling_register()
> cpuidle_of_cooling_register()
> 
> So maybe this is the most consistent. :-)
> of_pd_warming_register()

Sure!

> 
>>
>>> Moreover, I think you could replace the "struct device *parent", with
>>> a "struct device_node *node" as in-parameter. That's all you need,
>>> right?
>>
>> You mean pd_wdev->dev.parent need not be populated ? The device
>> in this case will be created under /sys/devices which I do not think
>> is the correct.
> 
> Good point!
> 
>> With a parent device specified, the power controller that resides the
>> power domain that can act as the warming dev, becomes the parent of the
>> warming dev. In case of this patch series, for the mx warming dev,
>> 179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The
>> device will be created under
>> /sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/)
>>
>> Other way might be to register the warming device under virtual devices
>> as a new category of devices.
> 
> No, that sounds wrong.
> 
> Another option is to create a specific bus type for these new
> pd_warming devices. But I admit that sounds a bit too much, let's
> assign a parent.
> 
>>
>> I prefer to keep it as a child of the power controller device, but I am
>> open to explore other options and to re-do this bit. What do you think?
> 
> Sure, sorry for the noise.

No issues!

> 
>>
>>>
>>>> +{
>>>> +       struct pd_warming_device *pd_wdev;
>>>> +       struct of_phandle_args pd_args;
>>>> +       int ret;
>>>> +
>>>> +       pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL);
>>>> +       if (!pd_wdev)
>>>> +               return ERR_PTR(-ENOMEM);
>>>> +
>>>> +       dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name);
>>>
>>> Perhaps skip the in-param *pd_name and make use of the suggested
>>> "struct device_node *node", the index and something with "warming...",
>>> when setting the name.
>>
>> Won't the index have to be in-param in this case ?
> 
> Isn't that already the case?
> 
> Huh, long time since I reviewed this.
> 
>>
>>>
>>> Just an idea, as to simplify for the caller.
>>>
>>>> +       pd_wdev->dev.parent = parent;
>>>
>>> This isn't needed, I think.
> 
> So ignore this comment, as discussed above.
> 
>>>
>>>> +
>>>> +       ret = device_register(&pd_wdev->dev);
>>>> +       if (ret)
>>>> +               goto error;
>>>> +
>>>> +       pd_args.np = parent->of_node;
>>>> +       pd_args.args[0] = pd_id;
>>>> +       pd_args.args_count = 1;
>>>> +
>>>> +       ret = of_genpd_add_device(&pd_args, &pd_wdev->dev);
>>>> +
>>>
>>> White space.
>>
>> Will fix it.
>>
>>>
>>>> +       if (ret)
>>>> +               goto error;
>>>> +
>>>> +       ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev);
>>>> +       if (ret < 0)
>>>> +               goto error;
>>>> +
>>>> +       pd_wdev->max_state = ret - 1;
>>>> +       pm_runtime_enable(&pd_wdev->dev);
>>>> +       pd_wdev->runtime_resumed = false;
>>>> +
>>>> +       pd_wdev->cdev = thermal_of_cooling_device_parent_register
>>>> +                                       (NULL, parent, pd_name, pd_wdev,
>>>> +                                        &pd_warming_device_ops);
>>>
>>> As stated in patch3, I don't get it why you need to use this new API
>>> for "parents".
>>
>> I agree with you. I cannot re-collect my thought process for this API.
>> I compiled and tested using the regular API and everything works fine.
>> I will drop patch 3 and use the thermal_of_cooling_device_register here.
> 
> Great, one confusing piece seems to go away then. :-)
> 
>>
>>>
>>>> +       if (IS_ERR(pd_wdev->cdev)) {
>>>> +               pr_err("unable to register %s cooling device\n", pd_name);
>>>> +               pm_runtime_disable(&pd_wdev->dev);
>>>> +               ret = PTR_ERR(pd_wdev->cdev);
>>>> +               goto error;
>>>> +       }
>>>> +
>>>> +       return pd_wdev->cdev;
>>>> +error:
>>>> +       put_device(&pd_wdev->dev);
>>>
>>> If device_register() succeeds you need to call device_unregister(),
>>> rather than put_device() as a part of the error handling.
>>
>> Will fix this.
>>
>>>
>>>> +       kfree(pd_wdev);
>>>
>>> You need a ->release() callback to manage kfree(), after you called
>>> device_register().
>>
>> mm?? I did not get this. What release callback? You mean for power
>> controller driver to call ?
> 
> No, this how life cycle management of devices should be implemented.
> 
> Have a look at genpd_release_dev() - and see how that is being used
> for genpd's virtual devices, that should explain more.

Ah yes. I get it now. Will fix this.

-- 
Warm Regards
Thara

  reply	other threads:[~2020-03-13 15:03 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20 12:56 [Patch v4 0/7] Introduce Power domain based " Thara Gopinath
2019-11-20 12:56 ` [Patch v4 1/7] PM/Domains: Add support for retrieving genpd performance states information Thara Gopinath
2019-11-20 12:56 ` [Patch v4 2/7] soc: qcom: rpmhpd: Introduce function to retrieve power domain performance state count Thara Gopinath
2020-02-04 16:10   ` Ulf Hansson
2019-11-20 12:56 ` [Patch v4 3/7] thermal: core: Allow cooling devices to register a parent Thara Gopinath
2020-02-04 16:22   ` Ulf Hansson
2019-11-20 12:56 ` [Patch v4 4/7] thermal: Add generic power domain warming device driver Thara Gopinath
2020-02-04 16:54   ` Ulf Hansson
2020-03-01 23:00     ` Thara Gopinath
2020-03-13 13:13       ` Ulf Hansson
2020-03-13 15:02         ` Thara Gopinath [this message]
2019-11-20 12:56 ` [Patch v4 5/7] soc: qcom: Extend RPMh power controller driver to register warming devices Thara Gopinath
2020-02-04 17:40   ` Ulf Hansson
2020-03-01 23:36     ` Thara Gopinath
2019-11-20 12:56 ` [Patch v4 6/7] dt-bindings: soc: qcom: Extend RPMh power controller binding to describe thermal warming device Thara Gopinath
2019-11-22 23:59   ` Rob Herring
2020-02-04 17:41   ` Ulf Hansson
2020-03-01 23:37     ` Thara Gopinath
2019-11-20 12:56 ` [Patch v4 7/7] arm64: dts: qcom: Indicate rpmhpd hosts a power domain that can be used as a " Thara Gopinath
2020-02-04 17:40   ` Ulf Hansson
2020-01-09 15:02 ` [Patch v4 0/7] Introduce Power domain based warming device driver Thara Gopinath

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=decacd00-9c41-a664-6329-dca494fd7302@linaro.org \
    --to=thara.gopinath@linaro.org \
    --cc=agross@kernel.org \
    --cc=amit.kucheria@verdurent.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=edubezval@gmail.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=rjw@rjwysocki.net \
    --cc=rui.zhang@intel.com \
    --cc=ulf.hansson@linaro.org \
    --subject='Re: [Patch v4 4/7] thermal: Add generic power domain warming device driver.' \
    /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

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).