[PATCH] IBM GPIO driver for PowerPC 4xx is back from the dead

Arnd Bergmann arnd.bergmann at de.ibm.com
Fri Sep 29 19:06:19 EST 2006


On Friday 29 September 2006 10:29, Jean-Baptiste Maneyrol wrote:
> diff -Naur linux-2.6.18_gpio/drivers/char/ibm_gpio.c tlgate_gpio/drivers/char/ibm_gpio.c
> --- linux-2.6.18_gpio/drivers/char/ibm_gpio.c   1970-01-01 01:00:00.000000000 +0100
> +++ tlgate_gpio/drivers/char/ibm_gpio.c 2006-09-28 16:46:42.000000000 +0200
> @@ -0,0 +1,349 @@
> +/*
> + * FILE NAME ibm_gpio.c

<insert usual comment about removing the file name from the file>

> + * BRIEF MODULE DESCRIPTION
> + *  API for IBM PowerPC 4xx GPIO device.
> + *  Driver for IBM PowerPC 4xx GPIO device.
> + *
> + *  Armin Kuster akuster at pacbell.net
> + *  Sept, 2001
> + *
> + *  Orignial driver
> + *  Author: MontaVista Software, Inc.  <source at mvista.com>
> + *          Frank Rowand <frank_rowand at mvista.com>
> + *          Debbie Chu   <debbie_chu at mvista.com>
> + *
> + * Copyright 2000,2001,2002 MontaVista Software Inc.

Any new copyright year?


> + *     TODO: devfs

surely not.

> + *     Version: 02/01/12 - Armin
> + *                      converted to ocp and using ioremap
> + *
> + *     1.2 02/21/01 - Armin
> + *             minor compiler warning fixes
> + *
> + *     1.3 02/22/01 - Armin
> + *             added apm
> + *
> + *     1.4 05/07/02 - Armin/David Mueller
> + *             coverted to core_ocp[];
> + *
> + *     1.5 05/25/02 - Armin
> + *      name change from *_driver to *_dev
> + *
> + *     1.6 06/04/02 - Matt Porter
> + *     ioremap paddr. Comment as 4xx generic driver.
> + *     Fix header to be userland safe and locate in
> + *     an accessible area.  Add ioctl to configure
> + *     multiplexed GPIO pins.
> + *
> + *     1.7 07/25/02 - Armin
> + *     added CPM to enable/disable in init/exit

kill that changelog

> + */
> +
> +#define VUFX "07.25.02"

should be MODULE_VERSION()

> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/init.h>
> +#include <linux/ioctl.h>
> +#include <linux/fs.h>
> +#include <asm/ibm_gpio.h>
> +#include <asm/uaccess.h>
> +#include <asm/io.h>
> +#include <asm/machdep.h>
> +#include <asm/ocp.h>
> +#include <asm/ibm4xx.h>
> +
> +struct miscdevice ibm_gpio_miscdev;
> +static struct gpio_regs *gpiop;
> +
> +int
> +ibm_gpio_config(__u32 device, __u32 mask, __u32 data)

any symbols should be static. Function arguments are not
visible to users, so they should be 'u32' instead of '__u32'.

> +{
> +       u32 cfg_reg;
> +
> +       if (device != 0)
> +               return -ENXIO;
> +
> +#ifdef CONFIG_40x
> +#ifdef DCRN_CHCR0
> +       /*
> +        * PPC405 uses CPC0_CR0 to select multiplexed GPIO pins.
> +        */
> +       cfg_reg = mfdcr(DCRN_CHCR0);
> +       cfg_reg = (cfg_reg & ~mask) | (data & mask);
> +       mtdcr(DCRN_CHCR0, cfg_reg);
> +#endif
> +#elif CONFIG_440GP
> +       /*
> +        * PPC440GP uses CPC0_GPIO to select multiplexed GPIO pins.
> +        */
> +       cfg_reg = mfdcr(DCRN_CPC0_GPIO);
> +       cfg_reg = (cfg_reg & ~mask) | (data & mask);
> +       mtdcr(DCRN_CPC0_GPIO, cfg_reg);
> +#elif CONFIG_440GX
> +       /*
> +        * PPC440GX uses SDR0_PFC0 to select multiplexed GPIO pins
> +        */
> +       cfg_reg = SDR_READ(DCRN_SDR_PFC0);
> +       cfg_reg = (cfg_reg & ~mask) | (data & mask);
> +       SDR_WRITE(DCRN_SDR_PFC0, cfg_reg);
> +#else
> +#error This driver is only supported on PPC40x and PPC440 CPUs
> +#endif

This prevents building a single kernel for multiple 440 version.
Please use a run-time check, or better get the necessary information
from the device tree.

> +int
> +ibm_gpio_in(__u32 device, __u32 mask, volatile __u32 * data)
> +{
> +       if (device != 0)
> +               return -ENXIO;
> +       gpiop->tcr = gpiop->tcr & ~mask;
> +       eieio();
> +
> +       /*
> +          ** If the previous state was OUT, and gpiop->ir is read once, then the
> +          ** data that was being OUTput will be read.  One way to get the right
> +          ** data is to read gpiop->ir twice.
> +        */
> +
> +       *data = gpiop->ir;
> +       *data = gpiop->ir & mask;
> +       eieio();
> +       return 0;
> +}

Don't just assign *data, make that an __iomem pointer and use
an appropriate accessor function (out_be32 or such).


> +
> +static int
> +ibm_gpio_open(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}
> +
> +static int
> +ibm_gpio_release(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}

You don't need these.


> +static int
> +ibm_gpio_ioctl(struct inode *inode, struct file *file,
> +              unsigned int cmd, unsigned long arg)
> +{
> +       static struct ibm_gpio_ioctl_data ioctl_data;
> +       int status;
> +
> +       switch (cmd) {
> +       case IBMGPIO_IN:
> +               if (copy_from_user(&ioctl_data,
> +                                  (struct ibm_gpio_ioctl_data *) arg,
> +                                  sizeof (ioctl_data))) {
> +                       return -EFAULT;
> +               }

Just do the copy_from_user() once, in the beginning here, instead
of each case.
> +
> +static int __init
> +ibm_gpio_probe(struct ocp_device *ocp)
> +{
> +       ibm_gpio_miscdev.minor = GPIO_MINOR;
> +       ibm_gpio_miscdev.name = ocp->name;
> +       ibm_gpio_miscdev.fops = &ibm_gpio_fops;
> +       misc_register(&ibm_gpio_miscdev);
> +
> +       if (!request_mem_region(ocp->def->paddr, sizeof(struct gpio_regs), "ibm_gpio"))
> +               return -EBUSY;
> +       
> +       gpiop = (struct gpio_regs *) ioremap(ocp->def->paddr,
> +                       sizeof(struct gpio_regs));
> +       if (!gpiop) {
> +               release_mem_region(ocp->def->paddr, sizeof(struct gpio_regs));
> +               return -ENOMEM;
> +       }
> +       
> +       printk("GPIO #%d at 0x%lx\n", ocp->def->index,
> +                       (unsigned long) gpiop);
> +
> +       return 0;
> +}
> +
> +static void __exit
> +ibm_gpio_remove(struct ocp_device *ocp)
> +{
> +       misc_deregister(&ibm_gpio_miscdev);
> +
> +       iounmap(gpiop);
> +       gpiop = NULL;
> +       
> +       release_mem_region(ocp->def->paddr, sizeof(struct gpio_regs));
> +}
> +
> +static struct ocp_device_id ibm_gpio_ids[] __devinitdata =
> +{
> +       { .vendor = OCP_VENDOR_IBM, .function = OCP_FUNC_GPIO },
> +       { .vendor = OCP_VENDOR_INVALID }
> +};
> +
> +MODULE_DEVICE_TABLE(ocp, ibm_gpio_ids);
> +
> +static struct ocp_driver ibm_gpio_driver =
> +{
> +       .name           = "ibm_gpio",
> +       .id_table       = ibm_gpio_ids,
> +       .probe          = ibm_gpio_probe,
> +       .remove         = __devexit_p(ibm_gpio_remove),
> +#if defined(CONFIG_PM)
> +       .suspend        = NULL,
> +       .resume         = NULL,
> +#endif
> +};
> +
> +static int __init
> +ibm_gpio_init(void)
> +{
> +       printk("IBM GPIO driver version %s\n", VUFX);
> +       return ocp_register_driver(&ibm_gpio_driver);
> +}
> +
> +static void __exit
> +ibm_gpio_exit(void)
> +{
> +       ocp_unregister_driver(&ibm_gpio_driver);
> +}

ocp will die a painful death soon, when 440 has been converted over
to use the device tree and of_device.

> +EXPORT_SYMBOL(ibm_gpio_tristate);
> +EXPORT_SYMBOL(ibm_gpio_open_drain);
> +EXPORT_SYMBOL(ibm_gpio_in);
> +EXPORT_SYMBOL(ibm_gpio_out);

Why export these at all? They don't seem to be used elsewhere?

If you need to export them, make it EXPORT_SYMBOL_GPL and put that
line directly under the functions.

> +
> +struct ibm_gpio_ioctl_data {
> +        __u32 device;
> +        __u32 mask;
> +        __u32 data;
> +};
> +
> +#define IBMGPIO_IOCTL_BASE     'Z'
> +
> +#define IBMGPIO_IN             _IOWR(IBMGPIO_IOCTL_BASE, 0, struct ibm_gpio_ioctl_data)
> +#define IBMGPIO_OUT            _IOW (IBMGPIO_IOCTL_BASE, 1, struct ibm_gpio_ioctl_data)
> +#define IBMGPIO_OPEN_DRAIN     _IOW (IBMGPIO_IOCTL_BASE, 2, struct ibm_gpio_ioctl_data)
> +#define IBMGPIO_TRISTATE       _IOW (IBMGPIO_IOCTL_BASE, 3, struct ibm_gpio_ioctl_data)
> +#define IBMGPIO_CFG            _IOW (IBMGPIO_IOCTL_BASE, 4, struct ibm_gpio_ioctl_data)

Is that the same ioctl interface as for the other gpio drivers?

> +#endif



More information about the Linuxppc-embedded mailing list