linux-kernel - Re: [PATCH v2] i2c-mlxbf: Add repeated start condition 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <db3daq7guy52ywmgslnmphx4mxyp2l7swq2y6erezokolg3v6u@p7ufvbi3tarc>
Date: Thu, 9 Jan 2025 16:09:46 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Chris Babroski <cbabroski@...dia.com>
Cc: kblaiech@...dia.com, asmaa@...dia.com, davthompson@...dia.com, 
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c-mlxbf: Add repeated start condition support

Hi Chris,

Can I have an ack from the other nvidia guys, please? Khalil and
Asmaa?

...

> @@ -694,16 +695,19 @@ static void mlxbf_i2c_smbus_read_data(struct mlxbf_i2c_priv *priv,
>  }
>  
>  static int mlxbf_i2c_smbus_enable(struct mlxbf_i2c_priv *priv, u8 slave,
> -				  u8 len, u8 block_en, u8 pec_en, bool read)
> +				  u8 len, u8 block_en, u8 pec_en, bool read, bool no_stop)

I don't really like how the alignment turned out here. This file
rarely exceeds 80 characters, so can we keep it that way?

>  {
> -	u32 command;
> +	u32 command = 0;
>  
>  	/* Set Master GW control word. */
> +	if (!no_stop)
> +		command |= MLXBF_I2C_MASTER_STOP_BIT;

If "no stop" we enable the stop bit? Can we make it a bit more
straight?

> +

...

> -/*
> - * Note that the mlxbf_i2c_timings->timeout value is not related to the
> - * bus frequency, it is impacted by the time it takes the driver to
> - * complete data transmission before transaction abort.
> - */
> +/* Timing values are in nanoseconds */
>  static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = {
>  	[MLXBF_I2C_TIMING_CONFIG_100KHZ] = {
>  		.scl_high = 4810,
> @@ -1203,8 +1218,8 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = {
>  		.scl_fall = 50,
>  		.hold_data = 300,
>  		.buf = 20000,
> -		.thigh_max = 5000,
> -		.timeout = 106500
> +		.thigh_max = 50000,
> +		.timeout = 35000000
>  	},
>  	[MLXBF_I2C_TIMING_CONFIG_400KHZ] = {
>  		.scl_high = 1011,
> @@ -1219,24 +1234,24 @@ static const struct mlxbf_i2c_timings mlxbf_i2c_timings[] = {
>  		.scl_fall = 50,
>  		.hold_data = 300,
>  		.buf = 20000,
> -		.thigh_max = 5000,
> -		.timeout = 106500
> +		.thigh_max = 50000,
> +		.timeout = 35000000
>  	},
>  	[MLXBF_I2C_TIMING_CONFIG_1000KHZ] = {
> -		.scl_high = 600,
> -		.scl_low = 1300,
> +		.scl_high = 383,
> +		.scl_low = 460,
>  		.hold_start = 600,
> -		.setup_start = 600,
> -		.setup_stop = 600,
> -		.setup_data = 100,
> +		.setup_start = 260,
> +		.setup_stop = 260,
> +		.setup_data = 50,
>  		.sda_rise = 50,
>  		.sda_fall = 50,
>  		.scl_rise = 50,
>  		.scl_fall = 50,
>  		.hold_data = 300,
> -		.buf = 20000,
> -		.thigh_max = 5000,
> -		.timeout = 106500
> +		.buf = 500,
> +		.thigh_max = 50000,
> +		.timeout = 35000000

Can you please explain better how the stop bit affects the timing
values?

Thanks,
Andi