Re: [PATCH 44/47] Minor fixes for CRISv32 io.h

[Jesper Nilsson <jesper.nilsson@axis.com>]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/