Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

wt., 20 lis 2018 o 18:17 Uwe Kleine-KÃnig
<u.kleine-koenig@xxxxxxxxxxxxxx> napisaÅ(a):
>
> On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
> > The irq_sim irqchip doesn't allow to configure the sensitivity so every
> > call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> > for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> > ("gpiolib: Don't support irq sharing for userspace") which made it
> > impossible for gpio-mockup to ignore certain events (e.g. only receive
> > notifications about rising edge events).
> >
> > Introduce a specialized variant of irq_sim_fire() which takes another
> > argument called edge. allowing to specify the trigger type for the
> > dummy interrupt.
>
> I wonder if it's worth the effort to fix irq_sim. If you take a look in
> my gpio-simulator patch, it is trivial to get it right without external
> help with an amount of code that is usual for a driver that handles
> irqs.
>

You're basically recommending handcrafting another local piece of code
for simulating interrupts - something that multiple users may be
interested in. You did that in your proposed gpio-simulator and I
still can't understand why you couldn't reuse the existing solution.
Even if it's broken for your use-case, it's surely easier to fix it
than to rewrite and duplicate it? There are very few cases where code
consolidation is not a good thing and I don't think this is one of
them.

The interrupt simulator exists and is used by two testing drivers
currently. One of them needs some more flexibility and this is what
this patch is trying to add. I believe it is worth the effort.

Best regards,
Bartosz

> > @@ -161,12 +171,28 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);
> >   */
> >  void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> >  {
> > -     if (sim->irqs[offset].enabled) {
> > +     /* Don't care about the edge. */
> > +     irq_sim_fire_edge(sim, offset, IRQ_TYPE_EDGE_BOTH);
>
> Conceptually irq_sim_fire_edge should be defined above irq_sim_fire.
>
> > +EXPORT_SYMBOL_GPL(irq_sim_fire);
> > +
> > +/**
> > + * irq_sim_fire_edge - Enqueue an interrupt, specify the edge.
> > + *
> > + * @sim:        The interrupt simulator object.
> > + * @offset:     Offset of the simulated interrupt which should be fired.
> > + * edge:        Edge of the interrupt (IRQ_TYPE_EDGE_RISING/FALLING).
> > + */
> > +void irq_sim_fire_edge(struct irq_sim *sim, unsigned int offset, int edge)
> > +{
> > +     struct irq_sim_irq_ctx *irq = &sim->irqs[offset];
> > +
> > +     if (irq->enabled && (irq->edge & edge)) {
> >               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_edge);
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-KÃnig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |