Hi Roger, On Friday 11 October 2013 08:32 PM, Roger Quadros wrote: > 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 <kishon@xxxxxx> >>>>> --- >>>>> 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 <linux/module.h> >>>>> #include <linux/platform_device.h> >>>>> #include <linux/slab.h> >>>>> -#include <linux/usb/omap_usb.h> >>>>> +#include <linux/phy/omap_pipe3.h> >>>>> +#include <linux/phy/phy.h> >>>>> #include <linux/of.h> >>>>> #include <linux/clk.h> >>>>> #include <linux/err.h> >>>>> @@ -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" No, I think we should have omap in the compatible since this PHY is specific to OMAP. > > 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. Ok. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html