Re: [PATCH v9 2/4] i2c: tegra: Add HS mode support

[Jon Hunter <jonathanh@nvidia.com>]

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