[PATCH] iio: adc: ti-ads1015: support deferred probe

[PATCH] iio: adc: ti-ads1015: support deferred probe

[Tomas Melin]

Support deferred probe for cases where communication on
i2c bus fails. These failures could happen for a variety of
reasons including bus arbitration error or power failure.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/iio/adc/ti-ads1015.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 075c75a87544..f7dfcb894408 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -1062,7 +1062,7 @@ static int ads1015_probe(struct i2c_client *client)
 		ret = regmap_update_bits(data->regmap, ADS1015_CFG_REG,
 					cfg_comp_mask, cfg_comp);
 		if (ret)
-			return ret;
+			goto out;
 
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
 						NULL, ads1015_event_handler,
@@ -1074,7 +1074,7 @@ static int ads1015_probe(struct i2c_client *client)
 
 	ret = ads1015_set_conv_mode(data, ADS1015_CONTINUOUS);
 	if (ret)
-		return ret;
+		goto out;
 
 	data->conv_invalid = true;
 
@@ -1092,6 +1092,11 @@ static int ads1015_probe(struct i2c_client *client)
 	}
 
 	return 0;
+
+out:
+	if ((ret == -EAGAIN) || (ret == -ENXIO))
+		return -EPROBE_DEFER;
+	return ret;
 }
 
 static void ads1015_remove(struct i2c_client *client)
-- 
2.39.2

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:
> Support deferred probe for cases where communication on
> i2c bus fails. These failures could happen for a variety of
> reasons including bus arbitration error or power failure.

> +out:
> +	if ((ret == -EAGAIN) || (ret == -ENXIO))
> +		return -EPROBE_DEFER;
> +	return ret;

Oh my... This looks so-o hackish.
If anything, it has to be fixed on the level of regmap I2C APIs or so.

Maybe something like regmap_i2c_try_write()/try_read() new APIs that
will provide the above. Otherwise you want to fix _every single driver_
in the Linux kernel

...

$ git grep -lw builtin_i2c_driver | wc
      5       5     123
$ git grep -lw module_i2c_driver | wc
      1164    1164   35240

(and more that don't use either of the above macros).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Jonathan Cameron]

On Mon, 4 Sep 2023 14:23:29 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:
> > Support deferred probe for cases where communication on
> > i2c bus fails. These failures could happen for a variety of
> > reasons including bus arbitration error or power failure.  
> 
> > +out:
> > +	if ((ret == -EAGAIN) || (ret == -ENXIO))
> > +		return -EPROBE_DEFER;
> > +	return ret;  
> 
> Oh my... This looks so-o hackish.

Agreed.  This is a non starter.

> If anything, it has to be fixed on the level of regmap I2C APIs or so.
> 
> Maybe something like regmap_i2c_try_write()/try_read() new APIs that
> will provide the above. Otherwise you want to fix _every single driver_
> in the Linux kernel

Any probe ordering dependencies should be described by the
firmware and the driver should 'get' the relevant resource.
If there is anything not describable today then that is what we need
to fix, not paper over the holes.

So can we have specifics of what is happening here?

If it's arbitration with some other entity then fix the arbitration
locking / hand over. If it's power, then make sure the relevant
regulator get gotten and turned on + has the right delays etc.

Jonathan

> 
> ...
> 
> $ git grep -lw builtin_i2c_driver | wc
>       5       5     123
> $ git grep -lw module_i2c_driver | wc
>       1164    1164   35240
> 
> (and more that don't use either of the above macros).
> 

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Tomas Melin]

Hi,

On 04/09/2023 16:12, Jonathan Cameron wrote:
> On Mon, 4 Sep 2023 14:23:29 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:
>>> Support deferred probe for cases where communication on
>>> i2c bus fails. These failures could happen for a variety of
>>> reasons including bus arbitration error or power failure.  
>>
>>> +out:
>>> +	if ((ret == -EAGAIN) || (ret == -ENXIO))
>>> +		return -EPROBE_DEFER;
>>> +	return ret;  
>>
>> Oh my... This looks so-o hackish.
> 
> Agreed.  This is a non starter.
> 
>> If anything, it has to be fixed on the level of regmap I2C APIs or so.
>>
>> Maybe something like regmap_i2c_try_write()/try_read() new APIs that
>> will provide the above. Otherwise you want to fix _every single driver_
>> in the Linux kernel
> 
> Any probe ordering dependencies should be described by the
> firmware and the driver should 'get' the relevant resource.
> If there is anything not describable today then that is what we need
> to fix, not paper over the holes
> 
> So can we have specifics of what is happening here?
> 
> If it's arbitration with some other entity then fix the arbitration
> locking / hand over. If it's power, then make sure the relevant
> regulator get gotten and turned on + has the right delays etc.

Yes, right. In this use case, the ads1015 is connected to a channel of
a i2c multiplexer. When the mux is probed, it also enumerates all the
multiplexed buses and probes devices connected to them.
For some reason, it behaves so that the ads1015 is not detected on the
first attempt. Since it's a mux, connected to main i2c line, perhaps
there really is some bus arbitration issue, or then something else.

Anyways, when deferring the probe for the ads1015, and attempting later
again it probes fine.

So, it might be I've taken the wrong angle at this issue, but
it does solve the issue at hand. Obviously, there could be some issue
with the i2c mux driver, or then on hardware level too.

Point is, that if the communication to the i2c bus has some temporary
error like EAGAIN, why could it not be reasonable to try again at a
later time instead of giving up completely.

Thanks,
Tomas



> 
> Jonathan
> 
>>
>> ...
>>
>> $ git grep -lw builtin_i2c_driver | wc
>>       5       5     123
>> $ git grep -lw module_i2c_driver | wc
>>       1164    1164   35240
>>
>> (and more that don't use either of the above macros).
>>
> 

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Lars-Peter Clausen]

On 9/5/23 04:43, Tomas Melin wrote:
> Hi,
>
> On 04/09/2023 16:12, Jonathan Cameron wrote:
>> On Mon, 4 Sep 2023 14:23:29 +0300
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>
>>> On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:
>>>> Support deferred probe for cases where communication on
>>>> i2c bus fails. These failures could happen for a variety of
>>>> reasons including bus arbitration error or power failure.
>>>> +out:
>>>> +	if ((ret == -EAGAIN) || (ret == -ENXIO))
>>>> +		return -EPROBE_DEFER;
>>>> +	return ret;
>>> Oh my... This looks so-o hackish.
>> Agreed.  This is a non starter.
>>
>>> If anything, it has to be fixed on the level of regmap I2C APIs or so.
>>>
>>> Maybe something like regmap_i2c_try_write()/try_read() new APIs that
>>> will provide the above. Otherwise you want to fix _every single driver_
>>> in the Linux kernel
>> Any probe ordering dependencies should be described by the
>> firmware and the driver should 'get' the relevant resource.
>> If there is anything not describable today then that is what we need
>> to fix, not paper over the holes
>>
>> So can we have specifics of what is happening here?
>>
>> If it's arbitration with some other entity then fix the arbitration
>> locking / hand over. If it's power, then make sure the relevant
>> regulator get gotten and turned on + has the right delays etc.
> Yes, right. In this use case, the ads1015 is connected to a channel of
> a i2c multiplexer. When the mux is probed, it also enumerates all the
> multiplexed buses and probes devices connected to them.
> For some reason, it behaves so that the ads1015 is not detected on the
> first attempt. Since it's a mux, connected to main i2c line, perhaps
> there really is some bus arbitration issue, or then something else.
>
> Anyways, when deferring the probe for the ads1015, and attempting later
> again it probes fine.
>
> So, it might be I've taken the wrong angle at this issue, but
> it does solve the issue at hand. Obviously, there could be some issue
> with the i2c mux driver, or then on hardware level too.
>
> Point is, that if the communication to the i2c bus has some temporary
> error like EAGAIN, why could it not be reasonable to try again at a
> later time instead of giving up completely.

The way probe deferral works, or is supposed to work, is that if a 
driver detects that it is missing a resource to initialize the device it 
can return EPROBE_DEFER to try again later. Once a new resource becomes 
available it will try again. In your case there is no resource 
dependency, but just a random failure. So there is no guarantee that 
probe will actually be called again since there might not be any new 
resources that become available.

The solution you've implemented might work on your specific platform, 
but it does not work by design, it only works by chance. Returning 
EPROBE_DEFER for things like IO errors is not the right approach. If you 
need a quick hack you can for example write a small userspace script 
that will trigger re-probe of the device at system startup.

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Tomas Melin]

On 05/09/2023 15:31, Lars-Peter Clausen wrote:
> On 9/5/23 04:43, Tomas Melin wrote:
>> Hi,
>>
>> Point is, that if the communication to the i2c bus has some temporary
>> error like EAGAIN, why could it not be reasonable to try again at a
>> later time instead of giving up completely.
> 
> The way probe deferral works, or is supposed to work, is that if a 
> driver detects that it is missing a resource to initialize the device it 
> can return EPROBE_DEFER to try again later. Once a new resource becomes 
> available it will try again. In your case there is no resource 
> dependency, but just a random failure. So there is no guarantee that 
> probe will actually be called again since there might not be any new 
> resources that become available.
> 
> The solution you've implemented might work on your specific platform, 
> but it does not work by design, it only works by chance. Returning 
> EPROBE_DEFER for things like IO errors is not the right approach. If you 
> need a quick hack you can for example write a small userspace script 
> that will trigger re-probe of the device at system startup.
Right, I will need to take a different approach with this. Thanks for
the input!

Tomas


> 
> 

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Andy Shevchenko]

+Cc: Peter to look from the i2c mux point of view on the issue.

On Tue, Sep 05, 2023 at 02:43:19PM +0300, Tomas Melin wrote:
> On 04/09/2023 16:12, Jonathan Cameron wrote:
> > On Mon, 4 Sep 2023 14:23:29 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:

> >>> Support deferred probe for cases where communication on
> >>> i2c bus fails. These failures could happen for a variety of
> >>> reasons including bus arbitration error or power failure.  

...

> >>> +out:
> >>> +	if ((ret == -EAGAIN) || (ret == -ENXIO))
> >>> +		return -EPROBE_DEFER;
> >>> +	return ret;  
> >>
> >> Oh my... This looks so-o hackish.
> > 
> > Agreed.  This is a non starter.
> > 
> >> If anything, it has to be fixed on the level of regmap I2C APIs or so.
> >>
> >> Maybe something like regmap_i2c_try_write()/try_read() new APIs that
> >> will provide the above. Otherwise you want to fix _every single driver_
> >> in the Linux kernel
> > 
> > Any probe ordering dependencies should be described by the
> > firmware and the driver should 'get' the relevant resource.
> > If there is anything not describable today then that is what we need
> > to fix, not paper over the holes
> > 
> > So can we have specifics of what is happening here?
> > 
> > If it's arbitration with some other entity then fix the arbitration
> > locking / hand over. If it's power, then make sure the relevant
> > regulator get gotten and turned on + has the right delays etc.
> 
> Yes, right. In this use case, the ads1015 is connected to a channel of
> a i2c multiplexer. When the mux is probed, it also enumerates all the
> multiplexed buses and probes devices connected to them.
> For some reason, it behaves so that the ads1015 is not detected on the
> first attempt. Since it's a mux, connected to main i2c line, perhaps
> there really is some bus arbitration issue, or then something else.
> 
> Anyways, when deferring the probe for the ads1015, and attempting later
> again it probes fine.
> 
> So, it might be I've taken the wrong angle at this issue, but
> it does solve the issue at hand. Obviously, there could be some issue
> with the i2c mux driver, or then on hardware level too.
> 
> Point is, that if the communication to the i2c bus has some temporary
> error like EAGAIN, why could it not be reasonable to try again at a
> later time instead of giving up completely.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Tomas Melin]

Hi,

On 04/09/2023 14:23, Andy Shevchenko wrote:
> On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:
>> Support deferred probe for cases where communication on
>> i2c bus fails. These failures could happen for a variety of
>> reasons including bus arbitration error or power failure.
> 
>> +out:
>> +	if ((ret == -EAGAIN) || (ret == -ENXIO))
>> +		return -EPROBE_DEFER;
>> +	return ret;
> 
> Oh my... This looks so-o hackish.> If anything, it has to be fixed on the level of regmap I2C APIs or
so.As such this does fix a real world issue. Providing some helpers for
this on regmap level would probably be great. To add such an API I
suspect there should first be existing users for it, and this would be
one example.
Until there would be such an generic API, I'm not sure if there are many
other alternatives than to check for invalid returns and defer the probe.

Thanks,
Tomas


> 
> Maybe something like regmap_i2c_try_write()/try_read() new APIs that
> will provide the above. Otherwise you want to fix _every single driver_
> in the Linux kern>
> ...
> 
> $ git grep -lw builtin_i2c_driver | wc
>       5       5     123
> $ git grep -lw module_i2c_driver | wc
>       1164    1164   35240
> 
> (and more that don't use either of the above macros).
> 

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 09:13:28AM +0300, Tomas Melin wrote:
> On 04/09/2023 14:23, Andy Shevchenko wrote:
> > On Mon, Sep 04, 2023 at 01:15:22PM +0300, Tomas Melin wrote:

...

> >> +out:
> >> +	if ((ret == -EAGAIN) || (ret == -ENXIO))
> >> +		return -EPROBE_DEFER;
> >> +	return ret;
> > 
> > Oh my... This looks so-o hackish.> If anything, it has to be fixed on the level of regmap I2C APIs or
> so.As such this does fix a real world issue. Providing some helpers for
> this on regmap level would probably be great. To add such an API I
> suspect there should first be existing users for it, and this would be
> one example.
> Until there would be such an generic API, I'm not sure if there are many
> other alternatives than to check for invalid returns and defer the probe.

As Jonathan said, please try to elaborate better what the real word issue is,
how it can be reproduced, etc.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ti-ads1015: support deferred probe

[Tomas Melin]

On 05/09/2023 13:45, Andy Shevchenko wrote:
> 
> As Jonathan said, please try to elaborate better what the real word issue is,
> how it can be reproduced, etc.
> 
Thanks for mentioning, for some reason the message from Jonathan had
been filtered out, found it now.

thanks,
Tomas