Re: mga_vid directions (was: insmod error)

On Tue, 08 Mar 2005 23:45:22 +0100
"Ferdinand O. Tempel" <[email protected]> wrote:


> I've had a look at your version of the driver, and it looks clean and
> compact to me. I can't test the actual driver right now as I don't have
> my matrox card handy (I'm at a vacation address). To have this included
> in the kernel (though I honestly doubt the kernel devs will accept such
> an ugly hack :-) I think a few things need to be done:
> 
> - Documentation/CodingStyle

That's easy.

> - SysFS support needs to be put back in

AFAIK that's done over the kobjects, didn't had the time
to read all about that.

> - udev support

No clue how this works. I only found docu for users, no
docu how to program for it.

> - Makefile

That's easy with the new Makefile from my .11 version,
just delete all the stuff in the else clause.

> - Kernel configuration

Very easy.


Missing here are:

* fix all race conditions (i know that there is at least one)
* add spinlocks around all critical sections (mostly needed
  for smp machines)
* add more sanity checks

> All of which aren't too hard to do. The first item is just cosmetic, but
> to get anything included into the kernel it's very important. I'm not a
> great C coder, but isn't a lot of the stuff which now is in mga_vid.c
> supposed to reside in the mga_vid.h file? I mean the definitions,
> macros, and all that...

I don't think so. Most of these defines are used only
inside mga_vid.c and do not to be known outside.
mga_vid.h is mainly a definition of the API for user space
programs.

> In other words, a good cosmetic cleanup of the
> code and comments (comment blocks should be enclosed by /* ... */, for
> instance) would be welcome.

> The second item was already there in a crude form, but you seem to have
> removed it (even though it worked fine). I noticed most (if not all) the
> #ifdef's and such have disappeared, which is a good thing. That's
> probably why sysfs also got axed. 

Hmm.. i must have overlooked that one. Otherwise it would have
stayed there.

> Though not a great priority (nor do I
> think it's actually a requirement for drivers), sysfs can be used to
> provide information about the hardware. udev has about the same status,
> I don't think it's a requirement for drivers (yet), but still it would
> be nice to have it included.

SysFS has to be done for inclusion as currently some configurations
are done over the read/write interface of the driver, which isnt
exactly nice.

> The last two items are more "kernel integration" work. I.e. stuff which
> will make the driver be part of the kernel.
> 
> I'd be willing to pick some of this stuff as they're likely small
> changes and not too specific to the hardware (which I really know
> nothing about...). Thoughts?

Go on! I'll gladly accept all patches that are clean and do what
they are supposed to do. For an idea how a good patch should look
like please read http://www.mplayerhq.hu/DOCS/tech/patches.txt.
In contradiction to that file, i'll accept cosmetic changes,
but if and only if they are separated from functional changes.


> Yeah, it's quite annoying to have to follow the whims of the kernel
> developers. And it doesn't get easier either, now they want to go to 4
> digit kernel versioning. I wonder what they've been smoking lately. Oh
> well...the choice is to keep seperate versions around, or again start
> using #if's to cater to small changes. 

Nope, i'm not going to add #if's for older kernel versions.
That just makes the code unmaintainable after some time.

> Not a very appealing solution. I
> don't have any ideas on how to deal with such things. Maybe keeping it
> in your SVN and then pushing the latest snapshot to mplayer CVS would be
> acceptable?

Not really. Currently CVS is kept there for the 2.[024] users.
Ok, that code could be tarred and put somewhere as i consider it
"stable" (though it isn't). Alternatively i can put up an svn/darcs
server somewhere public and move mga_vid development there.
But i really don't know what would be best.

				Attila Kinali

-- 
郷に入れば郷に従え