Re: [PATCH] i2c: mediatek: fix WRRD for SoCs without auto_restart option

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/9/26 14:12, Roman Vivchar wrote:
> Hi AngeloGioacchino,
> 
> On Thursday, July 9th, 2026 at 2:46 PM, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> On 6/23/26 23:40, Roman Vivchar via B4 Relay wrote:
>>> From: Roman Vivchar <rva333@protonmail.com>
>>>
>>> MediaTek mt65xx family SoCs have no auto restart, however, they still
>>> support the WRRD mode in the hardware. Because auto_restart is set to 0,
>>> the WRRD mode will be never enabled, leading to read errors.
>>>
>>> Fix this by removing auto_restart check from the WRRD enable path.
>>>
>>
>> Are you sure that MT6577 and MT6589 do support WRRD?
> 
> Yes, I have checked their downstream kernels[1][2] and they both mention
> WRRD.
> 
> We also tested mt6589 (privately) and the i2c was working. To be fair,
> it had some other fixes, but at worst this patch is no-op.
> 
> mt6577 is a tough one... I don't think there's any recent kernel with
> it. I plan to make it work with upstream, but that's a TODO. Either way,
> vendor code has WRRD support.
> 
>> If you're not 100% sure, or you want to be careful (which I would, in this case),
>> please add a `unsigned char wrrd_supported:1` to struct mtk_i2c_compatible, set
>> it to `1` on *all* of the SoCs that support auto_restart (as that's effectively
>> a different feature - it just happens that all the ones supporting this also do
>> support that, up until today), and add a new one for your MT6572, declaring no
>> auto_restart but wrrd_supported.
>>
>> That would make me sure that you're not breaking anything, and would guarantee
>> you a R-b.
>>
> 
> As far as I know, there's no SoC without both auto restart and WRRD.
> Adding wrrd_supported that always equals to true sounds redundant to
> me.

I have just verified on datasheets.
You're right, both MT6577 and MT6589 do support WRRD, hence your change is good.

With this commit, you're fixing a fix... Leilk from MediaTek was indeed wrong, as
in the commit description he says "the old IC does not support I2C_MASTER_WRRD",
and that's *very* wrong.

I can see though what he did, as he inadvertently also fixed the WRRD algo because
the fix was to increment msgs by 2 instead of 1 when op==WRRD.


Please, resend your commit as a v2, and add:

Fixes: b49218365280 ("i2c: mediatek: fix potential incorrect use of I2C_MASTER_WRRD")

and also
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers!

> 
> [1]: https://github.com/GoldRenard/lenovo_A390_mt6577_uboot_kernel/blob/38ea59263984e26c0dd8e5891b22a35f61e60ed5/mediatek/platform/mt6577/kernel/drivers/i2c/i2c.c#L963-L975
> [2]: https://github.com/bq/aquaris-5/blob/bc1d0d0e35c969670e2d869001be944ad0c9c5e5/mediatek/platform/mt6589/kernel/drivers/i2c/i2c.c#L728
> 
> Best regards,
> Roman