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

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 3 Dec 2007 11:16:25 +0100 Jesper Nilsson <jesper.nilsson@axis.com> wrote:

> - Shorten include paths for machine dependent header files.
> - Add volatile to hardeware register pointers.
> - Add spinlocks around critical region.
> - Expand macros for handling of leds.
> 
> ...
>
>  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.

>  struct crisv32_iopin
> @@ -34,22 +36,37 @@ extern struct crisv32_iopin crisv32_led2_red;
>  extern struct crisv32_iopin crisv32_led3_green;
>  extern struct crisv32_iopin crisv32_led3_red;
>  
> +extern struct crisv32_iopin crisv32_led_net0_green;
> +extern struct crisv32_iopin crisv32_led_net0_red;
> +extern struct crisv32_iopin crisv32_led_net1_green;
> +extern struct crisv32_iopin crisv32_led_net1_red;
> +
>  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.

> +#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.

Several of the above macros reference their argument more than once and
hence cannot be used as, for example,

	LED_ACTIVE_SET(foo++);