linux-kernel - Re: [PATCH v9 2/4] i2c: tegra: Add HS mode 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: <17d683d1-c306-4860-acc2-9c09bbce0d55@nvidia.com>
Date: Thu, 6 Nov 2025 16:02:36 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Akhil R <akhilrajeev@...dia.com>
Cc: andi.shyti@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org,
 digetx@...il.com, kkartik@...dia.com, krzk+dt@...nel.org,
 ldewangan@...dia.com, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org, robh@...nel.org,
 thierry.reding@...il.com
Subject: Re: [PATCH v9 2/4] i2c: tegra: Add HS mode support


On 06/11/2025 06:17, Akhil R wrote:
> On Fri, 24 Oct 2025 16:28:50 +0100, Jon Hunter wrote:
>> On 01/10/2025 07:47, Kartik Rajput wrote:
> 
> ...
> 
>>>    /**
>>> @@ -678,16 +685,28 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>>>                 tegra_i2c_vi_init(i2c_dev);
>>>   
>>>         switch (t->bus_freq_hz) {
>>> -     case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
>>>         default:
>>> +             if (!i2c_dev->hw->has_hs_mode_support)
>>> +                     t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
>>> +             fallthrough;
>>> +
>>>
>> This looks odd. I guess this is carry over from the previous code, but
>> now it looks very odd to someone reviewing the code after this change
>> has been made. We need to make the code here more logical so that the
>> reader stands a chance of understanding the new logic.
> 
> Would it look better if I update as below?
> 
> @@ -678,8 +685,26 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>                  tegra_i2c_vi_init(i2c_dev);
>   
>          switch (t->bus_freq_hz) {
> -       case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
>          default:

I can't say I am a fan of this 'default' part. I don't find this very 
clear at all.

> +               /*
> +                * When HS mode is supported, the non-hs timing registers will be used for the
> +                * master code byte for transition to HS mode. As per the spec, the 8 bit master
> +                * code should be sent at max 400kHz. Therefore, limit the bus speed to fast mode.
> +                * Whereas when HS mode is not supported, allow the highest speed mode capable.
> +                */
> +               if (i2c_dev->hw->has_hs_mode_support) {
> +                       tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
> +                       thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
> +                       tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
> +                       non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
> +
> +                       break;
> +               } else {
> +                       t->bus_freq_hz = I2C_MAX_FAST_MODE_PLUS_FREQ;
> +               }
> +               fallthrough;
> +
> +       case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
>                  tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
>                  thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
>                  tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;


Looking at this code are we better off converting this to a simple 
if-statement?

So we have ...

     if (t->bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
         tlow = i2c_dev->hw->tlow_std_mode;
         thigh = i2c_dev->hw->thigh_std_mode;
         tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
         non_hs_mode = i2c_dev->hw->clk_divisor_std_mode;
     } else {
         ...
     }

Jon

-- 
nvpublic