folks,
Ive worked up a fairly trivial patch (attached) to scx200 and scx200_gpio.
Ive gotten the OK from the maintainer (also attached), so Im looking for
advice on how to proceed further. I thought about posting to kernel-mentors,
but one look at the size of work being reviewed there (plan9 fs), its not where
I belong..
Patch is trivial - w 1 small caveat - it adds a 'v' 'command-letter' to the
device-write-handler in the file-ops; this letter causes the driver to print the
current state of the pin-config. Its not incompatible, but its not invisible either.
echo vtp0v > /dev/gpio-20
prints to syslog 2x; once b4 setting 2 pin config bits, once after.
I also have a number of other changes in-queue, and I wanted some advice on which / how-many / what order to present to LKML for maximum likelyhood of success, and minimum of churn.
1. patch above comments out printks in scx200_gpio_write, maybe better way is to macro-ize them so theyre DBG able. (should I, and whats an acceptable Kconfig name ?) 2. shuffle some gpio code out of scx200.c, into scx200_gpio.c 3. eliminate some EXPORT_SYMBOLs
4. add more 'command-letters' V - to syslog the events-config-register various - to change bits in events-config-register
'Various' are debatable since they expose some configurablity to user-space that could get user into trouble, and dont give complete control either (ex enabling/disabling SMI, POWER & regular interrupts, etc) It could be made slightly safer with a CONFIG_SCX_GPIO_EXTRA
5. converting to new-style cdev driver registration I havent yet looked at doing this.
6. struct gpio_access_methods {
+ void (*gpio_config) (int index, u32 mask, u32 bits);
+ void (*gpio_dump) (int index);
+ int (*gpio_get) (int index);
+ int (*gpio_current) (int index);
...Goal here is to move towards supporting both scx200_gpio and pc87366 gpio in one nsc_gpio module. Both devices have some commonalities; pin-config bits, but differences also (access methods, address mapping, etc) I dont really know what Im doing here, and it may be sometime b4 I figure out how to proceed with this. But I do have code thats using the struct, and is able to toggle the LED (attached to gpio-20) on my soekris board. I can yank the current hackery for the 87366 (ugly, and not working yet), and just submit the abstraction..
7. playing nice with others
pc87366 is a super-io device thats driven by lm_sensors, so if I drive the gpio,
Ive got to share access with lm-sensors. My problem here is that the access-lock
is buried in a private structure, and in any case I need to be independent of
the lm-sensors drivers, not dependent upon them
So how does one share a lock with another module without 'hard-coding'
a symbol thats in one of the modules ? Is there a singleton aquire_lock() primitive
that would let me and a modified i2c/chips/pc8736x driver share a lock that neither
module 'owns', and is not hardcoded into the kernel-core itself ?
8. What have I missed ?
Ok, thats enough. tia Jim Cromie
diff -ruN -X exclude-diffs linux-2.6.11-r10c3-soekris-int/arch/i386/kernel/scx200.c myscx200-1/arch/i386/kernel/scx200.c
--- linux-2.6.11-r10c3-soekris-int/arch/i386/kernel/scx200.c 2005-05-05 22:35:27.000000000 -0600
+++ myscx200-1/arch/i386/kernel/scx200.c 2005-05-05 22:50:37.000000000 -0600
@@ -83,34 +83,17 @@
void scx200_gpio_dump(unsigned index)
{
- u32 config = scx200_gpio_configure(index, ~0, 0);
- printk(KERN_DEBUG "GPIO%02u: 0x%08lx", index, (unsigned long)config);
-
- if (config & 1)
- printk(" OE"); /* output enabled */
- else
- printk(" TS"); /* tristate */
- if (config & 2)
- printk(" PP"); /* push pull */
- else
- printk(" OD"); /* open drain */
- if (config & 4)
- printk(" PUE"); /* pull up enabled */
- else
- printk(" PUD"); /* pull up disabled */
- if (config & 8)
- printk(" LOCKED"); /* locked */
- if (config & 16)
- printk(" LEVEL"); /* level input */
- else
- printk(" EDGE"); /* edge input */
- if (config & 32)
- printk(" HI"); /* trigger on rising edge */
- else
- printk(" LO"); /* trigger on falling edge */
- if (config & 64)
- printk(" DEBOUNCE"); /* debounce */
- printk("\n");
+ u32 config = scx200_gpio_configure(index, ~0, 0);
+
+ printk(KERN_INFO NAME ": GPIO-%02u: 0x%08lx %s %s %s %s %s %s %s\n",
+ index, (unsigned long) config,
+ (config & 1) ? "OE" : "TS", /* output enabled / tristate */
+ (config & 2) ? "PP" : "OD", /* push pull / open drain */
+ (config & 4) ? "PUE" : "PUD", /* pull up enabled/disabled */
+ (config & 8) ? "LOCKED" : "", /* locked / unlocked */
+ (config & 16) ? "LEVEL" : "EDGE", /* level/edge input */
+ (config & 32) ? "HI" : "LO", /* trigger on rising/falling edge */
+ (config & 64) ? "DEBOUNCE" : ""); /* debounce */
}
int __init scx200_init(void)
diff -ruN -X exclude-diffs linux-2.6.11-r10c3-soekris-int/drivers/char/scx200_gpio.c myscx200-1/drivers/char/scx200_gpio.c
--- linux-2.6.11-r10c3-soekris-int/drivers/char/scx200_gpio.c 2005-05-05 22:35:40.000000000 -0600
+++ myscx200-1/drivers/char/scx200_gpio.c 2005-05-05 22:49:51.000000000 -0600
@@ -31,6 +31,7 @@
{
unsigned m = iminor(file->f_dentry->d_inode);
size_t i;
+ int err = 0;
for (i = 0; i < len; ++i) {
char c;
@@ -45,32 +46,43 @@
scx200_gpio_set(m, 1);
break;
case 'O':
- printk(KERN_INFO NAME ": GPIO%d output enabled\n", m);
+ /* printk(KERN_INFO NAME ": GPIO%d output enabled\n", m); */
scx200_gpio_configure(m, ~1, 1);
break;
case 'o':
- printk(KERN_INFO NAME ": GPIO%d output disabled\n", m);
+ /* printk(KERN_INFO NAME ": GPIO%d output disabled\n", m); */
scx200_gpio_configure(m, ~1, 0);
break;
case 'T':
- printk(KERN_INFO NAME ": GPIO%d output is push pull\n", m);
+ /* printk(KERN_INFO NAME ": GPIO%d output is push pull\n", m); */
scx200_gpio_configure(m, ~2, 2);
break;
case 't':
- printk(KERN_INFO NAME ": GPIO%d output is open drain\n", m);
+ /* printk(KERN_INFO NAME ": GPIO%d output is open drain\n", m); */
scx200_gpio_configure(m, ~2, 0);
break;
case 'P':
- printk(KERN_INFO NAME ": GPIO%d pull up enabled\n", m);
+ /* printk(KERN_INFO NAME ": GPIO%d pull up enabled\n", m); */
scx200_gpio_configure(m, ~4, 4);
break;
case 'p':
- printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m);
+ /* printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m); */
scx200_gpio_configure(m, ~4, 0);
break;
+
+ case 'v':
+ /* View Current pin settings */
+ scx200_gpio_dump(m);
+ break;
+ case '\n':
+ /* end of settings string, do nothing */
+ break;
+ default:
+ printk(KERN_ERR NAME ": GPIO-%2d bad setting: chr<0x%2x>\n", m, (int)c);
+ err++;
}
}
-
+ if (err) return -EINVAL; /* full string handled, report error */
return len;
}
--- Begin Message ---
- To: Jim Cromie <jcromie@xxxxxxxxxx>
- Subject: Re: scx200 & gpio [trivial patch]
- From: Christer Weinigel <christer@xxxxxxxxxxx>
- Date: 16 May 2005 22:56:45 +0200
- Delivered-to: jcromie@mail01.powweb.com
- In-reply-to: <427B6F4E.7070105@divsol.com>
- Organization: Weinigel Ingenjorsbyra AB
- References: <427B6F4E.7070105@divsol.com>
- Sender: wingel@xxxxxxxxxxxxxxx
- User-agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.3
Jim Cromie <jcromie@xxxxxxxxxx> writes: > hi Christer, > > he attached patch does 3 things (as will be obvious if you look): It looks good to me. > Are you actively maintaining scx200, and accepting patches ? > Do you have a publically accessible CVS of this code, > perhaps hidden at http://weinegel.se ? I'm not actively maintaining it anymore. I left the company that built Geode based computers a couple of years ago and haven't had much time for it since then. So please submit this directly to the linux-kernel mailing list, it probably has a better chance of getting into the kernel that way. Thanks, Christer -- "Just how much can I get away with and still go to heaven?" Freelance consultant specializing in device driver programming for Linux Christer Weinigel <christer@xxxxxxxxxxx> http://www.weinigel.se
--- End Message ---
