Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Doug Berger]

On 03/13/2017 07:43 PM, Andrew Lunn wrote:
> On Mon, Mar 13, 2017 at 07:06:25PM -0700, Doug Berger wrote:
>> On 03/13/2017 06:06 PM, Andrew Lunn wrote:
>>> On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
>>>> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	/* set shadow mode 2 */
>>>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
>>>> +			       MII_BCM7XXX_SHD_MODE_2, 0);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
>>>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
>>>> +	if (ret < 0)
>>>> +		goto reset_shadow_mode;
>>>> +
>>>> +	/* Cal reset */
>>>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>>>> +			MII_BCM7XXX_SHD_3_TL4);
>>>> +	if (ret < 0)
>>>> +		goto reset_shadow_mode;
>>>
>>> Hi Doug
>>>
>>> It would be nice to have a few blank lines here and there...
>>>
>> Thanks for taking the time to review this.
>>
>> In general I try to keep lines of related functionality together and use
>> the blank lines to help identify boundaries.  In this particular case, I
>> believe it is clearer to keep the code that may return an error code
>> together with the code that tests for the error.
> 
> Hi Doug
> 
> I agree with that. Which is why i placed the comment between the goto
> and the next block of code. This is where i think there should be a
> blank line, to separate it from setting the trim values.
> 
OK, I see.  I thought you were referring to the code blocks above the
comment.  In that case, as described earlier, the code below the comment
is tightly coupled with the code above the comment since the pair of
transactions are how we "/* Cal reset */".  The idea of introducing a
subroutine/helper function for these paired (addr/data) transactions
might help readability so I will consider it for a future patch.

Thanks again for the feedback,
    Doug