| Message ID | 1377599455-9379-1-git-send-email-george.cherian@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 27, 2013 at 12:30 PM, George Cherian <george.cherian@ti.com> wrote: > This patch > - removes the irq_demux_work > - Uses devm_request_threaded_irq > - Call the user handler iff gpio_to_irq is done. > > Signed-off-by: George Cherian <george.cherian@ti.com> Can you please split this up? It seems like three different patches, and now they block each other. The threading patch is fine and I could apply it unless this was mixed up with other stuff. I'd like Kuninoro and/or Nikolay to have a look at this, so please CC them on subsequent iterations. NB: I really like that you move the irq handling to a thread, good job. > static const struct i2c_device_id pcf857x_id[] = { > @@ -89,12 +89,12 @@ struct pcf857x { > struct gpio_chip chip; > struct i2c_client *client; > struct mutex lock; /* protect 'out' */ > - struct work_struct work; /* irq demux work */ > struct irq_domain *irq_domain; /* for irq demux */ > spinlock_t slock; /* protect irq demux */ > unsigned out; /* software latch */ > unsigned status; /* current status */ > int irq; /* real irq number */ > + int irq_mapped; /* mapped gpio irqs */ This seems like an u32 or atleast unsigned, and state that its one bit flag per IRQ. How many GPIO lines are there? > -static void pcf857x_irq_demux_work(struct work_struct *work) > +static irqreturn_t pcf857x_irq(int irq, void *data) > { > - struct pcf857x *gpio = container_of(work, > - struct pcf857x, > - work); > + struct pcf857x *gpio = data; > unsigned long change, i, status, flags; > > status = gpio->read(gpio->client); > > spin_lock_irqsave(&gpio->slock, flags); > + > + /* > + * call the interrupt handler iff gpio is used as > + * interrupt source, just to avoid bad irqs > + */ > > - change = gpio->status ^ status; > + change = ((gpio->status ^ status) & gpio->irq_mapped); I don't know if that is right. If this happens you are getting what we call a "spurious IRQ" and this shall be reported to the IRQ core by returning IRQ_NONE and handled from there. > @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data) > static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq, > irq_hw_number_t hw) > { > + struct pcf857x *gpio = domain->host_data; > irq_set_chip_and_handler(virq, > &dummy_irq_chip, > handle_level_irq); > + set_irq_flags(virq, IRQF_VALID); > + gpio->irq_mapped |= (1 << hw); I'm a bit uneasy about this. It feels like its the irqdomain's responsibility to keep track of whether an IRQ is mapped or not. Maybe Grant should have a look at this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, Thanks for the review. I will split it and send v2. On 8/29/2013 6:27 PM, Linus Walleij wrote: > On Tue, Aug 27, 2013 at 12:30 PM, George Cherian <george.cherian@ti.com> wrote: > >> This patch >> - removes the irq_demux_work >> - Uses devm_request_threaded_irq >> - Call the user handler iff gpio_to_irq is done. >> >> Signed-off-by: George Cherian <george.cherian@ti.com> > Can you please split this up? It seems like three different patches, > and now they block each other. The threading patch is fine and > I could apply it unless this was mixed up with other stuff. > > I'd like Kuninoro and/or Nikolay to have a look at this, so please > CC them on subsequent iterations. okay > NB: I really like that you move the irq handling to a thread, good > job. > >> static const struct i2c_device_id pcf857x_id[] = { >> @@ -89,12 +89,12 @@ struct pcf857x { >> struct gpio_chip chip; >> struct i2c_client *client; >> struct mutex lock; /* protect 'out' */ >> - struct work_struct work; /* irq demux work */ >> struct irq_domain *irq_domain; /* for irq demux */ >> spinlock_t slock; /* protect irq demux */ >> unsigned out; /* software latch */ >> unsigned status; /* current status */ >> int irq; /* real irq number */ >> + int irq_mapped; /* mapped gpio irqs */ > This seems like an u32 or atleast unsigned, and state that its one > bit flag per IRQ. How many GPIO lines are there? pcf857x driver supports expanders with 8 and 16 gpio lines. >> -static void pcf857x_irq_demux_work(struct work_struct *work) >> +static irqreturn_t pcf857x_irq(int irq, void *data) >> { >> - struct pcf857x *gpio = container_of(work, >> - struct pcf857x, >> - work); >> + struct pcf857x *gpio = data; >> unsigned long change, i, status, flags; >> >> status = gpio->read(gpio->client); >> >> spin_lock_irqsave(&gpio->slock, flags); >> + >> + /* >> + * call the interrupt handler iff gpio is used as >> + * interrupt source, just to avoid bad irqs >> + */ >> >> - change = gpio->status ^ status; >> + change = ((gpio->status ^ status) & gpio->irq_mapped); > I don't know if that is right. > > If this happens you are getting what we call a "spurious IRQ" > and this shall be reported to the IRQ core by returning > IRQ_NONE and handled from there. While testing I got prints like bad irq and no handler installed. Now I notice its mostly when the n_latch is not passed properly. > >> @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data) >> static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq, >> irq_hw_number_t hw) >> { >> + struct pcf857x *gpio = domain->host_data; >> irq_set_chip_and_handler(virq, >> &dummy_irq_chip, >> handle_level_irq); >> + set_irq_flags(virq, IRQF_VALID); >> + gpio->irq_mapped |= (1 << hw); > I'm a bit uneasy about this. It feels like its the irqdomain's > responsibility to keep track of whether an IRQ is mapped > or not. Mainly these expanders dont have an ier sort of registers and if at all the initial value is not set proper then it gives bad irq prints only once per changed bit for which there is no handler. Should I drop this in v2? > > Maybe Grant should have a look at this. > > Yours, > Linus Walleij
On Thu, Aug 29, 2013 at 4:11 PM, George Cherian <george.cherian@ti.com> wrote: > On 8/29/2013 6:27 PM, Linus Walleij wrote: >>> int irq; /* real irq number */ >>> + int irq_mapped; /* mapped gpio irqs */ >> >> This seems like an u32 or atleast unsigned, and state that its one >> bit flag per IRQ. How many GPIO lines are there? > > pcf857x driver supports expanders with 8 and 16 gpio lines. OK should be an u16 then? >>> static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned >>> int virq, >>> irq_hw_number_t hw) >>> { >>> + struct pcf857x *gpio = domain->host_data; >>> irq_set_chip_and_handler(virq, >>> &dummy_irq_chip, >>> handle_level_irq); >>> + set_irq_flags(virq, IRQF_VALID); >>> + gpio->irq_mapped |= (1 << hw); >> >> I'm a bit uneasy about this. It feels like its the irqdomain's >> responsibility to keep track of whether an IRQ is mapped >> or not. > > Mainly these expanders dont have an ier sort of registers and if at all the > initial value is not set proper > then it gives bad irq prints only once per changed bit for which there is no > handler. > Should I drop this in v2? I don't know, I mean if it's causing a problem or filling the console with error prints it is really annoying, and if there is nothing to do about the condition then there is not much more we can do I guess. But please take a second look. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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-pcf857x.c b/drivers/gpio/gpio-pcf857x.c index 947cff4..aebbba6 100644 --- a/drivers/gpio/gpio-pcf857x.c +++ b/drivers/gpio/gpio-pcf857x.c @@ -30,7 +30,7 @@ #include <linux/of_device.h> #include <linux/slab.h> #include <linux/spinlock.h> -#include <linux/workqueue.h> +#include <linux/of_irq.h> static const struct i2c_device_id pcf857x_id[] = { @@ -89,12 +89,12 @@ struct pcf857x { struct gpio_chip chip; struct i2c_client *client; struct mutex lock; /* protect 'out' */ - struct work_struct work; /* irq demux work */ struct irq_domain *irq_domain; /* for irq demux */ spinlock_t slock; /* protect irq demux */ unsigned out; /* software latch */ unsigned status; /* current status */ int irq; /* real irq number */ + int irq_mapped; /* mapped gpio irqs */ int (*write)(struct i2c_client *client, unsigned data); int (*read)(struct i2c_client *client); @@ -187,38 +187,35 @@ static void pcf857x_set(struct gpio_chip *chip, unsigned offset, int value) static int pcf857x_to_irq(struct gpio_chip *chip, unsigned offset) { struct pcf857x *gpio = container_of(chip, struct pcf857x, chip); + int ret; - return irq_create_mapping(gpio->irq_domain, offset); + ret = irq_create_mapping(gpio->irq_domain, offset); + if (ret > 0) + gpio->irq_mapped |= (1 << offset); + + return ret; } -static void pcf857x_irq_demux_work(struct work_struct *work) +static irqreturn_t pcf857x_irq(int irq, void *data) { - struct pcf857x *gpio = container_of(work, - struct pcf857x, - work); + struct pcf857x *gpio = data; unsigned long change, i, status, flags; status = gpio->read(gpio->client); spin_lock_irqsave(&gpio->slock, flags); + + /* + * call the interrupt handler iff gpio is used as + * interrupt source, just to avoid bad irqs + */ - change = gpio->status ^ status; + change = ((gpio->status ^ status) & gpio->irq_mapped); for_each_set_bit(i, &change, gpio->chip.ngpio) generic_handle_irq(irq_find_mapping(gpio->irq_domain, i)); gpio->status = status; spin_unlock_irqrestore(&gpio->slock, flags); -} - -static irqreturn_t pcf857x_irq_demux(int irq, void *data) -{ - struct pcf857x *gpio = data; - - /* - * pcf857x can't read/write data here, - * since i2c data access might go to sleep. - */ - schedule_work(&gpio->work); return IRQ_HANDLED; } @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data) static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq, irq_hw_number_t hw) { + struct pcf857x *gpio = domain->host_data; irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_level_irq); + set_irq_flags(virq, IRQF_VALID); + gpio->irq_mapped |= (1 << hw); + return 0; } @@ -241,30 +242,31 @@ static void pcf857x_irq_domain_cleanup(struct pcf857x *gpio) if (gpio->irq_domain) irq_domain_remove(gpio->irq_domain); - if (gpio->irq) - free_irq(gpio->irq, gpio); } static int pcf857x_irq_domain_init(struct pcf857x *gpio, struct i2c_client *client) { int status; + struct device_node *node; + node = client->dev.of_node; gpio->irq_domain = irq_domain_add_linear(client->dev.of_node, gpio->chip.ngpio, &pcf857x_irq_domain_ops, - NULL); + gpio); if (!gpio->irq_domain) goto fail; /* enable real irq */ - status = request_irq(client->irq, pcf857x_irq_demux, 0, - dev_name(&client->dev), gpio); + status = devm_request_threaded_irq(&client->dev, client->irq, + NULL, pcf857x_irq, IRQF_ONESHOT | + IRQF_TRIGGER_FALLING, + dev_name(&client->dev), gpio); if (status) goto fail; /* enable gpio_to_irq() */ - INIT_WORK(&gpio->work, pcf857x_irq_demux_work); gpio->chip.to_irq = pcf857x_to_irq; gpio->irq = client->irq;
This patch - removes the irq_demux_work - Uses devm_request_threaded_irq - Call the user handler iff gpio_to_irq is done. Signed-off-by: George Cherian <george.cherian@ti.com> --- drivers/gpio/gpio-pcf857x.c | 52 +++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 25 deletions(-)