Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3454 bytes --]

On Mon, Jul 29, 2024 at 12:41:25PM +0200, Thomas Gleixner wrote:
> On Tue, Jul 23 2024 at 12:27, Conor Dooley wrote:
> > +
> > +struct mpfs_irq_mux_bank_config {
> > +	u32 mask;
> > +	u8 shift;
> > +};
> 
> Please see:
> 
>   https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> vs. coding style.
> 
> > +/*
> > + * Returns an unsigned long, where a set bit indicates the corresponding
> > + * interrupt is in non-direct/muxed mode for that bank/GPIO controller.
> > + */
> > +static inline unsigned long mpfs_irq_mux_get_muxed_irqs(struct mpfs_irq_mux *priv,
> > +							unsigned int bank)
> > +{
> > +	unsigned long mux_config = priv->mux_config, muxed_irqs = -1;
> > +	struct mpfs_irq_mux_bank_config bank_config = mpfs_irq_mux_bank_configs[bank];
> > +
> > +	/*
> > +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
> > +	 * controller 2 is direct and that controllers 0 or 1 is muxed.
> 
> This is not a coherent sentence.

It should read "controller 0 or 1;s interrupt is muxed". Does that make
more sense to you?

> > +	 * Invert the bits in the configuration register, so that set bits
> > +	 * equate to non-direct mode, for GPIO controller 2.
> > +	 */
> > +	if (bank == 2u)
> > +		mux_config = ~mux_config;
> > +
> 
> > +static int mpfs_irq_mux_nondirect_alloc(struct irq_domain *d, unsigned int virq,
> > +					struct irq_fwspec *fwspec, struct mpfs_irq_mux *priv)
> > +{
> > +	unsigned int bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
> > +
> > +	if (bank > 2)
> > +		return -EINVAL;
> > +
> > +	priv->nondirect_irqchips[bank].domain = d;
> > +
> > +	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0],
> > +				      &mpfs_irq_mux_nondirect_irq_chip, priv);
> > +	irq_set_chained_handler_and_data(virq, handle_untracked_irq,
> 
> Why does this use handle_untracked_irq()?

I'll have to go and dig back in my notes as to why it is untracked. It
was probably something like irqd_set() in handle_irq_event() blowing up
on the irq_data being invalid (which I figure could relate back to my
questions in the cover letter about issues with irqd_to_hwirq()) - but
I'll double check what exactly prompted it when I get back from my
holidays, but...

> This sets up a chained handler
> but handle_untracked_irq() is a regular interrupt handler.

...what I was likely using before was handle_simple_irq() which isn't
chained either. You're expecting to see mpfs_irq_mux_nondirect_handler()
here I suppose?

>+static void mpfs_irq_mux_nondirect_handler(struct irq_desc *desc)
>+{
>+	struct mpfs_irq_mux_irqchip *irqchip_data = irq_desc_get_handler_data(desc);
>+	struct mpfs_irq_mux *priv = container_of(irqchip_data, struct mpfs_irq_mux,
>+						 nondirect_irqchips[irqchip_data->bank]);
>+	unsigned long muxed_irqs;
>+	int pos;
>+
>+	chained_irq_enter(irq_desc_get_chip(desc), desc);
>+
>+	muxed_irqs = mpfs_irq_mux_get_muxed_irqs(priv, irqchip_data->bank);
>+
>+	for_each_set_bit(pos, &muxed_irqs, MPFS_MAX_IRQS_PER_GPIO)
>+		generic_handle_domain_irq(irqchip_data->domain, irqchip_data->offset + pos);
>+
>+	chained_irq_exit(irq_desc_get_chip(desc), desc);
>+}

Given you've only commented on one significant issue and two minor items,
is it safe to conclude that the overall approach doesn't have you
screaming and running for the hills?

Cheers,
Conor.

> > +					 &priv->nondirect_irqchips[bank]);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]