Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Coiby Xu <coiby.xu@gmail.com>
Date:   Thu, 24 Jun 2021 19:55:36 +0800
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     linux-staging@lists.linux.dev, netdev@vger.kernel.org,
        Benjamin Poirier <benjamin.poirier@gmail.com>,
        Shung-Hsi Yu <shung-hsi.yu@suse.com>,
        Manish Chopra <manishc@marvell.com>,
        "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" 
        <GR-Linux-NIC-Dev@marvell.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:CLANG/LLVM BUILD SUPPORT" 
        <clang-built-linux@googlegroups.com>
Subject: Re: [RFC 17/19] staging: qlge: fix weird line wrapping
Message-ID: <20210624115536.5y4oqzrbms63rjcy@Rk>
References: <20210621134902.83587-1-coiby.xu@gmail.com>
 <20210621134902.83587-18-coiby.xu@gmail.com>
 <20210622084611.GM1861@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20210622084611.GM1861@kadam>
Precedence: bulk

On Tue, Jun 22, 2021 at 11:46:11AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:49:00PM +0800, Coiby Xu wrote:
>> @@ -524,8 +523,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>>  		{
>>  			value = RT_IDX_DST_DFLT_Q | /* dest */
>>  				RT_IDX_TYPE_NICQ | /* type */
>> -				(RT_IDX_IP_CSUM_ERR_SLOT <<
>> -				RT_IDX_IDX_SHIFT); /* index */
>> +			(RT_IDX_IP_CSUM_ERR_SLOT
>> +			 << RT_IDX_IDX_SHIFT); /* index */
>
>The original is not great but the new indenting is definitely worse.
>It might look nicer with the comments moved in the front?  Why does
>RT_IDX_IDX_SHIFT have two IDX strings?

I'm not sure about it. Two IDX strings seems to be a typo.
>
>			/* value = dest | type | index; */
>			value = RT_IDX_DST_DFLT_Q |
>				RT_IDX_TYPE_NICQ  |
>				(RT_IDX_IP_CSUM_ERR_SLOT << RT_IDX_IDX_SHIFT);
>

This looks better! Thanks!

>
>>  			break;
>>  		}
>>  	case RT_IDX_TU_CSUM_ERR: /* Pass up TCP/UDP CSUM error frames. */
>> @@ -554,7 +553,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>>  		{
>>  			value = RT_IDX_DST_DFLT_Q |	/* dest */
>>  			    RT_IDX_TYPE_NICQ |	/* type */
>> -			    (RT_IDX_MCAST_MATCH_SLOT << RT_IDX_IDX_SHIFT);/* index */
>> +			(RT_IDX_MCAST_MATCH_SLOT
>> +			 << RT_IDX_IDX_SHIFT); /* index */
>
>Original is better.

I'll also move the comments in the front.
>
>>  			break;
>>  		}
>>  	case RT_IDX_RSS_MATCH:	/* Pass up matched RSS frames. */
>> @@ -648,15 +648,15 @@ static int qlge_read_flash_word(struct qlge_adapter *qdev, int offset, __le32 *d
>>  {
>>  	int status = 0;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
>> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
>> +				   FLASH_ADDR_ERR);
>>  	if (status)
>>  		goto exit;
>>  	/* set up for reg read */
>>  	qlge_write32(qdev, FLASH_ADDR, FLASH_ADDR_R | offset);
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
>> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
>> +				   FLASH_ADDR_ERR);
>>  	if (status)
>>  		goto exit;
>>  	/* This data is stored on flash as an array of
>> @@ -792,8 +792,8 @@ static int qlge_write_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 data)
>>  {
>>  	int status;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>>  	if (status)
>>  		return status;
>>  	/* write the data to the data reg */
>> @@ -811,15 +811,15 @@ int qlge_read_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 *data)
>>  {
>>  	int status = 0;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>
>Need a blank line after the declaration block.

Sure, I will fix it in next version.

>
>>  	if (status)
>>  		goto exit;
>>  	/* set up for reg read */
>>  	qlge_write32(qdev, XGMAC_ADDR, reg | XGMAC_ADDR_R);
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>>  	if (status)
>>  		goto exit;
>>  	/* get the data */
>
>regards,
>dan carpenter

-- 
Best regards,
Coiby