Re: [PATCH] synclink_gt add gpio feature

[Andrew Morton <akpm@osdl.org>]

Paul Fulghum <paulkf@microgate.com> wrote:
>
> Add driver support for general purpose I/O feature
> of the Synclink GT adapters.
> 
>  
> ...
>
>  /*
> + * conditional wait facility
> + */
> +struct cond_wait {
> +	struct cond_wait *next;
> +	wait_queue_head_t q;
> +	wait_queue_t wait;
> +	unsigned int data;
> +};
> +static void init_cond_wait(struct cond_wait *w, unsigned int data);
> +static void add_cond_wait(struct cond_wait **head, struct cond_wait *w);
> +static void remove_cond_wait(struct cond_wait **head, struct cond_wait *w);
> +static void flush_cond_wait(struct cond_wait **head);

Adding new generic-looking infrastructure into a driver is a worry.  Either
we're missing some facility, or the driver is doing something unnecessary
or the driver's requirements are unique.

Tell use more about conditional waits?

> ...
> +static int wait_gpio(struct slgt_info *info, struct gpio_desc __user *user_gpio)
> +{
> + 	unsigned long flags;
> +	int rc = 0;
> +	struct gpio_desc gpio;
> +	struct cond_wait wait;
> +	u32 state;
> +
> +	if (!info->gpio_present)
> +		return -EINVAL;
> +	if (copy_from_user(&gpio, user_gpio, sizeof(gpio)))
> +		return -EFAULT;
> +	DBGINFO(("%s wait_gpio() state=%08x smask=%08x\n",
> +		 info->device_name, gpio.state, gpio.smask));
> +	/* ignore output pins identified by set IODR bit */
> +	if ((gpio.smask &= ~rd_reg32(info, IODR)) == 0)
> +		return -EINVAL;
> +	init_cond_wait(&wait, gpio.smask);
> +
> +	spin_lock_irqsave(&info->lock, flags);
> +	/* enable interrupts for watched pins */
> +	wr_reg32(info, IOER, rd_reg32(info, IOER) | gpio.smask);
> +	/* get current pin states */
> +	state = rd_reg32(info, IOVR);
> +
> +	if (gpio.smask & ~(state ^ gpio.state)) {
> +		/* already in target state */
> +		gpio.state = state;
> +	} else {
> +		/* wait for target state */
> +		add_cond_wait(&info->gpio_wait_q, &wait);
> +		spin_unlock_irqrestore(&info->lock, flags);
> +		schedule();
> +		if (signal_pending(current))
> +			rc = -ERESTARTSYS;
> +		else
> +			gpio.state = wait.data;
> +		spin_lock_irqsave(&info->lock, flags);
> +		remove_cond_wait(&info->gpio_wait_q, &wait);
> +	}
> +
> +	/* disable all GPIO interrupts if no waiting processes */
> +	if (info->gpio_wait_q == NULL)
> +		wr_reg32(info, IOER, 0);

Should we be dong that write if rc!=0?  I guess so..

> +	spin_unlock_irqrestore(&info->lock,flags);
> +
> +	if ((rc == 0) && copy_to_user(user_gpio, &gpio, sizeof(gpio)))
> +		rc = -EFAULT;
> +	return rc;
> +}