[PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Akhil R]

The acpi_evaluate_object() returns an ACPI error code and not the
Linux one. For the some platforms the error will have positive code
which may be interpreted incorrectly. Use ACPI_FAILURE() to determine
the failure and return the error. Also move the reset to a separate
function to handle this better.

Fixes: bd2fdedbf2ba ("i2c: tegra: Add the ACPI support")
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
New patch suggested in v4.

 drivers/i2c/busses/i2c-tegra.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 049b4d154c23..6f3d770c5a67 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -604,10 +604,25 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
+static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
+{
+	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
+	int err;
+
+	if (handle) {
+		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
+		if (ACPI_FAILURE(err))
+			return -EIO;
+
+		return 0;
+	}
+
+	return reset_control_reset(i2c_dev->rst);
+}
+
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
-	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
 	struct i2c_timings *t = &i2c_dev->timings;
 	int err;
 
@@ -619,11 +634,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	 * emit a noisy warning on error, which won't stay unnoticed and
 	 * won't hose machine entirely.
 	 */
-	if (handle)
-		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
-	else
-		err = reset_control_reset(i2c_dev->rst);
-
+	err = tegra_i2c_reset(i2c_dev);
 	WARN_ON_ONCE(err);
 
 	if (IS_DVC(i2c_dev))
-- 
2.49.0

[PATCH v5 2/3] i2c: tegra: make reset an optional property

[Akhil R]

For controllers that has an internal software reset, make the reset
property optional. This provides an option to use Tegra I2C in systems
that choose to restrict reset control from Linux or not to implement
the ACPI _RST method.

Internal reset was not required when the reset control was mandatory.
But on platforms where the resets are outside the control of Linux,
this had to be implemented by just returning success from BPMP or with
an empty _RST method in the ACPI table, basically ignoring the reset.

While the internal reset is not identical to the hard reset of the
controller, this will reset all the internal state of the controller
including FIFOs. This may slightly alter the behaviour in systems
which were ignoring the reset but it should not cause any functional
difference since all the required I2C registers are configured after
this reset, just as in boot. Considering that this sequence is hit
during the boot or during the I2C recovery path from an error, the
internal reset provides a better alternative than just ignoring the
reset.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
v4->v5:
  * Added check for ACPI _RST method.
  * Updated commit message.
v2->v4: No change
v1->v2:
  * Call devm_reset_control_get_optional_exclusive() unconditionally.

 drivers/i2c/busses/i2c-tegra.c | 35 +++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 6f3d770c5a67..b24882b76c6d 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -134,6 +134,8 @@
 #define I2C_MST_FIFO_STATUS_TX			GENMASK(23, 16)
 #define I2C_MST_FIFO_STATUS_RX			GENMASK(7, 0)
 
+#define I2C_MASTER_RESET_CNTRL			0x0a8
+
 /* configuration load timeout in microseconds */
 #define I2C_CONFIG_LOAD_TIMEOUT			1000000
 
@@ -184,6 +186,9 @@ enum msg_end_type {
  * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
  *		provides additional features and allows for longer messages to
  *		be transferred in one go.
+ * @has_mst_reset: The I2C controller contains MASTER_RESET_CTRL register which
+ *		provides an alternative to controller reset when configured as
+ *		I2C master
  * @quirks: I2C adapter quirks for limiting write/read transfer size and not
  *		allowing 0 length transfers.
  * @supports_bus_clear: Bus Clear support to recover from bus hang during
@@ -213,6 +218,7 @@ struct tegra_i2c_hw_feature {
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
+	bool has_mst_reset;
 	const struct i2c_adapter_quirks *quirks;
 	bool supports_bus_clear;
 	bool has_apb_dma;
@@ -603,13 +609,27 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 
 	return 0;
 }
+
+static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
+{
+	if (!i2c_dev->hw->has_mst_reset)
+		return -EOPNOTSUPP;
+
+	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);
+	udelay(2);
+
+	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);
+	udelay(2);
+
+	return 0;
+}
 
 static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
 {
 	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
 	int err;
 
-	if (handle) {
+	if (handle && acpi_has_method(handle, "_RST")) {
 		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
 		if (ACPI_FAILURE(err))
 			return -EIO;
@@ -617,7 +637,10 @@ static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
 		return 0;
 	}
 
-	return reset_control_reset(i2c_dev->rst);
+	if (i2c_dev->rst)
+		return reset_control_reset(i2c_dev->rst);
+
+	return tegra_i2c_master_reset(i2c_dev);
 }
 
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
@@ -1483,6 +1506,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = false,
 	.has_apb_dma = true,
@@ -1507,6 +1531,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = false,
 	.has_apb_dma = true,
@@ -1531,6 +1556,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = true,
@@ -1555,6 +1581,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = true,
@@ -1579,6 +1606,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = true,
@@ -1603,6 +1631,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.has_mst_reset = false,
 	.quirks = &tegra_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = false,
@@ -1627,6 +1656,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
+	.has_mst_reset = true,
 	.quirks = &tegra194_i2c_quirks,
 	.supports_bus_clear = true,
 	.has_apb_dma = false,
@@ -1682,7 +1712,7 @@ static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev)
 	if (ACPI_HANDLE(i2c_dev->dev))
 		return 0;
 
-	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
+	i2c_dev->rst = devm_reset_control_get_optional_exclusive(i2c_dev->dev, "i2c");
 	if (IS_ERR(i2c_dev->rst))
 		return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
 				      "failed to get reset control\n");
-- 
2.49.0

[PATCH v5 3/3] i2c: tegra: Remove dma_sync_*() calls

[Akhil R]

Calling dma_sync_*() on a buffer from dma_alloc_coherent() is pointless.
The driver should not be doing its own bounce-buffering if the buffer is
allocated through dma_alloc_coherent()

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
Reviewed-by: Thierry Reding <treding@nvidia.com>
---
v1->v5: No change.

 drivers/i2c/busses/i2c-tegra.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b24882b76c6d..4f1925f74655 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1303,17 +1303,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	if (i2c_dev->dma_mode) {
 		if (i2c_dev->msg_read) {
-			dma_sync_single_for_device(i2c_dev->dma_dev,
-						   i2c_dev->dma_phys,
-						   xfer_size, DMA_FROM_DEVICE);
-
 			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
 			if (err)
 				return err;
-		} else {
-			dma_sync_single_for_cpu(i2c_dev->dma_dev,
-						i2c_dev->dma_phys,
-						xfer_size, DMA_TO_DEVICE);
 		}
 	}
 
@@ -1323,11 +1315,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		if (i2c_dev->dma_mode) {
 			memcpy(i2c_dev->dma_buf + I2C_PACKET_HEADER_SIZE,
 			       msg->buf, i2c_dev->msg_len);
-
-			dma_sync_single_for_device(i2c_dev->dma_dev,
-						   i2c_dev->dma_phys,
-						   xfer_size, DMA_TO_DEVICE);
-
 			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
 			if (err)
 				return err;
@@ -1368,13 +1355,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 			return -ETIMEDOUT;
 		}
 
-		if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE) {
-			dma_sync_single_for_cpu(i2c_dev->dma_dev,
-						i2c_dev->dma_phys,
-						xfer_size, DMA_FROM_DEVICE);
-
+		if (i2c_dev->msg_read && i2c_dev->msg_err == I2C_ERR_NONE)
 			memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf, i2c_dev->msg_len);
-		}
 	}
 
 	time_left = tegra_i2c_wait_completion(i2c_dev, &i2c_dev->msg_complete,
-- 
2.49.0

Re: [PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 07:04:47PM +0530, Akhil R wrote:
> The acpi_evaluate_object() returns an ACPI error code and not the
> Linux one. For the some platforms the error will have positive code
> which may be interpreted incorrectly. Use ACPI_FAILURE() to determine
> the failure and return the error. Also move the reset to a separate
> function to handle this better.

...

> +static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
> +{
> +	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
> +	int err;
> +
> +	if (handle) {
> +		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> +		if (ACPI_FAILURE(err))
> +			return -EIO;
> +
> +		return 0;
> +	}
> +
> +	return reset_control_reset(i2c_dev->rst);


It's better to be written other way around:

	acpi_handle handle;
	int err;

	handle = ACPI_HANDLE(i2c_dev->dev);
	if (!handle)
		return reset_control_reset(i2c_dev->rst);

	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
	if (ACPI_FAILURE(err))
		return -EIO;

	return 0;

> +}

Other than that, LGTM,

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> +	err = tegra_i2c_reset(i2c_dev);
>  	WARN_ON_ONCE(err);

Suggestion to improve in a separate change in the future:

Add a comment explaining why we WARN() here. I.o.w. why this condition
is so critical that we need to WARN().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/3] i2c: tegra: make reset an optional property

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 07:04:48PM +0530, Akhil R wrote:
> For controllers that has an internal software reset, make the reset
> property optional. This provides an option to use Tegra I2C in systems
> that choose to restrict reset control from Linux or not to implement
> the ACPI _RST method.
> 
> Internal reset was not required when the reset control was mandatory.
> But on platforms where the resets are outside the control of Linux,
> this had to be implemented by just returning success from BPMP or with
> an empty _RST method in the ACPI table, basically ignoring the reset.
> 
> While the internal reset is not identical to the hard reset of the
> controller, this will reset all the internal state of the controller
> including FIFOs. This may slightly alter the behaviour in systems
> which were ignoring the reset but it should not cause any functional
> difference since all the required I2C registers are configured after
> this reset, just as in boot. Considering that this sequence is hit
> during the boot or during the I2C recovery path from an error, the
> internal reset provides a better alternative than just ignoring the
> reset.

...

> +static int tegra_i2c_master_reset(struct tegra_i2c_dev *i2c_dev)
> +{
> +	if (!i2c_dev->hw->has_mst_reset)
> +		return -EOPNOTSUPP;
> +
> +	i2c_writel(i2c_dev, 0x1, I2C_MASTER_RESET_CNTRL);

> +	udelay(2);

fsleep()
and perhaps explain why this is needed.

> +	i2c_writel(i2c_dev, 0x0, I2C_MASTER_RESET_CNTRL);

> +	udelay(2);

Ditto.

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 06:04:44PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 02, 2025 at 07:04:47PM +0530, Akhil R wrote:

...

> > +static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
> > +	int err;
> > +
> > +	if (handle) {
> > +		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> > +		if (ACPI_FAILURE(err))
> > +			return -EIO;
> > +
> > +		return 0;
> > +	}
> > +
> > +	return reset_control_reset(i2c_dev->rst);
> 
> It's better to be written other way around:
> 
> 	acpi_handle handle;
> 	int err;
> 
> 	handle = ACPI_HANDLE(i2c_dev->dev);
> 	if (!handle)
> 		return reset_control_reset(i2c_dev->rst);
> 
> 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> 	if (ACPI_FAILURE(err))
> 		return -EIO;
> 
> 	return 0;
> 
> > +}
> 
> Other than that, LGTM,
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Actually I have to withdraw the tag. The above function is repetition of
the device_reset() / device_reset_optional(). Please use that instead.

Also in the next version provide a cover letter. I use my own script [1]
that makes me sure I won't skip it.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/3] i2c: tegra: Remove dma_sync_*() calls

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 07:04:49PM +0530, Akhil R wrote:
> Calling dma_sync_*() on a buffer from dma_alloc_coherent() is pointless.
> The driver should not be doing its own bounce-buffering if the buffer is
> allocated through dma_alloc_coherent()

Missing period at the end of the sentence.

Code wise LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Akhil R]

On Wed, 2 Jul 2025 18:13:51 +0300, Andy Shevchenko wrote:
>> > +static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
>> > +{
>> > +	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
>> > +	int err;
>> > +
>> > +	if (handle) {
>> > +		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
>> > +		if (ACPI_FAILURE(err))
>> > +			return -EIO;
>> > +
>> > +		return 0;
>> > +	}
>> > +
>> > +	return reset_control_reset(i2c_dev->rst);
>> 
>> It's better to be written other way around:
>> 
>> 	acpi_handle handle;
>> 	int err;
>> 
>> 	handle = ACPI_HANDLE(i2c_dev->dev);
>> 	if (!handle)
>> 		return reset_control_reset(i2c_dev->rst);
>> 
>> 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
>> 	if (ACPI_FAILURE(err))
>> 		return -EIO;
>> 
>> 	return 0;
>> 
>> > +}
>> 
>> Other than that, LGTM,
>> 
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Actually I have to withdraw the tag. The above function is repetition of
> the device_reset() / device_reset_optional(). Please use that instead.

I did check that. But device_reset_optional() returns '0' if reset is
not available or when the reset succeeds. Then there is no option to
conditionally trigger the internal reset when the reset is not available.

Other option was to do the internal reset unconditionally. But then the
devices that do not have an internal reset will have to skip the reset
silently if the reset property is absent in the device tree (or _RST
method is absent in the ACPI table).

Though device_reset() returns error when reset is absent, it looks to
be not so straight-forward to detect from the return value that if there
is an actual error during reset or if the reset is absent.

I do agree that some part of it is redundant to device_ reset()/_optional]().
But I couldn't find a proper way to make it work for the above issue.

> Also in the next version provide a cover letter. I use my own script [1]
> that makes me sure I won't skip it.
>
>[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

Sure. Will add. Thanks for sharing.


Best Regards,
Akhil

Re: [PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Philipp Zabel]

Hi Akhil,

On Mi, 2025-07-02 at 22:40 +0530, Akhil R wrote:
> On Wed, 2 Jul 2025 18:13:51 +0300, Andy Shevchenko wrote:
> > > > +static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
> > > > +{
> > > > +	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
> > > > +	int err;
> > > > +
> > > > +	if (handle) {
> > > > +		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> > > > +		if (ACPI_FAILURE(err))
> > > > +			return -EIO;
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	return reset_control_reset(i2c_dev->rst);
> > > 
> > > It's better to be written other way around:
> > > 
> > > 	acpi_handle handle;
> > > 	int err;
> > > 
> > > 	handle = ACPI_HANDLE(i2c_dev->dev);
> > > 	if (!handle)
> > > 		return reset_control_reset(i2c_dev->rst);
> > > 
> > > 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> > > 	if (ACPI_FAILURE(err))
> > > 		return -EIO;
> > > 
> > > 	return 0;
> > > 
> > > > +}
> > > 
> > > Other than that, LGTM,
> > > 
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > Actually I have to withdraw the tag. The above function is repetition of
> > the device_reset() / device_reset_optional(). Please use that instead.
> 
> I did check that. But device_reset_optional() returns '0' if reset is
> not available or when the reset succeeds. Then there is no option to
> conditionally trigger the internal reset when the reset is not available.
> 
> Other option was to do the internal reset unconditionally. But then the
> devices that do not have an internal reset will have to skip the reset
> silently if the reset property is absent in the device tree (or _RST
> method is absent in the ACPI table).
> 
> Though device_reset() returns error when reset is absent, it looks to
> be not so straight-forward to detect from the return value that if there
> is an actual error during reset or if the reset is absent.

device_reset() should return -ENOENT if the reset is absent (as opposed
to present but somehow broken). If there is any code path where this
isn't the case, we should probably fix this.

In the ACPI case, -ENOENT is returned by __device_reset() if the "_RST"
method is not found.

In the OF case, -ENOENT is returned by __of_reset_control_get() if the
requested id can't be found in a "reset-names" property, or if
of_parse_phandle_with_args() returns -ENOENT for the "resets" (or
"reset-gpios") property - that is, when this property doesn't exist or
the entry indicated by the reset id is empty.


regards
Philipp

Re: [PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Andy Shevchenko]

On Thu, Jul 03, 2025 at 10:31:25AM +0200, Philipp Zabel wrote:
> On Mi, 2025-07-02 at 22:40 +0530, Akhil R wrote:
> > On Wed, 2 Jul 2025 18:13:51 +0300, Andy Shevchenko wrote:

...

> > > > > +static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
> > > > > +{
> > > > > +	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
> > > > > +	int err;
> > > > > +
> > > > > +	if (handle) {
> > > > > +		err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> > > > > +		if (ACPI_FAILURE(err))
> > > > > +			return -EIO;
> > > > > +
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	return reset_control_reset(i2c_dev->rst);
> > > > 
> > > > It's better to be written other way around:
> > > > 
> > > > 	acpi_handle handle;
> > > > 	int err;
> > > > 
> > > > 	handle = ACPI_HANDLE(i2c_dev->dev);
> > > > 	if (!handle)
> > > > 		return reset_control_reset(i2c_dev->rst);
> > > > 
> > > > 	err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
> > > > 	if (ACPI_FAILURE(err))
> > > > 		return -EIO;
> > > > 
> > > > 	return 0;
> > > > 
> > > > > +}
> > > > 
> > > > Other than that, LGTM,
> > > > 
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > Actually I have to withdraw the tag. The above function is repetition of
> > > the device_reset() / device_reset_optional(). Please use that instead.
> > 
> > I did check that. But device_reset_optional() returns '0' if reset is
> > not available or when the reset succeeds. Then there is no option to
> > conditionally trigger the internal reset when the reset is not available.
> > 
> > Other option was to do the internal reset unconditionally. But then the
> > devices that do not have an internal reset will have to skip the reset
> > silently if the reset property is absent in the device tree (or _RST
> > method is absent in the ACPI table).
> > 
> > Though device_reset() returns error when reset is absent, it looks to
> > be not so straight-forward to detect from the return value that if there
> > is an actual error during reset or if the reset is absent.
> 
> device_reset() should return -ENOENT if the reset is absent (as opposed
> to present but somehow broken). If there is any code path where this
> isn't the case, we should probably fix this.
> 
> In the ACPI case, -ENOENT is returned by __device_reset() if the "_RST"
> method is not found.
> 
> In the OF case, -ENOENT is returned by __of_reset_control_get() if the
> requested id can't be found in a "reset-names" property, or if
> of_parse_phandle_with_args() returns -ENOENT for the "resets" (or
> "reset-gpios") property - that is, when this property doesn't exist or
> the entry indicated by the reset id is empty.

I have nothing to add to what Philipp just said. I believe we don't want
open coded variant of the device_reset*().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/3] i2c: tegra: make reset an optional property

[kernel test robot]

Hi Akhil,

kernel test robot noticed the following build errors:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on andi-shyti/i2c/i2c-host linus/master v6.16-rc4 next-20250703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Akhil-R/i2c-tegra-make-reset-an-optional-property/20250702-214005
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
patch link:    https://lore.kernel.org/r/20250702133450.64257-2-akhilrajeev%40nvidia.com
patch subject: [PATCH v5 2/3] i2c: tegra: make reset an optional property
config: sh-randconfig-002-20250703 (https://download.01.org/0day-ci/archive/20250703/202507032359.AfHPKWEQ-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250703/202507032359.AfHPKWEQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507032359.AfHPKWEQ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/build_bug.h:5,
                    from include/linux/bits.h:22,
                    from include/linux/ioport.h:13,
                    from include/linux/acpi.h:12,
                    from drivers/i2c/busses/i2c-tegra.c:9:
   drivers/i2c/busses/i2c-tegra.c: In function 'tegra_i2c_reset':
>> drivers/i2c/busses/i2c-tegra.c:632:23: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Wimplicit-function-declaration]
     632 |         if (handle && acpi_has_method(handle, "_RST")) {
         |                       ^~~~~~~~~~~~~~~
   include/linux/compiler.h:57:52: note: in definition of macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   drivers/i2c/busses/i2c-tegra.c:632:9: note: in expansion of macro 'if'
     632 |         if (handle && acpi_has_method(handle, "_RST")) {
         |         ^~


vim +632 drivers/i2c/busses/i2c-tegra.c

   626	
   627	static int tegra_i2c_reset(struct tegra_i2c_dev *i2c_dev)
   628	{
   629		acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
   630		int err;
   631	
 > 632		if (handle && acpi_has_method(handle, "_RST")) {
   633			err = acpi_evaluate_object(handle, "_RST", NULL, NULL);
   634			if (ACPI_FAILURE(err))
   635				return -EIO;
   636	
   637			return 0;
   638		}
   639	
   640		if (i2c_dev->rst)
   641			return reset_control_reset(i2c_dev->rst);
   642	
   643		return tegra_i2c_master_reset(i2c_dev);
   644	}
   645	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 1/3] i2c: tegra: Fix reset error handling with ACPI

[Akhil R]

On Thu, 3 Jul 2025 17:11:11 +0300, Andy Shevchenko wrote:

...

>>> I did check that. But device_reset_optional() returns '0' if reset is
>>> not available or when the reset succeeds. Then there is no option to
>>> conditionally trigger the internal reset when the reset is not available.
>>> 
>>> Other option was to do the internal reset unconditionally. But then the
>>> devices that do not have an internal reset will have to skip the reset
>>> silently if the reset property is absent in the device tree (or _RST
>>> method is absent in the ACPI table).
>>> 
>>> Though device_reset() returns error when reset is absent, it looks to
>>> be not so straight-forward to detect from the return value that if there
>>> is an actual error during reset or if the reset is absent.
>> 
>> device_reset() should return -ENOENT if the reset is absent (as opposed
>> to present but somehow broken). If there is any code path where this
>> isn't the case, we should probably fix this.
>> 
>> In the ACPI case, -ENOENT is returned by __device_reset() if the "_RST"
>> method is not found.
>> 
>> In the OF case, -ENOENT is returned by __of_reset_control_get() if the
>> requested id can't be found in a "reset-names" property, or if
>> of_parse_phandle_with_args() returns -ENOENT for the "resets" (or
>> "reset-gpios") property - that is, when this property doesn't exist or
>> the entry indicated by the reset id is empty.
>
> I have nothing to add to what Philipp just said. I believe we don't want
> open coded variant of the device_reset*().

Agree and makes sense. Will update the code as below and will separate the
change in two patches similar to this version. Hope it looks good.

	err = device_reset(i2c_dev->dev);
	if (err == -ENOENT)
		err = tegra_i2c_master_reset(i2c_dev);
	
	WARN_ON_ONCE(err);

Best Regards,
Akhil