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

[Andrew Morton <akpm@osdl.org>]

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?