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
