On 9/24/26 11:02 AM, Arnaud Ferraris wrote: > This driver (ported from the Linux kernel) implements 1-byte wide read > and write operations for the SPMI-PMIF bus found on MediaTek SoCs. It > currently supports MT8195 and MT8196. > > Signed-off-by: Arnaud Ferraris <[email protected]> > --- > drivers/spmi/Kconfig | 7 + > drivers/spmi/Makefile | 1 + > drivers/spmi/spmi-mtk-pmif.c | 462 > +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 470 insertions(+) > > diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig > index f8972371150..cab82e83c24 100644 > --- a/drivers/spmi/Kconfig > +++ b/drivers/spmi/Kconfig > @@ -27,6 +27,13 @@ config SPL_SPMI_MSM > Support SPMI bus implementation found on Qualcomm Snapdragon SoCs > in SPL. > > +config SPMI_MTK_PMIF > + bool "Support MediaTek SPMI-PMIF bus" > + depends on SPMI > + help > + Support SPMI bus implementation found on MediaTek SoCs. This > + driver currently support the MT8195 and MT8196 SoCs. > + > config SPMI_SANDBOX > bool "Support for Sandbox SPMI bus" > depends on SPMI > diff --git a/drivers/spmi/Makefile b/drivers/spmi/Makefile > index 8cb93c820e0..016a8e6b734 100644 > --- a/drivers/spmi/Makefile > +++ b/drivers/spmi/Makefile > @@ -4,4 +4,5 @@ > > obj-$(CONFIG_$(PHASE_)SPMI) += spmi-uclass.o > obj-$(CONFIG_$(PHASE_)SPMI_MSM) += spmi-msm.o > +obj-$(CONFIG_SPMI_MTK_PMIF) += spmi-mtk-pmif.o > obj-$(CONFIG_SPMI_SANDBOX) += spmi-sandbox.o > diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c > new file mode 100644 > index 00000000000..0bc8309b447 > --- /dev/null > +++ b/drivers/spmi/spmi-mtk-pmif.c > @@ -0,0 +1,462 @@ > +// SPDX-License-Identifier: GPL-2.0
GPL-2.0 is deprecated. Prefer GPL-2.0-only or GPL-2.0-or-later. > +/* > + * Copyright (c) 2021 MediaTek Inc. > + * Copyright (c) 2025-2026 Collabora Ltd > + * Authors: AngeloGioacchino Del Regno > <[email protected]> > + * Arnaud Ferraris <[email protected]> > + */ > + > +#include <clk.h> > +#include <dm.h> > +#include <malloc.h> > +#include <regmap.h> > +#include <syscon.h> > +#include <asm/io.h> > +#include <asm/processor.h> > +#include <linux/bitops.h> > +#include <linux/err.h> > +#include <linux/iopoll.h> > +#include <linux/time.h> > +#include <dm/device_compat.h> > +#include <dm/device-internal.h> > +#include <dm/devres.h> > +#include <inttypes.h> > +#include <spmi/spmi.h> Are we actually using all of these headers? > +static int pmif_spmi_read(struct udevice *dev, int sid, int pid, int reg) > +{ > + struct pmif_bus *pbus = dev_get_priv(dev); > + struct pmif *arb = pbus->arb; > + struct ch_reg *inf_reg; > + int ret, data; > + u32 cmd; > + > + /* Check for argument validation. */ > + if (sid & ~0xf) { > + dev_err(dev, "exceed the max slv id\n"); > + return -EINVAL; > + } > + > + /* Wait for Software Interface FSM state to be IDLE. */ > + inf_reg = &arb->chan; > + ret = readl_poll_sleep_timeout(pbus->base + > arb->data->regs[inf_reg->ch_sta], > + data, GET_SWINF(data) == SWINF_IDLE, > + PMIF_DELAY_US, PMIF_TIMEOUT_US); > + if (ret < 0) { > + /* set channel ready if the data has transferred */ > + if (pmif_is_fsm_vldclr(arb, pbus)) > + pmif_writel(arb, pbus, 1, inf_reg->ch_rdy); > + dev_err(dev, "failed to wait for SWINF_IDLE\n"); > + return ret; > + } > + > + /* Send the command. */ > + cmd = (PMIF_CMD_EXT_REG_LONG << 30) | (sid << 24) | reg; > + pmif_writel(arb, pbus, cmd, inf_reg->ch_send); > + > + /* > + * Wait for Software Interface FSM state to be WFVLDCLR, > + * read the data and clear the valid flag. > + */ > + ret = readl_poll_sleep_timeout(pbus->base + > arb->data->regs[inf_reg->ch_sta], > + data, GET_SWINF(data) == SWINF_WFVLDCLR, > + PMIF_DELAY_US, PMIF_TIMEOUT_US); > + if (ret < 0) { > + dev_err(dev, "failed to wait for SWINF_WFVLDCLR\n"); > + return ret; > + } > + > + data = (int)pmif_readl(arb, pbus, inf_reg->rdata); This cast looks potentially dangerous. If bit 31 is set, it will be treated as an error. Should we be doing && 0xff instead since we are dealing with only 8-bit values here. > + pmif_writel(arb, pbus, 1, inf_reg->ch_rdy); > + > + return data; > +} > + ... > +static int mtk_spmi_probe(struct udevice *dev) > +{ > + struct pmif_bus *pbus = dev_get_priv(dev); > + int ret; > + > + if (!pbus) { > + dev_info(dev, "Not probing a non-bus spmi device\n"); > + return 0; > + } > + > + pbus->base = devfdt_remap_addr_name(dev, "pmif"); This returns NULL on error, not negative error code. > + if (IS_ERR(pbus->base)) > + return PTR_ERR(pbus->base); > + > + pbus->spmimst_base = devfdt_remap_addr_name(dev, "spmimst"); > + if (IS_ERR(pbus->spmimst_base)) > + return PTR_ERR(pbus->spmimst_base); > + > + ret = clk_get_bulk(dev, &pbus->clks); > + if (ret < 0) { > + dev_err(dev, "Failed to get clocks\n"); > + return ret; > + } > + > + ret = clk_enable_bulk(&pbus->clks); > + if (ret) { > + dev_err(dev, "Failed to enable clocks\n"); > + return ret; > + } > + > + return 0; > +} > + > +static const struct udevice_id mtk_spmi_match_table[] = { > + { > + .compatible = "mediatek,mt8195-spmi", > + .data = (unsigned long)&mt8195_pmif_arb, > + }, { > + .compatible = "mediatek,mt8196-spmi", > + .data = (unsigned long)&mt8196_pmif_arb, > + }, { > + /* sentinel */ > + }, > +}; > + > +static int mtk_spmi_bind(struct udevice *dev) > +{ > + struct pmif_data *drv_data = (struct pmif_data > *)dev_get_driver_data(dev); > + ofnode child, node = dev_ofnode(dev); > + int ret = 0; > + > + if (!drv_data) { > + dev_err(dev, "Cannot get drv_data\n"); > + return -EINVAL; > + } > + This could use some comments to explain how it is supposed to work. It looks like we are using the same driver to also bind child nodes? That seems a bit strange/confusing. And the MT8195 parent node (only node) is named "spmi", so this looks broken since it would match that and try to use the Soc node as the arbitrator. I would probably go as far as making 3 different driver structs each with their own bind function. One for the MT8196 arbitrator that registers new devices for each spmi instance. One for the MT8196 child devices and one for MT8195 which is just a single node (no arbitrator). > + if (ofnode_name_eq(node, "spmi")) { > + struct pmif_bus *bus = devm_kzalloc(dev, sizeof(struct > pmif_bus), GFP_KERNEL); > + > + if (!bus) > + return -ENOMEM; > + bus->arb = dev_get_plat(dev->parent); > + dev_set_priv(dev, bus); > + } else { > + struct pmif *arb; > + u32 chan_offset; > + > + arb = devm_kzalloc(dev, sizeof(struct pmif), GFP_KERNEL); > + if (!arb) > + return -ENOMEM; > + dev_set_plat(dev, arb); > + > + arb->data = drv_data; > + chan_offset = PMIF_CHAN_OFFSET * arb->data->soc_chan; > + arb->chan.ch_sta = PMIF_SWINF_0_STA + chan_offset; > + arb->chan.wdata = PMIF_SWINF_0_WDATA_31_0 + chan_offset; > + arb->chan.rdata = PMIF_SWINF_0_RDATA_31_0 + chan_offset; > + arb->chan.ch_send = PMIF_SWINF_0_ACC + chan_offset; > + arb->chan.ch_rdy = PMIF_SWINF_0_VLD_CLR + chan_offset; > + > + if (arb->data->num_spmi_buses) { > + ofnode_for_each_subnode(child, node) { > + if (!ofnode_name_eq(child, "spmi")) > + continue; > + > + /* Bind the subnode to this driver */ > + ret = device_bind_with_driver_data(dev, > dev->driver, > + > ofnode_get_name(child), > + > dev->driver_data, > + child, > + NULL); > + if (ret == -ENODEV) > + printf("Driver '%s' refuses to bind\n", > + dev->driver->name); > + > + if (ret) > + printf("Error binding driver '%s': > %d\n", > + dev->driver->name, ret);> + > } > + } else { > + struct pmif_bus *bus = devm_kzalloc(dev, > + sizeof(struct > pmif_bus), > + GFP_KERNEL); > + > + if (!bus) > + return -ENOMEM; > + bus->arb = arb; > + dev_set_priv(dev, bus); > + } > + } > + > + return 0; > +} > +
