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

Stephan von Krawczynski (skraw@ithnet.com)
Sat, 08 Dec 2001 16:12:37 +0100


> 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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/