Message-ID: <20090120214832.GE3027@local>
Date:	Tue, 20 Jan 2009 22:48:33 +0100
From:	"Hans J. Koch" <hjk@...utronix.de>
To:	Brandon Philips <brandon@...p.org>
Cc:	hjk@...utronix.de, Greg KH <gregkh@...e.de>,
	linux-kernel@...r.kernel.org
Subject: Re: uio: add the uio_aec driver

On Tue, Jan 20, 2009 at 12:47:29PM -0800, Brandon Philips wrote:
> UIO driver for the Adrienne Electronics Corporation PCI time code
> device.
> 
> This device differs from other UIO devices since it uses I/O ports instead of
> memory mapped I/O. In order to make it possible for UIO to work with this
> device a utility, uioport, can be used to read and write the ports.

We just added a feature to UIO that allows you to pass information about
such I/O ports to userspace. In struct uio_info, there's now also a port[]
array for that purpose. In your case, you will probably want to set the
the porttype member of struct uio_port to UIO_PORT_X86.

The portio feature is available since .29-rc1, please use it.

Otherwise, your driver looks quite good, I'd say.
I've added a few remarks, though.

Thanks,
Hans

> 
> uioport is designed to be a setuid program and checks the permissions of
> the /dev/uio* node and if the user has write permissions it will use
> iopl and out*/in* to access the device.

What happens with PCI on PowerPC ?

> 
> [1] git clone git://ifup.org/philips/uioport.git
> 
> Signed-off-by: Brandon Philips <brandon@...p.org>
> 
> ---
>  drivers/uio/Kconfig        |   18 ++++
>  drivers/uio/Makefile       |    1 
>  drivers/uio/uio_aec.c      |  177 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/uio_driver.h |    1 
>  4 files changed, 197 insertions(+)
> 
> Index: linux-2.6/drivers/uio/Makefile
> ===================================================================
> --- linux-2.6.orig/drivers/uio/Makefile
> +++ linux-2.6/drivers/uio/Makefile
> @@ -3,4 +3,5 @@ obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
>  obj-$(CONFIG_UIO_PDRV)	+= uio_pdrv.o
>  obj-$(CONFIG_UIO_PDRV_GENIRQ)	+= uio_pdrv_genirq.o
>  obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
> +obj-$(CONFIG_UIO_AEC)	+= uio_aec.o
>  obj-$(CONFIG_UIO_SERCOS3)	+= uio_sercos3.o
> Index: linux-2.6/include/linux/uio_driver.h
> ===================================================================
> --- linux-2.6.orig/include/linux/uio_driver.h
> +++ linux-2.6/include/linux/uio_driver.h
> @@ -91,5 +91,6 @@ extern void uio_event_notify(struct uio_
>  #define UIO_MEM_PHYS	1
>  #define UIO_MEM_LOGICAL	2
>  #define UIO_MEM_VIRTUAL 3
> +#define UIO_MEM_PORT	4

As I explained above, this is not needed anymore. The reason we don't want
this: It breaks generic userspace tools that rely on the fact that any
memory found underneath the maps/ directory in sysfs can be mapped.

In case of ports we don't have anything to be mapped, we simply want to
pass information to userspace. That justifies having a different sysfs
directory for it.

>  
>  #endif /* _LINUX_UIO_DRIVER_H_ */
> Index: linux-2.6/drivers/uio/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/uio/Kconfig
> +++ linux-2.6/drivers/uio/Kconfig
> @@ -58,6 +58,24 @@ config UIO_SMX
>  
>  	  If you compile this as a module, it will be called uio_smx.
>  
> +config UIO_AEC
> +	tristate "AEC video timestamp device"
> +	depends on PCI
> +	default n
> +	help
> +
> +	  UIO driver for the Adrienne Electronics Corporation PCI time
> +	  code device.
> +
> +	  This device differs from other UIO devices since it uses I/O
> +	  ports instead of memory mapped I/O. In order to make it
> +	  possible for UIO to work with this device a utility, uioport,
> +	  can be used to read and write the ports:
> +
> +	    git clone git://ifup.org/philips/uioport.git
> +
> +	  If you compile this as a module, it will be called uio_aec.
> +
>  config UIO_SERCOS3
>  	tristate "Automata Sercos III PCI card driver"
>  	default n
> Index: linux-2.6/drivers/uio/uio_aec.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6/drivers/uio/uio_aec.c
> @@ -0,0 +1,177 @@
> +/*
> + * uio_aec.c -- simple driver for Adrienne Electronics Corp time code PCI device
> + *
> + * Copyright (C) 2008 Brandon Philips <brandon@...p.org>
> + *
> + *   This program is free software; you can redistribute it and/or modify it
> + *   under the terms of the GNU General Public License version 2 as published
> + *   by the Free Software Foundation.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License along
> + *   with this program; if not, write to the Free Software Foundation, Inc., 59
> + *   Temple Place, Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <asm/system.h>

please put this <asm/...> include under the <linux/...> includes, separated by
one blank line. Makes it easier to find. BTW, do you actually need this one?

> +#include <linux/uaccess.h>
> +#include <linux/uio_driver.h>
> +
> +#define PCI_VENDOR_ID_AEC 0xaecb
> +#define PCI_DEVICE_ID_AEC_VITCLTC 0x6250
> +
> +#define INT_ENABLE_ADDR		0xFC
> +#define INT_ENABLE		0x10
> +#define INT_DISABLE		0x0
> +
> +#define INT_MASK_ADDR		0x2E
> +#define INT_MASK_ALL		0x3F
> +
> +#define INTA_DRVR_ADDR		0xFE
> +#define INTA_ENABLED_FLAG	0x08
> +#define INTA_FLAG		0x01
> +
> +#define MAILBOX			0x0F
> +
> +static struct pci_device_id ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_AEC, PCI_DEVICE_ID_AEC_VITCLTC), },
> +	{ 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, ids);
> +
> +static irqreturn_t aectc_irq(int irq, struct uio_info *dev_info)
> +{
> +	void __iomem *int_flag = dev_info->mem[0].internal_addr
> +					+ INTA_DRVR_ADDR;
> +	unsigned char status = ioread8(int_flag);
> +
> +
> +	if ((status & INTA_ENABLED_FLAG) && (status & INTA_FLAG)) {
> +		/* application writes 0x00 to 0x2F to get next interrupt */
> +		status = ioread8(dev_info->mem[0].internal_addr + MAILBOX);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static void print_board_data(struct uio_info *i)
> +{
> +	printk(KERN_INFO "PCI-TC board vendor: %x%x number: %x%x"
> +		" revision: %c%c\n",
> +		ioread8(i->mem[0].internal_addr + 0x01),
> +		ioread8(i->mem[0].internal_addr + 0x00),
> +		ioread8(i->mem[0].internal_addr + 0x03),
> +		ioread8(i->mem[0].internal_addr + 0x02),
> +		ioread8(i->mem[0].internal_addr + 0x06),
> +		ioread8(i->mem[0].internal_addr + 0x07));
> +}
> +
> +static int __devinit probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> +	struct uio_info *info;
> +	int ret;
> +
> +	info = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	if (pci_enable_device(dev))
> +		goto out_free;
> +
> +	if (pci_request_regions(dev, "aectc"))
> +		goto out_disable;
> +
> +	info->name = "aectc";
> +	info->mem[0].addr = pci_resource_start(dev, 0);
> +	if (!info->mem[0].addr)
> +		goto out_release;
> +	info->mem[0].internal_addr = pci_iomap(dev, 0, 0);
> +	if (!info->mem[0].internal_addr)
> +		goto out_release;
> +	info->mem[0].size = pci_resource_len(dev, 0);
> +	info->mem[0].memtype = UIO_MEM_PORT;

port[] instead of mem[].

> +
> +	info->version = "0.0.1";
> +	info->irq = dev->irq;
> +	info->irq_flags = IRQF_DISABLED | IRQF_SHARED;
> +	info->handler = aectc_irq;
> +
> +	print_board_data(info);
> +	ret = uio_register_device(&dev->dev, info);
> +	if (ret)
> +		goto out_unmap;
> +
> +	iowrite32(INT_ENABLE, info->mem[0].internal_addr + INT_ENABLE_ADDR);
> +	iowrite8(INT_MASK_ALL, info->mem[0].internal_addr + INT_MASK_ADDR);
> +	if (ioread8(info->mem[0].internal_addr + INTA_DRVR_ADDR)
> +			& INTA_ENABLED_FLAG)
> +		printk(KERN_INFO "aectc: interrupts successfully enabled\n");

I'd find it better if you printed a message in case of failure...
And please use dev_err() etc. instead of printk().

BTW, if this test fails, are you sure you can continue and let probe()
succeed?

> +
> +	pci_set_drvdata(dev, info);
> +
> +	return 0;
> +
> +out_unmap:
> +	pci_iounmap(dev, info->mem[0].internal_addr);
> +out_release:
> +	pci_release_regions(dev);
> +out_disable:
> +	pci_disable_device(dev);
> +out_free:
> +	kfree(info);
> +	return -ENODEV;
> +}
> +
> +static void remove(struct pci_dev *dev)
> +{
> +	struct uio_info *info = pci_get_drvdata(dev);
> +
> +	/* disable interrupts */
> +	iowrite8(INT_DISABLE, info->mem[0].internal_addr + INT_MASK_ADDR);
> +	iowrite32(INT_DISABLE, info->mem[0].internal_addr + INT_ENABLE_ADDR);
> +	/* read mailbox to ensure board drops irq */
> +	ioread8(info->mem[0].internal_addr + MAILBOX);
> +
> +	uio_unregister_device(info);
> +	pci_release_regions(dev);
> +	pci_disable_device(dev);
> +	pci_set_drvdata(dev, NULL);
> +	iounmap(info->mem[0].internal_addr);
> +
> +	kfree(info);
> +}
> +
> +static struct pci_driver pci_driver = {
> +	.name = "aectc",
> +	.id_table = ids,
> +	.probe = probe,
> +	.remove = remove,
> +};
> +
> +static int __init aectc_init(void)
> +{
> +	return pci_register_driver(&pci_driver);
> +}
> +
> +static void __exit aectc_exit(void)
> +{
> +	pci_unregister_driver(&pci_driver);
> +}
> +
> +MODULE_LICENSE("GPL");
> +
> +module_init(aectc_init);
> +module_exit(aectc_exit);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists