[patch -mm 01/20] chardev: GPIO for SCx200 & PC-8736x: whitespace pre-clean

[patch -mm 01/20] chardev: GPIO for SCx200 & PC-8736x: whitespace pre-clean

[Jim Cromie]

1/20. patch.preclean

Removed editor format-control comments, and used scripts/Lindent to
clean up whitespace, then deleted the bogus chunks :-(

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.preclean
 arch/i386/kernel/scx200.c   |    7 -------
 drivers/char/scx200_gpio.c  |   26 +++++++++-----------------
 include/linux/scx200.h      |    7 -------
 include/linux/scx200_gpio.h |    7 -------
 4 files changed, 9 insertions(+), 38 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-0/arch/i386/kernel/scx200.c ax-1/arch/i386/kernel/scx200.c
--- ax-0/arch/i386/kernel/scx200.c	2006-03-19 22:53:29.000000000 -0700
+++ ax-1/arch/i386/kernel/scx200.c	2006-06-17 00:55:59.000000000 -0600
@@ -159,10 +159,3 @@ EXPORT_SYMBOL(scx200_gpio_base);
 EXPORT_SYMBOL(scx200_gpio_shadow);
 EXPORT_SYMBOL(scx200_gpio_configure);
 EXPORT_SYMBOL(scx200_cb_base);
-
-/*
-    Local variables:
-        compile-command: "make -k -C ../../.. SUBDIRS=arch/i386/kernel modules"
-        c-basic-offset: 8
-    End:
-*/
diff -ruNp -X dontdiff -X exclude-diffs ax-0/drivers/char/scx200_gpio.c ax-1/drivers/char/scx200_gpio.c
--- ax-0/drivers/char/scx200_gpio.c	2006-03-19 22:53:29.000000000 -0700
+++ ax-1/drivers/char/scx200_gpio.c	2006-06-17 00:55:59.000000000 -0600
@@ -1,4 +1,4 @@
-/* linux/drivers/char/scx200_gpio.c 
+/* linux/drivers/char/scx200_gpio.c
 
    National Semiconductor SCx200 GPIO driver.  Allows a user space
    process to play with the GPIO pins.
@@ -26,7 +26,7 @@ static int major = 0;		/* default to dyn
 module_param(major, int, 0);
 MODULE_PARM_DESC(major, "Major device number");
 
-static ssize_t scx200_gpio_write(struct file *file, const char __user *data, 
+static ssize_t scx200_gpio_write(struct file *file, const char __user *data,
 				 size_t len, loff_t *ppos)
 {
 	unsigned m = iminor(file->f_dentry->d_inode);
@@ -34,15 +34,14 @@ static ssize_t scx200_gpio_write(struct 
 
 	for (i = 0; i < len; ++i) {
 		char c;
-		if (get_user(c, data+i))
+		if (get_user(c, data + i))
 			return -EFAULT;
-		switch (c)
-		{
-		case '0': 
-			scx200_gpio_set(m, 0); 
+		switch (c) {
+		case '0':
+			scx200_gpio_set(m, 0);
 			break;
-		case '1': 
-			scx200_gpio_set(m, 1); 
+		case '1':
+			scx200_gpio_set(m, 1);
 			break;
 		case 'O':
 			printk(KERN_INFO NAME ": GPIO%d output enabled\n", m);
@@ -83,7 +82,7 @@ static ssize_t scx200_gpio_read(struct f
 	value = scx200_gpio_get(m);
 	if (put_user(value ? '1' : '0', buf))
 		return -EFAULT;
-	
+
 	return 1;
 }
 
@@ -140,10 +139,3 @@ static void __exit scx200_gpio_cleanup(v
 
 module_init(scx200_gpio_init);
 module_exit(scx200_gpio_cleanup);
-
-/*
-    Local variables:
-        compile-command: "make -k -C ../.. SUBDIRS=drivers/char modules"
-        c-basic-offset: 8
-    End:
-*/
diff -ruNp -X dontdiff -X exclude-diffs ax-0/include/linux/scx200_gpio.h ax-1/include/linux/scx200_gpio.h
--- ax-0/include/linux/scx200_gpio.h	2006-03-19 22:53:29.000000000 -0700
+++ ax-1/include/linux/scx200_gpio.h	2006-06-17 00:55:59.000000000 -0600
@@ -87,10 +87,3 @@ static inline void scx200_gpio_change(in
 #undef __SCx200_GPIO_SHADOW
 #undef __SCx200_GPIO_INDEX
 #undef __SCx200_GPIO_OUT
-
-/*
-    Local variables:
-        compile-command: "make -C ../.. bzImage modules"
-        c-basic-offset: 8
-    End:
-*/
diff -ruNp -X dontdiff -X exclude-diffs ax-0/include/linux/scx200.h ax-1/include/linux/scx200.h
--- ax-0/include/linux/scx200.h	2006-03-19 22:53:29.000000000 -0700
+++ ax-1/include/linux/scx200.h	2006-06-17 00:55:59.000000000 -0600
@@ -49,10 +49,3 @@ extern unsigned scx200_cb_base;
 #define SCx200_REV 0x3d		/* Revision Register */
 #define SCx200_CBA 0x3e		/* Configuration Base Address Register */
 #define SCx200_CBA_SCRATCH 0x64	/* Configuration Base Address Scratchpad */
-
-/*
-    Local variables:
-        compile-command: "make -C ../.. bzImage modules"
-        c-basic-offset: 8
-    End:
-*/

[patch -mm 02/20] chardev: GPIO for SCx200 & PC-8736x: modernize driver init to 2.6 api

[Jim Cromie]

2/20. patch.api26

Adopt many modern 2.6 coding practices, ala LDD3, chapter 3.
Changes are limited to initialization calls from module init,
ie: cdev_init, cdev_add, *_chrdev_region, mkdev.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.api26
 scx200_gpio.c |   55 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 42 insertions(+), 13 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-1/drivers/char/scx200_gpio.c ax-2/drivers/char/scx200_gpio.c
--- ax-1/drivers/char/scx200_gpio.c	2006-06-17 00:55:59.000000000 -0600
+++ ax-2/drivers/char/scx200_gpio.c	2006-06-17 01:01:13.000000000 -0600
@@ -14,6 +14,9 @@
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
+#include <linux/types.h>
+#include <linux/cdev.h>
+
 #include <linux/scx200_gpio.h>
 
 #define NAME "scx200_gpio"
@@ -26,6 +29,8 @@ static int major = 0;		/* default to dyn
 module_param(major, int, 0);
 MODULE_PARM_DESC(major, "Major device number");
 
+extern void scx200_gpio_dump(unsigned index);
+
 static ssize_t scx200_gpio_write(struct file *file, const char __user *data,
 				 size_t len, loff_t *ppos)
 {
@@ -108,33 +113,57 @@ static struct file_operations scx200_gpi
 	.release = scx200_gpio_release,
 };
 
+struct cdev *scx200_devices;
+int num_devs = 32;
+
 static int __init scx200_gpio_init(void)
 {
-	int r;
+	int rc, i;
+	dev_t dev = MKDEV(major, 0);
 
 	printk(KERN_DEBUG NAME ": NatSemi SCx200 GPIO Driver\n");
 
 	if (!scx200_gpio_present()) {
-		printk(KERN_ERR NAME ": no SCx200 gpio pins available\n");
+		printk(KERN_ERR NAME ": no SCx200 gpio present\n");
 		return -ENODEV;
 	}
-
-	r = register_chrdev(major, NAME, &scx200_gpio_fops);
-	if (r < 0) {
-		printk(KERN_ERR NAME ": unable to register character device\n");
-		return r;
-	}
-	if (!major) {
-		major = r;
-		printk(KERN_DEBUG NAME ": got dynamic major %d\n", major);
+	if (major)
+		rc = register_chrdev_region(dev, num_devs, "scx200_gpio");
+	else {
+		rc = alloc_chrdev_region(&dev, 0, num_devs, "scx200_gpio");
+		major = MAJOR(dev);
+	}
+	if (rc < 0) {
+		printk(KERN_ERR NAME ": SCx200 chrdev_region: %d\n", rc);
+		return rc;
+	}
+	scx200_devices = kzalloc(num_devs * sizeof(struct cdev), GFP_KERNEL);
+	if (!scx200_devices) {
+		rc = -ENOMEM;
+		goto fail_malloc;
+	}
+	for (i = 0; i < num_devs; i++) {
+		struct cdev *cdev = &scx200_devices[i];
+		cdev_init(cdev, &scx200_gpio_fops);
+		cdev->owner = THIS_MODULE;
+		cdev->ops = &scx200_gpio_fops;
+		rc = cdev_add(cdev, MKDEV(major, i), 1);
+		/* Fail gracefully if need be */
+		if (rc)
+			printk(KERN_ERR NAME "Error %d on minor %d", rc, i);
 	}
 
-	return 0;
+	return 0;		/* succeed */
+
+fail_malloc:
+	unregister_chrdev_region(dev, num_devs);
+	return rc;
 }
 
 static void __exit scx200_gpio_cleanup(void)
 {
-	unregister_chrdev(major, NAME);
+	kfree(scx200_devices);
+	unregister_chrdev_region(MKDEV(major, 0), num_devs);
 }
 
 module_init(scx200_gpio_init);

Re: [patch -mm 02/20] chardev: GPIO for SCx200 & PC-8736x: modernize driver init to 2.6 api

[Andrew Morton]

On Sat, 17 Jun 2006 12:24:10 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> 2/20. patch.api26
> 
> Adopt many modern 2.6 coding practices, ala LDD3, chapter 3.
> Changes are limited to initialization calls from module init,
> ie: cdev_init, cdev_add, *_chrdev_region, mkdev.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> 
> ---
> 
> diffstat gpio-scx/patch.api26
>  scx200_gpio.c |   55 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff -ruNp -X dontdiff -X exclude-diffs ax-1/drivers/char/scx200_gpio.c ax-2/drivers/char/scx200_gpio.c
> --- ax-1/drivers/char/scx200_gpio.c	2006-06-17 00:55:59.000000000 -0600
> +++ ax-2/drivers/char/scx200_gpio.c	2006-06-17 01:01:13.000000000 -0600
> @@ -14,6 +14,9 @@
>  #include <asm/uaccess.h>
>  #include <asm/io.h>
>  
> +#include <linux/types.h>
> +#include <linux/cdev.h>
> +
>  #include <linux/scx200_gpio.h>
>  
>  #define NAME "scx200_gpio"
> @@ -26,6 +29,8 @@ static int major = 0;		/* default to dyn
>  module_param(major, int, 0);
>  MODULE_PARM_DESC(major, "Major device number");
>  
> +extern void scx200_gpio_dump(unsigned index);

extern declarations should go in .h files.

>  static ssize_t scx200_gpio_write(struct file *file, const char __user *data,
>  				 size_t len, loff_t *ppos)
>  {
> @@ -108,33 +113,57 @@ static struct file_operations scx200_gpi
>  	.release = scx200_gpio_release,
>  };
>  
> +struct cdev *scx200_devices;
> +int num_devs = 32;

`num_devs' is too generic a name for a global symbol.

[patch -mm 03/20] chardev: GPIO for SCx200 & PC-8736x: add platforn_device for use w dev_dbg

[Jim Cromie]

3/20. patch.platform-dev-2

Add a platform-device to scx200_gpio, and use its struct device dev
member (ie: devp) in dev_dbg() once.

There are 2 alternatives here (Im soliciting guidance/commentary):

- use isa_device, if/when its added to the kernel.

- alter scx200.c to EXPORT_GPL its private devp so that both
scx200_gpio, and the (to be added) nsc_gpio module can use it.
Since the available devp is in 'grandparent', this seems like
too much 'action at a distance'.


Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.platform-dev-2
 scx200_gpio.c |   40 +++++++++++++++++++++++++++++-----------
 1 files changed, 29 insertions(+), 11 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-2/drivers/char/scx200_gpio.c ax-3/drivers/char/scx200_gpio.c
--- ax-2/drivers/char/scx200_gpio.c	2006-06-17 01:01:13.000000000 -0600
+++ ax-3/drivers/char/scx200_gpio.c	2006-06-17 01:04:20.000000000 -0600
@@ -6,11 +6,13 @@
    Copyright (c) 2001,2002 Christer Weinigel <wingel@nano-system.com> */
 
 #include <linux/config.h>
+#include <linux/device.h>
 #include <linux/fs.h>
 #include <linux/module.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/platform_device.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
@@ -20,6 +22,9 @@
 #include <linux/scx200_gpio.h>
 
 #define NAME "scx200_gpio"
+#define DEVNAME NAME
+
+static struct platform_device *pdev;
 
 MODULE_AUTHOR("Christer Weinigel <wingel@nano-system.com>");
 MODULE_DESCRIPTION("NatSemi SCx200 GPIO Pin Driver");
@@ -121,12 +126,20 @@ static int __init scx200_gpio_init(void)
 	int rc, i;
 	dev_t dev = MKDEV(major, 0);
 
-	printk(KERN_DEBUG NAME ": NatSemi SCx200 GPIO Driver\n");
-
 	if (!scx200_gpio_present()) {
 		printk(KERN_ERR NAME ": no SCx200 gpio present\n");
 		return -ENODEV;
 	}
+
+	/* support dev_dbg() with pdev->dev */
+	pdev = platform_device_alloc(DEVNAME, 0);
+	if (!pdev)
+		return -ENODEV;
+
+	rc = platform_device_add(pdev);
+	if (rc)
+		goto undo_platform_device_add;
+
 	if (major)
 		rc = register_chrdev_region(dev, num_devs, "scx200_gpio");
 	else {
@@ -134,29 +147,31 @@ static int __init scx200_gpio_init(void)
 		major = MAJOR(dev);
 	}
 	if (rc < 0) {
-		printk(KERN_ERR NAME ": SCx200 chrdev_region: %d\n", rc);
-		return rc;
+		dev_err(&pdev->dev, "SCx200 chrdev_region err: %d\n", rc);
+		goto undo_platform_device_add;
 	}
 	scx200_devices = kzalloc(num_devs * sizeof(struct cdev), GFP_KERNEL);
 	if (!scx200_devices) {
 		rc = -ENOMEM;
-		goto fail_malloc;
+		goto undo_chrdev_region;
 	}
 	for (i = 0; i < num_devs; i++) {
 		struct cdev *cdev = &scx200_devices[i];
 		cdev_init(cdev, &scx200_gpio_fops);
 		cdev->owner = THIS_MODULE;
-		cdev->ops = &scx200_gpio_fops;
 		rc = cdev_add(cdev, MKDEV(major, i), 1);
-		/* Fail gracefully if need be */
+		/* tolerate 'minor' errors */
 		if (rc)
-			printk(KERN_ERR NAME "Error %d on minor %d", rc, i);
+			dev_err(&pdev->dev, "Error %d on minor %d", rc, i);
 	}
 
-	return 0;		/* succeed */
+	return 0; /* succeed */
 
-fail_malloc:
-	unregister_chrdev_region(dev, num_devs);
+undo_chrdev_region:
+        unregister_chrdev_region(dev, num_devs);
+undo_platform_device_add:
+	platform_device_put(pdev);
+	kfree(pdev);		/* undo platform_device_alloc */
 	return rc;
 }
 
@@ -164,6 +179,9 @@ static void __exit scx200_gpio_cleanup(v
 {
 	kfree(scx200_devices);
 	unregister_chrdev_region(MKDEV(major, 0), num_devs);
+	platform_device_put(pdev);
+	platform_device_unregister(pdev);
+	/* kfree(pdev); */
 }
 
 module_init(scx200_gpio_init);

Re: [patch -mm 03/20] chardev: GPIO for SCx200 & PC-8736x: add platforn_device for use w dev_dbg

[Andrew Morton]

On Sat, 17 Jun 2006 12:25:08 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> 3/20. patch.platform-dev-2
> 
> Add a platform-device to scx200_gpio, and use its struct device dev
> member (ie: devp) in dev_dbg() once.
> 
> There are 2 alternatives here (Im soliciting guidance/commentary):
> 
> - use isa_device, if/when its added to the kernel.
> 
> - alter scx200.c to EXPORT_GPL its private devp so that both
> scx200_gpio, and the (to be added) nsc_gpio module can use it.
> Since the available devp is in 'grandparent', this seems like
> too much 'action at a distance'.
> 
> 
> @@ -121,12 +126,20 @@ static int __init scx200_gpio_init(void)
>  	int rc, i;
>  	dev_t dev = MKDEV(major, 0);
>  
> -	printk(KERN_DEBUG NAME ": NatSemi SCx200 GPIO Driver\n");
> -
>  	if (!scx200_gpio_present()) {
>  		printk(KERN_ERR NAME ": no SCx200 gpio present\n");
>  		return -ENODEV;
>  	}
> +
> +	/* support dev_dbg() with pdev->dev */
> +	pdev = platform_device_alloc(DEVNAME, 0);
> +	if (!pdev)
> +		return -ENODEV;

-ENOMEM would be more accurate.

> +	rc = platform_device_add(pdev);
> +	if (rc)
> +		goto undo_platform_device_add;

If the platform_device_add() didn't work, I don't think we need to undo it?

>  	if (major)
>  		rc = register_chrdev_region(dev, num_devs, "scx200_gpio");
>  	else {
> @@ -134,29 +147,31 @@ static int __init scx200_gpio_init(void)
>  		major = MAJOR(dev);
>  	}
>  	if (rc < 0) {
> -		printk(KERN_ERR NAME ": SCx200 chrdev_region: %d\n", rc);
> -		return rc;
> +		dev_err(&pdev->dev, "SCx200 chrdev_region err: %d\n", rc);
> +		goto undo_platform_device_add;
>  	}
>  	scx200_devices = kzalloc(num_devs * sizeof(struct cdev), GFP_KERNEL);
>  	if (!scx200_devices) {
>  		rc = -ENOMEM;
> -		goto fail_malloc;
> +		goto undo_chrdev_region;
>  	}
>  	for (i = 0; i < num_devs; i++) {
>  		struct cdev *cdev = &scx200_devices[i];
>  		cdev_init(cdev, &scx200_gpio_fops);
>  		cdev->owner = THIS_MODULE;
> -		cdev->ops = &scx200_gpio_fops;
>  		rc = cdev_add(cdev, MKDEV(major, i), 1);
> -		/* Fail gracefully if need be */
> +		/* tolerate 'minor' errors */
>  		if (rc)
> -			printk(KERN_ERR NAME "Error %d on minor %d", rc, i);
> +			dev_err(&pdev->dev, "Error %d on minor %d", rc, i);
>  	}
>  
> -	return 0;		/* succeed */
> +	return 0; /* succeed */
>  
> -fail_malloc:
> -	unregister_chrdev_region(dev, num_devs);
> +undo_chrdev_region:
> +        unregister_chrdev_region(dev, num_devs);

needs a tab.

> +undo_platform_device_add:
> +	platform_device_put(pdev);
> +	kfree(pdev);		/* undo platform_device_alloc */
>  	return rc;
>  }
>  
> @@ -164,6 +179,9 @@ static void __exit scx200_gpio_cleanup(v
>  {
>  	kfree(scx200_devices);
>  	unregister_chrdev_region(MKDEV(major, 0), num_devs);
> +	platform_device_put(pdev);
> +	platform_device_unregister(pdev);
> +	/* kfree(pdev); */
>  }

[patch -mm 04/20] chardev: GPIO for SCx200 & PC-8736x: device minor numbers are unsigned ints

[Jim Cromie]

4/20. patch.unsigned-minor

Per kernel headers, device minor numbers are unsigned ints.
Do the same in this driver.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.unsigned-minor
 arch/i386/kernel/scx200.c   |    2 +-
 include/linux/scx200_gpio.h |   14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-3/arch/i386/kernel/scx200.c ax-4/arch/i386/kernel/scx200.c
--- ax-3/arch/i386/kernel/scx200.c	2006-06-17 00:55:59.000000000 -0600
+++ ax-4/arch/i386/kernel/scx200.c	2006-06-17 01:07:24.000000000 -0600
@@ -87,7 +87,7 @@ static int __devinit scx200_probe(struct
 	return 0;
 }
 
-u32 scx200_gpio_configure(int index, u32 mask, u32 bits)
+u32 scx200_gpio_configure(unsigned index, u32 mask, u32 bits)
 {
 	u32 config, new_config;
 	unsigned long flags;
diff -ruNp -X dontdiff -X exclude-diffs ax-3/include/linux/scx200_gpio.h ax-4/include/linux/scx200_gpio.h
--- ax-3/include/linux/scx200_gpio.h	2006-06-17 00:55:59.000000000 -0600
+++ ax-4/include/linux/scx200_gpio.h	2006-06-17 01:07:24.000000000 -0600
@@ -1,6 +1,6 @@
 #include <linux/spinlock.h>
 
-u32 scx200_gpio_configure(int index, u32 set, u32 clear);
+u32 scx200_gpio_configure(unsigned index, u32 set, u32 clear);
 
 extern unsigned scx200_gpio_base;
 extern long scx200_gpio_shadow[2];
@@ -17,7 +17,7 @@ extern long scx200_gpio_shadow[2];
 
 /* returns the value of the GPIO pin */
 
-static inline int scx200_gpio_get(int index) {
+static inline int scx200_gpio_get(unsigned index) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR + 0x04;
 	__SCx200_GPIO_INDEX;
@@ -29,7 +29,7 @@ static inline int scx200_gpio_get(int in
    driven if the GPIO is configured as an output, it might not be the
    state of the GPIO right now if the GPIO is configured as an input) */
 
-static inline int scx200_gpio_current(int index) {
+static inline int scx200_gpio_current(unsigned index) {
         __SCx200_GPIO_BANK;
 	__SCx200_GPIO_INDEX;
 		
@@ -38,7 +38,7 @@ static inline int scx200_gpio_current(in
 
 /* drive the GPIO signal high */
 
-static inline void scx200_gpio_set_high(int index) {
+static inline void scx200_gpio_set_high(unsigned index) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR;
 	__SCx200_GPIO_SHADOW;
@@ -49,7 +49,7 @@ static inline void scx200_gpio_set_high(
 
 /* drive the GPIO signal low */
 
-static inline void scx200_gpio_set_low(int index) {
+static inline void scx200_gpio_set_low(unsigned index) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR;
 	__SCx200_GPIO_SHADOW;
@@ -60,7 +60,7 @@ static inline void scx200_gpio_set_low(i
 
 /* drive the GPIO signal to state */
 
-static inline void scx200_gpio_set(int index, int state) {
+static inline void scx200_gpio_set(unsigned index, int state) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR;
 	__SCx200_GPIO_SHADOW;
@@ -73,7 +73,7 @@ static inline void scx200_gpio_set(int i
 }
 
 /* toggle the GPIO signal */
-static inline void scx200_gpio_change(int index) {
+static inline void scx200_gpio_change(unsigned index) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR;
 	__SCx200_GPIO_SHADOW;

[patch -mm 05/20] chardev: GPIO for SCx200 & PC-8736x: put gpio_dump on a diet

[Jim Cromie]

5/20. patch.dump-diet

Shrink scx200_gpio_dump() to a single printk with ternary ops.  The
function is still ifdef'd out, this is corrected in next patch, when
it is actually used.

The patch 'inadvertently' changed loglevel from DEBUG to INFO.  This
is Good, because in next patch, its wired to a 'command' which the
user can invoke when they want.  When they do so, its because they
want INFO to support their developement effort, and we want to give it
to them without compiling a DEBUG version of the driver.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.dump-diet
 scx200.c |   39 +++++++++++----------------------------
 1 files changed, 11 insertions(+), 28 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-4/arch/i386/kernel/scx200.c ax-5/arch/i386/kernel/scx200.c
--- ax-4/arch/i386/kernel/scx200.c	2006-06-17 01:07:24.000000000 -0600
+++ ax-5/arch/i386/kernel/scx200.c	2006-06-17 01:10:02.000000000 -0600
@@ -108,34 +108,17 @@ u32 scx200_gpio_configure(unsigned index
 #if 0
 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 */
 }
 #endif  /*  0  */
 

Re: [patch -mm 05/20] chardev: GPIO for SCx200 & PC-8736x: put gpio_dump on a diet

[Andrew Morton]

On Sat, 17 Jun 2006 12:27:02 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> +        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 */
>  }

Please use hard tabs.

Casting `config' to ulong isn't needed here.

[patch -mm 06/20] chardev: GPIO for SCx200 & PC-8736x: add 'v' command to device-file

[Jim Cromie]

6/20. patch.viewpins

Add a new driver command: 'v' which calls gpio_dump() on the pin.  The
output goes to the log, like all other INFO messages in the original
driver.  Giving the user control over the feedback they 'need' is
construed to be a user-friendly feature, and allows us (later) to dial
down many INFO messages to DEBUG log-level.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.viewpins
 arch/i386/kernel/scx200.c  |    3 +--
 drivers/char/scx200_gpio.c |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-5/arch/i386/kernel/scx200.c ax-6/arch/i386/kernel/scx200.c
--- ax-5/arch/i386/kernel/scx200.c	2006-06-17 01:10:02.000000000 -0600
+++ ax-6/arch/i386/kernel/scx200.c	2006-06-17 01:13:26.000000000 -0600
@@ -105,7 +105,6 @@ u32 scx200_gpio_configure(unsigned index
 	return config;
 }
 
-#if 0
 void scx200_gpio_dump(unsigned index)
 {
         u32 config = scx200_gpio_configure(index, ~0, 0);
@@ -120,7 +119,6 @@ void scx200_gpio_dump(unsigned index)
                (config & 32) ? "HI"     : "LO",		/* trigger on rising/falling edge */ 
                (config & 64) ? "DEBOUNCE" : "");	/* debounce */
 }
-#endif  /*  0  */
 
 static int __init scx200_init(void)
 {
@@ -141,4 +139,5 @@ module_exit(scx200_cleanup);
 EXPORT_SYMBOL(scx200_gpio_base);
 EXPORT_SYMBOL(scx200_gpio_shadow);
 EXPORT_SYMBOL(scx200_gpio_configure);
+EXPORT_SYMBOL(scx200_gpio_dump);
 EXPORT_SYMBOL(scx200_cb_base);
diff -ruNp -X dontdiff -X exclude-diffs ax-5/drivers/char/scx200_gpio.c ax-6/drivers/char/scx200_gpio.c
--- ax-5/drivers/char/scx200_gpio.c	2006-06-17 01:04:20.000000000 -0600
+++ ax-6/drivers/char/scx200_gpio.c	2006-06-17 01:13:26.000000000 -0600
@@ -41,6 +41,7 @@ static ssize_t scx200_gpio_write(struct 
 {
 	unsigned m = iminor(file->f_dentry->d_inode);
 	size_t i;
+	int err = 0;
 
 	for (i = 0; i < len; ++i) {
 		char c;
@@ -77,8 +78,23 @@ static ssize_t scx200_gpio_write(struct 
 			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;
 }

[patch -mm 07/20] chardev: GPIO for SCx200 & PC-8736x: refactor scx200_probe to better segregate _gpio initialization

[Jim Cromie]

7/20. patch.init-refactor

Pull shadow-reg initialization into separate function now, rather than
doing it 2x later (scx200, pc8736x).  When we revisit 2nd drvr below,
it will be to reimplement an init function, rather than another
refactor.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.init-refactor
 scx200.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-6/arch/i386/kernel/scx200.c ax-7/arch/i386/kernel/scx200.c
--- ax-6/arch/i386/kernel/scx200.c	2006-06-17 01:13:26.000000000 -0600
+++ ax-7/arch/i386/kernel/scx200.c	2006-06-17 01:17:11.000000000 -0600
@@ -47,9 +47,17 @@ static struct pci_driver scx200_pci_driv
 
 static DEFINE_SPINLOCK(scx200_gpio_config_lock);
 
-static int __devinit scx200_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+static void __devinit scx200_init_shadow(void)
 {
 	int bank;
+
+	/* read the current values driven on the GPIO signals */
+	for (bank = 0; bank < 2; ++bank)
+		scx200_gpio_shadow[bank] = inl(scx200_gpio_base + 0x10 * bank);
+}
+
+static int __devinit scx200_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
 	unsigned base;
 
 	if (pdev->device == PCI_DEVICE_ID_NS_SCx200_BRIDGE ||
@@ -63,10 +71,7 @@ static int __devinit scx200_probe(struct
 		}
 
 		scx200_gpio_base = base;
-
-		/* read the current values driven on the GPIO signals */
-		for (bank = 0; bank < 2; ++bank)
-			scx200_gpio_shadow[bank] = inl(scx200_gpio_base + 0x10 * bank);
+		scx200_init_shadow();
 
 	} else {
 		/* find the base of the Configuration Block */

[patch -mm 09/20] chardev: GPIO for SCx200 & PC-8736x: dispatch via vtable

[Jim Cromie]

9/20. patch.vtable-calls

Now actually call the gpio operations thru the vtable.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.vtable-calls
 scx200_gpio.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-8/drivers/char/scx200_gpio.c ax-9/drivers/char/scx200_gpio.c
--- ax-8/drivers/char/scx200_gpio.c	2006-06-17 01:20:34.000000000 -0600
+++ ax-9/drivers/char/scx200_gpio.c	2006-06-17 01:23:47.000000000 -0600
@@ -53,6 +53,7 @@ static ssize_t scx200_gpio_write(struct 
 				 size_t len, loff_t *ppos)
 {
 	unsigned m = iminor(file->f_dentry->d_inode);
+	struct nsc_gpio_ops *amp = file->private_data;
 	size_t i;
 	int err = 0;
 
@@ -62,39 +63,39 @@ static ssize_t scx200_gpio_write(struct 
 			return -EFAULT;
 		switch (c) {
 		case '0':
-			scx200_gpio_set(m, 0);
+			amp->gpio_set(m, 0);
 			break;
 		case '1':
-			scx200_gpio_set(m, 1);
+			amp->gpio_set(m, 1);
 			break;
 		case 'O':
 			printk(KERN_INFO NAME ": GPIO%d output enabled\n", m);
-			scx200_gpio_configure(m, ~1, 1);
+			amp->gpio_config(m, ~1, 1);
 			break;
 		case 'o':
 			printk(KERN_INFO NAME ": GPIO%d output disabled\n", m);
-			scx200_gpio_configure(m, ~1, 0);
+			amp->gpio_config(m, ~1, 0);
 			break;
 		case 'T':
 			printk(KERN_INFO NAME ": GPIO%d output is push pull\n", m);
-			scx200_gpio_configure(m, ~2, 2);
+			amp->gpio_config(m, ~2, 2);
 			break;
 		case 't':
 			printk(KERN_INFO NAME ": GPIO%d output is open drain\n", m);
-			scx200_gpio_configure(m, ~2, 0);
+			amp->gpio_config(m, ~2, 0);
 			break;
 		case 'P':
 			printk(KERN_INFO NAME ": GPIO%d pull up enabled\n", m);
-			scx200_gpio_configure(m, ~4, 4);
+			amp->gpio_config(m, ~4, 4);
 			break;
 		case 'p':
 			printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m);
-			scx200_gpio_configure(m, ~4, 0);
+			amp->gpio_config(m, ~4, 0);
 			break;
 
 		case 'v':
 			/* View Current pin settings */
-			scx200_gpio_dump(m);
+			amp->gpio_dump(m);
 			break;
 		case '\n':
 			/* end of settings string, do nothing */
@@ -117,8 +118,9 @@ static ssize_t scx200_gpio_read(struct f
 {
 	unsigned m = iminor(file->f_dentry->d_inode);
 	int value;
+	struct nsc_gpio_ops *amp = file->private_data;
 
-	value = scx200_gpio_get(m);
+	value = amp->gpio_get(m);
 	if (put_user(value ? '1' : '0', buf))
 		return -EFAULT;
 
@@ -128,6 +130,8 @@ static ssize_t scx200_gpio_read(struct f
 static int scx200_gpio_open(struct inode *inode, struct file *file)
 {
 	unsigned m = iminor(inode);
+	file->private_data = &scx200_access;
+
 	if (m > 63)
 		return -EINVAL;
 	return nonseekable_open(inode, file);

[patch -mm 10/20] chardev: GPIO for SCx200 & PC-8736x: add empty common-module

[Jim Cromie]

10/20. patch.nscgpio-shell

Add the nsc_gpio common-support module as an empty shell.  Next patch
starts the migration of the common gpio support routines.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.nscgpio-shell
 Makefile   |    2 +-
 nsc_gpio.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-9/drivers/char/Makefile ax-10/drivers/char/Makefile
--- ax-9/drivers/char/Makefile	2006-06-06 17:15:36.000000000 -0600
+++ ax-10/drivers/char/Makefile	2006-06-17 01:27:04.000000000 -0600
@@ -81,7 +81,7 @@ obj-$(CONFIG_COBALT_LCD)	+= lcd.o
 obj-$(CONFIG_PPDEV)		+= ppdev.o
 obj-$(CONFIG_NWBUTTON)		+= nwbutton.o
 obj-$(CONFIG_NWFLASH)		+= nwflash.o
-obj-$(CONFIG_SCx200_GPIO)	+= scx200_gpio.o
+obj-$(CONFIG_SCx200_GPIO)	+= scx200_gpio.o nsc_gpio.o
 obj-$(CONFIG_CS5535_GPIO)	+= cs5535_gpio.o
 obj-$(CONFIG_GPIO_VR41XX)	+= vr41xx_giu.o
 obj-$(CONFIG_TANBAC_TB0219)	+= tb0219.o
diff -ruNp -X dontdiff -X exclude-diffs ax-9/drivers/char/nsc_gpio.c ax-10/drivers/char/nsc_gpio.c
--- ax-9/drivers/char/nsc_gpio.c	1969-12-31 17:00:00.000000000 -0700
+++ ax-10/drivers/char/nsc_gpio.c	2006-06-17 01:27:04.000000000 -0600
@@ -0,0 +1,45 @@
+/* linux/drivers/char/nsc_gpio.c
+
+   National Semiconductor common GPIO device-file/VFS methods.
+   Allows a user space process to control the GPIO pins.
+
+   Copyright (c) 2001,2002 Christer Weinigel <wingel@nano-system.com>
+   Copyright (c) 2005      Jim Cromie <jim.cromie@gmail.com>
+*/
+
+#include <linux/config.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/nsc_gpio.h>
+#include <asm/uaccess.h>
+#include <asm/io.h>
+
+#define NAME "nsc_gpio"
+
+MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
+MODULE_DESCRIPTION("NatSemi GPIO Common Methods");
+MODULE_LICENSE("GPL");
+
+static int __init nsc_gpio_init(void)
+{
+	printk(KERN_DEBUG NAME " initializing\n");
+	return 0;
+}
+
+static void __exit nsc_gpio_cleanup(void)
+{
+	printk(KERN_DEBUG NAME " cleanup\n");
+}
+
+/* prepare for
+   common routines for both scx200_gpio and pc87360_gpio
+EXPORT_SYMBOL(scx200_gpio_write);
+EXPORT_SYMBOL(scx200_gpio_read);
+EXPORT_SYMBOL(scx200_gpio_release);
+*/
+
+module_init(nsc_gpio_init);
+module_exit(nsc_gpio_cleanup);

[patch -mm 11/20] chardev: GPIO for SCx200 & PC-8736x: migrate file-ops to common module

[Jim Cromie]

11/20. patch.migrate-fops

Now that the read(), write() file-ops are dispatching gpio-ops via the
vtable, they are generic, and can be moved 'verbatim' to the nsc_gpio
common-support module.  After the move, various symbols are renamed to
update 'scx200_' to 'nsc_', and headers are adjusted accordingly.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.migrate-fops
 drivers/char/nsc_gpio.c    |   97 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/char/scx200_gpio.c |   88 +++-------------------------------------
 include/linux/nsc_gpio.h   |    5 ++
 3 files changed, 100 insertions(+), 90 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-10/drivers/char/nsc_gpio.c ax-11/drivers/char/nsc_gpio.c
--- ax-10/drivers/char/nsc_gpio.c	2006-06-17 01:27:04.000000000 -0600
+++ ax-11/drivers/char/nsc_gpio.c	2006-06-17 01:33:50.000000000 -0600
@@ -19,9 +19,89 @@
 
 #define NAME "nsc_gpio"
 
-MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
-MODULE_DESCRIPTION("NatSemi GPIO Common Methods");
-MODULE_LICENSE("GPL");
+ssize_t nsc_gpio_write(struct file *file, const char __user *data,
+		       size_t len, loff_t *ppos)
+{
+	unsigned m = iminor(file->f_dentry->d_inode);
+	struct nsc_gpio_ops *amp = file->private_data;
+	size_t i;
+	int err = 0;
+
+	for (i = 0; i < len; ++i) {
+		char c;
+		if (get_user(c, data + i))
+			return -EFAULT;
+		switch (c) {
+		case '0':
+			amp->gpio_set(m, 0);
+			break;
+		case '1':
+			amp->gpio_set(m, 1);
+			break;
+		case 'O':
+			printk(KERN_INFO NAME ": GPIO%d output enabled\n", m);
+			amp->gpio_config(m, ~1, 1);
+			break;
+		case 'o':
+			printk(KERN_INFO NAME ": GPIO%d output disabled\n", m);
+			amp->gpio_config(m, ~1, 0);
+			break;
+		case 'T':
+			printk(KERN_INFO NAME ": GPIO%d output is push pull\n",
+			       m);
+			amp->gpio_config(m, ~2, 2);
+			break;
+		case 't':
+			printk(KERN_INFO NAME ": GPIO%d output is open drain\n",
+			       m);
+			amp->gpio_config(m, ~2, 0);
+			break;
+		case 'P':
+			printk(KERN_INFO NAME ": GPIO%d pull up enabled\n", m);
+			amp->gpio_config(m, ~4, 4);
+			break;
+		case 'p':
+			printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m);
+			amp->gpio_config(m, ~4, 0);
+			break;
+
+		case 'v':
+			/* View Current pin settings */
+			amp->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;
+}
+
+ssize_t nsc_gpio_read(struct file *file, char __user * buf,
+		      size_t len, loff_t * ppos)
+{
+	unsigned m = iminor(file->f_dentry->d_inode);
+	int value;
+	struct nsc_gpio_ops *amp = file->private_data;
+
+	value = amp->gpio_get(m);
+	if (put_user(value ? '1' : '0', buf))
+		return -EFAULT;
+
+	return 1;
+}
+
+/* common routines for both scx200_gpio and pc87360_gpio */
+EXPORT_SYMBOL(nsc_gpio_write);
+EXPORT_SYMBOL(nsc_gpio_read);
 
 static int __init nsc_gpio_init(void)
 {
@@ -34,12 +114,9 @@ static void __exit nsc_gpio_cleanup(void
 	printk(KERN_DEBUG NAME " cleanup\n");
 }
 
-/* prepare for
-   common routines for both scx200_gpio and pc87360_gpio
-EXPORT_SYMBOL(scx200_gpio_write);
-EXPORT_SYMBOL(scx200_gpio_read);
-EXPORT_SYMBOL(scx200_gpio_release);
-*/
-
 module_init(nsc_gpio_init);
 module_exit(nsc_gpio_cleanup);
+
+MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
+MODULE_DESCRIPTION("NatSemi GPIO Common Methods");
+MODULE_LICENSE("GPL");
diff -ruNp -X dontdiff -X exclude-diffs ax-10/drivers/char/scx200_gpio.c ax-11/drivers/char/scx200_gpio.c
--- ax-10/drivers/char/scx200_gpio.c	2006-06-17 01:23:47.000000000 -0600
+++ ax-11/drivers/char/scx200_gpio.c	2006-06-17 01:33:50.000000000 -0600
@@ -37,6 +37,12 @@ MODULE_PARM_DESC(major, "Major device nu
 
 extern void scx200_gpio_dump(unsigned index);
 
+extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
+			      size_t len, loff_t *ppos);
+
+extern ssize_t nsc_gpio_read(struct file *file, char __user *buf,
+			     size_t len, loff_t *ppos);
+
 struct nsc_gpio_ops scx200_access = {
 	.owner		= THIS_MODULE,
 	.gpio_config	= scx200_gpio_configure,
@@ -49,84 +55,6 @@ struct nsc_gpio_ops scx200_access = {
 	.gpio_current	= scx200_gpio_current
 };
 
-static ssize_t scx200_gpio_write(struct file *file, const char __user *data,
-				 size_t len, loff_t *ppos)
-{
-	unsigned m = iminor(file->f_dentry->d_inode);
-	struct nsc_gpio_ops *amp = file->private_data;
-	size_t i;
-	int err = 0;
-
-	for (i = 0; i < len; ++i) {
-		char c;
-		if (get_user(c, data + i))
-			return -EFAULT;
-		switch (c) {
-		case '0':
-			amp->gpio_set(m, 0);
-			break;
-		case '1':
-			amp->gpio_set(m, 1);
-			break;
-		case 'O':
-			printk(KERN_INFO NAME ": GPIO%d output enabled\n", m);
-			amp->gpio_config(m, ~1, 1);
-			break;
-		case 'o':
-			printk(KERN_INFO NAME ": GPIO%d output disabled\n", m);
-			amp->gpio_config(m, ~1, 0);
-			break;
-		case 'T':
-			printk(KERN_INFO NAME ": GPIO%d output is push pull\n", m);
-			amp->gpio_config(m, ~2, 2);
-			break;
-		case 't':
-			printk(KERN_INFO NAME ": GPIO%d output is open drain\n", m);
-			amp->gpio_config(m, ~2, 0);
-			break;
-		case 'P':
-			printk(KERN_INFO NAME ": GPIO%d pull up enabled\n", m);
-			amp->gpio_config(m, ~4, 4);
-			break;
-		case 'p':
-			printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m);
-			amp->gpio_config(m, ~4, 0);
-			break;
-
-		case 'v':
-			/* View Current pin settings */
-			amp->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;
-}
-
-static ssize_t scx200_gpio_read(struct file *file, char __user *buf,
-				size_t len, loff_t *ppos)
-{
-	unsigned m = iminor(file->f_dentry->d_inode);
-	int value;
-	struct nsc_gpio_ops *amp = file->private_data;
-
-	value = amp->gpio_get(m);
-	if (put_user(value ? '1' : '0', buf))
-		return -EFAULT;
-
-	return 1;
-}
-
 static int scx200_gpio_open(struct inode *inode, struct file *file)
 {
 	unsigned m = iminor(inode);
@@ -145,8 +73,8 @@ static int scx200_gpio_release(struct in
 
 static struct file_operations scx200_gpio_fops = {
 	.owner   = THIS_MODULE,
-	.write   = scx200_gpio_write,
-	.read    = scx200_gpio_read,
+	.write   = nsc_gpio_write,
+	.read    = nsc_gpio_read,
 	.open    = scx200_gpio_open,
 	.release = scx200_gpio_release,
 };
diff -ruNp -X dontdiff -X exclude-diffs ax-10/include/linux/nsc_gpio.h ax-11/include/linux/nsc_gpio.h
--- ax-10/include/linux/nsc_gpio.h	2006-06-17 01:20:34.000000000 -0600
+++ ax-11/include/linux/nsc_gpio.h	2006-06-17 01:33:50.000000000 -0600
@@ -31,3 +31,8 @@ struct nsc_gpio_ops {
 	int	(*gpio_current)	(unsigned iminor);
 };
 
+extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
+			      size_t len, loff_t *ppos);
+
+extern ssize_t nsc_gpio_read(struct file *file, char __user *buf,
+			     size_t len, loff_t *ppos);

Re: [patch -mm 11/20] chardev: GPIO for SCx200 & PC-8736x: migrate file-ops to common module

[Andrew Morton]

On Sat, 17 Jun 2006 12:32:36 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> 11/20. patch.migrate-fops
> 
> Now that the read(), write() file-ops are dispatching gpio-ops via the
> vtable, they are generic, and can be moved 'verbatim' to the nsc_gpio
> common-support module.  After the move, various symbols are renamed to
> update 'scx200_' to 'nsc_', and headers are adjusted accordingly.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> 
> --- ax-10/drivers/char/scx200_gpio.c	2006-06-17 01:23:47.000000000 -0600
> +++ ax-11/drivers/char/scx200_gpio.c	2006-06-17 01:33:50.000000000 -0600
> @@ -37,6 +37,12 @@ MODULE_PARM_DESC(major, "Major device nu
>  
>  extern void scx200_gpio_dump(unsigned index);
>  
> +extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
> +			      size_t len, loff_t *ppos);
> +
> +extern ssize_t nsc_gpio_read(struct file *file, char __user *buf,
> +			     size_t len, loff_t *ppos);
> +

extern decls always always go into .h files.

> --- ax-10/include/linux/nsc_gpio.h	2006-06-17 01:20:34.000000000 -0600
> +++ ax-11/include/linux/nsc_gpio.h	2006-06-17 01:33:50.000000000 -0600
> @@ -31,3 +31,8 @@ struct nsc_gpio_ops {
>  	int	(*gpio_current)	(unsigned iminor);
>  };
>  
> +extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
> +			      size_t len, loff_t *ppos);
> +
> +extern ssize_t nsc_gpio_read(struct file *file, char __user *buf,
> +			     size_t len, loff_t *ppos);
> 

Yeah, like that ;)

[patch -mm 12/20] chardev: GPIO for SCx200 & PC-8736x: migrate gpio_dump to common module

[Jim Cromie]

12/20. patch.common-dump

Since the meaning of config-bits is the same for scx200 and pc8736x
_gpios, we can share a function to deliver this to user.  Since it is
called via the vtable, its also completely replaceable.
For now, we keep using printk...


Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.common-dump
 arch/i386/kernel/scx200.c  |   16 ----------------
 drivers/char/nsc_gpio.c    |   16 +++++++++++++++-
 drivers/char/scx200_gpio.c |    4 ++--
 3 files changed, 17 insertions(+), 19 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-11/arch/i386/kernel/scx200.c ax-12/arch/i386/kernel/scx200.c
--- ax-11/arch/i386/kernel/scx200.c	2006-06-17 01:17:11.000000000 -0600
+++ ax-12/arch/i386/kernel/scx200.c	2006-06-17 01:36:56.000000000 -0600
@@ -110,21 +110,6 @@ u32 scx200_gpio_configure(unsigned index
 	return config;
 }
 
-void scx200_gpio_dump(unsigned index)
-{
-        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 */
-}
-
 static int __init scx200_init(void)
 {
 	printk(KERN_INFO NAME ": NatSemi SCx200 Driver\n");
@@ -144,5 +129,4 @@ module_exit(scx200_cleanup);
 EXPORT_SYMBOL(scx200_gpio_base);
 EXPORT_SYMBOL(scx200_gpio_shadow);
 EXPORT_SYMBOL(scx200_gpio_configure);
-EXPORT_SYMBOL(scx200_gpio_dump);
 EXPORT_SYMBOL(scx200_cb_base);
diff -ruNp -X dontdiff -X exclude-diffs ax-11/drivers/char/nsc_gpio.c ax-12/drivers/char/nsc_gpio.c
--- ax-11/drivers/char/nsc_gpio.c	2006-06-17 01:33:50.000000000 -0600
+++ ax-12/drivers/char/nsc_gpio.c	2006-06-17 01:36:56.000000000 -0600
@@ -19,6 +19,19 @@
 
 #define NAME "nsc_gpio"
 
+void nsc_gpio_dump(unsigned index, u32 config)
+{
+	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 rise/fall edge */
+               (config & 64) ? "DEBOUNCE" : "");        /* debounce */
+}
+
 ssize_t nsc_gpio_write(struct file *file, const char __user *data,
 		       size_t len, loff_t *ppos)
 {
@@ -99,9 +112,10 @@ ssize_t nsc_gpio_read(struct file *file,
 	return 1;
 }
 
-/* common routines for both scx200_gpio and pc87360_gpio */
+/* common file-ops routines for both scx200_gpio and pc87360_gpio */
 EXPORT_SYMBOL(nsc_gpio_write);
 EXPORT_SYMBOL(nsc_gpio_read);
+EXPORT_SYMBOL(nsc_gpio_dump);
 
 static int __init nsc_gpio_init(void)
 {
diff -ruNp -X dontdiff -X exclude-diffs ax-11/drivers/char/scx200_gpio.c ax-12/drivers/char/scx200_gpio.c
--- ax-11/drivers/char/scx200_gpio.c	2006-06-17 01:33:50.000000000 -0600
+++ ax-12/drivers/char/scx200_gpio.c	2006-06-17 01:36:56.000000000 -0600
@@ -35,7 +35,7 @@ static int major = 0;		/* default to dyn
 module_param(major, int, 0);
 MODULE_PARM_DESC(major, "Major device number");
 
-extern void scx200_gpio_dump(unsigned index);
+extern void nsc_gpio_dump(unsigned index);
 
 extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
 			      size_t len, loff_t *ppos);
@@ -46,7 +46,7 @@ extern ssize_t nsc_gpio_read(struct file
 struct nsc_gpio_ops scx200_access = {
 	.owner		= THIS_MODULE,
 	.gpio_config	= scx200_gpio_configure,
-	.gpio_dump	= scx200_gpio_dump,
+	.gpio_dump	= nsc_gpio_dump,
 	.gpio_get	= scx200_gpio_get,
 	.gpio_set	= scx200_gpio_set,
 	.gpio_set_high	= scx200_gpio_set_high,

[patch -mm 08/20] chardev: GPIO for SCx200 & PC-8736x: add gpio-ops vtable

[Jim Cromie]

8/20. patch.access-vtable

Abstract the gpio operations into a new nsc_gpio_ops vtable.  

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.access-vtable
 drivers/char/scx200_gpio.c |   13 +++++++++++++
 include/linux/nsc_gpio.h   |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff -ruNp -X dontdiff -X exclude-diffs ax-7/drivers/char/scx200_gpio.c ax-8/drivers/char/scx200_gpio.c
--- ax-7/drivers/char/scx200_gpio.c	2006-06-17 01:13:26.000000000 -0600
+++ ax-8/drivers/char/scx200_gpio.c	2006-06-17 01:20:34.000000000 -0600
@@ -20,6 +20,7 @@
 #include <linux/cdev.h>
 
 #include <linux/scx200_gpio.h>
+#include <linux/nsc_gpio.h>
 
 #define NAME "scx200_gpio"
 #define DEVNAME NAME
@@ -36,6 +37,18 @@ MODULE_PARM_DESC(major, "Major device nu
 
 extern void scx200_gpio_dump(unsigned index);
 
+struct nsc_gpio_ops scx200_access = {
+	.owner		= THIS_MODULE,
+	.gpio_config	= scx200_gpio_configure,
+	.gpio_dump	= scx200_gpio_dump,
+	.gpio_get	= scx200_gpio_get,
+	.gpio_set	= scx200_gpio_set,
+	.gpio_set_high	= scx200_gpio_set_high,
+	.gpio_set_low	= scx200_gpio_set_low,
+	.gpio_change	= scx200_gpio_change,
+	.gpio_current	= scx200_gpio_current
+};
+
 static ssize_t scx200_gpio_write(struct file *file, const char __user *data,
 				 size_t len, loff_t *ppos)
 {
diff -ruNp -X dontdiff -X exclude-diffs ax-7/include/linux/nsc_gpio.h ax-8/include/linux/nsc_gpio.h
--- ax-7/include/linux/nsc_gpio.h	1969-12-31 17:00:00.000000000 -0700
+++ ax-8/include/linux/nsc_gpio.h	2006-06-17 01:20:34.000000000 -0600
@@ -0,0 +1,33 @@
+/**
+   nsc_gpio.c
+
+   National Semiconductor GPIO common access methods.
+
+   struct nsc_gpio_ops abstracts the low-level access
+   operations for the GPIO units on 2 NSC chip families; the GEODE
+   integrated CPU, and the PC-8736[03456] integrated PC-peripheral
+   chips.
+
+   The GPIO units on these chips have the same pin architecture, but
+   the access methods differ.  Thus, scx200_gpio and pc8736x_gpio
+   implement their own versions of these routines; and use the common
+   file-operations routines implemented in nsc_gpio module.
+
+   Copyright (c) 2005 Jim Cromie <jim.cromie@gmail.com>
+
+   NB: this work was tested on the Geode SC-1100 and PC-87366 chips.
+   NSC sold the GEODE line to AMD, and the PC-8736x line to Winbond.
+*/
+
+struct nsc_gpio_ops {
+	struct module*	owner;
+	u32	(*gpio_config)	(unsigned iminor, u32 mask, u32 bits);
+	void	(*gpio_dump)	(unsigned iminor);
+	int	(*gpio_get)	(unsigned iminor);
+	void	(*gpio_set)	(unsigned iminor, int state);
+	void	(*gpio_set_high)(unsigned iminor);
+	void	(*gpio_set_low)	(unsigned iminor);
+	void	(*gpio_change)	(unsigned iminor);
+	int	(*gpio_current)	(unsigned iminor);
+};
+

[patch -mm 13/20] chardev: GPIO for SCx200 & PC-8736x: add new pc8736x_gpio module

[Jim Cromie]

13/20. patch.add-pc8736x-gpio

Add the brand new pc8736x_gpio driver.  This is mostly based upon
scx200_gpio.c, but the platform_dev is treated separately, since its
fairly big too.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.add-pc8736x-gpio
 Makefile       |    2 
 nsc_gpio.c     |    1 
 pc8736x_gpio.c |  295 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 296 insertions(+), 2 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-12/drivers/char/Makefile ax-13/drivers/char/Makefile
--- ax-12/drivers/char/Makefile	2006-06-17 01:27:04.000000000 -0600
+++ ax-13/drivers/char/Makefile	2006-06-17 01:39:58.000000000 -0600
@@ -81,7 +81,7 @@ obj-$(CONFIG_COBALT_LCD)	+= lcd.o
 obj-$(CONFIG_PPDEV)		+= ppdev.o
 obj-$(CONFIG_NWBUTTON)		+= nwbutton.o
 obj-$(CONFIG_NWFLASH)		+= nwflash.o
-obj-$(CONFIG_SCx200_GPIO)	+= scx200_gpio.o nsc_gpio.o
+obj-$(CONFIG_SCx200_GPIO)	+= scx200_gpio.o nsc_gpio.o pc8736x_gpio.o
 obj-$(CONFIG_CS5535_GPIO)	+= cs5535_gpio.o
 obj-$(CONFIG_GPIO_VR41XX)	+= vr41xx_giu.o
 obj-$(CONFIG_TANBAC_TB0219)	+= tb0219.o
diff -ruNp -X dontdiff -X exclude-diffs ax-12/drivers/char/nsc_gpio.c ax-13/drivers/char/nsc_gpio.c
--- ax-12/drivers/char/nsc_gpio.c	2006-06-17 01:36:56.000000000 -0600
+++ ax-13/drivers/char/nsc_gpio.c	2006-06-17 01:39:58.000000000 -0600
@@ -77,7 +77,6 @@ ssize_t nsc_gpio_write(struct file *file
 			printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m);
 			amp->gpio_config(m, ~4, 0);
 			break;
-
 		case 'v':
 			/* View Current pin settings */
 			amp->gpio_dump(m);
diff -ruNp -X dontdiff -X exclude-diffs ax-12/drivers/char/pc8736x_gpio.c ax-13/drivers/char/pc8736x_gpio.c
--- ax-12/drivers/char/pc8736x_gpio.c	1969-12-31 17:00:00.000000000 -0700
+++ ax-13/drivers/char/pc8736x_gpio.c	2006-06-17 01:39:58.000000000 -0600
@@ -0,0 +1,295 @@
+/* linux/drivers/char/pc8736x_gpio.c
+
+   National Semiconductor PC8736x GPIO driver.  Allows a user space
+   process to play with the GPIO pins.
+
+   Copyright (c) 2005 Jim Cromie <jim.cromie@gmail.com>
+
+   adapted from linux/drivers/char/scx200_gpio.c
+   Copyright (c) 2001,2002 Christer Weinigel <wingel@nano-system.com>,
+*/
+
+#include <linux/config.h>
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/nsc_gpio.h>
+#include <asm/uaccess.h>
+#include <asm/io.h>
+
+#define NAME "pc8736x_gpio"
+
+MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
+MODULE_DESCRIPTION("NatSemi SCx200 GPIO Pin Driver");
+MODULE_LICENSE("GPL");
+
+static int major = 0;		/* default to dynamic major */
+module_param(major, int, 0);
+MODULE_PARM_DESC(major, "Major device number");
+
+static DEFINE_SPINLOCK(pc8736x_gpio_config_lock);
+static unsigned pc8736x_gpio_base;
+
+#define SIO_BASE1       0x2E	/* 1st command-reg to check */
+#define SIO_BASE2       0x4E	/* alt command-reg to check */
+#define SIO_BASE_OFFSET 0x20
+
+#define SIO_SID		0x20	/* SuperI/O ID Register */
+#define SIO_SID_VALUE	0xe9	/* Expected value in SuperI/O ID Register */
+
+#define SIO_CF1		0x21	/* chip config, bit0 is chip enable */
+
+#define SIO_UNIT_SEL	0x7	/* unit select reg */
+#define SIO_UNIT_ACT	0x30	/* unit enable */
+#define SIO_GPIO_UNIT	0x7	/* unit number of GPIO */
+#define SIO_VLM_UNIT	0x0D
+#define SIO_TMS_UNIT	0x0E
+
+/* config-space addrs to read/write each unit's runtime addr */
+#define SIO_BASE_HADDR		0x60
+#define SIO_BASE_LADDR		0x61
+
+/* GPIO config-space pin-control addresses */
+#define SIO_GPIO_PIN_SELECT	0xF0
+#define SIO_GPIO_PIN_CONFIG     0xF1
+#define SIO_GPIO_PIN_EVENT      0xF2
+
+static unsigned char superio_cmd = 0;
+static unsigned char selected_device = 0xFF;	/* bogus start val */
+
+/* GPIO port runtime access, functionality */
+static int port_offset[] = { 0, 4, 8, 10 };	/* non-uniform offsets ! */
+/* static int event_capable[] = { 1, 1, 0, 0 };   ports 2,3 are hobbled */
+
+#define PORT_OUT	0
+#define PORT_IN		1
+#define PORT_EVT_EN	2
+#define PORT_EVT_STST	3
+
+static inline void superio_outb(int addr, int val)
+{
+	outb_p(addr, superio_cmd);
+	outb_p(val, superio_cmd + 1);
+}
+
+static inline int superio_inb(int addr)
+{
+	outb_p(addr, superio_cmd);
+	return inb_p(superio_cmd + 1);
+}
+
+static int pc8736x_superio_present(void)
+     /* try the 2 possible values, read a hardware reg to verify */
+{
+	superio_cmd = SIO_BASE1;
+	if (superio_inb(SIO_SID) == SIO_SID_VALUE)
+		return superio_cmd;
+
+	superio_cmd = SIO_BASE2;
+	if (superio_inb(SIO_SID) == SIO_SID_VALUE)
+		return superio_cmd;
+
+	return 0;
+}
+
+static void device_select(unsigned devldn)
+{
+	superio_outb(SIO_UNIT_SEL, devldn);
+	selected_device = devldn;
+}
+
+static void select_pin(unsigned iminor)
+{
+	/* select GPIO port/pin from device minor number */
+	device_select(SIO_GPIO_UNIT);
+	superio_outb(SIO_GPIO_PIN_SELECT,
+		     ((iminor << 1) & 0xF0) | (iminor & 0x7));
+}
+
+static inline u32 pc8736x_gpio_configure_fn(unsigned index, u32 mask, u32 bits,
+					    u32 func_slct)
+{
+	u32 config, new_config;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pc8736x_gpio_config_lock, flags);
+
+	device_select(SIO_GPIO_UNIT);
+	select_pin(index);
+
+	/* read current config value */
+	config = superio_inb(func_slct);
+
+	/* set new config */
+	new_config = (config & mask) | bits;
+	superio_outb(func_slct, new_config);
+
+	spin_unlock_irqrestore(&pc8736x_gpio_config_lock, flags);
+
+	return config;
+}
+
+static u32 pc8736x_gpio_configure(unsigned index, u32 mask, u32 bits)
+{
+	return pc8736x_gpio_configure_fn(index, mask, bits,
+					 SIO_GPIO_PIN_CONFIG);
+}
+
+static int pc8736x_gpio_get(unsigned minor)
+{
+	int port, bit, val;
+
+	port = minor >> 3;
+	bit = minor & 7;
+	val = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
+	val >>= bit;
+	val &= 1;
+
+	printk(KERN_INFO NAME ": _gpio_get(%d from %x bit %d) == val %d\n",
+	       minor, pc8736x_gpio_base + port_offset[port] + PORT_IN, bit,
+	       val);
+
+	return val;
+}
+
+static void pc8736x_gpio_set(unsigned minor, int val)
+{
+	int port, bit, curval;
+
+	minor &= 0x1f;
+	port = minor >> 3;
+	bit = minor & 7;
+	curval = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+
+	printk(KERN_INFO NAME
+	       ": addr:%x cur:%x bit-pos:%d cur-bit:%x + new:%d -> bit-new:%d\n",
+	       pc8736x_gpio_base + port_offset[port] + PORT_OUT,
+	       curval, bit, (curval & ~(1 << bit)), val, (val << bit));
+
+	val = (curval & ~(1 << bit)) | (val << bit);
+
+	printk(KERN_INFO NAME ": gpio_set(minor:%d port:%d bit:%d"
+	       ") %2x -> %2x\n", minor, port, bit, curval, val);
+
+	outb_p(val, pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+
+	curval = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+	val = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
+
+	printk(KERN_INFO NAME ": wrote %x, read: %x\n", curval, val);
+}
+
+static void pc8736x_gpio_set_high(unsigned index)
+{
+	pc8736x_gpio_set(index, 1);
+}
+
+static void pc8736x_gpio_set_low(unsigned index)
+{
+	pc8736x_gpio_set(index, 0);
+}
+
+static int pc8736x_gpio_current(unsigned index)
+{
+	printk(KERN_WARNING NAME ": pc8736x_gpio_current unimplemented\n");
+	return 0;
+}
+
+static void pc8736x_gpio_change(unsigned index)
+{
+	pc8736x_gpio_set(index, !pc8736x_gpio_get(index));
+}
+
+extern void nsc_gpio_dump(unsigned iminor);
+
+static struct nsc_gpio_ops pc8736x_access = {
+	.owner		= THIS_MODULE,
+	.gpio_config	= pc8736x_gpio_configure,
+	.gpio_dump	= nsc_gpio_dump,
+	.gpio_get	= pc8736x_gpio_get,
+	.gpio_set	= pc8736x_gpio_set,
+	.gpio_set_high	= pc8736x_gpio_set_high,
+	.gpio_set_low	= pc8736x_gpio_set_low,
+	.gpio_change	= pc8736x_gpio_change,
+	.gpio_current	= pc8736x_gpio_current
+};
+
+static int pc8736x_gpio_open(struct inode *inode, struct file *file)
+{
+	unsigned m = iminor(inode);
+	file->private_data = &pc8736x_access;
+
+	printk(KERN_NOTICE NAME " open %d\n", m);
+
+	if (m > 63)
+		return -EINVAL;
+	return nonseekable_open(inode, file);
+}
+
+static struct file_operations pc8736x_gpio_fops = {
+	.owner = THIS_MODULE,
+	.open = pc8736x_gpio_open,
+	.write = nsc_gpio_write,
+	.read = nsc_gpio_read,
+};
+
+static int __init pc8736x_gpio_init(void)
+{
+	int r, rc;
+
+	printk(KERN_DEBUG NAME " initializing\n");
+
+	if (!pc8736x_superio_present()) {
+		printk(KERN_ERR NAME ": no device found\n");
+		return -ENODEV;
+	}
+
+	/* Verify that chip and it's GPIO unit are both enabled.
+	   My BIOS does this, so I take minimum action here
+	 */
+	rc = superio_inb(SIO_CF1);
+	if (!(rc & 0x01)) {
+		printk(KERN_ERR NAME ": device not enabled\n");
+		return -ENODEV;
+	}
+	device_select(SIO_GPIO_UNIT);
+	if (!superio_inb(SIO_UNIT_ACT)) {
+		printk(KERN_ERR NAME ": GPIO unit not enabled\n");
+		return -ENODEV;
+	}
+
+	/* read GPIO unit base address */
+	pc8736x_gpio_base = (superio_inb(SIO_BASE_HADDR) << 8
+			     | superio_inb(SIO_BASE_LADDR));
+
+	if (request_region(pc8736x_gpio_base, 16, NAME))
+		printk(KERN_INFO NAME ": GPIO ioport %x reserved\n",
+		       pc8736x_gpio_base);
+
+	r = register_chrdev(major, NAME, &pc8736x_gpio_fops);
+	if (r < 0) {
+		printk(KERN_ERR NAME ": unable to register character device\n");
+		return r;
+	}
+	if (!major) {
+		major = r;
+		printk(KERN_DEBUG NAME ": got dynamic major %d\n", major);
+	}
+
+	return 0;
+}
+
+static void __exit pc8736x_gpio_cleanup(void)
+{
+	printk(KERN_DEBUG NAME " cleanup\n");
+
+	release_region(pc8736x_gpio_base, 16);
+
+	unregister_chrdev(major, NAME);
+}
+
+module_init(pc8736x_gpio_init);
+module_exit(pc8736x_gpio_cleanup);

Re: [patch -mm 13/20] chardev: GPIO for SCx200 & PC-8736x: add new pc8736x_gpio module

[Andrew Morton]

On Sat, 17 Jun 2006 12:34:37 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> --- ax-12/drivers/char/pc8736x_gpio.c	1969-12-31 17:00:00.000000000 -0700
> +++ ax-13/drivers/char/pc8736x_gpio.c	2006-06-17 01:39:58.000000000 -0600
> @@ -0,0 +1,295 @@
> +/* linux/drivers/char/pc8736x_gpio.c
> +
> +   National Semiconductor PC8736x GPIO driver.  Allows a user space
> +   process to play with the GPIO pins.
> +
> +   Copyright (c) 2005 Jim Cromie <jim.cromie@gmail.com>
> +
> +   adapted from linux/drivers/char/scx200_gpio.c
> +   Copyright (c) 2001,2002 Christer Weinigel <wingel@nano-system.com>,
> +*/
> +
> +#include <linux/config.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/nsc_gpio.h>
> +#include <asm/uaccess.h>
> +#include <asm/io.h>

New code should use linux/io.h and linux/uaccess.h.  It won't matter in
this case, but you might pick up some goodies in the future.

> +#define NAME "pc8736x_gpio"
> +
> +MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
> +MODULE_DESCRIPTION("NatSemi SCx200 GPIO Pin Driver");
> +MODULE_LICENSE("GPL");
> +
> +static int major = 0;		/* default to dynamic major */

Unneeded initialisation.

> +static int pc8736x_superio_present(void)
> +     /* try the 2 possible values, read a hardware reg to verify */
> +{

weird comment placement.

> +extern void nsc_gpio_dump(unsigned iminor);

Goes in a .h file.

> +static int __init pc8736x_gpio_init(void)
> +{
> +	int r, rc;
> +
> +	printk(KERN_DEBUG NAME " initializing\n");
> +
> +	if (!pc8736x_superio_present()) {
> +		printk(KERN_ERR NAME ": no device found\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Verify that chip and it's GPIO unit are both enabled.
> +	   My BIOS does this, so I take minimum action here
> +	 */
> +	rc = superio_inb(SIO_CF1);
> +	if (!(rc & 0x01)) {
> +		printk(KERN_ERR NAME ": device not enabled\n");
> +		return -ENODEV;
> +	}
> +	device_select(SIO_GPIO_UNIT);
> +	if (!superio_inb(SIO_UNIT_ACT)) {
> +		printk(KERN_ERR NAME ": GPIO unit not enabled\n");
> +		return -ENODEV;
> +	}
> +
> +	/* read GPIO unit base address */
> +	pc8736x_gpio_base = (superio_inb(SIO_BASE_HADDR) << 8
> +			     | superio_inb(SIO_BASE_LADDR));
> +
> +	if (request_region(pc8736x_gpio_base, 16, NAME))
> +		printk(KERN_INFO NAME ": GPIO ioport %x reserved\n",
> +		       pc8736x_gpio_base);

Isn't this fatal?  If this IO region is in use by some other device, we
don't want this driver poking at it?


> +	r = register_chrdev(major, NAME, &pc8736x_gpio_fops);
> +	if (r < 0) {
> +		printk(KERN_ERR NAME ": unable to register character device\n");
> +		return r;

release_region()?

undo that device_select()?

> +	}
> +	if (!major) {
> +		major = r;
> +		printk(KERN_DEBUG NAME ": got dynamic major %d\n", major);
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit pc8736x_gpio_cleanup(void)
> +{
> +	printk(KERN_DEBUG NAME " cleanup\n");
> +
> +	release_region(pc8736x_gpio_base, 16);
> +
> +	unregister_chrdev(major, NAME);
> +}

No need to shut down any hardware here?

[patch -mm 14/20] chardev: GPIO for SCx200 & PC-8736x: add platform_device for use w dev_dbg

[Jim Cromie]

14/20. patch.pdev-pc8736x

Adds platform-device to (just introduced) driver, and uses it to
replace many printks with dev_dbg() etc.  This could trivially be
merged into previous patch, but this way matches better with the
corresponding patch that does the same change to scx200_gpio.


Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.pdev-pc8736x
 pc8736x_gpio.c |   76 +++++++++++++++++++++++++++++++++------------------------
 1 files changed, 45 insertions(+), 31 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-13/drivers/char/pc8736x_gpio.c ax-14/drivers/char/pc8736x_gpio.c
--- ax-13/drivers/char/pc8736x_gpio.c	2006-06-17 01:39:58.000000000 -0600
+++ ax-14/drivers/char/pc8736x_gpio.c	2006-06-17 01:42:57.000000000 -0600
@@ -17,13 +17,14 @@
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/nsc_gpio.h>
+#include <linux/platform_device.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
-#define NAME "pc8736x_gpio"
+#define DEVNAME "pc8736x_gpio"
 
 MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
-MODULE_DESCRIPTION("NatSemi SCx200 GPIO Pin Driver");
+MODULE_DESCRIPTION("NatSemi PC-8736x GPIO Pin Driver");
 MODULE_LICENSE("GPL");
 
 static int major = 0;		/* default to dynamic major */
@@ -42,6 +43,8 @@ static unsigned pc8736x_gpio_base;
 
 #define SIO_CF1		0x21	/* chip config, bit0 is chip enable */
 
+#define PC8736X_GPIO_SIZE	16
+
 #define SIO_UNIT_SEL	0x7	/* unit select reg */
 #define SIO_UNIT_ACT	0x30	/* unit enable */
 #define SIO_GPIO_UNIT	0x7	/* unit number of GPIO */
@@ -69,6 +72,8 @@ static int port_offset[] = { 0, 4, 8, 10
 #define PORT_EVT_EN	2
 #define PORT_EVT_STST	3
 
+static struct platform_device *pdev;  /* use in dev_*() */
+
 static inline void superio_outb(int addr, int val)
 {
 	outb_p(addr, superio_cmd);
@@ -148,9 +153,9 @@ static int pc8736x_gpio_get(unsigned min
 	val >>= bit;
 	val &= 1;
 
-	printk(KERN_INFO NAME ": _gpio_get(%d from %x bit %d) == val %d\n",
-	       minor, pc8736x_gpio_base + port_offset[port] + PORT_IN, bit,
-	       val);
+	dev_dbg(&pdev->dev, "_gpio_get(%d from %x bit %d) == val %d\n",
+		minor, pc8736x_gpio_base + port_offset[port] + PORT_IN, bit,
+		val);
 
 	return val;
 }
@@ -164,22 +169,21 @@ static void pc8736x_gpio_set(unsigned mi
 	bit = minor & 7;
 	curval = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
 
-	printk(KERN_INFO NAME
-	       ": addr:%x cur:%x bit-pos:%d cur-bit:%x + new:%d -> bit-new:%d\n",
-	       pc8736x_gpio_base + port_offset[port] + PORT_OUT,
-	       curval, bit, (curval & ~(1 << bit)), val, (val << bit));
+	dev_dbg(&pdev->dev, "addr:%x cur:%x bit-pos:%d cur-bit:%x + new:%d -> bit-new:%d\n",
+		pc8736x_gpio_base + port_offset[port] + PORT_OUT,
+		curval, bit, (curval & ~(1 << bit)), val, (val << bit));
 
 	val = (curval & ~(1 << bit)) | (val << bit);
 
-	printk(KERN_INFO NAME ": gpio_set(minor:%d port:%d bit:%d"
-	       ") %2x -> %2x\n", minor, port, bit, curval, val);
+	dev_dbg(&pdev->dev, "gpio_set(minor:%d port:%d bit:%d)"
+		" %2x -> %2x\n", minor, port, bit, curval, val);
 
 	outb_p(val, pc8736x_gpio_base + port_offset[port] + PORT_OUT);
 
 	curval = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
 	val = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
 
-	printk(KERN_INFO NAME ": wrote %x, read: %x\n", curval, val);
+	dev_dbg(&pdev->dev, "wrote %x, read: %x\n", curval, val);
 }
 
 static void pc8736x_gpio_set_high(unsigned index)
@@ -194,7 +198,7 @@ static void pc8736x_gpio_set_low(unsigne
 
 static int pc8736x_gpio_current(unsigned index)
 {
-	printk(KERN_WARNING NAME ": pc8736x_gpio_current unimplemented\n");
+	dev_warn(&pdev->dev, "pc8736x_gpio_current unimplemented\n");
 	return 0;
 }
 
@@ -222,7 +226,7 @@ static int pc8736x_gpio_open(struct inod
 	unsigned m = iminor(inode);
 	file->private_data = &pc8736x_access;
 
-	printk(KERN_NOTICE NAME " open %d\n", m);
+	dev_dbg(&pdev->dev, "open %d\n", m);
 
 	if (m > 63)
 		return -EINVAL;
@@ -230,20 +234,30 @@ static int pc8736x_gpio_open(struct inod
 }
 
 static struct file_operations pc8736x_gpio_fops = {
-	.owner = THIS_MODULE,
-	.open = pc8736x_gpio_open,
-	.write = nsc_gpio_write,
-	.read = nsc_gpio_read,
+	.owner	= THIS_MODULE,
+	.open	= pc8736x_gpio_open,
+	.write	= nsc_gpio_write,
+	.read	= nsc_gpio_read,
 };
 
 static int __init pc8736x_gpio_init(void)
 {
 	int r, rc;
 
-	printk(KERN_DEBUG NAME " initializing\n");
+        pdev = platform_device_alloc(DEVNAME, 0);
+        if (!pdev)
+                return -ENOMEM;
+
+        rc = platform_device_add(pdev);
+        if (rc) {
+                platform_device_put(pdev);
+                return -ENODEV;
+        }
+        dev_info(&pdev->dev, "NatSemi pc8736x GPIO Driver Initializing\n");
 
 	if (!pc8736x_superio_present()) {
-		printk(KERN_ERR NAME ": no device found\n");
+		dev_err(&pdev->dev, "no device found\n");
+                platform_device_put(pdev);
 		return -ENODEV;
 	}
 
@@ -252,31 +266,31 @@ static int __init pc8736x_gpio_init(void
 	 */
 	rc = superio_inb(SIO_CF1);
 	if (!(rc & 0x01)) {
-		printk(KERN_ERR NAME ": device not enabled\n");
+		dev_err(&pdev->dev, "device not enabled\n");
 		return -ENODEV;
 	}
 	device_select(SIO_GPIO_UNIT);
 	if (!superio_inb(SIO_UNIT_ACT)) {
-		printk(KERN_ERR NAME ": GPIO unit not enabled\n");
+		dev_err(&pdev->dev, "GPIO unit not enabled\n");
 		return -ENODEV;
 	}
 
-	/* read GPIO unit base address */
+	/* read the GPIO unit base addr that chip responds to */
 	pc8736x_gpio_base = (superio_inb(SIO_BASE_HADDR) << 8
 			     | superio_inb(SIO_BASE_LADDR));
 
-	if (request_region(pc8736x_gpio_base, 16, NAME))
-		printk(KERN_INFO NAME ": GPIO ioport %x reserved\n",
-		       pc8736x_gpio_base);
+	if (request_region(pc8736x_gpio_base, 16, DEVNAME))
+		dev_info(&pdev->dev, "GPIO ioport %x reserved\n",
+			 pc8736x_gpio_base);
 
-	r = register_chrdev(major, NAME, &pc8736x_gpio_fops);
+	r = register_chrdev(major, DEVNAME, &pc8736x_gpio_fops);
 	if (r < 0) {
-		printk(KERN_ERR NAME ": unable to register character device\n");
+		dev_err(&pdev->dev, "unable to register character device\n");
 		return r;
 	}
 	if (!major) {
 		major = r;
-		printk(KERN_DEBUG NAME ": got dynamic major %d\n", major);
+		dev_dbg(&pdev->dev, "got dynamic major %d\n", major);
 	}
 
 	return 0;
@@ -284,11 +298,11 @@ static int __init pc8736x_gpio_init(void
 
 static void __exit pc8736x_gpio_cleanup(void)
 {
-	printk(KERN_DEBUG NAME " cleanup\n");
+	dev_dbg(&pdev->dev, " cleanup\n");
 
 	release_region(pc8736x_gpio_base, 16);
 
-	unregister_chrdev(major, NAME);
+	unregister_chrdev(major, DEVNAME);
 }
 
 module_init(pc8736x_gpio_init);

Re: [patch -mm 14/20] chardev: GPIO for SCx200 & PC-8736x: add platform_device for use w dev_dbg

[Andrew Morton]

On Sat, 17 Jun 2006 12:35:43 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> 14/20. patch.pdev-pc8736x
> 
> Adds platform-device to (just introduced) driver, and uses it to
> replace many printks with dev_dbg() etc.  This could trivially be
> merged into previous patch, but this way matches better with the
> corresponding patch that does the same change to scx200_gpio.
> 
> 
>  static int __init pc8736x_gpio_init(void)
>  {
>  	int r, rc;
>  
> -	printk(KERN_DEBUG NAME " initializing\n");
> +        pdev = platform_device_alloc(DEVNAME, 0);
> +        if (!pdev)
> +                return -ENOMEM;
> +
> +        rc = platform_device_add(pdev);
> +        if (rc) {
> +                platform_device_put(pdev);
> +                return -ENODEV;
> +        }
> +        dev_info(&pdev->dev, "NatSemi pc8736x GPIO Driver Initializing\n");

The whitespace is all screwed up here.  Use hard tabs.

It's simplest if I edit all the diffs.

<does that>

[patch -mm 15/20] chardev: GPIO for SCx200 & PC-8736x: use dev_dbg in common module

[Jim Cromie]

15/20. patch.devdbg-nscgpio

Use of dev_dbg() and friends is considered good practice.  dev_dbg()
needs a struct device *devp, but nsc_gpio is only a helper module, so
it doesnt have/need its own.  To provide devp to the user-modules
(scx200 & pc8736x _gpio), we add it to the vtable, and set it during
init.

Also squeeze nsc_gpio_dump()'s format a little.

[  199.259879]  pc8736x_gpio.0: io09: 0x0044 TS OD PUE  EDGE LO DEBOUNCE


Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.devdbg-nscgpio
 drivers/char/nsc_gpio.c     |   45 ++++++++++++++++++++++++--------------------
 drivers/char/pc8736x_gpio.c |    3 +-
 drivers/char/scx200_gpio.c  |   11 ++--------
 include/linux/nsc_gpio.h    |    6 ++++-
 4 files changed, 35 insertions(+), 30 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-14/drivers/char/nsc_gpio.c ax-15/drivers/char/nsc_gpio.c
--- ax-14/drivers/char/nsc_gpio.c	2006-06-17 01:39:58.000000000 -0600
+++ ax-15/drivers/char/nsc_gpio.c	2006-06-17 01:45:49.000000000 -0600
@@ -14,22 +14,27 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/nsc_gpio.h>
+#include <linux/platform_device.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
 #define NAME "nsc_gpio"
 
-void nsc_gpio_dump(unsigned index, u32 config)
+void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned index)
 {
-	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 rise/fall edge */
-               (config & 64) ? "DEBOUNCE" : "");        /* debounce */
+	/* retrieve current config w/o changing it */
+	u32 config = amp->gpio_config(index, ~0, 0);
+
+	/* user requested via 'v' command, so its INFO */
+	dev_info(amp->dev, "io%02u: 0x%04x %s %s %s %s %s %s %s\n",
+		 index, 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 rise/fall edge */
+		 (config & 64) ? "DEBOUNCE" : "");        /* debounce */
 }
 
 ssize_t nsc_gpio_write(struct file *file, const char __user *data,
@@ -37,6 +42,7 @@ ssize_t nsc_gpio_write(struct file *file
 {
 	unsigned m = iminor(file->f_dentry->d_inode);
 	struct nsc_gpio_ops *amp = file->private_data;
+	struct device *dev = amp->dev;
 	size_t i;
 	int err = 0;
 
@@ -52,42 +58,41 @@ ssize_t nsc_gpio_write(struct file *file
 			amp->gpio_set(m, 1);
 			break;
 		case 'O':
-			printk(KERN_INFO NAME ": GPIO%d output enabled\n", m);
+			dev_dbg(dev, "GPIO%d output enabled\n", m);
 			amp->gpio_config(m, ~1, 1);
 			break;
 		case 'o':
-			printk(KERN_INFO NAME ": GPIO%d output disabled\n", m);
+			dev_dbg(dev, "GPIO%d output disabled\n", m);
 			amp->gpio_config(m, ~1, 0);
 			break;
 		case 'T':
-			printk(KERN_INFO NAME ": GPIO%d output is push pull\n",
+			dev_dbg(dev, "GPIO%d output is push pull\n",
 			       m);
 			amp->gpio_config(m, ~2, 2);
 			break;
 		case 't':
-			printk(KERN_INFO NAME ": GPIO%d output is open drain\n",
+			dev_dbg(dev, "GPIO%d output is open drain\n",
 			       m);
 			amp->gpio_config(m, ~2, 0);
 			break;
 		case 'P':
-			printk(KERN_INFO NAME ": GPIO%d pull up enabled\n", m);
+			dev_dbg(dev, "GPIO%d pull up enabled\n", m);
 			amp->gpio_config(m, ~4, 4);
 			break;
 		case 'p':
-			printk(KERN_INFO NAME ": GPIO%d pull up disabled\n", m);
+			dev_dbg(dev, "GPIO%d pull up disabled\n", m);
 			amp->gpio_config(m, ~4, 0);
 			break;
 		case 'v':
 			/* View Current pin settings */
-			amp->gpio_dump(m);
+			amp->gpio_dump(amp, 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);
+			dev_err(dev, "io%2d bad setting: chr<0x%2x>\n",
+				m, (int)c);
 			err++;
 		}
 	}
diff -ruNp -X dontdiff -X exclude-diffs ax-14/drivers/char/pc8736x_gpio.c ax-15/drivers/char/pc8736x_gpio.c
--- ax-14/drivers/char/pc8736x_gpio.c	2006-06-17 01:42:57.000000000 -0600
+++ ax-15/drivers/char/pc8736x_gpio.c	2006-06-17 01:45:49.000000000 -0600
@@ -207,7 +207,7 @@ static void pc8736x_gpio_change(unsigned
 	pc8736x_gpio_set(index, !pc8736x_gpio_get(index));
 }
 
-extern void nsc_gpio_dump(unsigned iminor);
+extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned iminor);
 
 static struct nsc_gpio_ops pc8736x_access = {
 	.owner		= THIS_MODULE,
@@ -260,6 +260,7 @@ static int __init pc8736x_gpio_init(void
                 platform_device_put(pdev);
 		return -ENODEV;
 	}
+	pc8736x_access.dev = &pdev->dev;
 
 	/* Verify that chip and it's GPIO unit are both enabled.
 	   My BIOS does this, so I take minimum action here
diff -ruNp -X dontdiff -X exclude-diffs ax-14/drivers/char/scx200_gpio.c ax-15/drivers/char/scx200_gpio.c
--- ax-14/drivers/char/scx200_gpio.c	2006-06-17 01:36:56.000000000 -0600
+++ ax-15/drivers/char/scx200_gpio.c	2006-06-17 01:45:49.000000000 -0600
@@ -35,14 +35,6 @@ static int major = 0;		/* default to dyn
 module_param(major, int, 0);
 MODULE_PARM_DESC(major, "Major device number");
 
-extern void nsc_gpio_dump(unsigned index);
-
-extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
-			      size_t len, loff_t *ppos);
-
-extern ssize_t nsc_gpio_read(struct file *file, char __user *buf,
-			     size_t len, loff_t *ppos);
-
 struct nsc_gpio_ops scx200_access = {
 	.owner		= THIS_MODULE,
 	.gpio_config	= scx200_gpio_configure,
@@ -101,6 +93,9 @@ static int __init scx200_gpio_init(void)
 	if (rc)
 		goto undo_platform_device_add;
 
+	/* nsc_gpio uses dev_dbg(), so needs this */
+	scx200_access.dev = &pdev->dev;
+
 	if (major)
 		rc = register_chrdev_region(dev, num_devs, "scx200_gpio");
 	else {
diff -ruNp -X dontdiff -X exclude-diffs ax-14/include/linux/nsc_gpio.h ax-15/include/linux/nsc_gpio.h
--- ax-14/include/linux/nsc_gpio.h	2006-06-17 01:33:50.000000000 -0600
+++ ax-15/include/linux/nsc_gpio.h	2006-06-17 01:45:49.000000000 -0600
@@ -22,13 +22,14 @@
 struct nsc_gpio_ops {
 	struct module*	owner;
 	u32	(*gpio_config)	(unsigned iminor, u32 mask, u32 bits);
-	void	(*gpio_dump)	(unsigned iminor);
+	void	(*gpio_dump)	(struct nsc_gpio_ops *amp, unsigned iminor);
 	int	(*gpio_get)	(unsigned iminor);
 	void	(*gpio_set)	(unsigned iminor, int state);
 	void	(*gpio_set_high)(unsigned iminor);
 	void	(*gpio_set_low)	(unsigned iminor);
 	void	(*gpio_change)	(unsigned iminor);
 	int	(*gpio_current)	(unsigned iminor);
+	struct device*	dev;	/* for dev_dbg() support, set in init  */
 };
 
 extern ssize_t nsc_gpio_write(struct file *file, const char __user *data,
@@ -36,3 +37,6 @@ extern ssize_t nsc_gpio_write(struct fil
 
 extern ssize_t nsc_gpio_read(struct file *file, char __user *buf,
 			     size_t len, loff_t *ppos);
+
+extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned index);
+

Re: [patch -mm 15/20] chardev: GPIO for SCx200 & PC-8736x: use dev_dbg in common module

[Andrew Morton]

On Sat, 17 Jun 2006 12:36:19 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> 15/20. patch.devdbg-nscgpio
> 
> Use of dev_dbg() and friends is considered good practice.  dev_dbg()
> needs a struct device *devp, but nsc_gpio is only a helper module, so
> it doesnt have/need its own.  To provide devp to the user-modules
> (scx200 & pc8736x _gpio), we add it to the vtable, and set it during
> init.
> 
> ...
>
> --- ax-14/drivers/char/pc8736x_gpio.c	2006-06-17 01:42:57.000000000 -0600
> +++ ax-15/drivers/char/pc8736x_gpio.c	2006-06-17 01:45:49.000000000 -0600
> @@ -207,7 +207,7 @@ static void pc8736x_gpio_change(unsigned
>  	pc8736x_gpio_set(index, !pc8736x_gpio_get(index));
>  }
>  
> -extern void nsc_gpio_dump(unsigned iminor);
> +extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned iminor);

Wants to be in a header file.

> +
> +extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned index);
> +

And there it is.  Odd.

[patch -mm 16/20] chardev: GPIO for SCx200 & PC-8736x: fix gpio_current, use shadow regs

[Jim Cromie]

16/20. patch.shadow-current

Add a working gpio_current() to pc8736x_gpio.c (the previous
implementation just threw a dev_warn), and fix gpio_change() to use
gpio_current() rather than the incorrect (and temporary) gpio_get().
Initialize shadow-regs so this all works.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.shadow-current
 pc8736x_gpio.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-15/drivers/char/pc8736x_gpio.c ax-16/drivers/char/pc8736x_gpio.c
--- ax-15/drivers/char/pc8736x_gpio.c	2006-06-17 01:45:49.000000000 -0600
+++ ax-16/drivers/char/pc8736x_gpio.c	2006-06-17 01:48:50.000000000 -0600
@@ -33,6 +33,7 @@ MODULE_PARM_DESC(major, "Major device nu
 
 static DEFINE_SPINLOCK(pc8736x_gpio_config_lock);
 static unsigned pc8736x_gpio_base;
+static u8 pc8736x_gpio_shadow[4];
 
 #define SIO_BASE1       0x2E	/* 1st command-reg to check */
 #define SIO_BASE2       0x4E	/* alt command-reg to check */
@@ -184,6 +185,7 @@ static void pc8736x_gpio_set(unsigned mi
 	val = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
 
 	dev_dbg(&pdev->dev, "wrote %x, read: %x\n", curval, val);
+	pc8736x_gpio_shadow[port] = val;
 }
 
 static void pc8736x_gpio_set_high(unsigned index)
@@ -196,15 +198,18 @@ static void pc8736x_gpio_set_low(unsigne
 	pc8736x_gpio_set(index, 0);
 }
 
-static int pc8736x_gpio_current(unsigned index)
+static int pc8736x_gpio_current(unsigned minor)
 {
-	dev_warn(&pdev->dev, "pc8736x_gpio_current unimplemented\n");
-	return 0;
+	int port, bit;
+	minor &= 0x1f;
+	port = minor >> 3;
+	bit = minor & 7;
+	return pc8736x_gpio_shadow[port] >> bit & 0x01;
 }
 
 static void pc8736x_gpio_change(unsigned index)
 {
-	pc8736x_gpio_set(index, !pc8736x_gpio_get(index));
+	pc8736x_gpio_set(index, !pc8736x_gpio_current(index));
 }
 
 extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned iminor);
@@ -240,6 +245,18 @@ static struct file_operations pc8736x_gp
 	.read	= nsc_gpio_read,
 };
 
+static void __init pc8736x_init_shadow(void)
+{
+	int port;
+
+	/* read the current values driven on the GPIO signals */
+	for (port = 0; port < 4; ++port)
+		pc8736x_gpio_shadow[port]
+		    = inb_p(pc8736x_gpio_base + port_offset[port]
+			    + PORT_OUT);
+
+}
+
 static int __init pc8736x_gpio_init(void)
 {
 	int r, rc;
@@ -306,5 +323,7 @@ static void __exit pc8736x_gpio_cleanup(
 	unregister_chrdev(major, DEVNAME);
 }
 
+EXPORT_SYMBOL(pc8736x_access);
+
 module_init(pc8736x_gpio_init);
 module_exit(pc8736x_gpio_cleanup);

Re: [patch -mm 16/20] chardev: GPIO for SCx200 & PC-8736x: fix gpio_current, use shadow regs

[Andrew Morton]

On Sat, 17 Jun 2006 12:37:04 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> +	return pc8736x_gpio_shadow[port] >> bit & 0x01;

<brain hurts>

You must know your C predence well...

[patch -mm 17/20] chardev: GPIO for SCx200 & PC-8736x: replace spinlocks w mutexes

[Jim Cromie]

17/20. patch.mutexes

Replace spinlocks guarding gpio config ops with mutexes.  This is a
me-too patch, and is justifiable insofar as mutexes have stricter
semantics and better debugging support, so are preferred where they
are applicable.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.mutexes
 arch/i386/kernel/scx200.c   |    8 ++++----
 drivers/char/pc8736x_gpio.c |    8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-16/arch/i386/kernel/scx200.c ax-17/arch/i386/kernel/scx200.c
--- ax-16/arch/i386/kernel/scx200.c	2006-06-17 01:36:56.000000000 -0600
+++ ax-17/arch/i386/kernel/scx200.c	2006-06-17 01:51:49.000000000 -0600
@@ -9,6 +9,7 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/mutex.h>
 #include <linux/pci.h>
 
 #include <linux/scx200.h>
@@ -45,7 +46,7 @@ static struct pci_driver scx200_pci_driv
 	.probe = scx200_probe,
 };
 
-static DEFINE_SPINLOCK(scx200_gpio_config_lock);
+DEFINE_MUTEX(scx200_gpio_config_lock);
 
 static void __devinit scx200_init_shadow(void)
 {
@@ -95,9 +96,8 @@ static int __devinit scx200_probe(struct
 u32 scx200_gpio_configure(unsigned index, u32 mask, u32 bits)
 {
 	u32 config, new_config;
-	unsigned long flags;
 
-	spin_lock_irqsave(&scx200_gpio_config_lock, flags);
+	mutex_lock(&scx200_gpio_config_lock);
 
 	outl(index, scx200_gpio_base + 0x20);
 	config = inl(scx200_gpio_base + 0x24);
@@ -105,7 +105,7 @@ u32 scx200_gpio_configure(unsigned index
 	new_config = (config & mask) | bits;
 	outl(new_config, scx200_gpio_base + 0x24);
 
-	spin_unlock_irqrestore(&scx200_gpio_config_lock, flags);
+	mutex_unlock(&scx200_gpio_config_lock);
 
 	return config;
 }
diff -ruNp -X dontdiff -X exclude-diffs ax-16/drivers/char/pc8736x_gpio.c ax-17/drivers/char/pc8736x_gpio.c
--- ax-16/drivers/char/pc8736x_gpio.c	2006-06-17 01:48:50.000000000 -0600
+++ ax-17/drivers/char/pc8736x_gpio.c	2006-06-17 01:51:49.000000000 -0600
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
+#include <linux/mutex.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
@@ -31,7 +32,7 @@ static int major = 0;		/* default to dyn
 module_param(major, int, 0);
 MODULE_PARM_DESC(major, "Major device number");
 
-static DEFINE_SPINLOCK(pc8736x_gpio_config_lock);
+DEFINE_MUTEX(pc8736x_gpio_config_lock);
 static unsigned pc8736x_gpio_base;
 static u8 pc8736x_gpio_shadow[4];
 
@@ -119,9 +120,8 @@ static inline u32 pc8736x_gpio_configure
 					    u32 func_slct)
 {
 	u32 config, new_config;
-	unsigned long flags;
 
-	spin_lock_irqsave(&pc8736x_gpio_config_lock, flags);
+	mutex_lock(&pc8736x_gpio_config_lock);
 
 	device_select(SIO_GPIO_UNIT);
 	select_pin(index);
@@ -133,7 +133,7 @@ static inline u32 pc8736x_gpio_configure
 	new_config = (config & mask) | bits;
 	superio_outb(func_slct, new_config);
 
-	spin_unlock_irqrestore(&pc8736x_gpio_config_lock, flags);
+	mutex_unlock(&pc8736x_gpio_config_lock);
 
 	return config;
 }

Re: [patch -mm 17/20] chardev: GPIO for SCx200 & PC-8736x: replace spinlocks w mutexes

[Andrew Morton]

On Sat, 17 Jun 2006 12:38:17 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> 17/20. patch.mutexes
> 
> Replace spinlocks guarding gpio config ops with mutexes.  This is a
> me-too patch, and is justifiable insofar as mutexes have stricter
> semantics and better debugging support, so are preferred where they
> are applicable.
> 

OK.  I trust this was all tester with all kernel debug options turned on?

Once it's in -mm you might hare to try out the lockdep checker too.


> -static DEFINE_SPINLOCK(scx200_gpio_config_lock);
> +DEFINE_MUTEX(scx200_gpio_config_lock);

But it doesn't need global scope.

> -static DEFINE_SPINLOCK(pc8736x_gpio_config_lock);
> +DEFINE_MUTEX(pc8736x_gpio_config_lock);

Nor does this?

[patch -mm 18/20] chardev: GPIO for SCx200 & PC-8736x: display pin values in/out in gpio_dump

[Jim Cromie]

18/20. patch.viewpins-values

Add current pin settings to gpio_dump() output. This adds the last
'word' to the syslog lines, which displays the input and output values
that the pin is set to.

  pc8736x_gpio.0: io00: 0x0044 TS OD PUE  EDGE LO DEBOUNCE        io:1/1

The 2 values may differ for a number of reasons:
1- the pin output circuitry is diaabled, (as the above 'TS' indicates)
2- it needs a pullup resistor to drive the attached circuit,
3- the external circuit needs a pullup so the open-drain has something
   to pull-down
4- the pin is wired to Vcc or Ground

It might be appropriate to add a WARN for 2,3,4, since they could
damage the chip and/or circuit, esp if misconfig goes unnoticed.


Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.viewpins-values
 nsc_gpio.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-17/drivers/char/nsc_gpio.c ax-18/drivers/char/nsc_gpio.c
--- ax-17/drivers/char/nsc_gpio.c	2006-06-17 01:45:49.000000000 -0600
+++ ax-18/drivers/char/nsc_gpio.c	2006-06-17 01:54:42.000000000 -0600
@@ -26,7 +26,7 @@ void nsc_gpio_dump(struct nsc_gpio_ops *
 	u32 config = amp->gpio_config(index, ~0, 0);
 
 	/* user requested via 'v' command, so its INFO */
-	dev_info(amp->dev, "io%02u: 0x%04x %s %s %s %s %s %s %s\n",
+	dev_info(amp->dev, "io%02u: 0x%04x %s %s %s %s %s %s %s\tio:%d/%d\n",
 		 index, config,
 		 (config & 1) ? "OE" : "TS",      /* output-enabled/tristate */
 		 (config & 2) ? "PP" : "OD",      /* push pull / open drain */
@@ -34,7 +34,9 @@ void nsc_gpio_dump(struct nsc_gpio_ops *
 		 (config & 8) ? "LOCKED" : "",    /* locked / unlocked */
 		 (config & 16) ? "LEVEL" : "EDGE",/* level/edge input */
 		 (config & 32) ? "HI" : "LO",     /* trigger on rise/fall edge */
-		 (config & 64) ? "DEBOUNCE" : "");        /* debounce */
+		 (config & 64) ? "DEBOUNCE" : "", /* debounce */
+
+		 amp->gpio_get(index), amp->gpio_current(index));
 }
 
 ssize_t nsc_gpio_write(struct file *file, const char __user *data,

[patch -mm 19/20] chardev: GPIO for SCx200 & PC-8736x: add proper Kconfig, Makefile entries

[Jim Cromie]

19/20. patch.kconfig

Replace the temp makefile hacks with proper CONFIG entries, which are
also added to Kconfig.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

---

diffstat gpio-scx/patch.kconfig
 Kconfig  |   23 +++++++++++++++++++++++
 Makefile |    4 +++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff -ruNp -X dontdiff -X exclude-diffs ax-18/drivers/char/Kconfig ax-19/drivers/char/Kconfig
--- ax-18/drivers/char/Kconfig	2006-06-06 17:15:36.000000000 -0600
+++ ax-19/drivers/char/Kconfig	2006-06-17 01:57:21.000000000 -0600
@@ -934,12 +934,35 @@ config MWAVE
 config SCx200_GPIO
 	tristate "NatSemi SCx200 GPIO Support"
 	depends on SCx200
+	select NSC_GPIO
 	help
 	  Give userspace access to the GPIO pins on the National
 	  Semiconductor SCx200 processors.
 
 	  If compiled as a module, it will be called scx200_gpio.
 
+config PC8736x_GPIO
+	tristate "NatSemi PC8736x GPIO Support"
+	depends on X86
+	default SCx200_GPIO	# mostly N
+	select NSC_GPIO		# needed for support routines
+	help
+	  Give userspace access to the GPIO pins on the National
+	  Semiconductor PC-8736x (x=[03456]) SuperIO chip.  The chip
+	  has multiple functional units, inc several managed by
+	  hwmon/pc87360 driver.  Tested with PC-87366
+
+	  If compiled as a module, it will be called pc8736x_gpio.
+
+config NSC_GPIO
+	tristate "NatSemi Base GPIO Support"
+	# selected by SCx200_GPIO and PC8736x_GPIO
+	# what about 2 selectors differing: m != y
+	help
+	  Common support used (and needed) by scx200_gpio and
+	  pc8736x_gpio drivers.  If those drivers are built as
+	  modules, this one will be too, named nsc_gpio
+
 config CS5535_GPIO
 	tristate "AMD CS5535/CS5536 GPIO (Geode Companion Device)"
 	depends on X86_32
diff -ruNp -X dontdiff -X exclude-diffs ax-18/drivers/char/Makefile ax-19/drivers/char/Makefile
--- ax-18/drivers/char/Makefile	2006-06-17 01:39:58.000000000 -0600
+++ ax-19/drivers/char/Makefile	2006-06-17 01:57:21.000000000 -0600
@@ -81,7 +81,9 @@ obj-$(CONFIG_COBALT_LCD)	+= lcd.o
 obj-$(CONFIG_PPDEV)		+= ppdev.o
 obj-$(CONFIG_NWBUTTON)		+= nwbutton.o
 obj-$(CONFIG_NWFLASH)		+= nwflash.o
-obj-$(CONFIG_SCx200_GPIO)	+= scx200_gpio.o nsc_gpio.o pc8736x_gpio.o
+obj-$(CONFIG_SCx200_GPIO)	+= scx200_gpio.o
+obj-$(CONFIG_PC8736x_GPIO)	+= pc8736x_gpio.o
+obj-$(CONFIG_NSC_GPIO)		+= nsc_gpio.o
 obj-$(CONFIG_CS5535_GPIO)	+= cs5535_gpio.o
 obj-$(CONFIG_GPIO_VR41XX)	+= vr41xx_giu.o
 obj-$(CONFIG_TANBAC_TB0219)	+= tb0219.o

[patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Jim Cromie]

Ok, 

heres the brand-spanking-new proto-sysfs-gpio interface,
preceded by some pseudo/proto-Documentation.



[jimc@harpo gpio-scx]$ more gpio-design-sysfs

Sysfs GPIO representation of Hardware (v0.3)
(v0.1 went to lm-sensors ML)
(v0.2 reviewed privately by a microcontroller jockey)

We need a standard rep for GPIO in sysfs, so heres a strawman.
Strike a match, lets have a campfire!

Essentially, this seeks to describe the directory of
'device-attribute-files' that are populated by a driver

forex:

soekris:/sys/bus/platform/devices/pc8736x_gpio.0# ls
bit_0.0_debounced       bit_1.2_totem           bit_2.5_pullup_enabled
bit_0.0_locked          bit_1.3_debounced       bit_2.5_totem
bit_0.0_output_enabled  bit_1.3_locked          bit_2.6_debounced
bit_0.0_pullup_enabled  bit_1.3_output_enabled  bit_2.6_locked
bit_0.0_totem           bit_1.3_pullup_enabled  bit_2.6_output_enabled
bit_0.1_debounced       bit_1.3_totem           bit_2.6_pullup_enabled
bit_0.1_locked          bit_1.4_debounced       bit_2.6_totem
bit_0.1_output_enabled  bit_1.4_locked          bit_2.7_debounced
bit_0.1_pullup_enabled  bit_1.4_output_enabled  bit_2.7_locked
bit_0.1_totem           bit_1.4_pullup_enabled  bit_2.7_output_enabled
bit_0.2_debounced       bit_1.4_totem           bit_2.7_pullup_enabled
bit_0.2_locked          bit_1.5_debounced       bit_2.7_totem
...

(Ive now seen *1.5* GPIO architectures, so please test this writeup
mentally against your GPIO experience).


Basic device-file Naming Convention.

I havent seen this stated anywhere at an 'all-of-sysfs-level' and I
think its true/valid (and so test this here - CMIIW).  If Im correct,
please suggest the optimal Doc/* file to contain this info.

All device-attr-files are named as <prefix>_<id>_<suffix>

in LM-sensors:
- prefix        sensor-type: in(volts), temp, fan, etc. (no trailing '_')
- id            usually single integer
- suffix        the sensor attribute in question.





GPIO-sysfs Prefix Names.

Basically, GPIO hardware design appears to have 2 top-level factors;
pin features, and pin-to-port grouping.  These are mapped onto
filename prefixes & suffixes.

All GPIOs (Ive seen) are organized as 1+ ports of 8-32 bits.  The
bits' attributes are addressable individually, but are also accessible
as a group via the port_* files.  If you change a bit-attribute, that
change will also exhibit in the port attr too.

IOW, we have bit_*, port_*.  They are interconnected at the hardware
level, and (I think) there is no need for inter-locks between the
sysfs handlers for bit_ and port_ (except for shadow regs, but I
digress)

In fact, it might be nice to have the option of not creating the bit_*
sysfs-device-files.  For apps where user-code is doing its own
bit-masking, the kernel could avoid some unused overheads.  OTOH, this
might be silly, premature, overoptimization.  This would be controlled
- assuming its worthwhile - by a modparam; 'nobits' or 'portsonly'


GPIO Architectures

GPIO pins have lots of hardware / architectural / naming-convention
variations, which makes this harder.  My simplifying assumption is
that drivers should reflect the hardware capabilities directly (or
very nearly so), and push the abstraction to user-space (at least in
part).  Obviously this needs

Drivers should create sysfs 'files' only for attributes that are
pertinent for the hardware being driven.  Ths way, the absense or
presense of files communicates functionality, as does their
readonlyness.  (these 'behaviors' may be different than lm-sensors)

Forex:
if a pin is input  only, it shouldnt have an _output_enabled attr.
if a pin is output only, it shouldnt have an _output_enabled attr.

This way, `ls` tells you that a particular port/bit cannot possibly
drive a value out of the chip.

- one of several different values (otherwize why show it ?
  After all, you dont to be told that PI=3.14159...)
- changed.  if a pin cannot be output, theres nothing to enable, and
  showing the attribute is confusing.

OTOH, a readonly _output_enabled would also convey info.


yield the same, but not as
visibly (ls vs ls -l)

So, Im somewhat ambivalent here, looking for input....


User-Space

Following LM-sensors approach, a user-side library would add the
niceties:

- provide any equivalences needed by users
  ie bit_x_tristate = ! bit_x_output_enabled.

- sub-port allocation and management.
  support for 3+3+2 bit sub-ports on an 8 bit port would be nice

I suspect that a sophisticated programmer would be able to add a
sub-port allocation facility w/in the driver.  I cannot,



GPIO Pin Features

As outlined above, pin features are represented as _<suffixes>

1st: there are several alternative naming schemes:

- name-as-verb          _output_enable          (conveys an 'action')
- name-as-state         _output_enabled         (conveys a 'current state')
- feature-name          _output                 (a knob to turn)
- feature+state         _output+(currval)       (currval in name is bad idea)

1,2 are quite close.  Ive done 2.

FWIW, heres the pin attributes of my GPIOs, as expressed in the syslog
by the legacy drivers: (these are

[15510.384000]  pc8736x_gpio.0: io16: 0x0004 TS OD PUE  EDGE LO         io:0/1
[15510.564000]  pc8736x_gpio.0: io17: 0x0004 TS OD PUE  EDGE LO         io:1/1
[15510.744000]  pc8736x_gpio.0: io18: 0x0004 TS OD PUE  EDGE LO         io:1/1
[15510.928000]  pc8736x_gpio.0: io19: 0x0004 TS OD PUE  EDGE LO         io:1/1


# whether output-drive is on/off
_output_enable          # 1 or 0,
_tristate               # ! _output_enable, logically linked.

Now, theres no need to have both of these; if there were, they would
have to be intrinsically linked (logically opposite values).

IOW, drivers should name the file as one of possible states of the
feature, which ever best describes it, and not expose it 2x.

To the extent that we need support for '_tristate' version of a
'_output_enabled' sysfs-file, user-space (libraries) should provide
that support.

# output circuit configuration
_opendrain              # only 1 transistor, can sink current from pin
_totem                  # has 2nd transistor, can drive pin hi.
_pushpull               # alias for _totempole

Ive chosen _totem as the attribute name

_pullup_enabled         # pin tied to power via resistor.
_pullup_off             # duh
_pullup_no              # how many aliases ?

_debounce               # present if supported, 0 if off, 1 if on.

It kinda works, but the pullup is a bit ugly, and all the aliases
suggest some semantic difficulty/mismatch/incompleteness, but adding
them all definitely creates clutter and has reached diminished
incremental value.


If hardware doesnt support a feature, like _opendrain, it:

 - sets _pushpull to 1, readonly ?
 - sets _opendrain to 0, readonly ?
    OR never creates _opendrain ?

Doing either of these works to communicate the feature-set to
user-space, but not creating _opendrain when pin doesnt do it means
that the file's presense communicates this; IOW, user issues 'ls', not
'ls -l' to find out.

(continuing strawman)

_value          # read the pin
(no-suffix)     # alias for _value

_current        # the value 'driven' by the pin (last written)

And here we can see some potential (user) difficulties;
under some conditions,
- read-value = current-value

but not on these:
- pin is input-only/tristate - (current is irrelevant, except as 'state')
- pin is over-driven by attached circuit
-- pin cannot sink/supply sufficient current

Detecting these situations is both hardware and circuit dependent, and
properly belongs in user-space.  It sounds a lot like what lm-sensors
does already.

For the 2 drivers Ive 'experienced', pin control was via device-file,
with this command-set.  Presumably the correspondence with the sysfs
strawman above is obvious.

case 'O':        output enabled
case 'o':        output disabled
case 'T':        output is push pull
case 't':        output is open drain
case 'P':        pull up enabled
case 'p':        pull up disabled




Port Organization.

My *vast* experience (with 1.5 GPIO architectures) suggests that all
chips organize their GPIOs into one or more ports.  Each port supports
reading and writing all bits simultaneously.

Some hardware also supports reading/writing pin-properties like
output-enable in a single-word (todo-research).  Drivers for these
hardwares could/should create attributes for each pin-property that is
accessible as a bit-vector.

Further, port (and pin) capabilities generally vary by port; hardware
will typically put a full set of features on 1 port, and less on
others, expecting a designer to allocate functions to pins
accordingly.  Forex, on the pc8736x chip, port 0 can issue interrupts,
so those pins should have extra properties.

These capabilities must be cleanly representable in any worthwhile
sysfs/GPIO model (and we continue to test this strawman)..


Port-names and Pin-names

# prefixes (note the trailing _)
port_[0..P]_
bit_[P]_[0..bits-per-word]_

Getting past the port/bit names, these files are populated by the
driver according to the device.  For the 2 drivers Ive touched, heres
the table:

driver:         ports   bits-per-port
scx200_gpio     1       32
pc8736x_gpio    4       8




Strawman tie-together:

bit_0_0_output_enable   # shows current output-drive of port 0 bit 0
bit_0_0_value
bit_0_0                 # 2 reads of same bit

# lessee what happens :->
port_0_value_bin        # 1-4 bytes typically returned (depending on device)
port_0_value_hex        # converted to human readable, always printable
port_0_value            #

port_1_output_enable    # read/write vector of enable bits to port
port_1_<suffix_set>     #


The driver should know which properties are readable/writable in a
bit-vector basis, and expose those sysfs-attributes only.  Thus the
presense of the port_N_value* attrs implies that all the bits in that
port are readable at once.

If the driver doesnt expose forex: port_1_output_enable, user-space is
free to loop over each bit, in essence 'emulating' the port-wide
operation.


RESTATING - whats above kinda hangs together.

NEXT - muddles

pin_XY_output_state     # one-of( 'output_enable', 'tristate')

This might be convenient for some situations, but probably is needless
complication / obfuscation.


pin_XY_state_bin                # binary state reader

This is intended an 'escape-valve' for things that are turn out to be
cumbersome with the above.  This is probably tantamount to an IOCTL,
so might be a hugely bad idea.


pin_XY_interrupt_enable        #
pin_XY_interrupt_trigger_edge
pin_XY_interrupt_trigger_level
pin_XY_interrupt_trigger_edge_rising
pin_XY_interrupt_trigger_edge_falling
pin_XY_interrupt_trigger_level_hi
pin_XY_interrupt_trigger_level_lo

Well - thats a big one - Do we expose any of this ?
- the ability to enable / disable / control hardware interrupt
- or is that insane meddling in such affairs ?

We cant afterall allow mapping of the actual interrupt handler, that
does sound insane (unless hugely carefull)

With the new genirq architecture, things are apparently more
orthogonal, which suggests there might be something to control by
means of attributes such as above.

Further, any of the above attributes could readily be RO; they would
convey info thats at least useful, even if 'control' is too exposing.

Forex, somewhere during boot, the APIC is setup to handle interrupts;
at this point its probably known what the configured state of all
interrrupts is, and this info could be exposed here.  Whether that has
sufficient value is unclear, and certainly not for v.submit-1


when a pin is level-triggered (presumably this can be
determined early in the boot process, as soon as APIC etc would be


setup), the _edge_* attributes vanish, and the _level_{hi,lo} attrs
are set 1/0, and RO.


OK - IM DONE.

Please be liberal with feedback -
- this is wrong
- poorly explained -
- correct ... - boil this down - reduce to a guiding statement
- strip out conjectures





diff -ruNp -X dontdiff -X exclude-diffs am-19/drivers/char/nsc_gpio.c am-20/drivers/char/nsc_gpio.c
--- am-19/drivers/char/nsc_gpio.c	2006-06-14 21:38:14.000000000 -0600
+++ am-20/drivers/char/nsc_gpio.c	2006-06-14 22:31:07.000000000 -0600
@@ -13,6 +13,8 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
@@ -39,63 +41,96 @@ void nsc_gpio_dump(struct nsc_gpio_ops *
 		 amp->gpio_get(index), amp->gpio_current(index));
 }
 
+/* the pin-mode-change 'commands' of the legacy device-file-interface,
+   now refactored for reuse in a sysfs-interface.  Includes some
+   updates which arent exposed via sysfs attributes.
+*/
+static int common_write(struct nsc_gpio_ops *amp, char c, unsigned m)
+{
+       struct device *dev = amp->dev;
+       int err = 0;
+       switch (c) {
+       case '0':
+               amp->gpio_set(m, 0);
+               break;
+       case '1':
+               amp->gpio_set(m, 1);
+               break;
+       case 'O':
+               dev_dbg(dev, "GPIO%d output enabled\n", m);
+               amp->gpio_config(m, ~1, 1);
+               break;
+       case 'o':
+               dev_dbg(dev, "GPIO%d output disabled\n", m);
+               amp->gpio_config(m, ~1, 0);
+               break;
+       case 'T':
+               dev_dbg(dev, "GPIO%d output is push pull\n", m);
+               amp->gpio_config(m, ~2, 2);
+               break;
+       case 't':
+               dev_dbg(dev, "GPIO%d output is open drain\n", m);
+               amp->gpio_config(m, ~2, 0);
+               break;
+       case 'P':
+               dev_dbg(dev, "GPIO%d pull up enabled\n", m);
+               amp->gpio_config(m, ~4, 4);
+               break;
+       case 'p':
+               dev_dbg(dev, "GPIO%d pull up disabled\n", m);
+               amp->gpio_config(m, ~4, 0);
+               break;
+       case 'L':
+               dev_dbg(dev, "GPIO%d lock pin\n", m);
+               amp->gpio_config(m, ~8, 8);
+               break;
+       case 'l':
+               dev_warn(dev, "GPIO%d cant unlock locked? pin\n", m);
+               amp->gpio_config(m, ~8, 0);
+               break;
+
+       case 'D':
+               dev_dbg(dev, "GPIO%d turn on debounce\n", m);
+               amp->gpio_config(m, ~PF_DEBOUNCE, PF_DEBOUNCE);
+               break;
+       case 'd':
+               dev_warn(dev, "GPIO%d cant unlock locked? pin\n", m);
+               amp->gpio_config(m, ~PF_DEBOUNCE, 0);
+               break;
+
+       case 'v':
+               /* View Current pin settings */
+               amp->gpio_dump(amp, m);
+               break;
+       case 'c':
+               /* view pin's current values: driven and read */
+               dev_info(dev, "io%02d: driven %d, input %d\n",
+                        m, amp->gpio_current(m), amp->gpio_get(m));
+               break;
+       case '\n':
+               /* end of settings string, do nothing */
+               break;
+       default:
+               dev_err(dev, "GPIO-%2d bad setting: chr<0x%2x>\n", m,
+                       (int)c);
+               err++;
+       }
+       return err;
+}
+
 ssize_t nsc_gpio_write(struct file *file, const char __user *data,
 		       size_t len, loff_t *ppos)
 {
+	int i, err = 0;
 	unsigned m = iminor(file->f_dentry->d_inode);
 	struct nsc_gpio_ops *amp = file->private_data;
-	struct device *dev = amp->dev;
-	size_t i;
-	int err = 0;
 
 	for (i = 0; i < len; ++i) {
 		char c;
 		if (get_user(c, data + i))
 			return -EFAULT;
-		switch (c) {
-		case '0':
-			amp->gpio_set(m, 0);
-			break;
-		case '1':
-			amp->gpio_set(m, 1);
-			break;
-		case 'O':
-			dev_dbg(dev, "GPIO%d output enabled\n", m);
-			amp->gpio_config(m, ~1, 1);
-			break;
-		case 'o':
-			dev_dbg(dev, "GPIO%d output disabled\n", m);
-			amp->gpio_config(m, ~1, 0);
-			break;
-		case 'T':
-			dev_dbg(dev, "GPIO%d output is push pull\n", m);
-			amp->gpio_config(m, ~2, 2);
-			break;
-		case 't':
-			dev_dbg(dev, "GPIO%d output is open drain\n", m);
-			amp->gpio_config(m, ~2, 0);
-			break;
-		case 'P':
-			dev_dbg(dev, "GPIO%d pull up enabled\n", m);
-			amp->gpio_config(m, ~4, 4);
-			break;
-		case 'p':
-			dev_dbg(dev, "GPIO%d pull up disabled\n", m);
-			amp->gpio_config(m, ~4, 0);
-			break;
-
-		case 'v':
-			/* View Current pin settings */
-			amp->gpio_dump(amp, m);
-			break;
-		case '\n':
-			/* end of settings string, do nothing */
-			break;
-		default:
-			dev_err(dev, "GPIO-%2d bad setting: chr<0x%2x>\n", m,
-			       (int)c);
-			err++;
-		}
+
+		err += common_write(amp, c, m);
 	}
 	if (err)
 		return -EINVAL;	/* full string handled, report error */
@@ -122,6 +157,85 @@ EXPORT_SYMBOL(nsc_gpio_write);
 EXPORT_SYMBOL(nsc_gpio_read);
 EXPORT_SYMBOL(nsc_gpio_dump);
 
+
+/* now sysfs versions.  We use 2D addressing, using struct
+   sensor_device_attribute_2's .index and .nr members, with which we
+
+   Slightly complicating things, these declarations must be made in
+   the client modules of this one, ie scx200 & pc8736x _gpio.  They
+   also must set device.driver_data to amp, as thats needed by
+   sysfs_set_value()
+*/
+
+ssize_t nsc_gpio_sysfs_set(struct device *dev,
+			   struct device_attribute *devattr, const char *buf,
+			   size_t count)
+{
+        struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+        int idx = attr->index;
+        int func = attr->nr;
+	struct nsc_gpio_ops *amp = dev->driver_data;
+	int err, xor;
+
+	/* invert cmd if setting low */
+	xor = simple_strtol(buf, NULL, 10) ? 0 : 'T'^'t';
+
+	dev_info(dev, "set func:%d  Func:%d, flp%d\n", func, func^xor, xor);
+
+	err = common_write(amp, func^xor, idx);
+
+	if (err)
+		return -EINVAL;	// full string handled, report error
+	
+	return strlen(buf);
+}
+
+ssize_t nsc_gpio_sysfs_get(struct device *dev,
+			   struct device_attribute *devattr, char *buf)
+{ 
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+        int idx = attr->index;
+        int func = attr->nr;
+        unsigned res = -1;
+	u32 config;
+	struct nsc_gpio_ops *amp = dev->driver_data;
+
+	if (!amp) {
+		dev_err(dev, "nsc_gpio_sysfs_get(), no amp\n");
+		return 0;
+	}
+	config = amp->gpio_config(idx, ~0, 0);
+
+	dev_dbg(dev, "nsc_gpio_sysfs_get(), bitconf=%02x, cmd='%c'\n",
+		config, func);
+
+	switch ((char)func) {
+	case 'O':
+		res = config & 1;
+		break;
+	case 'P':
+		res = config & 2;
+		break;
+	case 'T':
+		res = config & 4;
+		break;
+	case 'L':
+		res = config & 8;
+		break;
+	case 'D':
+		res = config & PF_DEBOUNCE;
+		break;
+        default:
+                dev_err(dev, "unknown cmd '%c' %d\n", func, func);
+        }
+	dev_dbg(dev, "bit[%d].cmd('%c')\n", idx, func);
+
+        return sprintf(buf, "%u\n", !!res);
+}
+
+EXPORT_SYMBOL(nsc_gpio_sysfs_get);
+EXPORT_SYMBOL(nsc_gpio_sysfs_set);
+
 static int __init nsc_gpio_init(void)
 {
 	printk(KERN_DEBUG NAME " initializing\n");
diff -ruNp -X dontdiff -X exclude-diffs am-19/drivers/char/pc8736x_gpio.c am-20/drivers/char/pc8736x_gpio.c
--- am-19/drivers/char/pc8736x_gpio.c	2006-06-14 21:37:51.000000000 -0600
+++ am-20/drivers/char/pc8736x_gpio.c	2006-06-14 21:39:03.000000000 -0600
@@ -17,6 +17,8 @@
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
@@ -251,6 +253,52 @@ static struct file_operations pc8736x_gp
 	.read	= nsc_gpio_read,
 };
 
+/* insert some sysfs decls and inits */
+
+static struct gpio_pin_attributes port0[] = {
+	GPIO_ATTRS(0,0), GPIO_ATTRS(0,1), GPIO_ATTRS(0,2), GPIO_ATTRS(0,3),
+	GPIO_ATTRS(0,4), GPIO_ATTRS(0,5), GPIO_ATTRS(0,6), GPIO_ATTRS(0,7),
+};
+
+static struct gpio_pin_attributes port1[] = {
+	GPIO_ATTRS(1,0), GPIO_ATTRS(1,1), GPIO_ATTRS(1,2), GPIO_ATTRS(1,3),
+	GPIO_ATTRS(1,4), GPIO_ATTRS(1,5), GPIO_ATTRS(1,6), GPIO_ATTRS(1,7),
+};
+
+static struct gpio_pin_attributes port2[] = {
+	GPIO_ATTRS(2,0), GPIO_ATTRS(2,1), GPIO_ATTRS(2,2), GPIO_ATTRS(2,3),
+	GPIO_ATTRS(2,4), GPIO_ATTRS(2,5), GPIO_ATTRS(2,6), GPIO_ATTRS(2,7),
+};
+
+static struct gpio_pin_attributes port3[] = {
+	GPIO_ATTRS(3,0), GPIO_ATTRS(3,1), GPIO_ATTRS(3,2), GPIO_ATTRS(3,3),
+	GPIO_ATTRS(3,4), GPIO_ATTRS(3,5), GPIO_ATTRS(3,6), GPIO_ATTRS(3,7),
+};
+
+static void __init nsc_gpio_sysfs_port_init(struct device* dev,
+					    struct gpio_pin_attributes pp[],
+					    int numdevs)
+{
+	int i;
+	dev_info(dev, "creating sysfs nodes\n");
+	for (i = 0; i < numdevs; i++) {
+		device_create_file(dev, &pp[i].output_enabled.dev_attr);
+		device_create_file(dev, &pp[i].totem_pole.dev_attr);
+		device_create_file(dev, &pp[i].locked.dev_attr);
+		device_create_file(dev, &pp[i].pullup_enabled.dev_attr);
+		device_create_file(dev, &pp[i].debounced.dev_attr);
+	}
+	
+}
+
+static void __init pc8736x_sysfs_init(struct device* dev)
+{
+	nsc_gpio_sysfs_port_init(dev, port0, 8);
+	nsc_gpio_sysfs_port_init(dev, port1, 8);
+	nsc_gpio_sysfs_port_init(dev, port2, 8);
+	nsc_gpio_sysfs_port_init(dev, port3, 8);
+}
+
 static void __init pc8736x_init_shadow(void)
 {
 	int port;
@@ -320,6 +368,12 @@ static int __init pc8736x_gpio_init(void
 		dev_dbg(&pdev->dev, ": got dynamic major %d\n", major);
 	}
 
+	pc8736x_sysfs_init(&pdev->dev);
+
+	/* provide info wheresysfs callbacks can get them */
+	pc8736x_access.dev->driver_data = &pc8736x_access;
+
+;
 	return 0;
 }
 
diff -ruNp -X dontdiff -X exclude-diffs am-19/drivers/char/scx200_gpio.c am-20/drivers/char/scx200_gpio.c
--- am-19/drivers/char/scx200_gpio.c	2006-06-14 21:37:10.000000000 -0600
+++ am-20/drivers/char/scx200_gpio.c	2006-06-14 22:10:28.000000000 -0600
@@ -12,16 +12,18 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
+#include <linux/scx200_gpio.h>
+
 #include <linux/types.h>
 #include <linux/cdev.h>
 
-#include <linux/scx200_gpio.h>
-#include <linux/nsc_gpio.h>
-
 #define NAME "scx200_gpio"
 #define DEVNAME NAME
 
@@ -74,6 +76,43 @@ static struct file_operations scx200_gpi
 struct cdev *scx200_devices;
 int num_devs = 32;
 
+
+/* insert sysfs decl and init-func here */
+
+static struct gpio_pin_attributes port0[] = {
+	GPIO_ATTRS(0,0), GPIO_ATTRS(0,1), GPIO_ATTRS(0,2), GPIO_ATTRS(0,3),
+	GPIO_ATTRS(0,4), GPIO_ATTRS(0,5), GPIO_ATTRS(0,6), GPIO_ATTRS(0,7),
+	GPIO_ATTRS(0,8), GPIO_ATTRS(0,9), GPIO_ATTRS(0,10), GPIO_ATTRS(0,11),
+	GPIO_ATTRS(0,12), GPIO_ATTRS(0,13), GPIO_ATTRS(0,14), GPIO_ATTRS(0,15),
+
+	GPIO_ATTRS(0,16), GPIO_ATTRS(0,17), GPIO_ATTRS(0,18), GPIO_ATTRS(0,19),
+	GPIO_ATTRS(0,20), GPIO_ATTRS(0,21), GPIO_ATTRS(0,22), GPIO_ATTRS(0,23),
+	GPIO_ATTRS(0,24), GPIO_ATTRS(0,25), GPIO_ATTRS(0,26), GPIO_ATTRS(0,27),
+	GPIO_ATTRS(0,28), GPIO_ATTRS(0,29), GPIO_ATTRS(0,30), GPIO_ATTRS(0,31),
+};
+
+static void __init nsc_gpio_sysfs_port_init(struct device* dev,
+					    struct gpio_pin_attributes pp[],
+					    int numdevs)
+{
+	int i;
+	dev_info(dev, "creating sysfs nodes\n");
+	for (i = 0; i < numdevs; i++) {
+		device_create_file(dev, &pp[i].output_enabled.dev_attr);
+		device_create_file(dev, &pp[i].totem_pole.dev_attr);
+		device_create_file(dev, &pp[i].locked.dev_attr);
+		device_create_file(dev, &pp[i].pullup_enabled.dev_attr);
+		device_create_file(dev, &pp[i].debounced.dev_attr);
+	}
+	
+}
+
+static void __init gpio_sysfs_init(struct device* dev)
+{
+	nsc_gpio_sysfs_port_init(dev, port0, 32);
+}
+
+
 static int __init scx200_gpio_init(void)
 {
 	int rc, i;
@@ -95,6 +134,7 @@ static int __init scx200_gpio_init(void)
 
 	/* nsc_gpio uses dev_dbg(), so needs this */
 	scx200_access.dev = &pdev->dev;
+	scx200_access.dev->driver_data = &scx200_access;
 
 	if (major)
 		rc = register_chrdev_region(dev, num_devs, "scx200_gpio");
diff -ruNp -X dontdiff -X exclude-diffs am-19/include/linux/nsc_gpio.h am-20/include/linux/nsc_gpio.h
--- am-19/include/linux/nsc_gpio.h	2006-06-14 21:37:10.000000000 -0600
+++ am-20/include/linux/nsc_gpio.h	2006-06-14 21:39:03.000000000 -0600
@@ -1,6 +1,4 @@
 /**
-   nsc_gpio.c
-
    National Semiconductor GPIO common access methods.
 
    struct nsc_gpio_ops abstracts the low-level access
@@ -19,6 +17,17 @@
    NSC sold the GEODE line to AMD, and the PC-8736x line to Winbond.
 */
 
+/* pin-feature to config-bit mapping is common to both chips
+   some ports' pins dont support upper nibble ops.
+*/
+#define PF_OUTPUT_ENA		1
+#define PF_TOTEM		2
+#define PF_PULLUP		4
+#define PF_LOCKED		8
+#define PF_INTRUPT_ENA		16
+#define PF_INTRUPT_TGR		32
+#define PF_DEBOUNCE		64
+
 struct nsc_gpio_ops {
 	struct module*	owner;
 	u32	(*gpio_config)	(unsigned iminor, u32 mask, u32 bits);
@@ -40,3 +49,58 @@ extern ssize_t nsc_gpio_read(struct file
 
 extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned index);
 
+
+/* Sysfs Interface: 2D callbacks: 
+	.index	= bit_num,
+	.nr	= attr_slct
+*/
+extern ssize_t nsc_gpio_sysfs_get(struct device *dev,
+				  struct device_attribute *devattr,
+				  char *buf);
+
+extern ssize_t nsc_gpio_sysfs_set(struct device *dev,
+				  struct device_attribute *devattr,
+				  const char *buf, size_t count);
+
+/* GPIO pins have 5 attributes we care about: group them */
+struct gpio_pin_attributes {
+	struct sensor_device_attribute_2
+		output_enabled,
+		totem_pole,
+		pullup_enabled,
+		debounced,
+		locked;
+};
+
+#define GPIO_PIN(_grp, _idx, _pre, _post, _mode, _show, _store, _nr)	\
+	{	.dev_attr = __ATTR(_pre## _grp._idx ##_post,		\
+				   _mode, _show, _store),		\
+		.index = _idx, .nr = _nr }
+
+#define GPIO_ATTR(GN, IN, FnSym, AttrNm)	\
+	GPIO_PIN(GN, IN, bit_, AttrNm,		\
+		 S_IWUSR | S_IRUGO,		\
+		 nsc_gpio_sysfs_get, nsc_gpio_sysfs_set, FnSym )
+
+/* command alphabet - initializer components */
+#define PIN_OE(P,N)		GPIO_ATTR(P, N, 'O', _output_enabled)
+#define PIN_PP(P,N)		GPIO_ATTR(P, N, 'T', _totem)
+#define PIN_PUE(P,N)		GPIO_ATTR(P, N, 'P', _pullup_enabled)
+#define PIN_LOCKED(P,N)		GPIO_ATTR(P, N, 'L', _locked)
+#define PIN_DEBOUNCE(P,N)	GPIO_ATTR(P, N, 'D', _debounced)
+
+/* initializer for ports; ie pin arrays */
+#define GPIO_ATTRS(Port, Idx) {			\
+	.output_enabled	= PIN_OE(Port, Idx),	\
+	.totem_pole	= PIN_PP(Port, Idx),	\
+	.pullup_enabled	= PIN_PUE(Port, Idx),	\
+	.locked		= PIN_LOCKED(Port, Idx), \
+	.debounced	= PIN_DEBOUNCE(Port, Idx) }
+
+/* The original scx200_gpio driver exposed pins to the user as
+   char-device-files.  We preserve this legacy command alphabet, and
+   map them onto struct sensor_device_attribute_2's .nr member.
+   This gives us 2D addressing, and puts all the bit-banging into a
+   single pair of callbacks.
+*/
+

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Andrew Morton]

On Sat, 17 Jun 2006 12:42:28 -0600
Jim Cromie <jim.cromie@gmail.com> wrote:

> Ok, 
> 
> heres the brand-spanking-new proto-sysfs-gpio interface,
> preceded by some pseudo/proto-Documentation.
> 

Well I stuck it in -mm anyway.  Let's see what happens.

We don't have a Signed-off-by: for this patch.

Fixup patches agains next -mm would be suitable.  Please keep them
super-short: basically one-patch-per-review-comment.  That way I can easily
instertion-sort the patches into place and we retain a nice patch series.

> Ive

Apostrophe aversion?

>  
> +/* the pin-mode-change 'commands' of the legacy device-file-interface,
> +   now refactored for reuse in a sysfs-interface.  Includes some
> +   updates which arent exposed via sysfs attributes.
> +*/
> +static int common_write(struct nsc_gpio_ops *amp, char c, unsigned m)
> +{
> +       struct device *dev = amp->dev;
> +       int err = 0;
> +       switch (c) {
> +       case '0':
> +               amp->gpio_set(m, 0);
> +               break;
> +       case '1':
> +               amp->gpio_set(m, 1);
> +               break;
> +       case 'O':
> +               dev_dbg(dev, "GPIO%d output enabled\n", m);
> +               amp->gpio_config(m, ~1, 1);
> +               break;
> +       case 'o':
> +               dev_dbg(dev, "GPIO%d output disabled\n", m);
> +               amp->gpio_config(m, ~1, 0);
> +               break;
> +       case 'T':
> +               dev_dbg(dev, "GPIO%d output is push pull\n", m);
> +               amp->gpio_config(m, ~2, 2);
> +               break;
> +       case 't':
> +               dev_dbg(dev, "GPIO%d output is open drain\n", m);
> +               amp->gpio_config(m, ~2, 0);
> +               break;
> +       case 'P':
> +               dev_dbg(dev, "GPIO%d pull up enabled\n", m);
> +               amp->gpio_config(m, ~4, 4);
> +               break;
> +       case 'p':
> +               dev_dbg(dev, "GPIO%d pull up disabled\n", m);
> +               amp->gpio_config(m, ~4, 0);
> +               break;
> +       case 'L':
> +               dev_dbg(dev, "GPIO%d lock pin\n", m);
> +               amp->gpio_config(m, ~8, 8);
> +               break;
> +       case 'l':
> +               dev_warn(dev, "GPIO%d cant unlock locked? pin\n", m);
> +               amp->gpio_config(m, ~8, 0);
> +               break;
> +
> +       case 'D':
> +               dev_dbg(dev, "GPIO%d turn on debounce\n", m);
> +               amp->gpio_config(m, ~PF_DEBOUNCE, PF_DEBOUNCE);
> +               break;
> +       case 'd':
> +               dev_warn(dev, "GPIO%d cant unlock locked? pin\n", m);
> +               amp->gpio_config(m, ~PF_DEBOUNCE, 0);
> +               break;
> +
> +       case 'v':
> +               /* View Current pin settings */
> +               amp->gpio_dump(amp, m);
> +               break;
> +       case 'c':
> +               /* view pin's current values: driven and read */
> +               dev_info(dev, "io%02d: driven %d, input %d\n",
> +                        m, amp->gpio_current(m), amp->gpio_get(m));
> +               break;
> +       case '\n':
> +               /* end of settings string, do nothing */
> +               break;
> +       default:
> +               dev_err(dev, "GPIO-%2d bad setting: chr<0x%2x>\n", m,
> +                       (int)c);
> +               err++;
> +       }
> +       return err;
> +}
> +
>  ssize_t nsc_gpio_write(struct file *file, const char __user *data,
>  		       size_t len, loff_t *ppos)
>  {
> +	int i, err = 0;
>  	unsigned m = iminor(file->f_dentry->d_inode);
>  	struct nsc_gpio_ops *amp = file->private_data;
> -	struct device *dev = amp->dev;
> -	size_t i;
> -	int err = 0;
>  
>  	for (i = 0; i < len; ++i) {
>  		char c;
>  		if (get_user(c, data + i))
>  			return -EFAULT;
> -		switch (c) {
> -		case '0':
> -			amp->gpio_set(m, 0);
> -			break;
> -		case '1':
> -			amp->gpio_set(m, 1);
> -			break;
> -		case 'O':
> -			dev_dbg(dev, "GPIO%d output enabled\n", m);
> -			amp->gpio_config(m, ~1, 1);
> -			break;
> -		case 'o':
> -			dev_dbg(dev, "GPIO%d output disabled\n", m);
> -			amp->gpio_config(m, ~1, 0);
> -			break;
> -		case 'T':
> -			dev_dbg(dev, "GPIO%d output is push pull\n", m);
> -			amp->gpio_config(m, ~2, 2);
> -			break;
> -		case 't':
> -			dev_dbg(dev, "GPIO%d output is open drain\n", m);
> -			amp->gpio_config(m, ~2, 0);
> -			break;
> -		case 'P':
> -			dev_dbg(dev, "GPIO%d pull up enabled\n", m);
> -			amp->gpio_config(m, ~4, 4);
> -			break;
> -		case 'p':
> -			dev_dbg(dev, "GPIO%d pull up disabled\n", m);
> -			amp->gpio_config(m, ~4, 0);
> -			break;
> -
> -		case 'v':
> -			/* View Current pin settings */
> -			amp->gpio_dump(amp, m);
> -			break;
> -		case '\n':
> -			/* end of settings string, do nothing */
> -			break;
> -		default:
> -			dev_err(dev, "GPIO-%2d bad setting: chr<0x%2x>\n", m,
> -			       (int)c);
> -			err++;
> -		}
> +
> +		err += common_write(amp, c, m);
>  	}
>  	if (err)
>  		return -EINVAL;	/* full string handled, report error */

This all spat a whopping great reject for reasons unknown, which I fixed by
hand.  Please check that it all still works.


> +ssize_t nsc_gpio_sysfs_set(struct device *dev,
> +			   struct device_attribute *devattr, const char *buf,
> +			   size_t count)
> +{
> +        struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> +        int idx = attr->index;
> +        int func = attr->nr;
> +	struct nsc_gpio_ops *amp = dev->driver_data;
> +	int err, xor;
> +
> +	/* invert cmd if setting low */
> +	xor = simple_strtol(buf, NULL, 10) ? 0 : 'T'^'t';

whoa.  How does this work?

> +	dev_info(dev, "set func:%d  Func:%d, flp%d\n", func, func^xor, xor);
> +
> +	err = common_write(amp, func^xor, idx);

The tricksies in here aren't very understandable.  Can it be simplified?

> +	if (err)
> +		return -EINVAL;	// full string handled, report error
> +	
> +	return strlen(buf);
> +}
>
> ...
>
>  static void __init pc8736x_init_shadow(void)
>  {
>  	int port;
> @@ -320,6 +368,12 @@ static int __init pc8736x_gpio_init(void
>  		dev_dbg(&pdev->dev, ": got dynamic major %d\n", major);
>  	}
>  
> +	pc8736x_sysfs_init(&pdev->dev);
> +
> +	/* provide info wheresysfs callbacks can get them */
> +	pc8736x_access.dev->driver_data = &pc8736x_access;
> +
> +;
>  	return 0;
>  }

stray semicolon.

> +/* GPIO pins have 5 attributes we care about: group them */
> +struct gpio_pin_attributes {
> +	struct sensor_device_attribute_2
> +		output_enabled,
> +		totem_pole,
> +		pullup_enabled,
> +		debounced,
> +		locked;
> +};

A bit hard to read.  Normally we'd do

struct gpio_pin_attributes {
	struct sensor_device_attribute_2 output_enabled;
	struct sensor_device_attribute_2 totem_pole;
	struct sensor_device_attribute_2 pullup_enabled;
	struct sensor_device_attribute_2 debounced;
	struct sensor_device_attribute_2 locked;
};

because kernel-style is super-simple-style.

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Jim Cromie]

Andrew Morton wrote:
> On Sat, 17 Jun 2006 12:42:28 -0600
> Jim Cromie <jim.cromie@gmail.com> wrote:
>
>   
>> Ok, 
>>
>> heres the brand-spanking-new proto-sysfs-gpio interface,
>> preceded by some pseudo/proto-Documentation.
>>
>>     
>
> Well I stuck it in -mm anyway.  Let's see what happens.
>   
Im clearly benefiting from the new streamlined inclusion process :-)
> We don't have a Signed-off-by: for this patch.
>   

Heres a better version
- with signoff
- the pc8736x_gpio driver now creates the port_[0-3]_<suffix>  sysfs-files.
    the setter is stubbed, the getter works for the port Values *only*
    the other port-wide attrs must be assembled by looping over the 
bit-configs
- it applies on what I sent,
- if its too late, or kicks out any rejects against your version, Ill 
repost a fresh one against 17-mm1
- its still pretty rough - so // marks *some* of the rough spots.
    theres some dissonance wrt port-wide access and common-write, which 
is for bits originally/still.
- its too big (but then so was the other one ;-) Ill chop it up into 
smaller chunks soon.

- sorry about the leading-spaces crap in some of the earlier patches,
    I now know that cut/paste mangles the tabs.

- Ill send a proper doc patch soon, post -mm1
    Hopefully, the prose youve got now will draw comments, and I can fix 
b4 sending.

> Fixup patches agains next -mm would be suitable.  Please keep them
> super-short: basically one-patch-per-review-comment.  That way I can easily
> instertion-sort the patches into place and we retain a nice patch series.
>
>   
OK.  Just so Im clear, Ill patch against the tail of the set (ie -mm1), 
and you'll push them forward into the
series as close as possible to where the blunder was made ?  (and less 
close for conflicts )
>> Ive
>>     
>
> Apostrophe aversion?
>   

Its PFL.  Its also IMO a trend in the language.  "Its" (the contraction) 
has dropped the '
IIUC, its (with the apostrophe) now only for possession, forex: Its 
bigger than the sum of it's parts.
Im just taking that to excess, everywhere its applicable ;-)

---

Signed-off-by:  Jim Cromie <jim.cromie@gmail.com>

$ diffstat diff.sys-gpio-rollup-2.2
 drivers/char/nsc_gpio.c     |  265 ++++++++++++++++++++++++++++++++++++--------
 drivers/char/pc8736x_gpio.c |  153 +++++++++++++++++++++----
 drivers/char/scx200_gpio.c  |   47 +++++++
 include/linux/nsc_gpio.h    |  116 ++++++++++++++++++-
 include/linux/scx200_gpio.h |   50 +++++++-
 5 files changed, 550 insertions(+), 81 deletions(-)


diff -ruNp -X dontdiff -X exclude-diffs az-1/drivers/char/nsc_gpio.c az-2/drivers/char/nsc_gpio.c
--- az-1/drivers/char/nsc_gpio.c	2006-06-18 13:14:52.000000000 -0600
+++ az-2/drivers/char/nsc_gpio.c	2006-06-20 12:36:40.000000000 -0600
@@ -13,6 +13,8 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
@@ -39,12 +41,68 @@ void nsc_gpio_dump(struct nsc_gpio_ops *
 		 amp->gpio_get(index), amp->gpio_current(index));
 }
 
+/* the pin-mode-change 'commands' of the legacy device-file-interface,
+   refactored for reuse in a sysfs-interface.  Includes some updates
+   which arent exposed via sysfs attributes.
+*/
+static int common_write(struct nsc_gpio_ops *amp, unsigned m, char c)
+{
+	struct device *dev = amp->dev;
+	int err = 0;
+
+	switch (c) {
+	case 0:
+	case '0':
+		amp->gpio_set(m, 0);
+		break;
+	case 1:
+	case '1':
+		amp->gpio_set(m, 1);
+		break;
+	case 'O':
+		dev_dbg(dev, "GPIO%d output enabled\n", m);
+		amp->gpio_config(m, ~1, 1);
+		break;
+	case 'o':
+		dev_dbg(dev, "GPIO%d output disabled\n", m);
+		amp->gpio_config(m, ~1, 0);
+		break;
+	case 'T':
+		dev_dbg(dev, "GPIO%d output is push pull\n", m);
+		amp->gpio_config(m, ~2, 2);
+		break;
+	case 't':
+		dev_dbg(dev, "GPIO%d output is open drain\n", m);
+		amp->gpio_config(m, ~2, 0);
+		break;
+	case 'P':
+		dev_dbg(dev, "GPIO%d pull up enabled\n", m);
+		amp->gpio_config(m, ~4, 4);
+		break;
+	case 'p':
+		dev_dbg(dev, "GPIO%d pull up disabled\n", m);
+		amp->gpio_config(m, ~4, 0);
+		break;
+	case 'v':
+		/* View Current pin settings */
+		amp->gpio_dump(amp, m);
+		break;
+	case '\n':
+		/* end of settings string, do nothing */
+		break;
+	default:
+		dev_err(dev, "io%2d bad setting: chr<0x%2x>\n",
+			m, (int)c);
+		err++;
+	}
+	return err;
+}
+
 ssize_t nsc_gpio_write(struct file *file, const char __user *data,
 		       size_t len, loff_t *ppos)
 {
 	unsigned m = iminor(file->f_dentry->d_inode);
 	struct nsc_gpio_ops *amp = file->private_data;
-	struct device *dev = amp->dev;
 	size_t i;
 	int err = 0;
 
@@ -52,51 +110,8 @@ ssize_t nsc_gpio_write(struct file *file
 		char c;
 		if (get_user(c, data + i))
 			return -EFAULT;
-		switch (c) {
-		case '0':
-			amp->gpio_set(m, 0);
-			break;
-		case '1':
-			amp->gpio_set(m, 1);
-			break;
-		case 'O':
-			dev_dbg(dev, "GPIO%d output enabled\n", m);
-			amp->gpio_config(m, ~1, 1);
-			break;
-		case 'o':
-			dev_dbg(dev, "GPIO%d output disabled\n", m);
-			amp->gpio_config(m, ~1, 0);
-			break;
-		case 'T':
-			dev_dbg(dev, "GPIO%d output is push pull\n",
-			       m);
-			amp->gpio_config(m, ~2, 2);
-			break;
-		case 't':
-			dev_dbg(dev, "GPIO%d output is open drain\n",
-			       m);
-			amp->gpio_config(m, ~2, 0);
-			break;
-		case 'P':
-			dev_dbg(dev, "GPIO%d pull up enabled\n", m);
-			amp->gpio_config(m, ~4, 4);
-			break;
-		case 'p':
-			dev_dbg(dev, "GPIO%d pull up disabled\n", m);
-			amp->gpio_config(m, ~4, 0);
-			break;
-		case 'v':
-			/* View Current pin settings */
-			amp->gpio_dump(amp, m);
-			break;
-		case '\n':
-			/* end of settings string, do nothing */
-			break;
-		default:
-			dev_err(dev, "io%2d bad setting: chr<0x%2x>\n",
-				m, (int)c);
-			err++;
-		}
+
+		err += common_write(amp, m, c);
 	}
 	if (err)
 		return -EINVAL;	/* full string handled, report error */
@@ -123,6 +138,164 @@ EXPORT_SYMBOL(nsc_gpio_write);
 EXPORT_SYMBOL(nsc_gpio_read);
 EXPORT_SYMBOL(nsc_gpio_dump);
 
+
+/* now sysfs versions.  We use 2D addressing, using struct
+   sensor_device_attribute_2's .index and .nr members, which specifies
+   the bit's index and attribute respectively.
+*/
+ssize_t nsc_gpio_sysfs_set(struct device *dev,
+			   struct device_attribute *devattr, const char *buf,
+			   size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int idx = attr->index;
+	int func = attr->nr;
+	struct nsc_gpio_ops *amp = dev->driver_data;
+	int val, err = 0;
+
+	val = simple_strtol(buf, NULL, 10);
+	switch (val) {
+	case 0:
+	case 1:
+		err = common_write(amp, val, idx);
+		break;
+	default:
+		dev_err(dev, "illegal val %d %c\n", val, func);
+	}
+
+	if (err)
+		return -EINVAL;	// full string handled, report error
+
+	return strlen(buf);
+}
+
+ssize_t nsc_gpio_sysfs_get(struct device *dev,
+			   struct device_attribute *devattr, char *buf)
+{ 
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int idx = attr->index;
+	int func = attr->nr;
+	unsigned res = -1;
+	u32 config;
+	struct nsc_gpio_ops *amp = dev->driver_data;
+
+	if (!amp) {
+		dev_err(dev, "nsc_gpio_sysfs_get(), no amp\n");
+		return 0;
+	}
+	config = amp->gpio_config(idx, ~0, 0);
+
+	dev_dbg(dev, "nsc_gpio_sysfs_get(), bitconf=%02x, cmd='%c'\n",
+		config, func);
+
+	switch ((char)func) {
+	case 'O':
+		res = config & 1;
+		break;
+	case 'P':
+		res = config & 2;
+		break;
+	case 'T':
+		res = config & 4;
+		break;
+	case 'L':
+		res = config & 8;
+		break;
+	case 'D':
+		res = config & PF_DEBOUNCE;
+		break;
+        default:
+                dev_err(dev, "unknown cmd '%c' %d\n", func, func);
+        }
+	dev_dbg(dev, "bit[%d].cmd('%c')\n", idx, func);
+
+        return sprintf(buf, "%u\n", !!res);
+}
+
+EXPORT_SYMBOL(nsc_gpio_sysfs_get);
+EXPORT_SYMBOL(nsc_gpio_sysfs_set);
+
+
+/* The port-wide sysfs callbacks are more complicated; for all but
+   '_value' attribute, they must assemble the return value by looping
+   over all bits, and collecting the attr-val from each.
+
+   ATM, cheat, and just implement _value getter.
+   The setter is stubbed out
+*/
+
+ssize_t nsc_gpio_port_sysfs_set(struct device *dev,
+				struct device_attribute *devattr,
+				const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int idx = attr->index;
+	int func = attr->nr;
+	struct nsc_gpio_ops *amp = dev->driver_data;
+	int err = 0, val;
+
+	val = simple_strtol(buf, NULL, 20);
+	dev_warn(dev, "TODO: set port%d func:%c val:0x%x\n", idx, func, val);
+
+	// err = common_write(amp, val, idx);
+	if (err)
+		return -EINVAL;
+	
+	return strlen(buf);
+}
+
+ssize_t nsc_gpio_port_sysfs_get(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+	int idx = attr->index;
+	int func = attr->nr;
+	unsigned res = -1;
+	u32 config;
+	struct nsc_gpio_ops *amp = dev->driver_data;
+
+	if (!amp) {
+		dev_err(dev, "nsc_gpio_port_sysfs_get(), no amp\n");
+		return 0;
+	}
+
+	// config = amp[1]->gpio_config(idx, ~0, 0);
+
+	dev_info(dev, "nsc_gpio_port_sysfs_get: cmd='%c' access %p\n",
+		 func, amp[1]);
+
+	switch ((char)func) {
+	case 'V':
+		/* read port */
+		res = amp[1].gpio_get(idx);
+		dev_info(dev, "nsc_gpio_port_sysfs_get: %x\n", res);
+		break;
+	case 'O':
+		res = config & 1;
+		break;
+	case 'P':
+		res = config & 2;
+		break;
+	case 'T':
+		res = config & 4;
+		break;
+	case 'L':
+		res = config & 8;
+		break;
+	case 'D':
+		res = config & PF_DEBOUNCE;
+		break;
+        default:
+                dev_err(dev, "unknown cmd '%c' %d\n", func, func);
+        }
+	dev_info(dev, "port[%d].cmd('%c') = %x\n", idx, func, res);
+
+        return sprintf(buf, "%x\n", res);
+}
+
+EXPORT_SYMBOL(nsc_gpio_port_sysfs_get);
+EXPORT_SYMBOL(nsc_gpio_port_sysfs_set);
+
 static int __init nsc_gpio_init(void)
 {
 	printk(KERN_DEBUG NAME " initializing\n");
diff -ruNp -X dontdiff -X exclude-diffs az-1/drivers/char/pc8736x_gpio.c az-2/drivers/char/pc8736x_gpio.c
--- az-1/drivers/char/pc8736x_gpio.c	2006-06-18 13:14:52.000000000 -0600
+++ az-2/drivers/char/pc8736x_gpio.c	2006-06-18 21:04:23.000000000 -0600
@@ -17,6 +17,8 @@
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
@@ -144,16 +146,52 @@ static u32 pc8736x_gpio_configure(unsign
 					 SIO_GPIO_PIN_CONFIG);
 }
 
-static int pc8736x_gpio_get(unsigned minor)
+/* port access */
+static unsigned pc8736x_gpio_port_get(unsigned port)
 {
-	int port, bit, val;
+	unsigned res =  inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
+	dev_dbg(&pdev->dev, "pc8736x_gpio_port_get(%d) => %x\n", port, res);
+	return res;
+}
 
-	port = minor >> 3;
-	bit = minor & 7;
-	val = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
+static void pc8736x_gpio_port_set(unsigned port, unsigned val)
+{
+	pc8736x_gpio_shadow[port] = val;
+	outb_p(val, pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+}
+
+static unsigned pc8736x_gpio_port_current(unsigned port)
+{
+	return inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+}
+
+static void pc8736x_gpio_port_set_high(unsigned port)
+{
+	pc8736x_gpio_port_set(port, 0xFF);
+}
+
+static void pc8736x_gpio_port_set_low(unsigned port)
+{
+	pc8736x_gpio_port_set(port, 0);
+}
+
+static void pc8736x_gpio_port_change(unsigned index)
+{
+	pc8736x_gpio_port_set(index, !pc8736x_gpio_port_current(index));
+}
+
+
+/* bit access, uses port accessors */
+
+static unsigned pc8736x_gpio_get(unsigned minor)
+{
+	int port = minor >> 3;
+	int bit = minor & 7;
+	unsigned val;
+
+	val = pc8736x_gpio_port_get(port);
 	val >>= bit;
 	val &= 1;
-
 	dev_dbg(&pdev->dev, "_gpio_get(%d from %x bit %d) == val %d\n",
 		minor, pc8736x_gpio_base + port_offset[port] + PORT_IN, bit,
 		val);
@@ -161,14 +199,15 @@ static int pc8736x_gpio_get(unsigned min
 	return val;
 }
 
-static void pc8736x_gpio_set(unsigned minor, int val)
+static void pc8736x_gpio_set(unsigned minor, unsigned val)
 {
 	int port, bit, curval;
 
 	minor &= 0x1f;
 	port = minor >> 3;
 	bit = minor & 7;
-	curval = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+
+	curval = pc8736x_gpio_port_current(port);
 
 	dev_dbg(&pdev->dev, "addr:%x cur:%x bit-pos:%d cur-bit:%x + new:%d -> bit-new:%d\n",
 		pc8736x_gpio_base + port_offset[port] + PORT_OUT,
@@ -179,7 +218,9 @@ static void pc8736x_gpio_set(unsigned mi
 	dev_dbg(&pdev->dev, "gpio_set(minor:%d port:%d bit:%d)"
 		" %2x -> %2x\n", minor, port, bit, curval, val);
 
-	outb_p(val, pc8736x_gpio_base + port_offset[port] + PORT_OUT);
+	pc8736x_gpio_port_set(port, val);
+
+	curval = pc8736x_gpio_port_current(port);
 
 	curval = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_OUT);
 	val = inb_p(pc8736x_gpio_base + port_offset[port] + PORT_IN);
@@ -198,7 +239,7 @@ static void pc8736x_gpio_set_low(unsigne
 	pc8736x_gpio_set(index, 0);
 }
 
-static int pc8736x_gpio_current(unsigned minor)
+static unsigned pc8736x_gpio_current(unsigned minor)
 {
 	int port, bit;
 	minor &= 0x1f;
@@ -214,16 +255,29 @@ static void pc8736x_gpio_change(unsigned
 
 extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned iminor);
 
-static struct nsc_gpio_ops pc8736x_access = {
-	.owner		= THIS_MODULE,
-	.gpio_config	= pc8736x_gpio_configure,
-	.gpio_dump	= nsc_gpio_dump,
-	.gpio_get	= pc8736x_gpio_get,
-	.gpio_set	= pc8736x_gpio_set,
-	.gpio_set_high	= pc8736x_gpio_set_high,
-	.gpio_set_low	= pc8736x_gpio_set_low,
-	.gpio_change	= pc8736x_gpio_change,
-	.gpio_current	= pc8736x_gpio_current
+static struct nsc_gpio_ops pc8736x_access[] = {
+	{ /* bit operations */
+		.owner		= THIS_MODULE,
+		.gpio_config	= pc8736x_gpio_configure,
+		.gpio_dump	= nsc_gpio_dump,
+		.gpio_get	= pc8736x_gpio_get,
+		.gpio_set	= pc8736x_gpio_set,
+		.gpio_set_high	= pc8736x_gpio_set_high,
+		.gpio_set_low	= pc8736x_gpio_set_low,
+		.gpio_change	= pc8736x_gpio_change,
+		.gpio_current	= pc8736x_gpio_current
+	},
+	{ /* port operations */
+		.owner		= THIS_MODULE,
+		.gpio_config	= pc8736x_gpio_configure,
+		.gpio_dump	= nsc_gpio_dump,
+		.gpio_get	= pc8736x_gpio_port_get,
+		.gpio_set	= pc8736x_gpio_port_set,
+		.gpio_set_high	= pc8736x_gpio_port_set_high,
+		.gpio_set_low	= pc8736x_gpio_port_set_low,
+		.gpio_change	= pc8736x_gpio_port_change,
+		.gpio_current	= pc8736x_gpio_port_current
+	}
 };
 
 static int pc8736x_gpio_open(struct inode *inode, struct file *file)
@@ -245,6 +299,58 @@ static struct file_operations pc8736x_gp
 	.read	= nsc_gpio_read,
 };
 
+/* insert some sysfs decls and inits */
+
+static struct gpio_pin_attributes port0[] = {
+	PIN_ATTRS(0,0), PIN_ATTRS(0,1), PIN_ATTRS(0,2), PIN_ATTRS(0,3),
+	PIN_ATTRS(0,4), PIN_ATTRS(0,5), PIN_ATTRS(0,6), PIN_ATTRS(0,7),
+};
+static struct gpio_pin_attributes port1[] = {
+	PIN_ATTRS(1,0), PIN_ATTRS(1,1), PIN_ATTRS(1,2), PIN_ATTRS(1,3),
+	PIN_ATTRS(1,4), PIN_ATTRS(1,5), PIN_ATTRS(1,6), PIN_ATTRS(1,7),
+};
+static struct gpio_pin_attributes port2[] = {
+	PIN_ATTRS(2,0), PIN_ATTRS(2,1), PIN_ATTRS(2,2), PIN_ATTRS(2,3),
+	PIN_ATTRS(2,4), PIN_ATTRS(2,5), PIN_ATTRS(2,6), PIN_ATTRS(2,7),
+};
+static struct gpio_pin_attributes port3[] = {
+	PIN_ATTRS(3,0), PIN_ATTRS(3,1), PIN_ATTRS(3,2), PIN_ATTRS(3,3),
+	PIN_ATTRS(3,4), PIN_ATTRS(3,5), PIN_ATTRS(3,6), PIN_ATTRS(3,7),
+};
+
+static struct gpio_pin_attributes ports[] = {
+	PORT_ATTRS(0), PORT_ATTRS(1), PORT_ATTRS(2), PORT_ATTRS(3),
+};
+
+static void __init nsc_gpio_sysfs_port_init(struct device* dev,
+					    struct gpio_pin_attributes pp[],
+					    int numdevs)
+{
+	int i;
+	for (i = 0; i < numdevs; i++) {
+		device_create_file(dev, &pp[i].value.dev_attr);
+		device_create_file(dev, &pp[i].output_enabled.dev_attr);
+		device_create_file(dev, &pp[i].totem_pole.dev_attr);
+		device_create_file(dev, &pp[i].locked.dev_attr);
+		device_create_file(dev, &pp[i].pullup_enabled.dev_attr);
+		device_create_file(dev, &pp[i].debounced.dev_attr);
+	}
+}
+
+static void __init pc8736x_sysfs_init(struct device* dev)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(ports); i++) {
+		nsc_gpio_sysfs_port_init(dev, &ports[i], 1);
+	}
+	nsc_gpio_sysfs_port_init(dev, port0, 8);
+	nsc_gpio_sysfs_port_init(dev, port1, 8);
+	nsc_gpio_sysfs_port_init(dev, port2, 8);
+	nsc_gpio_sysfs_port_init(dev, port3, 8);
+
+	dev_info(dev, "created sysfs nodes\n");
+}
+
 static void __init pc8736x_init_shadow(void)
 {
 	int port;
@@ -277,7 +383,11 @@ static int __init pc8736x_gpio_init(void
                 platform_device_put(pdev);
 		return -ENODEV;
 	}
-	pc8736x_access.dev = &pdev->dev;
+	pc8736x_access[0].dev = &pdev->dev;
+	pc8736x_access[1].dev = &pdev->dev;
+	pdev->dev.driver_data = &pc8736x_access;
+
+        dev_info(&pdev->dev, "access %p\n", &pc8736x_access);
 
 	/* Verify that chip and it's GPIO unit are both enabled.
 	   My BIOS does this, so I take minimum action here
@@ -312,6 +422,7 @@ static int __init pc8736x_gpio_init(void
 	}
 
 	pc8736x_init_shadow();
+	pc8736x_sysfs_init(&pdev->dev);
 	return 0;
 }
 
diff -ruNp -X dontdiff -X exclude-diffs az-1/drivers/char/scx200_gpio.c az-2/drivers/char/scx200_gpio.c
--- az-1/drivers/char/scx200_gpio.c	2006-06-18 13:14:52.000000000 -0600
+++ az-2/drivers/char/scx200_gpio.c	2006-06-18 16:43:43.000000000 -0600
@@ -12,16 +12,18 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
+#include <linux/scx200_gpio.h>
+
 #include <linux/types.h>
 #include <linux/cdev.h>
 
-#include <linux/scx200_gpio.h>
-#include <linux/nsc_gpio.h>
-
 #define NAME "scx200_gpio"
 #define DEVNAME NAME
 
@@ -74,6 +76,43 @@ static struct file_operations scx200_gpi
 struct cdev *scx200_devices;
 int num_devs = 32;
 
+
+/* insert sysfs decl and init-func here */
+
+static struct gpio_pin_attributes port0[] = {
+	PIN_ATTRS(0,0), PIN_ATTRS(0,1), PIN_ATTRS(0,2), PIN_ATTRS(0,3),
+	PIN_ATTRS(0,4), PIN_ATTRS(0,5), PIN_ATTRS(0,6), PIN_ATTRS(0,7),
+	PIN_ATTRS(0,8), PIN_ATTRS(0,9), PIN_ATTRS(0,10), PIN_ATTRS(0,11),
+	PIN_ATTRS(0,12), PIN_ATTRS(0,13), PIN_ATTRS(0,14), PIN_ATTRS(0,15),
+
+	PIN_ATTRS(0,16), PIN_ATTRS(0,17), PIN_ATTRS(0,18), PIN_ATTRS(0,19),
+	PIN_ATTRS(0,20), PIN_ATTRS(0,21), PIN_ATTRS(0,22), PIN_ATTRS(0,23),
+	PIN_ATTRS(0,24), PIN_ATTRS(0,25), PIN_ATTRS(0,26), PIN_ATTRS(0,27),
+	PIN_ATTRS(0,28), PIN_ATTRS(0,29), PIN_ATTRS(0,30), PIN_ATTRS(0,31),
+};
+
+static void __init nsc_gpio_sysfs_port_init(struct device* dev,
+					    struct gpio_pin_attributes pp[],
+					    int numdevs)
+{
+	int i;
+	dev_info(dev, "creating sysfs nodes\n");
+	for (i = 0; i < numdevs; i++) {
+		device_create_file(dev, &pp[i].output_enabled.dev_attr);
+		device_create_file(dev, &pp[i].totem_pole.dev_attr);
+		device_create_file(dev, &pp[i].locked.dev_attr);
+		device_create_file(dev, &pp[i].pullup_enabled.dev_attr);
+		device_create_file(dev, &pp[i].debounced.dev_attr);
+	}
+	
+}
+
+static void __init gpio_sysfs_init(struct device* dev)
+{
+	nsc_gpio_sysfs_port_init(dev, port0, 32);
+}
+
+
 static int __init scx200_gpio_init(void)
 {
 	int rc, i;
@@ -95,6 +134,7 @@ static int __init scx200_gpio_init(void)
 
 	/* nsc_gpio uses dev_dbg(), so needs this */
 	scx200_access.dev = &pdev->dev;
+	scx200_access.dev->driver_data = &scx200_access;
 
 	if (major)
 		rc = register_chrdev_region(dev, num_devs, "scx200_gpio");
@@ -121,6 +161,7 @@ static int __init scx200_gpio_init(void)
 			dev_err(&pdev->dev, "Error %d on minor %d", rc, i);
 	}
 
+	gpio_sysfs_init(&pdev->dev);
 	return 0; /* succeed */
 
 undo_chrdev_region:
diff -ruNp -X dontdiff -X exclude-diffs az-1/include/linux/nsc_gpio.h az-2/include/linux/nsc_gpio.h
--- az-1/include/linux/nsc_gpio.h	2006-06-18 13:14:52.000000000 -0600
+++ az-2/include/linux/nsc_gpio.h	2006-06-18 20:19:29.000000000 -0600
@@ -1,6 +1,4 @@
 /**
-   nsc_gpio.c
-
    National Semiconductor GPIO common access methods.
 
    struct nsc_gpio_ops abstracts the low-level access
@@ -19,16 +17,27 @@
    NSC sold the GEODE line to AMD, and the PC-8736x line to Winbond.
 */
 
+/* pin-feature to config-bit mapping is common to both chips
+   some ports' pins dont support upper nibble ops.
+*/
+#define PF_OUTPUT_ENA		1
+#define PF_TOTEM		2
+#define PF_PULLUP		4
+#define PF_LOCKED		8
+#define PF_INTRUPT_ENA		16
+#define PF_INTRUPT_TGR		32
+#define PF_DEBOUNCE		64
+
 struct nsc_gpio_ops {
 	struct module*	owner;
 	u32	(*gpio_config)	(unsigned iminor, u32 mask, u32 bits);
 	void	(*gpio_dump)	(struct nsc_gpio_ops *amp, unsigned iminor);
-	int	(*gpio_get)	(unsigned iminor);
-	void	(*gpio_set)	(unsigned iminor, int state);
+	u32	(*gpio_get)	(unsigned iminor);
+	void	(*gpio_set)	(unsigned iminor, unsigned state);
 	void	(*gpio_set_high)(unsigned iminor);
 	void	(*gpio_set_low)	(unsigned iminor);
 	void	(*gpio_change)	(unsigned iminor);
-	int	(*gpio_current)	(unsigned iminor);
+	u32	(*gpio_current)	(unsigned iminor);
 	struct device*	dev;	/* for dev_dbg() support, set in init  */
 };
 
@@ -40,3 +49,100 @@ extern ssize_t nsc_gpio_read(struct file
 
 extern void nsc_gpio_dump(struct nsc_gpio_ops *amp, unsigned index);
 
+
+/* The original scx200_gpio driver exposed pins to the user as
+   char-device-files.  We preserve this legacy command alphabet, and
+   map them onto struct sensor_device_attribute_2's .nr member.
+   This gives us 2D addressing, and puts all the bit-banging into a
+   single pair of callbacks.
+*/
+
+
+/* Sysfs Interface: 2D callbacks: 
+	.index	= bit_num,
+	.nr	= attr_slct
+*/
+extern ssize_t nsc_gpio_sysfs_get(struct device *dev,
+				  struct device_attribute *devattr,
+				  char *buf);
+
+extern ssize_t nsc_gpio_sysfs_set(struct device *dev,
+				  struct device_attribute *devattr,
+				  const char *buf, size_t count);
+
+/* GPIO pins have 5 attributes we care about: group them */
+struct gpio_pin_attributes {
+	struct sensor_device_attribute_2
+		value,
+		output_enabled,
+		totem_pole,
+		pullup_enabled,
+		debounced,
+		locked;
+};
+
+#define GPIO_ATTR(_grp, _idx, _pre, _post, _mode, _show, _store, _nr)	\
+	{	.dev_attr = __ATTR(_pre## _grp._idx ##_post,		\
+				   _mode, _show, _store),		\
+		.index = _idx, .nr = _nr }
+
+#define GPIO_PIN(GN, IN, FnSym, AttrNm)	\
+	GPIO_ATTR(GN, IN, bit_, AttrNm,	\
+		  S_IWUSR | S_IRUGO,	\
+		  nsc_gpio_sysfs_get, nsc_gpio_sysfs_set, FnSym )
+
+/* command alphabet - initializer components */
+#define PIN_VALUE(P,N)		GPIO_PIN(P, N, 'V', _value)
+#define PIN_OE(P,N)		GPIO_PIN(P, N, 'O', _output_enabled)
+#define PIN_PP(P,N)		GPIO_PIN(P, N, 'T', _totem)
+#define PIN_PUE(P,N)		GPIO_PIN(P, N, 'P', _pullup_enabled)
+#define PIN_LOCKED(P,N)		GPIO_PIN(P, N, 'L', _locked)
+#define PIN_DEBOUNCE(P,N)	GPIO_PIN(P, N, 'D', _debounced)
+
+/* initializer for ports; ie pin arrays */
+#define PIN_ATTRS(Port, Idx) {			\
+	.value		= PIN_VALUE(Port, Idx),	\
+	.output_enabled	= PIN_OE(Port, Idx),	\
+	.totem_pole	= PIN_PP(Port, Idx),	\
+	.pullup_enabled	= PIN_PUE(Port, Idx),	\
+	.locked		= PIN_LOCKED(Port, Idx), \
+	.debounced	= PIN_DEBOUNCE(Port, Idx), }
+
+
+/* whole-port interface */
+extern ssize_t nsc_gpio_port_sysfs_get(struct device *dev,
+				       struct device_attribute *devattr,
+				       char *buf);
+
+extern ssize_t nsc_gpio_port_sysfs_set(struct device *dev,
+				       struct device_attribute *devattr,
+				       const char *buf, size_t count);
+
+#define GPIO_PORT_ATTR(_grp,  _pre, _post, _mode, _show, _store, _nr)	\
+	{	.dev_attr = __ATTR(_pre## _grp ##_post,			\
+				   _mode, _show, _store),		\
+		.index = _grp, .nr = _nr }
+
+#define GPIO_PORT(GN, FnSym, AttrNm)			\
+	GPIO_PORT_ATTR(GN, port_, AttrNm,		\
+		       S_IWUSR | S_IRUGO,		\
+		       nsc_gpio_port_sysfs_get, 	\
+		       nsc_gpio_port_sysfs_set, FnSym )
+
+/* command alphabet - initializer components */
+#define PORT_VALUE(P)		GPIO_PORT(P, 'V', _value)
+#define PORT_OE(P)		GPIO_PORT(P, 'O', _output_enabled)
+#define PORT_PP(P)		GPIO_PORT(P, 'T', _totem)
+#define PORT_PUE(P)		GPIO_PORT(P, 'P', _pullup_enabled)
+#define PORT_LOCKED(P)		GPIO_PORT(P, 'L', _locked)
+#define PORT_DEBOUNCE(P)	GPIO_PORT(P, 'D', _debounced)
+
+/* initializer for ports; ie pin arrays */
+#define PORT_ATTRS(Port) {			\
+	.value		= PORT_VALUE(Port),	\
+	.output_enabled	= PORT_OE(Port),	\
+	.totem_pole	= PORT_PP(Port),	\
+	.pullup_enabled	= PORT_PUE(Port),	\
+	.locked		= PORT_LOCKED(Port),	\
+	.debounced	= PORT_DEBOUNCE(Port), }
+
diff -ruNp -X dontdiff -X exclude-diffs az-1/include/linux/scx200_gpio.h az-2/include/linux/scx200_gpio.h
--- az-1/include/linux/scx200_gpio.h	2006-06-18 13:14:51.000000000 -0600
+++ az-2/include/linux/scx200_gpio.h	2006-06-18 16:49:23.000000000 -0600
@@ -17,23 +17,33 @@ extern long scx200_gpio_shadow[2];
 
 /* returns the value of the GPIO pin */
 
-static inline int scx200_gpio_get(unsigned index) {
+static inline int scx200_gpio_port_get(unsigned index) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR + 0x04;
 	__SCx200_GPIO_INDEX;
-		
-	return (inl(ioaddr) & (1<<index)) ? 1 : 0;
+
+	return inl(ioaddr);
+}
+
+static inline int scx200_gpio_get(unsigned index)
+{
+	return (scx200_gpio_port_get(index) & (1<<index)) ? 1 : 0;
 }
 
 /* return the value driven on the GPIO signal (the value that will be
    driven if the GPIO is configured as an output, it might not be the
    state of the GPIO right now if the GPIO is configured as an input) */
 
-static inline int scx200_gpio_current(unsigned index) {
+static inline int scx200_gpio_port_current(unsigned index) {
         __SCx200_GPIO_BANK;
 	__SCx200_GPIO_INDEX;
 		
-	return (scx200_gpio_shadow[bank] & (1<<index)) ? 1 : 0;
+	return scx200_gpio_shadow[bank];
+}
+
+static inline int scx200_gpio_current(unsigned index)
+{
+	return (scx200_gpio_port_current(index) & (1<<index)) ? 1 : 0;
 }
 
 /* drive the GPIO signal high */
@@ -60,7 +70,7 @@ static inline void scx200_gpio_set_low(u
 
 /* drive the GPIO signal to state */
 
-static inline void scx200_gpio_set(unsigned index, int state) {
+static inline void scx200_gpio_set(unsigned index, u32 state) {
 	__SCx200_GPIO_BANK;
 	__SCx200_GPIO_IOADDR;
 	__SCx200_GPIO_SHADOW;
@@ -82,6 +92,34 @@ static inline void scx200_gpio_change(un
 	__SCx200_GPIO_OUT;
 }
 
+static inline void scx200_gpio_port_set(unsigned bank, u32 vals) {
+	__SCx200_GPIO_IOADDR;
+	__SCx200_GPIO_SHADOW;
+	*shadow = vals;
+	__SCx200_GPIO_OUT;
+}
+
+static inline void scx200_gpio_port_set_high(unsigned bank) {
+	__SCx200_GPIO_IOADDR;
+	__SCx200_GPIO_SHADOW;
+	*shadow = 0xFFFFFFFF;
+	__SCx200_GPIO_OUT;
+}
+
+static inline void scx200_gpio_port_set_low(unsigned bank) {
+	__SCx200_GPIO_IOADDR;
+	__SCx200_GPIO_SHADOW;
+	*shadow = 0;
+	__SCx200_GPIO_OUT;
+}
+
+static inline void scx200_gpio_port_change(unsigned bank) {
+	__SCx200_GPIO_IOADDR;
+	__SCx200_GPIO_SHADOW;
+	*shadow = ~(*shadow);
+	__SCx200_GPIO_OUT;
+}
+
 #undef __SCx200_GPIO_BANK
 #undef __SCx200_GPIO_IOADDR
 #undef __SCx200_GPIO_SHADOW

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Randy.Dunlap]

On Tue, 20 Jun 2006 13:57:21 -0600 Jim Cromie wrote:

> Andrew Morton wrote:
> > On Sat, 17 Jun 2006 12:42:28 -0600
> > Jim Cromie <jim.cromie@gmail.com> wrote:
> >
> > Fixup patches agains next -mm would be suitable.  Please keep them
> > super-short: basically one-patch-per-review-comment.  That way I can easily
> > instertion-sort the patches into place and we retain a nice patch series.
> >
> >   
> OK.  Just so Im clear, Ill patch against the tail of the set (ie -mm1), 
> and you'll push them forward into the

WTH?  "you'll" ??

> series as close as possible to where the blunder was made ?  (and less 
> close for conflicts )
> >> Ive
> >>     
> >
> > Apostrophe aversion?
> >   
> 
> Its PFL.  Its also IMO a trend in the language.  "Its" (the contraction) 
> has dropped the '
> IIUC, its (with the apostrophe) now only for possession, forex: Its 
> bigger than the sum of it's parts.
> Im just taking that to excess, everywhere its applicable ;-)

and you have it totally bassackwards too.  Maybe you meant to
do that, a la Rusty.

Possessive "its" did not and does not use apostrophe.
and "forex" is the Foreign Exchange market (or a brand of
prophylactic).

http://en.wikipedia.org/wiki/PFL ??
---
~Randy

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Jim Cromie]

Randy.Dunlap wrote:
> On Tue, 20 Jun 2006 13:57:21 -0600 Jim Cromie wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> On Sat, 17 Jun 2006 12:42:28 -0600
>>> Jim Cromie <jim.cromie@gmail.com> wrote:
>>>
>>> Fixup patches agains next -mm would be suitable.  Please keep them
>>> super-short: basically one-patch-per-review-comment.  That way I can easily
>>> instertion-sort the patches into place and we retain a nice patch series.
>>>
>>>   
>>>       
>> OK.  Just so Im clear, Ill patch against the tail of the set (ie -mm1), 
>> and you'll push them forward into the
>>     
>
> WTH?  "you'll" ??
>
>   

are we talking apostrophes here, or division of labor ?
If the latter, what have I missed ?
Andrew specifically said 'patch against next -mm', I intended to follow 
his instructions.

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Randy.Dunlap]

On Tue, 20 Jun 2006 14:40:49 -0600 Jim Cromie wrote:

> Randy.Dunlap wrote:
> > On Tue, 20 Jun 2006 13:57:21 -0600 Jim Cromie wrote:
> >
> >   
> >> Andrew Morton wrote:
> >>     
> >>> On Sat, 17 Jun 2006 12:42:28 -0600
> >>> Jim Cromie <jim.cromie@gmail.com> wrote:
> >>>
> >>> Fixup patches agains next -mm would be suitable.  Please keep them
> >>> super-short: basically one-patch-per-review-comment.  That way I can easily
> >>> instertion-sort the patches into place and we retain a nice patch series.
> >>>
> >>>   
> >>>       
> >> OK.  Just so Im clear, Ill patch against the tail of the set (ie -mm1), 
> >> and you'll push them forward into the
> >>     
> >
> > WTH?  "you'll" ??
> >
> >   
> 
> are we talking apostrophes here, or division of labor ?
> If the latter, what have I missed ?
> Andrew specifically said 'patch against next -mm', I intended to follow 
> his instructions.

apostrophes :)

and ISTM that you didn't follow his request.

---
~Randy

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Jim Cromie]

Randy.Dunlap wrote:
> On Tue, 20 Jun 2006 14:40:49 -0600 Jim Cromie wrote:
>
>   
>> Randy.Dunlap wrote:
>>     
>>> On Tue, 20 Jun 2006 13:57:21 -0600 Jim Cromie wrote:
>>>
>>>   
>>>       
>>>> Andrew Morton wrote:
>>>>     
>>>>         
>>>>> On Sat, 17 Jun 2006 12:42:28 -0600
>>>>> Jim Cromie <jim.cromie@gmail.com> wrote:
>>>>>
>>>>> Fixup patches agains next -mm would be suitable.  Please keep them
>>>>> super-short: basically one-patch-per-review-comment.  That way I can easily
>>>>> instertion-sort the patches into place and we retain a nice patch series.
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> OK.  Just so Im clear, Ill patch against the tail of the set (ie -mm1), 
>>>> and you'll push them forward into the
>>>>     
>>>>         
>>> WTH?  "you'll" ??
>>>
>>>   
>>>       
>> are we talking apostrophes here, or division of labor ?
>> If the latter, what have I missed ?
>> Andrew specifically said 'patch against next -mm', I intended to follow 
>> his instructions.
>>     
>
> apostrophes :)
>
>   
oops :)   I guess youll is just too visually grating

> and ISTM that you didn't follow his request.
>
> ---
> ~Randy
>
>   
are you referring to the replacement patch for 20/20 ?
The one that said RFC ??

heh - youre right, tho  :-}  I retract that patch.
Forgive the over-wound (and in retrospect, kinda stunned) newbie !

I'll spend some time making the Doc more coherent, and into an actual patch.
I guess I was hoping for some comments before doing this..

Whats a good name for this file?  (for purposes of focusing the tone and 
content)
     Doc/sysfs/gpio-interface ?  Doc/hwmon/gpio-sysfs-interface ?

thanks !
Jim Cromie

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Randy.Dunlap]

On Tue, 20 Jun 2006 15:50:51 -0600 Jim Cromie wrote:

> are you referring to the replacement patch for 20/20 ?
> The one that said RFC ??

Yes.

> heh - youre right, tho  :-}  I retract that patch.
> Forgive the over-wound (and in retrospect, kinda stunned) newbie !

Stunned by what?
Maybe you should skim over
  http://www.xenotime.net/linux/mentor/linux-mentoring-2006.pdf
<end plug>

> I'll spend some time making the Doc more coherent, and into an actual patch.
> I guess I was hoping for some comments before doing this..
> 
> Whats a good name for this file?  (for purposes of focusing the tone and 
> content)
>      Doc/sysfs/gpio-interface ?  Doc/hwmon/gpio-sysfs-interface ?

I like Documentation/hwmon/gpio-sysfs-interface, but if the driver(s)
don't live in drivers/hwmon/, then something like
Documentation/gpio/sysfs-interface makes sense to me.
I'd leave Doc/sysfs for sysfs details/internals.

---
~Randy

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Andrew Morton]

Jim Cromie <jim.cromie@gmail.com> wrote:
>
> Heres a better version

Well..

***************
*** 312,317 ****
  	}
  
  	pc8736x_init_shadow();
  	return 0;
  }
  
--- 422,428 ----
  	}
  
  	pc8736x_init_shadow();
+ 	pc8736x_sysfs_init(&pdev->dev);
  	return 0;
  }

I don't know how to fix that - pc8736x_gpio_init() doesn't call
pc8736x_init_shadow().

Plus the patch you sent still has spaces all over the place where we should
have tabs.  You can fix that by editing the diff, search for "^+   ".

So I'll drop this patch altogether.  Which is probably for the best at this
stage, because we're expecting a pile of teeny fixes from you agaisnt the
other 19 patches as a result of review comments (yes?)

Once all that has landed we can look at the sysfs-GPIO interface.

Re: [patch -mm 20/20 RFC] chardev: GPIO for SCx200 & PC-8736x: add sysfs-GPIO interface

[Randy.Dunlap]

On Sat, 17 Jun 2006 12:42:28 -0600 Jim Cromie wrote:

> Ok, 
> heres the brand-spanking-new proto-sysfs-gpio interface,
> preceded by some pseudo/proto-Documentation.
> 
> 
> We need a standard rep for GPIO in sysfs, so heres a strawman.
> Strike a match, lets have a campfire!
> 
> Essentially, this seeks to describe the directory of
> 'device-attribute-files' that are populated by a driver
> 
> All device-attr-files are named as <prefix>_<id>_<suffix>
> 
> in LM-sensors:
> - prefix        sensor-type: in(volts), temp, fan, etc. (no trailing '_')
> - id            usually single integer
> - suffix        the sensor attribute in question.
> 
> 
> GPIO-sysfs Prefix Names.
> 
> Basically, GPIO hardware design appears to have 2 top-level factors;
> pin features, and pin-to-port grouping.  These are mapped onto
> filename prefixes & suffixes.
> 
> All GPIOs (Ive seen) are organized as 1+ ports of 8-32 bits.  The
> bits' attributes are addressable individually, but are also accessible
> as a group via the port_* files.  If you change a bit-attribute, that
> change will also exhibit in the port attr too.

Drop one of "also" and "too".  (redundant)

> IOW, we have bit_*, port_*.  They are interconnected at the hardware
> level, and (I think) there is no need for inter-locks between the
> sysfs handlers for bit_ and port_ (except for shadow regs, but I
> digress)

add period ('.')

> In fact, it might be nice to have the option of not creating the bit_*
> sysfs-device-files.  For apps where user-code is doing its own
> bit-masking, the kernel could avoid some unused overheads.  OTOH, this
> might be silly, premature, overoptimization.  This would be controlled
> - assuming its worthwhile - by a modparam; 'nobits' or 'portsonly'
> 
> 
> GPIO Architectures
> 
> GPIO pins have lots of hardware / architectural / naming-convention
> variations, which makes this harder.  My simplifying assumption is
> that drivers should reflect the hardware capabilities directly (or
> very nearly so), and push the abstraction to user-space (at least in
> part).  Obviously this needs

needs completing....

> Drivers should create sysfs 'files' only for attributes that are
> pertinent for the hardware being driven.  Ths way, the absense or
s/Ths/This/
s/absense/absence/

> presense of files communicates functionality, as does their
s/presense/presence/

> readonlyness.  (these 'behaviors' may be different than lm-sensors)

read-only-ness IMO.
s/these/These/
s/than/from/
add a period at end.

> Forex:

Use "For example:" or "E.g.:".

> if a pin is input  only, it shouldnt have an _output_enabled attr.
> if a pin is output only, it shouldnt have an _output_enabled attr.

s/output_enabled/input_enabled/

> This way, `ls` tells you that a particular port/bit cannot possibly
> drive a value out of the chip.
> 
> - one of several different values (otherwize why show it ?
>   After all, you dont to be told that PI=3.14159...)

Some lines were rearranged ??

> - changed.  if a pin cannot be output, theres nothing to enable, and
>   showing the attribute is confusing.
> 
> OTOH, a readonly _output_enabled would also convey info.

Confusing lines...

> yield the same, but not as
> visibly (ls vs ls -l)
> 
> So, Im somewhat ambivalent here, looking for input....

You are doing pretty well here, with the exception of the
aversion to tic marks.

> User-Space
> 
> Following LM-sensors approach, a user-side library would add the
> niceties:
> 
> - provide any equivalences needed by users
>   ie bit_x_tristate = ! bit_x_output_enabled.

I strongly prefer "i.e." to "ie".

> - sub-port allocation and management.
>   support for 3+3+2 bit sub-ports on an 8 bit port would be nice
> 
> I suspect that a sophisticated programmer would be able to add a
> sub-port allocation facility w/in the driver.  I cannot,

s/,/./

> GPIO Pin Features
> 
> As outlined above, pin features are represented as _<suffixes>
> 
> 1st: there are several alternative naming schemes:
> 
> - name-as-verb          _output_enable          (conveys an 'action')
> - name-as-state         _output_enabled         (conveys a 'current state')
> - feature-name          _output                 (a knob to turn)
> - feature+state         _output+(currval)       (currval in name is bad idea)
> 
> 1,2 are quite close.  Ive done 2.

I can't tell which choices above are 1, 2, etc.

> FWIW, heres the pin attributes of my GPIOs, as expressed in the syslog
> by the legacy drivers: (these are

??

> [15510.384000]  pc8736x_gpio.0: io16: 0x0004 TS OD PUE  EDGE LO         io:0/1
> [15510.564000]  pc8736x_gpio.0: io17: 0x0004 TS OD PUE  EDGE LO         io:1/1
> [15510.744000]  pc8736x_gpio.0: io18: 0x0004 TS OD PUE  EDGE LO         io:1/1
> [15510.928000]  pc8736x_gpio.0: io19: 0x0004 TS OD PUE  EDGE LO         io:1/1

More confusing interspersed lines.

> # whether output-drive is on/off
> _output_enable          # 1 or 0,
> _tristate               # ! _output_enable, logically linked.
> 
> Now, theres no need to have both of these; if there were, they would
> have to be intrinsically linked (logically opposite values).
> 
> IOW, drivers should name the file as one of possible states of the
> feature, which ever best describes it, and not expose it 2x.
> 
> To the extent that we need support for '_tristate' version of a
> '_output_enabled' sysfs-file, user-space (libraries) should provide
> that support.
> 
> # output circuit configuration
> _opendrain              # only 1 transistor, can sink current from pin
> _totem                  # has 2nd transistor, can drive pin hi.
> _pushpull               # alias for _totempole
> 
> Ive chosen _totem as the attribute name
> 
> _pullup_enabled         # pin tied to power via resistor.
> _pullup_off             # duh
> _pullup_no              # how many aliases ?
> 
> _debounce               # present if supported, 0 if off, 1 if on.
> 
> It kinda works, but the pullup is a bit ugly, and all the aliases
> suggest some semantic difficulty/mismatch/incompleteness, but adding
> them all definitely creates clutter and has reached diminished
> incremental value.
> 
> 
> If hardware doesnt support a feature, like _opendrain, it:
> 
>  - sets _pushpull to 1, readonly ?
>  - sets _opendrain to 0, readonly ?
>     OR never creates _opendrain ?
> 
> Doing either of these works to communicate the feature-set to
> user-space, but not creating _opendrain when pin doesnt do it means
> that the file's presense communicates this; IOW, user issues 'ls', not

s/presense/presence/

> 'ls -l' to find out.
> 
> (continuing strawman)
> 
> _value          # read the pin
> (no-suffix)     # alias for _value
> 
> _current        # the value 'driven' by the pin (last written)
> 
> And here we can see some potential (user) difficulties;
> under some conditions,
> - read-value = current-value
> 
> but not on these:
> - pin is input-only/tristate - (current is irrelevant, except as 'state')
> - pin is over-driven by attached circuit
> -- pin cannot sink/supply sufficient current
> 
> Detecting these situations is both hardware and circuit dependent, and
> properly belongs in user-space.  It sounds a lot like what lm-sensors
> does already.
> 
> For the 2 drivers Ive 'experienced', pin control was via device-file,
> with this command-set.  Presumably the correspondence with the sysfs
> strawman above is obvious.
> 
> case 'O':        output enabled
> case 'o':        output disabled
> case 'T':        output is push pull
> case 't':        output is open drain
> case 'P':        pull up enabled
> case 'p':        pull up disabled
> 
> 
> Port Organization.
> 
> My *vast* experience (with 1.5 GPIO architectures) suggests that all
> chips organize their GPIOs into one or more ports.  Each port supports
> reading and writing all bits simultaneously.
> 
> Some hardware also supports reading/writing pin-properties like
> output-enable in a single-word (todo-research).  Drivers for these
> hardwares could/should create attributes for each pin-property that is
> accessible as a bit-vector.
> 
> Further, port (and pin) capabilities generally vary by port; hardware
> will typically put a full set of features on 1 port, and less on
> others, expecting a designer to allocate functions to pins
> accordingly.  Forex, on the pc8736x chip, port 0 can issue interrupts,

not "Forex".

> so those pins should have extra properties.
> 
> These capabilities must be cleanly representable in any worthwhile
> sysfs/GPIO model (and we continue to test this strawman)..
> 
> 
> Port-names and Pin-names
> 
> # prefixes (note the trailing _)
> port_[0..P]_
> bit_[P]_[0..bits-per-word]_
> 
> Getting past the port/bit names, these files are populated by the
> driver according to the device.  For the 2 drivers Ive touched, heres
> the table:
> 
> driver:         ports   bits-per-port
> scx200_gpio     1       32
> pc8736x_gpio    4       8
> 
> 
> Strawman tie-together:
> 
> bit_0_0_output_enable   # shows current output-drive of port 0 bit 0
> bit_0_0_value
> bit_0_0                 # 2 reads of same bit
> 
> # lessee what happens :->

Let's see ...

> port_0_value_bin        # 1-4 bytes typically returned (depending on device)
> port_0_value_hex        # converted to human readable, always printable
> port_0_value            #
> 
> port_1_output_enable    # read/write vector of enable bits to port
> port_1_<suffix_set>     #
> 
> 
> The driver should know which properties are readable/writable in a
> bit-vector basis, and expose those sysfs-attributes only.  Thus the
> presense of the port_N_value* attrs implies that all the bits in that
> port are readable at once.
> 
> If the driver doesnt expose forex: port_1_output_enable, user-space is

s/forex/e.g./

> free to loop over each bit, in essence 'emulating' the port-wide
> operation.
> 
> 
> RESTATING - whats above kinda hangs together.
> 
> NEXT - muddles
> 
> pin_XY_output_state     # one-of( 'output_enable', 'tristate')
> 
> This might be convenient for some situations, but probably is needless
> complication / obfuscation.
> 
> 
> pin_XY_state_bin                # binary state reader
> 
> This is intended an 'escape-valve' for things that are turn out to be

s/an/as an/
s/that are/that/

> cumbersome with the above.  This is probably tantamount to an IOCTL,
> so might be a hugely bad idea.
> 
> 
> pin_XY_interrupt_enable        #
> pin_XY_interrupt_trigger_edge
> pin_XY_interrupt_trigger_level
> pin_XY_interrupt_trigger_edge_rising
> pin_XY_interrupt_trigger_edge_falling
> pin_XY_interrupt_trigger_level_hi
> pin_XY_interrupt_trigger_level_lo
> 
> Well - thats a big one - Do we expose any of this ?
> - the ability to enable / disable / control hardware interrupt
> - or is that insane meddling in such affairs ?
> 
> We cant afterall allow mapping of the actual interrupt handler, that
> does sound insane (unless hugely carefull)

s/carefull/careful/

> With the new genirq architecture, things are apparently more
> orthogonal, which suggests there might be something to control by
> means of attributes such as above.
> 
> Further, any of the above attributes could readily be RO; they would
> convey info thats at least useful, even if 'control' is too exposing.
> 
> Forex, somewhere during boot, the APIC is setup to handle interrupts;

s/Forex/E.g./

> at this point its probably known what the configured state of all
> interrrupts is, and this info could be exposed here.  Whether that has
> sufficient value is unclear, and certainly not for v.submit-1
> 
> 
> when a pin is level-triggered (presumably this can be
> determined early in the boot process, as soon as APIC etc would be
> 

eh?

> 
> setup), the _edge_* attributes vanish, and the _level_{hi,lo} attrs
> are set 1/0, and RO.
> 
> 
> OK - IM DONE.
> 
> Please be liberal with feedback -
> - this is wrong
> - poorly explained -
> - correct ... - boil this down - reduce to a guiding statement
> - strip out conjectures

I can't tell which parts of this are the documented interface
and which parts are you just thinking in bits/bytes, so I made
corrections to all of it...

I would say that you need to make a proposal and see what feedback
(if any) you get on it.

And I'm not in the right group of people to tell you how GPIO
pins should be represented.


[deleted some source files]

---
~Randy