At Mon, 2 Nov 2009 22:13:06 +0100,
Krzysztof Helt wrote:
>
> On Mon, 02 Nov 2009 11:20:26 +0100
> Takashi Iwai <tiwai@xxxxxxx> wrote:
>
> > At Sun, 1 Nov 2009 16:50:38 +0100,
> > Krzysztof Helt wrote:
> > >
> > > From: Krzysztof Helt <krzysztof.h1@xxxxx>
> > >
> > > The cs4236 was two step detection with call to the snd_wss_free()
> > > between two steps. The snd_wss_free() did not free a sound device
> > > created in the snd_wss_create(). This caused an OOPS during module
> > > removal as the same sound device was released twice. The same OOPS
> > > happened if the cs4236 module loading failed.
> > >
> > > Fix this by adapting the snd_cs4236_create() to correctly work with
> > > chips less capable then cs4236. The snd_cs4236_create() behaves the
> > > same as the snd_wss_create() if the chip is less capable than the cs4236.
> > >
> > > Signed-off-by: Krzysztof Helt <krzysztof.h1@xxxxx>
> > > ---
> > > This is the second version of the patch with added snd_device_free() call
> > > in error path.
> >
> > Thanks. The change looks good, but...
> >
> > > @@ -281,83 +285,89 @@ int snd_cs4236_create(struct snd_card *card,
> > ...
> > > err = snd_wss_create(card, port, cport,
> > > irq, dma1, dma2, hardware, hwshare, &chip);
> > > if (err < 0)
> > > return err;
> > >
> > > - if (!(chip->hardware & WSS_HW_CS4236B_MASK)) {
> > > - snd_printk(KERN_ERR "CS4236+: MODE3 and extended registers "
> > > - "not available, hardware=0x%x\n", chip->hardware);
> > > - snd_device_free(card, chip);
> > > - return -ENODEV;
> >
> > I'd just return 0 here instead of error (with *rchip = chip, of course).
> > Then the code flow gets simpler and the patch becomes smaller.
> >
>
> These lines now inside if (CS4236B) clause. If one removes this condition,
> the CS4236B cannot be fully initialized and the CS4236 controls do not work
> (some of them require control port access).
Hm? I mean to have code like
if (!(chip->hardware & WSS_HW_CS4236B_MASK)) {
snd_printd("chip is not CS4236+, hardware=0x%x\n",
chip->hardware);
*rchip = chip;
return 0;
}
Then the rest can be kept as was.
#if 0
{
int idx;
for (idx = 0; idx < 8; idx++)
snd_printk(KERN_DEBUG "CD%i = 0x%x\n",
idx, inb(chip->cport + idx));
for (idx = 0; idx < 9; idx++)
snd_printk(KERN_DEBUG "C%i = 0x%x\n",
idx, snd_cs4236_ctrl_in(chip, idx));
}
#endif
ver1 = snd_cs4236_ctrl_in(chip, 1);
ver2 = snd_cs4236_ext_in(chip, CS4236_VERSION);
snd_printdd("CS4236: [0x%lx] C1 (version) = 0x%x, ext = 0x%x\n", cport,
....
So you don't need to reindent the whole lines.
Or do I miss something around it?
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
