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

[Andi Shyti <andi.shyti@kernel.org>]

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.

> +	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.

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;
> +}