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 X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0FD39C433DB for ; Fri, 12 Mar 2021 02:20:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E66F864F90 for ; Fri, 12 Mar 2021 02:20:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231634AbhCLCUF (ORCPT ); Thu, 11 Mar 2021 21:20:05 -0500 Received: from vps0.lunn.ch ([185.16.172.187]:53188 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231591AbhCLCTc (ORCPT ); Thu, 11 Mar 2021 21:19:32 -0500 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1lKXOk-00ASDr-PV; Fri, 12 Mar 2021 03:19:30 +0100 Date: Fri, 12 Mar 2021 03:19:30 +0100 From: Andrew Lunn To: David Thompson Cc: netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org, limings@nvidia.com, Asmaa Mnebhi Subject: Re: [PATCH net-next v4] Add Mellanox BlueField Gigabit Ethernet driver Message-ID: References: <1615380399-31970-1-git-send-email-davthompson@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1615380399-31970-1-git-send-email-davthompson@nvidia.com> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org > +#define DRV_VERSION 1.19 > +static int mlxbf_gige_probe(struct platform_device *pdev) > +{ > + unsigned int phy_int_gpio; > + struct phy_device *phydev; > + struct net_device *netdev; > + struct resource *mac_res; > + struct resource *llu_res; > + struct resource *plu_res; > + struct mlxbf_gige *priv; > + void __iomem *llu_base; > + void __iomem *plu_base; > + void __iomem *base; > + int addr, version; > + u64 control; > + int err = 0; > + > + if (device_property_read_u32(&pdev->dev, "version", &version)) { > + dev_err(&pdev->dev, "Version Info not found\n"); > + return -EINVAL; > + } Is this a device tree property? ACPI? If it is device tree property you need to document the binding, Documentation/devicetree/bindinds/... > + > + if (version != (int)DRV_VERSION) { > + dev_err(&pdev->dev, "Version Mismatch. Expected %d Returned %d\n", > + (int)DRV_VERSION, version); > + return -EINVAL; > + } That is odd. Doubt odd. First of, why (int)1.19? Why not just set DRV_VERSION to 1? This is the only place you use this, so the .19 seems pointless. Secondly, what does this version in DT/ACPI actually represent? The hardware version? Then you should be using a compatible string? Or read a hardware register which tells you have hardware version. > + > + err = device_property_read_u32(&pdev->dev, "phy-int-gpio", &phy_int_gpio); > + if (err < 0) > + phy_int_gpio = MLXBF_GIGE_DEFAULT_PHY_INT_GPIO; Again, this probably needs documenting. This is not how you do interrupts with DT. I also don't think it is correct for ACPI, but i don't know ACPI. > + phydev = phy_find_first(priv->mdiobus); > + if (!phydev) { > + mlxbf_gige_mdio_remove(priv); > + return -ENODEV; > + } If you are using DT, please use a phandle to the device on the MDIO bus. > + /* Sets netdev->phydev to phydev; which will eventually > + * be used in ioctl calls. > + * Cannot pass NULL handler. > + */ > + err = phy_connect_direct(netdev, phydev, > + mlxbf_gige_adjust_link, > + PHY_INTERFACE_MODE_GMII); It does a lot more than just set netdev->phydev. I'm not sure this comment has any real value. Andrew