linux-kernel - Re: [PATCH v5 3/4] i2c: tegra: Add logic to support different register offsets

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <27c283ca-a7d8-41eb-8fed-0ee9d08f26c5@nvidia.com>
Date: Thu, 8 Jan 2026 08:53:37 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Kartik Rajput <kkartik@...dia.com>, ldewangan@...dia.com,
 digetx@...il.com, andi.shyti@...nel.org, thierry.reding@...il.com,
 akhilrajeev@...dia.com, smangipudi@...dia.com, linux-i2c@...r.kernel.org,
 linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] i2c: tegra: Add logic to support different
 register offsets


On 08/01/2026 08:33, Kartik Rajput wrote:

...

>>> @@ -159,6 +137,149 @@
>>>    */
>>>   #define I2C_PIO_MODE_PREFERRED_LEN        32
>>> +struct tegra_i2c_regs {
>>> +    unsigned int cnfg;
>>> +    unsigned int status;
>>> +    unsigned int sl_cnfg;
>>> +    unsigned int sl_addr1;
>>> +    unsigned int sl_addr2;
>>> +    unsigned int tlow_sext;
>>> +    unsigned int tx_fifo;
>>> +    unsigned int rx_fifo;
>>> +    unsigned int packet_transfer_status;
>>> +    unsigned int fifo_control;
>>> +    unsigned int fifo_status;
>>> +    unsigned int int_mask;
>>> +    unsigned int int_status;
>>> +    unsigned int clk_divisor;
>>> +    unsigned int bus_clear_cnfg;
>>> +    unsigned int bus_clear_status;
>>> +    unsigned int config_load;
>>> +    unsigned int clken_override;
>>> +    unsigned int interface_timing_0;
>>> +    unsigned int interface_timing_1;
>>> +    unsigned int hs_interface_timing_0;
>>> +    unsigned int hs_interface_timing_1;
>>> +    unsigned int master_reset_cntrl;
>>> +    unsigned int mst_fifo_control;
>>> +    unsigned int mst_fifo_status;
>>> +    unsigned int sw_mutex;
>>> +    unsigned int dvc_ctrl_reg1;
>>> +    unsigned int dvc_ctrl_reg3;
>>> +    unsigned int dvc_status;
>>> +};
>>> +
>>> +static const struct tegra_i2c_regs tegra20_i2c_regs = {
>>> +    .cnfg = 0x000,
>>> +    .status = 0x01c,
>>> +    .sl_cnfg = 0x020,
>>> +    .sl_addr1 = 0x02c,
>>> +    .sl_addr2 = 0x030,
>>> +    .tx_fifo = 0x050,
>>> +    .rx_fifo = 0x054,
>>> +    .packet_transfer_status = 0x058,
>>> +    .fifo_control = 0x05c,
>>> +    .fifo_status = 0x060,
>>> +    .int_mask = 0x064,
>>> +    .int_status = 0x068,
>>> +    .clk_divisor = 0x06c,
>>> +    .bus_clear_cnfg = 0x084,
>>> +    .bus_clear_status = 0x088,
>>> +    .config_load = 0x08c,
>>> +    .clken_override = 0x090,
>>> +    .interface_timing_0 = 0x094,
>>> +    .interface_timing_1 = 0x098,
>>> +    .hs_interface_timing_0 = 0x09c,
>>> +    .hs_interface_timing_1 = 0x0a0,
>>> +    .master_reset_cntrl = 0x0a8,
>>> +    .mst_fifo_control = 0x0b4,
>>> +    .mst_fifo_status = 0x0b8,
>>> +};
>>> +
>>> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC)
>>> +static const struct tegra_i2c_regs tegra20_dvc_i2c_regs = {
>>> +    .dvc_ctrl_reg1 = 0x000,
>>> +    .dvc_ctrl_reg3 = 0x008,
>>> +    .dvc_status = 0x00c,
>>
>> Why do we need to add the DVC? These don't change.
>>
> 
> For consistency, as we are consolidating all the regs in `struct 
> tegra_i2c_regs` and moving away from static macro defines.

I am really not sure we need to do this. DVC is only applicable to the 
Tegra20 devices and no other devices use this. Hence, adding this for 
every device since then seems wasteful. I prefer we don't include this.

>>> +    .cnfg = 0x040,
>>> +    .status = 0x05c,
>>> +    .tx_fifo = 0x060,
>>> +    .rx_fifo = 0x064,
>>> +    .packet_transfer_status = 0x068,
>>> +    .fifo_control = 0x06c,
>>> +    .fifo_status = 0x070,
>>> +    .int_mask = 0x074,
>>> +    .int_status = 0x078,
>>> +    .clk_divisor = 0x07c,
>>> +    .bus_clear_cnfg = 0x0c4,
>>
>> Shouldn't this be 0x094? We are only adding 0x10 if greater or equal 
>> to TX_FIFO.
>>
> 
> You're right, I will fix this in the next patch set.


Thinking about this some more I had a tough time reviewing this and feel 
that this transition is error prone. I would prefer if we kept the 
current definitions and then just ...

static const struct tegra_i2c_regs tegra20_i2c_regs = {
     .cnfg = I2C_CNFG,
     ...
};

static const struct tegra_i2c_regs tegra20_dvc_i2c_regs = {
     .cnfg = DVC_OFFSET(I2C_CNFG),
     ...
};

static const struct tegra_i2c_regs tegra210_vi_i2c_regs = {
     .cnfg = VI_OFFSET(I2C_CNFG),
     ...
};

Where the macros DVC_OFFSET and VI_OFFSET handle the conversion. This 
way it will be simpler to review and less error prone.

Jon

-- 
nvpublic