Home » Mailing list archives » linux-kernel-mentors » 2010 » April » 22

[PATCH] Staging: add opencbm driver

Previous thread: Re: New to Kernel programming and device drivers by archieval briones on Wednesday, April 21, 2010 - 5:29 pm. (1 message)

Next thread: Re: [PATCH] Staging: add opencbm driver by Archieval Briones on Thursday, April 22, 2010 - 9:43 pm. (2 messages)
[view original message]
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Bri=C3=A8re?=
Subject: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 3:37 pm

This driver allows for communicating with and controlling devices
using the Commodore (CBM) IEC serial bus, via the parallel port.

The source was copied directly from the OpenCBM 0.4.x CVS branch.

Signed-off-by: Frédéric Brière <fbriere@fbriere.net>
---
This is technically not an official request for inclusion, as upstream
is still mulling over this.  Also, it's much easier for me to commit
patches outside of the kernel tree.  Nevertheless, if inclusion makes
reviewing easier for you, go right ahead and I will manage.

 drivers/staging/Kconfig              |    2 +
 drivers/staging/Makefile             |    1 +
 drivers/staging/opencbm/Kconfig      |   13 +
 drivers/staging/opencbm/Makefile     |    3 +
 drivers/staging/opencbm/TODO         |   11 +
 drivers/staging/opencbm/cbm_module.c | 1312 ++++++++++++++++++++++++++++++++++
 drivers/staging/opencbm/cbm_module.h |   51 ++
 7 files changed, 1393 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/opencbm/Kconfig
 create mode 100644 drivers/staging/opencbm/Makefile
 create mode 100644 drivers/staging/opencbm/TODO
 create mode 100644 drivers/staging/opencbm/cbm_module.c
 create mode 100644 drivers/staging/opencbm/cbm_module.h

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 5589616..a3993b9 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -141,5 +141,7 @@ source "drivers/staging/dt3155/Kconfig"
 
 source "drivers/staging/crystalhd/Kconfig"
 
+source "drivers/staging/opencbm/Kconfig"
+
 endif # !STAGING_EXCLUDE_BUILD
 endif # STAGING
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index ec45d4b..7fdaba4 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -52,3 +52,4 @@ obj-$(CONFIG_PCMCIA_NETWAVE)	+= netwave/
 obj-$(CONFIG_FB_SM7XX)		+= sm7xx/
 obj-$(CONFIG_DT3155)		+= dt3155/
 obj-$(CONFIG_CRYSTALHD)		+= crystalhd/
+obj-$(CONFIG_OPENCBM)		+= opencbm/
diff --git a/drivers/staging/opencbm/Kconfig ...
[ message continues ]
[view original message]
From: Randy Dunlap
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 4:19 pm

This mailing list isn't really the correct one for driver reviews for
getting into staging/ or anywhere else in the kernel.  It's just for advice.  :)

IOW, we can review and make comments, but the driver still needs to be posted




                           IO port address


Use (instead):
module_init(cbm_init)


There probably are several in-kernel debug techniques that could be used

style:

style:


style:  has extra spaces; use:
	for (i = 0; i < 8; i++)

but better to have a #define for 8.


	while (

"while" is not a function, so put a space after it.





		for (

			if (


size_t cnt should be printed with %zu format.








	style:		if (condition)



That would be a lot more readable as:

#ifdef KERNEL_VERSION
<full function header>
#else
<other function header>

style:	if (pp == NULL) {

or:	if (!pp) {



style:		if (condition)





---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Randy Dunlap
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 4:35 pm

Looks like that was just a mail viewing glitch on my side.  Sorry about that.



---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Frédéric Brière
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 4:53 pm

I'd seen other patches posted here, so I was kinda hoping I wouldn't

My main concern at the moment is figuring out whether or not this driver
can be accepted in the mainline, given its nasty nature.  I provided a
patch at Greg's request, but I wasn't looking for style criticism so
much as a general sense of what hope there is for its inclusion.  If
none, then I won't waste much time bothering with CodingStyle.

I'm sorry if I didn't make this clear.  If you think that I should
enquire on lkml, what would you recommend; should I send a patch with a
better explanation, or the bare module source, or just a URL?


That being said, let me thank you for your prompt response -- man was
that fast!  Thanks!


-- 
Think of it!  With VLSI we can pack 100 ENIACs in 1 sq. cm.!

_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Greg KH
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 5:09 pm

I don't object to it, I'll be glad to take it and queue it up.  Sound
reasonable?

thanks,

greg k-h
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Frédéric Brière
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 6:16 pm

Alright, let's go for it.  The worst that can happen is that you'll
revert it two days later.  :)

From now one, shall I send patches to lkml, with a CC to you?


-- 
The Macintosh is Xerox technology at its best.

_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Luis R. Rodriguez
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 6:22 pm

There is also a staging drivers list (devel@driverdev.osuosl.org, ), use that.

  Luis
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Randy Dunlap
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 5:21 pm

Not just an URL.  Basically the same as you sent here.

Hopefully others will chime in on some sense about mainline inclusion.

I would ask (if anyone knows) how many users this driver has (or could have).

Given that it's for very old hardware and requires a custom cable, I don't
see much need to merge it, but other people want to merge *all* drivers into
the kernel source tree.  ;)




---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Frédéric Brière
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Thursday, April 22, 2010 - 5:58 pm

I'll relay the question; there appears to be an active community, but I
can't say how much of it is using Linux vs. Windows.

(I'm guessing many people install it once, copy their floppy collection,
and then move on.  At least that was my case; I'm not contributing for

TBH, it kinda flew over my head, so I have some reading-up to do on
these concepts.

My concern is that upstream might not look favorably on a rewrite that
only works on fast enough hardware, given how they aim for accurate
timing.  (There are no official specs, and therefore no way to know how
far one can safely stray from the ideal values.)  I don't think it would
be worth it to try and maintain a modified version without their
support.

I'll inquire and see what they think of this.

Thanks!


-- 
Real programmers don't bring brown-bag lunches.  If the vending machine
doesn't sell it, they don't eat it.  Vending machines don't sell quiche.

_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Frédéric Brière
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Friday, April 23, 2010 - 7:29 pm

I don't know if this is the proper place to follow up on your review,
then, but I did appreciate your advice, and would like to comment on a

It's conditional on MODULE_LICENSE itself; that's merely compatibility

For 8-bits-in-a-byte?  Does anyone do that?  (I count nearly 500

There are many timing values that occur only once throughout the code;
I'm not sure that replacing them all with #defines would be more
readable than a good set of comments.

Even worse, in cases such as above, the 20 is purely arbitrary.  So is
the udelay(100) in the loop.  What matters is the product, ie. we will
give up after 2000us, while polling a few times in the interval.

I'll try to look around for inspiration and see if there are other

On x86-64, I assume?  It's dead silent on i386, unfortunately.  Thanks

Does it matter, as long as it's not any less permissive?  I mean:

  $ git grep -l '(at your option) any later version' | wc -l
  5854

Contacting all copyright holders would be a hassle, so I just want to
make sure there's a good reason for it.


Again, thanks for your generous help!


-- 
Staff meeting in the conference room in 3 minutes.

_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Randy Dunlap
Subject: Re: [PATCH] Staging: add opencbm driver
Date: Friday, April 23, 2010 - 9:32 pm

Yes, it's conditional on MODULE_LICENSE() and KERNEL_VERSION().

I think that I wrote that then I saw what it meant and didn't go back


It's OK as is.  I recall some comments about making the kernel GPL v2 only,
but I guess that it never happened.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Greg KH
Subject: Re: Staging: add opencbm driver
Date: Thursday, April 29, 2010 - 11:47 am

The code fails to build:
  CC [M]  drivers/staging/opencbm/cbm_module.o
  drivers/staging/opencbm/cbm_module.c:260: error: conflicting type qualifiers for ‘irq_count’
  /home/gregkh/work/linux/git/staging-next-2.6/arch/x86/include/asm/processor.h:403: note: previous declaration of ‘irq_count’ was here

Care to fix this up and submit it again?

thanks,

greg k-h
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
[view original message]
From: Frédéric Brière
Subject: Re: Staging: add opencbm driver
Date: Thursday, April 29, 2010 - 12:53 pm

Damn my old 32-bit CPU, that was #ifdef CONFIG_X86_64.  Sorry about

Will do.  Thanks!


-- 
Old mail has arrived.
_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors

[ message continues ]
Previous thread: Re: New to Kernel programming and device drivers by archieval briones on Wednesday, April 21, 2010 - 5:29 pm. (1 message)

Next thread: Re: [PATCH] Staging: add opencbm driver by Archieval Briones on Thursday, April 22, 2010 - 9:43 pm. (2 messages)