diff mbox series

[1/8] driver core: Add helper for accessing Power Management callbacs

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

Commit Message

Krzysztof Wilczyński May 25, 2020, 6:26 p.m. UTC
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(+)

Comments

Greg KH May 26, 2020, 6:33 a.m. UTC | #1
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
Alex Elder May 26, 2020, 11:53 a.m. UTC | #2
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
>
Krzysztof Wilczyński May 26, 2020, 3:01 p.m. UTC | #3
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 mbox series

Patch

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);