Hi Eugeniu, > BTW, thanks to Bhuvanesh, we've got another patch [*] which tries > to combine the best of both worlds: > > * DMA support in the v1/v2 patches from Andrew/Bhuvanesh > * Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/ This was nice to see. But where does it come from? I don't see it on this list and I also couldn't find it in the regular BSP? > Unfortunately, this patch has a dependency to the rcar_i2c_is_pio() > in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 > (which should be resolvable by extracting the function). This patch is obsolete since March 2019. It has been properly fixed with 94e290b0e9a6 ("i2c: rcar: wait for data empty before starting DMA"). I am still trying to feed this information back. > Do you think we are on the right track with this new approach or do > you feel the implementation is still overly complicated? The approach is much better but there are still things I don't like. The use of 'goto next_txn' is bad. I hope it could be done better with refactoring the code, so DMA will be tried at one place (with two conditions then). Not sure yet, I am still working on refactoring the one-byte transfer which is broken with my patch. What we surely can use from this patch is the -EPROTO handling because I have given up on converting the max read block size first. We can still remove it from this driver if that gets implemented somewhen. > + if (!rcar_i2c_is_pio(priv)) { > + /* > + * Still try to use DMA to receive the rest of > + * data > + */ > + rcar_i2c_dma(priv); > + goto next_txn; > + } else { > + recv_len_init = false; > + } So, I'd like to get rid of this block with refactoring. > u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE | > - (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); > + (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) | > + I2C_FUNC_SMBUS_READ_BLOCK_DATA; Still not using the new macro to include PROC_CALL, but that is easy to change. All the best, Wolfram
Attachment:
signature.asc
Description: PGP signature
