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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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