Re: [PATCH v6 1/2] ARM: irqchip: mxs: prepare driver for HW with different offsets

[Thomas Gleixner <tglx@linutronix.de>]

Oleksij,

On Sat, 10 Oct 2015, Oleksij Rempel wrote:

The proper subject line starts with:

irqchip/mxs: 

> Some HW has similar functionality but different register offsets.
> Make sure we can change offsets dynamically.

The patch does way more than that. I told you in V2 already:

> > You forgot to mention the other preparatory changes.

There is still nothing in the changelog.

> +static void __init icoll_add_domain(struct device_node *np,
> +			  int num)
> +{
> +	icoll_domain = irq_domain_add_linear(np, num,
> +					     &icoll_irq_domain_ops, NULL);
> +
> +	if (!icoll_domain)
> +		panic("%s: unable add irq domain", np->full_name);

This splitout should be a separate patch with an explanation why you
add

> +	irq_set_default_host(icoll_domain);

and

> +	set_handle_irq(icoll_handle_irq);

The latter is already done via the DT_MACHINE_START magic. So you
should it remove there, because otherwise that call is just
pointless. See the implementation of set_handle_irq.

> +}
> +
> +static void __iomem * __init icoll_init_iobase(struct device_node *np)
> +{
> +	void __iomem *icoll_base;
> +
> +	icoll_base = of_io_request_and_map(np, 0, np->name);
> +	if (!icoll_base)
> +		panic("%s: unable to map resource", np->full_name);
> +	return icoll_base;
> +}

The panic() is actually a bug fix, because the old code had a
WARN_ON() and then happily dereferenced the NULL pointer. So this
wants to go into a separate patch as well.

> +	icoll_add_domain(np, ICOLL_NUM_IRQS);
>  
> -	icoll_domain = irq_domain_add_linear(np, ICOLL_NUM_IRQS,
> -					     &icoll_irq_domain_ops, NULL);
>  	return icoll_domain ? 0 : -ENODEV;

In case of !icoll_domain this return is not reached as you paniced
already. So why would we still check icoll_domain?

Thanks,

	tglx