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

[Akhil R]

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:
+               /*
+                * 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;
@@ -688,6 +713,7 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)

...

>> @@ -717,6 +736,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>>   	if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
>>   		i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
>>   
>>   
>> +	/* Write HS mode registers. These will get used only for HS mode*/
>> +	if (i2c_dev->hw->has_hs_mode_support) {
>> +		tlow = i2c_dev->hw->tlow_hs_mode;
>> +		thigh = i2c_dev->hw->thigh_hs_mode;
>> +		tsu_thd = i2c_dev->hw->setup_hold_time_hs_mode;
>> +
>> +		val = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH, thigh) |
>> +			FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW, tlow);
>> +		i2c_writel(i2c_dev, val, I2C_HS_INTERFACE_TIMING_0);
>> +		i2c_writel(i2c_dev, tsu_thd, I2C_HS_INTERFACE_TIMING_1);
>> +	}
>> +
>
> I still think all of the above needs a bit of work.

I suppose the above section can be as is since HS mode registers are independent
of other speed modes. Any suggestions or thoughts?

Regards,
Akhil