Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 28.06.2024 10:55, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: claudiu beznea <claudiu.beznea@tuxon.dev>
>> Sent: Friday, June 28, 2024 8:32 AM
>> Subject: Re: [PATCH v2 07/12] i2c: riic: Define individual arrays to describe the register offsets
>>
>> Hi, Biju,
>>
>> On 28.06.2024 08:59, Biju Das wrote:
>>> Hi Claudiu,
>>>
>>>> -----Original Message-----
>>>> From: Claudiu <claudiu.beznea@tuxon.dev>
>>>> Sent: Tuesday, June 25, 2024 1:14 PM
>>>> Subject: [PATCH v2 07/12] i2c: riic: Define individual arrays to
>>>> describe the register offsets
>>>>
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Define individual arrays to describe the register offsets. In this
>>>> way we can describe different IP variants that share the same
>>>> register offsets but have differences in other characteristics. Commit prepares for the addition
>> of fast mode plus.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - none
>>>>
>>>>  drivers/i2c/busses/i2c-riic.c | 58
>>>> +++++++++++++++++++----------------
>>>>  1 file changed, 31 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-riic.c
>>>> b/drivers/i2c/busses/i2c-riic.c index
>>>> 9fe007609076..8ffbead95492 100644
>>>> --- a/drivers/i2c/busses/i2c-riic.c
>>>> +++ b/drivers/i2c/busses/i2c-riic.c
>>>> @@ -91,7 +91,7 @@ enum riic_reg_list {  };
>>>>
>>>>  struct riic_of_data {
>>>> -	u8 regs[RIIC_REG_END];
>>>> +	const u8 *regs;
>>>
>>>
>>> Since you are touching this part, can we drop struct and Use u8* as
>>> device_data instead?
>>
>> Patch 09/12 "i2c: riic: Add support for fast mode plus" adds a new member to struct riic_of_data.
>> That new member is needed to differentiate b/w hardware versions supporting fast mode plus based on
>> compatible.
> 
> Are we sure RZ/A does not support fast mode plus?

From commit description of patch 09/12:

Fast mode plus is available on most of the IP variants that RIIC driver
is working with. The exception is (according to HW manuals of the SoCs
where this IP is available) the Renesas RZ/A1H. For this, patch
introduces the struct riic_of_data::fast_mode_plus.

I checked the manuals of all the SoCs where this driver is used.

I haven't checked the H/W manual?

On the manual I've downloaded from Renesas web site the FMPE bit of
RIICnFER is not available on RZ/A1H.

Thank you,
Claudiu Beznea

> If it does not, then it make sense to keep the patch as it is.
> 
> Cheers,
> Biju
> 
>>
>> Keeping struct riic_of_data is necessary (unless I misunderstood your proposal).
>>
>> Thank you,
>> Claudiu Beznea
>>
>>>
>>> ie, replace const struct riic_of_data *info->const u8 *regs in struct
>>> riic_dev and use .data = riic_rz_xx_regs in of_match_table?
>>>
>>> Cheers,
>>> Biju
>>>>  };
>>>>
>>>>  struct riic_dev {
>>>> @@ -531,36 +531,40 @@ static void riic_i2c_remove(struct platform_device *pdev)
>>>>  	pm_runtime_dont_use_autosuspend(dev);
>>>>  }
>>>>
>>>> +static const u8 riic_rz_a_regs[RIIC_REG_END] = {
>>>> +	[RIIC_ICCR1] = 0x00,
>>>> +	[RIIC_ICCR2] = 0x04,
>>>> +	[RIIC_ICMR1] = 0x08,
>>>> +	[RIIC_ICMR3] = 0x10,
>>>> +	[RIIC_ICSER] = 0x18,
>>>> +	[RIIC_ICIER] = 0x1c,
>>>> +	[RIIC_ICSR2] = 0x24,
>>>> +	[RIIC_ICBRL] = 0x34,
>>>> +	[RIIC_ICBRH] = 0x38,
>>>> +	[RIIC_ICDRT] = 0x3c,
>>>> +	[RIIC_ICDRR] = 0x40,
>>>> +};
>>>> +
>>>>  static const struct riic_of_data riic_rz_a_info = {
>>>> -	.regs = {
>>>> -		[RIIC_ICCR1] = 0x00,
>>>> -		[RIIC_ICCR2] = 0x04,
>>>> -		[RIIC_ICMR1] = 0x08,
>>>> -		[RIIC_ICMR3] = 0x10,
>>>> -		[RIIC_ICSER] = 0x18,
>>>> -		[RIIC_ICIER] = 0x1c,
>>>> -		[RIIC_ICSR2] = 0x24,
>>>> -		[RIIC_ICBRL] = 0x34,
>>>> -		[RIIC_ICBRH] = 0x38,
>>>> -		[RIIC_ICDRT] = 0x3c,
>>>> -		[RIIC_ICDRR] = 0x40,
>>>> -	},
>>>> +	.regs = riic_rz_a_regs,
>>>> +};
>>>> +
>>>> +static const u8 riic_rz_v2h_regs[RIIC_REG_END] = {
>>>> +	[RIIC_ICCR1] = 0x00,
>>>> +	[RIIC_ICCR2] = 0x01,
>>>> +	[RIIC_ICMR1] = 0x02,
>>>> +	[RIIC_ICMR3] = 0x04,
>>>> +	[RIIC_ICSER] = 0x06,
>>>> +	[RIIC_ICIER] = 0x07,
>>>> +	[RIIC_ICSR2] = 0x09,
>>>> +	[RIIC_ICBRL] = 0x10,
>>>> +	[RIIC_ICBRH] = 0x11,
>>>> +	[RIIC_ICDRT] = 0x12,
>>>> +	[RIIC_ICDRR] = 0x13,
>>>>  };
>>>>
>>>>  static const struct riic_of_data riic_rz_v2h_info = {
>>>> -	.regs = {
>>>> -		[RIIC_ICCR1] = 0x00,
>>>> -		[RIIC_ICCR2] = 0x01,
>>>> -		[RIIC_ICMR1] = 0x02,
>>>> -		[RIIC_ICMR3] = 0x04,
>>>> -		[RIIC_ICSER] = 0x06,
>>>> -		[RIIC_ICIER] = 0x07,
>>>> -		[RIIC_ICSR2] = 0x09,
>>>> -		[RIIC_ICBRL] = 0x10,
>>>> -		[RIIC_ICBRH] = 0x11,
>>>> -		[RIIC_ICDRT] = 0x12,
>>>> -		[RIIC_ICDRR] = 0x13,
>>>> -	},
>>>> +	.regs = riic_rz_v2h_regs,
>>>>  };
>>>>
>>>>  static int riic_i2c_suspend(struct device *dev)
>>>> --
>>>> 2.39.2
>>>>
>>>