Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Sep 2022 16:59:51 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Jan Dabros <jsd@semihalf.com>
Cc:     linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
        jarkko.nikula@linux.intel.com, wsa@kernel.org,
        rrangel@chromium.org, upstream@semihalf.com,
        mario.limonciello@amd.com
Subject: Re: [PATCH -next 2/2] i2c: designware: Add support for new SoCs in
 AMDPSP driver
Message-ID: <YySdjEXHqvTL8mEl@smile.fi.intel.com>
References: <20220916131854.687371-1-jsd@semihalf.com>
 <20220916131854.687371-3-jsd@semihalf.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220916131854.687371-3-jsd@semihalf.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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