[alsa-devel] [PATCH] cs4236: detect chip in one pass

[Takashi Iwai]

At Mon, 2 Nov 2009 22:13:06 +0100,
Krzysztof Helt wrote:
> 
> On Mon, 02 Nov 2009 11:20:26 +0100
> Takashi Iwai <tiwai at suse.de> wrote:
> 
> > At Sun, 1 Nov 2009 16:50:38 +0100,
> > Krzysztof Helt wrote:
> > > 
> > > From: Krzysztof Helt <krzysztof.h1 at wp.pl>
> > > 
> > > 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 at wp.pl>
> > > ---
> > > 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