Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
and modify the i2c_init_recovery() function to call that if specified
instead of the generic i2c_gpio_init_recovery() function.

This allows controller drivers to skip calling the generic code by
implementing a dummy callback function, or alternatively to run a
fine tuned custom implementation.

This is needed for the 'i2c-pxa' driver in order to be able to fix
a long standing bug for which the fix will be implemented in a
followup patch.

Cc: stable@vger.kernel.org # 6.3+
Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
  - add 'Reviewed-by' tag from Linus Walleij
  - rebase on tip of i2c/for-current
---
 drivers/i2c/i2c-core-base.c | 8 +++++++-
 include/linux/i2c.h         | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ecca8c006b020379fb53293b20ab09ba25314609..e38be81dbcc17dd15947a189fd3b89def8529d1e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -427,12 +427,18 @@ static int i2c_init_recovery(struct i2c_adapter *adap)
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 	bool is_error_level = true;
 	char *err_str;
+	int ret;
 
 	if (!bri)
 		return 0;
 
-	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
+	if (bri->init_recovery) {
+		ret = bri->init_recovery(adap);
+		if (ret)
+			return ret;
+	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
 		return -EPROBE_DEFER;
+	}
 
 	if (!bri->recover_bus) {
 		err_str = "no suitable method provided";
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 20fd41b51d5c85ee1665395c07345faafd8e2fca..4cefdd4c730fc9dc4655f6581f3dea64435d7124 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -636,6 +636,9 @@ struct i2c_timings {
  *	for generic GPIO recovery.
  * @get_bus_free: Returns the bus free state as seen from the IP core in case it
  *	has a more complex internal logic than just reading SDA. Optional.
+ * @init_recovery: If specified, it will be called instead of the generic GPIO
+ *	recovery initialization code. Platform may use a dummy callback to skip
+ *	calling the generic code, or it may use a custom implementation.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
@@ -660,6 +663,7 @@ struct i2c_bus_recovery_info {
 	void (*set_sda)(struct i2c_adapter *adap, int val);
 	int (*get_bus_free)(struct i2c_adapter *adap);
 
+	int (*init_recovery)(struct i2c_adapter *adap);
 	void (*prepare_recovery)(struct i2c_adapter *adap);
 	void (*unprepare_recovery)(struct i2c_adapter *adap);
 

-- 
2.50.1

On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
> Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
> and modify the i2c_init_recovery() function to call that if specified
> instead of the generic i2c_gpio_init_recovery() function.
> 
> This allows controller drivers to skip calling the generic code by
> implementing a dummy callback function, or alternatively to run a
> fine tuned custom implementation.
> 
> This is needed for the 'i2c-pxa' driver in order to be able to fix
> a long standing bug for which the fix will be implemented in a

> followup patch.

"...next change."

...

The first traditional question is why the generic recovery is not working.

...

> -	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
> +	if (bri->init_recovery) {
> +		ret = bri->init_recovery(adap);
> +		if (ret)
> +			return ret;

> +	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
>  		return -EPROBE_DEFER;
> +	}

If the above stays, I think we would drop the last and always have
init_recovery to be assigned.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Aug 11, 2025 at 11:17:41PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
> > Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
> > and modify the i2c_init_recovery() function to call that if specified
> > instead of the generic i2c_gpio_init_recovery() function.
> > 
> > This allows controller drivers to skip calling the generic code by
> > implementing a dummy callback function, or alternatively to run a
> > fine tuned custom implementation.
> > 
> > This is needed for the 'i2c-pxa' driver in order to be able to fix
> > a long standing bug for which the fix will be implemented in a
> 
> > followup patch.
> 
> "...next change."
> 
> ...
> 
> The first traditional question is why the generic recovery is not working.

I opposed the conversion of my recovery code to generic recovery when
it was first mooted for this driver, and was over-ruled. Lo and behold,
as I predicted, generic recovery fails with i2c-pxa. I now don't
remember the details, but there has been a regression reported...

Given that I was over-ruled, I am not minded to go back and try and
find either the previous discussion (google can be exceedingly
difficult now to find such history) nor to try and work it out again
(I've other things, including meetings today.)

Nevertheless, all I now remember is that generic recovery breaks
i2c-pxa, whereas my recovery works.

If I had to guess, I suspect it's something to do with how careful I
was to ensure a glitchless transition between i2c mode and GPIO mode
on the pinmux, and the generic recovery probably isn't as careful, but
I could be wrong there.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2025. 08. 11. 22:17 keltezéssel, Andy Shevchenko írta:
> On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
>> Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
>> and modify the i2c_init_recovery() function to call that if specified
>> instead of the generic i2c_gpio_init_recovery() function.
>>
>> This allows controller drivers to skip calling the generic code by
>> implementing a dummy callback function, or alternatively to run a
>> fine tuned custom implementation.
>>
>> This is needed for the 'i2c-pxa' driver in order to be able to fix
>> a long standing bug for which the fix will be implemented in a
> 
>> followup patch.
> 
> "...next change."

Ok.

> 
> ...
> 
> The first traditional question is why the generic recovery is not working.


The details are in the driver specific patches. Should I write it all down here too?

> 
> ...
> 
>> -	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
>> +	if (bri->init_recovery) {
>> +		ret = bri->init_recovery(adap);
>> +		if (ret)
>> +			return ret;
> 
>> +	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
>>  		return -EPROBE_DEFER;
>> +	}
> 
> If the above stays, I think we would drop the last and always have
> init_recovery to be assigned.
> 

In that case we would have something like this:

    if (!bri->init_recovery)
        bri->init_recovery = i2c_gpio_init_recovery;

    ret = bri->init_recovery(adap);
    if (ret)
        return ret;

Since the callback is used only once, and within the same fuction where it is
assigned, I don't really see the advantage of the assignment. Although it
definitely looks cleaner as far as error handling is concerned.

Originally, I have used the following solution:

    if (bri->init_recovery)
        ret = bri->init_recovery(adap);
    else
        ret = i2c_gpio_init_recovery(adap);

    if (ret)
        return ret;

However the existing code ignores errors from i2c_gpio_init_recovery() except
EPROBE_DEFER, so I changed this to the code proposed in the patch in order to
keep the existing behaviour.

Regards,
Gabor

On Wed, Aug 13, 2025 at 12:24:22PM +0200, Gabor Juhos wrote:
> 2025. 08. 11. 22:17 keltezéssel, Andy Shevchenko írta:
> > On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:

...

> >> This is needed for the 'i2c-pxa' driver in order to be able to fix
> >> a long standing bug for which the fix will be implemented in a

The above left for some context for the below discussion.

...

> > The first traditional question is why the generic recovery is not working.
> 
> The details are in the driver specific patches. Should I write it all down here too?

Instead of the above paragraph, give a summary of your use case to answer 'why'
it can not be done differently.

...

> >> -	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
> >> +	if (bri->init_recovery) {
> >> +		ret = bri->init_recovery(adap);
> >> +		if (ret)
> >> +			return ret;
> > 
> >> +	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
> >>  		return -EPROBE_DEFER;
> >> +	}
> > 
> > If the above stays, I think we would drop the last and always have
> > init_recovery to be assigned.
> 
> In that case we would have something like this:
> 
>     if (!bri->init_recovery)
>         bri->init_recovery = i2c_gpio_init_recovery;
> 
>     ret = bri->init_recovery(adap);
>     if (ret)
>         return ret;
> 
> Since the callback is used only once, and within the same fuction where it is
> assigned, I don't really see the advantage of the assignment. Although it
> definitely looks cleaner as far as error handling is concerned.
> 
> Originally, I have used the following solution:
> 
>     if (bri->init_recovery)
>         ret = bri->init_recovery(adap);
>     else
>         ret = i2c_gpio_init_recovery(adap);

> 

Without this blank line...

>     if (ret)
>         return ret;

...this looks like the best compromise among proposed implementations.

> However the existing code ignores errors from i2c_gpio_init_recovery() except
> EPROBE_DEFER, so I changed this to the code proposed in the patch in order to
> keep the existing behaviour.

-- 
With Best Regards,
Andy Shevchenko

2025. 08. 13. 15:06 keltezéssel, Andy Shevchenko írta:
> On Wed, Aug 13, 2025 at 12:24:22PM +0200, Gabor Juhos wrote:
>> 2025. 08. 11. 22:17 keltezéssel, Andy Shevchenko írta:
>>> On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
> 
> ...
> 
>>>> This is needed for the 'i2c-pxa' driver in order to be able to fix
>>>> a long standing bug for which the fix will be implemented in a
> 
> The above left for some context for the below discussion.
> 
> ...
> 
>>> The first traditional question is why the generic recovery is not working.
>>
>> The details are in the driver specific patches. Should I write it all down here too?
> 
> Instead of the above paragraph, give a summary of your use case to answer 'why'
> it can not be done differently.

Ok.

> 
> ...
> 

...

>>
>> Originally, I have used the following solution:
>>
>>     if (bri->init_recovery)
>>         ret = bri->init_recovery(adap);
>>     else
>>         ret = i2c_gpio_init_recovery(adap);
> 
>>
> 
> Without this blank line...
> 
>>     if (ret)
>>         return ret;
> 
> ...this looks like the best compromise among proposed implementations.

Ok, I will use this in the next version.

Regards,
Gabor