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

[Janne Grunau <j@jannau.net>]

On Wed, Aug 05, 2026 at 12:11:14PM +0200, Sasha Finkelstein wrote:
> From: Alba Mendez <me@alba.sh>
> 
> Add support for zero write and power management commands

missing dot at the end of the sentence.

> Signed-off-by: Alba Mendez <me@alba.sh>
> Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
> ---
>  drivers/spmi/spmi-apple-controller.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
>  1 file changed, 68 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 7ebf899edf17..49827e51e93e 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -46,9 +46,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 */
> @@ -69,14 +69,14 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
>  	return 0;
>  }
>  
> -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)

Instead of two sets of not very distinctive variable names ("buf"/"len",
"ibuf"/ilen") please use just buf/len and a boolean to indicate whether
the command is a read or write if you insists of routing all commands
through a single function. This can be simply added as first condition
in the "while" iterating over the write/read buffers.

>  {
>  	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;
>  
>  	if (spmi->prev_fail) {
> @@ -86,6 +86,14 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	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;
> @@ -93,7 +101,7 @@ 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) {
>  			spmi->prev_fail = true;
>  			dev_err_ratelimited(&ctrl->dev,
> @@ -101,11 +109,9 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  			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), ilen - len_read);
> +		memcpy(ibuf + len_read, &rsp, i);
> +		len_read += i;
>  	}
>  
>  	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
> @@ -113,54 +119,69 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  		spmi->prev_fail = true;
>  	}
>  
> -	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;
> -
> -	if (spmi->prev_fail) {
> -		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);
> -		spmi->prev_fail = false;
> -	}
> -
> -	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);
> -
> -	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
> -		dev_warn(&ctrl->dev, "FIFO has extra data\n");
> -		spmi->prev_fail = true;
> +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)
> @@ -183,6 +204,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)

looks good otherwise

Janne