Hi Stan, On Mon, 2013-10-07 at 12:22 +0300, Stanimir Varbanov wrote: > Hi Ivan, > > Few comments below. > > On 10/07/2013 10:44 AM, Ivan T. Ivanov wrote: > > From: "Ivan T. Ivanov" <iivanov@xxxxxxxxxx> > > > > These drivers handles control and configuration of the HS > > and SS USB PHY transceivers. They are part of the driver > > which manage Synopsys DesignWare USB3 controller stack > > inside Qualcomm SoC's. > > > > Signed-off-by: Ivan T. Ivanov <iivanov@xxxxxxxxxx> > > --- > > drivers/usb/phy/Kconfig | 11 ++ > > drivers/usb/phy/Makefile | 2 + > > drivers/usb/phy/phy-msm-dw-hs.c | 329 ++++++++++++++++++++++++++++++++++ > > drivers/usb/phy/phy-msm-dw-ss.c | 375 +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 717 insertions(+) > > create mode 100644 drivers/usb/phy/phy-msm-dw-hs.c > > create mode 100644 drivers/usb/phy/phy-msm-dw-ss.c > > > > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig > > index d5589f9..bbb2d0e 100644 > > --- a/drivers/usb/phy/Kconfig > > +++ b/drivers/usb/phy/Kconfig > > @@ -214,6 +214,17 @@ config USB_RCAR_PHY > > To compile this driver as a module, choose M here: the > > module will be called phy-rcar-usb. > > > > +config USB_MSM_DW_PHYS > > + tristate "Qualcomm USB controller DW PHY's wrappers support" > > + depends on (USB || USB_GADGET) && ARCH_MSM > > + select USB_PHY > > + help > > + Enable this to support the DW USB PHY transceivers on MSM chips > > + with DWC3 USB core. It handles PHY initialization, clock > > + management required after resetting the hardware and power > > + management. This driver is required even for peripheral only or > > + host only mode configurations. > > + > > config USB_ULPI > > bool "Generic ULPI Transceiver Driver" > > depends on ARM > > diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile > > index 2135e85..4813eb5 100644 > > --- a/drivers/usb/phy/Makefile > > +++ b/drivers/usb/phy/Makefile > > @@ -26,6 +26,8 @@ obj-$(CONFIG_USB_EHCI_TEGRA) += phy-tegra-usb.o > > obj-$(CONFIG_USB_GPIO_VBUS) += phy-gpio-vbus-usb.o > > obj-$(CONFIG_USB_ISP1301) += phy-isp1301.o > > obj-$(CONFIG_USB_MSM_OTG) += phy-msm-usb.o > > +obj-$(CONFIG_USB_MSM_DW_PHYS) += phy-msm-dw-hs.o > > +obj-$(CONFIG_USB_MSM_DW_PHYS) += phy-msm-dw-ss.o > > obj-$(CONFIG_USB_MV_OTG) += phy-mv-usb.o > > obj-$(CONFIG_USB_MXS_PHY) += phy-mxs-usb.o > > obj-$(CONFIG_USB_RCAR_PHY) += phy-rcar-usb.o > > diff --git a/drivers/usb/phy/phy-msm-dw-hs.c b/drivers/usb/phy/phy-msm-dw-hs.c > > new file mode 100644 > > index 0000000..d29c1f1 > > --- /dev/null > > +++ b/drivers/usb/phy/phy-msm-dw-hs.c > > @@ -0,0 +1,329 @@ > > +/* Copyright (c) 2013, Code Aurora Forum. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/usb/phy.h> > > + > > +/** > > + * USB QSCRATCH Hardware registers > > + */ > > +#define QSCRATCH_CTRL_REG (0x04) > > +#define QSCRATCH_GENERAL_CFG (0x08) > > +#define PHY_CTRL_REG (0x10) > > +#define PARAMETER_OVERRIDE_X_REG (0x14) > > +#define CHARGING_DET_CTRL_REG (0x18) > > +#define CHARGING_DET_OUTPUT_REG (0x1c) > > +#define ALT_INTERRUPT_EN_REG (0x20) > > +#define PHY_IRQ_STAT_REG (0x24) > > +#define CGCTL_REG (0x28) > > + > > Please remove braces above and below. ok > > > +#define PHY_3P3_VOL_MIN 3050000 /* uV */ > > +#define PHY_3P3_VOL_MAX 3300000 /* uV */ > > +#define PHY_3P3_HPM_LOAD 16000 /* uA */ > > + > > +#define PHY_1P8_VOL_MIN 1800000 /* uV */ > > +#define PHY_1P8_VOL_MAX 1800000 /* uV */ > > +#define PHY_1P8_HPM_LOAD 19000 /* uA */ > > + > > +/* TODO: these are suspicious */ > > +#define USB_VDDCX_NO 1 /* index */ > > +#define USB_VDDCX_MIN 5 /* index */ > > +#define USB_VDDCX_MAX 7 /* index */ > > + > > +struct msm_dw_hs_phy { > > + struct usb_phy phy; > > + void __iomem *base; > > + struct device *dev; > > + > > + struct clk *xo_clk; > > + struct clk *sleep_a_clk; > > + > > + struct regulator *v3p3; > > + struct regulator *v1p8; > > + struct regulator *vddcx; > > + struct regulator *vbus; > > +}; > > + > > +#define phy_to_dw_phy(x) container_of((x), struct msm_dw_hs_phy, phy) > > I think that using tab after #define is uncommon, isn't it? Not in this case. I see both patterns used. > > > + > > + > > +/** > > + * Write register. > > + * > > + * @base - MSM DWC3 PHY base virtual address. > > + * @offset - register offset. > > + * @val - value to write. > > + */ > > +static inline void msm_dw_hs_write(void __iomem *base, u32 offset, u32 val) > > Why inline? here and below > Hm, because I will like function to be inlined?? > > +{ > > + iowrite32(val, base + offset); > > +} > > + > > +/** > > + * Write register and read back masked value to confirm it is written > > + * > > + * @base - MSM DWC3 PHY base virtual address. > > + * @offset - register offset. > > + * @mask - register bitmask specifying what should be updated > > + * @val - value to write. > > + */ > > +static inline void msm_dw_hs_write_readback(void __iomem *base, u32 offset, > > + const u32 mask, u32 val) > > +{ > > + u32 write_val, tmp = ioread32(base + offset); > > + > > + tmp &= ~mask; /* retain other bits */ > > + write_val = tmp | val; > > + > > + iowrite32(write_val, base + offset); > > + > > + /* Read back to see if val was written */ > > + tmp = ioread32(base + offset); > > + tmp &= mask; /* clear other bits */ > > + > > + if (tmp != val) > > + pr_err("write: %x to QSCRATCH: %x FAILED\n", val, offset); > > You should use dev_err() here. Or better remove the error printing and > make the function to return int. I change it to dev_err(). This could happen only if clocks are not properly setup. > > > +} > > + > > +static void msm_dw_hs_phy_shutdown(struct usb_phy *x) > > +{ > > + struct msm_dw_hs_phy *phy = phy_to_dw_phy(x); > > + int ret; > > + > > + ret = regulator_set_voltage(phy->v3p3, 0, PHY_3P3_VOL_MAX); > > + if (ret) > > + dev_err(phy->dev, "cannot set voltage for v3p3\n"); > > + > > + ret = regulator_set_voltage(phy->v1p8, 0, PHY_1P8_VOL_MAX); > > + if (ret) > > + dev_err(phy->dev, "cannot set voltage for v1p8\n"); > > + > > + ret = regulator_disable(phy->v1p8); > > + if (ret) > > + dev_err(phy->dev, "cannot disable v1p8\n"); > > + > > + ret = regulator_disable(phy->v3p3); > > + if (ret) > > + dev_err(phy->dev, "cannot disable v3p3\n"); > > + > > + ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_NO, USB_VDDCX_MAX); > > + if (ret) > > + dev_err(phy->dev, "cannot set voltage for vddcx\n"); > > + > > + ret = regulator_disable(phy->vddcx); > > + if (ret) > > + dev_err(phy->dev, "cannot enable vddcx\n"); > > + > > + clk_disable_unprepare(phy->sleep_a_clk); > > +} > > + > > +static int msm_dw_hs_phy_init(struct usb_phy *x) > > +{ > > + struct msm_dw_hs_phy *phy = phy_to_dw_phy(x); > > Using tab after struct name is uncommon IMO. Take a look at OMAP's USB PHY driver, on which thes driver are based. > > > + int ret; Important point is to consistent, and in this case I am not :-) Will remove variables indentation. > > + > > + clk_prepare_enable(phy->sleep_a_clk); > > + > > + ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX); > > + if (ret) { > > + dev_err(phy->dev, "cannot set voltage for vddcx\n"); > > + return ret; > > + } > > + > > + ret = regulator_enable(phy->vddcx); > > + if (ret) { > > + dev_err(phy->dev, "cannot enable vddcx\n"); > > + return ret; > > + } > > + > > + ret = regulator_set_voltage(phy->v3p3, PHY_3P3_VOL_MIN, > > + PHY_3P3_VOL_MAX); > > + if (ret) { > > + dev_err(phy->dev, "cannot set voltage for v3p3\n"); > > + return ret; > > + } > > + > > + ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN, > > + PHY_1P8_VOL_MAX); > > + if (ret) { > > + dev_err(phy->dev, "cannot set voltage for v1p8\n"); > > + return ret; > > + } > > + > > + ret = regulator_set_optimum_mode(phy->v1p8, PHY_1P8_HPM_LOAD); > > + if (ret < 0) { > > + dev_err(phy->dev, "cannot set HPM of regulator v1p8\n"); > > + return ret; > > + } > > + > > + ret = regulator_enable(phy->v1p8); > > + if (ret) { > > + dev_err(phy->dev, "cannot enable v1p8\n"); > > + return ret; > > + } > > + > > + ret = regulator_set_optimum_mode(phy->v3p3, PHY_3P3_HPM_LOAD); > > + if (ret < 0) { > > + dev_err(phy->dev, "cannot set HPM of regulator v3p3\n"); > > + return ret; > > + } > > + > > + ret = regulator_enable(phy->v3p3); > > + if (ret) { > > + dev_err(phy->dev, "cannot enable v3p3\n"); > > + return ret; > > + } > > + > > + /* > > + * HSPHY Initialization: Enable UTMI clock and clamp enable HVINTs, > > + * and disable RETENTION (power-on default is ENABLED) > > + */ > > + msm_dw_hs_write(phy->base, PHY_CTRL_REG, 0x5220bb2); > > + usleep_range(2000, 2200); > > + > > + /* > > + * write HSPHY init value to QSCRATCH reg to set HSPHY parameters like > > + * VBUS valid threshold, disconnect valid threshold, DC voltage level, > > + * preempasis and rise/fall time. > > + */ > > + msm_dw_hs_write_readback(phy->base, PARAMETER_OVERRIDE_X_REG, > > + 0x03ffffff, 0x00d191a4); > > + > > + /* Disable (bypass) VBUS and ID filters */ > > + msm_dw_hs_write(phy->base, QSCRATCH_GENERAL_CFG, 0x78); > > + > > + return 0; > > +} > > + > > +static int msm_dw_hs_phy_set_vbus(struct usb_phy *x, int on) > > +{ > > + struct msm_dw_hs_phy *phy = phy_to_dw_phy(x); > > + int ret; > > + > > + if (IS_ERR(phy->vbus)) > > + return on ? PTR_ERR(phy->vbus) : 0; > > + > > + if (on) > > + ret = regulator_enable(phy->vbus); > > + else > > + ret = regulator_disable(phy->vbus); > > + > > + if (ret) > > + dev_err(x->dev, "Cannot %s Vbus\n", on ? "set" : "off"); > > This looks like debug code. Should the error being handled by the upper > layers? I'd remove this. I could lower this to debug, but will prefer to keep message here. > > > + return ret; > > +} > > + > > +static int msm_dw_hs_probe(struct platform_device *pdev) > > +{ > > + struct msm_dw_hs_phy *phy; > > + struct resource *res; > > + void __iomem *base; > > + > > + phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL); > > + if (!phy) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, phy); > > + > > + phy->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + base = devm_ioremap_resource(phy->dev, res); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + phy->vddcx = devm_regulator_get(phy->dev, "vddcx"); > > + if (IS_ERR(phy->vddcx)) { > > + dev_dbg(phy->dev, "cannot get vddcx\n"); > > + return PTR_ERR(phy->vddcx); > > + } > > + > > + phy->v3p3 = devm_regulator_get(phy->dev, "v3p3"); > > + if (IS_ERR(phy->v3p3)) { > > + dev_dbg(phy->dev, "cannot get v3p3\n"); > > + return PTR_ERR(phy->v3p3); > > + } > > + > > + phy->v1p8 = devm_regulator_get(phy->dev, "v1p8"); > > + if (IS_ERR(phy->v1p8)) { > > + dev_dbg(phy->dev, "cannot get v1p8\n"); > > + return PTR_ERR(phy->v1p8); > > + } > > + > > + phy->vbus = devm_regulator_get(phy->dev, "vbus"); > > + if (IS_ERR(phy->vbus)) > > + dev_dbg(phy->dev, "Failed to get vbus\n"); > > + > > + phy->xo_clk = devm_clk_get(phy->dev, "xo"); > > + if (IS_ERR(phy->xo_clk)) { > > + dev_dbg(phy->dev, "cannot get TCXO buffer handle\n"); > > + return PTR_ERR(phy->xo_clk); > > + } > > + > > + phy->sleep_a_clk = devm_clk_get(phy->dev, "sleep_a"); > > What means the "_a" in clock name? They are 2 PHY's on 8074 chip. This drivers is supposed to operate on PHY 0, thus sleep_a. PHY 1 is using sleep_b. Take a look here http://www.spinics.net/lists/arm-kernel/msg276945.html > > > + if (IS_ERR(phy->sleep_a_clk)) { > > + dev_dbg(phy->dev, "failed to get sleep_a clock\n"); > > + return PTR_ERR(phy->sleep_a_clk); > > + } > > + > > + clk_prepare_enable(phy->xo_clk); > > + > > + phy->base = base; > > + phy->phy.dev = phy->dev; > > + phy->phy.label = "msm-dw-hsphy"; > > + phy->phy.init = msm_dw_hs_phy_init; > > + phy->phy.shutdown = msm_dw_hs_phy_shutdown; > > + phy->phy.set_vbus = msm_dw_hs_phy_set_vbus; > > + phy->phy.type = USB_PHY_TYPE_USB2; > > + phy->phy.state = OTG_STATE_UNDEFINED; > > + > > + usb_add_phy_dev(&phy->phy); > > this function returns int, why you don't checked it? > Functions will fail only if we do not pass correct 'dev' instance, which could not happen here. Anyway will check for error. > > + > > + return 0; > > +} <snip> > > + > > +static struct platform_driver msm_dw_hs_driver = { > > + .probe = msm_dw_hs_probe, > > + .remove = msm_dw_hs_remove, > > + .driver = { > > + .name = "msm-dw-hsphy", > > + .owner = THIS_MODULE, > > + .of_match_table = msm_dw_hs_id_table, > > + }, > > +}; > > + > > +module_platform_driver(msm_dw_hs_driver); > > + > > +MODULE_ALIAS("platform:msm-dw-hsphy"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("DesignWare USB3 MSM HSPHY driver"); > > diff --git a/drivers/usb/phy/phy-msm-dw-ss.c b/drivers/usb/phy/phy-msm-dw-ss.c > > new file mode 100644 > > index 0000000..6734fa1 > > --- /dev/null > > +++ b/drivers/usb/phy/phy-msm-dw-ss.c > > @@ -0,0 +1,375 @@ > > +/* Copyright (c) 2013, Code Aurora Forum. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/usb/phy.h> > > + > > +/** > > + * USB QSCRATCH Hardware registers > > + */ > > +#define PHY_CTRL_REG (0x00) > > +#define PHY_PARAM_CTRL_1 (0x04) > > +#define PHY_PARAM_CTRL_2 (0x08) > > +#define CR_PROTOCOL_DATA_IN_REG (0x0c) > > +#define CR_PROTOCOL_DATA_OUT_REG (0x10) > > +#define CR_PROTOCOL_CAP_ADDR_REG (0x14) > > +#define CR_PROTOCOL_CAP_DATA_REG (0x18) > > +#define CR_PROTOCOL_READ_REG (0x1c) > > +#define CR_PROTOCOL_WRITE_REG (0x20) > > Remove braces. sure. > <snip> > > + > > +static int msm_dw_ss_phy_init(struct usb_phy *x) > > +{ > > + struct msm_dw_ss_phy *phy = phy_to_dw_phy(x); > > + u32 data = 0; > > + int ret; > > + > > + ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX); > > + if (ret) { > > + dev_err(phy->dev, "cannot set voltage for vddcx\n"); > > + return ret; > > + } > > + > > + ret = regulator_enable(phy->vddcx); > > + if (ret) { > > + dev_err(phy->dev, "cannot enable vddcx\n"); > > + return ret; > > + } > > + > > + ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN, > > + PHY_1P8_VOL_MAX); > > + if (ret) { > > + regulator_disable(phy->vddcx); > > + dev_err(phy->dev, "cannot set v1p8\n"); > > + return ret; > > + } > > + > > + ret = regulator_enable(phy->v1p8); > > + if (ret) { > > + regulator_disable(phy->vddcx); > > + dev_err(phy->dev, "cannot enable v1p8\n"); > > + return ret; > > + } > > + > > + clk_prepare_enable(phy->ref_clk); > > + usleep_range(1000, 1200); > > + > > + /* SSPHY: Use ref_clk from pads and set its parameters */ > > + msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002); > > Defines? > > > + msleep(30); > > + /* Assert SSPHY reset */ > > + msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210082); > > + usleep_range(2000, 2200); > > + /* De-assert SSPHY reset - power and ref_clock must be ON */ > > + msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002); > > + usleep_range(2000, 2200); > > + /* Ref clock must be stable now, enable ref clock for HS mode */ > > + msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210102); > > + usleep_range(2000, 2200); > > You can extract the bits and add defines with names got from comments. You know as much as I know, suggestions for define names? > > > + > > + /* > > + * WORKAROUND: There is SSPHY suspend bug due to which USB enumerates > > + * in HS mode instead of SS mode. Workaround it by asserting > > + * LANE0.TX_ALT_BLOCK.EN_ALT_BUS to enable TX to use alt bus mode > > + */ > > + data = msm_dw_ss_read_phycreg(phy->base, 0x102d); > > + data |= (1 << 7); > > + msm_dw_ss_write_phycreg(phy->base, 0x102D, data); > > + > > Defines? ok. this one is easy :-) > Also you mixing uppper and lower case in hex numberes, use > lower case. > will fix it. > > + data = msm_dw_ss_read_phycreg(phy->base, 0x1010); > > + data &= ~0xff0; > > + data |= 0x20; > > + msm_dw_ss_write_phycreg(phy->base, 0x1010, data); > > + > > + /* > > + * Fix RX Equalization setting as follows > > + * LANE0.RX_OVRD_IN_HI. RX_EQ_EN set to 0 > > + * LANE0.RX_OVRD_IN_HI.RX_EQ_EN_OVRD set to 1 > > + * LANE0.RX_OVRD_IN_HI.RX_EQ set to 3 > > + * LANE0.RX_OVRD_IN_HI.RX_EQ_OVRD set to 1 > > + */ > > + data = msm_dw_ss_read_phycreg(phy->base, 0x1006); > > Defines? Ok this is also easy, but will #define make code easy to read. We have such nice comment here. > > > + data &= ~(1 << 6); > > + data |= (1 << 7); > > + data &= ~(0x7 << 8); > > + data |= (0x3 << 8); > > + data |= (0x1 << 11); > > + msm_dw_ss_write_phycreg(phy->base, 0x1006, data); > > You mixing two styles. The first is using magic numbers combined with > comment and the second style is mixing magic bits and masks. Could you > use one defines on all places? Will try to make it consistent. > > > + > > + /* > > + * Set EQ and TX launch amplitudes as follows > > + * LANE0.TX_OVRD_DRV_LO.PREEMPH set to 22 > > + * LANE0.TX_OVRD_DRV_LO.AMPLITUDE set to 127 > > + * LANE0.TX_OVRD_DRV_LO.EN set to 1. > > + */ > > + data = msm_dw_ss_read_phycreg(phy->base, 0x1002); > > + data &= ~0x3f80; > > + data |= (0x16 << 7); > > + data &= ~0x7f; > > + data |= (0x7f | (1 << 14)); > > + msm_dw_ss_write_phycreg(phy->base, 0x1002, data); > > + > > + /* > > + * Set the QSCRATCH PHY_PARAM_CTRL1 parameters as follows > > + * TX_FULL_SWING [26:20] amplitude to 127 > > + * TX_DEEMPH_3_5DB [13:8] to 22 > > + * LOS_BIAS [2:0] to 0x5 > > + */ > > + msm_dw_ss_write_readback(phy->base, PHY_PARAM_CTRL_1, > > + 0x07f03f07, 0x07f01605); > > + return 0; > > +} > > + > > +static int msm_dw_ss_probe(struct platform_device *pdev) > > +{ > > + struct msm_dw_ss_phy *phy; > > + struct resource *res; > > + void __iomem *base; > > + > > + phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL); > > + if (!phy) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, phy); > > + > > + phy->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + base = devm_ioremap_resource(phy->dev, res); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + phy->vddcx = devm_regulator_get(phy->dev, "vddcx"); > > + if (IS_ERR(phy->vddcx)) { > > + dev_dbg(phy->dev, "cannot get vddcx\n"); > > + return PTR_ERR(phy->vddcx); > > + } > > + > > + phy->v1p8 = devm_regulator_get(phy->dev, "v1p8"); > > + if (IS_ERR(phy->v1p8)) { > > + dev_dbg(phy->dev, "cannot get v1p8\n"); > > + return PTR_ERR(phy->v1p8); > > + } > > + > > + phy->xo_clk = devm_clk_get(phy->dev, "xo"); > > + if (IS_ERR(phy->xo_clk)) { > > + dev_dbg(phy->dev, "cannot get TCXO buffer handle\n"); > > + return PTR_ERR(phy->xo_clk); > > + } > > + > > + phy->ref_clk = devm_clk_get(phy->dev, "ref"); > > + if (IS_ERR(phy->ref_clk)) { > > + dev_dbg(phy->dev, "cannot get ref clock handle\n"); > > + return PTR_ERR(phy->ref_clk); > > + } > > + > > + clk_prepare_enable(phy->xo_clk); > > + > > + phy->base = base; > > + phy->phy.dev = phy->dev; > > + phy->phy.label = "msm-dw-ssphy"; > > + phy->phy.init = msm_dw_ss_phy_init; > > + phy->phy.shutdown = msm_dw_ss_phy_shutdown; > > + phy->phy.type = USB_PHY_TYPE_USB3; > > + > > + usb_add_phy_dev(&phy->phy); > > Check the error, please. Same comment. > > > + > > + return 0; > > +} > > + <snip> > > > > regards, > Stan Thanks, Ivan -- 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