Re: [PATCH 4/4] ASoC: clean up cache accesser

[Dimitris Papastamos <dp@opensource.wolfsonmicro.com>]

On Mon, 2010-12-20 at 17:47 +0100, Takashi Iwai wrote:
> At Mon, 20 Dec 2010 16:27:49 +0000,
> Dimitris Papastamos wrote:
> > 
> > On Mon, 2010-12-20 at 17:05 +0100, Takashi Iwai wrote:
> > > +static inline bool set_cache_val(void *base, unsigned int idx,
> > > +				unsigned int val, unsigned int word_size)
> > > +{
> > > +	if (word_size == 1) {
> > > +		u8 *cache = base;
> > > +		if (cache[idx] == val)
> > > +			return true;
> > > +		cache[idx] = val;
> > > +	} else {
> > > +		u16 *cache = base;
> > > +		if (cache[idx] == val)
> > > +			return true;
> > > +		cache[idx] = val;
> > > +	}
> > > +	return false;
> > > +}
> > 
> > If word_size is anything other than 1 byte, the above else will try to
> > handle it and assume it is 16 bits.  I'd expect for an explicit check
> > for word_size == 2.  A switch statement would perhaps be preferred for
> > legibility.  It'd perhaps be wise to simply die via BUG() or similar if
> > an unsupported word size was passed in.
> 
> Yes, this would be safer.  I didn't put it since I wasn't sure whether
> BUG() content is also expanded at each caller.  If yes, it would
> bloat.   (Alternatively we may use snd_BUG_ON() -- it's built in only
> when CONFIG_SND_DEBUG is set.)
> 
> Anyway, such a check should have been done rather at initialization.

I'd expect macro expansion to happen before inline function expansion.

I'll send in a patch to perform this check at init time once yours has
been merged.

Thanks,
Dimitrios