Re: [PATCH 2/2] irqchip: introduce TC9564 SoC MSIGEN support

[Thomas Gleixner <tglx@kernel.org>]

On Mon, Oct 12 2026 at 14:02, Alex Elder wrote:
> +config TC9564_MSIGEN
> +	tristate "Toshiba TC9564 MSI distributor"
> +	depends on TC9564_PCI
> +	depends on GENERIC_IRQ_CHIP

  s/depends/select/

Otherwise this relies on some other driver to select GENERIC_IRQ_CHIP...

> +/**
> + * struct tc9564_msigen - Context for MSIGEN IRQ domain
> + * @ioaddr:	Pointer to mapped MSIGEN memory
> + * @irq:	IRQ number for MSIGEN
> + */
> +struct tc9564_msigen {
> +	void __iomem *ioaddr;
> +	unsigned int irq;

https://docs.kernel.org/process/maintainer-tip.html#struct-declarations-and-initializers

> +};
> +
> +#define HWIRQ_COUNT			25
> +
> +#define MSI_OUT_EN_OFFSET		0x0000
> +#define MSI_MASK_CLR_OFFSET		0x000c
> +#define MSI_MASK_VALUE			BIT(0)
> +#define MSI_INT_STS_OFFSET		0x0010
> +
> +static void tc9564_msigen_irq_handler(struct irq_desc *desc)
> +{
> +	struct irq_domain *irq_domain = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct irq_chip_generic *gc;
> +	unsigned long status;
> +	unsigned long hwirq;
> +
> +	gc = irq_get_domain_generic_chip(irq_domain, 0);
> +
> +	chained_irq_enter(chip, desc);
> +
> +	status = irq_reg_readl(gc, MSI_INT_STS_OFFSET);
> +	for_each_set_bit(hwirq, &status, HWIRQ_COUNT)
> +		generic_handle_domain_irq(irq_domain, hwirq);

You want to use generic_handle_demux_domain_irq(), which allows you to
actually implement affinity settings for individual interrupts. 

> +	/*
> +	 * Clear the MSI flag. Most interrupts within TC9564 are level-high
> +	 * type. If any interrupts are still asserted then clearing this flag
> +	 * will cause the (edge-triggered) MSI to be regenerated.
> +	 */
> +	irq_reg_writel(gc, MSI_MASK_VALUE, MSI_MASK_CLR_OFFSET);
> +
> +	chained_irq_exit(chip, desc);
> +}
> +static int tc9564_msigen_irq_domain_init(struct irq_domain *irq_domain)
> +{
> +	struct tc9564_msigen *msigen = irq_domain->host_data;
> +
> +	irq_set_chained_handler_and_data(msigen->irq, tc9564_msigen_irq_handler,
> +					 irq_domain);

No line break required. You have 100 characters. All over the place.

> +/* Find the PCI device we're associated with (not necessarily the parent) */

What does that mean and why is this platform device driver dealing with
a PCI device in the first place?

This really wants proper comments explaining this in the source _and_
the change log of this patch. And no, your sloppy cover letter
explanation does not cut it at all.

> +static struct pci_dev *tc9564_msigen_find_pci_dev(struct device *dev)

> +static int tc9564_msigen_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain_chip_generic_info dgc_info;

Struct is uninitialized. 

> +	struct device *dev = &pdev->dev;
> +	struct irq_domain *irq_domain;
> +	struct tc9564_msigen *msigen;
> +	struct irq_domain_info info;

Ditto. Look at other drivers how this is done correctly.

> +	struct reset_control *reset;
> +	struct pci_dev *pci_dev;
> +	struct clk *clk;
> +	int ret;
> +
> +	pci_dev = tc9564_msigen_find_pci_dev(dev);
> +	if (IS_ERR(pci_dev))
> +		return dev_err_probe(dev, PTR_ERR(pci_dev),
> +				     "failed to find PCI device\n");

See bracket rules in the documentation I linked to or simply get rid of
that line break. Same all over the place.

> +	dgc_info.name = "tc9564-msigen";
> +	dgc_info.handler = handle_level_irq;
> +	dgc_info.irqs_per_chip = HWIRQ_COUNT;
> +	dgc_info.num_ct = 1;
> +	dgc_info.init = tc9564_msigen_irq_chip_init;
> +	dgc_info.exit = tc9564_msigen_irq_chip_exit;
> +
> +	info.fwnode = of_fwnode_handle(dev->of_node);
> +	info.domain_flags = IRQ_DOMAIN_FLAG_DESTROY_GC;
> +	info.size = HWIRQ_COUNT;
> +	info.hwirq_max = HWIRQ_COUNT;
> +	info.ops = &irq_generic_chip_ops;
> +	info.host_data = msigen;
> +	info.dgc_info = &dgc_info;
> +	info.init = tc9564_msigen_irq_domain_init;
> +	info.exit = tc9564_msigen_irq_domain_exit;

Both initializations (except for info.host_data) can be done at the
declaration with a static initialization, which avoids that parts of the
structs contain random stack content. No?

Please look through the sashiko comments as well.

Thanks,

        tglx