[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.11.1501131117120.17382@nanos>
Date: Tue, 13 Jan 2015 11:38:14 +0100 (CET)
From: Thomas Gleixner <tglx@...utronix.de>
To: Boris Brezillon <boris.brezillon@...e-electrons.com>
cc: Nicolas Ferre <nicolas.ferre@...el.com>,
Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [RFC PATCH] irqchip: add dumb demultiplexer implementation
On Thu, 8 Jan 2015, Boris Brezillon wrote:
> 1) Is it close to what you had in mind ?
Yes.
> 2) I'm not sure which part of the code should go where, so don't hesitate
> to point out misplaced sections.
Looks sane. Nits below.
> 3) Do I need to disable the source irq (the one feeding the irqchip) when
> entering suspend (and enable it on resume) ?
That probably needs to be part of the dumb mask/unmask functions.,
i.e. if no demux interrupt is enabled, the source irq should be
masked.
> 4) I'm not sure of what flags should be set/cleared when mapping an
> interrupt. Should I let the caller decide of this config (something
> similar to what is done in generic-chip) ?
I don't think you need to set/clear anything. Lets look at that dumb
demux chip as a real electronic circuit
|----------------|
| |
| --|Unmasked|--|---- Demux0
| | |
SRC irq -----|-- -|Unmasked|--|---- Demux1
| | |
| --|Unmasked|--|---- Demux2
| |
|----------------|
Whether a demultiplexed interrupt is mapped or not is not really
important. The only relevant information is whether its masked or
not. So the default state is masked until a demultiplexed interrupt
gets requested.
> +/**
> + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips
> + * @IRQ_DD_INIT_NESTED_LOCK: Set the lock class of the irqs to nested for
> + * irq chips which need to call irq_set_wake() on
> + * the parent irq. Usually GPIO implementations
> + */
> +enum irq_dumb_demux_flags {
> + IRQ_DD_INIT_NESTED_LOCK = 1 << 0,
> +};
> +
> +/**
> + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
> + * @domain: irq domain pointer
> + * @max_irq: Last valid irq
> + * @available: Bitfield of valid irqs
> + * @unmasked: Bitfield containing irqs status
> + * @flags: irq_dumb_demux_flags flags
> + *
> + * Note, that irq_chip_generic can have multiple irq_chip_type
> + * implementations which can be associated to a particular irq line of
> + * an irq_chip_generic instance. That allows to share and protect
> + * state in an irq_chip_generic instance when we need to implement
> + * different flow mechanisms (level/edge) for it.
> + */
> +struct irq_chip_dumb_demux {
> + struct irq_domain *domain;
> + int max_irq;
> + unsigned long *available;
> + unsigned long *unmasked;
Why pointers? A single ulong is certainly enough.
> +/**
> + * handle_dumb_demux_irq - Dumb demuxer irq handle function.
> + * @irq: the interrupt number
> + * @desc: the interrupt description structure for this irq
> + *
> + * Dumb demux interrupts are sent from a demultiplexing interrupt handler
> + * which is not able to decide which child interrupt interrupt handler
> + * should be called.
> + *
> + * Note: The caller is expected to handle the ack, clear, mask and
> + * unmask issues if necessary.
> + */
> +irqreturn_t
> +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc)
> +{
> + irqreturn_t retval = IRQ_NONE;
> +
> + raw_spin_lock(&desc->lock);
> +
> + if (!irq_may_run(desc))
> + goto out_unlock;
> +
> + desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> + kstat_incr_irqs_this_cpu(irq, desc);
> +
> + if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> + desc->istate |= IRQS_PENDING;
> + goto out_unlock;
> + }
> +
> + retval = handle_irq_event_no_spurious_check(desc);
> +
> +out_unlock:
> + raw_spin_unlock(&desc->lock);
> +
> + return retval;
> +}
> +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq);
This should go into the new file as well, so it gets compiled out when
not enabled.
> +static void irq_dumb_demux_mask(struct irq_data *d)
> +{
> + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> + clear_bit(d->hwirq, demux->unmasked);
> +}
> +
> +static void irq_dumb_demux_unmask(struct irq_data *d)
> +{
> + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> + set_bit(d->hwirq, demux->unmasked);
> +}
So this needs the handling of enabling/disabling the source irq.
> +static struct irq_chip irq_dumb_demux_chip = {
> + .name = "dumb-demux-irq",
> + .irq_mask = irq_dumb_demux_mask,
> + .irq_unmask = irq_dumb_demux_unmask,
+ .name = "dumb-demux-irq",
+ .irq_mask = irq_dumb_demux_mask,
+ .irq_unmask = irq_dumb_demux_unmask,
Makes it simpler to read.
> +struct irq_domain_ops irq_dumb_demux_domain_ops = {
> + .map = irq_map_dumb_demux_chip,
> + .xlate = irq_domain_xlate_onecell,
> +};
> +EXPORT_SYMBOL(irq_dumb_demux_domain_ops);
SYMBOL_GPL please
Rest looks good.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists