Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752541AbdIVUAl (ORCPT ); Fri, 22 Sep 2017 16:00:41 -0400 Received: from mail-qk0-f193.google.com ([209.85.220.193]:35804 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751915AbdIVUAh (ORCPT ); Fri, 22 Sep 2017 16:00:37 -0400 X-Google-Smtp-Source: AOwi7QAWnR0jcvQdIbjEd8LpEoOCRsIMgy+A1GAkuIk0HYCKJNKXbUmf2S52lBn35PBLbu1R+jlH9A== Subject: Re: [PATCH net-next v2 1/3] net: dsa: use slave device phydev To: Vivien Didelot , netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com, "David S. Miller" , Andrew Lunn References: <20170922194045.18814-1-vivien.didelot@savoirfairelinux.com> <20170922194045.18814-2-vivien.didelot@savoirfairelinux.com> From: Florian Fainelli Message-ID: <2262f266-6fb9-cce4-f83a-933dd41b9062@gmail.com> Date: Fri, 22 Sep 2017 13:00:32 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170922194045.18814-2-vivien.didelot@savoirfairelinux.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2558 Lines: 93 On 09/22/2017 12:40 PM, Vivien Didelot wrote: > There is no need to store a phy_device in dsa_slave_priv since > net_device already provides one. Simply s/p->phy/dev->phydev/. You can therefore remove the phy_device from dsa_slave_priv, see below for more comments. I will have to regress test the heck out of this, this should take a few hours. > > While at it, return -ENODEV when it is NULL instead of -EOPNOTSUPP. > > Signed-off-by: Vivien Didelot > --- > static int dsa_slave_port_attr_set(struct net_device *dev, > @@ -435,12 +433,10 @@ static int > dsa_slave_get_link_ksettings(struct net_device *dev, > struct ethtool_link_ksettings *cmd) > { > - struct dsa_slave_priv *p = netdev_priv(dev); > + if (!dev->phydev) > + return -ENODEV; > > - if (!p->phy) > - return -EOPNOTSUPP; > - > - phy_ethtool_ksettings_get(p->phy, cmd); > + phy_ethtool_ksettings_get(dev->phydev, cmd); This can be replaced by phy_ethtool_get_link_ksettings() > > return 0; > } > @@ -449,12 +445,10 @@ static int > dsa_slave_set_link_ksettings(struct net_device *dev, > const struct ethtool_link_ksettings *cmd) > { > - struct dsa_slave_priv *p = netdev_priv(dev); > + if (!dev->phydev) > + return -ENODEV; > > - if (p->phy != NULL) > - return phy_ethtool_ksettings_set(p->phy, cmd); > - > - return -EOPNOTSUPP; > + return phy_ethtool_ksettings_set(dev->phydev, cmd); > } This can disappear and you can assign this ethtool operation to phy_ethtool_set_link_ksettings() > > static void dsa_slave_get_drvinfo(struct net_device *dev, > @@ -488,24 +482,20 @@ dsa_slave_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *_p) > > static int dsa_slave_nway_reset(struct net_device *dev) > { > - struct dsa_slave_priv *p = netdev_priv(dev); > + if (!dev->phydev) > + return -ENODEV; > > - if (p->phy != NULL) > - return genphy_restart_aneg(p->phy); > - > - return -EOPNOTSUPP; > + return genphy_restart_aneg(dev->phydev); > } This can now disappear and you can use phy_ethtool_nway_reset() directly in ethtool_ops > > static u32 dsa_slave_get_link(struct net_device *dev) > { > - struct dsa_slave_priv *p = netdev_priv(dev); > + if (!dev->phydev) > + return -ENODEV; > > - if (p->phy != NULL) { > - genphy_update_link(p->phy); > - return p->phy->link; > - } > + genphy_update_link(dev->phydev); > > - return -EOPNOTSUPP; > + return dev->phydev->link; > } This should certainly be just ethtool_op_get_link(), not sure why we kept that around here... -- Florian