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;
Message-ID: <68ec9a1e-81d3-93b9-d68f-47df07db8965@quicinc.com>
Date:   Wed, 5 Jul 2023 16:35:28 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v1 1/2] net: phy: at803x: support qca8081 1G chip type
Content-Language: en-US
To:     Andrew Lunn <andrew@lunn.ch>
CC:     <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230704090016.7757-1-quic_luoj@quicinc.com>
 <20230704090016.7757-2-quic_luoj@quicinc.com>
 <66a5f898-e3fd-460a-b604-bb11a000e4e9@lunn.ch>
From:   Jie Luo <quic_luoj@quicinc.com>
In-Reply-To: <66a5f898-e3fd-460a-b604-bb11a000e4e9@lunn.ch>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/5/2023 7:24 AM, Andrew Lunn wrote:
> On Tue, Jul 04, 2023 at 05:00:15PM +0800, Luo Jie wrote:
>> The qca8081 1G chip version does not support 2.5 capability, which
>> is distinguished from qca8081 2.5G chip according to the bit0 of
>> register mmd7.0x901d.
>>
>> The fast retrain and master slave seed configs are only needed when
>> the 2.5G capability is supported.
>>
>> Switch to use genphy_c45_pma_read_abilities for .get_features API.
> 
> It is better to have lots of small patches, each doing one thing. If
> something regresses, a git bisect gives a much finer idea where the
> problem is. It is also easier to review small patches with good commit
> messages.
> 
> So please break this patch up.

Ok, i will split this patch to small patches, thanks for the suggestion.

> 
>> -	/* Configure lower ramdom seed to make phy linked as slave mode */
>> -	ret = qca808x_phy_ms_random_seed_set(phydev);
>> -	if (ret)
>> -		return ret;
>> +		/* Configure lower ramdom seed to make phy linked as slave mode */
>> +		ret = qca808x_phy_ms_random_seed_set(phydev);
>> +		if (ret)
>> +			return ret;
> 
> Shouldn't this depend on how MDIO_MMD_AN, MDIO_AN_T1_ADV_L bit
> MDIO_AN_T1_ADV_M_MST is set? Maybe the user wants it to prefer master
> rather than slave?
> 
> I know you are just trying to move code around, but it does seem like
> a good time to also improve the code.
> 
> FYI: net-next is closed at the moment. Officially you should post as
> RFC, or wait until it opens again.
> 
>    Andrew

Hi Andrew,
The master/slave configuration/status is only existed in MII reg9, 10 on 
qca8081 PHY, which is not existed in MDIO_MMD_AN or MDIO_MMD_PMAPMD, i 
will improve this code according to the user's prefer master/slave 
configuration in the next patch series.

Thanks for the reminder of close window, i will upload the patches after 
the open of next window.