[alsa-devel] [PATCH, for-next] ALSA: usb-audio: use FMTBITs in parse_audio_format_i

[Eldad Zack]

On Mon, 22 Apr 2013, Takashi Iwai wrote:

> At Mon, 22 Apr 2013 13:29:41 +0200 (CEST),
> Eldad Zack wrote:
> > 
> > 
> > 
> > On Mon, 22 Apr 2013, Takashi Iwai wrote:
> > 
> > > At Mon, 22 Apr 2013 01:44:04 +0200,
> > > Eldad Zack wrote:
> > > > 
> > > > Replace the usage of SNDRV_PCM_FORMAT_* macros with the equivalent
> > > > SNDRV_PCM_FMTBIT_* macros, and the result be can assigned directly
> > > > to the formats field.
> > > 
> > > Note that SNDRV_PCM_FMTBIT_* are 64bit integer.  And it makes sense to
> > > use FMTBIT only when multiple formats are supposed.  For a single
> > > format, keeping SNDRV_PCM_FORMAT_* is more reasonable.
> > > 
> > > In other words, a proper fix would be to replace the type of variable
> > > format with snd_pcm_format_t, and cast at converting to format bits
> > > like
> > > 	1ULL << (unsigned int)pcm_format
> > 
> > I see your point. I just figured making the assignment directly would
> > make sense just for this case.
> > 
> > There are a couple of other places with the same conversion that 
> > "break" the strong typing - how about something like this:
> > 
> > 	#define pcm_format_to_bits(fmt) (1uLL << ((__force int)(fmt)))
> > 
> > Or an equivalent function?
> 
> Yep, I just wanted to propose that :)

Cool :)

I think a function would be better so the format can be checked to use
the correct type as well, and put it in pcm.h:

static inline u64 pcm_format_to_bits(snd_pcm_format_t pcm_format)
{
	return 1ULL << (__force int) pcm_format;
}

If that looks good I'll search for all the relevant places and change 
them (in one patch).

Cheers,
Eldad

> 
> 
> Takashi
> 
> > 
> > Cheers,
> > Eldad
> > 
> > > > Modify the variable name to reflect the change.
> > > > 
> > > > The following sparse messages are silenced:
> > > > 
> > > > sound/usb/format.c:377:44: warning: incorrect type in assignment (different base types)
> > > > sound/usb/format.c:377:44:    expected int [signed] pcm_format
> > > > sound/usb/format.c:377:44:    got restricted snd_pcm_format_t [usertype] <noident>
> > > > sound/usb/format.c:379:44: warning: incorrect type in assignment (different base types)
> > > > sound/usb/format.c:379:44:    expected int [signed] pcm_format
> > > > sound/usb/format.c:379:44:    got restricted snd_pcm_format_t [usertype] <noident>
> > > > sound/usb/format.c:382:36: warning: incorrect type in assignment (different base types)
> > > > sound/usb/format.c:382:36:    expected int [signed] pcm_format
> > > > sound/usb/format.c:382:36:    got restricted snd_pcm_format_t [usertype] <noident>
> > > > 
> > > > Signed-off-by: Eldad Zack <eldad at fogrefinery.com>
> > > > ---
> > > >  sound/usb/format.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/sound/usb/format.c b/sound/usb/format.c
> > > > index 020ede0..e025e28 100644
> > > > --- a/sound/usb/format.c
> > > > +++ b/sound/usb/format.c
> > > > @@ -365,7 +365,7 @@ static int parse_audio_format_i(struct snd_usb_audio *chip,
> > > >  {
> > > >  	struct usb_interface_descriptor *altsd = get_iface_desc(iface);
> > > >  	int protocol = altsd->bInterfaceProtocol;
> > > > -	int pcm_format, ret;
> > > > +	int pcm_formats, ret;
> > > >  
> > > >  	if (fmt->bFormatType == UAC_FORMAT_TYPE_III) {
> > > >  		/* FIXME: the format type is really IECxxx
> > > > @@ -377,14 +377,14 @@ static int parse_audio_format_i(struct snd_usb_audio *chip,
> > > >  		case USB_ID(0x0763, 0x2003): /* M-Audio Audiophile USB */
> > > >  			if (chip->setup == 0x00 && 
> > > >  			    fp->altsetting == 6)
> > > > -				pcm_format = SNDRV_PCM_FORMAT_S16_BE;
> > > > +				pcm_formats = SNDRV_PCM_FMTBIT_S16_BE;
> > > >  			else
> > > > -				pcm_format = SNDRV_PCM_FORMAT_S16_LE;
> > > > +				pcm_formats = SNDRV_PCM_FMTBIT_S16_LE;
> > > >  			break;
> > > >  		default:
> > > > -			pcm_format = SNDRV_PCM_FORMAT_S16_LE;
> > > > +			pcm_formats = SNDRV_PCM_FMTBIT_S16_LE;
> > > >  		}
> > > > -		fp->formats = 1uLL << pcm_format;
> > > > +		fp->formats = pcm_formats;
> > > >  	} else {
> > > >  		fp->formats = parse_audio_format_i_type(chip, fp, format,
> > > >  							fmt, protocol);
> > > > -- 
> > > > 1.8.1.5
> > > > 
> > > 
> > 
>