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

[Akhil R <akhilrajeev@nvidia.com>]

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