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

Jon Hunter
2025-11-06T16:02:45Z

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