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 EA33B3D8B for ; Wed, 19 Jul 2023 07:22:02 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AAC3EC433C8; Wed, 19 Jul 2023 07:21:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1689751322; bh=3iiA3Y9v1L1k9xx1PzYcaYLgH17AKV0yDCqMSrfyEF4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=nmVKHiZsNnQMe/6FLFncveBM2z3lNTQkHbzhiUP8aLpud297muIMGvdjTef4SGS/k I3tf9OzQT50AQy3GiWpiG5MCsiQlF/hD9PxNnkTAeODaxjc0sB/Xf+S7SgREBeOh3q wmCT/NQAPpqImQw3Z5djwf9CoMiOeAwwHybpbYOPfTyFmBLWmF1eouxSUlKr1NZdCu 3UpnNQ+tI4fQKC6T1miwkiXnqdF0BvNHyZ/CnHZGclnPdU4tIf4M+YVg8UcaVBR89P dICY5VOfXeP9dOtWgc/NH7zQuqbAnAPerNf+oopqzjxR93iFNTSc9Ft/Ycl7Lzo7rC ypyNg8yF612kA== Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Date: Wed, 19 Jul 2023 09:21:57 +0200 From: Michael Walle To: Andrew Lunn Cc: Heiner Kallweit , Russell King , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Yisen Zhuang , Salil Mehta , Florian Fainelli , Broadcom internal kernel review list , =?UTF-8?Q?Marek_Beh=C3=BAn?= , Xu Liang , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Simon Horman Subject: Re: [PATCH net-next v3 07/11] net: phy: introduce phy_mdiobus_read_mmd() In-Reply-To: <3cfe5af5-31b6-492c-af55-cd0397b07eb1@lunn.ch> References: <20230620-feature-c45-over-c22-v3-0-9eb37edf7be0@kernel.org> <20230620-feature-c45-over-c22-v3-7-9eb37edf7be0@kernel.org> <3cfe5af5-31b6-492c-af55-cd0397b07eb1@lunn.ch> Message-ID: <8147c4e37adfa7b7d9ee95d242223f39@kernel.org> X-Sender: mwalle@kernel.org Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Am 2023-07-19 01:54, schrieb Andrew Lunn: >> +static int __phy_mdiobus_read_mmd(struct mii_bus *bus, int phy_addr, >> + enum phy_access_mode access_mode, >> + int devad, u32 regnum) >> +{ >> + switch (access_mode) { >> + case PHY_ACCESS_C45: >> + return __mdiobus_c45_read(bus, phy_addr, devad, regnum); >> + case PHY_ACCESS_C22: >> + /* ignore return value for legacy reasons */ >> + mmd_phy_indirect(bus, phy_addr, devad, regnum, false); >> + >> + /* Read the content of the MMD's selected register */ >> + return __mdiobus_read(bus, phy_addr, MII_MMD_DATA); >> + default: >> + return -EOPNOTSUPP; >> + } > > So this is reading a C45 register space register, otherwise it would > not be called _mmd and have a devad. So access_mode should really be > transfer mode. Until now, only transfer mode C45 can be used to access > C45 register space. The point of this patchset is to add a new > C45_OVER_C22 transfer mode. So you suggest to rename access_mode to transfer_mode, right? > And C22 would should give -EINVAL, since you cannot use plain C22 bus > transactions to access C45 register space. We had indirect mmd access before with c22 PHYs before, we could theoretically fold that into c45-over-c22, but the old indirect mmd access wasn't checking for error codes and according to Russell we cannot change that. Honestly, I'd just duplicate the code and leave the old non-error checking code in __phy_read_mmd() while __phy_mdiobus_read_mmd() will do error checking and return -EINVAL with PHY_ACCESS_C22. I had that in one of the first versions but you suggested to not copy the code :), then this ugly check_rc thing came. Or __phy_mdiobus_(read,write)_mmd() will have a check_rc parameter. -michael