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);
