Re: [PATCH] for pre6: hisax user config MAX_CARD and fix potential data trashing

[Stephan von Krawczynski]

> On Sat, 8 Dec 2001, Stephan von Krawczynski wrote:                  
>                                                                     
> > attached is the second patch for ISDN-Driver HiSax for            
kernel-inclusion that does the                                        
> > following:                                                        
>                                                                     
> Please send patches to the maintainers (i.e. Karsten/me) first, not 
> directly to Linus/Marcelo. Also, CC'ing lkml isn't really necessary,
                                                                      
> either. (Documentation/SubmittingPatches is worth reading)          
                                                                      
Well, in fact my hope was, somebody from lkml would give it a quick   
look, because it again turned out the original author may not see     
minor problems :-)                                                    
                                                                      
> This makes the procedure take a bit longer, but things like missing 
> semicolons get caught in the process ;-)                            
                                                                      
Yes, I declare myself guilty. It's a nice example how a real simlpe   
patch woes.                                                           
                                                                      
> So let's drop Linus/Marcelo/lkml from CC. I'll submit the patch     
> eventually. (If it's already applied, that's fine, too)             
>                                                                     
> Generally the patch looks good (still wondering if you've got that  
many                                                                  
> cards in one machine).                                              
                                                                      
Lots. :-)                                                             
Last event which made me think about this patch to submit (and not    
just hacking in every time I needed it), were 12. But to be honest, I 
made it work for less than 8 because I do think that 8 is a bit       
oversized for most people.                                            
                                                                      
> Some comments:                                                      
>                                                                     
> (original patch)                                                    
>                                                                     
> > +/* string magic */                                               
> > +#define h1(s) h2(s)                                              
> > +#define h2(s) #s                                                 
> > +#define PARM_PARA "1-"h1(HISAX_MAX_CARDS)"i"                     
> > +                                                                 
> you should use __MODULE_STRING (from linux/module.h)                
                                                                      
I love to in the future, now that I _know_ it. :-) Please convert it  
on your next submission to Marcelo & L. We don't wanna keep           
superfluous code.                                                     
                                                                      
> > --- linux/drivers/isdn/hisax/hisax.h-orig	Sat Dec  8 00:24:20 2001
> > +++ linux/drivers/isdn/hisax/hisax.h	Sat Dec  8 00:40:49 2001     
> > @@ -950,7 +950,9 @@                                               
> >  #define  MON0_TX	4                                               
> >  #define  MON1_TX	8                                               
> >                                                                   
> > +#ifndef HISAX_MAX_CARDS                                          
> >  #define	 HISAX_MAX_CARDS	8                                       
> > +#endif                                                           
> >                                                                   
> >  #define  ISDN_CTYPE_16_0	1                                       
> >  #define  ISDN_CTYPE_8_0		2                                       
>                                                                     
> Why is that necessary?                                              
                                                                      
Hm, good question. The thing is: I wanted to make 1000% sure it       
doesn't get lost. This should have been already in the first patch,   
where it was not really clear how to get the CONFIG definition.       
Additionally I feel uncomfortable with code being not complete in     
terms of symbols. You cannot grep through *.c/*.h to find             
missing/mis-spelled symbols then. I always lived with the idea that   
compile-time definitions _tune_ the code, but it should be complete   
even without. In fact it doesn't make a difference in the created     
binary.                                                               
                                                                      
> > @@ -1563,6 +1564,8 @@                                             
> >  		if (protocol[i]) {                                             
> >  			cards[i].protocol = protocol[i];                              
> >  		}                                                              
> > +		else                                                           
> > +			cards[i].protocol = DEFAULT_PROTO;                            
> >  	}                                                               
> >  	cards[0].para[0] = pcm_irq;                                     
> >  	cards[0].para[1] = (int) pcm_iob;                               
>                                                                     
> nitpicking:                                                         
>                                                                     
> This should be                                                      
> 		} else                                                            
> 			cards[i]...                                                      
>                                                                     
> Actually, I'd prefer                                                
>                                                                     
> 		} else {                                                          
> 			cards[i]...                                                      
> 		}                                                                 
                                                                      
Feel free. I in fact thought about the other way round, but that's    
only a personal viewing and reading issue.                            
                                                                      
> I'll fix up these small bits, commit it to our CVS and take care of 
> submitting the patch.                                               
>                                                                     
> Thanks again for your work.                                         
                                                                      
You're welcome.                                                       
                                                                      
While reading config.c I came to the conclusion that this is pretty   
"organic" code :-) .                                                  
As in most ISDN drivers I read so far there is a whole lot of         
duplication inside. I had the strong urge to clean it up, but didn't  
want to submit a whole new file as a patch dealing with something     
completely different.                                                 
Perhaps I will in the future.                                         
                                                                      
Regards,                                                              
Stephan