Re: [PATCH v2 3/9] i2c: rk3x: Adjust offset for i2c2 on rv1126

[Tim Lunn <tim@feathertop.org>]

On 11/27/23 11:26, Heiko Stübner wrote:
> Hi Andi,
>
> Am Sonntag, 26. November 2023, 20:43:11 CET schrieb Andi Shyti:
>> Hi Tim,
>>
>> On Wed, Nov 22, 2023 at 11:22:26PM +1100, Tim Lunn wrote:
>>> Rockchip RV1126 has special case mask bits for i2c2.
>>>
>>> i2c2 wasnt previously enabled in rv1126.dtsi, adding DT node alone
>>> is not sufficient to enable i2c2. This patch fixes the i2c2 bus.
>> If I don't have sufficient information about the hardware this
>> description is completely meaningless to me.
>>
>>> Signed-off-by: Tim Lunn <tim@feathertop.org>
>>> ---
>>>
>>> Changes in v2:
>>> - i2c: clarify commit message
>>>
>>>   drivers/i2c/busses/i2c-rk3x.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>>> index a044ca0c35a1..151927466d1d 100644
>>> --- a/drivers/i2c/busses/i2c-rk3x.c
>>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>>> @@ -1288,8 +1288,11 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>>>   			return -EINVAL;
>>>   		}
>>>   
>>> -		/* 27+i: write mask, 11+i: value */
>>> -		value = BIT(27 + bus_nr) | BIT(11 + bus_nr);
>>> +		if (i2c->soc_data == &rv1126_soc_data && bus_nr == 2)
>>> +			value = BIT(20) | BIT(4);
>> Any chance to put a comment here as it is in the other
>> assignment?
>>
>> Are the two assignment mutually exclusive?
Yes they are mutually exclusive, and its only i2c2 that is 
non-sequential (as per Heikos description below).
>>
>> Heiko, any chance to take a look here?
> So the background is, that on some SoCs Rockchip implemented to
> different variants for the i2c controller. One new-style controller
> that they started using in rk3066 and are using even today.
>
> For these old socs they kept the "old" controller block as a sort
> of fallback if the new thing didn't work out, and the bit above is
> switching between the
>
> Hence that is limited to rk3066, rk3188 and seemingly the rv1126.
> And while the bits controlling the i2c controllers on the original socs
> are order sequentially in the grf register, the rv1126 seems to have
> those bits in non-consequtive places.
>
>
> So TL;DR the change itself is likely good, and hopefully there won't
> be any more of those, as all the new socs don't need this anymore.
rv1108 is also similar but different bits again (only going off the BSP 
sources).
I dont have hardware or the TRM to validate this on rv1108.
>
> I do agree with the request for a comment describing the issue
> in the code, but otherwise

I will fix this.

> Acked-by: Heiko Stuebner <heiko@sntech.de>
>
>