Re: [PATCH 3/3] irq/irq_sim: implement irq_sim_irq2offset()

[Marc Zyngier]

On 18/10/17 10:51, Bartosz Golaszewski wrote:
> 2017-10-18 10:58 GMT+02:00 Marc Zyngier <marc.zyngier@xxxxxxx>:
>> On 05/10/17 13:44, Bartosz Golaszewski wrote:
>>> Add a routine allowing to retrieve the offset corresponding with an
>>> allocated interrupt number from an irq_sim object.
>>>
>>> Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
>>> ---
>>>  include/linux/irq_sim.h |  1 +
>>>  kernel/irq/irq_sim.c    | 15 +++++++++++++++
>>>  2 files changed, 16 insertions(+)
>>>
>>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
>>> index 246f593face8..e9768ca81e3e 100644
>>> --- a/include/linux/irq_sim.h
>>> +++ b/include/linux/irq_sim.h
>>> @@ -41,5 +41,6 @@ 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_baseirq(struct irq_sim *sim);
>>> +unsigned int irq_sim_irq2offset(struct irq_sim *sim, int irq);
>>>
>>>  #endif /* _LINUX_IRQ_SIM_H */
>>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
>>> index 1be10d0e295f..9a9a1bc8853c 100644
>>> --- a/kernel/irq/irq_sim.c
>>> +++ b/kernel/irq/irq_sim.c
>>> @@ -174,3 +174,18 @@ int irq_sim_baseirq(struct irq_sim *sim)
>>>       return irq_sim_irqnum(sim, 0);
>>>  }
>>>  EXPORT_SYMBOL_GPL(irq_sim_baseirq);
>>> +
>>> +/**
>>> + * irq_sim_irq2offset - Get the offset of an allocated interrupt.
>>> + *
>>> + * @sim:        The interrupt simulator object.
>>> + * irq:         Allocated interrupt number.
>>> + *
>>> + * Perform a reverse lookup for the interrupt number and return its offset in
>>> + * the array allocated by sim_irq.
>>> + */
>>> +unsigned int irq_sim_irq2offset(struct irq_sim *sim, int irq)
>>> +{
>>> +     return irq - sim->irq_base;
>>> +}
>>> +EXPORT_SYMBOL_GPL(irq_sim_irq2offset);
>>>
>>
>> How is that useful in a random driver? The irq base and the offset are
>> things that should be internal to an irqchip. Maybe you could explain
>> the context you want to use this in.
>>
> 
> The interrupts are allocated within the interrupt simulator code, but
> it's up to users to actually request these irqs. That's why we already
> have sim_irq_irqnum().
> 
> The IIO dummy driver (one of the users of irq_sim) uses these irq
> numbers retrieved from the simulator to also access private data
> stored in the iio_dummy_evgen module. Currently we store the base
> interrupt number in a separate variable in the dummy_evgen context
> struct and calculate the offset in the interrupt array manually. We
> could simplify that code by moving this logic into the interrupt
> simulator.

It looks to me that this is all because the irq_sim creation is a bit awkward.
You end-up with all kind of exotic interfaces because you don't know the base
of the irq range at creation time.

How about something like this:

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 24caabf1a0f7..484c3544c0d1 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -49,7 +49,8 @@ static void irq_sim_handle_irq(struct irq_work *work)
  * @sim:        The interrupt simulator object to initialize.
  * @num_irqs:   Number of interrupts to allocate
  *
- * Returns 0 on success and a negative error number on failure.
+ * Returns the interrupt base on success and a negative error number
+ * on failure.
  */
 int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 {
@@ -78,7 +79,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 	init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
 	sim->irq_count = num_irqs;
 
-	return 0;
+	return sim->irq_base;
 }
 EXPORT_SYMBOL_GPL(irq_sim_init);
 
You can then deal with the offset directly in your driver, as you're
guaranteed a 1:1 mapping.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...