Hi Kartik,

On Thu, Aug 28, 2025 at 11:29:32AM +0530, Kartik Rajput wrote:

Add support for SW mutex register introduced in Tegra264 to provide
an option to share the interface between multiple firmwares and/or
VMs.

You could add a short description on how to use the mutex
register here.

However, the hardware does not ensure any protection based on the
values. The driver/firmware should honor the peer who already holds
the mutex.

Signed-off-by: Kartik Rajput <redacted>
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>

...

this is a bool function.

+{
+	unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
+	u32 val, id;
+
+	val = readl(i2c_dev->base + reg);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+
+	if (id != I2C_SW_MUTEX_ID_CCPLEX)
+		return 0;
+
+	return 1;

return id != I2C_SW_MUTEX_ID_CCPLEX;

+}
+
+static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev *i2c_dev)

I think this can be bool.

+{
+	unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
+	u32 val, id;
+
+	val = readl(i2c_dev->base + reg);
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+	if (id != 0 && id != I2C_SW_MUTEX_ID_CCPLEX)
+		return 0;
+
+	val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID_CCPLEX);
+	writel(val, i2c_dev->base + reg);
+
+	return tegra_i2c_mutex_acquired(i2c_dev);
+}
+
+static int tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
+{
+	int locked;

I guess this can be bool.

+	int ret;
+
+	if (i2c_dev->atomic_mode)
+		ret = read_poll_timeout_atomic(tegra_i2c_mutex_trylock, locked, locked,
+					       USEC_PER_MSEC, I2C_SW_MUTEX_TIMEOUT_US,
+					       false, i2c_dev);
+	else
+		ret = read_poll_timeout(tegra_i2c_mutex_trylock, locked, locked, USEC_PER_MSEC,
+					I2C_SW_MUTEX_TIMEOUT_US, false, i2c_dev);
+
+	if (!tegra_i2c_mutex_acquired(i2c_dev))
+		dev_warn(i2c_dev->dev, "failed to acquire mutex\n");

I would try a few times before giving up.

Besides, is there a chance where ret is '0' and the mutex is not
acquired? If so, we are not signalling error if the mutex is not
acquired, but I think we should.

I would do:

	if (...)
		ret = ...
	else
		ret = ...
	
	if (ret)
		return ret;

	if (!tegra_i2c_mutex_acquired(i2c_dev)) {
		dev_warn(i2c_dev->dev, "failed to acquire mutex\n");
		return -ESOMETHING;
	}

	return 0;

Makes sense?

+
+	return ret;
+}
+
+static int tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned int reg = tegra_i2c_reg_addr(i2c_dev, I2C_SW_MUTEX);
+	u32 val, id;
+
+	val = readl(i2c_dev->base + reg);
+
+	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
+	if (id && id != I2C_SW_MUTEX_ID_CCPLEX) {
+		dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
+		return -EPERM;

I would try a few times before giving up.

no need for this extra blank line.

+	if (i2c_dev->hw->has_mutex) {

I would put this check in tegra_i2c_mutex_lock() and _unlock() in
order to avoid two level indentation here.

We are skipping pm_runtime_put(), though.

Thanks,
Andi

+	}
+
 	pm_runtime_put(i2c_dev->dev);
 
 	return ret ?: i;