Received-SPF: pass (google.com: domain of linux-kernel+bounces-158778-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 25 Apr 2024 17:13:54 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Sky Huang <SkyLake.Huang@mediatek.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Daniel Golle <daniel@makrotopia.org>,
	Qingfang Deng <dqfext@gmail.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Steven Liu <Steven.Liu@mediatek.com>
Subject: Re: [PATCH 2/3] net: phy: mediatek: Add mtk phy lib for token ring
 access & LED/other manipulations
Message-ID: <d6c62ab9-256b-47d2-8145-7dc6ffcbd020@lunn.ch>
References: <20240425023325.15586-1-SkyLake.Huang@mediatek.com>
 <20240425023325.15586-3-SkyLake.Huang@mediatek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240425023325.15586-3-SkyLake.Huang@mediatek.com>

On Thu, Apr 25, 2024 at 10:33:24AM +0800, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> 
> Integrate some common phy manipulations in mtk-phy-lib:
> 1. Token ring access: This is proprietary register access on page 52b5.
> Use these APIs so we can see which fields we're going to modify/set/clear.
> 2. LED: External/internal giga & internal 2.5g phy share almost the same
> LED control registers/logic.
> 3. Extend 1G TX/RX link pulse time: We observe that some devices' 1G
> training time violates specification, which may last 2230ms and affect
> later TX/RX link pulse time. So we try to extend our 1G TX/RX link pulse
> time so that we can still detect such devices.

Please break this up into 3 patches.

> -#define MTK_PHY_PAGE_EXTENDED_52B5		0x52b5
> +
> +/* Registers on Token Ring debug nodes */
> +/* ch_addr = 0x0, node_addr = 0x7, data_addr = 0x15 */
> +#define NORMAL_MSE_LO_THRESH_MASK		GENMASK(15, 8) /* NormMseLoThresh */
> +
> +/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x3c */
> +#define REMOTE_ACK_COUNT_LIMIT_CTRL_MASK	GENMASK(2, 1) /* RemAckCntLimitCtrl */
> +
> +/* ch_addr = 0x1, node_addr = 0xd, data_addr = 0x20 */
> +#define VCO_SLICER_THRESH_HIGH_MASK		GENMASK(23, 0) /* VcoSlicerThreshBitsHigh */
> +
> +/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x0 */
> +#define DFE_TAIL_EANBLE_VGA_TRHESH_1000		GENMASK(5, 1) /* DfeTailEnableVgaThresh1000 */
> +
> +/* ch_addr = 0x1, node_addr = 0xf, data_addr = 0x1 */
> +#define MRVL_TR_FIX_100KP_MASK			GENMASK(22, 20) /* MrvlTrFix100Kp */
> +#define MRVL_TR_FIX_100KF_MASK			GENMASK(19, 17) /* MrvlTrFix100Kf */
> +#define MRVL_TR_FIX_1000KP_MASK			GENMASK(16, 14) /* MrvlTrFix1000Kp */
> +#define MRVL_TR_FIX_1000KF_MASK			GENMASK(13, 11) /* MrvlTrFix1000Kf */

This is not what i would expect for moving code into a library. I
expect code to be deleted from one file and appear in another file,
with some minor changes, static dropped, EXPORT_SYMBOL_PHY added.  If
i see code just move, i don't need to review it line for line. If you
do want to change the code, please move it into the library first, and
make modifications.

Lots of small patches, which are obviously correct.

	Andrew