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

[Eugeniu Rosca]

Hi Wolfram,

On Fri, Apr 01, 2022 at 06:38:56PM +0200, Wolfram Sang wrote:
> > 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?

The patch was worked on and tested collaboratively w/o submission.
The idea was to push it to LKML, once/after you are happy with it.

> > 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.

Thanks for the precious feedback. We've requested Renesas to revert the
obsolete BSP commit, based on your recommendation.

In general, the Renesas kernel always carries a set of patches with
non-mainlined changes, Fortunately, for i2c specifically (as opposed
to other subsystems), it is narrow enough to not raise major concerns:

$ git log --oneline v5.10.41..rcar-5.1.2 -- drivers/i2c/busses/i2c-rcar.c
6745303b2bfa i2c: rcar: Add support for r8a77961 (R-Car M3-W+)
3422d3131700 i2c: rcar: Support the suspend/resume
5680e77f2427 i2c: rcar: Tidy up the register order for hardware specification ver1.00.
41394ab7420f i2c: rcar: Fix I2C DMA transmission by setting sequence

> 
> > 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.

Thank you for the review comments. We are still working on a cleaner
solution. In case it comes from you first, we are very much keen to
give it a try on the target and report the results.

Best regards,
Eugeniu