Re: [Patch v4 4/7] thermal: Add generic power domain warming device driver.

[Thara Gopinath]

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