| Message ID | 20200525182608.1823735-2-kw@linux.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add helper for accessing Power Management callbacs | expand |
On Mon, May 25, 2020 at 06:26:01PM +0000, Krzysztof Wilczyński wrote: > Add driver_to_pm() helper allowing for accessing the Power Management > callbacs for a particular device. Access to the callbacs (struct > dev_pm_ops) is normally done through using the pm pointer that is > embedded within the device_driver struct. > > Helper allows for the code required to reference the pm pointer and > access Power Management callbas to be simplified. Changing the > following: > > struct device_driver *drv = dev->driver; > if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) { > int ret = dev->driver->pm->prepare(dev); > > To: > > const struct dev_pm_ops *pm = driver_to_pm(dev->driver); > if (pm && pm->prepare) { > int ret = pm->prepare(dev); > > Or, changing the following: > > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; > > To: > const struct dev_pm_ops *pm = driver_to_pm(dev->driver); > > Signed-off-by: Krzysztof Wilczyński <kw@linux.com> > --- > include/linux/device/driver.h | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index ee7ba5b5417e..ccd0b315fd93 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -236,6 +236,21 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) > } > #endif > > +/** > + * driver_to_pm - Return Power Management callbacs (struct dev_pm_ops) for > + * a particular device. > + * @drv: Pointer to a device (struct device_driver) for which you want to access > + * the Power Management callbacks. > + * > + * Returns a pointer to the struct dev_pm_ops embedded within the device (struct > + * device_driver), or returns NULL if Power Management is not present and the > + * pointer is not valid. > + */ > +static inline const struct dev_pm_ops *driver_to_pm(struct device_driver *drv) > +{ > + return drv && drv->pm ? drv->pm : NULL; I hate ? : lines with a passion, as they break normal pattern mattching in my brain. Please just spell this all out: if (drv && drv->pm) return drv->pm; return NULL; Much easier to read, and the compiler will do the exact same thing. Only place ? : are ok to use in my opinion, are as function arguments. thanks, greg k-h
On 5/26/20 1:33 AM, Greg Kroah-Hartman wrote: > On Mon, May 25, 2020 at 06:26:01PM +0000, Krzysztof Wilczyński wrote: >> Add driver_to_pm() helper allowing for accessing the Power Management >> callbacs for a particular device. Access to the callbacs (struct >> dev_pm_ops) is normally done through using the pm pointer that is >> embedded within the device_driver struct. >> >> Helper allows for the code required to reference the pm pointer and >> access Power Management callbas to be simplified. Changing the >> following: >> >> struct device_driver *drv = dev->driver; >> if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) { >> int ret = dev->driver->pm->prepare(dev); >> >> To: >> >> const struct dev_pm_ops *pm = driver_to_pm(dev->driver); >> if (pm && pm->prepare) { >> int ret = pm->prepare(dev); >> >> Or, changing the following: >> >> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; >> >> To: >> const struct dev_pm_ops *pm = driver_to_pm(dev->driver); >> >> Signed-off-by: Krzysztof Wilczyński <kw@linux.com> >> --- >> include/linux/device/driver.h | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h >> index ee7ba5b5417e..ccd0b315fd93 100644 >> --- a/include/linux/device/driver.h >> +++ b/include/linux/device/driver.h >> @@ -236,6 +236,21 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) >> } >> #endif >> >> +/** >> + * driver_to_pm - Return Power Management callbacs (struct dev_pm_ops) for >> + * a particular device. >> + * @drv: Pointer to a device (struct device_driver) for which you want to access >> + * the Power Management callbacks. >> + * >> + * Returns a pointer to the struct dev_pm_ops embedded within the device (struct >> + * device_driver), or returns NULL if Power Management is not present and the >> + * pointer is not valid. >> + */ >> +static inline const struct dev_pm_ops *driver_to_pm(struct device_driver *drv) >> +{ >> + return drv && drv->pm ? drv->pm : NULL; This could just be: if (drv) return drv->pm; return NULL; Or if you want to evoke passion in Greg: return drv ? drv->pm : NULL; -Alex > I hate ? : lines with a passion, as they break normal pattern mattching > in my brain. Please just spell this all out: > if (drv && drv->pm) > return drv->pm; > return NULL; > > Much easier to read, and the compiler will do the exact same thing. > > Only place ? : are ok to use in my opinion, are as function arguments. > > thanks, > > greg k-h > _______________________________________________ > greybus-dev mailing list > greybus-dev@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/greybus-dev >
Hello Alex and Greg, [...] > This could just be: > > if (drv) > return drv->pm; > > return NULL; > > Or if you want to evoke passion in Greg: > > return drv ? drv->pm : NULL; > > -Alex > > > I hate ? : lines with a passion, as they break normal pattern mattching > > in my brain. Please just spell this all out: > > if (drv && drv->pm) > > return drv->pm; > > return NULL; > > > > Much easier to read, and the compiler will do the exact same thing. > > > > Only place ? : are ok to use in my opinion, are as function arguments. I will steer away from the ternary operator next time. Also, good to learn about Greg's preference. Thank you both! Krzysztof
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index ee7ba5b5417e..ccd0b315fd93 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -236,6 +236,21 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) } #endif +/** + * driver_to_pm - Return Power Management callbacs (struct dev_pm_ops) for + * a particular device. + * @drv: Pointer to a device (struct device_driver) for which you want to access + * the Power Management callbacks. + * + * Returns a pointer to the struct dev_pm_ops embedded within the device (struct + * device_driver), or returns NULL if Power Management is not present and the + * pointer is not valid. + */ +static inline const struct dev_pm_ops *driver_to_pm(struct device_driver *drv) +{ + return drv && drv->pm ? drv->pm : NULL; +} + extern int driver_deferred_probe_timeout; void driver_deferred_probe_add(struct device *dev); int driver_deferred_probe_check_state(struct device *dev);
Add driver_to_pm() helper allowing for accessing the Power Management callbacs for a particular device. Access to the callbacs (struct dev_pm_ops) is normally done through using the pm pointer that is embedded within the device_driver struct. Helper allows for the code required to reference the pm pointer and access Power Management callbas to be simplified. Changing the following: struct device_driver *drv = dev->driver; if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) { int ret = dev->driver->pm->prepare(dev); To: const struct dev_pm_ops *pm = driver_to_pm(dev->driver); if (pm && pm->prepare) { int ret = pm->prepare(dev); Or, changing the following: const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL; To: const struct dev_pm_ops *pm = driver_to_pm(dev->driver); Signed-off-by: Krzysztof Wilczyński <kw@linux.com> --- include/linux/device/driver.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)