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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Mar 2017 10:23:27 -0700
From:   Doug Berger <opendmb@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     f.fainelli@...il.com, robh+dt@...nel.org, mark.rutland@....com,
        davem@...emloft.net, rafal@...ecki.pl, xow@...gle.com,
        joel@....id.au, jon.mason@...adcom.com, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        pgynther@...gle.com, jaedon.shin@...il.com
Subject: Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm
 EPHY

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