I was *extremely* annoyed to see that somebody had put "#include g_NCR5380.
h" into NCR5380.c. The whole idea is to copy the "generic" files (g_NCR5380.
[hc]) to new files for your device, and tweak them for your device, and have
the *new* xxx.c include NCR5380.c.
Graphically:
NCR5380.c ------->v
NCR5380.h--->v v
t128.h ----->|--->t128.c ----->t128-driver.o
NCR5380.c ------->v
NCR5380.h--->v v
pas16.h ---->|--->pas16.c ---->pas-driver.o
NCR5380.c ------>v
NCR5380.h---->v v
dtc.h ------->|--->dtc.c ----->dtc-driver.o
In words: "To get the FOO driver, you take the template NCR5380.h, the
tailored FOO.h, and the tailored FOO.c file, together with the template
NCR5380.c"
Drew's whole idea was sound, and brilliant. As I see it, his goal was:
"come up with a template for a driver using the 5380 chipset, so that
writing a driver for a new controller using this chip is very easy---95% of
the work is already done." (Drew, correct me if I have this wrong.) In
fact, an "object oriented" friend said that this was an example of
inheritance.
Where this mysterious person went wrong (aside from not including update
version comments in the file), is that he had other modules include
g_NCR5380.h. NOTHING should include this---this is a *template*.
Where I think you went wrong is in your statement:
>" The point of there being no #ifdef's originally
> was to allow one driver to detect and drive multiple cards of the same
> basic type."
I don't see ANY evidence in Drew's code, or readme files, that this was the
intent. In fact, the driver template (NCR5380.c") is FULL of #ifdef's. I
think his design intent was to have one set of _source code_ that would work
for a multitude of basically similar cards, and NOT to have one _driver_ to
serve multiple basically similar cards.
In other words, the tailoring should be done at compile-time (via ifdef's)
rather than at runtime (via flags).
Here's the problem I have with this new code: My card uses a DTC406 chip,
which is similar to the 53C400 chip, but slightly different. Some of the
bits are different, and there are more registers, and the interrupts have
another entire layer over and above the 53C400 and 5380.
So *some* of the code that you have activated by "if (hostdata->flags &
FLAG_NCR53C400)" is correct for this card, but some is incorrect. And some
is missing entirely. So what do we do--add another flag? and say "if
(hostdata->flags & (FLAG_NCR53C400 | FLAG_DTC406)"??
Oh, and add another member to the struct, like
"instance->DTC406_instance_name = DTC_extra_register_set" ?? I thought
about doing just that.
It's just a *whole* lot cleaner to use #ifdef's to handle what little
tailoring is necessary. And seems to fit in nicely with the architecture of
Drew's original code.
I'm pretty close to being done with my driver--it all works, but I want to
improve the performance a bit, and add support for /proc/scsi/dtc/0. Let's
co-ordinate our enhancements so that we can get all these drivers working
with ONE set of source-code. I have a scsi-pas16 card, and a DTC card, but
not a t128 or 53c400.
Regards,
Ray Van Tassle
rayvt@comm.mot.com
____________________________________________________________________________
___
> To: michael@jarvis.com@INTERNET Cc:
> linux-kernel@vger.rutgers.edu@INTERNET From:
> K.Lentin@cs.monash.edu.au@INTERNET on Thu, Feb 15, 1996 8:04 AM Subject:
> Re: Problems in 1.3.62 with PAS16
>
> X-Mailer: ELM [version 2.4 PL24 ME8b] Mime-Version: 1.0 Content-Type:
> text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence:
> bulk
>
> Michael Jarvis Wrote ... > > I tried compiling a fresh copy of 1.3.62,
> and ran into problems with > trying to use PAS16 for CD-ROM support
> only. The errors I would get > were:
>
> Somebody added something to the pas16 code in 1.3.6? that is wrong. The
> pas16.c file MUST NOT include g_NCR5380.h. It's completely non-sensical.
> The correct fix for the problem that appeared in about 1.3.59 or 60 is
> to ifdef around the 2 places in NCR5380.c where it gave errors.
>
> ie:
>
> --- linux/drivers/scsi/NCR5380.c.orig Fri Feb 9 17:11:41 1996 +++
> linux/drivers/scsi/NCR5380.c Fri Feb 9 17:11:48 1996 @@ -786,8 +786,10
> @@
> * the base address.
> */
> +#ifdef NCR53C400
> if (flags & FLAG_NCR53C400)
> instance->NCR5380_instance_name += NCR53C400_address_adjust; +#endif
>
> NCR5380_setup(instance);
> @@ -837,9 +839,11 @@
> NCR5380_write(TARGET_COMMAND_REG, 0);
> NCR5380_write(SELECT_ENABLE_REG, 0);
> +#ifdef NCR53C400
> if (hostdata->flags & FLAG_NCR53C400) {
> NCR5380_write(C400_CONTROL_STATUS_REG, CSR_BASE);
> } +#endif
>
> /*
> * Detect and correct bus wedge problems.
>
>
> Including one low level driver include file into another driver causes
> huge redefinition problems as you saw. And the results are completely
> wrong.
>
> Copying in old NCR5380.* files will fix the problem if all you use is
> the pas16 but the above is the correct solution. I mailed this to Linus
> about 2 patchlevels ago.
>
> I will put those 2 bits back in soon. They won't cause _any_ problems
> not being there since the only driver to use them is the g_NCR5380 and
> it defines NCR53C400. The point of there being no #ifdef's originally
> was to allow one driver to detect and drive multiple cards of the same
> basic type.
>
> --
> [=======================================================================]
> [ Kevin Lentin |finger kevinl@fangorn.cs.monash.edu.au|
> ] [ K.Lentin@cs.monash.edu.au |for PGP public key block. Fingerprint
> | ] [ Macintrash: 'Just say NO!' |6024308DE1F84314 811B511DBA6FD596
> | ]
> [=======================================================================]
>