Re: [PATCH v2] i2c: rcar: add SMBus block read support

[Wolfram Sang]

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