| Message ID | 20171204123640.3382-1-wsa+renesas@sang-engineering.com |
|---|---|
| Headers | show |
| Series | i2c: send STOP after recovery; use it for i2c-rcar | expand |
On 4/12/2017 20:36, Wolfram Sang wrote: > When implementing bus recovery for the i2c-rcar driver, two problems were > encountered: 1) When reading the SDA bit, not the SDA status was returned but > the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to > consider the bus free again. SCL/SDA high is not enough, and there is no other > way known to reset the internal logic otherwise. > > The obvious solution to just send STOP after recovery makes sense for the > generic case, too, IMO. If we made a device release SDA again, and are about > start a new transfer using START, then we should terminate the previous state > properly with STOP. This may help with some devices and shouldn't create any > drawback AFAICS. > > For this, we need to introduce a 'set_sda' callback to the recovery > infrastructure. Because of this and some other minor recovery core changes, I > CCed a few people working on I2C bus recovery recently. The first five patches > may be interesting for you, so your input is greatly appreciated. Also, testing > the new features with GPIO based recovery would be awesome to have. > > This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure > is documented here: Patches 1-5. Tested-by: Phil Reid <preid@electromag.com.au>
On 04.12.2017 13:36, Wolfram Sang wrote: > When implementing bus recovery for the i2c-rcar driver, two problems were > encountered: 1) When reading the SDA bit, not the SDA status was returned but > the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to > consider the bus free again. SCL/SDA high is not enough, and there is no other > way known to reset the internal logic otherwise. > > The obvious solution to just send STOP after recovery makes sense for the > generic case, too, IMO. If we made a device release SDA again, and are about > start a new transfer using START, then we should terminate the previous state > properly with STOP. This may help with some devices and shouldn't create any > drawback AFAICS. > > For this, we need to introduce a 'set_sda' callback to the recovery > infrastructure. Because of this and some other minor recovery core changes, I > CCed a few people working on I2C bus recovery recently. The first five patches > may be interesting for you, so your input is greatly appreciated. Also, testing > the new features with GPIO based recovery would be awesome to have. > > This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure > is documented here: > > https://elinux.org/Tests:I2C-bus-recovery > > A branch is available here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/rcar-i2c-recovery > > Please let me know what you think. I do not feel myself experienced I2C developer, so please be merciful if I write stupid things :) From what I see the whole recovery infrastructure partially duplicates i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates i2c-gpio driver. Wouldn't be better to somehow reuse existing code? For example by adding recovery callback in i2c_algorithm, and call i2c-gpio::recovery or i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case). I am not sure how much work it requires, and if it is worth it. Please consider it as suggestion. Regards Andrzej
> I do not feel myself experienced I2C developer, so please be merciful if > I write stupid things :) Don't worry :) Thanks for sharing! > From what I see the whole recovery infrastructure partially duplicates > i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates > i2c-gpio driver. > Wouldn't be better to somehow reuse existing code? For example by adding > recovery callback in i2c_algorithm, and call i2c-gpio::recovery or > i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case). I understand what you mean but I also don't think it is a good idea in practice. We can't save much by using i2c-gpio, because GPIO is only one use-case, having custom set/get functions being another one, for example. A bigger chance for reuse would, in deed, be i2c-algo-bit.c, but there are some subtle differences and encoding them in the generic functions would make them more unreadable IMHO. But the real argument is: dependency hell. The core might be build-in, the rest is a module, and things get more complicated. And I don't want to enforce this on _every_ I2C user out there. So, given the really simple functions, I think it makes sense to have them in the core.