[alsa-devel] [PATCH 4/4] ASoC: clean up cache accesser

[Takashi Iwai]

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.


thanks,

Takashi