Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756645Ab3JKPD1 (ORCPT ); Fri, 11 Oct 2013 11:03:27 -0400 Received: from devils.ext.ti.com ([198.47.26.153]:49556 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752211Ab3JKPDY (ORCPT ); Fri, 11 Oct 2013 11:03:24 -0400 Message-ID: <52581304.9010205@ti.com> Date: Fri, 11 Oct 2013 18:02:28 +0300 From: Roger Quadros User-Agent: Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20121011 Thunderbird/16.0.1 MIME-Version: 1.0 To: Kishon Vijay Abraham I CC: , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH 3/7] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework References: <1378136591-7463-1-git-send-email-kishon@ti.com> <1378136591-7463-4-git-send-email-kishon@ti.com> <5231A35D.7060006@ti.com> <5236749F.3070707@ti.com> <5236B537.1090902@ti.com> In-Reply-To: <5236B537.1090902@ti.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 20165 Lines: 518 On 09/16/2013 10:37 AM, Roger Quadros wrote: > On 09/16/2013 06:01 AM, Kishon Vijay Abraham I wrote: >> On Thursday 12 September 2013 04:49 PM, Roger Quadros wrote: >>> Hi Kishon, >>> >>> On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: >>>> Adapted omap-usb3 PHY driver to Generic PHY Framework and moved phy-omap-usb3 >>>> driver in drivers/usb/phy to drivers/phy and also renamed the file to >>>> phy-omap-pipe3 since this same driver will be used for SATA PHY and >>>> PCIE PHY. >>> >>> I would suggest to split the renaming and PHY adaptation into 2 separate patches. >> >> Alright. >>> >>>> >>>> Signed-off-by: Kishon Vijay Abraham I >>>> --- >>>> Documentation/devicetree/bindings/usb/usb-phy.txt | 5 +- >>>> drivers/phy/Kconfig | 10 + >>>> drivers/phy/Makefile | 1 + >>>> .../phy/phy-omap-usb3.c => phy/phy-omap-pipe3.c} | 206 +++++++++++--------- >>> >>> how about naming it to phy-ti-pipe3.c as it is used on OMAP as well as non-OMAP e.g. DRA7. >> >> hmm.. I thought it would be consistent with other PHY drivers (phy-omap-usb2). Moreover DRA7 is OMAP based platform ;-) Maybe we should reserve that for later? > > OK. Up to you. > >>> >>>> drivers/usb/phy/Kconfig | 11 -- >>>> drivers/usb/phy/Makefile | 1 - >>>> include/linux/phy/omap_pipe3.h | 52 +++++ >>>> 7 files changed, 177 insertions(+), 109 deletions(-) >>>> rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-omap-pipe3.c} (57%) >>>> create mode 100644 include/linux/phy/omap_pipe3.h >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/usb-phy.txt b/Documentation/devicetree/bindings/usb/usb-phy.txt >>>> index c0245c8..36bdb17 100644 >>>> --- a/Documentation/devicetree/bindings/usb/usb-phy.txt >>>> +++ b/Documentation/devicetree/bindings/usb/usb-phy.txt >>>> @@ -21,10 +21,11 @@ usb2phy@4a0ad080 { >>>> #phy-cells = <0>; >>>> }; >>>> >>>> -OMAP USB3 PHY >>>> +OMAP PIPE3 PHY >>>> >>>> Required properties: >>>> - - compatible: Should be "ti,omap-usb3" >>>> + - compatible: Should be "ti,omap-usb3", "ti,omap-pipe3", "ti,omap-sata" >>>> + or "ti,omap-pcie" >>>> - reg : Address and length of the register set for the device. >>>> - reg-names: The names of the register addresses corresponding to the registers >>>> filled in "reg". >>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >>>> index ac239ac..5c2e7a0 100644 >>>> --- a/drivers/phy/Kconfig >>>> +++ b/drivers/phy/Kconfig >>>> @@ -27,6 +27,16 @@ config OMAP_USB2 >>>> The USB OTG controller communicates with the comparator using this >>>> driver. >>>> >>>> +config OMAP_PIPE3 >>>> + tristate "OMAP PIPE3 PHY Driver" >>>> + select GENERIC_PHY >>>> + select OMAP_CONTROL_USB >>> how about >>> depends on OMAP_CONTROL_USB >> >> From whatever I could make out from comments of Greg in my Generic PHY Framework, it's better to do a select of dependent modules instead of depends on. > > You can use select, provided the item you are selecting doesn't depend on anything else. > As OMAP_CONTROL_USB depends on ARCH_OMAP2PLUS, your configuration will fail if a user enables > OMAP_PIPE3 on non OMAP configuration. > > Further, in this case since it is OMAP related driver, there is no point in showing/building it > if OMAP platform is not selected, so you at least need [depends on "ARCH_OMAP2PLUS"] to fix > both issue I mentioned. > >>> >>> Also, if this is TI/OMAP it might as well depend on ARCH_OMAP. >>> >>>> + help >>>> + Enable this to support the PIPE3 PHY that is part of SOC. This >>> >>> worth mentioning TI OMAP/DRA SoCs. >> >> right. >>> >>>> + driver takes care of all the PHY functionality apart from comparator. >>>> + This driver interacts with the "OMAP Control PHY Driver" to power >>>> + on/off the PHY. >>>> + >>>> config TWL4030_USB >>>> tristate "TWL4030 USB Transceiver Driver" >>>> depends on TWL4030_CORE && REGULATOR_TWL4030 && USB_MUSB_OMAP2PLUS >>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile >>>> index 0dd8a98..48bf9f2 100644 >>>> --- a/drivers/phy/Makefile >>>> +++ b/drivers/phy/Makefile >>>> @@ -4,4 +4,5 @@ >>>> >>>> obj-$(CONFIG_GENERIC_PHY) += phy-core.o >>>> obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o >>>> +obj-$(CONFIG_OMAP_PIPE3) += phy-omap-pipe3.o >>>> obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o >>>> diff --git a/drivers/usb/phy/phy-omap-usb3.c b/drivers/phy/phy-omap-pipe3.c >>>> similarity index 57% >>>> rename from drivers/usb/phy/phy-omap-usb3.c >>>> rename to drivers/phy/phy-omap-pipe3.c >>>> index 4004f82..ee9a901 100644 >>>> --- a/drivers/usb/phy/phy-omap-usb3.c >>>> +++ b/drivers/phy/phy-omap-pipe3.c >>>> @@ -1,5 +1,5 @@ >>>> /* >>>> - * omap-usb3 - USB PHY, talking to dwc3 controller in OMAP. >>>> + * omap-pipe3 - PHY driver for SATA, USB and PCIE in OMAP platforms >>>> * >>>> * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com >>>> * This program is free software; you can redistribute it and/or modify >>>> @@ -19,7 +19,8 @@ >>>> #include >>>> #include >>>> #include >>>> -#include >>>> +#include >>>> +#include >>>> #include >>>> #include >>>> #include >>>> @@ -52,17 +53,17 @@ >>>> >>>> /* >>>> * This is an Empirical value that works, need to confirm the actual >>>> - * value required for the USB3PHY_PLL_CONFIGURATION2.PLL_IDLE status >>>> - * to be correctly reflected in the USB3PHY_PLL_STATUS register. >>>> + * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status >>>> + * to be correctly reflected in the PIPE3PHY_PLL_STATUS register. >>>> */ >>>> # define PLL_IDLE_TIME 100; >>>> >>>> -struct usb_dpll_map { >>>> +struct pipe3_dpll_map { >>>> unsigned long rate; >>>> - struct usb_dpll_params params; >>>> + struct pipe3_dpll_params params; >>>> }; >>>> >>>> -static struct usb_dpll_map dpll_map[] = { >>>> +static struct pipe3_dpll_map dpll_map[] = { >>>> {12000000, {1250, 5, 4, 20, 0} }, /* 12 MHz */ >>>> {16800000, {3125, 20, 4, 20, 0} }, /* 16.8 MHz */ >>>> {19200000, {1172, 8, 4, 20, 65537} }, /* 19.2 MHz */ >>>> @@ -71,7 +72,7 @@ static struct usb_dpll_map dpll_map[] = { >>>> {38400000, {3125, 47, 4, 20, 92843} }, /* 38.4 MHz */ >>>> }; >>>> >>>> -static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate) >>>> +static struct pipe3_dpll_params *omap_pipe3_get_dpll_params(unsigned long rate) >>>> { >>>> int i; >>>> >>>> @@ -83,110 +84,113 @@ static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate) >>>> return 0; >>>> } >>>> >>>> -static int omap_usb3_suspend(struct usb_phy *x, int suspend) >>>> +static int omap_pipe3_power_off(struct phy *x) >>>> { >>>> - struct omap_usb *phy = phy_to_omapusb(x); >>>> - int val; >>>> + struct omap_pipe3 *phy = phy_get_drvdata(x); >>>> + int val; >>>> int timeout = PLL_IDLE_TIME; >>>> >>>> - if (suspend && !phy->is_suspended) { >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2); >>>> - val |= PLL_IDLE; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val); >>>> - >>>> - do { >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS); >>>> - if (val & PLL_TICOPWDN) >>>> - break; >>>> - udelay(1); >>>> - } while (--timeout); >>>> - >>>> - omap_control_usb_phy_power(phy->control_dev, 0); >>>> - >>>> - phy->is_suspended = 1; >>>> - } else if (!suspend && phy->is_suspended) { >>>> - phy->is_suspended = 0; >>>> - >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2); >>>> - val &= ~PLL_IDLE; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val); >>>> - >>>> - do { >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS); >>>> - if (!(val & PLL_TICOPWDN)) >>>> - break; >>>> - udelay(1); >>>> - } while (--timeout); >>>> - } >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2); >>>> + val |= PLL_IDLE; >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val); >>>> + >>>> + do { >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS); >>>> + if (val & PLL_TICOPWDN) >>>> + break; >>>> + udelay(1); >>> >>> Is it better to sleep instead of udelay? >> >> hmm.. yeah, I guess this function wont be called from interrupt context. >>> >>>> + } while (--timeout); >>>> + >>>> + omap_control_usb_phy_power(phy->control_dev, 0); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int omap_pipe3_power_on(struct phy *x) >>>> +{ >>>> + struct omap_pipe3 *phy = phy_get_drvdata(x); >>>> + int val; >>>> + int timeout = PLL_IDLE_TIME; >>>> + >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2); >>>> + val &= ~PLL_IDLE; >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val); >>>> + >>>> + do { >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS); >>>> + if (!(val & PLL_TICOPWDN)) >>>> + break; >>>> + udelay(1); >>> >>> here too. >> >> Ok. >>> >>>> + } while (--timeout); >>>> >>>> return 0; >>>> } >>>> >>>> -static void omap_usb_dpll_relock(struct omap_usb *phy) >>>> +static void omap_pipe3_dpll_relock(struct omap_pipe3 *phy) >>>> { >>>> u32 val; >>>> unsigned long timeout; >>>> >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO); >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO); >>>> >>>> timeout = jiffies + msecs_to_jiffies(20); >>>> do { >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS); >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS); >>>> if (val & PLL_LOCK) >>>> break; >>>> } while (!WARN_ON(time_after(jiffies, timeout))); >>>> } >>>> >>>> -static int omap_usb_dpll_lock(struct omap_usb *phy) >>>> +static int omap_pipe3_dpll_lock(struct omap_pipe3 *phy) >>>> { >>>> u32 val; >>>> unsigned long rate; >>>> - struct usb_dpll_params *dpll_params; >>>> + struct pipe3_dpll_params *dpll_params; >>>> >>>> rate = clk_get_rate(phy->sys_clk); >>>> - dpll_params = omap_usb3_get_dpll_params(rate); >>>> + dpll_params = omap_pipe3_get_dpll_params(rate); >>>> if (!dpll_params) { >>>> dev_err(phy->dev, >>>> "No DPLL configuration for %lu Hz SYS CLK\n", rate); >>>> return -EINVAL; >>>> } >>>> >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1); >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1); >>>> val &= ~PLL_REGN_MASK; >>>> val |= dpll_params->n << PLL_REGN_SHIFT; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val); >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val); >>>> >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2); >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2); >>>> val &= ~PLL_SELFREQDCO_MASK; >>>> val |= dpll_params->freq << PLL_SELFREQDCO_SHIFT; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val); >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val); >>>> >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1); >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1); >>>> val &= ~PLL_REGM_MASK; >>>> val |= dpll_params->m << PLL_REGM_SHIFT; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val); >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val); >>>> >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4); >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4); >>>> val &= ~PLL_REGM_F_MASK; >>>> val |= dpll_params->mf << PLL_REGM_F_SHIFT; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val); >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val); >>>> >>>> - val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3); >>>> + val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3); >>>> val &= ~PLL_SD_MASK; >>>> val |= dpll_params->sd << PLL_SD_SHIFT; >>>> - omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val); >>>> + omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val); >>>> >>>> - omap_usb_dpll_relock(phy); >>>> + omap_pipe3_dpll_relock(phy); >>>> >>>> return 0; >>>> } >>>> >>>> -static int omap_usb3_init(struct usb_phy *x) >>>> +static int omap_pipe3_init(struct phy *x) >>>> { >>>> - struct omap_usb *phy = phy_to_omapusb(x); >>>> + struct omap_pipe3 *phy = phy_get_drvdata(x); >>>> int ret; >>>> >>>> - ret = omap_usb_dpll_lock(phy); >>>> + ret = omap_pipe3_dpll_lock(phy); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -195,9 +199,18 @@ static int omap_usb3_init(struct usb_phy *x) >>>> return 0; >>>> } >>>> >>>> -static int omap_usb3_probe(struct platform_device *pdev) >>>> +static struct phy_ops ops = { >>>> + .init = omap_pipe3_init, >>>> + .power_on = omap_pipe3_power_on, >>>> + .power_off = omap_pipe3_power_off, >>>> + .owner = THIS_MODULE, >>>> +}; >>>> + >>>> +static int omap_pipe3_probe(struct platform_device *pdev) >>>> { >>>> - struct omap_usb *phy; >>>> + struct omap_pipe3 *phy; >>>> + struct phy *generic_phy; >>>> + struct phy_provider *phy_provider; >>>> struct resource *res; >>>> struct device_node *node = pdev->dev.of_node; >>>> struct device_node *control_node; >>>> @@ -208,7 +221,7 @@ static int omap_usb3_probe(struct platform_device *pdev) >>>> >>>> phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL); >>>> if (!phy) { >>>> - dev_err(&pdev->dev, "unable to alloc mem for OMAP USB3 PHY\n"); >>>> + dev_err(&pdev->dev, "unable to alloc mem for OMAP PIPE3 PHY\n"); >>>> return -ENOMEM; >>>> } >>>> >>>> @@ -219,13 +232,6 @@ static int omap_usb3_probe(struct platform_device *pdev) >>>> >>>> phy->dev = &pdev->dev; >>>> >>>> - phy->phy.dev = phy->dev; >>>> - phy->phy.label = "omap-usb3"; >>>> - phy->phy.init = omap_usb3_init; >>>> - phy->phy.set_suspend = omap_usb3_suspend; >>>> - phy->phy.type = USB_PHY_TYPE_USB3; >>>> - >>>> - phy->is_suspended = 1; >>>> phy->wkupclk = devm_clk_get(phy->dev, "usb_phy_cm_clk32k"); >>> >>> As this is no longer USB specific, we need to make the clock binding generic as well. >> >> right. Its also needed when we have the same driver work for sata and pcie. Maybe do that in a separate patch? > > OK. up to you. >>> >>>> if (IS_ERR(phy->wkupclk)) { >>>> dev_err(&pdev->dev, "unable to get usb_phy_cm_clk32k\n"); >>>> @@ -251,6 +257,11 @@ static int omap_usb3_probe(struct platform_device *pdev) >>>> dev_err(&pdev->dev, "Failed to get control device phandle\n"); >>>> return -EINVAL; >>>> } >>>> + phy_provider = devm_of_phy_provider_register(phy->dev, >>>> + of_phy_simple_xlate); >>>> + if (IS_ERR(phy_provider)) >>>> + return PTR_ERR(phy_provider); >>>> + >>>> control_pdev = of_find_device_by_node(control_node); >>>> if (!control_pdev) { >>>> dev_err(&pdev->dev, "Failed to get control device\n"); >>>> @@ -260,23 +271,27 @@ static int omap_usb3_probe(struct platform_device *pdev) >>>> phy->control_dev = &control_pdev->dev; >>>> >>>> omap_control_usb_phy_power(phy->control_dev, 0); >>>> - usb_add_phy_dev(&phy->phy); >>>> >>>> platform_set_drvdata(pdev, phy); >>>> - >>>> pm_runtime_enable(phy->dev); >>>> + >>>> + generic_phy = devm_phy_create(phy->dev, &ops, NULL); >>>> + if (IS_ERR(generic_phy)) >>>> + return PTR_ERR(generic_phy); >>>> + >>>> + phy_set_drvdata(generic_phy, phy); >>>> + >>>> pm_runtime_get(&pdev->dev); >>>> >>>> return 0; >>>> } >>>> >>>> -static int omap_usb3_remove(struct platform_device *pdev) >>>> +static int omap_pipe3_remove(struct platform_device *pdev) >>>> { >>>> - struct omap_usb *phy = platform_get_drvdata(pdev); >>>> + struct omap_pipe3 *phy = platform_get_drvdata(pdev); >>>> >>>> clk_unprepare(phy->wkupclk); >>>> clk_unprepare(phy->optclk); >>>> - usb_remove_phy(&phy->phy); >>>> if (!pm_runtime_suspended(&pdev->dev)) >>>> pm_runtime_put(&pdev->dev); >>>> pm_runtime_disable(&pdev->dev); >>>> @@ -286,10 +301,9 @@ static int omap_usb3_remove(struct platform_device *pdev) >>>> >>>> #ifdef CONFIG_PM_RUNTIME >>>> >>>> -static int omap_usb3_runtime_suspend(struct device *dev) >>>> +static int omap_pipe3_runtime_suspend(struct device *dev) >>>> { >>>> - struct platform_device *pdev = to_platform_device(dev); >>>> - struct omap_usb *phy = platform_get_drvdata(pdev); >>>> + struct omap_pipe3 *phy = dev_get_drvdata(dev); >>>> >>>> clk_disable(phy->wkupclk); >>>> clk_disable(phy->optclk); >>>> @@ -297,11 +311,10 @@ static int omap_usb3_runtime_suspend(struct device *dev) >>>> return 0; >>>> } >>>> >>>> -static int omap_usb3_runtime_resume(struct device *dev) >>>> +static int omap_pipe3_runtime_resume(struct device *dev) >>>> { >>>> u32 ret = 0; >>>> - struct platform_device *pdev = to_platform_device(dev); >>>> - struct omap_usb *phy = platform_get_drvdata(pdev); >>>> + struct omap_pipe3 *phy = dev_get_drvdata(dev); >>>> >>>> ret = clk_enable(phy->optclk); >>>> if (ret) { >>>> @@ -324,38 +337,41 @@ err1: >>>> return ret; >>>> } >>>> >>>> -static const struct dev_pm_ops omap_usb3_pm_ops = { >>>> - SET_RUNTIME_PM_OPS(omap_usb3_runtime_suspend, omap_usb3_runtime_resume, >>>> - NULL) >>>> +static const struct dev_pm_ops omap_pipe3_pm_ops = { >>>> + SET_RUNTIME_PM_OPS(omap_pipe3_runtime_suspend, >>>> + omap_pipe3_runtime_resume, NULL) >>>> }; >>>> >>>> -#define DEV_PM_OPS (&omap_usb3_pm_ops) >>>> +#define DEV_PM_OPS (&omap_pipe3_pm_ops) >>>> #else >>>> #define DEV_PM_OPS NULL >>>> #endif >>>> >>>> #ifdef CONFIG_OF >>>> -static const struct of_device_id omap_usb3_id_table[] = { >>>> +static const struct of_device_id omap_pipe3_id_table[] = { >>>> + { .compatible = "ti,omap-pipe3" }, >>> >>> why do you need "omap-pipe3", isn't sata, pcie and usb3 sufficient? >> >> I thought it would be better if everyone uses omap-pipe3 and added pcie, sata if there are any specific settings (for pcie or sata) that should be done. > > We can always add specialized options later when needed. AFAIK just the > DPLL data is different among the different PHYs. >>> >>>> + { .compatible = "ti,omap-sata" }, >>>> + { .compatible = "ti,omap-pcie" }, >>>> { .compatible = "ti,omap-usb3" }, > > I think compatible strings should be improved to indicate that it is a PHY. > > e.g. "ti,omap-phy-sata" or just "ti,pipe3-phy-sata" > Please remove "ti,omap-pcie" for now, you can add it later whenever you add dpll settings for pcie. Also, please change the newly added compatible strings to "ti,phy-pipe3-usb3" and "ti,phy-pipe3-sata" Also you need to rearrange the patches so that the DT binding document is first moded from usb/ to phy/ and then the change is done for pipe3. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/