| Message ID | 20181024193402.16698-2-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (ina3221) Implement PM runtime to save power | expand |
Quoting Nicolin Chen <nicoleotsuka@gmail.com>: > The new hdev is a child device related to the original parent > hwmon driver and its device. However, it doesn't support the > power features, typically being defined in the parent driver. > > So this patch inherits three necessary power properties from > the parent dev to hdev: power, pm_domain and driver pointers. > > Note that the dev->driver pointer is the place that contains > a dev_pm_ops pointer defined in the parent device driver and > the pm runtime core also checks this pointer: > if (!cb && dev->driver && dev->driver->pm) > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > Changelog > v2->v3: > * N/A > v1->v2: > * Added device pointers > > drivers/hwmon/hwmon.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > index 975c95169884..14cfab64649f 100644 > --- a/drivers/hwmon/hwmon.c > +++ b/drivers/hwmon/hwmon.c > @@ -625,7 +625,12 @@ __hwmon_device_register(struct device *dev, > const char *name, void *drvdata, > hwdev->name = name; > hdev->class = &hwmon_class; > hdev->parent = dev; > - hdev->of_node = dev ? dev->of_node : NULL; > + if (dev) { > + hdev->driver = dev->driver; > + hdev->power = dev->power; > + hdev->pm_domain = dev->pm_domain; > + hdev->of_node = dev->of_node; > + } We'l need to dig into this more; I suspect it may be inappropriate to do this. With this change, every hwmon driver supporting (runtime ?) suspend/resume will have the problem worked around in #5, and that just seems wrong. Guenter > hwdev->chip = chip; > dev_set_drvdata(hdev, drvdata); > dev_set_name(hdev, HWMON_ID_FORMAT, id); > -- > 2.17.1
On Thu, Oct 25, 2018 at 12:13:01AM +0000, linux@roeck-us.net wrote: > > + if (dev) { > > + hdev->driver = dev->driver; > > + hdev->power = dev->power; > > + hdev->pm_domain = dev->pm_domain; > > + hdev->of_node = dev->of_node; > > + } > > We'l need to dig into this more; I suspect it may be inappropriate to do this. > With this change, every hwmon driver supporting (runtime ?) suspend/resume > will have the problem worked around in #5, and that just seems wrong. Hmm..that's true...thanks for catching it. Actually I am not sure the reason of having a child device in the core, but could we use the parent dev pointer in the hwmon core as hwmon_dev upon confirming parent dev pointer != NULL? The problem here is that the power directory under each hwmon directory is tied to the hwmon_dev pointer, not to the parent dev pointer, and the hwmon core creates all sysfs nodes based on the child node. So those nodes under power directory won't be valid unless we copy all pm information, especially PM ops. There is an option of ignoring this problem though, while all hwmon drivers will need to be careful of mixing using the dev pointers. So it'd be a lot of easier if we could just use the original dev pointer in the core since we mainly just need to create sysfs nodes. Another way of doing this might be to pass down the PM pointer via _info structure instead of linking it to the parent driver, which then will forbid all hwmon drivers having its own PM ops callbacks -- the very opposite way of this patch, and it does not sound fully reasonable and feasible to me... What do you think about? Thanks Nicolin
On Wed, Oct 24, 2018 at 06:01:16PM -0700, Nicolin Chen wrote: > On Thu, Oct 25, 2018 at 12:13:01AM +0000, linux@roeck-us.net wrote: > > > > + if (dev) { > > > + hdev->driver = dev->driver; > > > + hdev->power = dev->power; > > > + hdev->pm_domain = dev->pm_domain; > > > + hdev->of_node = dev->of_node; > > > + } > > > > We'l need to dig into this more; I suspect it may be inappropriate to do this. > > With this change, every hwmon driver supporting (runtime ?) suspend/resume > > will have the problem worked around in #5, and that just seems wrong. > > Hmm..that's true...thanks for catching it. > > Actually I am not sure the reason of having a child device in > the core, but could we use the parent dev pointer in the hwmon > core as hwmon_dev upon confirming parent dev pointer != NULL? I just noticed that it is used to link to hwmon class. So this won't work then. I guess it'd be safer to ignore the problem of the power node, i.e. using parent dev pointer for pm runtime. Thanks Nicolin > The problem here is that the power directory under each hwmon > directory is tied to the hwmon_dev pointer, not to the parent > dev pointer, and the hwmon core creates all sysfs nodes based > on the child node. So those nodes under power directory won't > be valid unless we copy all pm information, especially PM ops. > > There is an option of ignoring this problem though, while all > hwmon drivers will need to be careful of mixing using the dev > pointers. So it'd be a lot of easier if we could just use the > original dev pointer in the core since we mainly just need to > create sysfs nodes. > > Another way of doing this might be to pass down the PM pointer > via _info structure instead of linking it to the parent driver, > which then will forbid all hwmon drivers having its own PM ops > callbacks -- the very opposite way of this patch, and it does > not sound fully reasonable and feasible to me... > > What do you think about? > > Thanks > Nicolin
Quoting Nicolin Chen <nicoleotsuka@gmail.com>: > On Wed, Oct 24, 2018 at 06:01:16PM -0700, Nicolin Chen wrote: >> On Thu, Oct 25, 2018 at 12:13:01AM +0000, linux@roeck-us.net wrote: >> >> > > + if (dev) { >> > > + hdev->driver = dev->driver; >> > > + hdev->power = dev->power; >> > > + hdev->pm_domain = dev->pm_domain; >> > > + hdev->of_node = dev->of_node; >> > > + } >> > >> > We'l need to dig into this more; I suspect it may be >> inappropriate to do this. >> > With this change, every hwmon driver supporting (runtime ?) suspend/resume >> > will have the problem worked around in #5, and that just seems wrong. >> >> Hmm..that's true...thanks for catching it. >> >> Actually I am not sure the reason of having a child device in >> the core, but could we use the parent dev pointer in the hwmon >> core as hwmon_dev upon confirming parent dev pointer != NULL? > > I just noticed that it is used to link to hwmon class. So this Exactly. > won't work then. I guess it'd be safer to ignore the problem of > the power node, i.e. using parent dev pointer for pm runtime. > It might be worthwhile looking up how other virtal devices handle this problem. Maybe the hwmon code could have its own suspend/resume callbacks. Not sure how to make that work, though, and what those callbacks would (have to) do. Guenter > Thanks > Nicolin > >> The problem here is that the power directory under each hwmon >> directory is tied to the hwmon_dev pointer, not to the parent >> dev pointer, and the hwmon core creates all sysfs nodes based >> on the child node. So those nodes under power directory won't >> be valid unless we copy all pm information, especially PM ops. >> >> There is an option of ignoring this problem though, while all >> hwmon drivers will need to be careful of mixing using the dev >> pointers. So it'd be a lot of easier if we could just use the >> original dev pointer in the core since we mainly just need to >> create sysfs nodes. >> >> Another way of doing this might be to pass down the PM pointer >> via _info structure instead of linking it to the parent driver, >> which then will forbid all hwmon drivers having its own PM ops >> callbacks -- the very opposite way of this patch, and it does >> not sound fully reasonable and feasible to me... >> >> What do you think about? >> >> Thanks >> Nicolin
On Thu, Oct 25, 2018 at 06:55:31AM +0000, linux@roeck-us.net wrote: > > won't work then. I guess it'd be safer to ignore the problem of > > the power node, i.e. using parent dev pointer for pm runtime. > > > It might be worthwhile looking up how other virtal devices handle > this problem. Maybe the hwmon code could have its own suspend/resume > callbacks. Not sure how to make that work, though, and what those > callbacks would (have to) do. I am adding a core pm pointer to the class. A hwmon driver will also need to pass a private pm pointer in a chip/info structure. I will send it soon. Thanks Nicolin
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c index 975c95169884..14cfab64649f 100644 --- a/drivers/hwmon/hwmon.c +++ b/drivers/hwmon/hwmon.c @@ -625,7 +625,12 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, hwdev->name = name; hdev->class = &hwmon_class; hdev->parent = dev; - hdev->of_node = dev ? dev->of_node : NULL; + if (dev) { + hdev->driver = dev->driver; + hdev->power = dev->power; + hdev->pm_domain = dev->pm_domain; + hdev->of_node = dev->of_node; + } hwdev->chip = chip; dev_set_drvdata(hdev, drvdata); dev_set_name(hdev, HWMON_ID_FORMAT, id);
The new hdev is a child device related to the original parent hwmon driver and its device. However, it doesn't support the power features, typically being defined in the parent driver. So this patch inherits three necessary power properties from the parent dev to hdev: power, pm_domain and driver pointers. Note that the dev->driver pointer is the place that contains a dev_pm_ops pointer defined in the parent device driver and the pm runtime core also checks this pointer: if (!cb && dev->driver && dev->driver->pm) Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v2->v3: * N/A v1->v2: * Added device pointers drivers/hwmon/hwmon.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)