diff mbox series

[3/9] irq/irq_sim: provide irq_sim_get_type()

Message ID 20190123141538.29408-4-brgl@bgdev.pl
State New
Headers show
Series gpio: mockup: improve the user-space testing interface | expand

Commit Message

Bartosz Golaszewski Jan. 23, 2019, 2:15 p.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/irq_sim.h |  2 ++
 kernel/irq/irq_sim.c    | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Uwe Kleine-König Jan. 23, 2019, 7:18 p.m. UTC | #1
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
Bartosz Golaszewski Jan. 24, 2019, 7:46 a.m. UTC | #2
ś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
Uwe Kleine-König Jan. 24, 2019, 8:09 a.m. UTC | #3
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 mbox series

Patch

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);