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

Jesper Nilsson
2007-12-12T17:05:07Z

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