| Message ID | 20190123141538.29408-4-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mockup: improve the user-space testing interface | expand |
Hello Bartosz, On Wed, Jan 23, 2019 at 03:15:32PM +0100, Bartosz Golaszewski wrote: > Provide a helper that allows users to retrieve the configured flow type > of dummy interrupts. That allows certain users to decide whether an irq > needs to be fired depending on its edge/level/... configuration. You don't talk about the .set_type callback here; is this intended? I wonder how you think this should be used. Assume the mockup-driver is directed to pull up a certain line, does it do something like that: def mockup_setval(self, val): irqtype = irq_sim_get_type(...) if irqtype == LEVEL_HIGH: if val: fire_irq() else if irqtype == EDGE_RISING: if val and not prev_val: fire_irq() else if irqtype == LEVEL_LOW: if not val: fire_irq() else if irqtype == EDGE_FALLING: if not val and prev_val: fire_irq() I wonder if that logic should be done in the same place as where the irq type is stored. Otherwise that .type member is only a data store provided by the irq simulator. So I suggest to either move the variable that mirrors the current level of the line into the irq simulator, or keep the irqtype variable in the mockup driver. Both approaches would make it unnecessary to provide an accessor function for the type member. Best regards Uwe
śr., 23 sty 2019 o 20:18 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > Hello Bartosz, > > On Wed, Jan 23, 2019 at 03:15:32PM +0100, Bartosz Golaszewski wrote: > > Provide a helper that allows users to retrieve the configured flow type > > of dummy interrupts. That allows certain users to decide whether an irq > > needs to be fired depending on its edge/level/... configuration. > > You don't talk about the .set_type callback here; is this intended? > > I wonder how you think this should be used. Assume the mockup-driver is > directed to pull up a certain line, does it do something like that: > > def mockup_setval(self, val): > irqtype = irq_sim_get_type(...) > if irqtype == LEVEL_HIGH: > if val: > fire_irq() > > else if irqtype == EDGE_RISING: > if val and not prev_val: > fire_irq() > > else if irqtype == LEVEL_LOW: > if not val: > fire_irq() > > else if irqtype == EDGE_FALLING: > if not val and prev_val: > fire_irq() > > I wonder if that logic should be done in the same place as where the irq > type is stored. Otherwise that .type member is only a data store > provided by the irq simulator. So I suggest to either move the variable > that mirrors the current level of the line into the irq simulator, or > keep the irqtype variable in the mockup driver. Both approaches would > make it unnecessary to provide an accessor function for the type member. > Yeah, might be better to go back to my previous idea of adding irq_sim_fire_edge(), but maybe it should be irq_sim_fire_type() instead, so that irq_sim_fire() fires unconditionally and irq_sim_fire_type() would fire only if the passed flag is the same as the one previously configured by the set_type() callback. Thanks, Bartosz
On Thu, Jan 24, 2019 at 08:46:56AM +0100, Bartosz Golaszewski wrote: > śr., 23 sty 2019 o 20:18 Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> napisał(a): > > > > Hello Bartosz, > > > > On Wed, Jan 23, 2019 at 03:15:32PM +0100, Bartosz Golaszewski wrote: > > > Provide a helper that allows users to retrieve the configured flow type > > > of dummy interrupts. That allows certain users to decide whether an irq > > > needs to be fired depending on its edge/level/... configuration. > > > > You don't talk about the .set_type callback here; is this intended? > > > > I wonder how you think this should be used. Assume the mockup-driver is > > directed to pull up a certain line, does it do something like that: > > > > def mockup_setval(self, val): > > irqtype = irq_sim_get_type(...) > > if irqtype == LEVEL_HIGH: > > if val: > > fire_irq() > > > > else if irqtype == EDGE_RISING: > > if val and not prev_val: > > fire_irq() > > > > else if irqtype == LEVEL_LOW: > > if not val: > > fire_irq() > > > > else if irqtype == EDGE_FALLING: > > if not val and prev_val: > > fire_irq() > > > > I wonder if that logic should be done in the same place as where the irq > > type is stored. Otherwise that .type member is only a data store > > provided by the irq simulator. So I suggest to either move the variable > > that mirrors the current level of the line into the irq simulator, or > > keep the irqtype variable in the mockup driver. Both approaches would > > make it unnecessary to provide an accessor function for the type member. > > > > Yeah, might be better to go back to my previous idea of adding > irq_sim_fire_edge(), but maybe it should be irq_sim_fire_type() > instead, so that irq_sim_fire() fires unconditionally and > irq_sim_fire_type() would fire only if the passed flag is the same as > the one previously configured by the set_type() callback. How (if at all) do you intend to support level sensitive irqs with this interface? It probably works (but I didn't thought it through completely), but firing a LEVEL_HIGH sensitive irq on irq_sim_fire_type(EDGE_RISING) might look at least surprising and needs proper comments and thoughts. Best regards Uwe
diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h index b96c2f752320..782dfc599632 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 { @@ -39,5 +40,6 @@ int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, void irq_sim_fini(struct irq_sim *sim); void irq_sim_fire(struct irq_sim *sim, unsigned int offset); int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); +int irq_sim_get_type(struct irq_sim *sim, unsigned int offset); #endif /* _LINUX_IRQ_SIM_H */ diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c index 2bcdbab1bc5a..9168e7100559 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) { @@ -220,3 +230,18 @@ int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset) return irq_find_mapping(sim->domain, offset); } EXPORT_SYMBOL_GPL(irq_sim_irqnum); + +/** + * irq_sim_get_type - Get the configured flow type of a dummy interrupt. + * + * @sim: The interrupt simulator object. + * @offset: Offset of the simulated interrupt for which to retrieve + * the type. + */ +int irq_sim_get_type(struct irq_sim *sim, unsigned int offset) +{ + struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset); + + return ctx->type; +} +EXPORT_SYMBOL_GPL(irq_sim_get_type);