Re: [PATCH v4 01/10] PCI: Add pci_host_alloc_intx_irqd() for allocating IRQ domain

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello,

One nit below.

On Thu, 14 Jun 2018 09:34:17 +0800, Shawn Lin wrote:

> +	if (!intc) {
> +		intc = of_get_next_child(dev->of_node, NULL);
> +		if (!intc) {
> +			dev_err(dev, "missing child interrupt-controller node\n");
> +			goto err_out;
> +		}
> +		need_put = true;
> +	}
> +
> +	if (!intx_domain_ops) {
> +		irqd_ops = (struct irq_domain_ops *)devm_kmalloc(dev,

The cast doesn't seem to be useful.

> +				sizeof(struct irq_domain_ops), GFP_KERNEL);
> +		if (!irqd_ops) {
> +			err = -ENOMEM;
> +			goto err_out;
> +		}
> +
> +		irqd_ops->map = &pcie_intx_map;
> +		if (general_xlate)
> +			irqd_ops->xlate = &pci_irqd_intx_xlate;
> +		intx_domain_ops = irqd_ops;
> +	}
> +#ifdef CONFIG_IRQ_DOMAIN
> +	domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
> +				       intx_domain_ops, host);
> +#endif

Isn't it a bit weird to have this in the middle of this function ?
Should you instead declare a stub pci_host_alloc_intx_irqd() function
in the header file if CONFIG_IRQ_DOMAIN is not enabled ?

I.e in pci.h:

#ifdef CONFIG_IRQ_DOMAIN
struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
				void *host, bool general_xlate,
				const struct irq_domain_ops *intx_domain_ops,
				struct device_node *local_intc);
#else
static inline struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
	void *host, bool general_xlate, const struct irq_domain_ops *intx_domain_ops,
	struct device_node *local_intc)
{
	return PTR_ERR(-EINVAL);
}
#endif

or something like that.

Finally a purely subjective and personal opinion: I'm not a big fan of
this boolean that tells whether the ->xlate hook should be populated or
not. I'm not sure if it makes sense for this function to create the
irqdomain_ops altogether, perhaps it should be mandatory for the
irq_domain_ops to be provided by the caller. The problem with the
boolean general_xlate is that it is an endless story of additional
booleans to tweak more stuff in the irq_domain_ops structure. But of
course that's just a personal view, and my view doesn't really count
here :-)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com