Re: [PATCH v3 2/2] i2c: qcom-target: Add driver for Qualcomm I2C target controller

[Viken Dadhaniya]

On 8/26/2026 7:27 PM, Andi Shyti wrote:
> Hi Viken,
> 
> just a quick look here.
> 
> ...
> 
>> +static void qcom_i2c_target_hw_reset(struct qcom_i2c_target *target)
>> +{
>> +	/* Clear error bits before SW_RESET; the reset may not be instantaneous */
>> +	writel(BIT(ERR_CONDITION) | BIT(CLOCK_LOW_TIMEOUT),
>> +	       target->base + I2C_S_IRQ_CLR);
>> +	writel(SW_RESET, target->base + I2C_S_SW_RESET_REG);
>> +	/*
>> +	 * I2C_S_SW_RESET_REG is write-only so completion cannot be polled.
>> +	 * Use a conservative delay to allow the reset to finish before
>> +	 * reconfiguring the controller.
>> +	 */
>> +	usleep_range(10, 20);
> 
> This is also called in atomic context from the irq handler.
> Please avoid using usleep_range(), perhaps you can put this in a
> thread.
> 

Thanks for the review. The delay is only 20 µs, so we were
thinking of replacing usleep_range() with udelay() instead of
converting to a threaded IRQ, to avoid the added complexity on
all the normal fast paths.

>> +	qcom_i2c_target_hw_init(target);
>> +	writel(target->slave->addr, target->base + I2C_S_DEVICE_ADDR);
>> +	writel(I2C_S_CORE_EN, target->base + I2C_S_CONFIG);
>> +}
>> +
>> +static irqreturn_t qcom_i2c_target_handle_error(struct qcom_i2c_target *target,
>> +						u32 irq_stat)
>> +{
>> +	u8 val = 0;
>> +
>> +	if (irq_stat & BIT(ERR_CONDITION))
>> +		dev_err(target->dev, "Error condition: unexpected Start/Stop bits\n");
>> +	else
>> +		dev_err(target->dev, "Clock low timeout\n");
>> +	qcom_i2c_target_dump_regs(target);
>> +	qcom_i2c_target_hw_reset(target);
>> +	i2c_slave_event(target->slave, I2C_SLAVE_STOP, &val);
>> +	target->status = 0;
>> +	return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +static irqreturn_t qcom_i2c_target_irq(int irq, void *dev)
>> +{
>> +	struct qcom_i2c_target *target = dev;
>> +	u32 irq_stat, rx_bits;
>> +
>> +	/*
>> +	 * Dispatch priority (highest first):
>> +	 *   ERR_CONDITION / CLOCK_LOW_TIMEOUT  — hardware error, triggers SW reset
>> +	 *   STOP_DETECTED                      — end of transaction, clears all state
>> +	 *   RESTART_DETECTED                   — repeated start, resets state before
>> +	 *                                        any data phase in the same snapshot
>> +	 *   STRCH_RD                           — read-phase data supply
>> +	 *   RX_FIFO_FULL / RX_DATA_AVAIL /
>> +	 *   STRCH_WR                           — write-phase Rx, coalesced into one drain
>> +	 */
>> +	irq_stat = readl_relaxed(target->base + I2C_S_IRQ_STATUS);
>> +	if (!irq_stat)
>> +		return IRQ_NONE;
>> +
>> +	dev_dbg(target->dev, "IRQ status: 0x%x\n", irq_stat);
>> +
>> +	/*
>> +	 * Load target->slave once. Both reg_slave() and unreg_slave() disable
>> +	 * the IRQ before writing the pointer, so it cannot change while this
>> +	 * handler runs. Sub-handlers may dereference target->slave directly.
>> +	 *
>> +	 * The core is enabled only in reg_slave() and disabled in unreg_slave(),
>> +	 * so no bus activity is expected here. Clear and discard any stale IRQ.
>> +	 */
>> +	if (!READ_ONCE(target->slave)) {
>> +		writel(irq_stat, target->base + I2C_S_IRQ_CLR);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (irq_stat & (BIT(ERR_CONDITION) | BIT(CLOCK_LOW_TIMEOUT)))
>> +		return qcom_i2c_target_handle_error(target, irq_stat);
>> +
>> +	if (irq_stat & BIT(STOP_DETECTED))
>> +		return qcom_i2c_target_handle_stop(target, irq_stat);
> 
> ...
> 
>> +	target->xo_clk = devm_clk_get(dev, "xo");
>> +	if (IS_ERR(target->xo_clk))
>> +		return dev_err_probe(dev, PTR_ERR(target->xo_clk),
>> +				     "failed to get XO clock\n");
>> +
>> +	target->ahb_clk = devm_clk_get(dev, "ahb");
>> +	if (IS_ERR(target->ahb_clk))
>> +		return dev_err_probe(dev, PTR_ERR(target->ahb_clk),
>> +				     "failed to get AHB clock\n");
>> +
>> +	ret = clk_prepare_enable(target->xo_clk);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to enable XO clock\n");
>> +
>> +	ret = clk_prepare_enable(target->ahb_clk);
>> +	if (ret) {
>> +		clk_disable_unprepare(target->xo_clk);
>> +		return dev_err_probe(dev, ret, "failed to enable AHB clock\n");
>> +	}
> 
> The problem is that these clocks are not disabled in the returns
> below.

Sure, I will add the missing clock cleanup paths in the next revision.
> 
> Thanks,
> Andi
> 
>> +
>> +	target->irq = platform_get_irq(pdev, 0);
>> +	if (target->irq < 0)
>> +		return target->irq;
>> +
>> +	ret = qcom_i2c_target_icc_init(target);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_request_irq(dev, target->irq, qcom_i2c_target_irq, 0,
>> +			       dev_name(dev), target);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "request_irq failed for IRQ %d\n",
>> +				     target->irq);
>> +
>> +	qcom_i2c_target_hw_init(target);
>> +
>> +	platform_set_drvdata(pdev, target);
>> +
>> +	ret = qcom_i2c_target_adap_init(target);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "i2c_add_adapter failed\n");
>> +
>> +	return 0;
>> +}