| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20100612022401.GC31045@fluff.org.uk>
Date: Sat, 12 Jun 2010 03:24:01 +0100
From: Ben Dooks <ben-linux@...ff.org>
To: Gregory Bean <gbean@...eaurora.org>
Cc: akpm@...ux-foundation.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, David Brown <davidb@...eaurora.org>,
Daniel Walker <dwalker@...eaurora.org>,
Bryan Huntsman <bryanh@...eaurora.org>
Subject: Re: [PATCH 2/2] gpio: msm7200a: Add irq support to msm-gpiolib.
On Fri, Jun 11, 2010 at 12:58:52PM -0700, Gregory Bean wrote:
> Change-Id: Ib7f7dd05246c87096aabab12eb6cc260551c67cf
> Signed-off-by: Gregory Bean <gbean@...eaurora.org>
> ---
~> drivers/gpio/msm7200a-gpio.c | 218 +++++++++++++++++++++++++++++++++++++++--
> include/linux/msm7200a-gpio.h | 7 ++
> 2 files changed, 218 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/msm7200a-gpio.c b/drivers/gpio/msm7200a-gpio.c
> index b31c25e..a9ea869 100644
> --- a/drivers/gpio/msm7200a-gpio.c
> +++ b/drivers/gpio/msm7200a-gpio.c
> @@ -23,13 +23,25 @@
> #include <linux/kernel.h>
> #include <linux/gpio.h>
> #include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/msm7200a-gpio.h>
>
> +/*
> + * 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
> +
> struct msm_gpio_dev {
> struct gpio_chip gpio_chip;
> spinlock_t lock;
> + unsigned irq_base;
> + unsigned irq_summary;
> struct msm7200a_gpio_regs regs;
> };
>
> @@ -119,12 +131,160 @@ static void gpio_chip_set(struct gpio_chip *chip, unsigned offset, int value)
> spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> }
>
> +static int gpio_chip_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> + struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip);
> + return msm_gpio->irq_base + offset;
> +}
> +
> +#if MSM_GPIO_DONT_LATCH_LEVEL_IRQS
> +static inline void forget_level_irq(struct msm_gpio_dev *msm_gpio,
> + unsigned offset)
> +{
> + unsigned v = readl(msm_gpio->regs.int_edge);
> + unsigned b = bit(offset);
> +
> + if (!(v & b))
> + writel(b, msm_gpio->regs.int_clear);
> +
> +}
> +#else
> +static inline void forget_level_irq(struct msm_gpio_dev *msm, unsigned off)
> +{
> +}
> +#endif
Hmm, that's a bit yucky, either Kconfig it or have it definable
in the platform data.
> +static void msm_gpio_irq_mask(unsigned int irq)
> +{
> + unsigned long irq_flags;
> + struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
> + unsigned offset = irq - msm_gpio->irq_base;
> +
> + spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> + forget_level_irq(msm_gpio, offset);
> + clr_gpio_bit(offset, msm_gpio->regs.int_en);
> + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> +}
> +
> +static void msm_gpio_irq_unmask(unsigned int irq)
> +{
> + unsigned long irq_flags;
> + struct msm_gpio_dev *msm_gpio = get_irq_chip_data(irq);
> + unsigned offset = irq - msm_gpio->irq_base;
> +
> + spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> + forget_level_irq(msm_gpio, offset);
> + set_gpio_bit(offset, msm_gpio->regs.int_en);
> + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> +}
> +
> +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;
Hmm, thought there was a BOTHEDGE for this, maybe worth adding
to the irq.h file at somepoint.
> + if ((flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW)) ==
> + (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> + return -ENOTSUPP;
> +
> + spin_lock_irqsave(&msm_gpio->lock, irq_flags);
> +
> + if (flow_type & (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)) {
> + set_gpio_bit(offset, msm_gpio->regs.int_edge);
> + irq_desc[irq].handle_irq = handle_edge_irq;
> + } else {
> + clr_gpio_bit(offset, msm_gpio->regs.int_edge);
> + irq_desc[irq].handle_irq = handle_level_irq;
> + }
> +
> + if (flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_RISING))
> + set_gpio_bit(offset, msm_gpio->regs.int_pos);
> + else
> + clr_gpio_bit(offset, msm_gpio->regs.int_pos);
> +
> + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> +
> + return 0;
> +}
> +
> +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;
> +}
do you really need to define these, surely the irq layer will
do the right thing if there's no handler defined?
> +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;
see prev. comment about casting void *, plus you seem to have a blank
line in here by accident.
would be worth organising the longest lines first.
+ /*
> + * 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;
how about slightly less terse interrupts.
> + if (v)
> + writel(v, msm_gpio->regs.int_clear);
> + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags);
> + if (!v)
> + return IRQ_NONE;
> +
> + while (v) {
> + m = v & -v;
> + b = fls(m) - 1;
> + v &= ~m;
i'm not entirely sure what you'e doing here, how aout a comment
on the m line to say what ti is up to.
> + generic_handle_irq(msm_gpio->irq_base + b);
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static struct irq_chip msm_gpio_irq_chip = {
> + .name = "msm_gpio",
> + .mask = msm_gpio_irq_mask,
> + .mask_ack = msm_gpio_irq_mask_ack,
> + .unmask = msm_gpio_irq_unmask,
> + .set_affinity = msm_gpio_irq_set_affinity,
> + .retrigger = msm_gpio_irq_retrigger,
> + .set_type = msm_gpio_irq_set_type,
> + .set_wake = msm_gpio_irq_set_wake,
> +};
> +
> static int msm_gpio_probe(struct platform_device *dev)
> {
> struct msm_gpio_dev *msm_gpio;
> struct msm7200a_gpio_platform_data *pdata =
> (struct msm7200a_gpio_platform_data *)dev->dev.platform_data;
> - int ret;
> + int i, irq, ret;
>
> if (!pdata)
> return -EINVAL;
> @@ -146,13 +306,53 @@ static int msm_gpio_probe(struct platform_device *dev)
> msm_gpio->gpio_chip.direction_output = gpio_chip_direction_output;
> msm_gpio->gpio_chip.get = gpio_chip_get;
> msm_gpio->gpio_chip.set = gpio_chip_set;
> + msm_gpio->gpio_chip.to_irq = gpio_chip_to_irq;
> + msm_gpio->irq_base = pdata->irq_base;
> + msm_gpio->irq_summary = pdata->irq_summary;
>
> ret = gpiochip_add(&msm_gpio->gpio_chip);
> if (ret < 0)
> - goto err;
> + goto err_post_malloc;
> +
> + for (i = 0; i < msm_gpio->gpio_chip.ngpio; ++i) {
> + irq = msm_gpio->irq_base + i;
> + set_irq_chip_data(irq, msm_gpio);
> + set_irq_chip(irq, &msm_gpio_irq_chip);
> + set_irq_handler(irq, handle_level_irq);
> + set_irq_flags(irq, IRQF_VALID);
> + }
> +
> + /*
> + * We use a level-triggered interrupt because of the nature
> + * of the shared GPIO-group interrupt.
> + *
> + * Many GPIO chips may be sharing the same group IRQ line, and
> + * it is possible for GPIO interrupt to re-occur while the system
> + * is still servicing the group interrupt associated with it.
> + * The group IRQ line would not de-assert and re-assert, and
> + * we'd get no second edge to cause the group IRQ to be handled again.
> + *
> + * Using a level interrupt guarantees that the group IRQ handlers
> + * will continue to be called as long as any GPIO chip in the group
> + * is asserting, even if the condition began while the group
> + * handler was in mid-pass.
> + */
> + ret = request_irq(msm_gpio->irq_summary,
> + msm_gpio_irq_handler,
> + IRQF_SHARED | IRQF_TRIGGER_HIGH,
> + dev->name,
iirc, dev_name() is the correct thing to use herre.
> + msm_gpio);
> + if (ret < 0)
> + goto err_post_gpiochip_add;
>
> return ret;
> -err:
> +err_post_gpiochip_add:
> + /*
> + * Under no circumstances should a line be held on a gpiochip
> + * which hasn't finished probing.
> + */
> + BUG_ON(gpiochip_remove(&msm_gpio->gpio_chip) < 0);
> +err_post_malloc:
> kfree(msm_gpio);
> return ret;
> }
> @@ -160,12 +360,16 @@ err:
> static int msm_gpio_remove(struct platform_device *dev)
> {
> struct msm_gpio_dev *msm_gpio = platform_get_drvdata(dev);
> - int ret = gpiochip_remove(&msm_gpio->gpio_chip);
> + int ret;
>
> - if (ret == 0)
> - kfree(msm_gpio);
> + ret = gpiochip_remove(&msm_gpio->gpio_chip);
> + if (ret < 0)
> + return ret;
>
> - return ret;
> + free_irq(msm_gpio->irq_summary, msm_gpio);
> + kfree(msm_gpio);
> +
> + return 0;
> }
>
> static struct platform_driver msm_gpio_driver = {
> diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h
> index 3f1ef38..7af4dd6 100644
> --- a/include/linux/msm7200a-gpio.h
> +++ b/include/linux/msm7200a-gpio.h
> @@ -33,11 +33,18 @@ struct msm7200a_gpio_regs {
> void __iomem *in;
> void __iomem *out;
> void __iomem *oe;
> + void __iomem *int_status;
> + void __iomem *int_clear;
> + void __iomem *int_en;
> + void __iomem *int_edge;
> + void __iomem *int_pos;
> };
see prev. comment on could we have single base and offsets?
> struct msm7200a_gpio_platform_data {
> unsigned gpio_base;~~
> unsigned ngpio;
> + unsigned irq_base;
> + unsigned irq_summary;
> struct msm7200a_gpio_regs regs;
> };
>
> --
> 1.7.0.4
>
> ---
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ben (ben@...ff.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists