Re: [PATCH 2/2 v2] gpio: msm7200a: Add irq support to msm-gpiolib.

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 11 Jun 2010 17:19:29 -0700
Gregory Bean <gbean@codeaurora.org> wrote:

>
> ...
>
> +/*
> + * The INT_STATUS register latches both edge- and level-detection events,
> + * which is atypical.  Turning on DONT_LATCH_LEVEL_IRQS causes level irq
> + * triggers to be forgotten across mask/unmask calls, emulating a more
> + * traditional setup.
> + */
> +#define MSM_GPIO_DONT_LATCH_LEVEL_IRQS 1

It's unusual to require a source-code edit to enable a compile-time
feature.

If this knob is actually useful then I'd suggest making it a Kconfig
thing.  Or, much much better, a module parameter settable at modprobe
time.  Or, much much better, a /sys knob (or whatever) which can be set
at runtime.  Or, much much better, just autodetect the desired
behaviour and don't hassle the nice users ;)

>
> ...
>
> +static int msm_gpio_irq_set_type(unsigned int irq, unsigned int flow_type)
> +{
> +	unsigned long irq_flags;
> +	struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
> +	unsigned offset = irq - msm_gpio->irq_base;
> +
> +	if ((flow_type & (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)) ==
> +		(IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING))
> +		return -ENOTSUPP;
> +
> +	if ((flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW)) ==
> +		(IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> +		return -ENOTSUPP;

According to the code comments, ENOTSUPP is "Defined for the NFSv3
protocol".  I'd imagine that cellphone software developers who haven't
even configured nfs3 into their builds will get a bit puzzled if this
comes out.

I'd suggest using something simple and generic: EIO, EINVAL, etc.

>
> ...
>
> +static void msm_gpio_irq_mask_ack(unsigned int irq)
> +{
> +	msm_gpio_irq_mask(irq);
> +}
> +
> +static int msm_gpio_irq_set_affinity(unsigned int irq,
> +				const struct cpumask *dest)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int msm_gpio_irq_retrigger(unsigned int irq)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int msm_gpio_irq_set_wake(unsigned int irq, unsigned int on)
> +{
> +	return -ENOTSUPP;
> +}

dittoes.

> +static irqreturn_t msm_gpio_irq_handler(int irq, void *dev)
> +{
> +	unsigned long irq_flags;
> +	int b, m;
> +	unsigned e, s, v;
> +
> +	struct msm_gpio_dev *msm_gpio = (struct msm_gpio_dev *)dev;

Didn't need that blank line there.

Unneeded cast.

> +	/*
> +	 * The int_status register latches trigger events whether or not
> +	 * the gpio line is enabled as an interrupt source.  Therefore,
> +	 * the set of pins which defines the interrupts which need to fire
> +	 * is the intersection of int_status and int_en - int_status
> +	 * alone provides an incomplete picture.
> +	 */
> +	spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> +	s = readl(msm_gpio->regs.int_status);
> +	e = readl(msm_gpio->regs.int_en);
> +	v = s & e;
> +	if (v)
> +		writel(v, msm_gpio->regs.int_clear);
> +	spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);

Plain old spin_lock() is probably OK in the IRQ handler.  It won't be
interrupting itself.

> +	if (!v)
> +		return IRQ_NONE;
> +
> +	while (v) {
> +		m = v & -v;
> +		b = fls(m) - 1;
> +		v &= ~m;
> +		generic_handle_irq(msm_gpio->irq_base + b);
> +	}
> +	return IRQ_HANDLED;
> +}
> +
>
> ...
>