Re: [PATCH -next 2/2] i2c: designware: Add support for new SoCs in AMDPSP driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Sep 16, 2022 at 03:18:54PM +0200, Jan Dabros wrote:
> New AMD SoCs are using different algorithm for x86-PSP communication,
> thus need to modify I2C arbitration driver. Since possible future
> revisions should have follow new approach, mark functions used only for
> Cezanne with "czn_" prefix.
> 
> While at it, remove redundant check by modifying psp_wait_cmd() to only
> check for MBOX_READY bit, since MBOX_CMD field being zero is verified by
> czn_psp_check_mbox_sts() later on in the sequence.

...

> -#define PSP_MBOX_CMD_OFFSET		0x3810570
> -#define PSP_MBOX_BUFFER_L_OFFSET	0x3810574
> -#define PSP_MBOX_BUFFER_H_OFFSET	0x3810578
> +#define CZN_PSP_MBOX_CMD_OFFSET		0x3810570
> +#define CZN_PSP_MBOX_BUFFER_L_OFFSET	0x3810574
> +#define CZN_PSP_MBOX_BUFFER_H_OFFSET	0x3810578

Can we avoid this renaming noise by putting the proper names in the first place
(in the first patch)? Respectively move the corresponding commit message piece
to there.

Or looking into the below maybe even split the renaming to a separate
non-functional change?

...

> +	if (req)
> +		ret = czn_psp_send_cmd(req);
> +	else
> +		ret = psp_send_cmd(i2c_req_type);

> +

Unnecessary blank line.

> +	if (ret)
>  		return -EIO;

Why you can't return the actual error code here?

...

>  	start = jiffies;
>  	ret = read_poll_timeout(psp_send_check_i2c_req, status,
>  				(status != -EBUSY),
>  				PSP_I2C_REQ_RETRY_DELAY_US,
>  				PSP_I2C_REQ_RETRY_CNT * PSP_I2C_REQ_RETRY_DELAY_US,
> -				0, req);
> +				0, req, i2c_req_type);

> +

Stray blank line addition.

>  	if (ret) {
>  		dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n",
>  			(i2c_req_type == PSP_I2C_REQ_ACQUIRE) ?

-- 
With Best Regards,
Andy Shevchenko