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

[Jon Hunter]

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