Re: [PATCH v4 5/9] i2c: piix4: Move SMBus port selection into function

[Jean Delvare <jdelvare@suse.de>]

On Sun, 30 Jan 2022 12:41:26 -0600, Terry Bowman wrote:
> Move port selection code into a separate function. Refactor is in
> preparation for following MMIO changes.
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index eab06e9e5fdf..32a30af5778a 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -699,6 +699,19 @@ static void piix4_imc_wakeup(void)
>  	release_region(KERNCZ_IMC_IDX, 2);
>  }
>  
> +static int piix4_sb800_port_sel(u8 port)
> +{
> +	u8 smba_en_lo, val;
> +
> +	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
> +	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +
> +	val = (smba_en_lo & ~piix4_port_mask_sb800) | port;
> +	if (smba_en_lo != val)
> +		outb_p(val, SB800_PIIX4_SMB_IDX + 1);
> +
> +	return (smba_en_lo & piix4_port_mask_sb800);
> +}
>  /*
>   * Handles access to multiple SMBus ports on the SB800.
>   * The port is selected by bits 2:1 of the smb_en register (0x2c).
> @@ -715,8 +728,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  	unsigned short piix4_smba = adapdata->smba;
>  	int retries = MAX_TIMEOUT;
>  	int smbslvcnt;
> -	u8 smba_en_lo;
> -	u8 port;
> +	u8 prev_port;
>  	int retval;
>  
>  	retval = piix4_sb800_region_request(&adap->dev);
> @@ -776,18 +788,12 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  		}
>  	}
>  
> -	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
> -	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> -
> -	port = adapdata->port;
> -	if ((smba_en_lo & piix4_port_mask_sb800) != port)
> -		outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port,
> -		       SB800_PIIX4_SMB_IDX + 1);
> +	prev_port = piix4_sb800_port_sel(adapdata->port);
>  
>  	retval = piix4_access(adap, addr, flags, read_write,
>  			      command, size, data);
>  
> -	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
> +	piix4_sb800_port_sel(prev_port);
>  
>  	/* Release the semaphore */
>  	outb_p(smbslvcnt | 0x20, SMBSLVCNT);

Hmm, this gets pretty inefficient. You set SB800_PIIX4_SMB_IDX to
piix4_port_sel_sb800 twice and compare the new port with the previous
port twice, even though the result will inevitably be the same. The
original code would just unconditionally restore the original port
value, which was also not always needed, but was cheaper in the end.

I can't really think of a better way though, so, so be it :-(

I wonder why we insist on restoring the port though. When you read
multiple values from the same bus (and I2C device drivers do that a
lot, obviously) you end up switching ports a lot for no good reason. I
looked at the git history and it has been that way since support for
the multiplexed buses was added back in 2015. There's a mutex
protecting that section of code anyway, so I don't think restoring the
port buys us anything we did not already have. The only benefit I can
see is to leave the port setting in its original state on suspend and
shutdown (and we know by experience that it can be important to the
BIOS) but then we should just do it in the suspend and remove
functions, instead of after every transfer.

-- 
Jean Delvare
SUSE L3 Support