From: Jesper Nilsson <jesper.nilsson@axis.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Mikael Starvik <mikael.starvik@axis.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 44/47] Minor fixes for CRISv32 io.h
Date: Wed, 12 Dec 2007 18:04:40 +0100 [thread overview]
Message-ID: <20071212170440.GF3689@axis.com> (raw)
In-Reply-To: <20071212032939.380d15dc.akpm@linux-foundation.org>
On Wed, Dec 12, 2007 at 03:29:39AM -0800, Andrew Morton wrote:
> On Mon, 3 Dec 2007 11:16:25 +0100 Jesper Nilsson <jesper.nilsson@axis.com> wrote:
> > struct crisv32_ioport
> > {
> > - unsigned long* oe;
> > - unsigned long* data;
> > - unsigned long* data_in;
> > + volatile unsigned long *oe;
> > + volatile unsigned long *data;
> > + volatile unsigned long *data_in;
> > unsigned int pin_count;
> > + spinlock_t lock;
> > };
>
> tabs.
Will fix.
> > static inline void crisv32_io_set(struct crisv32_iopin* iopin,
> > int val)
> > {
> > + long flags;
> > + spin_lock_irqsave(&iopin->port->lock, flags);
> > +
> > if (val)
> > *iopin->port->data |= iopin->bit;
> > else
> > *iopin->port->data &= ~iopin->bit;
> > +
> > + spin_unlock_irqrestore(&iopin->port->lock, flags);
> > }
> >
> > static inline void crisv32_io_set_dir(struct crisv32_iopin* iopin,
> > enum crisv32_io_dir dir)
> > {
> > + long flags;
> > + spin_lock_irqsave(&iopin->port->lock, flags);
> > +
> > if (dir == crisv32_io_dir_in)
> > *iopin->port->oe &= ~iopin->bit;
> > else
> > *iopin->port->oe |= iopin->bit;
> > +
> > + spin_unlock_irqrestore(&iopin->port->lock, flags);
> > }
>
> These surely are far too large to be inlined.
Actually not, since the CRISv32 is UP, these should be ok to inline,
and since we most often call them with a constant argument,
the compiler is free to optimize away the if statement...
> > +#define LED_NETWORK_GRP0_SET(x) \
> > + do { \
> > + LED_NETWORK_GRP0_SET_G((x) & LED_GREEN); \
> > + LED_NETWORK_GRP0_SET_R((x) & LED_RED); \
> > } while (0)
> > +#else
> > +#define LED_NETWORK_GRP0_SET(x) while (0) {}
> > +#endif
> > +
> > +#define LED_NETWORK_GRP0_SET_G(x) \
> > + crisv32_io_set(&crisv32_led_net0_green, !(x));
> > +
> > +#define LED_NETWORK_GRP0_SET_R(x) \
> > + crisv32_io_set(&crisv32_led_net0_red, !(x));
> > +
> > +#if defined(CONFIG_ETRAX_NBR_LED_GRP_TWO)
> > +#define LED_NETWORK_GRP1_SET(x) \
> > + do { \
> > + LED_NETWORK_GRP1_SET_G((x) & LED_GREEN); \
> > + LED_NETWORK_GRP1_SET_R((x) & LED_RED); \
> > + } while (0)
> > +#else
> > +#define LED_NETWORK_GRP1_SET(x) while (0) {}
> > +#endif
> > +
> > +#define LED_NETWORK_GRP1_SET_G(x) \
> > + crisv32_io_set(&crisv32_led_net1_green, !(x));
> > +
> > +#define LED_NETWORK_GRP1_SET_R(x) \
> > + crisv32_io_set(&crisv32_led_net1_red, !(x));
> > +
> > #define LED_ACTIVE_SET(x) \
> > do { \
> > LED_ACTIVE_SET_G((x) & LED_GREEN); \
> > LED_ACTIVE_SET_R((x) & LED_RED); \
> > } while (0)
> >
>
> PLease generally prefer (lower-case) inlined functions over macros. They
> are cleaner, clearer, safer and have better typechecking.
Yes, you are right, I'll have to check where this is used, but hopefully
I can pare this down to a few inlines instead.
> Several of the above macros reference their argument more than once and
> hence cannot be used as, for example,
>
> LED_ACTIVE_SET(foo++);
Also true, although the argument should never be such a statement,
it is still bad form.
Thanks again for commenting, it is enormously helpful!
/^JN - Jesper Nilsson
--
Jesper Nilsson -- jesper.nilsson@axis.com
next prev parent reply other threads:[~2007-12-12 17:05 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-11-29 16:02 [PATCH 00/47] CRIS patches for CRISv32 EtraxFS and ARTPEC-3 Jesper Nilsson
2007-11-29 16:02 ` [PATCH 01/47] Rearrange Kconfigs for CRIS v10 and v32 to allow compilation without warnings Jesper Nilsson
2007-11-29 16:03 ` [PATCH 02/47] Add new driver files for Artpec-3 Jesper Nilsson
2007-12-12 11:09 ` Andrew Morton
2007-11-29 16:05 ` [PATCH 03/47] Add new driver files for Etrax-FS Jesper Nilsson
2007-11-29 16:11 ` [PATCH 04/47] Add new machine dependent files for Etrax-FS and Artpec-3 Jesper Nilsson
2007-11-29 16:23 ` [PATCH 06/47] Add serial driver for CRISv32 Jesper Nilsson
2007-12-12 11:14 ` Andrew Morton
2007-11-29 16:24 ` [PATCH 07/47] Add L2 cache initialization code Jesper Nilsson
2007-11-29 16:26 ` [PATCH 08/47] Add SECOND_WORD_SYNC, used in CRISv32 version of sync_serial Jesper Nilsson
2007-11-29 16:30 ` [PATCH 09/47] Update CRISv32 synchronous serial driver Jesper Nilsson
2007-11-29 16:58 ` [PATCH 10/47] Merge axisflashmap.h with Axis internal changes Jesper Nilsson
2007-11-29 17:11 ` [PATCH 11/47] Minor fixes to CRISv32 irq defines Jesper Nilsson
2007-11-29 17:19 ` [PATCH 12/47] Remove unnecessary CVS log from cris/mm/init.c Jesper Nilsson
2007-11-30 9:11 ` [PATCH 13/47] Add prototypes for cache flushing on CRISv32 Jesper Nilsson
2007-11-30 9:12 ` [PATCH 14/47] Add headers for CRISv32 chips EtraxFS and Artpec-3 Jesper Nilsson
2007-11-30 12:59 ` [PATCH 15/47] Minor fixes to mm/fault.c for CRIS Jesper Nilsson
2007-12-12 11:17 ` Andrew Morton
2007-12-12 13:36 ` Jesper Nilsson
2007-11-30 13:11 ` [PATCH 16/47] Minor CRIS generic kernel/traps.c changes Jesper Nilsson
2007-11-30 13:14 ` [PATCH 17/47] Add handling of PTRACE_DETATCH for CRISv32. Whitespace and formatting changes to avoid checkpatch errors Jesper Nilsson
2007-11-30 14:40 ` [PATCH 18/47] Remove define ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY from CRIS Jesper Nilsson
2007-11-30 14:44 ` [PATCH 19/47] Update CRISv32 entry.S to working order Jesper Nilsson
2007-11-30 14:47 ` [PATCH 20/47] Fixup CRISv32 kernel Makefile Jesper Nilsson
2007-11-30 14:54 ` [PATCH 21/47] New version of CRISv32 I2C driver Jesper Nilsson
2007-12-12 11:19 ` Andrew Morton
2007-11-30 15:01 ` [PATCH 22/47] Update and improve axisflashmap for CRISv32 Jesper Nilsson
2007-11-30 15:07 ` [PATCH 23/47] Update CRIS main Kbuild makefile Jesper Nilsson
2007-11-30 15:08 ` [PATCH 24/47] Update CRISv10 boot " Jesper Nilsson
2007-11-30 15:10 ` [PATCH 25/47] Update CRISv10 boot compressed " Jesper Nilsson
2007-11-30 15:11 ` [PATCH 26/47] Update CRISv10 rescue " Jesper Nilsson
2007-11-30 15:13 ` [PATCH 27/47] Update CRISv10 rescue head.s Jesper Nilsson
2007-12-06 13:58 ` David Miller
2007-12-07 0:46 ` Jesper Nilsson
2007-11-30 15:17 ` [PATCH 28/47] Update and improve CRISv10 axisflashmap.c Jesper Nilsson
2007-11-30 15:22 ` [PATCH 29/47] Update CRISv32 traps.c Jesper Nilsson
2007-12-12 11:21 ` Andrew Morton
2007-11-30 15:24 ` [PATCH 30/47] Update CRISv32 boot Kbuild makefile Jesper Nilsson
2007-11-30 15:25 ` [PATCH 31/47] Update CRISv32 boot compressed " Jesper Nilsson
2007-11-30 15:28 ` [PATCH 32/47] Update CRISv32 boot rescue " Jesper Nilsson
2007-11-30 15:30 ` [PATCH 33/47] Remove CRISv32 common gpio and nandflash, add mach-fs and mach-a3 as subdirs Jesper Nilsson
2007-11-30 15:40 ` [PATCH 34/47] Update CRISv32 compressed head.S Jesper Nilsson
2007-11-30 16:16 ` [PATCH 35/47] Update CRISv32 boot/compressed/misc.c Jesper Nilsson
2007-11-30 16:20 ` [PATCH 36/47] Update CRISv32 boot/rescue/head.S code Jesper Nilsson
2007-11-30 16:26 ` [PATCH 37/47] Update CRISv32 debugport Jesper Nilsson
2007-11-30 16:28 ` [PATCH 38/47] Include path fix for CRISv32 timex.h Jesper Nilsson
2007-11-30 16:46 ` [PATCH 39/47] Update and improve CRISv32 fasttimer.c Jesper Nilsson
2007-12-12 11:23 ` Andrew Morton
2007-11-30 16:54 ` [PATCH 40/47] Update CRISv32 kernel/head.S Jesper Nilsson
2007-11-30 17:09 ` [PATCH 41/47] Update and simplify CRISv32 kernel/irq.c Jesper Nilsson
2007-12-03 9:54 ` [PATCH 42/47] Minor updates to CRISv32 kernel/process.c Jesper Nilsson
2007-12-12 11:24 ` Andrew Morton
2007-12-03 10:12 ` [PATCH 43/47] Update and improve CRISv32 kernel/traps.c Jesper Nilsson
2007-12-03 10:16 ` [PATCH 44/47] Minor fixes for CRISv32 io.h Jesper Nilsson
2007-12-12 11:29 ` Andrew Morton
2007-12-12 17:04 ` Jesper Nilsson [this message]
2007-12-03 10:37 ` [PATCH 45/47] New default config for CRISv10 Jesper Nilsson
2007-12-04 16:25 ` [PATCH 46/47] Update and improve CRISv32 kernel/time.c Jesper Nilsson
2007-12-12 11:33 ` Andrew Morton
2007-12-04 16:40 ` [PATCH 47/47] Add new defconfigs for Artpec-3 and EtraxFS, both CRISv32 Jesper Nilsson
2007-12-12 10:52 ` [PATCH 00/47] CRIS patches for CRISv32 EtraxFS and ARTPEC-3 Andrew Morton
2007-12-12 13:28 ` Jesper Nilsson
2007-12-12 13:28 ` Adrian Bunk
2007-12-12 13:46 ` Jesper Nilsson
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=20071212170440.GF3689@axis.com \
--to=jesper.nilsson@axis.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mikael.starvik@axis.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).