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
next prev parent 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).