All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] staging: board: Remove macro board_staging
@ 2020-12-25  9:54 Song Chen
  2021-01-04 14:39   ` Dan Carpenter
  0 siblings, 1 reply; 3+ messages in thread
From: Song Chen @ 2020-12-25  9:54 UTC (permalink / raw)
  To: greg, linux-kernel; +Cc: devel, Song Chen

Macro is not supposed to have flow control in it's
statement, remove.

Signed-off-by: Song Chen <chensong_2000@189.cn>
---
 drivers/staging/board/armadillo800eva.c | 10 ++++++----
 drivers/staging/board/board.h           | 11 -----------
 drivers/staging/board/kzm9d.c           | 18 ++++++++++--------
 3 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/board/armadillo800eva.c b/drivers/staging/board/armadillo800eva.c
index 0225234..a7e8487 100644
--- a/drivers/staging/board/armadillo800eva.c
+++ b/drivers/staging/board/armadillo800eva.c
@@ -80,9 +80,11 @@ static const struct board_staging_dev armadillo800eva_devices[] __initconst = {
 
 static void __init armadillo800eva_init(void)
 {
-	board_staging_gic_setup_xlate("arm,pl390", 32);
-	board_staging_register_devices(armadillo800eva_devices,
-				       ARRAY_SIZE(armadillo800eva_devices));
+	if (of_machine_is_compatible("renesas,armadillo800eva")) {
+		board_staging_gic_setup_xlate("arm,pl390", 32);
+		board_staging_register_devices(armadillo800eva_devices,
+					       ARRAY_SIZE(armadillo800eva_devices));
+	}
 }
 
-board_staging("renesas,armadillo800eva", armadillo800eva_init);
+device_initcall(armadillo800eva_init);
diff --git a/drivers/staging/board/board.h b/drivers/staging/board/board.h
index 5609daf..f1c233e 100644
--- a/drivers/staging/board/board.h
+++ b/drivers/staging/board/board.h
@@ -32,15 +32,4 @@ int board_staging_register_device(const struct board_staging_dev *dev);
 void board_staging_register_devices(const struct board_staging_dev *devs,
 				    unsigned int ndevs);
 
-#define board_staging(str, fn)			\
-static int __init runtime_board_check(void)	\
-{						\
-	if (of_machine_is_compatible(str))	\
-		fn();				\
-						\
-	return 0;				\
-}						\
-						\
-device_initcall(runtime_board_check)
-
 #endif /* __BOARD_H__ */
diff --git a/drivers/staging/board/kzm9d.c b/drivers/staging/board/kzm9d.c
index d449a83..72b1ad45 100644
--- a/drivers/staging/board/kzm9d.c
+++ b/drivers/staging/board/kzm9d.c
@@ -12,15 +12,17 @@ static struct resource usbs1_res[] __initdata = {
 
 static void __init kzm9d_init(void)
 {
-	board_staging_gic_setup_xlate("arm,pl390", 32);
+	if (of_machine_is_compatible("renesas,kzm9d")) {
+		board_staging_gic_setup_xlate("arm,pl390", 32);
 
-	if (!board_staging_dt_node_available(usbs1_res,
-					     ARRAY_SIZE(usbs1_res))) {
-		board_staging_gic_fixup_resources(usbs1_res,
-						  ARRAY_SIZE(usbs1_res));
-		platform_device_register_simple("emxx_udc", -1, usbs1_res,
-						ARRAY_SIZE(usbs1_res));
+		if (!board_staging_dt_node_available(usbs1_res,
+						     ARRAY_SIZE(usbs1_res))) {
+			board_staging_gic_fixup_resources(usbs1_res,
+							  ARRAY_SIZE(usbs1_res));
+			platform_device_register_simple("emxx_udc", -1, usbs1_res,
+							ARRAY_SIZE(usbs1_res));
+		}
 	}
 }
 
-board_staging("renesas,kzm9d", kzm9d_init);
+device_initcall(kzm9d_init);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging: board: Remove macro board_staging
  2020-12-25  9:54 [PATCH] staging: board: Remove macro board_staging Song Chen
@ 2021-01-04 14:39   ` Dan Carpenter
  0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-01-04 14:39 UTC (permalink / raw)
  To: Song Chen; +Cc: greg, linux-kernel, devel

On Fri, Dec 25, 2020 at 05:54:45PM +0800, Song Chen wrote:
> Macro is not supposed to have flow control in it's
> statement, remove.
> 

It took me a long time to figure out what this commit message is saying.
It turns out that it is inspired by checkpatch.  Forget all that nonsense
about "imperative tense" commit messages.  The only thing which matters
for drivers/staging/ is that the commit message is clear what the
problem is and how you are going to fix it.

  Checkpatch complains that macros should not have return statements in
  them.  "WARNING: Macros with flow control statements should be avoided"
  So I am removing the board_staging() macro and open coding it.

But in this case the checkpatch warning is a false positive.  The issue
that checkpatch is trying to fix is that we don't want return, break, or
goto statements in a macro.  Otherwise the code looks like this:

	frob();
	frob();
	frob();

It has three invisible return statements and we don't know what error
codes it is returning.

In this case, the board_staging() driver is implementing whole functions
so there are no hidden gotos.

That said, the board_staging() macro looks pretty rubbish.  It breaks
cscope.  It's not readable.  So I quite like the patch, it just needs a
new commit message.  It can be simple:

    It's cleaner and less code to delete the board_staging().

> Signed-off-by: Song Chen <chensong_2000@189.cn>
> ---
>  drivers/staging/board/armadillo800eva.c | 10 ++++++----
>  drivers/staging/board/board.h           | 11 -----------
>  drivers/staging/board/kzm9d.c           | 18 ++++++++++--------
>  3 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/board/armadillo800eva.c b/drivers/staging/board/armadillo800eva.c
> index 0225234..a7e8487 100644
> --- a/drivers/staging/board/armadillo800eva.c
> +++ b/drivers/staging/board/armadillo800eva.c
> @@ -80,9 +80,11 @@ static const struct board_staging_dev armadillo800eva_devices[] __initconst = {
>  
>  static void __init armadillo800eva_init(void)

I think device_initcall() functions need to return int.  I am surprised
this even compiles.

>  {
> -	board_staging_gic_setup_xlate("arm,pl390", 32);
> -	board_staging_register_devices(armadillo800eva_devices,
> -				       ARRAY_SIZE(armadillo800eva_devices));
> +	if (of_machine_is_compatible("renesas,armadillo800eva")) {

Reverse this if statement.

	if (!of_machine_is_compatible("renesas,armadillo800eva"))
		return 0;


> +		board_staging_gic_setup_xlate("arm,pl390", 32);
> +		board_staging_register_devices(armadillo800eva_devices,
> +					       ARRAY_SIZE(armadillo800eva_devices));
> +	}
>  }
>  
> -board_staging("renesas,armadillo800eva", armadillo800eva_init);
> +device_initcall(armadillo800eva_init);
> diff --git a/drivers/staging/board/board.h b/drivers/staging/board/board.h
> index 5609daf..f1c233e 100644
> --- a/drivers/staging/board/board.h
> +++ b/drivers/staging/board/board.h
> @@ -32,15 +32,4 @@ int board_staging_register_device(const struct board_staging_dev *dev);
>  void board_staging_register_devices(const struct board_staging_dev *devs,
>  				    unsigned int ndevs);
>  
> -#define board_staging(str, fn)			\
> -static int __init runtime_board_check(void)	\
> -{						\
> -	if (of_machine_is_compatible(str))	\
> -		fn();				\
> -						\
> -	return 0;				\
> -}						\
> -						\
> -device_initcall(runtime_board_check)
> -
>  #endif /* __BOARD_H__ */
> diff --git a/drivers/staging/board/kzm9d.c b/drivers/staging/board/kzm9d.c
> index d449a83..72b1ad45 100644
> --- a/drivers/staging/board/kzm9d.c
> +++ b/drivers/staging/board/kzm9d.c
> @@ -12,15 +12,17 @@ static struct resource usbs1_res[] __initdata = {
>  
>  static void __init kzm9d_init(void)

Same.  return int.

>  {
> -	board_staging_gic_setup_xlate("arm,pl390", 32);
> +	if (of_machine_is_compatible("renesas,kzm9d")) {

Same reverse the if statement.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging: board: Remove macro board_staging
@ 2021-01-04 14:39   ` Dan Carpenter
  0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-01-04 14:39 UTC (permalink / raw)
  To: Song Chen; +Cc: devel, linux-kernel

On Fri, Dec 25, 2020 at 05:54:45PM +0800, Song Chen wrote:
> Macro is not supposed to have flow control in it's
> statement, remove.
> 

It took me a long time to figure out what this commit message is saying.
It turns out that it is inspired by checkpatch.  Forget all that nonsense
about "imperative tense" commit messages.  The only thing which matters
for drivers/staging/ is that the commit message is clear what the
problem is and how you are going to fix it.

  Checkpatch complains that macros should not have return statements in
  them.  "WARNING: Macros with flow control statements should be avoided"
  So I am removing the board_staging() macro and open coding it.

But in this case the checkpatch warning is a false positive.  The issue
that checkpatch is trying to fix is that we don't want return, break, or
goto statements in a macro.  Otherwise the code looks like this:

	frob();
	frob();
	frob();

It has three invisible return statements and we don't know what error
codes it is returning.

In this case, the board_staging() driver is implementing whole functions
so there are no hidden gotos.

That said, the board_staging() macro looks pretty rubbish.  It breaks
cscope.  It's not readable.  So I quite like the patch, it just needs a
new commit message.  It can be simple:

    It's cleaner and less code to delete the board_staging().

> Signed-off-by: Song Chen <chensong_2000@189.cn>
> ---
>  drivers/staging/board/armadillo800eva.c | 10 ++++++----
>  drivers/staging/board/board.h           | 11 -----------
>  drivers/staging/board/kzm9d.c           | 18 ++++++++++--------
>  3 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/board/armadillo800eva.c b/drivers/staging/board/armadillo800eva.c
> index 0225234..a7e8487 100644
> --- a/drivers/staging/board/armadillo800eva.c
> +++ b/drivers/staging/board/armadillo800eva.c
> @@ -80,9 +80,11 @@ static const struct board_staging_dev armadillo800eva_devices[] __initconst = {
>  
>  static void __init armadillo800eva_init(void)

I think device_initcall() functions need to return int.  I am surprised
this even compiles.

>  {
> -	board_staging_gic_setup_xlate("arm,pl390", 32);
> -	board_staging_register_devices(armadillo800eva_devices,
> -				       ARRAY_SIZE(armadillo800eva_devices));
> +	if (of_machine_is_compatible("renesas,armadillo800eva")) {

Reverse this if statement.

	if (!of_machine_is_compatible("renesas,armadillo800eva"))
		return 0;


> +		board_staging_gic_setup_xlate("arm,pl390", 32);
> +		board_staging_register_devices(armadillo800eva_devices,
> +					       ARRAY_SIZE(armadillo800eva_devices));
> +	}
>  }
>  
> -board_staging("renesas,armadillo800eva", armadillo800eva_init);
> +device_initcall(armadillo800eva_init);
> diff --git a/drivers/staging/board/board.h b/drivers/staging/board/board.h
> index 5609daf..f1c233e 100644
> --- a/drivers/staging/board/board.h
> +++ b/drivers/staging/board/board.h
> @@ -32,15 +32,4 @@ int board_staging_register_device(const struct board_staging_dev *dev);
>  void board_staging_register_devices(const struct board_staging_dev *devs,
>  				    unsigned int ndevs);
>  
> -#define board_staging(str, fn)			\
> -static int __init runtime_board_check(void)	\
> -{						\
> -	if (of_machine_is_compatible(str))	\
> -		fn();				\
> -						\
> -	return 0;				\
> -}						\
> -						\
> -device_initcall(runtime_board_check)
> -
>  #endif /* __BOARD_H__ */
> diff --git a/drivers/staging/board/kzm9d.c b/drivers/staging/board/kzm9d.c
> index d449a83..72b1ad45 100644
> --- a/drivers/staging/board/kzm9d.c
> +++ b/drivers/staging/board/kzm9d.c
> @@ -12,15 +12,17 @@ static struct resource usbs1_res[] __initdata = {
>  
>  static void __init kzm9d_init(void)

Same.  return int.

>  {
> -	board_staging_gic_setup_xlate("arm,pl390", 32);
> +	if (of_machine_is_compatible("renesas,kzm9d")) {

Same reverse the if statement.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-01-04 14:40 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-25  9:54 [PATCH] staging: board: Remove macro board_staging Song Chen
2021-01-04 14:39 ` Dan Carpenter
2021-01-04 14:39   ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.