From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 684004DC7C for ; Tue, 1 Aug 2023 15:20:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 224C1C433C9; Tue, 1 Aug 2023 15:20:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1690903227; bh=S/yh4HFwSMKveBH7hjxGoJ0RILF31mQprfeX/QR7adw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=IMNAd8WNqR7fhcTMzEQSUIa5VmK1a8jm9JXX+D0lRJGb7jbrisVJPkWNSoDgE7wOx Ke+R5iMvLsGiLC+LRZny1cThYLu2k8ndFT2LA+RqzReI7kZk5+ocbsBdfxMLitMZ2f dlHXQt2+wFrm09Gle72M+pbcreDfAngpUXQ9QLZ/XFzQsZDEzBgL3B95v0Yf2l6mdT KmSPBNfPDNpsgyk2HBJ4ysAiz0KH9sDaeOesAAzjom5yU2l6iA8mxBcwCQ9Ob6q43y A0nrdpBGYg1RHo4/NTMbojQc/Z/gUPtTdXSoN/im/otr7bHBsxmlXODbfZuOWk1S/e L8BjiNlhiRAIg== Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Date: Tue, 01 Aug 2023 17:20:22 +0200 From: Michael Walle To: "Russell King (Oracle)" Cc: Andrew Lunn , Heiner Kallweit , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Yisen Zhuang , Salil Mehta , Florian Fainelli , Broadcom internal kernel review list , =?UTF-8?Q?Marek_B?= =?UTF-8?Q?eh=C3=BAn?= , Xu Liang , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Simon Horman Subject: Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45_registers() In-Reply-To: References: <20230620-feature-c45-over-c22-v3-0-9eb37edf7be0@kernel.org> <20230620-feature-c45-over-c22-v3-2-9eb37edf7be0@kernel.org> <7be8b305-f287-4e99-bddd-55646285c427@lunn.ch> <867ae3cc05439599d63e4712bca79e27@kernel.org> Message-ID: X-Sender: mwalle@kernel.org Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Hi, >> > > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c >> > > > index a64186dc53f8..686a57d56885 100644 >> > > > --- a/drivers/net/phy/phy-core.c >> > > > +++ b/drivers/net/phy/phy-core.c >> > > > @@ -556,7 +556,7 @@ int __phy_read_mmd(struct phy_device >> > > > *phydev, int devad, u32 regnum) >> > > > >> > > > if (phydev->drv && phydev->drv->read_mmd) { >> > > > val = phydev->drv->read_mmd(phydev, devad, regnum); >> > > > - } else if (phydev->is_c45) { >> > > > + } else if (phy_has_c45_registers(phydev)) { >> > > >> > > This i would say should be >> > > >> > > phy_has_c45_transfers(phydev). This is about, can we do C45 transfers >> > > on the bus, and if not, fall back to C45 over C22. >> > >> > Shouldn't this then be a bus property? I.e. mdiobus_has_c45_transfers(). >> > I've have a similar helper introduced in 9/11: >> > >> > static inline bool mdiobus_supports_c45(struct mii_bus *bus) >> > { >> > return bus->read_c45 && !bus->prevent_c45_access; >> > } > > In the case of the above (the code in __phy_read_mmd()), I wouldn't > at least initially change the test there. > > phydev->is_c45 will only be true if we probed the PHY using clause > 45 accesses. Thus, it will be set if "the bus supports clause 45 > accesses" _and_ "the PHY responds to those accesses". > > Changing that to only "the bus supports clause 45 accesses" means > that a PHY supporting only clause 22 access with indirect clause > 45 access then fails if it's used with a bus that supports both > clause 22 and clause 45 accesses. Yeah of course. It was more about the naming, but I just realized that with mdiobus_supports_c45() you can't access the original "is_c45" property of the PHY. So maybe this patch needs to be split into two to get rid of .is_c45: First a mechanical one: phy_has_c45_registers() { return phydev->is_c45; } phy_has_c22_registers() { return !phydev->is_c45; } For all the places Andrew said it's correct. Leave all the other uses of .is_c45 as is for now and rework them in a later patch to use mdiobus_supports_{c22,c45}(). -michael