[PATCH v3 0/6] spmi: apple: Additional commands and interrupt support.

[PATCH v3 0/6] spmi: apple: Additional commands and interrupt support.

[Sasha Finkelstein]

The SPMI bus has grown additional responsibilities in the M3
generation, making the current driver insufficient. Add M3 comatibles,
support for slave-sent interrupts, FIFO interrupts, power management
commands, parity validation, and fix locking.

To simplify the merge strategy, the device tree entries will be sent
in a future patch series.

Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
Changes in v3:
- Rework interrupt support
- Address review comments
- Link to v2: https://patch.msgid.link/20260728-t603x-spmi-v2-0-f43e5f10e583@chaosmail.tech

Changes in v2:
- Change locking to non-interruptible
- Reorder irq ack
- Clarify dt binding
- Some data type cleanups
- Link to v1: https://patch.msgid.link/20260725-t603x-spmi-v1-0-e1a29fcd2d38@chaosmail.tech

---
Alba Mendez (5):
      spmi: apple: Validate FIFO state
      spmi: apple: check transaction status
      spmi: apple: Implement remaining commands
      spmi: apple: lock around FIFOs
      spmi: apple: Add interrupt functionality

Sasha Finkelstein (1):
      dt-bindings: spmi: apple,spmi: Add t603x

 Documentation/devicetree/bindings/spmi/apple,spmi.yaml |  15 +++++
 drivers/spmi/Kconfig                                   |   3 +-
 drivers/spmi/spmi-apple-controller.c                   | 381 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 367 insertions(+), 32 deletions(-)
---
base-commit: 0ce37745d4bfbc493f718169c3974898ffec8ee7
change-id: 20260725-t603x-spmi-74630bf1b0a0

Best regards,
--  
Sasha Finkelstein <k@chaosmail.tech>

[PATCH v3 1/6] dt-bindings: spmi: apple,spmi: Add t603x

[Sasha Finkelstein]

Add t603x compatibles, interrupt support, and support for SPMI
controllers that are not always-on.

Keeping the "interrupt-controller" property as optional, as taking the
j514c as an example, we only need this functionality on 3 out of 10
SPMI controllers present on this SoC.

Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
 Documentation/devicetree/bindings/spmi/apple,spmi.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
index 3e5b14bc8c31..fceed47b2cd1 100644
--- a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
+++ b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
@@ -20,8 +20,11 @@ properties:
       - items:
           - enum:
               - apple,t6020-spmi
+              - apple,t6030-spmi
+              - apple,t6031-spmi
               - apple,t8012-spmi
               - apple,t8015-spmi
+              - apple,t8122-spmi
           - const: apple,t8103-spmi
       - items:
           - enum:
@@ -34,6 +37,18 @@ properties:
   reg:
     maxItems: 1
 
+  interrupts:
+    maxItems: 1
+    description: Optional, operates in polled mode if not present
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  power-domains:
+    maxItems: 1
+
 required:
   - compatible
   - reg

-- 
2.55.0

[PATCH v3 2/6] spmi: apple: Validate FIFO state

[Sasha Finkelstein]

From: Alba Mendez <me@alba.sh>

Check for data before reading the body of a reply, and check for
end of data afterwards.

Signed-off-by: Alba Mendez <me@alba.sh>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
 drivers/spmi/spmi-apple-controller.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index 376cf682c43e..e037ef914b6c 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -57,6 +57,20 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
 	return 0;
 }
 
+static void flush_extra_data(struct spmi_controller *ctrl)
+{
+	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
+	bool warned = false;
+
+	while (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
+		if (!warned) {
+			dev_warn(&ctrl->dev, "FIFO has extra data\n");
+			warned = true;
+		}
+		readl(spmi->regs + SPMI_RSP_REG);
+	}
+}
+
 static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 			 u16 saddr, u8 *buf, size_t len)
 {
@@ -78,6 +92,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 
 	/* Read SPMI data reply */
 	while (len_read < len) {
+		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
+			dev_err_ratelimited(&ctrl->dev,
+					    "FIFO lacks reply data, controller stuck?\n");
+			return -EIO;
+		}
 		rsp = readl(spmi->regs + SPMI_RSP_REG);
 		i = 0;
 		while ((len_read < len) && (i < 4)) {
@@ -86,6 +105,8 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 		}
 	}
 
+	flush_extra_data(ctrl);
+
 	return 0;
 }
 
@@ -115,6 +136,8 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	/* Discard */
 	readl(spmi->regs + SPMI_RSP_REG);
 
+	flush_extra_data(ctrl);
+
 	return 0;
 }
 

-- 
2.55.0

[PATCH v3 3/6] spmi: apple: check transaction status

[Sasha Finkelstein]

From: Alba Mendez <me@alba.sh>

Check for parity errors and missing command ACKs

Signed-off-by: Alba Mendez <me@alba.sh>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
 drivers/spmi/spmi-apple-controller.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index e037ef914b6c..db03081796d1 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -22,6 +22,12 @@
 #define SPMI_CMD_REG 0x4
 #define SPMI_RSP_REG 0x8
 
+/* SPMI_RSP_REG reply word */
+#define SPMI_REPLY_FRAME_PARITY_STATUS GENMASK(31, 16)
+#define SPMI_REPLY_ACK BIT(15)
+#define SPMI_REPLY_SLAVE_ID GENMASK(14, 8)
+#define SPMI_REPLY_CMD GENMASK(7, 0)
+
 #define SPMI_RX_FIFO_EMPTY BIT(24)
 
 #define REG_POLL_INTERVAL_US 10000
@@ -76,7 +82,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 {
 	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
 	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
-	u32 rsp;
+	u32 reply, rsp;
 	size_t len_read = 0;
 	u8 i;
 	int ret;
@@ -87,8 +93,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	if (ret)
 		return ret;
 
-	/* Discard SPMI reply status */
-	readl(spmi->regs + SPMI_RSP_REG);
+	reply = readl(spmi->regs + SPMI_RSP_REG);
 
 	/* Read SPMI data reply */
 	while (len_read < len) {
@@ -107,6 +112,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 
 	flush_extra_data(ctrl);
 
+	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
+		dev_err(&ctrl->dev, "some frames failed parity check\n");
+		return -EIO;
+	}
 	return 0;
 }
 
@@ -115,6 +124,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 {
 	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
 	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
+	u32 reply;
 	size_t i = 0, j;
 	int ret;
 
@@ -133,11 +143,14 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	if (ret)
 		return ret;
 
-	/* Discard */
-	readl(spmi->regs + SPMI_RSP_REG);
+	reply = readl(spmi->regs + SPMI_RSP_REG);
 
 	flush_extra_data(ctrl);
 
+	if (!FIELD_GET(SPMI_REPLY_ACK, reply)) {
+		dev_err(&ctrl->dev, "command not acknowledged\n");
+		return -EIO;
+	}
 	return 0;
 }
 

-- 
2.55.0

[PATCH v3 4/6] spmi: apple: Implement remaining commands

[Sasha Finkelstein]

From: Alba Mendez <me@alba.sh>

Add support for zero write and power management commands

Signed-off-by: Alba Mendez <me@alba.sh>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
 drivers/spmi/spmi-apple-controller.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------
 1 file changed, 70 insertions(+), 41 deletions(-)

diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index db03081796d1..8c9a70ba0422 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -41,9 +41,9 @@ struct apple_spmi {
 	readl_poll_timeout((spmi)->regs + (reg), (val), (cond), \
 			   REG_POLL_INTERVAL_US, REG_POLL_TIMEOUT_US)
 
-static inline u32 apple_spmi_pack_cmd(u8 opc, u8 sid, u16 saddr, size_t len)
+static inline u32 apple_spmi_pack_cmd(u8 opc, u8 sid, u16 param)
 {
-	return opc | sid << 8 | saddr << 16 | (len - 1) | (1 << 15);
+	return opc | sid << 8 | (u32)param << 16 | (1 << 15);
 }
 
 /* Wait for Rx FIFO to have something */
@@ -77,18 +77,26 @@ static void flush_extra_data(struct spmi_controller *ctrl)
 	}
 }
 
-static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
-			 u16 saddr, u8 *buf, size_t len)
+static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+			 u16 param, const u8 *buf, size_t len, u8 *ibuf, size_t ilen)
 {
 	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
-	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
+	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, param);
 	u32 reply, rsp;
 	size_t len_read = 0;
-	u8 i;
+	size_t i = 0, j;
 	int ret;
 
 	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
 
+	while (i < len) {
+		j = min_t(size_t, sizeof(spmi_cmd), len - i);
+		spmi_cmd = 0;
+		memcpy(&spmi_cmd, buf + i, j);
+		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+		i += j;
+	}
+
 	ret = apple_spmi_wait_rx_not_empty(ctrl);
 	if (ret)
 		return ret;
@@ -96,62 +104,82 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	reply = readl(spmi->regs + SPMI_RSP_REG);
 
 	/* Read SPMI data reply */
-	while (len_read < len) {
+	while (len_read < ilen) {
 		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
-			dev_err_ratelimited(&ctrl->dev,
-					    "FIFO lacks reply data, controller stuck?\n");
+			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
 			return -EIO;
 		}
 		rsp = readl(spmi->regs + SPMI_RSP_REG);
-		i = 0;
-		while ((len_read < len) && (i < 4)) {
-			buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);
-			i += 1;
-		}
+		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
+		memcpy(ibuf + len_read, &rsp, i);
+		len_read += i;
 	}
 
 	flush_extra_data(ctrl);
 
-	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
+	if (!ilen && !FIELD_GET(SPMI_REPLY_ACK, reply)) {
+		dev_err(&ctrl->dev, "command not acknowledged\n");
+		return -EIO;
+	}
+	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << ilen) - 1)) {
 		dev_err(&ctrl->dev, "some frames failed parity check\n");
 		return -EIO;
 	}
 	return 0;
 }
 
-static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
-			  u16 saddr, const u8 *buf, size_t len)
+/* Send a raw command with 1..16 input data frames */
+static int spmi_raw_cmd_input(struct spmi_controller *ctrl, u8 opc, u8 sid,
+			 u16 param, u8 *buf, size_t len)
 {
-	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
-	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
-	u32 reply;
-	size_t i = 0, j;
-	int ret;
-
-	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+	return spmi_raw_cmd(ctrl, opc, sid, param, NULL, 0, buf, len);
+}
 
-	while (i < len) {
-		j = 0;
-		spmi_cmd = 0;
-		while ((j < 4) & (i < len))
-			spmi_cmd |= buf[i++] << (j++ * 8);
+/* Send a raw command with (optional) body and an input ACK */
+static int spmi_raw_cmd_ack(struct spmi_controller *ctrl, u8 opc, u8 sid,
+			  u16 param, const u8 *buf, size_t len)
+{
+	return spmi_raw_cmd(ctrl, opc, sid, param, buf, len, NULL, 0);
+}
 
-		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
+static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+			 u16 saddr, u8 *buf, size_t len)
+{
+	switch (opc) {
+	case SPMI_CMD_EXT_READ:
+	case SPMI_CMD_EXT_READL:
+		return spmi_raw_cmd_input(ctrl, opc | (len - 1), sid, saddr, buf, len);
+	case SPMI_CMD_READ:
+		return spmi_raw_cmd_input(ctrl, opc | saddr, sid, saddr, buf, len);
 	}
+	return -EINVAL;
+}
 
-	ret = apple_spmi_wait_rx_not_empty(ctrl);
-	if (ret)
-		return ret;
-
-	reply = readl(spmi->regs + SPMI_RSP_REG);
-
-	flush_extra_data(ctrl);
+static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+			  u16 saddr, const u8 *buf, size_t len)
+{
+	switch (opc) {
+	case SPMI_CMD_WRITE:
+		return spmi_raw_cmd_ack(ctrl, opc | saddr, sid, buf[0] << 8 | saddr, NULL, 0);
+	case SPMI_CMD_ZERO_WRITE:
+		return spmi_raw_cmd_ack(ctrl, opc | buf[0], sid, buf[0] << 8 | saddr, NULL, 0);
+	case SPMI_CMD_EXT_WRITE:
+	case SPMI_CMD_EXT_WRITEL:
+		return spmi_raw_cmd_ack(ctrl, opc | (len - 1), sid, saddr, buf, len);
+	}
+	return -EINVAL;
+}
 
-	if (!FIELD_GET(SPMI_REPLY_ACK, reply)) {
-		dev_err(&ctrl->dev, "command not acknowledged\n");
-		return -EIO;
+static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
+{
+	switch (opc) {
+	case SPMI_CMD_RESET:
+	case SPMI_CMD_SLEEP:
+	case SPMI_CMD_SHUTDOWN:
+	case SPMI_CMD_WAKEUP:
+		return spmi_raw_cmd_ack(ctrl, opc, sid, 0, NULL, 0);
 	}
-	return 0;
+	return -EINVAL;
 }
 
 static int apple_spmi_probe(struct platform_device *pdev)
@@ -174,6 +202,7 @@ static int apple_spmi_probe(struct platform_device *pdev)
 
 	ctrl->read_cmd = spmi_read_cmd;
 	ctrl->write_cmd = spmi_write_cmd;
+	ctrl->cmd = spmi_cmd;
 
 	ret = devm_spmi_controller_add(&pdev->dev, ctrl);
 	if (ret)

-- 
2.55.0

[PATCH v3 5/6] spmi: apple: lock around FIFOs

[Sasha Finkelstein]

From: Alba Mendez <me@alba.sh>

The driver was missing locking around register interactions

Signed-off-by: Alba Mendez <me@alba.sh>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
 drivers/spmi/spmi-apple-controller.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index 8c9a70ba0422..74d4b5d98b11 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -14,6 +14,7 @@
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/spmi.h>
 
@@ -35,6 +36,7 @@
 
 struct apple_spmi {
 	void __iomem *regs;
+	struct mutex fifo_lock;
 };
 
 #define poll_reg(spmi, reg, val, cond) \
@@ -87,6 +89,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	size_t i = 0, j;
 	int ret;
 
+	guard(mutex)(&spmi->fifo_lock);
+
 	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
 
 	while (i < len) {
@@ -193,6 +197,7 @@ static int apple_spmi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	spmi = spmi_controller_get_drvdata(ctrl);
+	mutex_init(&spmi->fifo_lock);
 
 	spmi->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(spmi->regs))

-- 
2.55.0

[PATCH v3 6/6] spmi: apple: Add interrupt functionality

[Sasha Finkelstein]

From: Alba Mendez <me@alba.sh>

Add support for interrupts sent by slave devices and use IRQ for
RX FIFO if possible, as that IRQ fires as soon as the reply is
available, which is usually takes a few us instead of the 10ms sleep
interval for polling

Signed-off-by: Alba Mendez <me@alba.sh>
Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
---
 drivers/spmi/Kconfig                 |   3 +-
 drivers/spmi/spmi-apple-controller.c | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 252 insertions(+), 2 deletions(-)

diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
index a80cf4047b86..7243863a09b4 100644
--- a/drivers/spmi/Kconfig
+++ b/drivers/spmi/Kconfig
@@ -13,7 +13,8 @@ if SPMI
 
 config SPMI_APPLE
 	tristate "Apple SoC SPMI Controller platform driver"
-	depends on ARCH_APPLE || COMPILE_TEST
+	select IRQ_DOMAIN_HIERARCHY
+	depends on ARCH_APPLE || (COMPILE_TEST && 64BIT)
 	help
 	  If you say yes to this option, support will be included for the
 	  SPMI controller present on many Apple SoCs, including the
diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
index 74d4b5d98b11..340d28126b3d 100644
--- a/drivers/spmi/spmi-apple-controller.c
+++ b/drivers/spmi/spmi-apple-controller.c
@@ -11,11 +11,17 @@
  *		spmi-pmic-arb.c Copyright (c) 2021, The Linux Foundation.
  */
 
+#include <linux/completion.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 #include <linux/spmi.h>
 
 /* SPMI Controller Registers */
@@ -23,6 +29,13 @@
 #define SPMI_CMD_REG 0x4
 #define SPMI_RSP_REG 0x8
 
+#define SPMI_IRQ_MASK_BASE 0x20
+#define SPMI_IRQ_ACK_BASE 0x60
+#define SPMI_NUM_PERIPHERAL_IRQS 256
+#define SPMI_NUM_IRQS (SPMI_NUM_PERIPHERAL_IRQS + 32)
+
+#define SPMI_IRQ_FIFO_RX 256
+
 /* SPMI_RSP_REG reply word */
 #define SPMI_REPLY_FRAME_PARITY_STATUS GENMASK(31, 16)
 #define SPMI_REPLY_ACK BIT(15)
@@ -37,6 +50,12 @@
 struct apple_spmi {
 	void __iomem *regs;
 	struct mutex fifo_lock;
+	struct completion fifo_rx;
+	struct irq_domain *irqd;
+	raw_spinlock_t irq_mask_lock;
+	DECLARE_BITMAP(irq_mask_cache, SPMI_NUM_PERIPHERAL_IRQS);
+	int irq;
+	bool fifo_rx_irq;
 };
 
 #define poll_reg(spmi, reg, val, cond) \
@@ -55,7 +74,19 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
 	int ret;
 	u32 status;
 
-	ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
+	if (spmi->fifo_rx_irq) {
+		ret = wait_for_completion_timeout(&spmi->fifo_rx,
+			usecs_to_jiffies(REG_POLL_TIMEOUT_US));
+		if (!ret)
+			ret = -ETIMEDOUT;
+		else if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)
+			ret = -EIO;
+		else
+			ret = 0;
+	} else {
+		ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
+	}
+
 	if (ret) {
 		dev_err(&ctrl->dev,
 			"failed to wait for RX FIFO not empty\n");
@@ -91,6 +122,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 
 	guard(mutex)(&spmi->fifo_lock);
 
+	reinit_completion(&spmi->fifo_rx);
+
 	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
 
 	while (i < len) {
@@ -186,6 +219,203 @@ static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
 	return -EINVAL;
 }
 
+static void apple_spmi_irq_ack_raw(struct apple_spmi *spmi, u32 irq)
+{
+	u32 __iomem *reg = spmi->regs + SPMI_IRQ_ACK_BASE + (irq / 32) * 4;
+
+	writel(BIT(irq % 32), reg);
+}
+
+static void apple_spmi_irq_mask_raw(struct apple_spmi *spmi, u32 irq)
+{
+	u32 __iomem *reg = spmi->regs + SPMI_IRQ_MASK_BASE + (irq / 32) * 4;
+
+	writel(readl(reg) & ~BIT(irq % 32), reg);
+}
+
+static void apple_spmi_irq_unmask_raw(struct apple_spmi *spmi, u32 irq)
+{
+	u32 __iomem *reg = spmi->regs + SPMI_IRQ_MASK_BASE + (irq / 32) * 4;
+
+	writel(readl(reg) | BIT(irq % 32), reg);
+}
+
+static void apple_spmi_irq_ack(struct irq_data *d)
+{
+	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
+
+	apple_spmi_irq_ack_raw(spmi, d->hwirq);
+}
+
+static void apple_spmi_irq_mask(struct irq_data *d)
+{
+	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
+	apple_spmi_irq_mask_raw(spmi, d->hwirq);
+	clear_bit(d->hwirq, spmi->irq_mask_cache);
+	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
+}
+
+static void apple_spmi_irq_unmask(struct irq_data *d)
+{
+	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
+	set_bit(d->hwirq, spmi->irq_mask_cache);
+	apple_spmi_irq_unmask_raw(spmi, d->hwirq);
+	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
+}
+
+static int apple_spmi_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	/* all interrupts have MSI semantics */
+	return type == IRQ_TYPE_EDGE_RISING ? 0 : -EINVAL;
+}
+
+static struct irq_chip apple_spmi_irq_chip = {
+	.name = "apple_spmi",
+	.irq_mask = apple_spmi_irq_mask,
+	.irq_unmask = apple_spmi_irq_unmask,
+	.irq_ack = apple_spmi_irq_ack,
+	.irq_set_type = apple_spmi_irq_set_type,
+	.flags = IRQCHIP_ONESHOT_SAFE,
+};
+
+static int apple_spmi_irq_domain_map(struct irq_domain *irqd,
+					unsigned int irq, irq_hw_number_t hw)
+{
+	irq_domain_set_info(irqd, irq, hw, &apple_spmi_irq_chip, irqd->host_data,
+				handle_edge_irq, NULL, NULL);
+	return 0;
+}
+
+static int apple_spmi_irq_domain_translate(struct irq_domain *irqd,
+					struct irq_fwspec *fwspec,
+					unsigned long *hwirq,
+					unsigned int *type)
+{
+	u32 *args = fwspec->param;
+
+	if (fwspec->param_count != 2)
+		return -EINVAL;
+
+	if (args[0] >= SPMI_NUM_PERIPHERAL_IRQS)
+		return -EINVAL;
+	*hwirq = args[0];
+	*type = args[1] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+
+static int apple_spmi_irq_domain_alloc(struct irq_domain *irqd, unsigned int virq,
+				unsigned int nr_irqs, void *arg)
+{
+	unsigned int type = IRQ_TYPE_NONE;
+	struct irq_fwspec *fwspec = arg;
+	irq_hw_number_t hwirq;
+	int i, ret;
+
+	ret = apple_spmi_irq_domain_translate(irqd, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	if (hwirq + nr_irqs > SPMI_NUM_PERIPHERAL_IRQS)
+		return -EINVAL;
+
+	for (i = 0; i < nr_irqs; i++) {
+		ret = apple_spmi_irq_domain_map(irqd, virq + i, hwirq + i);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void apple_spmi_irq_domain_free(struct irq_domain *irqd, unsigned int virq,
+				unsigned int nr_irqs)
+{
+	int i;
+
+	for (i = 0; i < nr_irqs; i++) {
+		struct irq_data *d = irq_domain_get_irq_data(irqd, virq + i);
+
+		irq_set_handler(virq + i, NULL);
+		irq_domain_reset_irq_data(d);
+	}
+}
+
+static const struct irq_domain_ops apple_spmi_irq_domain_ops = {
+	.translate	= apple_spmi_irq_domain_translate,
+	.alloc		= apple_spmi_irq_domain_alloc,
+	.free		= apple_spmi_irq_domain_free,
+};
+
+static void apple_spmi_irq_handler(struct irq_desc *desc)
+{
+	struct apple_spmi *spmi = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	bool handled = false;
+	unsigned long val, offset, bit;
+
+	chained_irq_enter(chip, desc);
+	val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + (SPMI_IRQ_FIFO_RX / 32) * 4);
+	if (val & BIT(SPMI_IRQ_FIFO_RX % 32)) {
+		apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_FIFO_RX);
+		complete(&spmi->fifo_rx);
+		handled = true;
+	}
+
+	for (offset = 0; offset < SPMI_NUM_PERIPHERAL_IRQS / 8; offset += sizeof(val)) {
+		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);
+		/**
+		 * because of other masters in the bus, we're going to get a multitude of
+		 * interrupts we're not interested in. irq_resolve_mapping isn't very
+		 * optimized for the nonexistent path, so instead we mask with (a locally
+		 * cached version of) the IRQ mask
+		 */
+		val &= spmi->irq_mask_cache[offset / sizeof(val)];
+		for_each_set_bit(bit, &val, 64) {
+			generic_handle_domain_irq(spmi->irqd, offset * 8 + bit);
+			handled = true;
+			val &= ~BIT(bit);
+		}
+	}
+	if (!handled)
+		handle_bad_irq(desc);
+	chained_irq_exit(chip, desc);
+}
+
+static int apple_spmi_init_irq(struct platform_device *pdev,
+			  struct apple_spmi *spmi, int irq)
+{
+	struct irq_domain_info info = {
+		.fwnode		= pdev->dev.fwnode,
+		.hwirq_max	= ~0U,
+		.ops		= &apple_spmi_irq_domain_ops,
+		.host_data	= spmi,
+	};
+
+	raw_spin_lock_init(&spmi->irq_mask_lock);
+
+	for (size_t offset = 0; offset < SPMI_NUM_IRQS / 8; offset += 4) {
+		writel(0, spmi->regs + SPMI_IRQ_MASK_BASE + offset);
+		writel(U32_MAX, spmi->regs + SPMI_IRQ_ACK_BASE + offset);
+	}
+
+	spmi->irqd = devm_irq_domain_instantiate(&pdev->dev, &info);
+	if (IS_ERR(spmi->irqd))
+		return PTR_ERR(spmi->irqd);
+
+	spmi->fifo_rx_irq = true;
+	apple_spmi_irq_unmask_raw(spmi, SPMI_IRQ_FIFO_RX);
+
+	irq_set_chained_handler_and_data(spmi->irq, apple_spmi_irq_handler, spmi);
+
+	return 0;
+}
+
 static int apple_spmi_probe(struct platform_device *pdev)
 {
 	struct apple_spmi *spmi;
@@ -198,6 +428,8 @@ static int apple_spmi_probe(struct platform_device *pdev)
 
 	spmi = spmi_controller_get_drvdata(ctrl);
 	mutex_init(&spmi->fifo_lock);
+	init_completion(&spmi->fifo_rx);
+	platform_set_drvdata(pdev, spmi);
 
 	spmi->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(spmi->regs))
@@ -209,6 +441,15 @@ static int apple_spmi_probe(struct platform_device *pdev)
 	ctrl->write_cmd = spmi_write_cmd;
 	ctrl->cmd = spmi_cmd;
 
+	spmi->irq = platform_get_irq_optional(pdev, 0);
+	if (spmi->irq < 0 && spmi->irq != -ENXIO)
+		return spmi->irq;
+	if (spmi->irq >= 0) {
+		ret = apple_spmi_init_irq(pdev, spmi, spmi->irq);
+		if (ret)
+			return ret;
+	}
+
 	ret = devm_spmi_controller_add(&pdev->dev, ctrl);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret,
@@ -217,6 +458,13 @@ static int apple_spmi_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static void apple_spmi_remove(struct platform_device *pdev)
+{
+	struct apple_spmi *spmi = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(spmi->irq, NULL, NULL);
+}
+
 static const struct of_device_id apple_spmi_match_table[] = {
 	{ .compatible = "apple,t8103-spmi", },
 	{ .compatible = "apple,spmi", },
@@ -226,6 +474,7 @@ MODULE_DEVICE_TABLE(of, apple_spmi_match_table);
 
 static struct platform_driver apple_spmi_driver = {
 	.probe		= apple_spmi_probe,
+	.remove		= apple_spmi_remove,
 	.driver		= {
 		.name	= "apple-spmi",
 		.of_match_table = apple_spmi_match_table,

-- 
2.55.0

Re: [PATCH v3 1/6] dt-bindings: spmi: apple,spmi: Add t603x

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] New provider capabilities (`interrupt-controller` and `#interrupt-cells`) are added without proper requirements or dependencies, and the example is not updated, violating subsystem guidelines.
--

--- Patch [1]: [PATCH v3 1/6] dt-bindings: spmi: apple,spmi: Add t603x ---
commit 93ff9762b75f8c79f11e2bf64785642aca6a9d56
Author: Sasha Finkelstein <k@chaosmail.tech>

dt-bindings: spmi: apple,spmi: Add t603x

Add t603x compatibles, interrupt support, and support for SPMI
controllers that are not always-on. The interrupt-controller property
is kept optional since it is only needed on a subset of the controllers.

> diff --git a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> index 3e5b14bc8c317..fceed47b2cd17 100644
> --- a/Documentation/devicetree/bindings/spmi/apple,spmi.yaml
> +++ b/Documentation/devicetree/bindings/spmi/apple,spmi.yaml

[ ... ]

> @@ -34,6 +37,18 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  interrupts:
> +    maxItems: 1
> +    description: Optional, operates in polled mode if not present
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2

[Severity: Medium]
Are these new provider capabilities properly enforced?

The guidelines state that when hardware gains new provider capabilities, the
binding must add the corresponding standard properties to the required list.

Since "interrupt-controller" is optional here, incomplete device tree nodes
could silently pass schema validation if they declare "interrupt-controller"
but omit "#interrupt-cells".

Should there be a dependencies block added to ensure "#interrupt-cells" is
present whenever the "interrupt-controller" property is used?

For example:
dependencies:
  interrupt-controller: [ "#interrupt-cells" ]

Also, should the examples section be updated to demonstrate these newly added
provider capabilities?

> +
> +  power-domains:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-t603x-spmi-v3-0-c17b506d91a1@chaosmail.tech?part=1

Re: [PATCH v3 2/6] spmi: apple: Validate FIFO state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Unbounded hardware polling loop in flush_extra_data() can cause a kernel soft lockup.
- [Medium] spmi_read_cmd() immediately aborts multi-word reads if the FIFO temporarily drains mid-transaction.

Pre-existing issues:
- [High] Missing validation on the `len` parameter causes an integer underflow when packing commands.
- [High] Lack of synchronization in spmi_read_cmd() and spmi_write_cmd() causes data corruption on concurrent access.
- [High] The driver violates I/O accessor guidelines by using register accessors for FIFOs.
--

--- Patch [2]: [PATCH v3 2/6] spmi: apple: Validate FIFO state ---
commit 93b3f99f86c4f9fba2be0ca9a0aaffd4f71aca98
Author: Alba Mendez <me@alba.sh>

spmi: apple: Validate FIFO state

Check for data before reading the body of a reply, and check for
end of data afterwards.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 376cf682c43e0..e037ef914b6c3 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> +static void flush_extra_data(struct spmi_controller *ctrl)
> +{
> +	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> +	bool warned = false;
> +
> +	while (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {

[Severity: High]
Could this loop lead to a kernel soft lockup?

If the hardware is powered down or in an unexpected state, the empty bit will
never assert. Since there is no iteration limit or timeout, the kernel would
spin indefinitely.

> +		if (!warned) {
> +			dev_warn(&ctrl->dev, "FIFO has extra data\n");
> +			warned = true;
> +		}
> +		readl(spmi->regs + SPMI_RSP_REG);
> +	}
> +}
> +
>  static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  			 u16 saddr, u8 *buf, size_t len)
>  {
[ ... ]
>  	/* Read SPMI data reply */
>  	while (len_read < len) {

[Severity: High]
This is a pre-existing issue, but does this transaction sequence lack
mutual exclusion?

The SPMI subsystem core delegates locking to the driver. Without a lock,
concurrent threads could interleave their writes to SPMI_CMD_REG and read
each other's responses from SPMI_RSP_REG.

The newly added flush_extra_data() might also inadvertently discard
responses intended for concurrently executing threads.

> +		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			dev_err_ratelimited(&ctrl->dev,
> +					    "FIFO lacks reply data, controller stuck?\n");
> +			return -EIO;
> +		}

[Severity: Medium]
Does this immediate error check risk aborting multi-word reads if the FIFO
temporarily drains mid-transaction?

Because the CPU MMIO reads execute much faster than the serial SPMI bus, the
FIFO might briefly empty before the hardware finishes buffering the entire
response.

Aborting immediately would bypass flush_extra_data(), leaving delayed
data stuck in the FIFO and potentially corrupting subsequent transactions.

>  		rsp = readl(spmi->regs + SPMI_RSP_REG);

[Severity: High]
This is a pre-existing issue, but does the use of readl() for a FIFO
violate subsystem endianness guidelines?

According to the I/O accessors guidelines, FIFOs should exclusively use
stream accessors (like readsl()) to prevent implicit CPU-to-device
endianness byteswapping.

On big-endian systems, readl() performs a byteswap, which could result
in a reversed byte stream.

>  		i = 0;
>  		while ((len_read < len) && (i < 4)) {
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does apple_spmi_pack_cmd()
lack validation on the len parameter?

If a client driver requests a 0-length SPMI transaction, subtracting 1 from
an unsigned size_t len could underflow to SIZE_MAX.

The bitwise OR would then corrupt the opc, sid, and saddr fields, completely
malforming the hardware command:

drivers/spmi/spmi-apple-controller.c:apple_spmi_pack_cmd()
    return opc | sid << 8 | saddr << 16 | (len - 1) | (1 << 15);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-t603x-spmi-v3-0-c17b506d91a1@chaosmail.tech?part=2

Re: [PATCH v3 4/6] spmi: apple: Implement remaining commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Integer underflow in SPMI read loop size calculation causes an out-of-bounds write (buffer overflow).
- [Medium] Using `memcpy()` combined with `writel()`/`readl()` to pack/unpack stream data breaks big-endian compatibility and causes data corruption.
--

--- Patch [4]: [PATCH v3 4/6] spmi: apple: Implement remaining commands ---
commit 92726351469b11b82f4d483115faf989e21b09b0
Author: Alba Mendez <me@alba.sh>

spmi: apple: Implement remaining commands

This commit adds support for zero write and power management commands to the
Apple SPMI controller. It also refactors the command transmission logic into
shared raw command helper functions for both read and write paths.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -77,18 +77,26 @@ static void flush_extra_data(struct spmi_controller *ctrl)
> -static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> -			 u16 saddr, u8 *buf, size_t len)
> +static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +			 u16 param, const u8 *buf, size_t len, u8 *ibuf, size_t ilen)
>  {
>  	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> -	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
> +	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, param);
>  	u32 reply, rsp;
>  	size_t len_read = 0;
> -	u8 i;
> +	size_t i = 0, j;
>  	int ret;
>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
> +	while (i < len) {
> +		j = min_t(size_t, sizeof(spmi_cmd), len - i);
> +		spmi_cmd = 0;
> +		memcpy(&spmi_cmd, buf + i, j);
> +		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: Medium]
Will this sequence cause data corruption on big-endian architectures?

By using memcpy() to copy the byte buffer directly into a u32 variable, and
then passing that variable to writel(), the implicit CPU-to-little-endian
byteswap in writel() will reverse the byte order on the bus.

The previous code avoided this by packing the integer using manual bit shifts,
which preserves the byte layout regardless of CPU endianness.

> +		i += j;
> +	}
> +
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);

[ ... ]

> -	while (len_read < len) {
> +	while (len_read < ilen) {
>  		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> -			dev_err_ratelimited(&ctrl->dev,
> -					    "FIFO lacks reply data, controller stuck?\n");
> +			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
>  			return -EIO;
>  		}
>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
> -		i = 0;
> -		while ((len_read < len) && (i < 4)) {
> -			buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);
> -			i += 1;
> -		}
> +		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
> +		memcpy(ibuf + len_read, &rsp, i);

[Severity: Critical]
Does this length calculation underflow and cause an out-of-bounds write?

Since len_read starts at 0 and the loop condition ensures it is strictly less
than ilen, the subtraction of len_read - ilen will mathematically yield a
negative number. When evaluated as an unsigned size_t, this underflows to a
massive positive number.

Because of this underflow, min_t() will always select sizeof(spmi_cmd), which
is 4. If a caller requests a read smaller than 4 bytes, memcpy() will
unconditionally write 4 bytes and overflow the destination buffer. Should this
calculation be ilen - len_read instead?

[Severity: Medium]
Similar to the write path above, does unpacking the result of readl() using
memcpy() introduce an endianness issue?

readl() performs an implicit little-endian-to-CPU byteswap, which is then
copied sequentially into the byte buffer, potentially scrambling the payload
on big-endian systems.

> +		len_read += i;
>  	}
>  
>  	flush_extra_data(ctrl);
>  
> -	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
> +	if (!ilen && !FIELD_GET(SPMI_REPLY_ACK, reply)) {
> +		dev_err(&ctrl->dev, "command not acknowledged\n");
> +		return -EIO;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-t603x-spmi-v3-0-c17b506d91a1@chaosmail.tech?part=4

Re: [PATCH v3 5/6] spmi: apple: lock around FIFOs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Out-of-bounds memory write in `spmi_raw_cmd` due to an unsigned integer underflow when calculating remaining bytes for SPMI reply payload copy.
- [High] Improper use of `memcpy` with `writel`/`readl` for stream I/O causes byte reordering and data corruption on Big-Endian architectures.
- [High] The `flush_extra_data` function uses an unbounded `while` loop that can permanently deadlock the SPMI controller and cause a soft lockup.
--

--- Patch [5]: [PATCH v3 5/6] spmi: apple: lock around FIFOs ---
commit a147e75e08f9462d0254d993c5221a8c13ead6d5
Author: Alba Mendez <me@alba.sh>

spmi: apple: lock around FIFOs

The driver was missing locking around register interactions, which this
commit addresses by adding a mutex around the FIFO operations.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 8c9a70ba04221..74d4b5d98b113 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -87,6 +89,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	size_t i = 0, j;
>  	int ret;
>  
> +	guard(mutex)(&spmi->fifo_lock);
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
>  	while (i < len) {

[Severity: Critical]
This is a pre-existing issue, but does this code have an integer underflow
when calculating the remaining bytes to copy?

Looking at spmi_raw_cmd() in drivers/spmi/spmi-apple-controller.c:

	while (len_read < ilen) {
		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
			return -EIO;
		}
		rsp = readl(spmi->regs + SPMI_RSP_REG);
		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
		memcpy(ibuf + len_read, &rsp, i);
		len_read += i;
	}

Since len_read is less than ilen in the loop condition, len_read - ilen
evaluates to a large positive integer. As a result, the min_t() call will
always return sizeof(spmi_cmd), which is 4.

If this performs a 1-byte read, will it blindly copy 4 bytes into a 1-byte
buffer on every iteration, leading to an out-of-bounds memory write?

[Severity: High]
This is a pre-existing issue, but could this implementation violate the I/O
accessor guidelines by mixing memcpy() with writel() and readl() for
stream data?

When writing to the TX FIFO in spmi_raw_cmd():

	while (i < len) {
		j = min_t(size_t, sizeof(spmi_cmd), len - i);
		spmi_cmd = 0;
		memcpy(&spmi_cmd, buf + i, j);
		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
		i += j;
	}

And when reading from the RX FIFO:

		rsp = readl(spmi->regs + SPMI_RSP_REG);
		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
		memcpy(ibuf + len_read, &rsp, i);

Because writel() and readl() perform implicit byte-swapping on big-endian
architectures, placing bytes into a host-endian u32 with memcpy() before
using these accessors reverses the byte order on the bus.

Should this code use the stream I/O accessors like writesl() and readsl()
instead to preserve byte order consistently across architectures?

[Severity: High]
This isn't a bug introduced by this patch, but can the loop in
flush_extra_data() cause a soft lockup?

In drivers/spmi/spmi-apple-controller.c:

static void flush_extra_data(struct spmi_controller *ctrl)
{
	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
	bool warned = false;

	while (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
		if (!warned) {
			dev_warn(&ctrl->dev, "FIFO has extra data\n");
			warned = true;
		}
		readl(spmi->regs + SPMI_RSP_REG);
	}
}

If the hardware misbehaves or enters a faulty state where the
SPMI_RX_FIFO_EMPTY flag is never set, will this unbounded while loop run
indefinitely?

Since it executes with the new fifo_lock held, could this permanently
deadlock any subsequent SPMI transactions and block the CPU core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-t603x-spmi-v3-0-c17b506d91a1@chaosmail.tech?part=5

Re: [PATCH v3 6/6] spmi: apple: Add interrupt functionality

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] 64-bit MMIO access (`readq`) on 32-bit registers causes SError on Apple Silicon.
- [High] Use-After-Free on probe error path due to un-cleaned chained IRQ handler.
- [High] Level-triggered interrupt not masked in handler causes interrupt storm.
- [High] Command timeouts race with `reinit_completion`, causing state corruption.
- [Medium] Unconditional deregistration of potentially negative IRQ in `apple_spmi_remove()`.

Pre-existing issues:
- [Critical] Buffer overflow in `spmi_raw_cmd` due to integer underflow.
--

--- Patch [6]: [PATCH v3 6/6] spmi: apple: Add interrupt functionality ---
commit 36738a2961c152dbbdb4e4ffe86859f785d88e60
Author: Alba Mendez <me@alba.sh>

spmi: apple: Add interrupt functionality
    
Add support for interrupts sent by slave devices and use IRQ for
RX FIFO if possible, as that IRQ fires as soon as the reply is
available, which is usually takes a few us instead of the 10ms sleep
interval for polling

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 74d4b5d98b113..340d28126b3d4 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -91,6 +122,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	guard(mutex)(&spmi->fifo_lock);
>  
> +	reinit_completion(&spmi->fifo_rx);

[Severity: High]
Can this reinit_completion() race with a late hardware interrupt?

If a previous command timed out and left the RX FIFO un-flushed, could a late
reply trigger complete() concurrently with this reinit_completion(), allowing
the new command to bypass the wait and read stale data?

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a potential buffer
overflow in spmi_raw_cmd() when calculating the reply length?

While reading the data reply:

	while (len_read < ilen) {
		...
		rsp = readl(spmi->regs + SPMI_RSP_REG);
		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
		memcpy(ibuf + len_read, &rsp, i);
		len_read += i;
	}

If a caller requests a small read where ilen is less than 4, the condition
len_read < ilen ensures len_read - ilen is negative. Will this underflow to a
massive positive value when cast to size_t by min_t(), causing it to select
sizeof(spmi_cmd) and overwrite the buffer bounds?

> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[ ... ]

> +static void apple_spmi_irq_handler(struct irq_desc *desc)
> +{
> +	struct apple_spmi *spmi = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	bool handled = false;
> +	unsigned long val, offset, bit;
> +
> +	chained_irq_enter(chip, desc);
> +	val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + (SPMI_IRQ_FIFO_RX / 32) * 4);
> +	if (val & BIT(SPMI_IRQ_FIFO_RX % 32)) {
> +		apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_FIFO_RX);
> +		complete(&spmi->fifo_rx);
> +		handled = true;
> +	}

[Severity: High]
Does this leave the level-triggered interrupt unmasked? 

If the RX FIFO isn't drained immediately before returning, could the persistent
hardware level condition re-assert the interrupt right after exiting the 
handler, creating an interrupt storm that starves the thread trying to drain
the FIFO?

> +
> +	for (offset = 0; offset < SPMI_NUM_PERIPHERAL_IRQS / 8; offset += sizeof(val)) {
> +		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);

[Severity: Critical]
Will using readq() on a 32-bit initialized MMIO region cause a synchronous
external abort (SError) on Apple Silicon? 

Since these registers are expected to be 32-bit regions, does a 64-bit access
trigger a hardware trap?

[ ... ]

> @@ -209,6 +441,15 @@ static int apple_spmi_probe(struct platform_device *pdev)
>  	ctrl->write_cmd = spmi_write_cmd;
>  	ctrl->cmd = spmi_cmd;
>  
> +	spmi->irq = platform_get_irq_optional(pdev, 0);
> +	if (spmi->irq < 0 && spmi->irq != -ENXIO)
> +		return spmi->irq;
> +	if (spmi->irq >= 0) {
> +		ret = apple_spmi_init_irq(pdev, spmi, spmi->irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = devm_spmi_controller_add(&pdev->dev, ctrl);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret,

[Severity: High]
If devm_spmi_controller_add() fails, will this leak the chained handler
registered inside apple_spmi_init_irq()? 

Since irq_set_chained_handler_and_data() is used without a devres cleanup 
action, could a subsequent interrupt dereference the freed spmi structure 
on this error path?

[ ... ]

> +static void apple_spmi_remove(struct platform_device *pdev)
> +{
> +	struct apple_spmi *spmi = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(spmi->irq, NULL, NULL);
> +}

[Severity: Medium]
Can spmi->irq be negative here? 

If platform_get_irq_optional() returned -ENXIO during probe, wouldn't we be
passing a negative value (which becomes a very large unsigned integer) to
irq_set_chained_handler_and_data() on removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-t603x-spmi-v3-0-c17b506d91a1@chaosmail.tech?part=6

Re: [PATCH v3 3/6] spmi: apple: check transaction status

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Missing explicit header includes for bitfield and bitwise macros.

Pre-existing issues:
- [High] Leftover FIFO data on error paths permanently desynchronizes subsequent transactions.
- [High] Unbounded `while` loop polling hardware status can cause a kernel hard lockup.
--

--- Patch [3]: [PATCH v3 3/6] spmi: apple: check transaction status ---
commit c52cd20ea1913f6b44118c7681235dcecf7cc8dc
Author: Alba Mendez <me@alba.sh>

spmi: apple: check transaction status

This commit adds checks for parity errors and missing command ACKs in the
Apple SPMI controller driver. It extracts the reply status from the response
register instead of discarding it and returns an error if checks fail.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -22,6 +22,12 @@
>  #define SPMI_CMD_REG 0x4
>  #define SPMI_RSP_REG 0x8
>  
> +/* SPMI_RSP_REG reply word */
> +#define SPMI_REPLY_FRAME_PARITY_STATUS GENMASK(31, 16)
> +#define SPMI_REPLY_ACK BIT(15)
> +#define SPMI_REPLY_SLAVE_ID GENMASK(14, 8)
> +#define SPMI_REPLY_CMD GENMASK(7, 0)
> +

[Severity: Low]
Are we missing explicit header includes for linux/bitfield.h and
linux/bits.h?

This patch introduces the use of GENMASK and FIELD_GET, but appears to rely
on transitive inclusions from other headers, which might cause future build
breakages if those headers change.

[ ... ]

> @@ -87,8 +93,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does this error path leave stale data in
the RX FIFO?

If apple_spmi_wait_rx_not_empty() times out, it returns an error and aborts
immediately without draining the RX FIFO:

drivers/spmi/spmi-apple-controller.c:apple_spmi_wait_rx_not_empty() {
    ...
    ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
    if (ret) {
        dev_err(&ctrl->dev, "failed to wait for RX FIFO not empty\n");
        return ret;
    }
}

The hardware might subsequently complete the transaction and push the reply
into the FIFO. Later commands will begin by writing to the command register
without first asserting that the RX FIFO is drained:

drivers/spmi/spmi-apple-controller.c:spmi_raw_cmd() {
    guard(mutex)(&spmi->fifo_lock);
    reinit_completion(&spmi->fifo_rx);
    writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
    ...
}

Could this permanently desynchronize subsequent transactions by causing them
to read the stale reply data left by the aborted transaction?

> -	/* Discard SPMI reply status */
> -	readl(spmi->regs + SPMI_RSP_REG);
> +	reply = readl(spmi->regs + SPMI_RSP_REG);

[ ... ]

> @@ -107,6 +112,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	flush_extra_data(ctrl);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for this call
to cause a kernel hard lockup?

The flush_extra_data() function implements an unbounded while loop polling
the hardware status:

drivers/spmi/spmi-apple-controller.c:flush_extra_data() {
    while (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
        ...
        readl(spmi->regs + SPMI_RSP_REG);
    }
}

If the hardware enters a faulty state or continuously reports that the FIFO
is not empty, could the CPU loop infinitely here without yielding or timing
out?

> +	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
> +		dev_err(&ctrl->dev, "some frames failed parity check\n");
> +		return -EIO;
> +	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-t603x-spmi-v3-0-c17b506d91a1@chaosmail.tech?part=3