Thread (28 messages) 28 messages, 4 authors, 2018-07-06

Re: [PATCH v7 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

From: Enric Balletbo Serra <eballetbo@gmail.com>
Date: 2018-06-26 14:24:54
Also in: linux-clk, linux-input, lkml

Hi Matti,

Missatge de Matti Vaittinen [off-list ref] del
dia dt., 26 de juny 2018 a les 14:03:
Hello Again Eric,

On Tue, Jun 26, 2018 at 01:40:40PM +0200, Enric Balletbo Serra wrote:
quoted
Hi Matti,
Missatge de Matti Vaittinen [off-list ref] del
dia dt., 26 de juny 2018 a les 13:25:
quoted
On Tue, Jun 26, 2018 at 11:06:33AM +0200, Enric Balletbo Serra wrote:
quoted
Missatge de Matti Vaittinen [off-list ref] del
dia dt., 19 de juny 2018 a les 12:57:
quoted
quoted
+static const struct of_device_id bd71837_of_match[] = {
+       { .compatible = "rohm,bd71837", .data = (void *)0},
+       { },
nit: { /* sentinel */ }
I am sorry but I didn't quite get the point here. Could you please
explain what did you expect to be added here?
It's a nit but It is a good practice to specify that the last entry is
a sentinel. Just this.

+static const struct of_device_id bd71837_of_match[] = {
+       { .compatible = "rohm,bd71837", .data = (void *)0},
+       { /* sentinel */ }
+};
Oh, I see. Finally something I can disagree =) I quickly opened few
random drivers which declare match table. None of them practiced this
good practice. So I guess it is not such a standard after all. And I
guess the meaning of last entry in match table should be quite obvious.
Adding the comment /* sentinel */ sounds like stating the obvious at
best - at worst it gets one just to wonder what the "sentinel" means =)
git grep "/\* sentinel \*/"

Anyway, I marked this change as a nit, so you don't need to change. I
just commented because at some point I received a "complain" when I
didn't add it. But this is up to the maintainer and I am not sure if
the "complain" was received in this subsystem :)

Cheers,
 Enric
quoted
And just noticed, is .data = (void *)0 really required?
As static structs should be initialized to zero I'd say it is not
required. Will remove this. Thanks for pointing this out.

Br,
        Matti Vaittinen
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help