Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer <elf@buici.com>]

On Fri, May 14, 2004 at 11:23:19PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday 14 of May 2004 21:47, Marc Singer wrote:
> 
> > > [ you used 'struct ide_hwif_s' in arch-lh7a40x/ide.h to workaround this
> > > 8) ]
> 
> struct hwif_s actually
> 
> > Copied from elsewhere.
> 
> superio.h, superio.c

I don't think I ever looked at that code.

> EEK, added to TODO
> 
> > Listen.  It is not my intention to be clever.  All I want to do is get
> > things working and to not break other people's code.  I'm certain that
> > most people are working with the same assumptions.  Aside from some
> > naive snippets I've see promulgated, the bulk of the kernel work I've
> > see is sane given limited information.  Certainly, once one
> > understands a sybsystem completely then the quality rises.  I hope
> > you'll admit that the IDE code is overly intricate.
> 
> With changes like this nobody will ever be able to understand
> IDE subsystem completely. ;-)

I get the feeling that you're blaming me and others for making the IDE
code a mess.  Might I suggest that this isn't a very productive tack?
The most helpful thing to do is to a) provide best-practice examples,
and b) to include some documentation.  I'm not talking about anything
extensive, but statements like

  "All references to linux/ide.h must reside in the ide tree."

Are pretty darn helpful. 

> > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
> > >   in many places in generic IDE code - anybody wanting to understand
> > >   interactions with your code + generic code will have serious
> > >   problems (especially if knows _nothing_ about lpd7a40x)
> >
> > I don't know what you mean.  I grep for that constant and found it
> > nowhere except for ide-io.c and in my code.  It doesn't take much to
> > find the references.
> 
> I'm talking about ide_init_hwif_ports() function.

Most of the ARM arch's use it.  Perhaps all of them need a good once
over.

> > What is it that you want changed?
> >
> > > - hwif->mmio is set to 2 but resource handling is missed
> >
> > Can you be more specific?  Did you notice the comment?  I didn't know
> > what it was for, but I set it because I thought that that was the
> > right way to go.
> >
> > Are you talking about reserving the address space?  At the moment,
> 
> Yes.

Easily done.

> > this is done in the arch setup.  I can certainly move it to the IDE
> > driver.
> 
> OK
> 
> > > > The OUTB breaks my interface because I don't really have byte-level
> > > > access to the resgisters.  So, is selectproc a pre-select procedure or
> > > > should it be a substitute?
> > >
> > > pre-select but you can change it to be substitute if you need
> > > (just remember to update all users if you decide to do this)
> >
> > I'll have to search the kernel to see what uses it.  Maybe the better
> > way would be to define a new select proc that *is* a substitute.
> 
> Nope.

So then we break anyone who is using the selectproc as a pre-select
proc?  I don't understand what you mean here.  There are several
drivers that use this function.  How do you propose that we provide
both types of behavior with one entry point?

> > of the core code is fine.  It is that core code that is necessary to
> > make the test possible.
> 
> Stuff in arch-lh7a40x/ide.h is really a driver code but
> abuses subsystem code instead - that's my complain.

Right.  We agree.  I am talking about the core code.  Not the ide
code.  The core is what supports the CPU.