On Mon, 3 Jul 2017 23:04:10 +0200 Angelo Compagnucci <angelo.compagnucci@xxxxxxxxx> wrote: > 2017-07-03 14:25 GMT+02:00 <jic23@xxxxxxxxxx>: > > On 03.07.2017 13:01, Maarten Brock wrote: > >> > >> On 2017-07-03 13:10, jic23@xxxxxxxxxx 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx > >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
