On Fri, 11 Jun 2010 17:19:29 -0700
Gregory Bean <gbean@xxxxxxxxxxxxxx> 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;
> +}
> +
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
