Messages in this thread | | | Date | Fri, 1 Apr 2022 18:38:56 +0200 | From | Wolfram Sang <> | Subject | Re: [PATCH v2] i2c: rcar: add SMBus block read support |
| |
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
[unhandled content-type:application/pgp-signature] | |