| Message ID | 1413431475-12799-4-git-send-email-rebecca.swee.fun.chang@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 10/16/2014 09:21 AM, Chang Rebecca Swee Fun wrote: > Intel Quark X1000 GPIO controller supports interrupt handling for > both core power well and resume power well. This patch is to enable > the IRQ support and provide IRQ handling for Intel Quark X1000 > GPIO-SCH device driver. (...) > static int sch_gpio_probe(struct platform_device *pdev) > { > struct sch_gpio *sch; > - struct resource *res; > + struct resource *res, *irq; > + int err; > > sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); > if (!sch) > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev) > pdev->name)) > return -EBUSY; > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + sch->irq_support = !!irq; > + if (sch->irq_support) { > + sch->irq_num = irq->start; > + if (sch->irq_num < 0) { > + dev_warn(&pdev->dev, > + "failed to obtain irq number for device\n"); > + sch->irq_support = false; > + } > + } > + > spin_lock_init(&sch->lock); > sch->iobase = res->start; > sch->chip = sch_gpio_chip; > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev) > return -ENODEV; > } > > + err = gpiochip_add(&sch->chip); > + if (err < 0) > + goto err_sch_gpio; > + > + if (sch->irq_support) { > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > + NUMA_NO_NODE); > + if (sch->irq_base < 0) { > + dev_err(&pdev->dev, > + "failed to allocate GPIO IRQ descs\n"); > + goto err_sch_intr_chip; > + } > + > + /* disable interrupts */ > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > + > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > + IRQF_SHARED, KBUILD_MODNAME, sch); > + if (err) { > + dev_err(&pdev->dev, > + "%s failed to request IRQ\n", __func__); > + goto err_sch_request_irq; > + } > + > + sch_gpio_irqs_init(sch, sch->chip.ngpio); > + } > + > platform_set_drvdata(pdev, sch); > > - return gpiochip_add(&sch->chip); > + return 0; > + > +err_sch_request_irq: > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + > +err_sch_intr_chip: > + if (gpiochip_remove(&sch->chip)) gpiochip_remove() return 'void' [0]. > + dev_err(&pdev->dev, > + "%s gpiochip_remove() failed\n", __func__); > + > +err_sch_gpio: > + release_region(res->start, resource_size(res)); > + > + return err; > } [0]:https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/tree/drivers/gpio/gpiolib.c#n311
On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote: > Intel Quark X1000 GPIO controller supports interrupt handling for > both core power well and resume power well. This patch is to enable > the IRQ support and provide IRQ handling for Intel Quark X1000 > GPIO-SCH device driver. > > This piece of work is derived from Dan O'Donovan's initial work for > Quark X1000 enabling. > > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> In addition to what Varka Bhadram pointed out, I have few comments. See below. Overall, looks good. > --- > drivers/gpio/gpio-sch.c | 257 ++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 245 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c > index 952990f..dd84b1f 100644 > --- a/drivers/gpio/gpio-sch.c > +++ b/drivers/gpio/gpio-sch.c > @@ -28,17 +28,30 @@ > #include <linux/pci_ids.h> > > #include <linux/gpio.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > > #define GEN 0x00 > #define GIO 0x04 > #define GLV 0x08 > +#define GTPE 0x0C > +#define GTNE 0x10 > +#define GGPE 0x14 > +#define GSMI 0x18 > +#define GTS 0x1C > +#define CGNMIEN 0x40 > +#define RGNMIEN 0x44 > > struct sch_gpio { > struct gpio_chip chip; > + struct irq_data data; > spinlock_t lock; > unsigned short iobase; > unsigned short core_base; > unsigned short resume_base; > + int irq_base; > + int irq_num; > + bool irq_support; > }; > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio) > static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > + unsigned long flags; > unsigned short offset, bit; > u8 enable; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > > offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > if (!(enable & BIT(bit))) > outb(enable | BIT(bit), sch->iobase + offset); > > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > + unsigned long flags; > unsigned short offset, bit; > u8 disable; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > > offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > if (disable & BIT(bit)) > outb(disable & ~BIT(bit), sch->iobase + offset); > > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg) > @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg, > static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_register_set(sch, gpio_num, GIO); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > return 0; > } > > @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) > static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_reg_set(gc, gpio_num, GLV, val); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_register_clear(sch, gpio_num, GIO); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > > /* > * according to the datasheet, writing to the level register has no > @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > return 0; > } > > +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > +{ > + struct sch_gpio *sch = to_sch_gpio(gc); > + > + return sch->irq_base + offset; > +} > + > static struct gpio_chip sch_gpio_chip = { > .label = "sch_gpio", > .owner = THIS_MODULE, > @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = { > .get = sch_gpio_get, > .direction_output = sch_gpio_direction_out, > .set = sch_gpio_set, > + .to_irq = sch_gpio_to_irq, > }; > > +static void sch_gpio_irq_enable(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_register_set(sch, gpio_num, GGPE); > +} > + > +static void sch_gpio_irq_disable(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_register_clear(sch, gpio_num, GGPE); > +} > + > +static void sch_gpio_irq_ack(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1); Maybe use the new sch_gpio_register_set() here instead? > +} > + > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + unsigned long flags; > + u32 gpio_num; > + > + if (d == NULL) > + return -EINVAL; How can that happen? > + > + gpio_num = d->irq - sch->irq_base; > + > + spin_lock_irqsave(&sch->lock, flags); > + > + switch (type) { > + case IRQ_TYPE_EDGE_RISING: > + sch_gpio_register_set(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + sch_gpio_register_set(sch, gpio_num, GTNE); > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + break; > + > + case IRQ_TYPE_EDGE_BOTH: > + sch_gpio_register_set(sch, gpio_num, GTPE); > + sch_gpio_register_set(sch, gpio_num, GTNE); > + break; > + > + case IRQ_TYPE_NONE: > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + break; > + > + default: > + spin_unlock_irqrestore(&sch->lock, flags); > + return -EINVAL; > + } > + > + spin_unlock_irqrestore(&sch->lock, flags); > + > + return 0; > +} > + > +static struct irq_chip sch_irq_chip = { > + .irq_enable = sch_gpio_irq_enable, > + .irq_disable = sch_gpio_irq_disable, > + .irq_ack = sch_gpio_irq_ack, > + .irq_set_type = sch_gpio_irq_type, > +}; > + > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + irq_set_chip_data(i + sch->irq_base, sch); > + irq_set_chip_and_handler_name(i + sch->irq_base, > + &sch_irq_chip, > + handle_simple_irq, > + "sch_gpio_irq_chip"); Hmm, I wonder if this irq_set_chip_and_handler_name(i + sch->irq_base, &sch_irq_chip, handle_simple_irq, "sch_gpio_irq_chip"); looks better. Up to you. > + } > +} > + > +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + irq_set_chip_data(i + sch->irq_base, 0); > + irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0); > + } > +} > + > +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned long flags; > + unsigned int gpio_num; > + > + spin_lock_irqsave(&sch->lock, flags); > + > + for (gpio_num = 0; gpio_num < num; gpio_num++) { > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + sch_gpio_register_clear(sch, gpio_num, GGPE); > + sch_gpio_register_clear(sch, gpio_num, GSMI); > + > + if (gpio_num >= 2) > + sch_gpio_register_clear(sch, gpio_num, RGNMIEN); > + else > + sch_gpio_register_clear(sch, gpio_num, CGNMIEN); > + > + /* clear any pending interrupts */ > + sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1); > + } > + > + spin_unlock_irqrestore(&sch->lock, flags); > +} > + > +static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id) > +{ > + struct sch_gpio *sch = dev_id; > + int res; > + unsigned int i; > + int ret = IRQ_NONE; > + > + for (i = 0; i < sch->chip.ngpio; i++) { > + res = sch_gpio_reg_get(&sch->chip, i, GTS); > + if (res) { > + /* clear by setting GTS to 1 */ > + sch_gpio_reg_set(&sch->chip, i, GTS, 1); > + generic_handle_irq(sch->irq_base + i); > + ret = IRQ_HANDLED; > + } > + } > + > + return ret; > +} > + > static int sch_gpio_probe(struct platform_device *pdev) > { > struct sch_gpio *sch; > - struct resource *res; > + struct resource *res, *irq; > + int err; > > sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); > if (!sch) > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev) > pdev->name)) > return -EBUSY; > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + sch->irq_support = !!irq; > + if (sch->irq_support) { > + sch->irq_num = irq->start; > + if (sch->irq_num < 0) { Is this really possible? I mean can't you just detect the irq support by looking sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not valid. > + dev_warn(&pdev->dev, > + "failed to obtain irq number for device\n"); > + sch->irq_support = false; > + } > + } > + > spin_lock_init(&sch->lock); > sch->iobase = res->start; > sch->chip = sch_gpio_chip; > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev) > return -ENODEV; > } > > + err = gpiochip_add(&sch->chip); > + if (err < 0) > + goto err_sch_gpio; > + > + if (sch->irq_support) { > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > + NUMA_NO_NODE); > + if (sch->irq_base < 0) { > + dev_err(&pdev->dev, > + "failed to allocate GPIO IRQ descs\n"); > + goto err_sch_intr_chip; > + } > + > + /* disable interrupts */ > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > + > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > + IRQF_SHARED, KBUILD_MODNAME, sch); > + if (err) { > + dev_err(&pdev->dev, > + "%s failed to request IRQ\n", __func__); > + goto err_sch_request_irq; > + } > + > + sch_gpio_irqs_init(sch, sch->chip.ngpio); > + } > + > platform_set_drvdata(pdev, sch); > > - return gpiochip_add(&sch->chip); > + return 0; > + > +err_sch_request_irq: > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + > +err_sch_intr_chip: > + if (gpiochip_remove(&sch->chip)) > + dev_err(&pdev->dev, > + "%s gpiochip_remove() failed\n", __func__); > + > +err_sch_gpio: > + release_region(res->start, resource_size(res)); This is not needed as devm_request_region() will clean it up for you automatically. Even if probe() fails. > + > + return err; > } > > static int sch_gpio_remove(struct platform_device *pdev) > { > struct sch_gpio *sch = platform_get_drvdata(pdev); > > + if (sch->irq_support) { > + sch_gpio_irqs_deinit(sch, sch->chip.ngpio); > + > + if (sch->irq_num >= 0) > + free_irq(sch->irq_num, sch); > + > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + } > + > gpiochip_remove(&sch->chip); > return 0; > } > -- > 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 16, 2014 at 12:51 PM, Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> wrote: > Intel Quark X1000 GPIO controller supports interrupt handling for > both core power well and resume power well. This patch is to enable > the IRQ support and provide IRQ handling for Intel Quark X1000 > GPIO-SCH device driver. > > This piece of work is derived from Dan O'Donovan's initial work for > Quark X1000 enabling. > > Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> > --- > drivers/gpio/gpio-sch.c | 257 ++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 245 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c > index 952990f..dd84b1f 100644 > --- a/drivers/gpio/gpio-sch.c > +++ b/drivers/gpio/gpio-sch.c > @@ -28,17 +28,30 @@ > #include <linux/pci_ids.h> > > #include <linux/gpio.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > > #define GEN 0x00 > #define GIO 0x04 > #define GLV 0x08 > +#define GTPE 0x0C > +#define GTNE 0x10 > +#define GGPE 0x14 > +#define GSMI 0x18 > +#define GTS 0x1C > +#define CGNMIEN 0x40 > +#define RGNMIEN 0x44 > > struct sch_gpio { > struct gpio_chip chip; > + struct irq_data data; > spinlock_t lock; > unsigned short iobase; > unsigned short core_base; > unsigned short resume_base; > + int irq_base; > + int irq_num; > + bool irq_support; > }; > > #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) > @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio) > static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > + unsigned long flags; > unsigned short offset, bit; > u8 enable; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > > offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, > if (!(enable & BIT(bit))) > outb(enable | BIT(bit), sch->iobase + offset); > > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > unsigned reg) > { > + unsigned long flags; > unsigned short offset, bit; > u8 disable; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > > offset = sch_gpio_offset(sch, gpio, reg); > bit = sch_gpio_bit(sch, gpio); > @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, > if (disable & BIT(bit)) > outb(disable & ~BIT(bit), sch->iobase + offset); > > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg) > @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg, > static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_register_set(sch, gpio_num, GIO); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > return 0; > } > > @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) > static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_reg_set(gc, gpio_num, GLV, val); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > } > > static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > int val) > { > struct sch_gpio *sch = to_sch_gpio(gc); > + unsigned long flags; > > - spin_lock(&sch->lock); > + spin_lock_irqsave(&sch->lock, flags); > sch_gpio_register_clear(sch, gpio_num, GIO); > - spin_unlock(&sch->lock); > + spin_unlock_irqrestore(&sch->lock, flags); > > /* > * according to the datasheet, writing to the level register has no > @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, > return 0; > } > > +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > +{ > + struct sch_gpio *sch = to_sch_gpio(gc); > + > + return sch->irq_base + offset; > +} > + > static struct gpio_chip sch_gpio_chip = { > .label = "sch_gpio", > .owner = THIS_MODULE, > @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = { > .get = sch_gpio_get, > .direction_output = sch_gpio_direction_out, > .set = sch_gpio_set, > + .to_irq = sch_gpio_to_irq, > }; > > +static void sch_gpio_irq_enable(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_register_set(sch, gpio_num, GGPE); > +} > + > +static void sch_gpio_irq_disable(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_register_clear(sch, gpio_num, GGPE); > +} > + > +static void sch_gpio_irq_ack(struct irq_data *d) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + u32 gpio_num; > + > + gpio_num = d->irq - sch->irq_base; > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1); > +} > + > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) > +{ > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > + unsigned long flags; > + u32 gpio_num; > + > + if (d == NULL) > + return -EINVAL; > + > + gpio_num = d->irq - sch->irq_base; > + > + spin_lock_irqsave(&sch->lock, flags); > + > + switch (type) { > + case IRQ_TYPE_EDGE_RISING: > + sch_gpio_register_set(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); You will have the same problem as I pointed out in patch 1/3 that sch_gpio_register_set/clear() will try to acquire the already-locked sch->lock. No way this can ever work, or I am under a serious misapprehension. > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + sch_gpio_register_set(sch, gpio_num, GTNE); > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + break; > + > + case IRQ_TYPE_EDGE_BOTH: > + sch_gpio_register_set(sch, gpio_num, GTPE); > + sch_gpio_register_set(sch, gpio_num, GTNE); > + break; > + > + case IRQ_TYPE_NONE: > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + break; > + > + default: > + spin_unlock_irqrestore(&sch->lock, flags); > + return -EINVAL; > + } > + > + spin_unlock_irqrestore(&sch->lock, flags); > + > + return 0; > +} > + > +static struct irq_chip sch_irq_chip = { > + .irq_enable = sch_gpio_irq_enable, > + .irq_disable = sch_gpio_irq_disable, > + .irq_ack = sch_gpio_irq_ack, > + .irq_set_type = sch_gpio_irq_type, > +}; > + > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + irq_set_chip_data(i + sch->irq_base, sch); > + irq_set_chip_and_handler_name(i + sch->irq_base, > + &sch_irq_chip, > + handle_simple_irq, > + "sch_gpio_irq_chip"); > + } > +} > + > +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + irq_set_chip_data(i + sch->irq_base, 0); > + irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0); > + } > +} > + > +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num) > +{ > + unsigned long flags; > + unsigned int gpio_num; > + > + spin_lock_irqsave(&sch->lock, flags); > + > + for (gpio_num = 0; gpio_num < num; gpio_num++) { > + sch_gpio_register_clear(sch, gpio_num, GTPE); > + sch_gpio_register_clear(sch, gpio_num, GTNE); > + sch_gpio_register_clear(sch, gpio_num, GGPE); > + sch_gpio_register_clear(sch, gpio_num, GSMI); Same here. > + > + if (gpio_num >= 2) > + sch_gpio_register_clear(sch, gpio_num, RGNMIEN); > + else > + sch_gpio_register_clear(sch, gpio_num, CGNMIEN); > + > + /* clear any pending interrupts */ > + sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1); > + } > + > + spin_unlock_irqrestore(&sch->lock, flags); > +} > + > +static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id) > +{ > + struct sch_gpio *sch = dev_id; > + int res; > + unsigned int i; > + int ret = IRQ_NONE; > + > + for (i = 0; i < sch->chip.ngpio; i++) { > + res = sch_gpio_reg_get(&sch->chip, i, GTS); > + if (res) { > + /* clear by setting GTS to 1 */ > + sch_gpio_reg_set(&sch->chip, i, GTS, 1); > + generic_handle_irq(sch->irq_base + i); > + ret = IRQ_HANDLED; > + } > + } > + > + return ret; > +} > + > static int sch_gpio_probe(struct platform_device *pdev) > { > struct sch_gpio *sch; > - struct resource *res; > + struct resource *res, *irq; > + int err; > > sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); > if (!sch) > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev) > pdev->name)) > return -EBUSY; > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + sch->irq_support = !!irq; > + if (sch->irq_support) { > + sch->irq_num = irq->start; > + if (sch->irq_num < 0) { > + dev_warn(&pdev->dev, > + "failed to obtain irq number for device\n"); > + sch->irq_support = false; > + } > + } > + > spin_lock_init(&sch->lock); > sch->iobase = res->start; > sch->chip = sch_gpio_chip; > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev) > return -ENODEV; > } > > + err = gpiochip_add(&sch->chip); > + if (err < 0) > + goto err_sch_gpio; > + > + if (sch->irq_support) { > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > + NUMA_NO_NODE); > + if (sch->irq_base < 0) { > + dev_err(&pdev->dev, > + "failed to allocate GPIO IRQ descs\n"); > + goto err_sch_intr_chip; > + } > + > + /* disable interrupts */ > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > + > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > + IRQF_SHARED, KBUILD_MODNAME, sch); > + if (err) { > + dev_err(&pdev->dev, > + "%s failed to request IRQ\n", __func__); > + goto err_sch_request_irq; > + } > + > + sch_gpio_irqs_init(sch, sch->chip.ngpio); > + } > + > platform_set_drvdata(pdev, sch); > > - return gpiochip_add(&sch->chip); > + return 0; > + > +err_sch_request_irq: > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + > +err_sch_intr_chip: > + if (gpiochip_remove(&sch->chip)) > + dev_err(&pdev->dev, > + "%s gpiochip_remove() failed\n", __func__); > + > +err_sch_gpio: > + release_region(res->start, resource_size(res)); > + > + return err; > } > > static int sch_gpio_remove(struct platform_device *pdev) > { > struct sch_gpio *sch = platform_get_drvdata(pdev); > > + if (sch->irq_support) { > + sch_gpio_irqs_deinit(sch, sch->chip.ngpio); > + > + if (sch->irq_num >= 0) > + free_irq(sch->irq_num, sch); > + > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > + } > + > gpiochip_remove(&sch->chip); > return 0; > } > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > + > > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) > > +{ > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > + unsigned long flags; > > + u32 gpio_num; > > + > > + if (d == NULL) > > + return -EINVAL; > > + > > + gpio_num = d->irq - sch->irq_base; > > + > > + spin_lock_irqsave(&sch->lock, flags); > > + > > + switch (type) { > > + case IRQ_TYPE_EDGE_RISING: > > + sch_gpio_register_set(sch, gpio_num, GTPE); > > + sch_gpio_register_clear(sch, gpio_num, GTNE); > > You will have the same problem as I pointed out in patch 1/3 that > sch_gpio_register_set/clear() will try to acquire the already-locked > sch->lock. No way this can ever work, or I am under a serious > misapprehension. > > > + break; > > + > > + case IRQ_TYPE_EDGE_FALLING: > > + sch_gpio_register_set(sch, gpio_num, GTNE); > > + sch_gpio_register_clear(sch, gpio_num, GTPE); > > + break; > > + > > + case IRQ_TYPE_EDGE_BOTH: > > + sch_gpio_register_set(sch, gpio_num, GTPE); > > + sch_gpio_register_set(sch, gpio_num, GTNE); > > + break; > > + > > + case IRQ_TYPE_NONE: > > + sch_gpio_register_clear(sch, gpio_num, GTPE); > > + sch_gpio_register_clear(sch, gpio_num, GTNE); > > + break; > > + > > + default: > > + spin_unlock_irqrestore(&sch->lock, flags); > > + return -EINVAL; > > + } > > + > > + spin_unlock_irqrestore(&sch->lock, flags); > > + > > + return 0; > > +} > > + > > +static struct irq_chip sch_irq_chip = { > > + .irq_enable = sch_gpio_irq_enable, > > + .irq_disable = sch_gpio_irq_disable, > > + .irq_ack = sch_gpio_irq_ack, > > + .irq_set_type = sch_gpio_irq_type, > > +}; > > + > > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < num; i++) { > > + irq_set_chip_data(i + sch->irq_base, sch); > > + irq_set_chip_and_handler_name(i + sch->irq_base, > > + &sch_irq_chip, > > + handle_simple_irq, > > + "sch_gpio_irq_chip"); > > + } > > +} > > + > > +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < num; i++) { > > + irq_set_chip_data(i + sch->irq_base, 0); > > + irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0); > > + } > > +} > > + > > +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num) > > +{ > > + unsigned long flags; > > + unsigned int gpio_num; > > + > > + spin_lock_irqsave(&sch->lock, flags); > > + > > + for (gpio_num = 0; gpio_num < num; gpio_num++) { > > + sch_gpio_register_clear(sch, gpio_num, GTPE); > > + sch_gpio_register_clear(sch, gpio_num, GTNE); > > + sch_gpio_register_clear(sch, gpio_num, GGPE); > > + sch_gpio_register_clear(sch, gpio_num, GSMI); > > Same here. Alright. I should have noticed this double locking issue earlier. Thanks. I think the next version will be much cleaner. Thanks for spending time and effort to review. Rebecca
Sorry for the late reply, was working on something else. > -----Original Message----- > From: Westerberg, Mika > Sent: 16 October, 2014 6:19 PM > To: Chang, Rebecca Swee Fun > Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List; Denis > Turischev > Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000 > > On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote: > > Intel Quark X1000 GPIO controller supports interrupt handling for both > > core power well and resume power well. This patch is to enable the IRQ > > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device > > driver. > > > > This piece of work is derived from Dan O'Donovan's initial work for > > Quark X1000 enabling. > > > > Signed-off-by: Chang Rebecca Swee Fun > > <rebecca.swee.fun.chang@intel.com> > > In addition to what Varka Bhadram pointed out, I have few comments. See > below. > > Overall, looks good. > > > --- > > drivers/gpio/gpio-sch.c | 257 > > ++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 245 insertions(+), 12 deletions(-) > > (...) > > +static void sch_gpio_irq_disable(struct irq_data *d) { > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > + u32 gpio_num; > > + > > + gpio_num = d->irq - sch->irq_base; > > + sch_gpio_register_clear(sch, gpio_num, GGPE); } > > + > > +static void sch_gpio_irq_ack(struct irq_data *d) { > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > + u32 gpio_num; > > + > > + gpio_num = d->irq - sch->irq_base; > > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1); > > Maybe use the new sch_gpio_register_set() here instead? According to Alexandre's review feedback, sch_gpio_register_set() and sch_gpio_register_clear() actually having similar block of codes with sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc, gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress of reworking the patches in that direction. > > > +} > > + > > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) { > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > + unsigned long flags; > > + u32 gpio_num; > > + > > + if (d == NULL) > > + return -EINVAL; > > How can that happen? This is just to ensure the irq_data struct contains data. If you think this is no needed, I will remove it in next submission. (...) > > + > > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int > > +num) { > > + unsigned int i; > > + > > + for (i = 0; i < num; i++) { > > + irq_set_chip_data(i + sch->irq_base, sch); > > + irq_set_chip_and_handler_name(i + sch->irq_base, > > + &sch_irq_chip, > > + handle_simple_irq, > > + "sch_gpio_irq_chip"); > > Hmm, I wonder if this > > irq_set_chip_and_handler_name(i + sch->irq_base, > &sch_irq_chip, > handle_simple_irq, > "sch_gpio_irq_chip"); > > looks better. Up to you. > I think I will take what you've suggested. :) (...) > > static int sch_gpio_probe(struct platform_device *pdev) { > > struct sch_gpio *sch; > > - struct resource *res; > > + struct resource *res, *irq; > > + int err; > > > > sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); > > if (!sch) > > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device > *pdev) > > pdev->name)) > > return -EBUSY; > > > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + sch->irq_support = !!irq; > > + if (sch->irq_support) { > > + sch->irq_num = irq->start; > > + if (sch->irq_num < 0) { > > Is this really possible? I mean can't you just detect the irq support by looking > sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not > valid. Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. This implementation is to avoid enabling irq support if LPC_SCH passed in a negative value. As for irq 0, I think it is valid. Based on my observation in "cat /proc/interrupt" there is a 0 irq line available. > > > + dev_warn(&pdev->dev, > > + "failed to obtain irq number for device\n"); > > + sch->irq_support = false; > > + } > > + } > > + > > spin_lock_init(&sch->lock); > > sch->iobase = res->start; > > sch->chip = sch_gpio_chip; > > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device > *pdev) > > return -ENODEV; > > } > > > > + err = gpiochip_add(&sch->chip); > > + if (err < 0) > > + goto err_sch_gpio; > > + > > + if (sch->irq_support) { > > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > > + NUMA_NO_NODE); > > + if (sch->irq_base < 0) { > > + dev_err(&pdev->dev, > > + "failed to allocate GPIO IRQ descs\n"); > > + goto err_sch_intr_chip; > > + } > > + > > + /* disable interrupts */ > > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > > + > > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > > + IRQF_SHARED, KBUILD_MODNAME, sch); > > + if (err) { > > + dev_err(&pdev->dev, > > + "%s failed to request IRQ\n", __func__); > > + goto err_sch_request_irq; > > + } > > + > > + sch_gpio_irqs_init(sch, sch->chip.ngpio); > > + } > > + > > platform_set_drvdata(pdev, sch); > > > > - return gpiochip_add(&sch->chip); > > + return 0; > > + > > +err_sch_request_irq: > > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > > + > > +err_sch_intr_chip: > > + if (gpiochip_remove(&sch->chip)) > > + dev_err(&pdev->dev, > > + "%s gpiochip_remove() failed\n", __func__); > > + > > +err_sch_gpio: > > + release_region(res->start, resource_size(res)); > > This is not needed as devm_request_region() will clean it up for you > automatically. Even if probe() fails. Noted with thanks. Rebecca -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 05, 2014 at 11:33:01AM +0200, Chang, Rebecca Swee Fun wrote: > Sorry for the late reply, was working on something else. > > > -----Original Message----- > > From: Westerberg, Mika > > Sent: 16 October, 2014 6:19 PM > > To: Chang, Rebecca Swee Fun > > Cc: Linus Walleij; GPIO Subsystem Mailing List; Linux Kernel Mailing List; Denis > > Turischev > > Subject: Re: [PATCHv3 3/3] gpio: sch: Enable IRQ support for Quark X1000 > > > > On Thu, Oct 16, 2014 at 11:51:15AM +0800, Chang Rebecca Swee Fun wrote: > > > Intel Quark X1000 GPIO controller supports interrupt handling for both > > > core power well and resume power well. This patch is to enable the IRQ > > > support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device > > > driver. > > > > > > This piece of work is derived from Dan O'Donovan's initial work for > > > Quark X1000 enabling. > > > > > > Signed-off-by: Chang Rebecca Swee Fun > > > <rebecca.swee.fun.chang@intel.com> > > > > In addition to what Varka Bhadram pointed out, I have few comments. See > > below. > > > > Overall, looks good. > > > > > --- > > > drivers/gpio/gpio-sch.c | 257 > > > ++++++++++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 245 insertions(+), 12 deletions(-) > > > > (...) > > > > +static void sch_gpio_irq_disable(struct irq_data *d) { > > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > > + u32 gpio_num; > > > + > > > + gpio_num = d->irq - sch->irq_base; > > > + sch_gpio_register_clear(sch, gpio_num, GGPE); } > > > + > > > +static void sch_gpio_irq_ack(struct irq_data *d) { > > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > > + u32 gpio_num; > > > + > > > + gpio_num = d->irq - sch->irq_base; > > > + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1); > > > > Maybe use the new sch_gpio_register_set() here instead? > > According to Alexandre's review feedback, sch_gpio_register_set() and > sch_gpio_register_clear() actually having similar block of codes with > sch_gpio_reg_set(). He suggested to call sch_gpio_reg_set(gc, gpio, > reg, 1) in place of sch_gpio_register_set() and sch_gpio_reg_set(gc, > gpio, reg, 0) for sch_gpio_register_clear(). I'm now in the progress > of reworking the patches in that direction. Sounds good. > > > > > +} > > > + > > > +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) { > > > + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > > > + unsigned long flags; > > > + u32 gpio_num; > > > + > > > + if (d == NULL) > > > + return -EINVAL; > > > > How can that happen? > > This is just to ensure the irq_data struct contains data. If you think > this is no needed, I will remove it in next submission. I think you can drop that check. Also if 'd' would be NULL you will crash already in struct sch_gpio *sch = container_of(d, struct sch_gpio, data); > (...) > > > + > > > +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int > > > +num) { > > > + unsigned int i; > > > + > > > + for (i = 0; i < num; i++) { > > > + irq_set_chip_data(i + sch->irq_base, sch); > > > + irq_set_chip_and_handler_name(i + sch->irq_base, > > > + &sch_irq_chip, > > > + handle_simple_irq, > > > + "sch_gpio_irq_chip"); > > > > Hmm, I wonder if this > > > > irq_set_chip_and_handler_name(i + sch->irq_base, > > &sch_irq_chip, > > handle_simple_irq, > > "sch_gpio_irq_chip"); > > > > looks better. Up to you. > > > I think I will take what you've suggested. :) > > (...) > > > static int sch_gpio_probe(struct platform_device *pdev) { > > > struct sch_gpio *sch; > > > - struct resource *res; > > > + struct resource *res, *irq; > > > + int err; > > > > > > sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); > > > if (!sch) > > > @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device > > *pdev) > > > pdev->name)) > > > return -EBUSY; > > > > > > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > > + sch->irq_support = !!irq; > > > + if (sch->irq_support) { > > > + sch->irq_num = irq->start; > > > + if (sch->irq_num < 0) { > > > > Is this really possible? I mean can't you just detect the irq support by looking > > sch->irq_num? If it is > 0 then it has interrupt support. I think that irq 0 is not > > valid. > > Yes, I can do that, but the irq_num was being passed from MFD LPC_SCH. > This implementation is to avoid enabling irq support if LPC_SCH passed > in a negative value. As for irq 0, I think it is valid. Based on my > observation in "cat /proc/interrupt" there is a 0 irq line available. I see. In that case, how about following? irq = platform_get_irq(pdev, 0) if (irq >= 0) { /* Add irq support here */ } > > > > > > + dev_warn(&pdev->dev, > > > + "failed to obtain irq number for device\n"); > > > + sch->irq_support = false; > > > + } > > > + } > > > + > > > spin_lock_init(&sch->lock); > > > sch->iobase = res->start; > > > sch->chip = sch_gpio_chip; > > > @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device > > *pdev) > > > return -ENODEV; > > > } > > > > > > + err = gpiochip_add(&sch->chip); > > > + if (err < 0) > > > + goto err_sch_gpio; > > > + > > > + if (sch->irq_support) { > > > + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, > > > + NUMA_NO_NODE); > > > + if (sch->irq_base < 0) { > > > + dev_err(&pdev->dev, > > > + "failed to allocate GPIO IRQ descs\n"); > > > + goto err_sch_intr_chip; > > > + } > > > + > > > + /* disable interrupts */ > > > + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); > > > + > > > + err = request_irq(sch->irq_num, sch_gpio_irq_handler, > > > + IRQF_SHARED, KBUILD_MODNAME, sch); > > > + if (err) { > > > + dev_err(&pdev->dev, > > > + "%s failed to request IRQ\n", __func__); > > > + goto err_sch_request_irq; > > > + } > > > + > > > + sch_gpio_irqs_init(sch, sch->chip.ngpio); > > > + } > > > + > > > platform_set_drvdata(pdev, sch); > > > > > > - return gpiochip_add(&sch->chip); > > > + return 0; > > > + > > > +err_sch_request_irq: > > > + irq_free_descs(sch->irq_base, sch->chip.ngpio); > > > + > > > +err_sch_intr_chip: > > > + if (gpiochip_remove(&sch->chip)) > > > + dev_err(&pdev->dev, > > > + "%s gpiochip_remove() failed\n", __func__); > > > + > > > +err_sch_gpio: > > > + release_region(res->start, resource_size(res)); > > > > This is not needed as devm_request_region() will clean it up for you > > automatically. Even if probe() fails. > > Noted with thanks. > > Rebecca -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c index 952990f..dd84b1f 100644 --- a/drivers/gpio/gpio-sch.c +++ b/drivers/gpio/gpio-sch.c @@ -28,17 +28,30 @@ #include <linux/pci_ids.h> #include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/irq.h> #define GEN 0x00 #define GIO 0x04 #define GLV 0x08 +#define GTPE 0x0C +#define GTNE 0x10 +#define GGPE 0x14 +#define GSMI 0x18 +#define GTS 0x1C +#define CGNMIEN 0x40 +#define RGNMIEN 0x44 struct sch_gpio { struct gpio_chip chip; + struct irq_data data; spinlock_t lock; unsigned short iobase; unsigned short core_base; unsigned short resume_base; + int irq_base; + int irq_num; + bool irq_support; }; #define to_sch_gpio(c) container_of(c, struct sch_gpio, chip) @@ -66,10 +79,11 @@ static unsigned sch_gpio_bit(struct sch_gpio *sch, unsigned gpio) static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, unsigned reg) { + unsigned long flags; unsigned short offset, bit; u8 enable; - spin_lock(&sch->lock); + spin_lock_irqsave(&sch->lock, flags); offset = sch_gpio_offset(sch, gpio, reg); bit = sch_gpio_bit(sch, gpio); @@ -78,16 +92,17 @@ static void sch_gpio_register_set(struct sch_gpio *sch, unsigned gpio, if (!(enable & BIT(bit))) outb(enable | BIT(bit), sch->iobase + offset); - spin_unlock(&sch->lock); + spin_unlock_irqrestore(&sch->lock, flags); } static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, unsigned reg) { + unsigned long flags; unsigned short offset, bit; u8 disable; - spin_lock(&sch->lock); + spin_lock_irqsave(&sch->lock, flags); offset = sch_gpio_offset(sch, gpio, reg); bit = sch_gpio_bit(sch, gpio); @@ -96,7 +111,7 @@ static void sch_gpio_register_clear(struct sch_gpio *sch, unsigned gpio, if (disable & BIT(bit)) outb(disable & ~BIT(bit), sch->iobase + offset); - spin_unlock(&sch->lock); + spin_unlock_irqrestore(&sch->lock, flags); } static int sch_gpio_reg_get(struct gpio_chip *gc, unsigned gpio, unsigned reg) @@ -134,10 +149,11 @@ static void sch_gpio_reg_set(struct gpio_chip *gc, unsigned gpio, unsigned reg, static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num) { struct sch_gpio *sch = to_sch_gpio(gc); + unsigned long flags; - spin_lock(&sch->lock); + spin_lock_irqsave(&sch->lock, flags); sch_gpio_register_set(sch, gpio_num, GIO); - spin_unlock(&sch->lock); + spin_unlock_irqrestore(&sch->lock, flags); return 0; } @@ -149,20 +165,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num) static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val) { struct sch_gpio *sch = to_sch_gpio(gc); + unsigned long flags; - spin_lock(&sch->lock); + spin_lock_irqsave(&sch->lock, flags); sch_gpio_reg_set(gc, gpio_num, GLV, val); - spin_unlock(&sch->lock); + spin_unlock_irqrestore(&sch->lock, flags); } static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, int val) { struct sch_gpio *sch = to_sch_gpio(gc); + unsigned long flags; - spin_lock(&sch->lock); + spin_lock_irqsave(&sch->lock, flags); sch_gpio_register_clear(sch, gpio_num, GIO); - spin_unlock(&sch->lock); + spin_unlock_irqrestore(&sch->lock, flags); /* * according to the datasheet, writing to the level register has no @@ -177,6 +195,13 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num, return 0; } +static int sch_gpio_to_irq(struct gpio_chip *gc, unsigned offset) +{ + struct sch_gpio *sch = to_sch_gpio(gc); + + return sch->irq_base + offset; +} + static struct gpio_chip sch_gpio_chip = { .label = "sch_gpio", .owner = THIS_MODULE, @@ -184,12 +209,160 @@ static struct gpio_chip sch_gpio_chip = { .get = sch_gpio_get, .direction_output = sch_gpio_direction_out, .set = sch_gpio_set, + .to_irq = sch_gpio_to_irq, }; +static void sch_gpio_irq_enable(struct irq_data *d) +{ + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); + u32 gpio_num; + + gpio_num = d->irq - sch->irq_base; + sch_gpio_register_set(sch, gpio_num, GGPE); +} + +static void sch_gpio_irq_disable(struct irq_data *d) +{ + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); + u32 gpio_num; + + gpio_num = d->irq - sch->irq_base; + sch_gpio_register_clear(sch, gpio_num, GGPE); +} + +static void sch_gpio_irq_ack(struct irq_data *d) +{ + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); + u32 gpio_num; + + gpio_num = d->irq - sch->irq_base; + sch_gpio_reg_set(&(sch->chip), gpio_num, GTS, 1); +} + +static int sch_gpio_irq_type(struct irq_data *d, unsigned type) +{ + struct sch_gpio *sch = container_of(d, struct sch_gpio, data); + unsigned long flags; + u32 gpio_num; + + if (d == NULL) + return -EINVAL; + + gpio_num = d->irq - sch->irq_base; + + spin_lock_irqsave(&sch->lock, flags); + + switch (type) { + case IRQ_TYPE_EDGE_RISING: + sch_gpio_register_set(sch, gpio_num, GTPE); + sch_gpio_register_clear(sch, gpio_num, GTNE); + break; + + case IRQ_TYPE_EDGE_FALLING: + sch_gpio_register_set(sch, gpio_num, GTNE); + sch_gpio_register_clear(sch, gpio_num, GTPE); + break; + + case IRQ_TYPE_EDGE_BOTH: + sch_gpio_register_set(sch, gpio_num, GTPE); + sch_gpio_register_set(sch, gpio_num, GTNE); + break; + + case IRQ_TYPE_NONE: + sch_gpio_register_clear(sch, gpio_num, GTPE); + sch_gpio_register_clear(sch, gpio_num, GTNE); + break; + + default: + spin_unlock_irqrestore(&sch->lock, flags); + return -EINVAL; + } + + spin_unlock_irqrestore(&sch->lock, flags); + + return 0; +} + +static struct irq_chip sch_irq_chip = { + .irq_enable = sch_gpio_irq_enable, + .irq_disable = sch_gpio_irq_disable, + .irq_ack = sch_gpio_irq_ack, + .irq_set_type = sch_gpio_irq_type, +}; + +static void sch_gpio_irqs_init(struct sch_gpio *sch, unsigned int num) +{ + unsigned int i; + + for (i = 0; i < num; i++) { + irq_set_chip_data(i + sch->irq_base, sch); + irq_set_chip_and_handler_name(i + sch->irq_base, + &sch_irq_chip, + handle_simple_irq, + "sch_gpio_irq_chip"); + } +} + +static void sch_gpio_irqs_deinit(struct sch_gpio *sch, unsigned int num) +{ + unsigned int i; + + for (i = 0; i < num; i++) { + irq_set_chip_data(i + sch->irq_base, 0); + irq_set_chip_and_handler_name(i + sch->irq_base, 0, 0, 0); + } +} + +static void sch_gpio_irq_disable_all(struct sch_gpio *sch, unsigned int num) +{ + unsigned long flags; + unsigned int gpio_num; + + spin_lock_irqsave(&sch->lock, flags); + + for (gpio_num = 0; gpio_num < num; gpio_num++) { + sch_gpio_register_clear(sch, gpio_num, GTPE); + sch_gpio_register_clear(sch, gpio_num, GTNE); + sch_gpio_register_clear(sch, gpio_num, GGPE); + sch_gpio_register_clear(sch, gpio_num, GSMI); + + if (gpio_num >= 2) + sch_gpio_register_clear(sch, gpio_num, RGNMIEN); + else + sch_gpio_register_clear(sch, gpio_num, CGNMIEN); + + /* clear any pending interrupts */ + sch_gpio_reg_set(&sch->chip, gpio_num, GTS, 1); + } + + spin_unlock_irqrestore(&sch->lock, flags); +} + +static irqreturn_t sch_gpio_irq_handler(int irq, void *dev_id) +{ + struct sch_gpio *sch = dev_id; + int res; + unsigned int i; + int ret = IRQ_NONE; + + for (i = 0; i < sch->chip.ngpio; i++) { + res = sch_gpio_reg_get(&sch->chip, i, GTS); + if (res) { + /* clear by setting GTS to 1 */ + sch_gpio_reg_set(&sch->chip, i, GTS, 1); + generic_handle_irq(sch->irq_base + i); + ret = IRQ_HANDLED; + } + } + + return ret; +} + static int sch_gpio_probe(struct platform_device *pdev) { struct sch_gpio *sch; - struct resource *res; + struct resource *res, *irq; + int err; sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL); if (!sch) @@ -203,6 +376,17 @@ static int sch_gpio_probe(struct platform_device *pdev) pdev->name)) return -EBUSY; + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + sch->irq_support = !!irq; + if (sch->irq_support) { + sch->irq_num = irq->start; + if (sch->irq_num < 0) { + dev_warn(&pdev->dev, + "failed to obtain irq number for device\n"); + sch->irq_support = false; + } + } + spin_lock_init(&sch->lock); sch->iobase = res->start; sch->chip = sch_gpio_chip; @@ -251,15 +435,64 @@ static int sch_gpio_probe(struct platform_device *pdev) return -ENODEV; } + err = gpiochip_add(&sch->chip); + if (err < 0) + goto err_sch_gpio; + + if (sch->irq_support) { + sch->irq_base = irq_alloc_descs(-1, 0, sch->chip.ngpio, + NUMA_NO_NODE); + if (sch->irq_base < 0) { + dev_err(&pdev->dev, + "failed to allocate GPIO IRQ descs\n"); + goto err_sch_intr_chip; + } + + /* disable interrupts */ + sch_gpio_irq_disable_all(sch, sch->chip.ngpio); + + err = request_irq(sch->irq_num, sch_gpio_irq_handler, + IRQF_SHARED, KBUILD_MODNAME, sch); + if (err) { + dev_err(&pdev->dev, + "%s failed to request IRQ\n", __func__); + goto err_sch_request_irq; + } + + sch_gpio_irqs_init(sch, sch->chip.ngpio); + } + platform_set_drvdata(pdev, sch); - return gpiochip_add(&sch->chip); + return 0; + +err_sch_request_irq: + irq_free_descs(sch->irq_base, sch->chip.ngpio); + +err_sch_intr_chip: + if (gpiochip_remove(&sch->chip)) + dev_err(&pdev->dev, + "%s gpiochip_remove() failed\n", __func__); + +err_sch_gpio: + release_region(res->start, resource_size(res)); + + return err; } static int sch_gpio_remove(struct platform_device *pdev) { struct sch_gpio *sch = platform_get_drvdata(pdev); + if (sch->irq_support) { + sch_gpio_irqs_deinit(sch, sch->chip.ngpio); + + if (sch->irq_num >= 0) + free_irq(sch->irq_num, sch); + + irq_free_descs(sch->irq_base, sch->chip.ngpio); + } + gpiochip_remove(&sch->chip); return 0; }
Intel Quark X1000 GPIO controller supports interrupt handling for both core power well and resume power well. This patch is to enable the IRQ support and provide IRQ handling for Intel Quark X1000 GPIO-SCH device driver. This piece of work is derived from Dan O'Donovan's initial work for Quark X1000 enabling. Signed-off-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> --- drivers/gpio/gpio-sch.c | 257 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 245 insertions(+), 12 deletions(-)