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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cd2c9d3c-b2b1-49f4-a427-16592be1cf0d@quicinc.com>
Date: Thu, 9 Jan 2025 18:07:32 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: Kartik Rajput <kkartik@...dia.com>, <akhilrajeev@...dia.com>,
        <andi.shyti@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <jonathanh@...dia.com>, <ldewangan@...dia.com>,
        <digetx@...il.com>, <linux-i2c@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] i2c: tegra: Add Tegra264 support

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@...dia.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@...dia.com>
>>> Signed-off-by: Kartik Rajput <kkartik@...dia.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