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

[Philipp Zabel <p.zabel@pengutronix.de>]

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