On Wed, Dec 12, 2007 at 03:29:39AM -0800, Andrew Morton wrote:
> On Mon, 3 Dec 2007 11:16:25 +0100 Jesper Nilsson <[email protected]> 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 -- [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
