| Message ID | 20190129084411.30495-4-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mockup: improve the user-space testing interface | expand |
Hello Bartosz, On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote: > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > +void irq_sim_fire_type(struct irq_sim *sim, > + unsigned int offset, unsigned int type) > { > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > - if (ctx->enabled) { > + /* Only care about relevant flags. */ > + type &= IRQ_TYPE_SENSE_MASK; > + > + if (ctx->enabled && (ctx->type & type)) { > set_bit(offset, sim->work_ctx.pending); > irq_work_queue(&sim->work_ctx.work); > } > } > -EXPORT_SYMBOL_GPL(irq_sim_fire); > +EXPORT_SYMBOL_GPL(irq_sim_fire_type); This looks better than the previous variant. I wonder if it would be still more sensible to have type only in the mockup driver. But I don't have the complete picture here and it might be easier this way. Best regards Uwe
wt., 29 sty 2019 o 10:07 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > Hello Bartosz, > > On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote: > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > +void irq_sim_fire_type(struct irq_sim *sim, > > + unsigned int offset, unsigned int type) > > { > > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > > > - if (ctx->enabled) { > > + /* Only care about relevant flags. */ > > + type &= IRQ_TYPE_SENSE_MASK; > > + > > + if (ctx->enabled && (ctx->type & type)) { > > set_bit(offset, sim->work_ctx.pending); > > irq_work_queue(&sim->work_ctx.work); > > } > > } > > -EXPORT_SYMBOL_GPL(irq_sim_fire); > > +EXPORT_SYMBOL_GPL(irq_sim_fire_type); > > This looks better than the previous variant. I wonder if it would be > still more sensible to have type only in the mockup driver. But I don't > have the complete picture here and it might be easier this way. > I'm afraid I don't follow. Wasn't that the way it was done in v1? Bart
On Tue, Jan 29, 2019 at 12:01:37PM +0100, Bartosz Golaszewski wrote: > wt., 29 sty 2019 o 10:07 Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> napisał(a): > > > > Hello Bartosz, > > > > On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote: > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > > +void irq_sim_fire_type(struct irq_sim *sim, > > > + unsigned int offset, unsigned int type) > > > { > > > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > > > > > - if (ctx->enabled) { > > > + /* Only care about relevant flags. */ > > > + type &= IRQ_TYPE_SENSE_MASK; > > > + > > > + if (ctx->enabled && (ctx->type & type)) { > > > set_bit(offset, sim->work_ctx.pending); > > > irq_work_queue(&sim->work_ctx.work); > > > } > > > } > > > -EXPORT_SYMBOL_GPL(irq_sim_fire); > > > +EXPORT_SYMBOL_GPL(irq_sim_fire_type); > > > > This looks better than the previous variant. I wonder if it would be > > still more sensible to have type only in the mockup driver. But I don't > > have the complete picture here and it might be easier this way. > > > > I'm afraid I don't follow. Wasn't that the way it was done in v1? No, in v1 you already had "type" in the irq_sim driver and the logic if the irq should trigger in mockup. My wondering is about having both in the mockup driver. Then you have the tracking of the line's level and the logic if it should trigger an irq in a single place. But as I said, I'm not sure if this is better than your proposed solution in v2. Best regards Uwe
wt., 29 sty 2019 o 13:55 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > On Tue, Jan 29, 2019 at 12:01:37PM +0100, Bartosz Golaszewski wrote: > > wt., 29 sty 2019 o 10:07 Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> napisał(a): > > > > > > Hello Bartosz, > > > > > > On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote: > > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > > > +void irq_sim_fire_type(struct irq_sim *sim, > > > > + unsigned int offset, unsigned int type) > > > > { > > > > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > > > > > > > - if (ctx->enabled) { > > > > + /* Only care about relevant flags. */ > > > > + type &= IRQ_TYPE_SENSE_MASK; > > > > + > > > > + if (ctx->enabled && (ctx->type & type)) { > > > > set_bit(offset, sim->work_ctx.pending); > > > > irq_work_queue(&sim->work_ctx.work); > > > > } > > > > } > > > > -EXPORT_SYMBOL_GPL(irq_sim_fire); > > > > +EXPORT_SYMBOL_GPL(irq_sim_fire_type); > > > > > > This looks better than the previous variant. I wonder if it would be > > > still more sensible to have type only in the mockup driver. But I don't > > > have the complete picture here and it might be easier this way. > > > > > > > I'm afraid I don't follow. Wasn't that the way it was done in v1? > > No, in v1 you already had "type" in the irq_sim driver and the logic if > the irq should trigger in mockup. My wondering is about having both in > the mockup driver. > > Then you have the tracking of the line's level and the logic if it should > trigger an irq in a single place. > > But as I said, I'm not sure if this is better than your proposed > solution in v2. > I think this a good compromise in case some other user would need it. If there are no major objections to it, I'd like to keep it that way. Bart
On 29/01/2019 08:44, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Provide a more specialized variant of irq_sim_fire() that allows to > specify the type of the fired interrupt. The type is stored in the > dummy irq context struct via the set_type callback. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > include/linux/irq_sim.h | 9 ++++++++- > kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > index b96c2f752320..647a6c8ffb31 100644 > --- a/include/linux/irq_sim.h > +++ b/include/linux/irq_sim.h > @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { > > struct irq_sim_irq_ctx { > bool enabled; > + unsigned int type; > }; > > struct irq_sim { > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > unsigned int num_irqs); > void irq_sim_fini(struct irq_sim *sim); > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > +void irq_sim_fire_type(struct irq_sim *sim, > + unsigned int offset, unsigned int type); > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > +{ > + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); > +} > + > #endif /* _LINUX_IRQ_SIM_H */ > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > index 2bcdbab1bc5a..e3160b5e59b8 100644 > --- a/kernel/irq/irq_sim.c > +++ b/kernel/irq/irq_sim.c > @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) > irq_ctx->enabled = true; > } > > +static int irq_sim_set_type(struct irq_data *data, unsigned int type) > +{ > + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); > + > + irq_ctx->type = type; > + > + return 0; > +} > + > static void irq_sim_handle_irq(struct irq_work *work) > { > struct irq_sim_work_ctx *work_ctx; > @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > sim->chip.name = "irq_sim"; > sim->chip.irq_mask = irq_sim_irqmask; > sim->chip.irq_unmask = irq_sim_irqunmask; > + sim->chip.irq_set_type = irq_sim_set_type; > > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > if (!sim->work_ctx.pending) { > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) > } > > /** > - * irq_sim_fire - Enqueue an interrupt. > + * irq_sim_fire_type - Enqueue an interrupt. > * > * @sim: The interrupt simulator object. > * @offset: Offset of the simulated interrupt which should be fired. > + * @type: Type of the fired interrupt. Must be one of the following: > + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, > + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, > + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT > */ > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > +void irq_sim_fire_type(struct irq_sim *sim, > + unsigned int offset, unsigned int type) > { > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > - if (ctx->enabled) { > + /* Only care about relevant flags. */ > + type &= IRQ_TYPE_SENSE_MASK; > + > + if (ctx->enabled && (ctx->type & type)) { I wonder how realistic this is, given that you do not track the release of a level. In short, mo matter what the type is, you treat everything as edge. What is the point of this? > set_bit(offset, sim->work_ctx.pending); > irq_work_queue(&sim->work_ctx.work); > } > } > -EXPORT_SYMBOL_GPL(irq_sim_fire); > +EXPORT_SYMBOL_GPL(irq_sim_fire_type); > > /** > * irq_sim_irqnum - Get the allocated number of a dummy interrupt. > Thanks, M.
wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > On 29/01/2019 08:44, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Provide a more specialized variant of irq_sim_fire() that allows to > > specify the type of the fired interrupt. The type is stored in the > > dummy irq context struct via the set_type callback. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > include/linux/irq_sim.h | 9 ++++++++- > > kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- > > 2 files changed, 30 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > > index b96c2f752320..647a6c8ffb31 100644 > > --- a/include/linux/irq_sim.h > > +++ b/include/linux/irq_sim.h > > @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { > > > > struct irq_sim_irq_ctx { > > bool enabled; > > + unsigned int type; > > }; > > > > struct irq_sim { > > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > > int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > > unsigned int num_irqs); > > void irq_sim_fini(struct irq_sim *sim); > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > > +void irq_sim_fire_type(struct irq_sim *sim, > > + unsigned int offset, unsigned int type); > > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > > > > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > +{ > > + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); > > +} > > + > > #endif /* _LINUX_IRQ_SIM_H */ > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > > index 2bcdbab1bc5a..e3160b5e59b8 100644 > > --- a/kernel/irq/irq_sim.c > > +++ b/kernel/irq/irq_sim.c > > @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) > > irq_ctx->enabled = true; > > } > > > > +static int irq_sim_set_type(struct irq_data *data, unsigned int type) > > +{ > > + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); > > + > > + irq_ctx->type = type; > > + > > + return 0; > > +} > > + > > static void irq_sim_handle_irq(struct irq_work *work) > > { > > struct irq_sim_work_ctx *work_ctx; > > @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > > sim->chip.name = "irq_sim"; > > sim->chip.irq_mask = irq_sim_irqmask; > > sim->chip.irq_unmask = irq_sim_irqunmask; > > + sim->chip.irq_set_type = irq_sim_set_type; > > > > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > > if (!sim->work_ctx.pending) { > > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) > > } > > > > /** > > - * irq_sim_fire - Enqueue an interrupt. > > + * irq_sim_fire_type - Enqueue an interrupt. > > * > > * @sim: The interrupt simulator object. > > * @offset: Offset of the simulated interrupt which should be fired. > > + * @type: Type of the fired interrupt. Must be one of the following: > > + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, > > + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, > > + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT > > */ > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > +void irq_sim_fire_type(struct irq_sim *sim, > > + unsigned int offset, unsigned int type) > > { > > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > > > - if (ctx->enabled) { > > + /* Only care about relevant flags. */ > > + type &= IRQ_TYPE_SENSE_MASK; > > + > > + if (ctx->enabled && (ctx->type & type)) { > > I wonder how realistic this is, given that you do not track the release > of a level. In short, mo matter what the type is, you treat everything > as edge. > > What is the point of this? > When userspace wants to monitor GPIO line interrupts, the GPIO framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, IRQF_TRIGGER_RISING or both. The testing module tries to act like real hardware and so if we pass only one of the *_TRIGGER_* flags, we want the simulated interrupt of corresponding type to be fired. Another solution - if you don't like this one - would be to have more specialized functions: irq_sim_fire_rising() and irq_sim_fire_falling(). How about that? Bart > > set_bit(offset, sim->work_ctx.pending); > > irq_work_queue(&sim->work_ctx.work); > > } > > } > > -EXPORT_SYMBOL_GPL(irq_sim_fire); > > +EXPORT_SYMBOL_GPL(irq_sim_fire_type); > > > > /** > > * irq_sim_irqnum - Get the allocated number of a dummy interrupt. > > > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On Tue, Feb 12, 2019 at 10:19:20AM +0100, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > > > On 29/01/2019 08:44, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Provide a more specialized variant of irq_sim_fire() that allows to > > > specify the type of the fired interrupt. The type is stored in the > > > dummy irq context struct via the set_type callback. > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > --- > > > include/linux/irq_sim.h | 9 ++++++++- > > > kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- > > > 2 files changed, 30 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > > > index b96c2f752320..647a6c8ffb31 100644 > > > --- a/include/linux/irq_sim.h > > > +++ b/include/linux/irq_sim.h > > > @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { > > > > > > struct irq_sim_irq_ctx { > > > bool enabled; > > > + unsigned int type; > > > }; > > > > > > struct irq_sim { > > > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > > > int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > > > unsigned int num_irqs); > > > void irq_sim_fini(struct irq_sim *sim); > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > > > +void irq_sim_fire_type(struct irq_sim *sim, > > > + unsigned int offset, unsigned int type); > > > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > > > > > > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > > +{ > > > + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); > > > +} > > > + > > > #endif /* _LINUX_IRQ_SIM_H */ > > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > > > index 2bcdbab1bc5a..e3160b5e59b8 100644 > > > --- a/kernel/irq/irq_sim.c > > > +++ b/kernel/irq/irq_sim.c > > > @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) > > > irq_ctx->enabled = true; > > > } > > > > > > +static int irq_sim_set_type(struct irq_data *data, unsigned int type) > > > +{ > > > + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); > > > + > > > + irq_ctx->type = type; > > > + > > > + return 0; > > > +} > > > + > > > static void irq_sim_handle_irq(struct irq_work *work) > > > { > > > struct irq_sim_work_ctx *work_ctx; > > > @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > > > sim->chip.name = "irq_sim"; > > > sim->chip.irq_mask = irq_sim_irqmask; > > > sim->chip.irq_unmask = irq_sim_irqunmask; > > > + sim->chip.irq_set_type = irq_sim_set_type; > > > > > > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > > > if (!sim->work_ctx.pending) { > > > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) > > > } > > > > > > /** > > > - * irq_sim_fire - Enqueue an interrupt. > > > + * irq_sim_fire_type - Enqueue an interrupt. > > > * > > > * @sim: The interrupt simulator object. > > > * @offset: Offset of the simulated interrupt which should be fired. > > > + * @type: Type of the fired interrupt. Must be one of the following: > > > + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, > > > + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, > > > + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT > > > */ > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > > +void irq_sim_fire_type(struct irq_sim *sim, > > > + unsigned int offset, unsigned int type) > > > { > > > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > > > > > - if (ctx->enabled) { > > > + /* Only care about relevant flags. */ > > > + type &= IRQ_TYPE_SENSE_MASK; > > > + > > > + if (ctx->enabled && (ctx->type & type)) { > > > > I wonder how realistic this is, given that you do not track the release > > of a level. In short, mo matter what the type is, you treat everything > > as edge. > > > > What is the point of this? > > > > When userspace wants to monitor GPIO line interrupts, the GPIO > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, > IRQF_TRIGGER_RISING or both. I think the background of the question was: Why is there no support for level sensitive irqs? The userspace API has this limitation (also for no good reason I think). Best regards Uwe
wt., 12 lut 2019 o 11:06 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > On Tue, Feb 12, 2019 at 10:19:20AM +0100, Bartosz Golaszewski wrote: > > wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > > > > > On 29/01/2019 08:44, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > Provide a more specialized variant of irq_sim_fire() that allows to > > > > specify the type of the fired interrupt. The type is stored in the > > > > dummy irq context struct via the set_type callback. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > --- > > > > include/linux/irq_sim.h | 9 ++++++++- > > > > kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- > > > > 2 files changed, 30 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > > > > index b96c2f752320..647a6c8ffb31 100644 > > > > --- a/include/linux/irq_sim.h > > > > +++ b/include/linux/irq_sim.h > > > > @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { > > > > > > > > struct irq_sim_irq_ctx { > > > > bool enabled; > > > > + unsigned int type; > > > > }; > > > > > > > > struct irq_sim { > > > > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > > > > int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > > > > unsigned int num_irqs); > > > > void irq_sim_fini(struct irq_sim *sim); > > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > > > > +void irq_sim_fire_type(struct irq_sim *sim, > > > > + unsigned int offset, unsigned int type); > > > > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > > > > > > > > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > > > +{ > > > > + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); > > > > +} > > > > + > > > > #endif /* _LINUX_IRQ_SIM_H */ > > > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > > > > index 2bcdbab1bc5a..e3160b5e59b8 100644 > > > > --- a/kernel/irq/irq_sim.c > > > > +++ b/kernel/irq/irq_sim.c > > > > @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) > > > > irq_ctx->enabled = true; > > > > } > > > > > > > > +static int irq_sim_set_type(struct irq_data *data, unsigned int type) > > > > +{ > > > > + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); > > > > + > > > > + irq_ctx->type = type; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static void irq_sim_handle_irq(struct irq_work *work) > > > > { > > > > struct irq_sim_work_ctx *work_ctx; > > > > @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > > > > sim->chip.name = "irq_sim"; > > > > sim->chip.irq_mask = irq_sim_irqmask; > > > > sim->chip.irq_unmask = irq_sim_irqunmask; > > > > + sim->chip.irq_set_type = irq_sim_set_type; > > > > > > > > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > > > > if (!sim->work_ctx.pending) { > > > > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) > > > > } > > > > > > > > /** > > > > - * irq_sim_fire - Enqueue an interrupt. > > > > + * irq_sim_fire_type - Enqueue an interrupt. > > > > * > > > > * @sim: The interrupt simulator object. > > > > * @offset: Offset of the simulated interrupt which should be fired. > > > > + * @type: Type of the fired interrupt. Must be one of the following: > > > > + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, > > > > + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, > > > > + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT > > > > */ > > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > > > > +void irq_sim_fire_type(struct irq_sim *sim, > > > > + unsigned int offset, unsigned int type) > > > > { > > > > struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > > > > > > > > - if (ctx->enabled) { > > > > + /* Only care about relevant flags. */ > > > > + type &= IRQ_TYPE_SENSE_MASK; > > > > + > > > > + if (ctx->enabled && (ctx->type & type)) { > > > > > > I wonder how realistic this is, given that you do not track the release > > > of a level. In short, mo matter what the type is, you treat everything > > > as edge. > > > > > > What is the point of this? > > > > > > > When userspace wants to monitor GPIO line interrupts, the GPIO > > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, > > IRQF_TRIGGER_RISING or both. > > I think the background of the question was: Why is there no support for > level sensitive irqs? > > The userspace API has this limitation (also for no good reason I think). > Fair point. We also need support for programmable pull-up/down resistors as it's something many users request. It's just a matter of someone getting down to doing it. :) Bart
On 12/02/2019 09:19, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a): >> >> On 29/01/2019 08:44, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> >>> Provide a more specialized variant of irq_sim_fire() that allows to >>> specify the type of the fired interrupt. The type is stored in the >>> dummy irq context struct via the set_type callback. >>> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> --- >>> include/linux/irq_sim.h | 9 ++++++++- >>> kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- >>> 2 files changed, 30 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h >>> index b96c2f752320..647a6c8ffb31 100644 >>> --- a/include/linux/irq_sim.h >>> +++ b/include/linux/irq_sim.h >>> @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { >>> >>> struct irq_sim_irq_ctx { >>> bool enabled; >>> + unsigned int type; >>> }; >>> >>> struct irq_sim { >>> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); >>> int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, >>> unsigned int num_irqs); >>> void irq_sim_fini(struct irq_sim *sim); >>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); >>> +void irq_sim_fire_type(struct irq_sim *sim, >>> + unsigned int offset, unsigned int type); >>> int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); >>> >>> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) >>> +{ >>> + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); >>> +} >>> + >>> #endif /* _LINUX_IRQ_SIM_H */ >>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c >>> index 2bcdbab1bc5a..e3160b5e59b8 100644 >>> --- a/kernel/irq/irq_sim.c >>> +++ b/kernel/irq/irq_sim.c >>> @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) >>> irq_ctx->enabled = true; >>> } >>> >>> +static int irq_sim_set_type(struct irq_data *data, unsigned int type) >>> +{ >>> + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); >>> + >>> + irq_ctx->type = type; >>> + >>> + return 0; >>> +} >>> + >>> static void irq_sim_handle_irq(struct irq_work *work) >>> { >>> struct irq_sim_work_ctx *work_ctx; >>> @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) >>> sim->chip.name = "irq_sim"; >>> sim->chip.irq_mask = irq_sim_irqmask; >>> sim->chip.irq_unmask = irq_sim_irqunmask; >>> + sim->chip.irq_set_type = irq_sim_set_type; >>> >>> sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); >>> if (!sim->work_ctx.pending) { >>> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) >>> } >>> >>> /** >>> - * irq_sim_fire - Enqueue an interrupt. >>> + * irq_sim_fire_type - Enqueue an interrupt. >>> * >>> * @sim: The interrupt simulator object. >>> * @offset: Offset of the simulated interrupt which should be fired. >>> + * @type: Type of the fired interrupt. Must be one of the following: >>> + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, >>> + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, >>> + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT >>> */ >>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) >>> +void irq_sim_fire_type(struct irq_sim *sim, >>> + unsigned int offset, unsigned int type) >>> { >>> struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); >>> >>> - if (ctx->enabled) { >>> + /* Only care about relevant flags. */ >>> + type &= IRQ_TYPE_SENSE_MASK; >>> + >>> + if (ctx->enabled && (ctx->type & type)) { >> >> I wonder how realistic this is, given that you do not track the release >> of a level. In short, mo matter what the type is, you treat everything >> as edge. >> >> What is the point of this? >> > > When userspace wants to monitor GPIO line interrupts, the GPIO > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, > IRQF_TRIGGER_RISING or both. The testing module tries to act like real > hardware and so if we pass only one of the *_TRIGGER_* flags, we want > the simulated interrupt of corresponding type to be fired. Well, that's not how HW works. > > Another solution - if you don't like this one - would be to have more > specialized functions: irq_sim_fire_rising() and > irq_sim_fire_falling(). How about that? I think you're missing the point. So far, your API has been "an interrupt has fired", no matter what the trigger is, and that's fine. That's just modeling the output of an abstract interrupt controller into whatever the irqsim is simulating. Now, what you're exposing is "this is how the line changed". Which is an entirely different business, as you're now exposing the device output line. Yes, you can model it with raising/falling, but you need at least resampling for level interrupts, and actual edge detection (raising followed by raising only generates a single interrupt, while raising-falling-raising generates two). At the moment, I don't see any of that so I seriously doubt the validity of the approach. Thanks, M.
wt., 12 lut 2019 o 11:27 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > > On 12/02/2019 09:19, Bartosz Golaszewski wrote: > > wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a): > >> > >> On 29/01/2019 08:44, Bartosz Golaszewski wrote: > >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >>> > >>> Provide a more specialized variant of irq_sim_fire() that allows to > >>> specify the type of the fired interrupt. The type is stored in the > >>> dummy irq context struct via the set_type callback. > >>> > >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >>> --- > >>> include/linux/irq_sim.h | 9 ++++++++- > >>> kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- > >>> 2 files changed, 30 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > >>> index b96c2f752320..647a6c8ffb31 100644 > >>> --- a/include/linux/irq_sim.h > >>> +++ b/include/linux/irq_sim.h > >>> @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { > >>> > >>> struct irq_sim_irq_ctx { > >>> bool enabled; > >>> + unsigned int type; > >>> }; > >>> > >>> struct irq_sim { > >>> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > >>> int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > >>> unsigned int num_irqs); > >>> void irq_sim_fini(struct irq_sim *sim); > >>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > >>> +void irq_sim_fire_type(struct irq_sim *sim, > >>> + unsigned int offset, unsigned int type); > >>> int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > >>> > >>> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > >>> +{ > >>> + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); > >>> +} > >>> + > >>> #endif /* _LINUX_IRQ_SIM_H */ > >>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > >>> index 2bcdbab1bc5a..e3160b5e59b8 100644 > >>> --- a/kernel/irq/irq_sim.c > >>> +++ b/kernel/irq/irq_sim.c > >>> @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) > >>> irq_ctx->enabled = true; > >>> } > >>> > >>> +static int irq_sim_set_type(struct irq_data *data, unsigned int type) > >>> +{ > >>> + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); > >>> + > >>> + irq_ctx->type = type; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static void irq_sim_handle_irq(struct irq_work *work) > >>> { > >>> struct irq_sim_work_ctx *work_ctx; > >>> @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > >>> sim->chip.name = "irq_sim"; > >>> sim->chip.irq_mask = irq_sim_irqmask; > >>> sim->chip.irq_unmask = irq_sim_irqunmask; > >>> + sim->chip.irq_set_type = irq_sim_set_type; > >>> > >>> sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > >>> if (!sim->work_ctx.pending) { > >>> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) > >>> } > >>> > >>> /** > >>> - * irq_sim_fire - Enqueue an interrupt. > >>> + * irq_sim_fire_type - Enqueue an interrupt. > >>> * > >>> * @sim: The interrupt simulator object. > >>> * @offset: Offset of the simulated interrupt which should be fired. > >>> + * @type: Type of the fired interrupt. Must be one of the following: > >>> + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, > >>> + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, > >>> + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT > >>> */ > >>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > >>> +void irq_sim_fire_type(struct irq_sim *sim, > >>> + unsigned int offset, unsigned int type) > >>> { > >>> struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); > >>> > >>> - if (ctx->enabled) { > >>> + /* Only care about relevant flags. */ > >>> + type &= IRQ_TYPE_SENSE_MASK; > >>> + > >>> + if (ctx->enabled && (ctx->type & type)) { > >> > >> I wonder how realistic this is, given that you do not track the release > >> of a level. In short, mo matter what the type is, you treat everything > >> as edge. > >> > >> What is the point of this? > >> > > > > When userspace wants to monitor GPIO line interrupts, the GPIO > > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, > > IRQF_TRIGGER_RISING or both. The testing module tries to act like real > > hardware and so if we pass only one of the *_TRIGGER_* flags, we want > > the simulated interrupt of corresponding type to be fired. > > Well, that's not how HW works. > > > > > Another solution - if you don't like this one - would be to have more > > specialized functions: irq_sim_fire_rising() and > > irq_sim_fire_falling(). How about that? > > I think you're missing the point. So far, your API has been "an > interrupt has fired", no matter what the trigger is, and that's fine. > That's just modeling the output of an abstract interrupt controller into > whatever the irqsim is simulating. > > Now, what you're exposing is "this is how the line changed". Which is an > entirely different business, as you're now exposing the device output > line. Yes, you can model it with raising/falling, but you need at least > resampling for level interrupts, and actual edge detection (raising > followed by raising only generates a single interrupt, while > raising-falling-raising generates two). > This logic is later taken care of in the gpio-mockup driver in this series. It checks the line state changes and decides if the interrupt should be fired. Bart > At the moment, I don't see any of that so I seriously doubt the validity > of the approach. >
On 12/02/2019 10:37, Bartosz Golaszewski wrote: > wt., 12 lut 2019 o 11:27 Marc Zyngier <marc.zyngier@arm.com> napisał(a): >> >> On 12/02/2019 09:19, Bartosz Golaszewski wrote: >>> wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a): >>>> >>>> On 29/01/2019 08:44, Bartosz Golaszewski wrote: >>>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>>>> >>>>> Provide a more specialized variant of irq_sim_fire() that allows to >>>>> specify the type of the fired interrupt. The type is stored in the >>>>> dummy irq context struct via the set_type callback. >>>>> >>>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>>>> --- >>>>> include/linux/irq_sim.h | 9 ++++++++- >>>>> kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- >>>>> 2 files changed, 30 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h >>>>> index b96c2f752320..647a6c8ffb31 100644 >>>>> --- a/include/linux/irq_sim.h >>>>> +++ b/include/linux/irq_sim.h >>>>> @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { >>>>> >>>>> struct irq_sim_irq_ctx { >>>>> bool enabled; >>>>> + unsigned int type; >>>>> }; >>>>> >>>>> struct irq_sim { >>>>> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); >>>>> int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, >>>>> unsigned int num_irqs); >>>>> void irq_sim_fini(struct irq_sim *sim); >>>>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); >>>>> +void irq_sim_fire_type(struct irq_sim *sim, >>>>> + unsigned int offset, unsigned int type); >>>>> int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); >>>>> >>>>> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) >>>>> +{ >>>>> + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); >>>>> +} >>>>> + >>>>> #endif /* _LINUX_IRQ_SIM_H */ >>>>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c >>>>> index 2bcdbab1bc5a..e3160b5e59b8 100644 >>>>> --- a/kernel/irq/irq_sim.c >>>>> +++ b/kernel/irq/irq_sim.c >>>>> @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) >>>>> irq_ctx->enabled = true; >>>>> } >>>>> >>>>> +static int irq_sim_set_type(struct irq_data *data, unsigned int type) >>>>> +{ >>>>> + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); >>>>> + >>>>> + irq_ctx->type = type; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static void irq_sim_handle_irq(struct irq_work *work) >>>>> { >>>>> struct irq_sim_work_ctx *work_ctx; >>>>> @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) >>>>> sim->chip.name = "irq_sim"; >>>>> sim->chip.irq_mask = irq_sim_irqmask; >>>>> sim->chip.irq_unmask = irq_sim_irqunmask; >>>>> + sim->chip.irq_set_type = irq_sim_set_type; >>>>> >>>>> sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); >>>>> if (!sim->work_ctx.pending) { >>>>> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) >>>>> } >>>>> >>>>> /** >>>>> - * irq_sim_fire - Enqueue an interrupt. >>>>> + * irq_sim_fire_type - Enqueue an interrupt. >>>>> * >>>>> * @sim: The interrupt simulator object. >>>>> * @offset: Offset of the simulated interrupt which should be fired. >>>>> + * @type: Type of the fired interrupt. Must be one of the following: >>>>> + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, >>>>> + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, >>>>> + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT >>>>> */ >>>>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) >>>>> +void irq_sim_fire_type(struct irq_sim *sim, >>>>> + unsigned int offset, unsigned int type) >>>>> { >>>>> struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); >>>>> >>>>> - if (ctx->enabled) { >>>>> + /* Only care about relevant flags. */ >>>>> + type &= IRQ_TYPE_SENSE_MASK; >>>>> + >>>>> + if (ctx->enabled && (ctx->type & type)) { >>>> >>>> I wonder how realistic this is, given that you do not track the release >>>> of a level. In short, mo matter what the type is, you treat everything >>>> as edge. >>>> >>>> What is the point of this? >>>> >>> >>> When userspace wants to monitor GPIO line interrupts, the GPIO >>> framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, >>> IRQF_TRIGGER_RISING or both. The testing module tries to act like real >>> hardware and so if we pass only one of the *_TRIGGER_* flags, we want >>> the simulated interrupt of corresponding type to be fired. >> >> Well, that's not how HW works. >> >>> >>> Another solution - if you don't like this one - would be to have more >>> specialized functions: irq_sim_fire_rising() and >>> irq_sim_fire_falling(). How about that? >> >> I think you're missing the point. So far, your API has been "an >> interrupt has fired", no matter what the trigger is, and that's fine. >> That's just modeling the output of an abstract interrupt controller into >> whatever the irqsim is simulating. >> >> Now, what you're exposing is "this is how the line changed". Which is an >> entirely different business, as you're now exposing the device output >> line. Yes, you can model it with raising/falling, but you need at least >> resampling for level interrupts, and actual edge detection (raising >> followed by raising only generates a single interrupt, while >> raising-falling-raising generates two). >> > > This logic is later taken care of in the gpio-mockup driver in this > series. It checks the line state changes and decides if the interrupt > should be fired. But that's only for edge, right? I don't really see any level support. Can you please trim this series to what you actually (1) need, (2) support. So far, this series feels like a bunch of half-baked ideas. Interesting ideas, but still half baked. And trying to rush me into ACKing its of it is not improving the situation. Thanks, M.
On Tue, Feb 12, 2019 at 10:27:54AM +0000, Marc Zyngier wrote: > On 12/02/2019 09:19, Bartosz Golaszewski wrote: > > When userspace wants to monitor GPIO line interrupts, the GPIO > > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, > > IRQF_TRIGGER_RISING or both. The testing module tries to act like real > > hardware and so if we pass only one of the *_TRIGGER_* flags, we want > > the simulated interrupt of corresponding type to be fired. > > Well, that's not how HW works. I cannot follow. I agree with Bartosz here. If you configure your SoC's irq-controller to only fire on a raising edge, you don't get an event when the line falls. > > Another solution - if you don't like this one - would be to have more > > specialized functions: irq_sim_fire_rising() and > > irq_sim_fire_falling(). How about that? > > I think you're missing the point. So far, your API has been "an > interrupt has fired", no matter what the trigger is, and that's fine. > That's just modeling the output of an abstract interrupt controller into > whatever the irqsim is simulating. > > Now, what you're exposing is "this is how the line changed". Which is an > entirely different business, as you're now exposing the device output > line. Yes, you can model it with raising/falling, but you need at least > resampling for level interrupts, and actual edge detection (raising > followed by raising only generates a single interrupt, while > raising-falling-raising generates two). This matches my concern and that's why I suggested somewhere else in this thread to put the configuration of the sensitiveness and the actual tracking of the line in the same component (either irqsim or gpio-mockup). Given that there are only two irqsim users and the other one (something in iio) doesn't need that sensitiveness stuff (and I cannot imagine another user of irqsim with the sensitiveness support) I think it is best to move this to the mockup driver. That's how "normal" hardware drivers have to do it, too. Best regards Uwe
wt., 12 lut 2019 o 12:05 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > On Tue, Feb 12, 2019 at 10:27:54AM +0000, Marc Zyngier wrote: > > On 12/02/2019 09:19, Bartosz Golaszewski wrote: > > > When userspace wants to monitor GPIO line interrupts, the GPIO > > > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, > > > IRQF_TRIGGER_RISING or both. The testing module tries to act like real > > > hardware and so if we pass only one of the *_TRIGGER_* flags, we want > > > the simulated interrupt of corresponding type to be fired. > > > > Well, that's not how HW works. > > I cannot follow. I agree with Bartosz here. If you configure your SoC's > irq-controller to only fire on a raising edge, you don't get an event > when the line falls. > > > > Another solution - if you don't like this one - would be to have more > > > specialized functions: irq_sim_fire_rising() and > > > irq_sim_fire_falling(). How about that? > > > > I think you're missing the point. So far, your API has been "an > > interrupt has fired", no matter what the trigger is, and that's fine. > > That's just modeling the output of an abstract interrupt controller into > > whatever the irqsim is simulating. > > > > Now, what you're exposing is "this is how the line changed". Which is an > > entirely different business, as you're now exposing the device output > > line. Yes, you can model it with raising/falling, but you need at least > > resampling for level interrupts, and actual edge detection (raising > > followed by raising only generates a single interrupt, while > > raising-falling-raising generates two). > > This matches my concern and that's why I suggested somewhere else in > this thread to put the configuration of the sensitiveness and the actual > tracking of the line in the same component (either irqsim or > gpio-mockup). Given that there are only two irqsim users and the other > one (something in iio) doesn't need that sensitiveness stuff (and I > cannot imagine another user of irqsim with the sensitiveness support) I > think it is best to move this to the mockup driver. That's how "normal" > hardware drivers have to do it, too. > This is what v1 of this series did - it provided a function to retrieve the type and the logic lived in gpio-mockup. Maybe we need to get back to that solution. Bart
On 12/02/2019 11:05, Uwe Kleine-König wrote: > On Tue, Feb 12, 2019 at 10:27:54AM +0000, Marc Zyngier wrote: >> On 12/02/2019 09:19, Bartosz Golaszewski wrote: >>> When userspace wants to monitor GPIO line interrupts, the GPIO >>> framework requests a threaded interrupt with IRQF_TRIGGER_FALLING, >>> IRQF_TRIGGER_RISING or both. The testing module tries to act like real >>> hardware and so if we pass only one of the *_TRIGGER_* flags, we want >>> the simulated interrupt of corresponding type to be fired. >> >> Well, that's not how HW works. > > I cannot follow. I agree with Bartosz here. If you configure your SoC's > irq-controller to only fire on a raising edge, you don't get an event > when the line falls. I was a bit quick here. It is more the fact that level gets treated as "just another type of edge" that really irritated me (yes, I've wasted way too much time implementing interrupt controllers). Thanks, M.
diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h index b96c2f752320..647a6c8ffb31 100644 --- a/include/linux/irq_sim.h +++ b/include/linux/irq_sim.h @@ -23,6 +23,7 @@ struct irq_sim_work_ctx { struct irq_sim_irq_ctx { bool enabled; + unsigned int type; }; struct irq_sim { @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, unsigned int num_irqs); void irq_sim_fini(struct irq_sim *sim); -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); +void irq_sim_fire_type(struct irq_sim *sim, + unsigned int offset, unsigned int type); int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset) +{ + irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT); +} + #endif /* _LINUX_IRQ_SIM_H */ diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c index 2bcdbab1bc5a..e3160b5e59b8 100644 --- a/kernel/irq/irq_sim.c +++ b/kernel/irq/irq_sim.c @@ -25,6 +25,15 @@ static void irq_sim_irqunmask(struct irq_data *data) irq_ctx->enabled = true; } +static int irq_sim_set_type(struct irq_data *data, unsigned int type) +{ + struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data); + + irq_ctx->type = type; + + return 0; +} + static void irq_sim_handle_irq(struct irq_work *work) { struct irq_sim_work_ctx *work_ctx; @@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) sim->chip.name = "irq_sim"; sim->chip.irq_mask = irq_sim_irqmask; sim->chip.irq_unmask = irq_sim_irqunmask; + sim->chip.irq_set_type = irq_sim_set_type; sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); if (!sim->work_ctx.pending) { @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) } /** - * irq_sim_fire - Enqueue an interrupt. + * irq_sim_fire_type - Enqueue an interrupt. * * @sim: The interrupt simulator object. * @offset: Offset of the simulated interrupt which should be fired. + * @type: Type of the fired interrupt. Must be one of the following: + * IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, + * IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH, + * IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT */ -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) +void irq_sim_fire_type(struct irq_sim *sim, + unsigned int offset, unsigned int type) { struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); - if (ctx->enabled) { + /* Only care about relevant flags. */ + type &= IRQ_TYPE_SENSE_MASK; + + if (ctx->enabled && (ctx->type & type)) { set_bit(offset, sim->work_ctx.pending); irq_work_queue(&sim->work_ctx.work); } } -EXPORT_SYMBOL_GPL(irq_sim_fire); +EXPORT_SYMBOL_GPL(irq_sim_fire_type); /** * irq_sim_irqnum - Get the allocated number of a dummy interrupt.