[PATCH 0/3] introduce assign_if() macros in attempt to reduce ifdeffery

[PATCH 0/3] introduce assign_if() macros in attempt to reduce ifdeffery

[Josh Cartwright]

Based on the observation that given the following:

	static void my_callback(void)
	{
	}
	void (*callback)(void) = 0 ? my_callback : NULL;

GCC will,

	1.) Not emit 'defined but unused' warnings for 'my_callback'
	2.) Typecheck my_callback against typeof(*callback)
	3.) Eliminate my_callback as dead code (assuming it's unused elsewhere)

this patchset explores where/how this might be used to reduce the amount of
ifdeffed code in device drivers.

Patch 1 in this series introduces two friendly-named helper macros:

	assign_if(const_expr, fn):   A friendlier form of (const_expr ? fn : NULL)
	assign_if_enabled(conf, fn): Composition of assign_if() and IS_ENABLED().

In order to show a good target usecase for these macros, patch 2 introduces a
new set of PM-related macros, similar to SET_*_PM_OPS, using
assign_if_enabled() under the hood.

In particular, this is aimed at reducing code like the following:

	#ifdef CONFIG_PM_SLEEP
	static int foo_suspend(struct device *dev)
	{
		/* ... */
	}
	#else
	#define foo_suspend NULL
	#endif

	static const struct dev_pm_ops foo_pm_ops = {
		SET_SYSTEM_SLEEP_PM_OPS(foo_suspend, NULL)
	};

With the ifdefless:

	static int foo_suspend(struct device *dev)
	{
		/* ... */
	}

	static const struct dev_pm_ops foo_pm_ops = {
		ASSIGN_SYSTEM_SLEEP_PM_OPS(foo_suspend, NULL),
	};

For patch 3, an existing consumer of SET_*_PM_OPS(), the MSM USB phy driver[1]
is modified to use the newly introduced ASSIGN_*_PM_OPS() macros to show in a
real case what simplifications could be gained.

For testing that GCC is actually eliminating the PM-related callbacks when it
can, the MSM USB phy driver was built before and after patch 3, in 4 different
configurations, and object sizes compared[2]:

   text	   data	    bss	    dec	    hex	filename
  11008	    488	     20	  11516	   2cfc	phy_objects_before/phy-msm-usb.nopm.o
  11008	    488	     20	  11516	   2cfc	phy_objects_after/phy-msm-usb.nopm.o

  13528	    584	     20	  14132	   3734	phy_objects_before/phy-msm-usb.runtime.o
  13528	    584	     20	  14132	   3734	phy_objects_after/phy-msm-usb.runtime.o

  13828	    632	     20	  14480	   3890	phy_objects_before/phy-msm-usb.runtime+sleep.o
  13828	    632	     20	  14480	   3890	phy_objects_after/phy-msm-usb.runtime+sleep.o

  13072	    560	     20	  13652	   3554	phy_objects_before/phy-msm-usb.sleep.o
  13072	    560	     20	  13652	   3554	phy_objects_after/phy-msm-usb.sleep.o

[1]: Chosen because it has recently broken randconfig builds due to improper
     #ifdeffery using CONFIG_PM* defines
[2]:
	$ ${CROSS_COMPILE}gcc --version
	arm-linux-gnueabihf-gcc (crosstool-NG linaro-1.13.1-4.8-2013.07-1 - Linaro GCC 2013.07) 4.8.2 20130624 (prerelease)
	Copyright (C) 2013 Free Software Foundation, Inc.
	This is free software; see the source for copying conditions.  There is NO
	warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


Josh Cartwright (3):
  typecheck: introduce assign_if() and assign_if_enabled()
  PM: define new ASSIGN_*_PM_OPS macros based on assign_if
  usb: phy: msm: use ASSIGN_*_PM_OPS variants

 drivers/usb/phy/phy-msm-usb.c | 13 +++----------
 include/linux/pm.h            | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/typecheck.h     | 18 ++++++++++++++++++
 3 files changed, 60 insertions(+), 10 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/3] typecheck: introduce assign_if() and assign_if_enabled()

[Josh Cartwright]

The assign_if() and assign_if_enable() macros are intended to be used
in static initializers for function pointers, where the pointer is
expected to be NULL when a compile-time condition does not hold.

These macros allow for implementing this behavior, without requiring the
functions be wrapped in #ifdef conditionals, and while providing
typesafety regardless of the value of the conditional.

For example, the following pattern is common:

	#ifdef CONFIG_FOO
	static void foo_callback(void)
	{
	}
	#else
	#define foo_callback NULL
	#endif

	static struct foo_object foo_obj = {
		.callback = foo_callback,
	};

Usage of assign_if_enabled() allows for achieving the same effect
without the preprocessor conditional, and in addition, allowing the
compiler to typecheck the function regardless of CONFIG_FOO.

	static void foo_callback(void)
	{
	}

	static struct foo_object foo_obj = {
		.callback = assign_if_enabled(CONFIG_FOO, foo_callback),
	};

Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 include/linux/typecheck.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
index eb5b74a..04134c7 100644
--- a/include/linux/typecheck.h
+++ b/include/linux/typecheck.h
@@ -21,4 +21,22 @@
 	(void)__tmp; \
 })
 
+/*
+ * Intended for use in static object initializers,
+ * assign_if(const_expr, function) evaluates to 'function' if 'const_expr',
+ * otherwise NULL.
+ *
+ * The type of the assign_if() expression is typeof(function), and therefore
+ * can provide typechecking regardless of 'const_expr'.
+ *
+ * gcc considers 'function' to be used and will not generate a 'defined but not
+ * used' warning when not 'const_expr', however, gcc is smart enough to
+ * eliminate 'function' if assign_if() is the only reference.
+ */
+#define assign_if(const_expr,function)	\
+	((const_expr) ? function : NULL)
+
+#define assign_if_enabled(option,function)	\
+	assign_if(IS_ENABLED(option), function)
+
 #endif		/* TYPECHECK_H_INCLUDED */
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 2/3] PM: define new ASSIGN_*_PM_OPS macros based on assign_if

[Josh Cartwright]

Similar to the SET_*_PM_OPS(), these functions are to be used in
initializers of struct dev_pm_ops, for example:

	static const struct dev_pm_ops foo_pm_ops = {
		ASSIGN_RUNTIME_PM_OPS(foo_rpm_suspend, foo_rpm_resume, NULL)
		ASSIGN_SYSTEM_SLEEP_PM_OPS(foo_suspend, foo_resume)
	};

Unlike their SET_*_PM_OPS() counter parts, it is unnecessary to wrap the
function callbacks in #ifdeffery in order to prevent 'defined but not
used' warnings when the corresponding CONFIG_PM* options are unset.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 include/linux/pm.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index db2be5f..3810d56 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -299,6 +299,15 @@ struct dev_pm_ops {
 	int (*runtime_idle)(struct device *dev);
 };
 
+#define assign_if_pm_sleep(fn) \
+	assign_if_enabled(CONFIG_PM_SLEEP, fn)
+
+#define assign_if_pm_runtime(fn) \
+	assign_if_enabled(CONFIG_PM_RUNTIME, fn)
+
+#define assign_if_pm(fn) \
+	assign_if_enabled(CONFIG_PM, fn)
+
 #ifdef CONFIG_PM_SLEEP
 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 	.suspend = suspend_fn, \
@@ -342,6 +351,36 @@ struct dev_pm_ops {
 #endif
 
 /*
+ * The ASSIGN_* variations of the above make wrapping the associated callback
+ * functions in preprocessor defines unnecessary.
+ */
+#define ASSIGN_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
+	.suspend = assign_if_pm_sleep(suspend_fn), \
+	.resume = assign_if_pm_sleep(resume_fn), \
+	.freeze = assign_if_pm_sleep(suspend_fn), \
+	.thaw = assign_if_pm_sleep(resume_fn), \
+	.poweroff = assign_if_pm_sleep(suspend_fn), \
+	.restore = assign_if_pm_sleep(resume_fn),
+
+#define ASSIGN_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
+	.suspend_late = assign_if_pm_sleep(suspend_fn), \
+	.resume_early = assign_if_pm_sleep(resume_fn), \
+	.freeze_late = assign_if_pm_sleep(suspend_fn), \
+	.thaw_early = assign_if_pm_sleep(resume_fn), \
+	.poweroff_late = assign_if_pm_sleep(suspend_fn), \
+	.restore_early = assign_if_pm_sleep(resume_fn),
+
+#define ASSIGN_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
+	.runtime_suspend = assign_if_pm_runtime(suspend_fn), \
+	.runtime_resume = assign_if_pm_runtime(resume_fn), \
+	.runtime_idle = assign_if_pm_runtime(idle_fn),
+
+#define ASSIGN_PM_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
+	.runtime_suspend = assign_if_pm(suspend_fn), \
+	.runtime_resume = assign_if_pm(resume_fn), \
+	.runtime_idle = assign_if_pm(idle_fn),
+
+/*
  * Use this if you want to use the same suspend and resume callbacks for suspend
  * to RAM and hibernation.
  */
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Josh Cartwright]

Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
preprocessor conditionals around the specified callbacks.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 drivers/usb/phy/phy-msm-usb.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
index 5b37b81..c04f2e3 100644
--- a/drivers/usb/phy/phy-msm-usb.c
+++ b/drivers/usb/phy/phy-msm-usb.c
@@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
 #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
 #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
 
-#ifdef CONFIG_PM
-
 #define USB_PHY_SUSP_DIG_VOL  500000
 static int msm_hsusb_config_vddcx(int high)
 {
@@ -609,7 +607,6 @@ skip_phy_resume:
 
 	return 0;
 }
-#endif
 
 static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
 {
@@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_RUNTIME
 static int msm_otg_runtime_idle(struct device *dev)
 {
 	struct msm_otg *motg = dev_get_drvdata(dev);
@@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
 	dev_dbg(dev, "OTG runtime resume\n");
 	return msm_otg_resume(motg);
 }
-#endif
 
-#ifdef CONFIG_PM_SLEEP
 static int msm_otg_pm_suspend(struct device *dev)
 {
 	struct msm_otg *motg = dev_get_drvdata(dev);
@@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static const struct dev_pm_ops msm_otg_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
-	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
-				msm_otg_runtime_idle)
+	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
+	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
+			      msm_otg_runtime_idle)
 };
 
 static struct platform_driver msm_otg_driver = {
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

Hi,

On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> preprocessor conditionals around the specified callbacks.
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> index 5b37b81..c04f2e3 100644
> --- a/drivers/usb/phy/phy-msm-usb.c
> +++ b/drivers/usb/phy/phy-msm-usb.c
> @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
>  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
>  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
>  
> -#ifdef CONFIG_PM
> -
>  #define USB_PHY_SUSP_DIG_VOL  500000
>  static int msm_hsusb_config_vddcx(int high)
>  {
> @@ -609,7 +607,6 @@ skip_phy_resume:
>  
>  	return 0;
>  }
> -#endif
>  
>  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
>  {
> @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_RUNTIME
>  static int msm_otg_runtime_idle(struct device *dev)
>  {
>  	struct msm_otg *motg = dev_get_drvdata(dev);
> @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
>  	dev_dbg(dev, "OTG runtime resume\n");
>  	return msm_otg_resume(motg);
>  }
> -#endif
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int msm_otg_pm_suspend(struct device *dev)
>  {
>  	struct msm_otg *motg = dev_get_drvdata(dev);
> @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> -				msm_otg_runtime_idle)
> +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> +			      msm_otg_runtime_idle)

if the patch introducing assign_if() gets accepted, I'm ok with this
patch.

Acked-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

Hi,

On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> preprocessor conditionals around the specified callbacks.
> 
> Signed-off-by: Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> index 5b37b81..c04f2e3 100644
> --- a/drivers/usb/phy/phy-msm-usb.c
> +++ b/drivers/usb/phy/phy-msm-usb.c
> @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
>  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
>  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
>  
> -#ifdef CONFIG_PM
> -
>  #define USB_PHY_SUSP_DIG_VOL  500000
>  static int msm_hsusb_config_vddcx(int high)
>  {
> @@ -609,7 +607,6 @@ skip_phy_resume:
>  
>  	return 0;
>  }
> -#endif
>  
>  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
>  {
> @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_RUNTIME
>  static int msm_otg_runtime_idle(struct device *dev)
>  {
>  	struct msm_otg *motg = dev_get_drvdata(dev);
> @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
>  	dev_dbg(dev, "OTG runtime resume\n");
>  	return msm_otg_resume(motg);
>  }
> -#endif
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int msm_otg_pm_suspend(struct device *dev)
>  {
>  	struct msm_otg *motg = dev_get_drvdata(dev);
> @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> -				msm_otg_runtime_idle)
> +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> +			      msm_otg_runtime_idle)

if the patch introducing assign_if() gets accepted, I'm ok with this
patch.

Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] typecheck: introduce assign_if() and assign_if_enabled()

[Greg Kroah-Hartman]

On Mon, Feb 24, 2014 at 11:08:25AM -0600, Josh Cartwright wrote:
> The assign_if() and assign_if_enable() macros are intended to be used
> in static initializers for function pointers, where the pointer is
> expected to be NULL when a compile-time condition does not hold.
> 
> These macros allow for implementing this behavior, without requiring the
> functions be wrapped in #ifdef conditionals, and while providing
> typesafety regardless of the value of the conditional.
> 
> For example, the following pattern is common:
> 
> 	#ifdef CONFIG_FOO
> 	static void foo_callback(void)
> 	{
> 	}
> 	#else
> 	#define foo_callback NULL
> 	#endif
> 
> 	static struct foo_object foo_obj = {
> 		.callback = foo_callback,
> 	};
> 
> Usage of assign_if_enabled() allows for achieving the same effect
> without the preprocessor conditional, and in addition, allowing the
> compiler to typecheck the function regardless of CONFIG_FOO.
> 
> 	static void foo_callback(void)
> 	{
> 	}
> 
> 	static struct foo_object foo_obj = {
> 		.callback = assign_if_enabled(CONFIG_FOO, foo_callback),
> 	};
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>  include/linux/typecheck.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
> index eb5b74a..04134c7 100644
> --- a/include/linux/typecheck.h
> +++ b/include/linux/typecheck.h
> @@ -21,4 +21,22 @@
>  	(void)__tmp; \
>  })
>  
> +/*
> + * Intended for use in static object initializers,
> + * assign_if(const_expr, function) evaluates to 'function' if 'const_expr',
> + * otherwise NULL.
> + *
> + * The type of the assign_if() expression is typeof(function), and therefore
> + * can provide typechecking regardless of 'const_expr'.
> + *
> + * gcc considers 'function' to be used and will not generate a 'defined but not
> + * used' warning when not 'const_expr', however, gcc is smart enough to
> + * eliminate 'function' if assign_if() is the only reference.
> + */

What version of gcc started doing this?  Does llvm also do this?

thanks,

greg k-h

Re: [PATCH 2/3] PM: define new ASSIGN_*_PM_OPS macros based on assign_if

[Greg Kroah-Hartman]

On Mon, Feb 24, 2014 at 11:08:26AM -0600, Josh Cartwright wrote:
> Similar to the SET_*_PM_OPS(), these functions are to be used in
> initializers of struct dev_pm_ops, for example:
> 
> 	static const struct dev_pm_ops foo_pm_ops = {
> 		ASSIGN_RUNTIME_PM_OPS(foo_rpm_suspend, foo_rpm_resume, NULL)
> 		ASSIGN_SYSTEM_SLEEP_PM_OPS(foo_suspend, foo_resume)
> 	};
> 
> Unlike their SET_*_PM_OPS() counter parts, it is unnecessary to wrap the
> function callbacks in #ifdeffery in order to prevent 'defined but not
> used' warnings when the corresponding CONFIG_PM* options are unset.
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>  include/linux/pm.h | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index db2be5f..3810d56 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -299,6 +299,15 @@ struct dev_pm_ops {
>  	int (*runtime_idle)(struct device *dev);
>  };
>  
> +#define assign_if_pm_sleep(fn) \
> +	assign_if_enabled(CONFIG_PM_SLEEP, fn)
> +
> +#define assign_if_pm_runtime(fn) \
> +	assign_if_enabled(CONFIG_PM_RUNTIME, fn)
> +
> +#define assign_if_pm(fn) \
> +	assign_if_enabled(CONFIG_PM, fn)
> +
>  #ifdef CONFIG_PM_SLEEP
>  #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>  	.suspend = suspend_fn, \
> @@ -342,6 +351,36 @@ struct dev_pm_ops {
>  #endif
>  
>  /*
> + * The ASSIGN_* variations of the above make wrapping the associated callback
> + * functions in preprocessor defines unnecessary.
> + */
> +#define ASSIGN_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	.suspend = assign_if_pm_sleep(suspend_fn), \
> +	.resume = assign_if_pm_sleep(resume_fn), \
> +	.freeze = assign_if_pm_sleep(suspend_fn), \
> +	.thaw = assign_if_pm_sleep(resume_fn), \
> +	.poweroff = assign_if_pm_sleep(suspend_fn), \
> +	.restore = assign_if_pm_sleep(resume_fn),
> +
> +#define ASSIGN_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	.suspend_late = assign_if_pm_sleep(suspend_fn), \
> +	.resume_early = assign_if_pm_sleep(resume_fn), \
> +	.freeze_late = assign_if_pm_sleep(suspend_fn), \
> +	.thaw_early = assign_if_pm_sleep(resume_fn), \
> +	.poweroff_late = assign_if_pm_sleep(suspend_fn), \
> +	.restore_early = assign_if_pm_sleep(resume_fn),
> +
> +#define ASSIGN_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> +	.runtime_suspend = assign_if_pm_runtime(suspend_fn), \
> +	.runtime_resume = assign_if_pm_runtime(resume_fn), \
> +	.runtime_idle = assign_if_pm_runtime(idle_fn),
> +
> +#define ASSIGN_PM_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> +	.runtime_suspend = assign_if_pm(suspend_fn), \
> +	.runtime_resume = assign_if_pm(resume_fn), \
> +	.runtime_idle = assign_if_pm(idle_fn),

Ugh, what a mess, really?  Is it that hard to get the #ifdef right in
the code?  Why not just always define the functions and then also always
have them in the structures, and if the feature isn't enabled, just
don't call/use them?

Yes, it would cause a _very_ tiny increase in code size if the option is
disabled, but really, does anyone ever disable those options becides on
the dreaded 'make randconfig' checkers?

It seems that would solve the problem much easier than this macro hell.

ick.

greg k-h

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Greg Kroah-Hartman]

On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> > preprocessor conditionals around the specified callbacks.
> > 
> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > ---
> >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> > index 5b37b81..c04f2e3 100644
> > --- a/drivers/usb/phy/phy-msm-usb.c
> > +++ b/drivers/usb/phy/phy-msm-usb.c
> > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
> >  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
> >  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
> >  
> > -#ifdef CONFIG_PM
> > -
> >  #define USB_PHY_SUSP_DIG_VOL  500000
> >  static int msm_hsusb_config_vddcx(int high)
> >  {
> > @@ -609,7 +607,6 @@ skip_phy_resume:
> >  
> >  	return 0;
> >  }
> > -#endif
> >  
> >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
> >  {
> > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -#ifdef CONFIG_PM_RUNTIME
> >  static int msm_otg_runtime_idle(struct device *dev)
> >  {
> >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
> >  	dev_dbg(dev, "OTG runtime resume\n");
> >  	return msm_otg_resume(motg);
> >  }
> > -#endif
> >  
> > -#ifdef CONFIG_PM_SLEEP
> >  static int msm_otg_pm_suspend(struct device *dev)
> >  {
> >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
> >  
> >  	return 0;
> >  }
> > -#endif
> >  
> >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> > -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > -				msm_otg_runtime_idle)
> > +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > +			      msm_otg_runtime_idle)
> 
> if the patch introducing assign_if() gets accepted, I'm ok with this
> patch.

I can't take that patch at this point in time, it's just too ugly...

As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
things?

What language are we trying to program in here people?

greg k-h

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[David Cohen]

On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> > > preprocessor conditionals around the specified callbacks.
> > > 
> > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > > ---
> > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
> > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> > > index 5b37b81..c04f2e3 100644
> > > --- a/drivers/usb/phy/phy-msm-usb.c
> > > +++ b/drivers/usb/phy/phy-msm-usb.c
> > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
> > >  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
> > >  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
> > >  
> > > -#ifdef CONFIG_PM
> > > -
> > >  #define USB_PHY_SUSP_DIG_VOL  500000
> > >  static int msm_hsusb_config_vddcx(int high)
> > >  {
> > > @@ -609,7 +607,6 @@ skip_phy_resume:
> > >  
> > >  	return 0;
> > >  }
> > > -#endif
> > >  
> > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
> > >  {
> > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
> > >  	return 0;
> > >  }
> > >  
> > > -#ifdef CONFIG_PM_RUNTIME
> > >  static int msm_otg_runtime_idle(struct device *dev)
> > >  {
> > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
> > >  	dev_dbg(dev, "OTG runtime resume\n");
> > >  	return msm_otg_resume(motg);
> > >  }
> > > -#endif
> > >  
> > > -#ifdef CONFIG_PM_SLEEP
> > >  static int msm_otg_pm_suspend(struct device *dev)
> > >  {
> > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
> > >  
> > >  	return 0;
> > >  }
> > > -#endif
> > >  
> > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> > > -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > -				msm_otg_runtime_idle)
> > > +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > +			      msm_otg_runtime_idle)
> > 
> > if the patch introducing assign_if() gets accepted, I'm ok with this
> > patch.
> 
> I can't take that patch at this point in time, it's just too ugly...
> 
> As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
> things?
> 
> What language are we trying to program in here people?

Since we're discussing this topic here, I'd like point my RFC which gets
rid of same ifdeffery in a different way:
http://lkml.org/lkml/2013/12/13/4

Comments are welcome.

Br, David

> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Greg Kroah-Hartman]

On Thu, Feb 27, 2014 at 03:41:31PM -0800, David Cohen wrote:
> On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
> > On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> > > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> > > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> > > > preprocessor conditionals around the specified callbacks.
> > > > 
> > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > > > ---
> > > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
> > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> > > > index 5b37b81..c04f2e3 100644
> > > > --- a/drivers/usb/phy/phy-msm-usb.c
> > > > +++ b/drivers/usb/phy/phy-msm-usb.c
> > > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
> > > >  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
> > > >  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
> > > >  
> > > > -#ifdef CONFIG_PM
> > > > -
> > > >  #define USB_PHY_SUSP_DIG_VOL  500000
> > > >  static int msm_hsusb_config_vddcx(int high)
> > > >  {
> > > > @@ -609,7 +607,6 @@ skip_phy_resume:
> > > >  
> > > >  	return 0;
> > > >  }
> > > > -#endif
> > > >  
> > > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
> > > >  {
> > > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -#ifdef CONFIG_PM_RUNTIME
> > > >  static int msm_otg_runtime_idle(struct device *dev)
> > > >  {
> > > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
> > > >  	dev_dbg(dev, "OTG runtime resume\n");
> > > >  	return msm_otg_resume(motg);
> > > >  }
> > > > -#endif
> > > >  
> > > > -#ifdef CONFIG_PM_SLEEP
> > > >  static int msm_otg_pm_suspend(struct device *dev)
> > > >  {
> > > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
> > > >  
> > > >  	return 0;
> > > >  }
> > > > -#endif
> > > >  
> > > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> > > > -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > > -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > > -				msm_otg_runtime_idle)
> > > > +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > > +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > > +			      msm_otg_runtime_idle)
> > > 
> > > if the patch introducing assign_if() gets accepted, I'm ok with this
> > > patch.
> > 
> > I can't take that patch at this point in time, it's just too ugly...
> > 
> > As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
> > things?
> > 
> > What language are we trying to program in here people?
> 
> Since we're discussing this topic here, I'd like point my RFC which gets
> rid of same ifdeffery in a different way:
> http://lkml.org/lkml/2013/12/13/4

Again, why can't we just always define these fields in the structure,
then we don't need any crazy, complicated mess for assigning the
function pointers?

Again, the odds that this config option is ever disabled in "real"
systems is so low these days, I have half a mind just to rip it out
entirely as the amount of work spent on compiler warnings and the like
in this area has proably offset any power savings the code was supposed
to save on systems :(

ick.

greg k-h

Re: [PATCH 1/3] typecheck: introduce assign_if() and assign_if_enabled()

[Josh Cartwright]

On Thu, Feb 27, 2014 at 11:00:32AM -0800, Greg Kroah-Hartman wrote:
> On Mon, Feb 24, 2014 at 11:08:25AM -0600, Josh Cartwright wrote:
> > +/*
> > + * Intended for use in static object initializers,
> > + * assign_if(const_expr, function) evaluates to 'function' if 'const_expr',
> > + * otherwise NULL.
> > + *
> > + * The type of the assign_if() expression is typeof(function), and therefore
> > + * can provide typechecking regardless of 'const_expr'.
> > + *
> > + * gcc considers 'function' to be used and will not generate a 'defined but not
> > + * used' warning when not 'const_expr', however, gcc is smart enough to
> > + * eliminate 'function' if assign_if() is the only reference.
> > + */
> 
> What version of gcc started doing this?  Does llvm also do this?

I'll need to dig up some old gcc's to give this a more thorough
testing; testing with clang 3.4, and it appears to have the same
behavior, at least when I throw a trivial usecase at it.

	$ clang --version
	clang version 3.4 (tags/RELEASE_34/final)
	Target: x86_64-unknown-linux-gnu
	Thread model: posix

	$ cat test.c
	static void foo(void)
	{
		extern void BROKEN(void);
		BROKEN();
	}
	void (*callback)(void) = 0 ? foo : 0;

	$ clang -Wall -Werror -c test.c

	$ size test.o
	   text	   data	    bss	    dec	    hex	filename
	      0	      0	      8	      8	      8	/tmp/test.o

	$ nm test.o
	0000000000000000 B callback

Although, given the feedback on the other patches, assign_if() may
become just be a solution in search of a problem :).

Thanks,
  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[David Cohen]

On Thu, Feb 27, 2014 at 03:44:25PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Feb 27, 2014 at 03:41:31PM -0800, David Cohen wrote:
> > On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
> > > On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> > > > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> > > > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> > > > > preprocessor conditionals around the specified callbacks.
> > > > > 
> > > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > > > > ---
> > > > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
> > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> > > > > index 5b37b81..c04f2e3 100644
> > > > > --- a/drivers/usb/phy/phy-msm-usb.c
> > > > > +++ b/drivers/usb/phy/phy-msm-usb.c
> > > > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
> > > > >  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
> > > > >  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
> > > > >  
> > > > > -#ifdef CONFIG_PM
> > > > > -
> > > > >  #define USB_PHY_SUSP_DIG_VOL  500000
> > > > >  static int msm_hsusb_config_vddcx(int high)
> > > > >  {
> > > > > @@ -609,7 +607,6 @@ skip_phy_resume:
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
> > > > >  {
> > > > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -#ifdef CONFIG_PM_RUNTIME
> > > > >  static int msm_otg_runtime_idle(struct device *dev)
> > > > >  {
> > > > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
> > > > >  	dev_dbg(dev, "OTG runtime resume\n");
> > > > >  	return msm_otg_resume(motg);
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > > -#ifdef CONFIG_PM_SLEEP
> > > > >  static int msm_otg_pm_suspend(struct device *dev)
> > > > >  {
> > > > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> > > > > -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > > > -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > > > -				msm_otg_runtime_idle)
> > > > > +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > > > +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > > > +			      msm_otg_runtime_idle)
> > > > 
> > > > if the patch introducing assign_if() gets accepted, I'm ok with this
> > > > patch.
> > > 
> > > I can't take that patch at this point in time, it's just too ugly...
> > > 
> > > As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
> > > things?
> > > 
> > > What language are we trying to program in here people?
> > 
> > Since we're discussing this topic here, I'd like point my RFC which gets
> > rid of same ifdeffery in a different way:
> > http://lkml.org/lkml/2013/12/13/4
> 
> Again, why can't we just always define these fields in the structure,
> then we don't need any crazy, complicated mess for assigning the
> function pointers?
> 
> Again, the odds that this config option is ever disabled in "real"
> systems is so low these days, I have half a mind just to rip it out
> entirely as the amount of work spent on compiler warnings and the like
> in this area has proably offset any power savings the code was supposed
> to save on systems :(

That makes sense :)
Thanks for your feedback.

BR, David

> 
> ick.
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[David Cohen]

On Thu, Feb 27, 2014 at 03:44:25PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Feb 27, 2014 at 03:41:31PM -0800, David Cohen wrote:
> > On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
> > > On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> > > > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> > > > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> > > > > preprocessor conditionals around the specified callbacks.
> > > > > 
> > > > > Signed-off-by: Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > > > > ---
> > > > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
> > > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> > > > > index 5b37b81..c04f2e3 100644
> > > > > --- a/drivers/usb/phy/phy-msm-usb.c
> > > > > +++ b/drivers/usb/phy/phy-msm-usb.c
> > > > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
> > > > >  #define PHY_SUSPEND_TIMEOUT_USEC	(500 * 1000)
> > > > >  #define PHY_RESUME_TIMEOUT_USEC	(100 * 1000)
> > > > >  
> > > > > -#ifdef CONFIG_PM
> > > > > -
> > > > >  #define USB_PHY_SUSP_DIG_VOL  500000
> > > > >  static int msm_hsusb_config_vddcx(int high)
> > > > >  {
> > > > > @@ -609,7 +607,6 @@ skip_phy_resume:
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
> > > > >  {
> > > > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -#ifdef CONFIG_PM_RUNTIME
> > > > >  static int msm_otg_runtime_idle(struct device *dev)
> > > > >  {
> > > > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
> > > > >  	dev_dbg(dev, "OTG runtime resume\n");
> > > > >  	return msm_otg_resume(motg);
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > > -#ifdef CONFIG_PM_SLEEP
> > > > >  static int msm_otg_pm_suspend(struct device *dev)
> > > > >  {
> > > > >  	struct msm_otg *motg = dev_get_drvdata(dev);
> > > > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -#endif
> > > > >  
> > > > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> > > > > -	SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > > > -	SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > > > -				msm_otg_runtime_idle)
> > > > > +	ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> > > > > +	ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> > > > > +			      msm_otg_runtime_idle)
> > > > 
> > > > if the patch introducing assign_if() gets accepted, I'm ok with this
> > > > patch.
> > > 
> > > I can't take that patch at this point in time, it's just too ugly...
> > > 
> > > As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
> > > things?
> > > 
> > > What language are we trying to program in here people?
> > 
> > Since we're discussing this topic here, I'd like point my RFC which gets
> > rid of same ifdeffery in a different way:
> > http://lkml.org/lkml/2013/12/13/4
> 
> Again, why can't we just always define these fields in the structure,
> then we don't need any crazy, complicated mess for assigning the
> function pointers?
> 
> Again, the odds that this config option is ever disabled in "real"
> systems is so low these days, I have half a mind just to rip it out
> entirely as the amount of work spent on compiler warnings and the like
> in this area has proably offset any power savings the code was supposed
> to save on systems :(

That makes sense :)
Thanks for your feedback.

BR, David

> 
> ick.
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Ulf Hansson]

On 28 February 2014 00:44, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Feb 27, 2014 at 03:41:31PM -0800, David Cohen wrote:
>> On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
>> > On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
>> > > Hi,
>> > >
>> > > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
>> > > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
>> > > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
>> > > > preprocessor conditionals around the specified callbacks.
>> > > >
>> > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
>> > > > ---
>> > > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
>> > > >  1 file changed, 3 insertions(+), 10 deletions(-)
>> > > >
>> > > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
>> > > > index 5b37b81..c04f2e3 100644
>> > > > --- a/drivers/usb/phy/phy-msm-usb.c
>> > > > +++ b/drivers/usb/phy/phy-msm-usb.c
>> > > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
>> > > >  #define PHY_SUSPEND_TIMEOUT_USEC       (500 * 1000)
>> > > >  #define PHY_RESUME_TIMEOUT_USEC        (100 * 1000)
>> > > >
>> > > > -#ifdef CONFIG_PM
>> > > > -
>> > > >  #define USB_PHY_SUSP_DIG_VOL  500000
>> > > >  static int msm_hsusb_config_vddcx(int high)
>> > > >  {
>> > > > @@ -609,7 +607,6 @@ skip_phy_resume:
>> > > >
>> > > >         return 0;
>> > > >  }
>> > > > -#endif
>> > > >
>> > > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
>> > > >  {
>> > > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
>> > > >         return 0;
>> > > >  }
>> > > >
>> > > > -#ifdef CONFIG_PM_RUNTIME
>> > > >  static int msm_otg_runtime_idle(struct device *dev)
>> > > >  {
>> > > >         struct msm_otg *motg = dev_get_drvdata(dev);
>> > > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
>> > > >         dev_dbg(dev, "OTG runtime resume\n");
>> > > >         return msm_otg_resume(motg);
>> > > >  }
>> > > > -#endif
>> > > >
>> > > > -#ifdef CONFIG_PM_SLEEP
>> > > >  static int msm_otg_pm_suspend(struct device *dev)
>> > > >  {
>> > > >         struct msm_otg *motg = dev_get_drvdata(dev);
>> > > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
>> > > >
>> > > >         return 0;
>> > > >  }
>> > > > -#endif
>> > > >
>> > > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
>> > > > -       SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
>> > > > -       SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
>> > > > -                               msm_otg_runtime_idle)
>> > > > +       ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
>> > > > +       ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
>> > > > +                             msm_otg_runtime_idle)
>> > >
>> > > if the patch introducing assign_if() gets accepted, I'm ok with this
>> > > patch.
>> >
>> > I can't take that patch at this point in time, it's just too ugly...
>> >
>> > As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
>> > things?
>> >
>> > What language are we trying to program in here people?
>>
>> Since we're discussing this topic here, I'd like point my RFC which gets
>> rid of same ifdeffery in a different way:
>> http://lkml.org/lkml/2013/12/13/4
>
> Again, why can't we just always define these fields in the structure,
> then we don't need any crazy, complicated mess for assigning the
> function pointers?
>
> Again, the odds that this config option is ever disabled in "real"
> systems is so low these days, I have half a mind just to rip it out
> entirely as the amount of work spent on compiler warnings and the like
> in this area has proably offset any power savings the code was supposed
> to save on systems :(

Your point is certainly valid. I suppose the footprint of the kernel
is nothing we should bother about? We have other solutions for that,
right?

Kind regards
Ulf Hansson

>
> ick.
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Greg Kroah-Hartman]

On Fri, Feb 28, 2014 at 09:48:08AM +0100, Ulf Hansson wrote:
> On 28 February 2014 00:44, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Feb 27, 2014 at 03:41:31PM -0800, David Cohen wrote:
> >> On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
> >> > On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
> >> > > Hi,
> >> > >
> >> > > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
> >> > > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
> >> > > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
> >> > > > preprocessor conditionals around the specified callbacks.
> >> > > >
> >> > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> >> > > > ---
> >> > > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
> >> > > >  1 file changed, 3 insertions(+), 10 deletions(-)
> >> > > >
> >> > > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
> >> > > > index 5b37b81..c04f2e3 100644
> >> > > > --- a/drivers/usb/phy/phy-msm-usb.c
> >> > > > +++ b/drivers/usb/phy/phy-msm-usb.c
> >> > > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
> >> > > >  #define PHY_SUSPEND_TIMEOUT_USEC       (500 * 1000)
> >> > > >  #define PHY_RESUME_TIMEOUT_USEC        (100 * 1000)
> >> > > >
> >> > > > -#ifdef CONFIG_PM
> >> > > > -
> >> > > >  #define USB_PHY_SUSP_DIG_VOL  500000
> >> > > >  static int msm_hsusb_config_vddcx(int high)
> >> > > >  {
> >> > > > @@ -609,7 +607,6 @@ skip_phy_resume:
> >> > > >
> >> > > >         return 0;
> >> > > >  }
> >> > > > -#endif
> >> > > >
> >> > > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
> >> > > >  {
> >> > > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
> >> > > >         return 0;
> >> > > >  }
> >> > > >
> >> > > > -#ifdef CONFIG_PM_RUNTIME
> >> > > >  static int msm_otg_runtime_idle(struct device *dev)
> >> > > >  {
> >> > > >         struct msm_otg *motg = dev_get_drvdata(dev);
> >> > > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
> >> > > >         dev_dbg(dev, "OTG runtime resume\n");
> >> > > >         return msm_otg_resume(motg);
> >> > > >  }
> >> > > > -#endif
> >> > > >
> >> > > > -#ifdef CONFIG_PM_SLEEP
> >> > > >  static int msm_otg_pm_suspend(struct device *dev)
> >> > > >  {
> >> > > >         struct msm_otg *motg = dev_get_drvdata(dev);
> >> > > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
> >> > > >
> >> > > >         return 0;
> >> > > >  }
> >> > > > -#endif
> >> > > >
> >> > > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
> >> > > > -       SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> >> > > > -       SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> >> > > > -                               msm_otg_runtime_idle)
> >> > > > +       ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
> >> > > > +       ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
> >> > > > +                             msm_otg_runtime_idle)
> >> > >
> >> > > if the patch introducing assign_if() gets accepted, I'm ok with this
> >> > > patch.
> >> >
> >> > I can't take that patch at this point in time, it's just too ugly...
> >> >
> >> > As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
> >> > things?
> >> >
> >> > What language are we trying to program in here people?
> >>
> >> Since we're discussing this topic here, I'd like point my RFC which gets
> >> rid of same ifdeffery in a different way:
> >> http://lkml.org/lkml/2013/12/13/4
> >
> > Again, why can't we just always define these fields in the structure,
> > then we don't need any crazy, complicated mess for assigning the
> > function pointers?
> >
> > Again, the odds that this config option is ever disabled in "real"
> > systems is so low these days, I have half a mind just to rip it out
> > entirely as the amount of work spent on compiler warnings and the like
> > in this area has proably offset any power savings the code was supposed
> > to save on systems :(
> 
> Your point is certainly valid. I suppose the footprint of the kernel
> is nothing we should bother about? We have other solutions for that,
> right?

What does the "footprint of the kernel" have to do with an option that
everyone enables as they want/need the functionality?  You are only
talking about saving size for systems that do not exist.  Do you know of
any commen system that cares about size and yet not power that would be
affected by this change?

And exactly how much "size" are we talking about here?  Did the
available memory size for new chips just increase more in me writing
this email than the size that this proposed patch would have offset?

greg k-h

Re: [PATCH 2/3] PM: define new ASSIGN_*_PM_OPS macros based on assign_if

[Pavel Machek]

Hi!

> > +#define ASSIGN_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> > +	.suspend = assign_if_pm_sleep(suspend_fn), \
> > +	.resume = assign_if_pm_sleep(resume_fn), \
> > +	.freeze = assign_if_pm_sleep(suspend_fn), \
> > +	.thaw = assign_if_pm_sleep(resume_fn), \
> > +	.poweroff = assign_if_pm_sleep(suspend_fn), \
> > +	.restore = assign_if_pm_sleep(resume_fn),
> 
> Ugh, what a mess, really?  Is it that hard to get the #ifdef right in
> the code?  Why not just always define the functions and then also always
> have them in the structures, and if the feature isn't enabled, just
> don't call/use them?

The functions may not compile with CONFIG_PM disabled. (And #ifdefs in
the code are considered ugly).

> Yes, it would cause a _very_ tiny increase in code size if the option is
> disabled, but really, does anyone ever disable those options becides on
> the dreaded 'make randconfig' checkers?

We don't want CONFIG_PM complexity on some embedded systems... and it
is useful tostart with simple (!PM) system when introducing new board.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/3] usb: phy: msm: use ASSIGN_*_PM_OPS variants

[Ulf Hansson]

On 28 February 2014 17:52, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Feb 28, 2014 at 09:48:08AM +0100, Ulf Hansson wrote:
>> On 28 February 2014 00:44, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Feb 27, 2014 at 03:41:31PM -0800, David Cohen wrote:
>> >> On Thu, Feb 27, 2014 at 11:03:24AM -0800, Greg Kroah-Hartman wrote:
>> >> > On Tue, Feb 25, 2014 at 12:33:36PM -0600, Felipe Balbi wrote:
>> >> > > Hi,
>> >> > >
>> >> > > On Mon, Feb 24, 2014 at 11:08:27AM -0600, Josh Cartwright wrote:
>> >> > > > Use ASSIGN_SYSTEM_SLEEP_PM_OPS and ASSIGN_RUNTIME_PM_OPS in the
>> >> > > > initializer for msm_otg_dev_pm_ops.  Doing so allows us to eliminate
>> >> > > > preprocessor conditionals around the specified callbacks.
>> >> > > >
>> >> > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
>> >> > > > ---
>> >> > > >  drivers/usb/phy/phy-msm-usb.c | 13 +++----------
>> >> > > >  1 file changed, 3 insertions(+), 10 deletions(-)
>> >> > > >
>> >> > > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c
>> >> > > > index 5b37b81..c04f2e3 100644
>> >> > > > --- a/drivers/usb/phy/phy-msm-usb.c
>> >> > > > +++ b/drivers/usb/phy/phy-msm-usb.c
>> >> > > > @@ -414,8 +414,6 @@ static int msm_otg_reset(struct usb_phy *phy)
>> >> > > >  #define PHY_SUSPEND_TIMEOUT_USEC       (500 * 1000)
>> >> > > >  #define PHY_RESUME_TIMEOUT_USEC        (100 * 1000)
>> >> > > >
>> >> > > > -#ifdef CONFIG_PM
>> >> > > > -
>> >> > > >  #define USB_PHY_SUSP_DIG_VOL  500000
>> >> > > >  static int msm_hsusb_config_vddcx(int high)
>> >> > > >  {
>> >> > > > @@ -609,7 +607,6 @@ skip_phy_resume:
>> >> > > >
>> >> > > >         return 0;
>> >> > > >  }
>> >> > > > -#endif
>> >> > > >
>> >> > > >  static void msm_otg_notify_charger(struct msm_otg *motg, unsigned mA)
>> >> > > >  {
>> >> > > > @@ -1664,7 +1661,6 @@ static int msm_otg_remove(struct platform_device *pdev)
>> >> > > >         return 0;
>> >> > > >  }
>> >> > > >
>> >> > > > -#ifdef CONFIG_PM_RUNTIME
>> >> > > >  static int msm_otg_runtime_idle(struct device *dev)
>> >> > > >  {
>> >> > > >         struct msm_otg *motg = dev_get_drvdata(dev);
>> >> > > > @@ -1699,9 +1695,7 @@ static int msm_otg_runtime_resume(struct device *dev)
>> >> > > >         dev_dbg(dev, "OTG runtime resume\n");
>> >> > > >         return msm_otg_resume(motg);
>> >> > > >  }
>> >> > > > -#endif
>> >> > > >
>> >> > > > -#ifdef CONFIG_PM_SLEEP
>> >> > > >  static int msm_otg_pm_suspend(struct device *dev)
>> >> > > >  {
>> >> > > >         struct msm_otg *motg = dev_get_drvdata(dev);
>> >> > > > @@ -1731,12 +1725,11 @@ static int msm_otg_pm_resume(struct device *dev)
>> >> > > >
>> >> > > >         return 0;
>> >> > > >  }
>> >> > > > -#endif
>> >> > > >
>> >> > > >  static const struct dev_pm_ops msm_otg_dev_pm_ops = {
>> >> > > > -       SET_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
>> >> > > > -       SET_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
>> >> > > > -                               msm_otg_runtime_idle)
>> >> > > > +       ASSIGN_SYSTEM_SLEEP_PM_OPS(msm_otg_pm_suspend, msm_otg_pm_resume)
>> >> > > > +       ASSIGN_RUNTIME_PM_OPS(msm_otg_runtime_suspend, msm_otg_runtime_resume,
>> >> > > > +                             msm_otg_runtime_idle)
>> >> > >
>> >> > > if the patch introducing assign_if() gets accepted, I'm ok with this
>> >> > > patch.
>> >> >
>> >> > I can't take that patch at this point in time, it's just too ugly...
>> >> >
>> >> > As are those crazy SET_SYSTEM_SLEEP_PM_OPS() macros, ick, who made those
>> >> > things?
>> >> >
>> >> > What language are we trying to program in here people?
>> >>
>> >> Since we're discussing this topic here, I'd like point my RFC which gets
>> >> rid of same ifdeffery in a different way:
>> >> http://lkml.org/lkml/2013/12/13/4
>> >
>> > Again, why can't we just always define these fields in the structure,
>> > then we don't need any crazy, complicated mess for assigning the
>> > function pointers?
>> >
>> > Again, the odds that this config option is ever disabled in "real"
>> > systems is so low these days, I have half a mind just to rip it out
>> > entirely as the amount of work spent on compiler warnings and the like
>> > in this area has proably offset any power savings the code was supposed
>> > to save on systems :(
>>
>> Your point is certainly valid. I suppose the footprint of the kernel
>> is nothing we should bother about? We have other solutions for that,
>> right?
>
> What does the "footprint of the kernel" have to do with an option that
> everyone enables as they want/need the functionality?  You are only
> talking about saving size for systems that do not exist.  Do you know of
> any commen system that cares about size and yet not power that would be
> affected by this change?

It was more a hypothetical thought. I can't give you any examples of
products and I certainly think your suggestion makes sense.

What I had in mind were my experience from flashloaders and
productiontools. Those often requires a fast boot which don't cares
about PM. One of many things that helps here, is a "small" footprint
of the kernel. But I guess this becomes quite hypothetical and I guess
we shouldn't consider this as a valid argument!?

>
> And exactly how much "size" are we talking about here?  Did the
> available memory size for new chips just increase more in me writing
> this email than the size that this proposed patch would have offset?

I only had the fast boot in mind... :-)

Kind regards
Ulf Hansson

>
> greg k-h

Re: [PATCH 2/3] PM: define new ASSIGN_*_PM_OPS macros based on assign_if

[Greg Kroah-Hartman]

On Sat, Mar 01, 2014 at 12:06:33PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > +#define ASSIGN_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> > > +	.suspend = assign_if_pm_sleep(suspend_fn), \
> > > +	.resume = assign_if_pm_sleep(resume_fn), \
> > > +	.freeze = assign_if_pm_sleep(suspend_fn), \
> > > +	.thaw = assign_if_pm_sleep(resume_fn), \
> > > +	.poweroff = assign_if_pm_sleep(suspend_fn), \
> > > +	.restore = assign_if_pm_sleep(resume_fn),
> > 
> > Ugh, what a mess, really?  Is it that hard to get the #ifdef right in
> > the code?  Why not just always define the functions and then also always
> > have them in the structures, and if the feature isn't enabled, just
> > don't call/use them?
> 
> The functions may not compile with CONFIG_PM disabled. (And #ifdefs in
> the code are considered ugly).
> 
> > Yes, it would cause a _very_ tiny increase in code size if the option is
> > disabled, but really, does anyone ever disable those options becides on
> > the dreaded 'make randconfig' checkers?
> 
> We don't want CONFIG_PM complexity on some embedded systems...

Really, what embedded systems do not want this?

> and it is useful tostart with simple (!PM) system when introducing new
> board.

I'm not saying to disable the option, I'm saying to stop worrying about
saving a few hundred bytes in individual drivers with this crazy #ifdef
and macro mess that no one understands and always gets wrong.

greg k-h

Re: [PATCH 2/3] PM: define new ASSIGN_*_PM_OPS macros based on assign_if

[Pavel Machek]

Hi!

> > > > +#define ASSIGN_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> > > > +	.suspend = assign_if_pm_sleep(suspend_fn), \
> > > > +	.resume = assign_if_pm_sleep(resume_fn), \
> > > > +	.freeze = assign_if_pm_sleep(suspend_fn), \
> > > > +	.thaw = assign_if_pm_sleep(resume_fn), \
> > > > +	.poweroff = assign_if_pm_sleep(suspend_fn), \
> > > > +	.restore = assign_if_pm_sleep(resume_fn),
> > > 
> > > Ugh, what a mess, really?  Is it that hard to get the #ifdef right in
> > > the code?  Why not just always define the functions and then also always
> > > have them in the structures, and if the feature isn't enabled, just
> > > don't call/use them?
> > 
> > The functions may not compile with CONFIG_PM disabled. (And #ifdefs in
> > the code are considered ugly).
> > 
> > > Yes, it would cause a _very_ tiny increase in code size if the option is
> > > disabled, but really, does anyone ever disable those options becides on
> > > the dreaded 'make randconfig' checkers?
> > 
> > We don't want CONFIG_PM complexity on some embedded systems...
> 
> Really, what embedded systems do not want this?

Most of them? :-) In last four years, I cooperated on 4 of those, and
device power management was not requirement on any of those. Just one
was operated on battery that was "small".

> > and it is useful tostart with simple (!PM) system when introducing new
> > board.
> 
> I'm not saying to disable the option, I'm saying to stop worrying about
> saving a few hundred bytes in individual drivers with this crazy #ifdef
> and macro mess that no one understands and always gets wrong.

I wrote: 

> > The functions may not compile with CONFIG_PM disabled. (And #ifdefs in
> > the code are considered ugly).

To reiterate: suspend_fn (etc) is likely to use stuff not avialable in
!PM case, so it will break your compilation if you don't add some
#ifdefs.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html