Re: [PATCH 3/5] i2c: tegra: Add Tegra264 support

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

Thanks Thierry !

On 1/9/2025 3:54 PM, Thierry Reding wrote:
> On Thu, Jan 09, 2025 at 10:13:48AM +0530, Mukesh Kumar Savaliya wrote:
>> Hi Kartik,
>>
>> On 1/8/2025 4:36 PM, Kartik Rajput wrote:
>>> From: Akhil R <akhilrajeev@nvidia.com>
>>>
>>> Add support for Tegra264 SoC which supports 17 generic I2C controllers,
>>> two of which are in the AON (always-on) partition of the SoC. Tegra264
>>> I2C supports all the features supported by Tegra194 I2C controllers.
>>>
>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
>>> ---
>>>    drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>> index 7b97c6d347ee..cf05937cb826 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -1646,7 +1646,35 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
>>>    	.has_hs_mode_support = true,
>>>    };
>>> +static const struct tegra_i2c_hw_feature tegra264_i2c_hw = {
>> I could see 7 controllers have been already added. And this may keep
>> growing.
> 
> I'm not sure I understand the concern here. This is IP that's been in
> use ever since the first Tegra chip was released about 15 years ago.
> It's quite normal that the list of supported hardware will grow over
> time. At the same time there will be occasional improvements of the
> hardware that require certain parameterization.
> 
yes, i understand it can grow with new controllers. Was trying to 
optimize the growing list with common fields.

Example: tegra30_i2c_hw and tegra20_i2c_hw has one field changing
from 20 fields. So was thinking after seeing this commonality.

One suggestion: can one structure be default and then delta can be 
overridden ?

No concern if no other way as you mentioned below.

>> Can we make either default set which is common for most of and change only
>> sepcific fields ?
> 
> It's difficult to do. These are const structures on purpose so that they
> can go into .rodata, so as such there's no good way to reuse defaults. I
> suppose we could do something like add preprocessor defines, but I doubt
> that they would make things any better (these are quite fine-grained, so
> macros would likely only cover one or two fields at a time).
> 
Sure. Let's wait for others opinion. I understand complexity.

>> Second option - read these fields from DT and overwrite default if it's
>> mentioned in DTSI.
> 
> Some information is already parsed from DT. What's in this structure can
> all be derived from the compatible string, hence why it's associated
> with the compatible string via the of_device_id table. Moreover, we
> cannot move any of this information out into device tree (at least not
> for existing chips) because it would break DT ABI.
> 
Got it.
>> Please review and see if this makes sense. what others say ?
> 
> I'm always open to suggestions, but I also don't see this as very
> problematic. It's data that is cleanly structured out, not difficult to
> maintain and doesn't take up a huge amount of space.
> 
I Agree.
> Thierry