From: Dan Carpenter <dan.carpenter@oracle.com> To: Song Chen <chensong_2000@189.cn> Cc: greg@kroah.com, linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org Subject: Re: [PATCH] staging: board: Remove macro board_staging Date: Mon, 4 Jan 2021 17:39:51 +0300 [thread overview] Message-ID: <20210104143951.GS2809@kadam> (raw) In-Reply-To: <1608890085-1267-1-git-send-email-chensong_2000@189.cn> 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
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com> To: Song Chen <chensong_2000@189.cn> Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] staging: board: Remove macro board_staging Date: Mon, 4 Jan 2021 17:39:51 +0300 [thread overview] Message-ID: <20210104143951.GS2809@kadam> (raw) In-Reply-To: <1608890085-1267-1-git-send-email-chensong_2000@189.cn> 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
next prev parent reply other threads:[~2021-01-04 14:40 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-25 9:54 [PATCH] staging: board: Remove macro board_staging Song Chen 2021-01-04 14:39 ` Dan Carpenter [this message] 2021-01-04 14:39 ` Dan Carpenter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210104143951.GS2809@kadam \ --to=dan.carpenter@oracle.com \ --cc=chensong_2000@189.cn \ --cc=devel@driverdev.osuosl.org \ --cc=greg@kroah.com \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.