Re: [PATCH v1 2/3] i2c: qcom-slave: Add driver for Qualcomm I2C slave controller

[Mukesh Savaliya]

On 6/28/2026 8:09 PM, Viken Dadhaniya wrote:
> Add support for the dedicated Qualcomm I2C slave controller found on
> QDU1000 and related SoCs. This IP block operates only in slave mode and is
> separate from the existing Qualcomm I2C master controllers, so those
> drivers cannot support systems that need the SoC to respond as an I2C or
> SMBus target.
>
> Register the controller as an SMBus adapter and support byte, byte-data,
> word-data and block-data transfers through the standard /dev/i2c-X
> interface. Handle the controller IRQ events for RX and TX FIFO service,
> STOP and repeated-start conditions, clock stretching, and error recovery.
> Enable the required AHB and XO clocks, vote for interconnect bandwidth, and
> restore the hardware state across suspend and resume.
>
> Read the initial slave address from the qcom,slave-addr device tree
> property. The controller node already uses reg for its MMIO resource, and
> the slave address is programmable, including through the SMBus ioctl
> interface.
>
> Signed-off-by: Viken Dadhaniya <viken.dadhaniya@xxxxxxxxxxxxxxxx>

[...]
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1070,6 +1070,20 @@ config I2C_QCOM_GENI
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called i2c-qcom-geni.
>   
> +config I2C_QCOM_SLAVE
> +	tristate "Qualcomm I2C slave controller"
slave -> Target
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on COMMON_CLK
> +	depends on INTERCONNECT
> +	help
> +	  This driver supports I2C slave mode on Qualcomm Technologies
> +	  SoCs. If you say yes to this option, support will be included
> +	  for the built-in I2C slave controller on QDU1000 and other
> +	  compatible Qualcomm SoCs.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-qcom-slave.
> +

[..]

> +
> +/* I2C_S_CONFIG register fields */
> +#define CORE_EN					BIT(0)
Just CORE_EN ? sounds very generic, any prefix ?
> +
> +/* I2C_S_CONTROL register fields */
> +#define CLEAR_RX_FIFO				BIT(0)
> +#define CLEAR_TX_FIFO				BIT(1)
> +#define NACK					BIT(2)
> +#define ACK_RESUME				BIT(3)
> +
> +/* I2C_S_SW_RESET_REG register fields */
> +#define SW_RESET				BIT(0)
> +
> +/* I2C_S_FIFOS_STATUS register fields */
> +#define TX_FIFO_COUNT_MASK			GENMASK(15, 0)
> +#define RX_FIFO_COUNT_MASK			GENMASK(31, 16)
> +
> +/* Enabled IRQ bits: 0-6 and 8-9 (bit 7 GCA and bits 10-11 SMBAlert not used) */
Define BIT(7) with some naming, so we don't need this comment.
> +#define QCOM_I2C_SLAVE_ALL_IRQ			(GENMASK(9, 0) & ~BIT(7))
> +
> +#define I2C_SLAVE_MAX_MSG_SIZE			32
> +#define I2C_SLAVE_BYTE_DATA			1
> +#define I2C_SLAVE_WORD_DATA			2
> +
> +/* Interconnect bandwidth vote in bytes per second */
Not sure what's prferred unit, but if not mentioned it would be in BPS ?
> +#define APPS_PROC_TO_I2C_SLAVE_VOTE		1190000
> +
> +/**
> + * enum qcom_i2c_slave_irq - IRQ bit positions in I2C_S_IRQ_STATUS
> + * @STOP_DETECTED:	I2C stop condition detected on the bus
> + * @RX_FIFO_FULL:	receive FIFO has reached capacity
> + * @TX_FIFO_EMPTY:	transmit FIFO is empty
> + * @RX_DATA_AVAIL:	receive data is available in the RX FIFO
> + * @CLOCK_LOW_TIMEOUT:	SCL held low longer than the configured timeout
> + * @STRCH_WR:		clock stretching during a write (Rx) phase
> + * @STRCH_RD:		clock stretching during a read (Tx) phase
> + * @ERR_CONDITION:	unexpected start or stop bit detected (error)
> + * @RESTART_DETECTED:	repeated start condition detected
> + */
> +enum qcom_i2c_slave_irq {
> +	STOP_DETECTED = 0,
> +	RX_FIFO_FULL,
> +	TX_FIFO_EMPTY,
> +	RX_DATA_AVAIL,
> +	CLOCK_LOW_TIMEOUT,
> +	STRCH_WR,
> +	STRCH_RD,
Can add GSA_DETECTED ? even though unsued. We don't need below comment
> +	ERR_CONDITION = 8, /* bit 7 (GCA_DETECTED) not used */
> +	RESTART_DETECTED,
> +};
> +
> +static const char *const qcom_i2c_slave_irq_names[] = {
> +	[STOP_DETECTED]		= "Stop bit detected",
> +	[RX_FIFO_FULL]		= "Rx FIFO full",
> +	[TX_FIFO_EMPTY]		= "Tx FIFO empty",
> +	[RX_DATA_AVAIL]		= "Rx data available",
> +	[CLOCK_LOW_TIMEOUT]	= "Clock low timeout",
> +	[STRCH_WR]		= "Clock stretching during write (Rx) phase",
> +	[STRCH_RD]		= "Clock stretching during read (Tx) phase",
> +	[ERR_CONDITION]		= "Error condition: unexpected Start/Stop bits",
> +	[RESTART_DETECTED]	= "Repeated start bit detected",
> +};

[...]

> +/**
> + * qcom_i2c_slave_interrupt - top-level interrupt handler
> + * @irq:	interrupt number
> + * @dev_id:	pointer to the controller private data
> + *
> + * Reads the IRQ status register and dispatches handling for each active
> + * interrupt source. Fatal conditions (ERR_CONDITION, CLOCK_LOW_TIMEOUT)
> + * trigger a full controller reset and return early. All other events are
> + * handled in order with the spinlock held.
> + *
> + * Return: %IRQ_HANDLED if at least one interrupt was processed, %IRQ_NONE
> + *         if the status register was empty.
> + */
> +static irqreturn_t qcom_i2c_slave_interrupt(int irq, void *dev_id)
> +{
> +	struct qcom_i2c_slave *slave = dev_id;
> +	u32 irq_stat;
> +
> +	irq_stat = readl_relaxed(slave->base + I2C_S_IRQ_STATUS);
> +	if (!irq_stat)
> +		return IRQ_NONE;
> +
> +	dev_dbg(slave->dev, "IRQ status: 0x%x\n", irq_stat);
> +
> +	/*
> +	 * ERR_CONDITION and CLOCK_LOW_TIMEOUT require full recovery.
minor- requires
> +	 * Return early after handling to avoid processing stale irq_stat bits.
> +	 */
> +	if (irq_stat & (BIT(ERR_CONDITION) | BIT(CLOCK_LOW_TIMEOUT))) {
> +		enum qcom_i2c_slave_irq irq_type = (irq_stat & BIT(ERR_CONDITION)) ?
> +						    ERR_CONDITION : CLOCK_LOW_TIMEOUT;
> +		dev_err(slave->dev, "%s\n", qcom_i2c_slave_irq_names[irq_type]);
> +		qcom_i2c_slave_dump_regs(slave);
> +		qcom_i2c_slave_set_bits(slave, I2C_S_SW_RESET_REG, SW_RESET);
> +		qcom_i2c_slave_clear_irq(slave, QCOM_I2C_SLAVE_ALL_IRQ);
> +		writel(QCOM_I2C_SLAVE_ALL_IRQ, slave->base + I2C_S_IRQ_EN);
> +		qcom_i2c_slave_set_bits(slave, I2C_S_CONTROL,
> +					CLEAR_TX_FIFO | CLEAR_RX_FIFO);
> +		qcom_i2c_slave_set_bits(slave, I2C_S_CONFIG, CORE_EN);
> +		writel(NACK, slave->base + I2C_S_CONTROL);
add a line space before return
> +		return IRQ_HANDLED;
> +	}
> +
> +	spin_lock(&slave->lock);
> +
> +	if (irq_stat & BIT(STOP_DETECTED)) {
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[STOP_DETECTED]);
> +		qcom_i2c_slave_read_fifo(slave);
> +		qcom_i2c_slave_clear_irq(slave, BIT(STOP_DETECTED));
> +	}
> +
> +	if (irq_stat & BIT(RX_FIFO_FULL)) {
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[RX_FIFO_FULL]);
> +		writel(NACK, slave->base + I2C_S_CONTROL);
> +		qcom_i2c_slave_clear_irq(slave, BIT(RX_FIFO_FULL));
> +	}
> +
> +	if (irq_stat & BIT(STRCH_RD)) {
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[STRCH_RD]);
> +		if (readl_relaxed(slave->base + I2C_S_FIFOS_STATUS) & TX_FIFO_COUNT_MASK)
> +			writel(ACK_RESUME, slave->base + I2C_S_CONTROL);
> +		else
> +			writel(NACK, slave->base + I2C_S_CONTROL);
> +		qcom_i2c_slave_clear_irq(slave, BIT(STRCH_RD));
> +	}
> +
> +	if (irq_stat & BIT(RX_DATA_AVAIL)) {
> +		/*
> +		 * Intermediate notification only — received data is consumed
> +		 * in the STOP_DETECTED handler. Acknowledge and clear.
> +		 */
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[RX_DATA_AVAIL]);
> +		qcom_i2c_slave_clear_irq(slave, BIT(RX_DATA_AVAIL));
> +	}
> +
> +	if (irq_stat & BIT(STRCH_WR)) {
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[STRCH_WR]);
> +		if (slave->rx_count < I2C_SLAVE_MAX_MSG_SIZE)
> +			writel(ACK_RESUME, slave->base + I2C_S_CONTROL);
> +		else
> +			writel(NACK, slave->base + I2C_S_CONTROL);
> +		qcom_i2c_slave_clear_irq(slave, BIT(STRCH_WR));
> +	}
> +
> +	if (irq_stat & BIT(TX_FIFO_EMPTY)) {
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[TX_FIFO_EMPTY]);
> +		if (slave->tx_count)
> +			qcom_i2c_slave_write_fifo(slave);
> +		qcom_i2c_slave_clear_irq(slave, BIT(TX_FIFO_EMPTY));
> +	}
> +
> +	if (irq_stat & BIT(RESTART_DETECTED)) {
> +		dev_dbg(slave->dev, "%s\n", qcom_i2c_slave_irq_names[RESTART_DETECTED]);
> +		writel(ACK_RESUME, slave->base + I2C_S_CONTROL);
> +		qcom_i2c_slave_clear_irq(slave, BIT(RESTART_DETECTED));
> +	}
> +
> +	spin_unlock(&slave->lock);
> +
> +	return IRQ_HANDLED;
> +}

[...]

> +/**
> + * qcom_i2c_slave_probe - probe the Qualcomm I2C slave controller
> + * @pdev:	platform device
> + *
> + * Allocates driver state, maps registers, enables clocks and the
> + * interconnect path, registers the interrupt handler, initialises the
> + * hardware, and registers the I2C adapter with the kernel.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int qcom_i2c_slave_probe(struct platform_device *pdev)
> +{
> +	struct qcom_i2c_slave *slave;
> +	struct device *dev = &pdev->dev;
> +	u32 addr;
> +	int ret;
> +
> +	slave = devm_kzalloc(dev, sizeof(*slave), GFP_KERNEL);
> +	if (!slave)
> +		return -ENOMEM;
> +
> +	slave->dev = dev;
> +	spin_lock_init(&slave->lock);
> +
> +	ret = of_property_read_u32(dev->of_node, "qcom,slave-addr", &addr);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "missing qcom,slave-addr property\n");
> +
> +	slave->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(slave->base))
> +		return PTR_ERR(slave->base);
> +
> +	slave->xo_clk = devm_clk_get_enabled(dev, "sm_bus_xo_clk");
> +	if (IS_ERR(slave->xo_clk))
> +		return dev_err_probe(dev, PTR_ERR(slave->xo_clk),
> +				     "failed to get and enable XO clock\n");
> +
> +	slave->ahb_clk = devm_clk_get_enabled(dev, "sm_bus_ahb_clk");
> +	if (IS_ERR(slave->ahb_clk))
> +		return dev_err_probe(dev, PTR_ERR(slave->ahb_clk),
> +				     "failed to get and enable AHB clock\n");
> +
> +	slave->irq = platform_get_irq(pdev, 0);
> +	if (slave->irq < 0)
> +		return slave->irq;
> +
> +	ret = devm_request_irq(dev, slave->irq, qcom_i2c_slave_interrupt, 0,
> +			       dev_name(dev), slave);
> +	if (ret) {
> +		dev_err(dev, "request_irq failed for IRQ %d: %d\n",
> +			slave->irq, ret);
dev_err_probe
> +		return ret;
> +	}
> +
> +	ret = qcom_i2c_slave_icc_init(slave);
> +	if (ret)
> +		return ret;
> +
> +	slave->slave_addr = addr;
> +
> +	qcom_i2c_slave_hw_init(slave);
> +
> +	slave->adap.owner = THIS_MODULE;
> +	slave->adap.algo = &qcom_i2c_slave_algo;
> +	slave->adap.dev.parent = dev;
> +	slave->adap.dev.of_node = dev->of_node;
> +	strscpy(slave->adap.name, "qcom-i2c-slave", sizeof(slave->adap.name));
> +
> +	i2c_set_adapdata(&slave->adap, slave);
> +	platform_set_drvdata(pdev, slave);
> +
> +	ret = i2c_add_adapter(&slave->adap);
> +	if (ret) {
> +		dev_err(dev, "i2c_add_adapter failed: %d\n", ret);
dev_err_probe
> +		icc_disable(slave->icc_path);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "Qualcomm I2C slave probed at address 0x%x\n", addr);
> +	return 0;
> +}
> +
> +/**
> + * qcom_i2c_slave_remove - remove the Qualcomm I2C slave controller
> + * @pdev:	platform device
> + *
> + * Unregisters the I2C adapter and disables the interconnect path.
> + * Controller clocks are disabled automatically by the devm framework.
> + */
> +static void qcom_i2c_slave_remove(struct platform_device *pdev)
> +{
> +	struct qcom_i2c_slave *slave = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&slave->adap);
> +	icc_disable(slave->icc_path);
> +	/* clocks are disabled automatically by devm */
> +}
> +
> +/**
> + * qcom_i2c_slave_suspend - suspend the controller
> + * @dev:	device associated with the controller
> + *
> + * Disables the interrupt, releases the interconnect bandwidth vote, and
> + * disables the controller clocks to allow the system to enter a low-power
> + * state.
> + *
> + * Return: 0 always.
> + */
> +static int qcom_i2c_slave_suspend(struct device *dev)
> +{
> +	struct qcom_i2c_slave *slave = dev_get_drvdata(dev);
> +
> +	disable_irq(slave->irq);
> +	icc_disable(slave->icc_path);
> +	clk_disable_unprepare(slave->xo_clk);
> +	clk_disable_unprepare(slave->ahb_clk);
> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_i2c_slave_resume - resume the controller
> + * @dev:	device associated with the controller
> + *
> + * Re-enables the controller clocks and the interconnect bandwidth path,
> + * restores the hardware register state, then re-enables the interrupt so
> + * the controller is ready to handle transactions.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int qcom_i2c_slave_resume(struct device *dev)
> +{
> +	struct qcom_i2c_slave *slave = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(slave->ahb_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable AHB clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(slave->xo_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable XO clock: %d\n", ret);
> +		clk_disable_unprepare(slave->ahb_clk);
> +		return ret;
> +	}
> +
> +	ret = icc_enable(slave->icc_path);
> +	if (ret) {
> +		dev_err(dev, "ICC enable failed: %d\n", ret);
> +		clk_disable_unprepare(slave->xo_clk);
> +		clk_disable_unprepare(slave->ahb_clk);
> +		return ret;
> +	}
> +
> +	qcom_i2c_slave_hw_init(slave);
> +	enable_irq(slave->irq);
line space before return
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(qcom_i2c_slave_pm_ops,
> +			  qcom_i2c_slave_suspend,
> +			  qcom_i2c_slave_resume);
> +
> +static const struct of_device_id qcom_i2c_slave_dt_match[] = {
> +	{ .compatible = "qcom,i2c-slave" },
let's add something with verion or target.
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_i2c_slave_dt_match);
> +
> +static struct platform_driver qcom_i2c_slave_driver = {
> +	.driver = {
> +		.name		= "qcom-i2c-slave",
> +		.pm		= &qcom_i2c_slave_pm_ops,
> +		.of_match_table	= qcom_i2c_slave_dt_match,
> +	},
> +	.probe	= qcom_i2c_slave_probe,
> +	.remove	= qcom_i2c_slave_remove,
> +};
> +module_platform_driver(qcom_i2c_slave_driver);
> +
> +MODULE_AUTHOR("Viken Dadhaniya <viken.dadhaniya@xxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Qualcomm I2C slave controller driver");