From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84CB3C433EF for ; Mon, 25 Oct 2021 13:58:54 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 86DCF60F9B for ; Mon, 25 Oct 2021 13:58:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 86DCF60F9B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6583983458; Mon, 25 Oct 2021 15:58:51 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="gIcYuha4"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 15D7483461; Mon, 25 Oct 2021 15:58:50 +0200 (CEST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E416082DA1 for ; Mon, 25 Oct 2021 15:58:46 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=kabel@kernel.org Received: by mail.kernel.org (Postfix) with ESMTPSA id 0C74560EC0; Mon, 25 Oct 2021 13:58:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1635170325; bh=TYrXJdFHpTKFSdORGsDfoiqeL+ilftMZqF8GiQikW3w=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=gIcYuha4s8heNiDszzqFYp3hGHekFsJv8bMJ467Xxxp4mD4JD3Mj9wDGBOoXWkC2Q kwHd7uxYI7n3sAgnRxPQtw+/P5XcDMV9si/ClGjumuQiOaq+wnvEYUfuFcbRM23frz QXlvdHa1QxZxPKgtxbj4zka0cmUIOUOyG+OSTdhEQM7s2sUjMkNNE5F2bZzg95hpkj V/kjJ5BbcUeqvBTj2FCFqWQR8CMu294yVP2A8mA0zQR7EfPVu08cR6wvmn4dYXK+Rs JF+Qi/f96255Pn6vORgwnapvdkxQgqt2Ki2dvKly4+o2NshKXrpo1GqAVEIdMlhcCD zY0GzTXLUe7OQ== Date: Mon, 25 Oct 2021 15:58:38 +0200 From: Marek =?UTF-8?B?QmVow7pu?= To: Roman Bacik Cc: U-Boot Mailing List , Bharat Gooty , Joe Hershberger , Ramon Fried Subject: Re: [PATCH v2 1/2] net: brcm: netXtreme driver Message-ID: <20211025155838.7b243f1f@thinkpad> In-Reply-To: <20211022162222.v2.1.I1edaad77041c1300213c307eef6741499504047@changeid> References: <20211022162222.v2.1.I1edaad77041c1300213c307eef6741499504047@changeid> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean NAK for this driver. - display_banner() spams the output unnecessarily, the information should be printed with debug() - you are introducing custom mechanism for setting / getting PHY parameters, via custom specific env variables, for example in the set_phy_speed() and set_phy_link() functions, i.e.: sprintf(name1, "bnxt_eth%u_phy_speed", bp->cardnum); env_set(name1, name); The whole point of several people in the past few years was to create generic mechanisms for such things. We have ethernet PHY DM class, you should use this. That way you won't need to introduce custom mechanisms to get the infromation, since there are mii/mdio commands. - print_mac() - the driver shouldn't even have this function, it should just set appropriate ethNaddr variable - in bnxt_eth_probe() you are looking at the variable "ethaddr": if (env_get("ethaddr")) secondary = 1; a driver should never look itself at this variable. Since your driver should be of UCLASS_ETH, the generic mechanism should use appropriate env variable by calling you .write_hwaddr method Basically you are going against all the points of the whole idea to have a generic API to set network driver parameters, and instead you are adding driver-specific custom mechanisms. Please change that in next version. Marek