Re: [PATCH 2/3] spmi: add a spmi driver for Apple SoC

[Stephen Boyd]

Quoting Sasha Finkelstein via B4 Relay (2025-03-05 12:26:40)
> From: Jean-Francois Bortolotti <jeff@xxxxxxxx>
> 

Please write some commit text explaining why this driver is important to
review. Maybe it's necessary for something to work?

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..194fa5dd7c2c6fc4ecfbee0db7930b0c73b02550
> --- /dev/null
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple SoC SPMI device driver
> + *
> + * Copyright The Asahi Linux Contributors
> + *
> + * Inspired by:
> + *             OpenBSD support Copyright (c) 2021 Mark Kettenis <kettenis@xxxxxxxxxxx>
> + *             Correllium support Copyright (C) 2021 Corellium LLC
> + *             hisi-spmi-controller.c
> + *             spmi-pmic-ard.c Copyright (c) 2021, The Linux Foundation.

spmi-pmic-arb?

> + */
> +
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>

Are these includes used? I think you need mod_devicetable.h

> +#include <linux/platform_device.h>
> +#include <linux/spmi.h>
> +
> +/* SPMI Controller Registers */
> +#define SPMI_STATUS_REG 0
> +#define SPMI_CMD_REG 0x4
> +#define SPMI_RSP_REG 0x8
> +
> +#define SPMI_RX_FIFO_EMPTY BIT(24)
> +
> +#define REG_POLL_INTERVAL 10000
> +#define REG_POLL_TIMEOUT (REG_POLL_INTERVAL * 5)
> +
> +struct apple_spmi {
> +       void __iomem *regs;
> +};
> +
> +#define poll_reg(spmi, reg, val, cond) \
> +       readl_relaxed_poll_timeout((spmi)->regs + (reg), (val), (cond), \
> +                                  REG_POLL_INTERVAL, REG_POLL_TIMEOUT)
> +
> +static inline u32 read_reg(struct apple_spmi *spmi, int offset)
> +{
> +       return readl_relaxed(spmi->regs + offset);
> +}
> +
> +static inline void write_reg(u32 value, struct apple_spmi *spmi, int offset)
> +{
> +       writel_relaxed(value, spmi->regs + offset);
> +}

I'm not a huge fan of these wrappers but OK. Why relaxed accessors?

> +
> +static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +                        u16 saddr, u8 *__buf, size_t bc)

Drop the underscore because the variable 'buf' is never used.

> +{
> +       struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> +       u32 spmi_cmd = opc | sid << 8 | saddr << 16 | (bc - 1) | (1 << 15);

Can this be some function like apple_spmi_pack_cmd()? I suspect 'bc' is
byte_count? Usually we just call that 'len'.

> +       u32 rsp;
> +       u32 status;
> +       size_t len_to_read = 0;

len_to_read would imply that it is non-zero to start. Maybe 'len_read'
past tense, or decrement 'bc'.

> +       u8 i;
> +       int ret;
> +
> +       write_reg(spmi_cmd, spmi, SPMI_CMD_REG);
> +
> +       /* Wait for Rx FIFO to have something */
> +       ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
> +       if (ret) {
> +               dev_err(&ctrl->dev,
> +                       "%s:Failed to wait for RX FIFO not empty\n", __func__);
> +               return ret;
> +       }

This chunk is the same. Maybe have apple_spmi_wait_for_rx_fifo() that
does everything including the error message?

> +
> +       /* Discard SPMI reply status */
> +       read_reg(spmi, SPMI_RSP_REG);
> +
> +       /* Read SPMI data reply */
> +       while (len_to_read < bc) {
> +               rsp = read_reg(spmi, SPMI_RSP_REG);
> +               i = 0;
> +               while ((len_to_read < bc) && (i < 4)) {
> +                       __buf[len_to_read++] = ((0xff << (8 * i)) & rsp) >>
> +                                              (8 * i);
> +                       i += 1;
> +               }

Is this ioread32_rep()?

> +       }
> +
> +       return 0;
> +}
> +
> +static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +                         u16 saddr, const u8 *__buf, size_t bc)
> +{
> +       struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> +       u32 spmi_cmd = opc | sid << 8 | saddr << 16 | (bc - 1) | (1 << 15);
> +       u32 status;
> +       size_t i = 0, j;
> +       int ret;
> +
> +       write_reg(spmi_cmd, spmi, SPMI_CMD_REG);
> +
> +       while (i < bc) {
> +               j = 0;
> +               spmi_cmd = 0;
> +               while ((j < 4) & (i < bc))
> +                       spmi_cmd |= __buf[i++] << (j++ * 8);

Is this iowrite32_rep()? Perhaps unaligned sizes have to be dealt with,
but otherwise I suspect it would be more efficient to use
iowrite32_rep() until the number of bytes is less than 4 and then do the
one extra pack.

> +
> +               write_reg(spmi_cmd, spmi, SPMI_CMD_REG);
> +       }
> +
> +       /* Wait for Rx FIFO to have something */
> +       ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
> +       if (ret) {
> +               dev_err(&ctrl->dev,
> +                       "%s:Failed to wait for RX FIFO not empty\n", __func__);
                             ^
Please put a space after  ---|

> +               return ret;
> +       }
> +
> +       /* Discard */
> +       read_reg(spmi, SPMI_RSP_REG);
> +
> +       return 0;
> +}
> +
> +static int spmi_controller_probe(struct platform_device *pdev)
> +{
> +       struct apple_spmi *spmi;
> +       struct spmi_controller *ctrl;
> +       int ret;
> +
> +       ctrl = devm_spmi_controller_alloc(&pdev->dev, sizeof(*spmi));
> +       if (IS_ERR(ctrl)) {
> +               dev_err_probe(&pdev->dev, PTR_ERR(ctrl),
> +                             "Can't allocate spmi_controller data\n");

This is likely redundant given that the spmi core API prints errors. I
could see a patch that moves to dev_err_probe() there.

> +               return -ENOMEM;
> +       }
> +
> +       spmi = spmi_controller_get_drvdata(ctrl);
> +
> +       spmi->regs = devm_platform_ioremap_resource(pdev, 0);

This already prints an error message so the dev_err_probe() later is
redundant. Please remove.

> +       if (IS_ERR(spmi->regs)) {
> +               dev_err_probe(&pdev->dev, PTR_ERR(spmi->regs),
> +                             "Can't get ioremap regs\n");
> +               return PTR_ERR(spmi->regs);
> +       }
> +
> +       ctrl->dev.of_node = of_node_get(pdev->dev.of_node);

Drop the of_node_get(), especially because it never gets put.

> +
> +       ctrl->read_cmd = spmi_read_cmd;
> +       ctrl->write_cmd = spmi_write_cmd;
> +
> +       ret = devm_spmi_controller_add(&pdev->dev, ctrl);
> +       if (ret) {
> +               dev_err(&pdev->dev,
> +                       "spmi_controller_add failed with error %d!\n", ret);

Use 'return dev_err_probe()'?

> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id spmi_controller_match_table[] = {
> +       { .compatible = "apple,spmi", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, spmi_controller_match_table);
> +
> +static struct platform_driver spmi_controller_driver = {

How about apple_spmi_driver?

> +       .probe          = spmi_controller_probe,

And apple_spmi_probe?

> +       .driver         = {
> +               .name   = "apple-spmi",
> +               .of_match_table = spmi_controller_match_table,

And apple_spmi_match_table?

> +       },
> +};
> +module_platform_driver(spmi_controller_driver);