At Wed, 30 Jul 2008 11:45:48 +0100,
Pawel MOLL wrote:
>
> > Could you split this as a separate patch? This unsigned int -> int
> > change is irrelevant with the fix for 8 PCM devices limit.
>
> Well, actually it is :-) Take a look here:
>
> static inline int snd_pcm_next(struct snd_card *card, int device)
> {
> struct snd_pcm *pcm;
>
> list_for_each_entry(pcm, &snd_pcm_devices, list) {
> if (pcm->card == card && pcm->device > device)
> ^^^^^^^^^^^^^^^^^^^^
> return pcm->device;
> else if (pcm->card->number > card->number)
> return -1;
> }
> return -1;
> }
Ah yeah, this new part contains it.
>
> When device = -1 (first step on enumeration) the expression:
>
> (unsigned)pcm->device > (unsigned)device
>
> will be always 0 as (unsigned)-1 == 0xffffffff...
>
> I wouldn't touch this at all in other case ;-)
>
> > You cannot use SNDRV_OS_MINORS unconditionally here.
> > In the case DYNAMIC_MINORS=n, SNDRV_PCM_DEVICES is still valid. Maybe
> > better to define SNDRV_PCM_DEVICES with ifdef, such as,
> >
> > #ifdef CONFIG_SND_DYNAMIC_MINORS
> > #define SNDRV_PCM_DEVICES (SNDRV_OS_MINORS-2)
> > #else
> > #define SNDRV_PCM_DEVICES 8
> > #endif
>
> To be perfectly honest I don't see the point, as in both cases the value
> may be simply wrong (I mean device not existing at all), so as for me
> these checks are just "safety fuses" and above is yet another #ifdef,
> but no problem - I will revive this constant.
Yes, it's for safety reason. And, no reason to break old behavior.
In slightest case, it can be a regression :)
> > > -static struct snd_pcm *snd_pcm_search(struct snd_card *card, int
> > device)
> > > +static inline struct snd_pcm *snd_pcm_get(struct snd_card *card,
> > int device)
> >
> > Don't use inline. It's good for a single-line or a pretty small
> > function, but not for else. The compiler is clever enough to inline
> > the functions automatically if needed.
>
> Well, I used to define functions which are called from just one place
> (which are usually just an extracted part of other huge function ;-) as
> inline, but as you wish - I wont persist on it.
Especially in such a case, you really don't have to make it inline.
> > Also, any reason to rename to *_get()? get() is usually paired with
> > put().
>
> I see your point. My motivation was: this function is not used for
> searching the list any more, just for getting a reference to one,
> defined element. Of course I can leave it as it was.
OK, point taken. No big problem to rename, but I just wondered.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
