| Message ID | 20170926164910.31137-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/26/2017 06:49 PM, Bartosz Golaszewski wrote: > Switch to using the recently added interrupt simulator for dummy irqs. > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> Thanks for doing this. [...] > static int iio_dummy_evgen_create(void) > { > - int ret, i; > + int ret; > > iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); > if (!iio_evgen) > return -ENOMEM; > > - iio_evgen->base = irq_alloc_descs(-1, 0, IIO_EVENTGEN_NO, 0); > - if (iio_evgen->base < 0) { > - ret = iio_evgen->base; > - kfree(iio_evgen); > + ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO); > + if (ret) What happened to the kfree(iio_evgen)? > return ret; > - } > - iio_evgen->chip.name = iio_evgen_name; > - iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask; > - iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask; > - for (i = 0; i < IIO_EVENTGEN_NO; i++) { > - irq_set_chip(iio_evgen->base + i, &iio_evgen->chip); > - irq_set_handler(iio_evgen->base + i, &handle_simple_irq); > - irq_modify_status(iio_evgen->base + i, > - IRQ_NOREQUEST | IRQ_NOAUTOEN, > - IRQ_NOPROBE); > - } > - init_irq_work(&iio_evgen->handler.work, iio_dummy_work_handler); > + > + iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); I saw you introduced irq_sim_baseirq(), to get rid of ->base. But as far as I can see the only remaining places where we need the base is to do the reverse lookup from IRQ to index. It would be nice if the irq_sim had a function for that, then we wouldn't have to know about the base at all. > mutex_init(&iio_evgen->lock); > + > return 0; > } > > @@ -132,15 +79,17 @@ int iio_dummy_evgen_get_irq(void) > return -ENODEV; > > mutex_lock(&iio_evgen->lock); > - for (i = 0; i < IIO_EVENTGEN_NO; i++) > + for (i = 0; i < IIO_EVENTGEN_NO; i++) { > if (!iio_evgen->inuse[i]) { > - ret = iio_evgen->base + i; > + ret = irq_sim_irqnum(&iio_evgen->irq_sim, i); > iio_evgen->inuse[i] = true; > break; > } > + } > mutex_unlock(&iio_evgen->lock); > if (i == IIO_EVENTGEN_NO) > return -ENOMEM; > + > return ret; > } -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-09-27 19:12 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>: > On 09/26/2017 06:49 PM, Bartosz Golaszewski wrote: >> Switch to using the recently added interrupt simulator for dummy irqs. >> >> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > > Thanks for doing this. > > [...] >> static int iio_dummy_evgen_create(void) >> { >> - int ret, i; >> + int ret; >> >> iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); >> if (!iio_evgen) >> return -ENOMEM; >> >> - iio_evgen->base = irq_alloc_descs(-1, 0, IIO_EVENTGEN_NO, 0); >> - if (iio_evgen->base < 0) { >> - ret = iio_evgen->base; >> - kfree(iio_evgen); >> + ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO); >> + if (ret) > > What happened to the kfree(iio_evgen)? > Oops, nice catch, thanks! >> return ret; >> - } >> - iio_evgen->chip.name = iio_evgen_name; >> - iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask; >> - iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask; >> - for (i = 0; i < IIO_EVENTGEN_NO; i++) { >> - irq_set_chip(iio_evgen->base + i, &iio_evgen->chip); >> - irq_set_handler(iio_evgen->base + i, &handle_simple_irq); >> - irq_modify_status(iio_evgen->base + i, >> - IRQ_NOREQUEST | IRQ_NOAUTOEN, >> - IRQ_NOPROBE); >> - } >> - init_irq_work(&iio_evgen->handler.work, iio_dummy_work_handler); >> + >> + iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); > I saw you introduced irq_sim_baseirq(), to get rid of ->base. But as far as > I can see the only remaining places where we need the base is to do the > reverse lookup from IRQ to index. It would be nice if the irq_sim had a > function for that, then we wouldn't have to know about the base at all. > I'm not sure I understand. Irq sim doesn't know anything about iio data structures, so how would such a reverse lookup work in this case? Best regards, Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/27/2017 09:23 PM, Bartosz Golaszewski wrote: >>> return ret; >>> - } >>> - iio_evgen->chip.name = iio_evgen_name; >>> - iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask; >>> - iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask; >>> - for (i = 0; i < IIO_EVENTGEN_NO; i++) { >>> - irq_set_chip(iio_evgen->base + i, &iio_evgen->chip); >>> - irq_set_handler(iio_evgen->base + i, &handle_simple_irq); >>> - irq_modify_status(iio_evgen->base + i, >>> - IRQ_NOREQUEST | IRQ_NOAUTOEN, >>> - IRQ_NOPROBE); >>> - } >>> - init_irq_work(&iio_evgen->handler.work, iio_dummy_work_handler); >>> + >>> + iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); >> I saw you introduced irq_sim_baseirq(), to get rid of ->base. But as far as >> I can see the only remaining places where we need the base is to do the >> reverse lookup from IRQ to index. It would be nice if the irq_sim had a >> function for that, then we wouldn't have to know about the base at all. >> > > I'm not sure I understand. Irq sim doesn't know anything about iio > data structures, so how would such a reverse lookup work in this case? Reverse lookup in the sense of translating IRQ number to offset. All we ever do with the base in the IIO code is `irq - base` to get the offset. I'd hide that calculation in a helper in the irq_sim code. This nicely splits functionality and implementation, the IIO code doesn't have to know how to get offset from the IRQ. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-09-28 10:04 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>: > On 09/27/2017 09:23 PM, Bartosz Golaszewski wrote: >>>> return ret; >>>> - } >>>> - iio_evgen->chip.name = iio_evgen_name; >>>> - iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask; >>>> - iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask; >>>> - for (i = 0; i < IIO_EVENTGEN_NO; i++) { >>>> - irq_set_chip(iio_evgen->base + i, &iio_evgen->chip); >>>> - irq_set_handler(iio_evgen->base + i, &handle_simple_irq); >>>> - irq_modify_status(iio_evgen->base + i, >>>> - IRQ_NOREQUEST | IRQ_NOAUTOEN, >>>> - IRQ_NOPROBE); >>>> - } >>>> - init_irq_work(&iio_evgen->handler.work, iio_dummy_work_handler); >>>> + >>>> + iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); >>> I saw you introduced irq_sim_baseirq(), to get rid of ->base. But as far as >>> I can see the only remaining places where we need the base is to do the >>> reverse lookup from IRQ to index. It would be nice if the irq_sim had a >>> function for that, then we wouldn't have to know about the base at all. >>> >> >> I'm not sure I understand. Irq sim doesn't know anything about iio >> data structures, so how would such a reverse lookup work in this case? > > Reverse lookup in the sense of translating IRQ number to offset. All we ever > do with the base in the IIO code is `irq - base` to get the offset. I'd hide > that calculation in a helper in the irq_sim code. This nicely splits > functionality and implementation, the IIO code doesn't have to know how to > get offset from the IRQ. I see. Sounds like a good improvement for 4.16. In the meantime, I'll send v2 with the missing kfree() added. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe linux-iio" 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/iio/dummy/Kconfig b/drivers/iio/dummy/Kconfig index aa5824d96a43..5a29fbd3c531 100644 --- a/drivers/iio/dummy/Kconfig +++ b/drivers/iio/dummy/Kconfig @@ -5,7 +5,7 @@ menu "IIO dummy driver" depends on IIO config IIO_DUMMY_EVGEN - select IRQ_WORK + select IRQ_SIM tristate config IIO_SIMPLE_DUMMY diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c index 9e83f348df51..29e1e402388e 100644 --- a/drivers/iio/dummy/iio_dummy_evgen.c +++ b/drivers/iio/dummy/iio_dummy_evgen.c @@ -24,97 +24,44 @@ #include "iio_dummy_evgen.h" #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> -#include <linux/irq_work.h> +#include <linux/irq_sim.h> /* Fiddly bit of faking and irq without hardware */ #define IIO_EVENTGEN_NO 10 /** - * struct iio_dummy_handle_irq - helper struct to simulate interrupt generation - * @work: irq_work used to run handlers from hardirq context - * @irq: fake irq line number to trigger an interrupt - */ -struct iio_dummy_handle_irq { - struct irq_work work; - int irq; -}; - -/** - * struct iio_dummy_evgen - evgen state - * @chip: irq chip we are faking - * @base: base of irq range - * @enabled: mask of which irqs are enabled - * @inuse: mask of which irqs are connected * @regs: irq regs we are faking * @lock: protect the evgen state - * @handler: helper for a 'hardware-like' interrupt simulation + * @inuse: mask of which irqs are connected + * @irq_sim: interrupt simulator + * @base: base of irq range */ struct iio_dummy_eventgen { - struct irq_chip chip; - int base; - bool enabled[IIO_EVENTGEN_NO]; - bool inuse[IIO_EVENTGEN_NO]; struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; struct mutex lock; - struct iio_dummy_handle_irq handler; + bool inuse[IIO_EVENTGEN_NO]; + struct irq_sim irq_sim; + int base; }; /* We can only ever have one instance of this 'device' */ static struct iio_dummy_eventgen *iio_evgen; -static const char *iio_evgen_name = "iio_dummy_evgen"; - -static void iio_dummy_event_irqmask(struct irq_data *d) -{ - struct irq_chip *chip = irq_data_get_irq_chip(d); - struct iio_dummy_eventgen *evgen = - container_of(chip, struct iio_dummy_eventgen, chip); - - evgen->enabled[d->irq - evgen->base] = false; -} - -static void iio_dummy_event_irqunmask(struct irq_data *d) -{ - struct irq_chip *chip = irq_data_get_irq_chip(d); - struct iio_dummy_eventgen *evgen = - container_of(chip, struct iio_dummy_eventgen, chip); - - evgen->enabled[d->irq - evgen->base] = true; -} - -static void iio_dummy_work_handler(struct irq_work *work) -{ - struct iio_dummy_handle_irq *irq_handler; - - irq_handler = container_of(work, struct iio_dummy_handle_irq, work); - handle_simple_irq(irq_to_desc(irq_handler->irq)); -} static int iio_dummy_evgen_create(void) { - int ret, i; + int ret; iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); if (!iio_evgen) return -ENOMEM; - iio_evgen->base = irq_alloc_descs(-1, 0, IIO_EVENTGEN_NO, 0); - if (iio_evgen->base < 0) { - ret = iio_evgen->base; - kfree(iio_evgen); + ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO); + if (ret) return ret; - } - iio_evgen->chip.name = iio_evgen_name; - iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask; - iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask; - for (i = 0; i < IIO_EVENTGEN_NO; i++) { - irq_set_chip(iio_evgen->base + i, &iio_evgen->chip); - irq_set_handler(iio_evgen->base + i, &handle_simple_irq); - irq_modify_status(iio_evgen->base + i, - IRQ_NOREQUEST | IRQ_NOAUTOEN, - IRQ_NOPROBE); - } - init_irq_work(&iio_evgen->handler.work, iio_dummy_work_handler); + + iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); mutex_init(&iio_evgen->lock); + return 0; } @@ -132,15 +79,17 @@ int iio_dummy_evgen_get_irq(void) return -ENODEV; mutex_lock(&iio_evgen->lock); - for (i = 0; i < IIO_EVENTGEN_NO; i++) + for (i = 0; i < IIO_EVENTGEN_NO; i++) { if (!iio_evgen->inuse[i]) { - ret = iio_evgen->base + i; + ret = irq_sim_irqnum(&iio_evgen->irq_sim, i); iio_evgen->inuse[i] = true; break; } + } mutex_unlock(&iio_evgen->lock); if (i == IIO_EVENTGEN_NO) return -ENOMEM; + return ret; } EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq); @@ -167,7 +116,7 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); static void iio_dummy_evgen_free(void) { - irq_free_descs(iio_evgen->base, IIO_EVENTGEN_NO); + irq_sim_fini(&iio_evgen->irq_sim); kfree(iio_evgen); } @@ -192,9 +141,7 @@ static ssize_t iio_evgen_poke(struct device *dev, iio_evgen->regs[this_attr->address].reg_id = this_attr->address; iio_evgen->regs[this_attr->address].reg_data = event; - iio_evgen->handler.irq = iio_evgen->base + this_attr->address; - if (iio_evgen->enabled[this_attr->address]) - irq_work_queue(&iio_evgen->handler.work); + irq_sim_fire(&iio_evgen->irq_sim, this_attr->address); return len; }
Switch to using the recently added interrupt simulator for dummy irqs. Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> --- bloat-o-meter output: add/remove: 0/3 grow/shrink: 1/5 up/down: 12/-540 (-528) function old new delta iio_dummy_evgen_get_irq 136 148 +12 iio_evgen_release 40 36 -4 iio_dummy_evgen_get_regs 32 28 -4 iio_dummy_work_handler 20 - -20 iio_dummy_event_irqunmask 32 - -32 iio_dummy_event_irqmask 32 - -32 iio_evgen_poke 140 100 -40 init_module 376 172 -204 iio_dummy_evgen_init 376 172 -204 Total: Before=2804, After=2276, chg -18.83% drivers/iio/dummy/Kconfig | 2 +- drivers/iio/dummy/iio_dummy_evgen.c | 91 ++++++++----------------------------- 2 files changed, 20 insertions(+), 73 deletions(-)