Date:	Tue, 08 Oct 2013 18:42:32 +0300
From:	"Ivan T. Ivanov" <iivanov@...sol.com>
To:	Stanimir Varbanov <svarbanov@...sol.com>
Cc:	balbi@...com, rob.herring@...xeda.com, pawel.moll@....com,
	mark.rutland@....com, swarren@...dotorg.org,
	ian.campbell@...rix.com, rob@...dley.net,
	gregkh@...uxfoundation.org, grant.likely@...aro.org,
	idos@...eaurora.org, mgautam@...eaurora.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-omap@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers
 for DW PHY's


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@...sol.com>
> > 
> > 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@...sol.com>
> > ---
> >  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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists