From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754202AbcEPSXJ (ORCPT ); Mon, 16 May 2016 14:23:09 -0400 Received: from arroyo.ext.ti.com ([198.47.19.12]:42588 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753856AbcEPSXI (ORCPT ); Mon, 16 May 2016 14:23:08 -0400 Message-ID: <573A1001.4000503@ti.com> Date: Mon, 16 May 2016 13:22:57 -0500 From: Dan Murphy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Alexander Graf , CC: , Subject: Re: [PATCH] phy dp83867: depend on CONFIG_OF_MDIO References: <1463398095-123269-1-git-send-email-agraf@suse.de> <5739CD4C.7020208@ti.com> <202f4843-bf2f-8131-cea8-923ba3766fc3@suse.de> In-Reply-To: <202f4843-bf2f-8131-cea8-923ba3766fc3@suse.de> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Alex On 05/16/2016 12:57 PM, Alexander Graf wrote: > Hi Dan, > > On 16.05.16 15:38, Dan Murphy wrote: >> Alexander >> >> On 05/16/2016 06:28 AM, Alexander Graf wrote: >>> The DP83867 phy driver doesn't actually work when CONFIG_OF_MDIO isn't enabled. >>> It simply passes the device tree test, but leaves all internal configuration >>> initialized at 0. Then it configures the phy with those values and renders a >>> previously working configuration useless. >>> >>> This patch makes sure that we only build the DP83867 phy code when >>> CONFIG_OF_MDIO is set, to not run into that problem. >>> >>> Signed-off-by: Alexander Graf >>> --- >>> drivers/net/phy/Kconfig | 1 + >>> drivers/net/phy/dp83867.c | 7 ------- >>> 2 files changed, 1 insertion(+), 7 deletions(-) >>> >>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >>> index 6dad9a9..4265ad5 100644 >>> --- a/drivers/net/phy/Kconfig >>> +++ b/drivers/net/phy/Kconfig >>> @@ -148,6 +148,7 @@ config DP83848_PHY >>> >>> config DP83867_PHY >>> tristate "Drivers for Texas Instruments DP83867 Gigabit PHY" >>> + depends on OF_MDIO >>> ---help--- >>> Currently supports the DP83867 PHY. >>> >>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>> index 2afa61b..ff867ba 100644 >>> --- a/drivers/net/phy/dp83867.c >>> +++ b/drivers/net/phy/dp83867.c >>> @@ -99,7 +99,6 @@ static int dp83867_config_intr(struct phy_device *phydev) >>> return phy_write(phydev, MII_DP83867_MICR, micr_status); >>> } >>> >>> -#ifdef CONFIG_OF_MDIO >>> static int dp83867_of_init(struct phy_device *phydev) >>> { >>> struct dp83867_private *dp83867 = phydev->priv; >>> @@ -123,12 +122,6 @@ static int dp83867_of_init(struct phy_device *phydev) >>> return of_property_read_u32(of_node, "ti,fifo-depth", >>> &dp83867->fifo_depth); >>> } >>> -#else >>> -static int dp83867_of_init(struct phy_device *phydev) >>> -{ >>> - return 0; >>> -} >>> -#endif /* CONFIG_OF_MDIO */ >>> >>> static int dp83867_config_init(struct phy_device *phydev) >>> { >> I don't think we want this to depend solely on OF_MDIO. >> >> The #else case should probably be coded to look at platform data, if >> it exists. I don't have any boards that still used platform data to test this >> out so I did not feel comfortable adding code I could not test. > Since there was no code to look at platform data, those boards would be > broken just as well today, no? So at the end of the day, this change > should be no regression for them. As Andrew pointed out if you are not using RGMII you don't need internal delay or fifo_depth so making the driver dependent on OF_MDIO does not make sense. The DP83867 RGMII tx and rx delays and fifo should really be changed to optional parameters and only programmed if set. Dan > > Alex -- ------------------ Dan Murphy