Not necessarily. If you have a card that is just a NCR5380 with nothing
else fancy on it, why not use the g_NCR5380 driver?
I then enhanced the g_NCR5380 driver to do NCR53C400 cards as well. I chose
to do that in preference to creating a new driver based on advice from
Drew. We ended up with a design that would allow that one g_NCR5380 driver
to drive multiple cards at the same time, some NCR5380, some NCR53C400
(which is VERY similar).
> 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*.
I disagree. The person did go wrong by including g_NCR5380.h but not
because it's a template. IT's because it belongs to a different driver.
It's like including "ahaxxxx.h" into NCR53x70.c IT makes no sense.
There is nothing wrong with using the g_NCR5380 driver to drive a generic
NCR5380 card.
>
> 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).
No. Firstly, NCR5380.c isn't really a template. IT's more a core. Secondly,
it was Drew who added in the BOARD_NCR5380 and BOARD_NCR53C400 flags. I
just used them. It was Drew's specific intent to allow the g_NCR5380 driver
to support both of those cards at the same time in the same machine using
the same code. I have email to that effect. It makes sense not to include
the same function in the kernel multiple times to do the identical thing.
> 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.
If your card is so dissimilar to the g_NCR5380 driver (loko at the
NCR53C406a, it might be closer) then you should write a new driver which
includes the NCR5380 core. It sounds like that is the case here since you
have a whole new interupt layer.
I admit that those 2 bits (which are no #ifdef'ed out in 1.3.63) are a
problem. That's why I commented them out.
The NCR5380.c file is a combined NCR5380/NCR53C400 core. If, through a tiny
bit of code we can add a third chip to that list then that is good.
My suggested fix would be to replace the FLAG_NCR53C400 with a differnet
flag: FLAG_NCR5380_NEED_TO_DO_THIS and then your driver initalisation sets
or resets that as appropriate.
It seems a shame to have to duplicate the entire piece of code for the sake
of one line being a problem. It just opens up huge problems with the code
diverging.
> 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.
With pleasure.
I really like the idea of the one NCR5380.c file supporting all the other
drivers. I also like the idea of the g_NCR5380 driver handling two very
similar cards. I was going to write a t130b.c driver but when I realised
that what I would have to do is replace most of the functions in NCR5380.c
just to add one line of code, Drew & I selected to put it in NCR5380.c
instead. It makes sense.
The idea was that the functions in NCR5380.c are enough to do their job and
you end up adding DMA or other such stuff. That works for g_NCR5380, t128
and pas16. Except for one or 2 lines of code. Seemed a shame to split one
away.
This is being sent to the list just so they know what's going on. Let's
continue privately.
-- [=======================================================================] [ 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 | ] [=======================================================================]