| Message ID | 1483613240-32501-1-git-send-email-mike.looijmans@topic.nl |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello Mike, On 01/05/2017 12:47 PM, Mike Looijmans wrote: > The driver calls i2c_add_adapter before writing to config registers, > resulting in dmesg output like this, where devices fail to initialize: > > cdns-i2c ff030000.i2c: timeout waiting on completion > pca953x 1-0041: failed reading register > pca953x: probe of 1-0041 failed with error -110 > at24 1-0050: 512 byte 24c04 EEPROM, writable, 1 bytes/write > cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197 > > The adapter is being used before it completed the "probe". To fix > this, make "i2c_add_adapter" the last thing it calls in probe. > It also makes sense to show the adapter initialization before > the devices on the bus. commonly "it also" in a commit message means a change, which should be done separately, and this is the case here as well. Because the adapter registration i2c_add_adapter() can fail, information about the adapter initialization would be expected only in case of successful registration. The information sent to the kernel log buffer here is quite trivial, probably dev_info() can be just removed, but in any case it should be a separate change. > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Because the adapter registration i2c_add_adapter() can fail, information > about the adapter initialization would be expected only in case of > successful registration. Exactly. > > The information sent to the kernel log buffer here is quite trivial, > probably dev_info() can be just removed, but in any case it should be > a separate change. I am not sure I get you here, but to not have false positive success messages, I'd think that should be all in one patch. Regards, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06-01-17 22:34, Vladimir Zapolskiy wrote: > Hello Mike, > > On 01/05/2017 12:47 PM, Mike Looijmans wrote: >> The driver calls i2c_add_adapter before writing to config registers, >> resulting in dmesg output like this, where devices fail to initialize: >> >> cdns-i2c ff030000.i2c: timeout waiting on completion >> pca953x 1-0041: failed reading register >> pca953x: probe of 1-0041 failed with error -110 >> at24 1-0050: 512 byte 24c04 EEPROM, writable, 1 bytes/write >> cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197 >> >> The adapter is being used before it completed the "probe". To fix >> this, make "i2c_add_adapter" the last thing it calls in probe. >> It also makes sense to show the adapter initialization before >> the devices on the bus. > > commonly "it also" in a commit message means a change, which should be done > separately, and this is the case here as well. > > Because the adapter registration i2c_add_adapter() can fail, information > about the adapter initialization would be expected only in case of > successful registration. I would argue that the "info" message means "the I2C adapter is ready for transaction now, and we'll start initializing devices on the bus". That is the case before it calls i2c_add_adapter(). When i2c_add_adapter() runs, it will start probing devices on the bus. This yields very confusing output, as it will output things in a reversed order: - device X on I2C bus - device Y on I2C bus - cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197 This is especially confusing if there are multiple I2C adapters with muxes behind them, the order then becomes like this: - Device X on bus 0 - Mux A on bus 0 registering bus 2, 3 and 4 - I2C controller for bus 0 - Device Y on bus 1 - I2C controller for bus 1 - Device Z on bus 2 - etc.. > The information sent to the kernel log buffer here is quite trivial, > probably dev_info() can be just removed, but in any case it should be > a separate change. Fine with me too. > >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > > -- > With best wishes, > Vladimir > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I would argue that the "info" message means "the I2C adapter is ready for > transaction now, and we'll start initializing devices on the bus". That is > the case before it calls i2c_add_adapter(). I know what you mean, but i2c_add_adapter does more, and it can fail because the adapter is *not* ready to transfer. Seeing the success message before is also confusing. > When i2c_add_adapter() runs, it will start probing devices on the bus. This > yields very confusing output, as it will output things in a reversed order: > > - device X on I2C bus > - device Y on I2C bus > - cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197 I agree. That being said, somewhen I started working on moving such messages into the core to save string space and have consistent output. Then, we can print at the proper time. So, until then, we should be consistent with the other driver, I'd say.
On 13-01-17 09:37, Wolfram Sang wrote: > >> I would argue that the "info" message means "the I2C adapter is ready for >> transaction now, and we'll start initializing devices on the bus". That is >> the case before it calls i2c_add_adapter(). > > I know what you mean, but i2c_add_adapter does more, and it can fail > because the adapter is *not* ready to transfer. Seeing the success > message before is also confusing. > >> When i2c_add_adapter() runs, it will start probing devices on the bus. This >> yields very confusing output, as it will output things in a reversed order: >> >> - device X on I2C bus >> - device Y on I2C bus >> - cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197 > > I agree. That being said, somewhen I started working on moving such > messages into the core to save string space and have consistent output. > Then, we can print at the proper time. > > So, until then, we should be consistent with the other driver, I'd say. Makes sense. I'll create a v2 patch to just move the i2c_add_adapter to after writing the configuration registers, and leave the dmesg output as is. Thanks for reviewing, Mike. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index 6869712..7793bb8 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -962,10 +962,6 @@ static int cdns_i2c_probe(struct platform_device *pdev) goto err_clk_dis; } - ret = i2c_add_adapter(&id->adap); - if (ret < 0) - goto err_clk_dis; - /* * Cadence I2C controller has a bug wherein it generates * invalid read transaction after HW timeout in master receiver mode. @@ -978,6 +974,10 @@ static int cdns_i2c_probe(struct platform_device *pdev) dev_info(&pdev->dev, "%u kHz mmio %08lx irq %d\n", id->i2c_clk / 1000, (unsigned long)r_mem->start, id->irq); + ret = i2c_add_adapter(&id->adap); + if (ret < 0) + goto err_clk_dis; + return 0; err_clk_dis:
The driver calls i2c_add_adapter before writing to config registers, resulting in dmesg output like this, where devices fail to initialize: cdns-i2c ff030000.i2c: timeout waiting on completion pca953x 1-0041: failed reading register pca953x: probe of 1-0041 failed with error -110 at24 1-0050: 512 byte 24c04 EEPROM, writable, 1 bytes/write cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197 The adapter is being used before it completed the "probe". To fix this, make "i2c_add_adapter" the last thing it calls in probe. It also makes sense to show the adapter initialization before the devices on the bus. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- drivers/i2c/busses/i2c-cadence.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)