[PATCH v4 4/6] spmi: apple: Implement remaining commands
Alba Mendez
me at alba.sh
Fri Aug 7 04:37:08 PDT 2026
On Wed, 2026-08-05 at 14:19 +0200, Janne Grunau wrote:
> On Wed, Aug 05, 2026 at 12:11:14PM +0200, Sasha Finkelstein wrote:
> > From: Alba Mendez <me at 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 at alba.sh>
> > Signed-off-by: Sasha Finkelstein <k at 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.
Hmm, even if constness was not an issue, I would still prefer if
buf/len and ibuf/ilen were kept separate... There are SPMI commands
with both controller-sent and peripheral-sent data frames (like
authentication) it just so happens that all the commands currently
implemented don't need data bytes in both directions. But the format of
the FIFO batches certainly seems prepared to acommodate those.
>
> > {
> > 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
More information about the linux-arm-kernel
mailing list