Re: [PATCH v2 05/10] i2c: i801: add i801_simple_transaction(), complementing i801_block_transaction()

[Jean Delvare <jdelvare@suse.de>]

On Mon, 19 Dec 2022 19:17:03 +0100, Heiner Kallweit wrote:
> This patch factors out non-block pre/post processing to a new function
> i801_simple_transaction(), complementing existing function
> i801_block_transaction(). This makes i801_access() better readable.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - rename new function to i801_simple_transaction
> - code style fixes
> ---
>  drivers/i2c/busses/i2c-i801.c | 92 +++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 882cf5135..d934d410b 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -732,6 +732,59 @@ static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>  	outb_p((addr << 1) | (read_write & 0x01), SMBHSTADD(priv));
>  }
>  
> +/* Single value transaction function */
> +static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> +				   char read_write, int command)
> +{
> +	int xact, ret;
> +
> +	switch (command) {

I did not notice during my initial review, but this parameter name is
confusing, and this becomes even more obvious with patch 7 later. What
you name "command" here is named "size" in function i801_access(). And
what is named "command" there, you will name "hstcmd" in this function
in patch 7. I would prefer to keep the same names (and parameter order)
throughout the function stack for consistency.

Rest is OK.

-- 
Jean Delvare
SUSE L3 Support