Re: [PATCH v8 9/9] drivers: mfd: Add support for MediaTek SPMI PMICs and MT6363/73

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Friday, 3 October 2025 11:11:58 Central European Summer Time AngeloGioacchino Del Regno wrote:
> This driver adds support for the MediaTek SPMI PMICs and their
> interrupt controller (which is present in 95% of the cases).
> 
> Other than probing all of the sub-devices of a SPMI PMIC, this
> sets up a regmap from the relevant SPMI bus and initializes an
> interrupt controller with its irq domain and irqchip to handle
> chained interrupts, with the SPMI bus itself being its parent
> irq controller, and the PMIC being the outmost device.
> 
> This driver hence holds all of the information about a specific
> PMIC's interrupts and will properly handle them, calling the
> ISR for any subdevice that requested an interrupt.
> 
> As for the interrupt spec, this driver wants 3 interrupt cells,
> but ignores the first one: this is because of how this first
> revision of the MediaTek SPMI 2.0 Controller works, which does
> not hold irq number information in its register, but delegates
> that to the SPMI device - it's possible that this will change
> in the future with a newer revision of the controller IP, and
> this is the main reason for that.
> 
> To make use of this implementation, this driver also adds the
> required bits to support MediaTek MT6363 and MT6373 SPMI PMICs.
> 
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/mfd/Kconfig         |  17 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/mtk-spmi-pmic.c | 410 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/mt6363.h  |  26 +++
>  include/linux/mfd/mt6373.h  |  21 ++
>  5 files changed, 475 insertions(+)
>  create mode 100644 drivers/mfd/mtk-spmi-pmic.c
>  create mode 100644 include/linux/mfd/mt6363.h
>  create mode 100644 include/linux/mfd/mt6373.h

Hi Angelo, came across something suspicious here while debugging why
I ran into IRQ parsing from DT issues here.

>
> [...]
> diff --git a/drivers/mfd/mtk-spmi-pmic.c b/drivers/mfd/mtk-spmi-pmic.c
> new file mode 100644
> index 000000000000..512b53bdb0d1
> --- /dev/null
> +++ b/drivers/mfd/mtk-spmi-pmic.c
> [...]
> +
> +static int mtk_spmi_pmic_irq_xlate(struct irq_domain *d, struct device_node *ctrlr,
> +				  const u32 *intspec, unsigned int intsize,
> +				  unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	struct mtk_spmi_pmic *pmic = d->host_data;
> +	struct device *dev = pmic->dev;
> +	struct irq_fwspec fwspec;
> +
> +	of_phandle_args_to_fwspec(ctrlr, intspec, intsize, &fwspec);
> +	if (WARN_ON(fwspec.param_count < 3))
> +		return -EINVAL;

What's the point of fwspec here? The caller in
irqdomain.c::irq_domain_translate has an fwspec, converts it to an
of_node with the intsize/intspec args, then passes it to this function,
which builds it back into an fwspec just to check fwspec.param_count,
which is equal intsize.

fwspec is then never used again.

Just check intsize instead, it's precisely that value.

spmi-mtk-pmif.c::mtk_spmi_rcs_irq_xlate does the same thing, which also
seems pointless.

> +
> +	/*
> +	 * The IRQ number in intspec[0] is ignored on purpose here!
> +	 *
> +	 * This is because of how at least the first revision of the SPMI 2.0
> +	 * controller works in MediaTek SoCs: the controller will raise an
> +	 * interrupt for each SID (but doesn't know the details!), and the
> +	 * specific IRQ number that got raised must be read from the PMIC or
> +	 * its sub-device driver.
> +	 * It's possible that this will change in the future with a newer
> +	 * revision of the SPMI controller, and this is why the devicetree
> +	 * holds the full intspec.
> +	 */
> +	*out_hwirq = intspec[1];
> +	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	dev_dbg(dev, "Found device IRQ %u chained from SPMI IRQ %x (map: 0x%lx)\n",
> +		intspec[1], intspec[0], *out_hwirq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops mtk_spmi_pmic_irq_domain_ops = {
> +	.map = mtk_spmi_pmic_irq_domain_map,
> +	.xlate = mtk_spmi_pmic_irq_xlate,
> +};
> +
> [...]

Kind regards,
Nicolas Frattaroli