Re: [PATCH 02/47] Add new driver files for Artpec-3.
On Thu, Dec 13, 2007 at 02:30:33PM -0800, David Brownell wrote:
> This is needlessly arch-specific though... some other recent platform updates
> have provided the Documentation/gpio.txt interfaces and plan to update them
> to use the new gpiolib infrastructure when that hits mainline. That gives a
> simpler and more extensible approach to I2C (or SPI, etc) GPIO expanders.
Yes, that might simplify things a lot, I'll take a look at that.
> I'd guess this talks to a pcf8575-ish GPIO expander (16 bits) though the
> code rather lacks relevant comments...
Only the "virtual GPIO" talks to an extender, the rest of the GPIO
uses direct access to memory registers for the ARTPEC-3 (and EtraxFS).
> The GPIO feature here that's not yet in mainline is a userspace interface.
> I think that gpiolib will make it a lot easier to offer one of those, since
> it maintains a registry that didn't previously exist.
>
> I'm a bit surprised that this doesn't seem to offer kernel interfaces for
> GPIOs. There seems to be no way for drivers to use them for input/output
> (other than direct register access, which won't work with e.g. leds-gpio
> or i2c-gpio) or request_irq(...) for something coming from a GPIO.
This is probably due to the old way to handle leds etc was precisely via
register access, and the driver uses those macros to extend the functions
to userspace.
> The LED stuff, and PWM support, should IMO be managed separately.
I suspect that the reason they're handled together is that the same
hardware module implements all three functions.
> We have
> a LED framework that's preferable to this stuff.
Ok, I'll have a look at that to see if we can use it instead.
> PWM is a different issue;
> I have a hard time imagining a good generic PWM framework, but maybe that's
> because I don't use PWM for anything interesting (like motor control), just
> for simple LED/backlight brightness control.
...and motor control is exactly what we use them for.
> - Dave
Thanks for the comments, I'll see what improvements I can come up with.
/Jesper
> > --- /dev/null
> > +++ b/arch/cris/arch-v32/drivers/mach-a3/gpio.c
> > @@ -0,0 +1,984 @@
> > +/*
> > + * Artec-3 general port I/O device
> > + *
> > + * Copyright (c) 2007 Axis Communications AB
> > + *
> > + * Authors:Bjorn Wesen (initial version)
> > + * Ola Knutsson (LED handling)
> > + * Johan Adolfsson (read/set directions, write, port G,
> > + * port to ETRAX FS.
> > + * Ricard Wanderlof (PWM for Artpec-3)
> > + *
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> > +#include "../i2c.h"
> > +
> > +#define VIRT_I2C_ADDR 0x40
> > +#endif
> > +
> > +/* The following gio ports on ARTPEC-3 is available:
> > + * pa 32 bits
> > + * pb 32 bits
> > + * pc 16 bits
> > + * each port has a rw_px_dout, r_px_din and rw_px_oe register.
> > + */
> > +
> > +#define GPIO_MAJOR 120 /* experimental MAJOR number */
> > +
> > +#define I2C_INTERRUPT_BITS 0x300 /* i2c0_done and i2c1_done bits */
> > +
> > +#define D(x)
> > +
> > +#if 0
> > +static int dp_cnt;
> > +#define DP(x) \
> > + do { \
> > + dp_cnt++; \
> > + if (dp_cnt % 1000 == 0) \
> > + x; \
> > + } while (0)
> > +#else
> > +#define DP(x)
> > +#endif
> > +
> > +static char gpio_name[] = "etrax gpio";
> > +
> > +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> > +static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg);
> > +#endif
> > +static int gpio_ioctl(struct inode *inode, struct file *file,
> > + unsigned int cmd, unsigned long arg);
> > +static ssize_t gpio_write(struct file *file, const char *buf, size_t count,
> > + loff_t *off);
> > +static int gpio_open(struct inode *inode, struct file *filp);
> > +static int gpio_release(struct inode *inode, struct file *filp);
> > +static unsigned int gpio_poll(struct file *filp,
> > + struct poll_table_struct *wait);
> > +
> > +/* private data per open() of this driver */
> > +
> > +struct gpio_private {
> > + struct gpio_private *next;
> > + /* The IO_CFG_WRITE_MODE_VALUE only support 8 bits: */
> > + unsigned char clk_mask;
> > + unsigned char data_mask;
> > + unsigned char write_msb;
> > + unsigned char pad1;
> > + /* These fields are generic */
> > + unsigned long highalarm, lowalarm;
> > + wait_queue_head_t alarm_wq;
> > + int minor;
> > +};
> > +
> > +static void gpio_set_alarm(struct gpio_private *priv);
> > +
> > +/* linked list of alarms to check for */
> > +
> > +static struct gpio_private *alarmlist;
> > +
> >
Re: [PATCH 02/47] Add new driver files for Artpec-3.
On Thu, Dec 13, 2007 at 02:30:33PM -0800, David Brownell wrote:
This is needlessly arch-specific though... some other recent platform updates
have provided the Documentation/gpio.txt interfaces and plan to update them
to use the new gpiolib infrastructure when that hits mainline. That gives a
simpler and more extensible approach to I2C (or SPI, etc) GPIO expanders.
Yes, that might simplify things a lot, I'll take a look at that.
I'd guess this talks to a pcf8575-ish GPIO expander (16 bits) though the
code rather lacks relevant comments...
Only the virtual GPIO talks to an extender, the rest of the GPIO
uses direct access to memory registers for the ARTPEC-3 (and EtraxFS).
The GPIO feature here that's not yet in mainline is a userspace interface.
I think that gpiolib will make it a lot easier to offer one of those, since
it maintains a registry that didn't previously exist.
I'm a bit surprised that this doesn't seem to offer kernel interfaces for
GPIOs. There seems to be no way for drivers to use them for input/output
(other than direct register access, which won't work with e.g. leds-gpio
or i2c-gpio) or request_irq(...) for something coming from a GPIO.
This is probably due to the old way to handle leds etc was precisely via
register access, and the driver uses those macros to extend the functions
to userspace.
The LED stuff, and PWM support, should IMO be managed separately.
I suspect that the reason they're handled together is that the same
hardware module implements all three functions.
We have
a LED framework that's preferable to this stuff.
Ok, I'll have a look at that to see if we can use it instead.
PWM is a different issue;
I have a hard time imagining a good generic PWM framework, but maybe that's
because I don't use PWM for anything interesting (like motor control), just
for simple LED/backlight brightness control.
...and motor control is exactly what we use them for.
- Dave
Thanks for the comments, I'll see what improvements I can come up with.
/Jesper
--- /dev/null
+++ b/arch/cris/arch-v32/drivers/mach-a3/gpio.c
@@ -0,0 +1,984 @@
+/*
+ * Artec-3 general port I/O device
+ *
+ * Copyright (c) 2007 Axis Communications AB
+ *
+ * Authors:Bjorn Wesen (initial version)
+ * Ola Knutsson (LED handling)
+ * Johan Adolfsson (read/set directions, write, port G,
+ * port to ETRAX FS.
+ * Ricard Wanderlof (PWM for Artpec-3)
+ *
+ */
+
+#include linux/module.h
+#include linux/sched.h
+#include linux/slab.h
+#include linux/ioport.h
+#include linux/errno.h
+#include linux/kernel.h
+#include linux/fs.h
+#include linux/string.h
+#include linux/poll.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/spinlock.h
+
+#include asm/etraxgpio.h
+#include hwregs/reg_map.h
+#include hwregs/reg_rdwr.h
+#include hwregs/gio_defs.h
+#include hwregs/intr_vect_defs.h
+#include asm/io.h
+#include asm/system.h
+#include asm/irq.h
+#include asm/arch/mach/pinmux.h
+
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+#include ../i2c.h
+
+#define VIRT_I2C_ADDR 0x40
+#endif
+
+/* The following gio ports on ARTPEC-3 is available:
+ * pa 32 bits
+ * pb 32 bits
+ * pc 16 bits
+ * each port has a rw_px_dout, r_px_din and rw_px_oe register.
+ */
+
+#define GPIO_MAJOR 120 /* experimental MAJOR number */
+
+#define I2C_INTERRUPT_BITS 0x300 /* i2c0_done and i2c1_done bits */
+
+#define D(x)
+
+#if 0
+static int dp_cnt;
+#define DP(x) \
+ do { \
+ dp_cnt++; \
+ if (dp_cnt % 1000 == 0) \
+ x; \
+ } while (0)
+#else
+#define DP(x)
+#endif
+
+static char gpio_name[] = etrax gpio;
+
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg);
+#endif
+static int gpio_ioctl(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg);
+static ssize_t gpio_write(struct file *file, const char *buf, size_t count,
+ loff_t *off);
+static int gpio_open(struct inode *inode, struct file *filp);
+static int gpio_release(struct inode *inode, struct file *filp);
+static unsigned int gpio_poll(struct file *filp,
+ struct poll_table_struct *wait);
+
+/* private data per open() of this driver */
+
+struct gpio_private {
+ struct gpio_private *next;
+ /* The IO_CFG_WRITE_MODE_VALUE only support 8 bits: */
+ unsigned char clk_mask;
+ unsigned char data_mask;
+ unsigned char write_msb;
+ unsigned char pad1;
+ /* These fields are generic */
+ unsigned long highalarm, lowalarm;
+ wait_queue_head_t alarm_wq;
+ int minor;
+};
+
+static void gpio_set_alarm(struct gpio_private *priv);
+
+/* linked list of alarms to check
Re: [PATCH 02/47] Add new driver files for Artpec-3.
This is needlessly arch-specific though... some other recent platform updates
have provided the Documentation/gpio.txt interfaces and plan to update them
to use the new gpiolib infrastructure when that hits mainline. That gives a
simpler and more extensible approach to I2C (or SPI, etc) GPIO expanders.
I'd guess this talks to a pcf8575-ish GPIO expander (16 bits) though the
code rather lacks relevant comments...
The GPIO feature here that's not yet in mainline is a userspace interface.
I think that gpiolib will make it a lot easier to offer one of those, since
it maintains a registry that didn't previously exist.
I'm a bit surprised that this doesn't seem to offer kernel interfaces for
GPIOs. There seems to be no way for drivers to use them for input/output
(other than direct register access, which won't work with e.g. leds-gpio
or i2c-gpio) or request_irq(...) for something coming from a GPIO.
The LED stuff, and PWM support, should IMO be managed separately. We have
a LED framework that's preferable to this stuff. PWM is a different issue;
I have a hard time imagining a good generic PWM framework, but maybe that's
because I don't use PWM for anything interesting (like motor control), just
for simple LED/backlight brightness control.
- Dave
> --- /dev/null
> +++ b/arch/cris/arch-v32/drivers/mach-a3/gpio.c
> @@ -0,0 +1,984 @@
> +/*
> + * Artec-3 general port I/O device
> + *
> + * Copyright (c) 2007 Axis Communications AB
> + *
> + * Authors:Bjorn Wesen (initial version)
> + * Ola Knutsson (LED handling)
> + * Johan Adolfsson (read/set directions, write, port G,
> + * port to ETRAX FS.
> + * Ricard Wanderlof (PWM for Artpec-3)
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> +#include "../i2c.h"
> +
> +#define VIRT_I2C_ADDR 0x40
> +#endif
> +
> +/* The following gio ports on ARTPEC-3 is available:
> + * pa 32 bits
> + * pb 32 bits
> + * pc 16 bits
> + * each port has a rw_px_dout, r_px_din and rw_px_oe register.
> + */
> +
> +#define GPIO_MAJOR 120 /* experimental MAJOR number */
> +
> +#define I2C_INTERRUPT_BITS 0x300 /* i2c0_done and i2c1_done bits */
> +
> +#define D(x)
> +
> +#if 0
> +static int dp_cnt;
> +#define DP(x) \
> + do { \
> + dp_cnt++; \
> + if (dp_cnt % 1000 == 0) \
> + x; \
> + } while (0)
> +#else
> +#define DP(x)
> +#endif
> +
> +static char gpio_name[] = "etrax gpio";
> +
> +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> +static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg);
> +#endif
> +static int gpio_ioctl(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long arg);
> +static ssize_t gpio_write(struct file *file, const char *buf, size_t count,
> + loff_t *off);
> +static int gpio_open(struct inode *inode, struct file *filp);
> +static int gpio_release(struct inode *inode, struct file *filp);
> +static unsigned int gpio_poll(struct file *filp,
> + struct poll_table_struct *wait);
> +
> +/* private data per open() of this driver */
> +
> +struct gpio_private {
> + struct gpio_private *next;
> + /* The IO_CFG_WRITE_MODE_VALUE only support 8 bits: */
> + unsigned char clk_mask;
> + unsigned char data_mask;
> + unsigned char write_msb;
> + unsigned char pad1;
> + /* These fields are generic */
> + unsigned long highalarm, lowalarm;
> + wait_queue_head_t alarm_wq;
> + int minor;
> +};
> +
> +static void gpio_set_alarm(struct gpio_private *priv);
> +
> +/* linked list of alarms to check for */
> +
> +static struct gpio_private *alarmlist;
> +
> +static int wanted_interrupts;
> +
> +static DEFINE_SPINLOCK(alarm_lock);
> +
> +#define NUM_PORTS (GPIO_MINOR_LAST+1)
> +#define GIO_REG_RD_ADDR(reg) \
> + (volatile unsigned long *)(regi_gio + REG_RD_ADDR_gio_##reg)
> +#define GIO_REG_WR_ADDR(reg) \
> + (volatile unsigned long *)(regi_gio + REG_WR_ADDR_gio_##reg)
> +unsigned long led_dummy;
> +unsigned long port_d_dummy; /* Only input on Artpec-3 */
> +unsigned long port_e_dummy; /* Non existent on Artpec-3 */
> +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> +static unsigned long virtual_dummy;
> +static unsigned long virtual_rw_pv_oe = CONFIG_ETRAX_DEF_GIO_PV_OE;
> +static unsigned short cached_virtual_gpio_read;
> +#endif
> +
> +static volatile unsigned long *data_out[NUM_PORTS] = {
> + GIO_REG_WR_ADDR(rw_pa_dout),
> + GIO_REG_WR_ADDR(rw_pb_dout),
> + _dummy,
> + GIO_REG_WR_ADDR(rw_pc_dout),
> + _d_dummy,
> +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> + _e_dummy,
> + _dummy,
>
Re: [PATCH 02/47] Add new driver files for Artpec-3.
This is needlessly arch-specific though... some other recent platform updates
have provided the Documentation/gpio.txt interfaces and plan to update them
to use the new gpiolib infrastructure when that hits mainline. That gives a
simpler and more extensible approach to I2C (or SPI, etc) GPIO expanders.
I'd guess this talks to a pcf8575-ish GPIO expander (16 bits) though the
code rather lacks relevant comments...
The GPIO feature here that's not yet in mainline is a userspace interface.
I think that gpiolib will make it a lot easier to offer one of those, since
it maintains a registry that didn't previously exist.
I'm a bit surprised that this doesn't seem to offer kernel interfaces for
GPIOs. There seems to be no way for drivers to use them for input/output
(other than direct register access, which won't work with e.g. leds-gpio
or i2c-gpio) or request_irq(...) for something coming from a GPIO.
The LED stuff, and PWM support, should IMO be managed separately. We have
a LED framework that's preferable to this stuff. PWM is a different issue;
I have a hard time imagining a good generic PWM framework, but maybe that's
because I don't use PWM for anything interesting (like motor control), just
for simple LED/backlight brightness control.
- Dave
--- /dev/null
+++ b/arch/cris/arch-v32/drivers/mach-a3/gpio.c
@@ -0,0 +1,984 @@
+/*
+ * Artec-3 general port I/O device
+ *
+ * Copyright (c) 2007 Axis Communications AB
+ *
+ * Authors:Bjorn Wesen (initial version)
+ * Ola Knutsson (LED handling)
+ * Johan Adolfsson (read/set directions, write, port G,
+ * port to ETRAX FS.
+ * Ricard Wanderlof (PWM for Artpec-3)
+ *
+ */
+
+#include linux/module.h
+#include linux/sched.h
+#include linux/slab.h
+#include linux/ioport.h
+#include linux/errno.h
+#include linux/kernel.h
+#include linux/fs.h
+#include linux/string.h
+#include linux/poll.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/spinlock.h
+
+#include asm/etraxgpio.h
+#include hwregs/reg_map.h
+#include hwregs/reg_rdwr.h
+#include hwregs/gio_defs.h
+#include hwregs/intr_vect_defs.h
+#include asm/io.h
+#include asm/system.h
+#include asm/irq.h
+#include asm/arch/mach/pinmux.h
+
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+#include ../i2c.h
+
+#define VIRT_I2C_ADDR 0x40
+#endif
+
+/* The following gio ports on ARTPEC-3 is available:
+ * pa 32 bits
+ * pb 32 bits
+ * pc 16 bits
+ * each port has a rw_px_dout, r_px_din and rw_px_oe register.
+ */
+
+#define GPIO_MAJOR 120 /* experimental MAJOR number */
+
+#define I2C_INTERRUPT_BITS 0x300 /* i2c0_done and i2c1_done bits */
+
+#define D(x)
+
+#if 0
+static int dp_cnt;
+#define DP(x) \
+ do { \
+ dp_cnt++; \
+ if (dp_cnt % 1000 == 0) \
+ x; \
+ } while (0)
+#else
+#define DP(x)
+#endif
+
+static char gpio_name[] = etrax gpio;
+
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg);
+#endif
+static int gpio_ioctl(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg);
+static ssize_t gpio_write(struct file *file, const char *buf, size_t count,
+ loff_t *off);
+static int gpio_open(struct inode *inode, struct file *filp);
+static int gpio_release(struct inode *inode, struct file *filp);
+static unsigned int gpio_poll(struct file *filp,
+ struct poll_table_struct *wait);
+
+/* private data per open() of this driver */
+
+struct gpio_private {
+ struct gpio_private *next;
+ /* The IO_CFG_WRITE_MODE_VALUE only support 8 bits: */
+ unsigned char clk_mask;
+ unsigned char data_mask;
+ unsigned char write_msb;
+ unsigned char pad1;
+ /* These fields are generic */
+ unsigned long highalarm, lowalarm;
+ wait_queue_head_t alarm_wq;
+ int minor;
+};
+
+static void gpio_set_alarm(struct gpio_private *priv);
+
+/* linked list of alarms to check for */
+
+static struct gpio_private *alarmlist;
+
+static int wanted_interrupts;
+
+static DEFINE_SPINLOCK(alarm_lock);
+
+#define NUM_PORTS (GPIO_MINOR_LAST+1)
+#define GIO_REG_RD_ADDR(reg) \
+ (volatile unsigned long *)(regi_gio + REG_RD_ADDR_gio_##reg)
+#define GIO_REG_WR_ADDR(reg) \
+ (volatile unsigned long *)(regi_gio + REG_WR_ADDR_gio_##reg)
+unsigned long led_dummy;
+unsigned long port_d_dummy; /* Only input on Artpec-3 */
+unsigned long port_e_dummy; /* Non existent on Artpec-3 */
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+static unsigned long virtual_dummy;
+static unsigned long virtual_rw_pv_oe = CONFIG_ETRAX_DEF_GIO_PV_OE;
+static unsigned short cached_virtual_gpio_read;
+#endif
+
+static volatile unsigned long *data_out[NUM_PORTS] = {
+ GIO_REG_WR_ADDR(rw_pa_dout),
+
Re: [PATCH 02/47] Add new driver files for Artpec-3.
On Thu, 29 Nov 2007 17:03:41 +0100 Jesper Nilsson <[EMAIL PROTECTED]> wrote:
> Adds gpio and nandflash handling for Artpec-3.
>
> ...
>
> +static volatile unsigned long *data_out[NUM_PORTS] = {
all these volatiles really shouldn't be here.
> +static void
> +gpio_set_alarm(struct gpio_private *priv)
> +{
> + int bit;
> + int intr_cfg;
> + int mask;
> + int pins;
> + unsigned long flags;
> +
> + local_irq_save(flags);
Will never run on SMP?
> +
> +inline unsigned long setget_input(struct gpio_private *priv, unsigned long
> arg)
> +{
> + /* Set direction 0=unchanged 1=input,
> + * return mask with 1=input
> + */
> + unsigned long flags;
> + unsigned long dir_shadow;
> +
> + local_irq_save(flags);
> + dir_shadow = *dir_oe[priv->minor];
> + dir_shadow &= ~(arg & changeable_dir[priv->minor]);
> + *dir_oe[priv->minor] = dir_shadow;
> + local_irq_restore(flags);
> +
> + if (priv->minor == GPIO_MINOR_C)
> + dir_shadow ^= 0x; /* Only 16 bits */
> +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> + else if (priv->minor == GPIO_MINOR_V)
> + dir_shadow ^= 0x; /* Only 16 bits */
> +#endif
> + else
> + dir_shadow ^= 0x; /* PA, PB and PD 32 bits */
> +
> + return dir_shadow;
> +
> +} /* setget_input */
> +
> +inline unsigned long setget_output(struct gpio_private *priv, unsigned long
> arg)
> +{
> + unsigned long flags;
> + unsigned long dir_shadow;
> +
> + local_irq_save(flags);
> + dir_shadow = *dir_oe[priv->minor];
> + dir_shadow |= (arg & changeable_dir[priv->minor]);
> + *dir_oe[priv->minor] = dir_shadow;
> + local_irq_restore(flags);
> + return dir_shadow;
> +} /* setget_output */
The `inline's here are surely deoptimising the code.
> +static int
> +gpio_leds_ioctl(unsigned int cmd, unsigned long arg);
> +
> +static int
> +gpio_pwm_ioctl(struct gpio_private *priv, unsigned int cmd, unsigned long
> arg);
> +
> +static int
> +gpio_ioctl(struct inode *inode, struct file *file,
> +unsigned int cmd, unsigned long arg)
> +{
> + unsigned long flags;
> + unsigned long val;
> + unsigned long shadow;
> + struct gpio_private *priv = (struct gpio_private *)file->private_data;
Unneeded and undesirable cast of void*.
> +
> + if (_IOC_TYPE(cmd) != ETRAXGPIO_IOCTYPE)
> + return -EINVAL;
Not ENOTTY?
> + /* Check for special ioctl handlers first */
> +
> +#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
> + if (priv->minor == GPIO_MINOR_V)
> + return virtual_gpio_ioctl(file, cmd, arg);
> +#endif
> +
> + if (priv->minor == GPIO_MINOR_LEDS)
> + return gpio_leds_ioctl(cmd, arg);
> +
> + if (priv->minor >= GPIO_MINOR_PWM0 &&
> + priv->minor <= GPIO_MINOR_LAST_PWM)
> + return gpio_pwm_ioctl(priv, cmd, arg);
> +
> + switch (_IOC_NR(cmd)) {
> + case IO_READBITS: /* Use IO_READ_INBITS and IO_READ_OUTBITS instead */
> + /* Read the port. */
> + return *data_in[priv->minor];
Really. Nuke the volatiles, use readl().
> + break;
> + case IO_SETBITS:
> + local_irq_save(flags);
> + /* Set changeable bits with a 1 in arg. */
> + shadow = *data_out[priv->minor];
readl()
> + shadow |= (arg & changeable_bits[priv->minor]);
> + *data_out[priv->minor] = shadow;
writel()
> + local_irq_restore(flags);
> + break;
> + case IO_CLRBITS:
> + local_irq_save(flags);
> + /* Clear changeable bits with a 1 in arg. */
> + shadow = *data_out[priv->minor];
> + shadow &= ~(arg & changeable_bits[priv->minor]);
> + *data_out[priv->minor] = shadow;
> + local_irq_restore(flags);
> + break;
> + case IO_HIGHALARM:
> + /* Set alarm when bits with 1 in arg go high. */
> + priv->highalarm |= arg;
> + gpio_set_alarm(priv);
> + break;
> + case IO_LOWALARM:
> + /* Set alarm when bits with 1 in arg go low. */
> + priv->lowalarm |= arg;
> + gpio_set_alarm(priv);
> + break;
> + case IO_CLRALARM:
> + /* Clear alarm for bits with 1 in arg. */
> + priv->highalarm &= ~arg;
> + priv->lowalarm &= ~arg;
> + gpio_set_alarm(priv);
> + break;
> + case IO_READDIR: /* Use IO_SETGET_INPUT/OUTPUT instead! */
> + /* Read direction 0=input 1=output */
> + return *dir_oe[priv->minor];
> + case IO_SETINPUT: /* Use IO_SETGET_INPUT instead! */
> + /* Set direction 0=unchanged 1=input,
> + * return mask with 1=input
> + */
> + return setget_input(priv, arg);
> + break;
That break is a bit pointless.
> + case IO_SETOUTPUT: /* Use
Re: [PATCH 02/47] Add new driver files for Artpec-3.
On Thu, 29 Nov 2007 17:03:41 +0100 Jesper Nilsson [EMAIL PROTECTED] wrote:
Adds gpio and nandflash handling for Artpec-3.
...
+static volatile unsigned long *data_out[NUM_PORTS] = {
all these volatiles really shouldn't be here.
+static void
+gpio_set_alarm(struct gpio_private *priv)
+{
+ int bit;
+ int intr_cfg;
+ int mask;
+ int pins;
+ unsigned long flags;
+
+ local_irq_save(flags);
Will never run on SMP?
+
+inline unsigned long setget_input(struct gpio_private *priv, unsigned long
arg)
+{
+ /* Set direction 0=unchanged 1=input,
+ * return mask with 1=input
+ */
+ unsigned long flags;
+ unsigned long dir_shadow;
+
+ local_irq_save(flags);
+ dir_shadow = *dir_oe[priv-minor];
+ dir_shadow = ~(arg changeable_dir[priv-minor]);
+ *dir_oe[priv-minor] = dir_shadow;
+ local_irq_restore(flags);
+
+ if (priv-minor == GPIO_MINOR_C)
+ dir_shadow ^= 0x; /* Only 16 bits */
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+ else if (priv-minor == GPIO_MINOR_V)
+ dir_shadow ^= 0x; /* Only 16 bits */
+#endif
+ else
+ dir_shadow ^= 0x; /* PA, PB and PD 32 bits */
+
+ return dir_shadow;
+
+} /* setget_input */
+
+inline unsigned long setget_output(struct gpio_private *priv, unsigned long
arg)
+{
+ unsigned long flags;
+ unsigned long dir_shadow;
+
+ local_irq_save(flags);
+ dir_shadow = *dir_oe[priv-minor];
+ dir_shadow |= (arg changeable_dir[priv-minor]);
+ *dir_oe[priv-minor] = dir_shadow;
+ local_irq_restore(flags);
+ return dir_shadow;
+} /* setget_output */
The `inline's here are surely deoptimising the code.
+static int
+gpio_leds_ioctl(unsigned int cmd, unsigned long arg);
+
+static int
+gpio_pwm_ioctl(struct gpio_private *priv, unsigned int cmd, unsigned long
arg);
+
+static int
+gpio_ioctl(struct inode *inode, struct file *file,
+unsigned int cmd, unsigned long arg)
+{
+ unsigned long flags;
+ unsigned long val;
+ unsigned long shadow;
+ struct gpio_private *priv = (struct gpio_private *)file-private_data;
Unneeded and undesirable cast of void*.
+
+ if (_IOC_TYPE(cmd) != ETRAXGPIO_IOCTYPE)
+ return -EINVAL;
Not ENOTTY?
+ /* Check for special ioctl handlers first */
+
+#ifdef CONFIG_ETRAX_VIRTUAL_GPIO
+ if (priv-minor == GPIO_MINOR_V)
+ return virtual_gpio_ioctl(file, cmd, arg);
+#endif
+
+ if (priv-minor == GPIO_MINOR_LEDS)
+ return gpio_leds_ioctl(cmd, arg);
+
+ if (priv-minor = GPIO_MINOR_PWM0
+ priv-minor = GPIO_MINOR_LAST_PWM)
+ return gpio_pwm_ioctl(priv, cmd, arg);
+
+ switch (_IOC_NR(cmd)) {
+ case IO_READBITS: /* Use IO_READ_INBITS and IO_READ_OUTBITS instead */
+ /* Read the port. */
+ return *data_in[priv-minor];
Really. Nuke the volatiles, use readl().
+ break;
+ case IO_SETBITS:
+ local_irq_save(flags);
+ /* Set changeable bits with a 1 in arg. */
+ shadow = *data_out[priv-minor];
readl()
+ shadow |= (arg changeable_bits[priv-minor]);
+ *data_out[priv-minor] = shadow;
writel()
+ local_irq_restore(flags);
+ break;
+ case IO_CLRBITS:
+ local_irq_save(flags);
+ /* Clear changeable bits with a 1 in arg. */
+ shadow = *data_out[priv-minor];
+ shadow = ~(arg changeable_bits[priv-minor]);
+ *data_out[priv-minor] = shadow;
+ local_irq_restore(flags);
+ break;
+ case IO_HIGHALARM:
+ /* Set alarm when bits with 1 in arg go high. */
+ priv-highalarm |= arg;
+ gpio_set_alarm(priv);
+ break;
+ case IO_LOWALARM:
+ /* Set alarm when bits with 1 in arg go low. */
+ priv-lowalarm |= arg;
+ gpio_set_alarm(priv);
+ break;
+ case IO_CLRALARM:
+ /* Clear alarm for bits with 1 in arg. */
+ priv-highalarm = ~arg;
+ priv-lowalarm = ~arg;
+ gpio_set_alarm(priv);
+ break;
+ case IO_READDIR: /* Use IO_SETGET_INPUT/OUTPUT instead! */
+ /* Read direction 0=input 1=output */
+ return *dir_oe[priv-minor];
+ case IO_SETINPUT: /* Use IO_SETGET_INPUT instead! */
+ /* Set direction 0=unchanged 1=input,
+ * return mask with 1=input
+ */
+ return setget_input(priv, arg);
+ break;
That break is a bit pointless.
+ case IO_SETOUTPUT: /* Use IO_SETGET_OUTPUT instead! */
+ /* Set direction 0=unchanged 1=output,
+ * return mask with 1=output
+ */
+ return setget_output(priv,
