On Tue, Aug 18, 2015 at 4:44 PM, Crt Mori <cmo@xxxxxxxxxxx> wrote: > Hi, few small comments inline, although I have problems applying the patch I've just reset my branch against origin/testing and it applies fine for me. > Crt > > On 18 August 2015 at 14:55, Cristina Opriceana > <cristina.opriceana@xxxxxxxxx> wrote: >> A more abstract way of handling interrupt generation in the dummy driver >> is represented by irq_work which substitutes the irq_chip, used to >> provide an IRQ line number. irq_work runs tasks from hardirq context, >> but in a NMI-safe environment and deals better with synchronization >> problems. An interrupt can be triggered by calling irq_work_queue() >> and the work will be performed by a pre-registered handler, which >> acts as a callback. >> >> Cc: Daniel Baluta <daniel.baluta@xxxxxxxxx> >> Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx> >> Signed-off-by: Cristina Opriceana <cristina.opriceana@xxxxxxxxx> >> --- >> This is the first draft for the irq_work replacement. >> As we want to move the iio dummy driver out of staging, >> we started to implement Jonathan's suggestions from here: >> <http://marc.info/?l=linux-iio&m=142403390705515&w=2> >> >> drivers/staging/iio/iio_dummy_evgen.c | 171 +++++++++----------------- >> drivers/staging/iio/iio_dummy_evgen.h | 15 ++- >> drivers/staging/iio/iio_simple_dummy.c | 6 +- >> drivers/staging/iio/iio_simple_dummy.h | 8 +- >> drivers/staging/iio/iio_simple_dummy_events.c | 71 ++++------- >> 5 files changed, 105 insertions(+), 166 deletions(-) >> >> diff --git a/drivers/staging/iio/iio_dummy_evgen.c b/drivers/staging/iio/iio_dummy_evgen.c >> index 6d38854..974ef8a 100644 >> --- a/drivers/staging/iio/iio_dummy_evgen.c >> +++ b/drivers/staging/iio/iio_dummy_evgen.c >> @@ -25,132 +25,87 @@ >> #include <linux/iio/iio.h> >> #include <linux/iio/sysfs.h> >> >> -/* Fiddly bit of faking and irq without hardware */ >> -#define IIO_EVENTGEN_NO 10 >> -/** >> - * 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 >> - */ >> -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; >> -}; >> +static LIST_HEAD(iio_dummy_event_list); >> +static DEFINE_MUTEX(iio_dummy_event_list_mutex); >> >> -/* 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 iio_dummy_event *get_event(int index) >> { >> - struct irq_chip *chip = irq_data_get_irq_chip(d); >> - struct iio_dummy_eventgen *evgen = >> - container_of(chip, struct iio_dummy_eventgen, chip); >> + struct list_head *ptr, *tmp; >> + struct iio_dummy_event *iio_event = NULL; >> >> - 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; >> + mutex_lock(&iio_dummy_event_list_mutex); >> + list_for_each_safe(ptr, tmp, &iio_dummy_event_list) { >> + iio_event = list_entry(ptr, struct iio_dummy_event, l); >> + if (iio_event->regs.reg_id == index) >> + break; >> + } >> + mutex_unlock(&iio_dummy_event_list_mutex); >> + return iio_event; >> } >> >> -static int iio_dummy_evgen_create(void) >> +int iio_dummy_evgen_create(struct iio_dev *indio_dev, int index) >> { >> - int ret, i; >> + struct iio_dummy_event *iio_event; >> >> - iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); >> - if (!iio_evgen) >> + iio_event = kzalloc(sizeof(*iio_event), GFP_KERNEL); >> + if (!iio_event) >> 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); >> - 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); >> - } >> - mutex_init(&iio_evgen->lock); >> + iio_event->dev = indio_dev; >> + iio_event->regs.reg_id = index; >> + mutex_lock(&iio_dummy_event_list_mutex); >> + list_add(&iio_event->l, &iio_dummy_event_list); >> + mutex_unlock(&iio_dummy_event_list_mutex); >> + >> return 0; >> } >> +EXPORT_SYMBOL_GPL(iio_dummy_evgen_create); >> >> -/** >> - * iio_dum - get an evgen provided irq for a device >> - * >> - * This function will give a free allocated irq to a client device. >> - * That irq can then be caused to 'fire' by using the associated sysfs file. >> - */ >> -int iio_dummy_evgen_get_irq(void) >> +void iio_dummy_init_work_handler(int index, void (*f)(struct irq_work *)) >> { >> - int i, ret = 0; >> - >> - if (!iio_evgen) >> - return -ENODEV; >> + struct iio_dummy_event *iio_event = get_event(index); >> >> - mutex_lock(&iio_evgen->lock); >> - for (i = 0; i < IIO_EVENTGEN_NO; i++) >> - if (!iio_evgen->inuse[i]) { >> - ret = iio_evgen->base + i; >> - iio_evgen->inuse[i] = true; >> - break; >> - } >> - mutex_unlock(&iio_evgen->lock); >> - if (i == IIO_EVENTGEN_NO) >> - return -ENOMEM; >> - return ret; >> + if (iio_event) >> + init_irq_work(&iio_event->work, f); >> } >> -EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq); >> +EXPORT_SYMBOL_GPL(iio_dummy_init_work_handler); >> >> -/** >> - * iio_dummy_evgen_release_irq() - give the irq back. >> - * @irq: irq being returned to the pool >> - * >> - * Used by client driver instances to give the irqs back when they disconnect >> - */ >> -void iio_dummy_evgen_release_irq(int irq) >> +struct iio_dummy_regs *iio_dummy_evgen_get_regs(int index) >> { >> - mutex_lock(&iio_evgen->lock); >> - iio_evgen->inuse[irq - iio_evgen->base] = false; >> - mutex_unlock(&iio_evgen->lock); >> -} >> -EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq); >> + struct iio_dummy_event *iio_event; >> + struct iio_dummy_regs *regs = NULL; >> >> -struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq) >> -{ >> - return &iio_evgen->regs[irq - iio_evgen->base]; >> + iio_event = get_event(index); >> + if (!iio_event) >> + goto err_regs; > This here is for future probably? - otherwise we can just return >> + >> + regs = &iio_event->regs; >> +err_regs: >> + return regs; >> } >> EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); >> >> -static void iio_dummy_evgen_free(void) >> +void iio_dummy_evgen_free(int index) >> { >> - irq_free_descs(iio_evgen->base, IIO_EVENTGEN_NO); >> - kfree(iio_evgen); >> + struct list_head *ptr, *tmp; >> + struct iio_dummy_event *iio_event; >> + >> + mutex_lock(&iio_dummy_event_list_mutex); >> + list_for_each_safe(ptr, tmp, &iio_dummy_event_list) { >> + iio_event = list_entry(ptr, struct iio_dummy_event, l); >> + if (iio_event->regs.reg_id == index) { >> + list_del(ptr); >> + kfree(iio_event); >> + break; >> + } >> + } >> + mutex_unlock(&iio_dummy_event_list_mutex); >> } >> +EXPORT_SYMBOL_GPL(iio_dummy_evgen_free); >> >> +/* Do nothing upon release */ >> static void iio_evgen_release(struct device *dev) >> { >> - iio_dummy_evgen_free(); >> } >> >> static ssize_t iio_evgen_poke(struct device *dev, >> @@ -159,6 +114,7 @@ static ssize_t iio_evgen_poke(struct device *dev, >> size_t len) >> { >> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); >> + struct iio_dummy_event *iio_event = get_event(this_attr->address); >> unsigned long event; >> int ret; >> >> @@ -166,12 +122,11 @@ static ssize_t iio_evgen_poke(struct device *dev, >> if (ret) >> return ret; >> >> - iio_evgen->regs[this_attr->address].reg_id = this_attr->address; >> - iio_evgen->regs[this_attr->address].reg_data = event; >> - >> - if (iio_evgen->enabled[this_attr->address]) >> - handle_nested_irq(iio_evgen->base + this_attr->address); >> - >> + if (iio_event) { >> + iio_event->regs.reg_data = event; >> + /* trigger interrupt */ >> + irq_work_queue(&iio_event->work); >> + } > iio_event is never checked- else return -ENODEV probably here? I left it like that to follow the return logic in the original driver. Also, the reading of event would be successful even if the interrupt generation can't be triggered. >> return len; >> } >> >> @@ -217,10 +172,6 @@ static struct device iio_evgen_dev = { >> >> static __init int iio_dummy_evgen_init(void) >> { >> - int ret = iio_dummy_evgen_create(); >> - >> - if (ret < 0) >> - return ret; >> device_initialize(&iio_evgen_dev); >> dev_set_name(&iio_evgen_dev, "iio_evgen"); >> return device_add(&iio_evgen_dev); >> diff --git a/drivers/staging/iio/iio_dummy_evgen.h b/drivers/staging/iio/iio_dummy_evgen.h >> index d044b94..b78b1eb 100644 >> --- a/drivers/staging/iio/iio_dummy_evgen.h >> +++ b/drivers/staging/iio/iio_dummy_evgen.h >> @@ -1,13 +1,22 @@ >> #ifndef _IIO_DUMMY_EVGEN_H_ >> #define _IIO_DUMMY_EVGEN_H_ >> +#include <linux/irq_work.h> >> >> struct iio_dummy_regs { >> u32 reg_id; >> u32 reg_data; >> }; >> >> -struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq); >> -int iio_dummy_evgen_get_irq(void); >> -void iio_dummy_evgen_release_irq(int irq); >> +struct iio_dummy_event { >> + struct iio_dev *dev; >> + struct iio_dummy_regs regs; >> + struct irq_work work; >> + struct list_head l; >> +}; >> + >> +int iio_dummy_evgen_create(struct iio_dev *indio_dev, int index); >> +void iio_dummy_init_work_handler(int index, void (*f)(struct irq_work *)); >> +struct iio_dummy_regs *iio_dummy_evgen_get_regs(int index); >> +void iio_dummy_evgen_free(int index); >> >> #endif /* _IIO_DUMMY_EVGEN_H_ */ >> diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c >> index 381f90f..1ae082d 100644 >> --- a/drivers/staging/iio/iio_simple_dummy.c >> +++ b/drivers/staging/iio/iio_simple_dummy.c >> @@ -635,7 +635,7 @@ static int iio_dummy_probe(int index) >> /* Specify that device provides sysfs type interfaces */ >> indio_dev->modes = INDIO_DIRECT_MODE; >> >> - ret = iio_simple_dummy_events_register(indio_dev); >> + ret = iio_simple_dummy_events_register(indio_dev, index); >> if (ret < 0) >> goto error_free_device; >> >> @@ -651,7 +651,7 @@ static int iio_dummy_probe(int index) >> error_unconfigure_buffer: >> iio_simple_dummy_unconfigure_buffer(indio_dev); >> error_unregister_events: >> - iio_simple_dummy_events_unregister(indio_dev); >> + iio_simple_dummy_events_unregister(index); >> error_free_device: >> iio_device_free(indio_dev); >> error_ret: >> @@ -682,7 +682,7 @@ static void iio_dummy_remove(int index) >> /* Buffered capture related cleanup */ >> iio_simple_dummy_unconfigure_buffer(indio_dev); >> >> - iio_simple_dummy_events_unregister(indio_dev); >> + iio_simple_dummy_events_unregister(index); >> >> /* Free all structures */ >> iio_device_free(indio_dev); >> diff --git a/drivers/staging/iio/iio_simple_dummy.h b/drivers/staging/iio/iio_simple_dummy.h >> index 8d00224..05b84f7 100644 >> --- a/drivers/staging/iio/iio_simple_dummy.h >> +++ b/drivers/staging/iio/iio_simple_dummy.h >> @@ -78,19 +78,19 @@ int iio_simple_dummy_write_event_value(struct iio_dev *indio_dev, >> enum iio_event_info info, int val, >> int val2); >> >> -int iio_simple_dummy_events_register(struct iio_dev *indio_dev); >> -void iio_simple_dummy_events_unregister(struct iio_dev *indio_dev); >> +int iio_simple_dummy_events_register(struct iio_dev *indio_dev, int index); >> +void iio_simple_dummy_events_unregister(int index); >> >> #else /* Stubs for when events are disabled at compile time */ >> >> static inline int >> -iio_simple_dummy_events_register(struct iio_dev *indio_dev) >> +iio_simple_dummy_events_register(struct iio_dev *indio_dev, int index) >> { >> return 0; >> }; >> >> static inline void >> -iio_simple_dummy_events_unregister(struct iio_dev *indio_dev) >> +iio_simple_dummy_events_unregister(int index) >> { }; >> >> #endif /* CONFIG_IIO_SIMPLE_DUMMY_EVENTS*/ >> diff --git a/drivers/staging/iio/iio_simple_dummy_events.c b/drivers/staging/iio/iio_simple_dummy_events.c >> index 73108ba..cf777b1 100644 >> --- a/drivers/staging/iio/iio_simple_dummy_events.c >> +++ b/drivers/staging/iio/iio_simple_dummy_events.c >> @@ -155,23 +155,25 @@ int iio_simple_dummy_write_event_value(struct iio_dev *indio_dev, >> >> /** >> * iio_simple_dummy_event_handler() - identify and pass on event >> - * @irq: irq of event line >> - * @private: pointer to device instance state. >> + * @work: struct irq_work which handles interrupt generation >> * >> * This handler is responsible for querying the device to find out what >> * event occurred and for then pushing that event towards userspace. >> * Here only one event occurs so we push that directly on with locally >> * grabbed timestamp. >> */ >> -static irqreturn_t iio_simple_dummy_event_handler(int irq, void *private) >> +void iio_simple_dummy_event_handler(struct irq_work *work) >> { >> - struct iio_dev *indio_dev = private; >> + struct iio_dummy_event *iio_event = container_of(work, >> + struct iio_dummy_event, >> + work); >> + struct iio_dev *indio_dev = iio_event->dev; >> struct iio_dummy_state *st = iio_priv(indio_dev); >> >> dev_dbg(&indio_dev->dev, "id %x event %x\n", >> - st->regs->reg_id, st->regs->reg_data); >> + iio_event->regs.reg_id, iio_event->regs.reg_data); >> >> - switch (st->regs->reg_data) { >> + switch (iio_event->regs.reg_data) { >> case 0: >> iio_push_event(indio_dev, >> IIO_EVENT_CODE(IIO_VOLTAGE, 0, 0, >> @@ -209,59 +211,36 @@ static irqreturn_t iio_simple_dummy_event_handler(int irq, void *private) >> default: >> break; >> } >> - >> - return IRQ_HANDLED; >> } >> >> /** >> * iio_simple_dummy_events_register() - setup interrupt handling for events >> - * @indio_dev: device instance data >> + * @indio_dev: device instance data >> + * @index: integer which identifies the dummy instance >> * >> - * This function requests the threaded interrupt to handle the events. >> - * Normally the irq is a hardware interrupt and the number comes >> - * from board configuration files. Here we get it from a companion >> - * module that fakes the interrupt for us. Note that module in >> - * no way forms part of this example. Just assume that events magically >> - * appear via the provided interrupt. >> + * Normally the irq is a hardware interrupt and the number comes from >> + * board configuration files. Here, the interrupt generation is handled >> + * by a companion module that will generate hardware interrupts using >> + * the irq_work API. Note that module in no way forms part of this example. >> + * Just assume that events magically appear via the provided interrupt. >> */ >> -int iio_simple_dummy_events_register(struct iio_dev *indio_dev) >> +int iio_simple_dummy_events_register(struct iio_dev *indio_dev, int index) >> { >> - struct iio_dummy_state *st = iio_priv(indio_dev); >> int ret; >> >> - /* Fire up event source - normally not present */ >> - st->event_irq = iio_dummy_evgen_get_irq(); >> - if (st->event_irq < 0) { >> - ret = st->event_irq; >> - goto error_ret; >> - } >> - st->regs = iio_dummy_evgen_get_regs(st->event_irq); >> - >> - ret = request_threaded_irq(st->event_irq, >> - NULL, >> - &iio_simple_dummy_event_handler, >> - IRQF_ONESHOT, >> - "iio_simple_event", >> - indio_dev); >> - if (ret < 0) >> - goto error_free_evgen; >> + ret = iio_dummy_evgen_create(indio_dev, index); >> + if (ret) >> + return ret; >> + /* register handler */ >> + iio_dummy_init_work_handler(index, iio_simple_dummy_event_handler); >> return 0; >> - >> -error_free_evgen: >> - iio_dummy_evgen_release_irq(st->event_irq); >> -error_ret: >> - return ret; >> } >> >> /** >> - * iio_simple_dummy_events_unregister() - tidy up interrupt handling on remove >> - * @indio_dev: device instance data >> + * iio_simple_dummy_events_unregister() >> + * @index: device instance identification >> */ >> -void iio_simple_dummy_events_unregister(struct iio_dev *indio_dev) >> +void iio_simple_dummy_events_unregister(int index) >> { >> - struct iio_dummy_state *st = iio_priv(indio_dev); >> - >> - free_irq(st->event_irq, indio_dev); >> - /* Not part of normal driver */ >> - iio_dummy_evgen_release_irq(st->event_irq); >> + iio_dummy_evgen_free(index); >> } >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
