From: Andrew Morton <akpm@linux-foundation.org>
To: Marc St-Jean <Marc_St-Jean@pmc-sierra.com>
Cc: stjeanma@pmc-sierra.com, linux-kernel@vger.kernel.org,
linux-mips@linux-mips.org
Subject: Re: [PATCH] drivers: PMC MSP71xx LED driver
Date: Fri, 9 Mar 2007 22:13:48 -0800 [thread overview]
Message-ID: <20070309221348.c5ab7acc.akpm@linux-foundation.org> (raw)
In-Reply-To: <45F1DDC8.1010503@pmc-sierra.com>
> On Fri, 9 Mar 2007 14:20:56 -0800 Marc St-Jean <Marc_St-Jean@pmc-sierra.com> wrote:
>
>
> > > +typedef enum {
> > > + MSP_LED_INPUT = 0,
> > > + MSP_LED_OUTPUT,
> > > +} msp_led_direction_t;
> >
> > No typedefs, please. Convert this to
> >
> > enum msp_led_direction {
> > ...
> > };
>
> Alright I'll change it but it wasn't mentioned in the review of
> the previous drivers and they've been resubmitted with some.
> A quick search shows several drivers typedef'ing enums with and
> without *_t suffixes.
Poeple do bad things, and it sometimes gets missed.
> Is there a new style rule or are only core kernel types allowed to
> use _t?
The general rule is: no typedefs.
typedefs make sense when the underlying type can change: u64, size_t,
resource_size_t, etc. We also use them for really hairy definitions like
filler_t. Nothing else. Please don't use them just to save typing.
>
> > > +/* Output modes */
> > > +typedef enum {
> > > + MSP_LED_OFF = 0, /* Off steady */
> > > + MSP_LED_ON, /* On steady */
> > > + MSP_LED_BLINK, /* On for initialPeriod, off
> > for finalPeriod */
> > > + MSP_LED_BLINK_INVERT, /* Off for initialPeriod, on for
> > finalPeriod */
> > > +} msp_led_mode_t;
> >
> > Ditto.
> >
> > > +/* For non-LED pins, these macros set HI and LO accordingly */
> > > +#define msp_led_pin_hi msp_led_turn_off
> > > +#define msp_led_pin_lo msp_led_turn_on
> >
> > eww.
> >
> > static inline wrapper functions are preferred. Write code in C, not cpp
> > where possible.
>
> I agree the wrappers are cleaner but don't understand how this relates
> to C++.
cpp == C preprocessor.
It would be better to do
/*
* nice comment goes here
*/
static inline void msp_led_pin_hi(...)
{
msp_led_turn_off(...);
}
next prev parent reply other threads:[~2007-03-10 6:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-03-09 22:20 [PATCH] drivers: PMC MSP71xx LED driver Marc St-Jean
2007-03-10 6:13 ` Andrew Morton [this message]
-- strict thread matches above, loose matches on Subject: below --
2007-03-13 17:42 Marc St-Jean
2007-03-12 18:50 Marc St-Jean
2007-03-13 8:33 ` Florian Fainelli
2007-02-26 23:48 Marc St-Jean
2007-03-05 10:17 ` Andrew Morton
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=20070309221348.c5ab7acc.akpm@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=Marc_St-Jean@pmc-sierra.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@linux-mips.org \
--cc=stjeanma@pmc-sierra.com \
/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;
as well as URLs for NNTP newsgroup(s).