From: Henk de Groot <henk.de.groot@hetnet.nl>
To: kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS
Date: Thu, 17 Jun 2010 21:07:50 +0000 [thread overview]
Message-ID: <4C1A8EA6.2080304@hetnet.nl> (raw)
In-Reply-To: <1276751864.14632.6.camel@lenovo>
Hello Javier,
Op 17-6-2010 19:46, Javier Martinez Canillas schreef:
> 2) #ifdefs are ugly
>
> Code cluttered with ifdefs is difficult to read and maintain. Don't do
> it. Instead, put your ifdefs in a header, and conditionally define
> 'static inline'
>
That's very true. The wlags49_h2 driver is full of them and it would
improve readability the code to clean it up.
> So I discard this option. Do you want me to do it anyway and send a
> new patch? I'm willing to solve it the right way.
>
Please do not listen to me. Your patch is fine. I'm not a regular kernel
hacker so I don't know the rules either. Its more likely you know more
about it than me already.
Of course I have idea's about the code and how to write it but I believe
it is more important to have a unified standard across the kernel. I'm
not seasoned enough to comment what's right or wrong with respect to the
established coding standard. For example my style is to always put {} in
the body of an "if" but I learned that its against the kernel coding
standard if the body is a single statement. And that has to take have
preference over my own ideas to get uniform code.
I know this driver does not keep up with the standards (yet?),
especially the HCF library part (the files that do not start with a wl_
prefix). It might even be generated by some design tool because Agere
did some strange things with structures and its very hard to read an
follow as it is.
Anyway, please keep the patch as is, unless somebody with much more
kernel knowledge comes along and tells us how it was supposed to be
done. And I think they already said its fine now.
Kind regards,
Henk.
prev parent reply other threads:[~2010-06-17 21:07 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-06-17 5:17 [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is Javier Martinez Canillas
2010-06-17 7:48 ` [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS Dan Carpenter
2010-06-17 11:48 ` [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is Javier Martinez Canillas
2010-06-17 17:23 ` [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS Henk de Groot
2010-06-17 17:46 ` [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS is Javier Martinez Canillas
2010-06-17 19:21 ` [PATCH] staging/wlags49_h2: Fix build error when CONFIG_SYSFS Dan Carpenter
2010-06-17 21:07 ` Henk de Groot [this message]
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=4C1A8EA6.2080304@hetnet.nl \
--to=henk.de.groot@hetnet.nl \
--cc=kernel-janitors@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox