[PATCH 1/3] iio: adc: mcp3422: Changing initial channel

[PATCH 1/3] iio: adc: mcp3422: Changing initial channel

[Angelo Compagnucci]

Initial channel should be the first available channel on
all configurations, so changing to channel 0 available on
all supported chips.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
 drivers/iio/adc/mcp3422.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index 254135e..6737df8 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -379,7 +379,7 @@ static int mcp3422_probe(struct i2c_client *client,
 
 	/* meaningful default configuration */
 	config = (MCP3422_CONT_SAMPLING
-		| MCP3422_CHANNEL_VALUE(1)
+		| MCP3422_CHANNEL_VALUE(0)
 		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
 		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
 	mcp3422_update_config(adc, config);
-- 
2.7.4

[PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Angelo Compagnucci]

Some part of the configuration are not touched after the probe
and if something goes wrong on writing the initial one,
the chip will misbehave.
Adding an error checking ensures that the inital configuration will
be written correctly. Moreover ensures that a sensible configuration
will be saved in driver data and used subsequently as intended.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
 drivers/iio/adc/mcp3422.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index 6737df8..63de705 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client *client,
 		| MCP3422_CHANNEL_VALUE(0)
 		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
 		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
-	mcp3422_update_config(adc, config);
+	err = mcp3422_update_config(adc, config);
+	if (err < 0)
+		return err;
 
 	err = devm_iio_device_register(&client->dev, indio_dev);
 	if (err < 0)
-- 
2.7.4

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Jonathan Cameron]

On Wed, 28 Jun 2017 23:53:10 +0200
Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:

> Some part of the configuration are not touched after the probe
> and if something goes wrong on writing the initial one,
> the chip will misbehave.
> Adding an error checking ensures that the inital configuration will
> be written correctly. Moreover ensures that a sensible configuration
> will be saved in driver data and used subsequently as intended.
> 
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing.
Added a reported by for Maarten.

Jonathan
> ---
>  drivers/iio/adc/mcp3422.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index 6737df8..63de705 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client *client,
>  		| MCP3422_CHANNEL_VALUE(0)
>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
>  		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> -	mcp3422_update_config(adc, config);
> +	err = mcp3422_update_config(adc, config);
> +	if (err < 0)
> +		return err;
>  
>  	err = devm_iio_device_register(&client->dev, indio_dev);
>  	if (err < 0)

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Maarten Brock]

On 2017-07-01 12:07, Jonathan Cameron wrote:
> On Wed, 28 Jun 2017 23:53:10 +0200
> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
> 
>> Some part of the configuration are not touched after the probe
>> and if something goes wrong on writing the initial one,
>> the chip will misbehave.
>> Adding an error checking ensures that the inital configuration will
>> be written correctly. Moreover ensures that a sensible configuration
>> will be saved in driver data and used subsequently as intended.
>> 
>> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> Applied to the togreg branch of iio.git and pushed out as testing.
> Added a reported by for Maarten.

Thanks. Also for the CC as I'm not (yet) subscribed to this list.

> 
> Jonathan

Would this fix mean that loading the driver fails if the update_config
fails? And thus if the driver is not a module, would require a reboot
of the OS?
Seems like a rather steep requirement for something that can be so
easily fixed later on by e.g. caching an invalid config channel.
There's not even a single retry. And I don't suppose the I2C driver
will auto-retry either.

Maarten

>> ---
>>  drivers/iio/adc/mcp3422.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
>> index 6737df8..63de705 100644
>> --- a/drivers/iio/adc/mcp3422.c
>> +++ b/drivers/iio/adc/mcp3422.c
>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client 
>> *client,
>>  		| MCP3422_CHANNEL_VALUE(0)
>>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
>>  		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>> -	mcp3422_update_config(adc, config);
>> +	err = mcp3422_update_config(adc, config);
>> +	if (err < 0)
>> +		return err;
>> 
>>  	err = devm_iio_device_register(&client->dev, indio_dev);
>>  	if (err < 0)

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[jic23]

On 03.07.2017 09:42, Maarten Brock wrote:
> On 2017-07-01 12:07, Jonathan Cameron wrote:
>> On Wed, 28 Jun 2017 23:53:10 +0200
>> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
>> 
>>> Some part of the configuration are not touched after the probe
>>> and if something goes wrong on writing the initial one,
>>> the chip will misbehave.
>>> Adding an error checking ensures that the inital configuration will
>>> be written correctly. Moreover ensures that a sensible configuration
>>> will be saved in driver data and used subsequently as intended.
>>> 
>>> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
>> Applied to the togreg branch of iio.git and pushed out as testing.
>> Added a reported by for Maarten.
> 
> Thanks. Also for the CC as I'm not (yet) subscribed to this list.
> 
>> 
>> Jonathan
> 
> Would this fix mean that loading the driver fails if the update_config
> fails? And thus if the driver is not a module, would require a reboot
> of the OS?
Hmm. This is difficult to handle.  If we were waiting on another 
resource
coming up that was reflected by the load of a later driver, we 'could'
use deferred probing. Is that true here?

Wolfram, any thoughts - the issue here is that the i2c bus master is
implemented on an FPGA which hasn't necessarily started by the time this
driver fires up.

I'm a little loath to put in a rather mysterious deferral if we don't
need it.  The slave driver definitely feels like the wrong place to be 
doing
this.

What we should be looking at here I think is the i2c bus not being 
instantiated
until the fpga is ready.  That way these slave devices wouldn't come up
until somewhat later in the process and the driver probe will succeed.

We would normally only retry i2c transactions if we had either:
* known flaky hardware - the sort of thing that fails once every 100 
times.
* a known reason the device isn't responding (and not able to use clock 
stretching)
So device is busy doing a conversion and ignores the bus during that.

Jonathan

> Seems like a rather steep requirement for something that can be so
> easily fixed later on by e.g. caching an invalid config channel.
> There's not even a single retry. And I don't suppose the I2C driver
> will auto-retry either.
> 
> Maarten
> 
>>> ---
>>>  drivers/iio/adc/mcp3422.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
>>> index 6737df8..63de705 100644
>>> --- a/drivers/iio/adc/mcp3422.c
>>> +++ b/drivers/iio/adc/mcp3422.c
>>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client 
>>> *client,
>>>  		| MCP3422_CHANNEL_VALUE(0)
>>>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
>>>  		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>>> -	mcp3422_update_config(adc, config);
>>> +	err = mcp3422_update_config(adc, config);
>>> +	if (err < 0)
>>> +		return err;
>>> 
>>>  	err = devm_iio_device_register(&client->dev, indio_dev);
>>>  	if (err < 0)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Maarten Brock]

On 2017-07-03 13:10, jic23@kernel.org wrote:
> On 03.07.2017 09:42, Maarten Brock wrote:
>> On 2017-07-01 12:07, Jonathan Cameron wrote:
>>> On Wed, 28 Jun 2017 23:53:10 +0200
>>> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
>>> 
>>>> Some part of the configuration are not touched after the probe
>>>> and if something goes wrong on writing the initial one,
>>>> the chip will misbehave.
>>>> Adding an error checking ensures that the inital configuration will
>>>> be written correctly. Moreover ensures that a sensible configuration
>>>> will be saved in driver data and used subsequently as intended.
>>> 
>>> Jonathan
>> 
>> Would this fix mean that loading the driver fails if the update_config
>> fails? And thus if the driver is not a module, would require a reboot
>> of the OS?
> Hmm. This is difficult to handle.  If we were waiting on another 
> resource
> coming up that was reflected by the load of a later driver, we 'could'
> use deferred probing. Is that true here?
> 
> Wolfram, any thoughts - the issue here is that the i2c bus master is
> implemented on an FPGA which hasn't necessarily started by the time 
> this
> driver fires up.

In my case it wasn't the master that was implemented in the FPGA, but 
the
channel from the master to the pins. I guess if the master was 
implemented
in the FPGA and not loaded yet, the master driver would fail to load.

> I'm a little loath to put in a rather mysterious deferral if we don't
> need it.  The slave driver definitely feels like the wrong place to be 
> doing
> this.
> 
> What we should be looking at here I think is the i2c bus not being 
> instantiated
> until the fpga is ready.  That way these slave devices wouldn't come up
> until somewhat later in the process and the driver probe will succeed.

I can envision other use-cases, like the device not yet being powered 
up.

> We would normally only retry i2c transactions if we had either:
> * known flaky hardware - the sort of thing that fails once every 100 
> times.

I would consider every I2C device in this category. Maybe not 1 in 100, 
but not 1
in a million either. With open-drain instead of push-pull drivers and 
thus a
relatively high impedance when signals are rising I would expect some 
disturbance
every once in while. And this is most probably perfectly fine when 
taking
samples. But this fix expects the initialization to always pass when it 
could
easily retry again later on and report an error to the application if it 
still
fails.

One could even argue that at probe time this device needs no write to 
the config
register at all. The driver will select the channel and PGA as necessary 
anyway,
which is a good moment to set the CONTINOUS conversion bit 
unconditionally as
well.

Maarten

> * a known reason the device isn't responding (and not able to use
> clock stretching)
> So device is busy doing a conversion and ignores the bus during that.
> 
> Jonathan
> 
>> Seems like a rather steep requirement for something that can be so
>> easily fixed later on by e.g. caching an invalid config channel.
>> There's not even a single retry. And I don't suppose the I2C driver
>> will auto-retry either.
>> 
>> Maarten
>> 
>>>> ---
>>>>  drivers/iio/adc/mcp3422.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
>>>> index 6737df8..63de705 100644
>>>> --- a/drivers/iio/adc/mcp3422.c
>>>> +++ b/drivers/iio/adc/mcp3422.c
>>>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client 
>>>> *client,
>>>>  		| MCP3422_CHANNEL_VALUE(0)
>>>>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
>>>>  		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>>>> -	mcp3422_update_config(adc, config);
>>>> +	err = mcp3422_update_config(adc, config);
>>>> +	if (err < 0)
>>>> +		return err;
>>>> 
>>>>  	err = devm_iio_device_register(&client->dev, indio_dev);
>>>>  	if (err < 0)
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Mike Looijmans]

=EF=BB=BFOn 03-07-17 14:01, Maarten Brock wrote:
> On 2017-07-03 13:10, jic23@kernel.org wrote:
>> On 03.07.2017 09:42, Maarten Brock wrote:
>>> On 2017-07-01 12:07, Jonathan Cameron wrote:
>>>> On Wed, 28 Jun 2017 23:53:10 +0200
>>>> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
>>>>
>>>>> Some part of the configuration are not touched after the probe
>>>>> and if something goes wrong on writing the initial one,
>>>>> the chip will misbehave.
>>>>> Adding an error checking ensures that the inital configuration will
>>>>> be written correctly. Moreover ensures that a sensible configuration
>>>>> will be saved in driver data and used subsequently as intended.
>>>>
>>>> Jonathan
>>>
>>> Would this fix mean that loading the driver fails if the update_config
>>> fails? And thus if the driver is not a module, would require a reboot
>>> of the OS?
>> Hmm. This is difficult to handle.  If we were waiting on another resourc=
e
>> coming up that was reflected by the load of a later driver, we 'could'
>> use deferred probing. Is that true here?
>>
>> Wolfram, any thoughts - the issue here is that the i2c bus master is
>> implemented on an FPGA which hasn't necessarily started by the time this
>> driver fires up.
>=20
> In my case it wasn't the master that was implemented in the FPGA, but the
> channel from the master to the pins. I guess if the master was implemente=
d
> in the FPGA and not loaded yet, the master driver would fail to load.

I've been using two methods for that:

(1) Build the driver as a module. On my platform, the FPGA is guaranteed to=
 be=20
ready when modules start to load (it loads the FPGA firmware during init=20
before starting the hotplug manager)

(2) Make the driver depend on a power-supply (or clock) that is "supplied"=
=20
once the FPGA powers up. Once way to accomplish this is to load that power=
=20
supply module after programming the FPGA. Because of (1) this just means to=
=20
build the powersupply as a module. This will defer the probe until the FGPA=
 is=20
ready.

>=20
>> I'm a little loath to put in a rather mysterious deferral if we don't
>> need it.  The slave driver definitely feels like the wrong place to be d=
oing
>> this.
>>
>> What we should be looking at here I think is the i2c bus not being insta=
ntiated
>> until the fpga is ready.  That way these slave devices wouldn't come up
>> until somewhat later in the process and the driver probe will succeed.
>=20
> I can envision other use-cases, like the device not yet being powered up.
>=20
>> We would normally only retry i2c transactions if we had either:
>> * known flaky hardware - the sort of thing that fails once every 100 tim=
es.
>=20
> I would consider every I2C device in this category. Maybe not 1 in 100, b=
ut not 1
> in a million either. With open-drain instead of push-pull drivers and thu=
s a
> relatively high impedance when signals are rising I would expect some dis=
turbance
> every once in while. And this is most probably perfectly fine when taking
> samples. But this fix expects the initialization to always pass when it c=
ould
> easily retry again later on and report an error to the application if it =
still
> fails.
>=20
> One could even argue that at probe time this device needs no write to the=
 config
> register at all. The driver will select the channel and PGA as necessary =
anyway,
> which is a good moment to set the CONTINOUS conversion bit unconditionall=
y as
> well.
>=20
> Maarten
>=20
>> * a known reason the device isn't responding (and not able to use
>> clock stretching)
>> So device is busy doing a conversion and ignores the bus during that.
>>
>> Jonathan
>>
>>> Seems like a rather steep requirement for something that can be so
>>> easily fixed later on by e.g. caching an invalid config channel.
>>> There's not even a single retry. And I don't suppose the I2C driver
>>> will auto-retry either.
>>>
>>> Maarten
>>>
>>>>> ---
>>>>>  drivers/iio/adc/mcp3422.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
>>>>> index 6737df8..63de705 100644
>>>>> --- a/drivers/iio/adc/mcp3422.c
>>>>> +++ b/drivers/iio/adc/mcp3422.c
>>>>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client *clien=
t,
>>>>>          | MCP3422_CHANNEL_VALUE(0)
>>>>>          | MCP3422_PGA_VALUE(MCP3422_PGA_1)
>>>>>          | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>>>>> -    mcp3422_update_config(adc, config);
>>>>> +    err =3D mcp3422_update_config(adc, config);
>>>>> +    if (err < 0)
>>>>> +        return err;
>>>>>
>>>>>      err =3D devm_iio_device_register(&client->dev, indio_dev);
>>>>>      if (err < 0)
>>>
>>> --=20
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>=20
> --=20
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[jic23]

On 03.07.2017 13:01, Maarten Brock wrote:
> On 2017-07-03 13:10, jic23@kernel.org wrote:
>> On 03.07.2017 09:42, Maarten Brock wrote:
>>> On 2017-07-01 12:07, Jonathan Cameron wrote:
>>>> On Wed, 28 Jun 2017 23:53:10 +0200
>>>> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
>>>> 
>>>>> Some part of the configuration are not touched after the probe
>>>>> and if something goes wrong on writing the initial one,
>>>>> the chip will misbehave.
>>>>> Adding an error checking ensures that the inital configuration will
>>>>> be written correctly. Moreover ensures that a sensible 
>>>>> configuration
>>>>> will be saved in driver data and used subsequently as intended.
>>>> 
>>>> Jonathan
>>> 
>>> Would this fix mean that loading the driver fails if the 
>>> update_config
>>> fails? And thus if the driver is not a module, would require a reboot
>>> of the OS?
>> Hmm. This is difficult to handle.  If we were waiting on another 
>> resource
>> coming up that was reflected by the load of a later driver, we 'could'
>> use deferred probing. Is that true here?
>> 
>> Wolfram, any thoughts - the issue here is that the i2c bus master is
>> implemented on an FPGA which hasn't necessarily started by the time 
>> this
>> driver fires up.
> 
> In my case it wasn't the master that was implemented in the FPGA, but 
> the
> channel from the master to the pins. I guess if the master was 
> implemented
> in the FPGA and not loaded yet, the master driver would fail to load.
Perhaps represent the FPGA explicitly as an i2c mux?  Kind of moves the 
problem
without solving it, but at least represents the hardware architecture.
> 
>> I'm a little loath to put in a rather mysterious deferral if we don't
>> need it.  The slave driver definitely feels like the wrong place to be 
>> doing
>> this.
>> 
>> What we should be looking at here I think is the i2c bus not being 
>> instantiated
>> until the fpga is ready.  That way these slave devices wouldn't come 
>> up
>> until somewhat later in the process and the driver probe will succeed.
> 
> I can envision other use-cases, like the device not yet being powered 
> up.
That should be explicitly represented as part of the devicetree or 
similar - i.e.
the regulator state should be known or controlled.  Any initial power up 
time
is usually handled by enforcing an appropriate sleep before talking to 
it in probe.
Naturally there will always be weird special cases though where a small 
number of
retries makes sense.
> 
>> We would normally only retry i2c transactions if we had either:
>> * known flaky hardware - the sort of thing that fails once every 100 
>> times.
> 
> I would consider every I2C device in this category. Maybe not 1 in
> 100, but not 1
> in a million either. With open-drain instead of push-pull drivers and 
> thus a
> relatively high impedance when signals are rising I would expect some
> disturbance
> every once in while. And this is most probably perfectly fine when 
> taking
> samples. But this fix expects the initialization to always pass when it 
> could
> easily retry again later on and report an error to the application if 
> it still
> fails.
One for Wolfram rather than me.
> 
> One could even argue that at probe time this device needs no write to 
> the config
> register at all. The driver will select the channel and PGA as 
> necessary anyway,
> which is a good moment to set the CONTINOUS conversion bit 
> unconditionally as
> well.
That would work for me as an alternative solution.
> 
> Maarten
> 
>> * a known reason the device isn't responding (and not able to use
>> clock stretching)
>> So device is busy doing a conversion and ignores the bus during that.
>> 
>> Jonathan
>> 
>>> Seems like a rather steep requirement for something that can be so
>>> easily fixed later on by e.g. caching an invalid config channel.
>>> There's not even a single retry. And I don't suppose the I2C driver
>>> will auto-retry either.
>>> 
>>> Maarten
>>> 
>>>>> ---
>>>>>  drivers/iio/adc/mcp3422.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
>>>>> index 6737df8..63de705 100644
>>>>> --- a/drivers/iio/adc/mcp3422.c
>>>>> +++ b/drivers/iio/adc/mcp3422.c
>>>>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client 
>>>>> *client,
>>>>>  		| MCP3422_CHANNEL_VALUE(0)
>>>>>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
>>>>>  		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>>>>> -	mcp3422_update_config(adc, config);
>>>>> +	err = mcp3422_update_config(adc, config);
>>>>> +	if (err < 0)
>>>>> +		return err;
>>>>> 
>>>>>  	err = devm_iio_device_register(&client->dev, indio_dev);
>>>>>  	if (err < 0)
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" 
>>> in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Angelo Compagnucci]

2017-07-03 14:25 GMT+02:00  <jic23@kernel.org>:
> On 03.07.2017 13:01, Maarten Brock wrote:
>>
>> On 2017-07-03 13:10, jic23@kernel.org wrote:
>>>
>>> On 03.07.2017 09:42, Maarten Brock wrote:
>>>>
>>>> On 2017-07-01 12:07, Jonathan Cameron wrote:
>>>>>
>>>>> On Wed, 28 Jun 2017 23:53:10 +0200
>>>>> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
>>>>>
>>>>>> Some part of the configuration are not touched after the probe
>>>>>> and if something goes wrong on writing the initial one,
>>>>>> the chip will misbehave.
>>>>>> Adding an error checking ensures that the inital configuration will
>>>>>> be written correctly. Moreover ensures that a sensible configuration
>>>>>> will be saved in driver data and used subsequently as intended.
>>>>>
>>>>>
>>>>> Jonathan
>>>>
>>>>
>>>> Would this fix mean that loading the driver fails if the update_config
>>>> fails? And thus if the driver is not a module, would require a reboot
>>>> of the OS?
>>>
>>> Hmm. This is difficult to handle.  If we were waiting on another resource
>>> coming up that was reflected by the load of a later driver, we 'could'
>>> use deferred probing. Is that true here?
>>>
>>> Wolfram, any thoughts - the issue here is that the i2c bus master is
>>> implemented on an FPGA which hasn't necessarily started by the time this
>>> driver fires up.
>>
>>
>> In my case it wasn't the master that was implemented in the FPGA, but the
>> channel from the master to the pins. I guess if the master was implemented
>> in the FPGA and not loaded yet, the master driver would fail to load.
>
> Perhaps represent the FPGA explicitly as an i2c mux?  Kind of moves the
> problem
> without solving it, but at least represents the hardware architecture.
>>
>>
>>> I'm a little loath to put in a rather mysterious deferral if we don't
>>> need it.  The slave driver definitely feels like the wrong place to be
>>> doing
>>> this.
>>>
>>> What we should be looking at here I think is the i2c bus not being
>>> instantiated
>>> until the fpga is ready.  That way these slave devices wouldn't come up
>>> until somewhat later in the process and the driver probe will succeed.
>>
>>
>> I can envision other use-cases, like the device not yet being powered up.
>
> That should be explicitly represented as part of the devicetree or similar -
> i.e.
> the regulator state should be known or controlled.  Any initial power up
> time
> is usually handled by enforcing an appropriate sleep before talking to it in
> probe.
> Naturally there will always be weird special cases though where a small
> number of
> retries makes sense.
>>
>>
>>> We would normally only retry i2c transactions if we had either:
>>> * known flaky hardware - the sort of thing that fails once every 100
>>> times.
>>
>>
>> I would consider every I2C device in this category. Maybe not 1 in
>> 100, but not 1
>> in a million either. With open-drain instead of push-pull drivers and thus
>> a
>> relatively high impedance when signals are rising I would expect some
>> disturbance
>> every once in while. And this is most probably perfectly fine when taking
>> samples. But this fix expects the initialization to always pass when it
>> could
>> easily retry again later on and report an error to the application if it
>> still
>> fails.
>
> One for Wolfram rather than me.
>>
>>
>> One could even argue that at probe time this device needs no write to the
>> config
>> register at all. The driver will select the channel and PGA as necessary
>> anyway,
>> which is a good moment to set the CONTINOUS conversion bit unconditionally
>> as
>> well.
>
> That would work for me as an alternative solution.

I think that for this driver, the simplest solution to this problem
would be to set the adc->config during probe, cause this configuration
will be updated each time a channel is changed. Checking for error on
probe probably could be optional.

The only thing that bothers me is when a device is unconfigured cause
an error and a user tries to read a value without changing channel.

IMHO a driver should fail when loaded on a erratic hardware and the
probe should reflect the fact that the driver is loaded properly. I
had a look at other I2C device drivers (rtc, eeprom) and usually they
fails when not probed correctly.

In case a user needs a driver at a later time in booting, she should
use that driver as a module an load it at proper time.

Sincerely, Angelo.

>
>>
>> Maarten
>>
>>> * a known reason the device isn't responding (and not able to use
>>> clock stretching)
>>> So device is busy doing a conversion and ignores the bus during that.
>>>
>>> Jonathan
>>>
>>>> Seems like a rather steep requirement for something that can be so
>>>> easily fixed later on by e.g. caching an invalid config channel.
>>>> There's not even a single retry. And I don't suppose the I2C driver
>>>> will auto-retry either.
>>>>
>>>> Maarten
>>>>
>>>>>> ---
>>>>>>  drivers/iio/adc/mcp3422.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
>>>>>> index 6737df8..63de705 100644
>>>>>> --- a/drivers/iio/adc/mcp3422.c
>>>>>> +++ b/drivers/iio/adc/mcp3422.c
>>>>>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client
>>>>>> *client,
>>>>>>                 | MCP3422_CHANNEL_VALUE(0)
>>>>>>                 | MCP3422_PGA_VALUE(MCP3422_PGA_1)
>>>>>>                 | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>>>>>> -       mcp3422_update_config(adc, config);
>>>>>> +       err = mcp3422_update_config(adc, config);
>>>>>> +       if (err < 0)
>>>>>> +               return err;
>>>>>>
>>>>>>         err = devm_iio_device_register(&client->dev, indio_dev);
>>>>>>         if (err < 0)
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

Re: [PATCH 2/3] iio: adc: mcp3422: Checking for error on probe

[Jonathan Cameron]

On Mon, 3 Jul 2017 23:04:10 +0200
Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:

> 2017-07-03 14:25 GMT+02:00  <jic23@kernel.org>:
> > On 03.07.2017 13:01, Maarten Brock wrote:  
> >>
> >> On 2017-07-03 13:10, jic23@kernel.org wrote:  
> >>>
> >>> On 03.07.2017 09:42, Maarten Brock wrote:  
> >>>>
> >>>> On 2017-07-01 12:07, Jonathan Cameron wrote:  
> >>>>>
> >>>>> On Wed, 28 Jun 2017 23:53:10 +0200
> >>>>> Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
> >>>>>  
> >>>>>> Some part of the configuration are not touched after the probe
> >>>>>> and if something goes wrong on writing the initial one,
> >>>>>> the chip will misbehave.
> >>>>>> Adding an error checking ensures that the inital configuration will
> >>>>>> be written correctly. Moreover ensures that a sensible configuration
> >>>>>> will be saved in driver data and used subsequently as intended.  
> >>>>>
> >>>>>
> >>>>> Jonathan  
> >>>>
> >>>>
> >>>> Would this fix mean that loading the driver fails if the update_config
> >>>> fails? And thus if the driver is not a module, would require a reboot
> >>>> of the OS?  
> >>>
> >>> Hmm. This is difficult to handle.  If we were waiting on another resource
> >>> coming up that was reflected by the load of a later driver, we 'could'
> >>> use deferred probing. Is that true here?
> >>>
> >>> Wolfram, any thoughts - the issue here is that the i2c bus master is
> >>> implemented on an FPGA which hasn't necessarily started by the time this
> >>> driver fires up.  
> >>
> >>
> >> In my case it wasn't the master that was implemented in the FPGA, but the
> >> channel from the master to the pins. I guess if the master was implemented
> >> in the FPGA and not loaded yet, the master driver would fail to load.  
> >
> > Perhaps represent the FPGA explicitly as an i2c mux?  Kind of moves the
> > problem
> > without solving it, but at least represents the hardware architecture.  
> >>
> >>  
> >>> I'm a little loath to put in a rather mysterious deferral if we don't
> >>> need it.  The slave driver definitely feels like the wrong place to be
> >>> doing
> >>> this.
> >>>
> >>> What we should be looking at here I think is the i2c bus not being
> >>> instantiated
> >>> until the fpga is ready.  That way these slave devices wouldn't come up
> >>> until somewhat later in the process and the driver probe will succeed.  
> >>
> >>
> >> I can envision other use-cases, like the device not yet being powered up.  
> >
> > That should be explicitly represented as part of the devicetree or similar -
> > i.e.
> > the regulator state should be known or controlled.  Any initial power up
> > time
> > is usually handled by enforcing an appropriate sleep before talking to it in
> > probe.
> > Naturally there will always be weird special cases though where a small
> > number of
> > retries makes sense.  
> >>
> >>  
> >>> We would normally only retry i2c transactions if we had either:
> >>> * known flaky hardware - the sort of thing that fails once every 100
> >>> times.  
> >>
> >>
> >> I would consider every I2C device in this category. Maybe not 1 in
> >> 100, but not 1
> >> in a million either. With open-drain instead of push-pull drivers and thus
> >> a
> >> relatively high impedance when signals are rising I would expect some
> >> disturbance
> >> every once in while. And this is most probably perfectly fine when taking
> >> samples. But this fix expects the initialization to always pass when it
> >> could
> >> easily retry again later on and report an error to the application if it
> >> still
> >> fails.  
> >
> > One for Wolfram rather than me.  
> >>
> >>
> >> One could even argue that at probe time this device needs no write to the
> >> config
> >> register at all. The driver will select the channel and PGA as necessary
> >> anyway,
> >> which is a good moment to set the CONTINOUS conversion bit unconditionally
> >> as
> >> well.  
> >
> > That would work for me as an alternative solution.  
> 
> I think that for this driver, the simplest solution to this problem
> would be to set the adc->config during probe, cause this configuration
> will be updated each time a channel is changed. Checking for error on
> probe probably could be optional.
> 
> The only thing that bothers me is when a device is unconfigured cause
> an error and a user tries to read a value without changing channel.
> 
> IMHO a driver should fail when loaded on a erratic hardware and the
> probe should reflect the fact that the driver is loaded properly. I
> had a look at other I2C device drivers (rtc, eeprom) and usually they
> fails when not probed correctly.
> 
> In case a user needs a driver at a later time in booting, she should
> use that driver as a module an load it at proper time.

I agree.  The art with 'weird' hardware that has a path that isn't
ready yet is to represent that hardware explicitly so the dependency
can be handled cleanly.

A failure due to unreliable comms is some something that should be fixed.
i2c transactions should not be failing.

Obviously actual flakey slave implementation is a different matter
(such as one board I had at one time where there were no pull ups
so it couldn't read the NACK or ACK responses...  That was evil
to handle :)

Jonathan
> 
> Sincerely, Angelo.
> 
> >  
> >>
> >> Maarten
> >>  
> >>> * a known reason the device isn't responding (and not able to use
> >>> clock stretching)
> >>> So device is busy doing a conversion and ignores the bus during that.
> >>>
> >>> Jonathan
> >>>  
> >>>> Seems like a rather steep requirement for something that can be so
> >>>> easily fixed later on by e.g. caching an invalid config channel.
> >>>> There's not even a single retry. And I don't suppose the I2C driver
> >>>> will auto-retry either.
> >>>>
> >>>> Maarten
> >>>>  
> >>>>>> ---
> >>>>>>  drivers/iio/adc/mcp3422.c | 4 +++-
> >>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> >>>>>> index 6737df8..63de705 100644
> >>>>>> --- a/drivers/iio/adc/mcp3422.c
> >>>>>> +++ b/drivers/iio/adc/mcp3422.c
> >>>>>> @@ -382,7 +382,9 @@ static int mcp3422_probe(struct i2c_client
> >>>>>> *client,
> >>>>>>                 | MCP3422_CHANNEL_VALUE(0)
> >>>>>>                 | MCP3422_PGA_VALUE(MCP3422_PGA_1)
> >>>>>>                 | MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> >>>>>> -       mcp3422_update_config(adc, config);
> >>>>>> +       err = mcp3422_update_config(adc, config);
> >>>>>> +       if (err < 0)
> >>>>>> +               return err;
> >>>>>>
> >>>>>>         err = devm_iio_device_register(&client->dev, indio_dev);
> >>>>>>         if (err < 0)  
> >>>>
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> >>>> the body of a message to majordomo@vger.kernel.org
> >>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 
> 
> 

[PATCH 3/3] iio: adc: mcp3422: cosmetic fixes

[Angelo Compagnucci]

This patch fixes some cosmetics in source code, it uniforms
tabs after carriage return on methods and fixes formatting.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
 drivers/iio/adc/mcp3422.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index 63de705..866d02f 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -135,7 +135,7 @@ static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config)
 }
 
 static int mcp3422_read_channel(struct mcp3422 *adc,
-				struct iio_chan_spec const *channel, int *value)
+		struct iio_chan_spec const *channel, int *value)
 {
 	int ret;
 	u8 config;
@@ -157,14 +157,14 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
 }
 
 static int mcp3422_read_raw(struct iio_dev *iio,
-			struct iio_chan_spec const *channel, int *val1,
-			int *val2, long mask)
+		struct iio_chan_spec const *channel, int *val1,
+		int *val2, long mask)
 {
 	struct mcp3422 *adc = iio_priv(iio);
 	int err;
 
 	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
-	u8 pga		 = MCP3422_PGA(adc->config);
+	u8 pga = MCP3422_PGA(adc->config);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -174,7 +174,6 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-
 		*val1 = 0;
 		*val2 = mcp3422_scales[sample_rate][pga];
 		return IIO_VAL_INT_PLUS_NANO;
@@ -191,8 +190,8 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 }
 
 static int mcp3422_write_raw(struct iio_dev *iio,
-			struct iio_chan_spec const *channel, int val1,
-			int val2, long mask)
+		struct iio_chan_spec const *channel, int val1,
+		int val2, long mask)
 {
 	struct mcp3422 *adc = iio_priv(iio);
 	u8 temp;
@@ -331,7 +330,7 @@ static const struct iio_info mcp3422_info = {
 };
 
 static int mcp3422_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+		const struct i2c_device_id *id)
 {
 	struct iio_dev *indio_dev;
 	struct mcp3422 *adc;
-- 
2.7.4

Re: [PATCH 3/3] iio: adc: mcp3422: cosmetic fixes

[Jonathan Cameron]

On Wed, 28 Jun 2017 23:53:11 +0200
Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:

> This patch fixes some cosmetics in source code, it uniforms
> tabs after carriage return on methods and fixes formatting.
> 
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
It's not a hard and fast rule, but, where possible, parameters
should be aligned after the opening bracket.  A few of these
already did this. It would be nice to make the others follow
that convention as well.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/mcp3422.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index 63de705..866d02f 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -135,7 +135,7 @@ static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config)
>  }
>  
>  static int mcp3422_read_channel(struct mcp3422 *adc,
> -				struct iio_chan_spec const *channel, int *value)
> +		struct iio_chan_spec const *channel, int *value)
>  {
>  	int ret;
>  	u8 config;
> @@ -157,14 +157,14 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
>  }
>  
>  static int mcp3422_read_raw(struct iio_dev *iio,
> -			struct iio_chan_spec const *channel, int *val1,
> -			int *val2, long mask)
> +		struct iio_chan_spec const *channel, int *val1,
> +		int *val2, long mask)
>  {
>  	struct mcp3422 *adc = iio_priv(iio);
>  	int err;
>  
>  	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
> -	u8 pga		 = MCP3422_PGA(adc->config);
> +	u8 pga = MCP3422_PGA(adc->config);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -174,7 +174,6 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -
>  		*val1 = 0;
>  		*val2 = mcp3422_scales[sample_rate][pga];
>  		return IIO_VAL_INT_PLUS_NANO;
> @@ -191,8 +190,8 @@ static int mcp3422_read_raw(struct iio_dev *iio,
>  }
>  
>  static int mcp3422_write_raw(struct iio_dev *iio,
> -			struct iio_chan_spec const *channel, int val1,
> -			int val2, long mask)
> +		struct iio_chan_spec const *channel, int val1,
> +		int val2, long mask)
>  {
>  	struct mcp3422 *adc = iio_priv(iio);
>  	u8 temp;
> @@ -331,7 +330,7 @@ static const struct iio_info mcp3422_info = {
>  };
>  
>  static int mcp3422_probe(struct i2c_client *client,
> -			 const struct i2c_device_id *id)
> +		const struct i2c_device_id *id)
>  {
>  	struct iio_dev *indio_dev;
>  	struct mcp3422 *adc;

Re: [PATCH 1/3] iio: adc: mcp3422: Changing initial channel

[Jonathan Cameron]

On Wed, 28 Jun 2017 23:53:09 +0200
Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:

> Initial channel should be the first available channel on
> all configurations, so changing to channel 0 available on
> all supported chips.
> 
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
I'm going to apply these the 'slow' route as the failure case in
patch 2 is reasonably uncommon.  Maarten if you want to request
a back port to stable after these have merged (will probably
be a while given where we are in the cycle) then that's fine
with me.

Applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/mcp3422.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
> index 254135e..6737df8 100644
> --- a/drivers/iio/adc/mcp3422.c
> +++ b/drivers/iio/adc/mcp3422.c
> @@ -379,7 +379,7 @@ static int mcp3422_probe(struct i2c_client *client,
>  
>  	/* meaningful default configuration */
>  	config = (MCP3422_CONT_SAMPLING
> -		| MCP3422_CHANNEL_VALUE(1)
> +		| MCP3422_CHANNEL_VALUE(0)
>  		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
>  		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
>  	mcp3422_update_config(adc, config);