All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
To: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
	t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
	s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
	m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
	gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
	mat.krawczuk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Subject: Re: [PATCH v2 1/5] phy: Add new Exynos USB PHY driver
Date: Fri, 25 Oct 2013 21:09:37 +0530	[thread overview]
Message-ID: <526A90B9.3040501@ti.com> (raw)
In-Reply-To: <1382710529-12082-2-git-send-email-k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Hi,

On Friday 25 October 2013 07:45 PM, Kamil Debski wrote:
> Add a new driver for the Exynos USB PHY. The new driver uses the generic
> PHY framework. The driver includes support for the Exynos 4x10 and 4x12
> SoC families.
> 
> Signed-off-by: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/phy/samsung-usbphy.txt     |   51 +++
>  drivers/phy/Kconfig                                |   21 ++
>  drivers/phy/Makefile                               |    3 +
>  drivers/phy/phy-exynos-usb.c                       |  245 ++++++++++++++
>  drivers/phy/phy-exynos-usb.h                       |   94 ++++++
>  drivers/phy/phy-exynos4210-usb.c                   |  295 +++++++++++++++++
>  drivers/phy/phy-exynos4212-usb.c                   |  343 ++++++++++++++++++++
>  7 files changed, 1052 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/samsung-usbphy.txt
>  create mode 100644 drivers/phy/phy-exynos-usb.c
>  create mode 100644 drivers/phy/phy-exynos-usb.h
>  create mode 100644 drivers/phy/phy-exynos4210-usb.c
>  create mode 100644 drivers/phy/phy-exynos4212-usb.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> new file mode 100644
> index 0000000..f112b37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> @@ -0,0 +1,51 @@
> +Samsung S5P/EXYNOS SoC series USB PHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the listed compatibles:
> +	- "samsung,exynos4210-usbphy"
> +	- "samsung,exynos4212-usbphy"
> +- reg : a list of registers used by phy driver
> +	- first and obligatory is the location of phy modules registers
> +	- second and also required is the location of isolation registers
> +	  (isolation registers control the physical connection between the in
> +	  SoC modules and outside of the SoC, this also can be called enable
> +	  control in the documentation of the SoC)
> +	- third is the location of the mode switch register, this only applies
> +	  to SoCs that have such a feature; mode switching enables to have
> +	  both host and device used the same SoC pins and is commonly used
> +	  when OTG is supported
> +- #phy-cells : from the generic phy bindings, must be 1;
> +
> +The second cell in the PHY specifier identifies the PHY its meaning is SoC
> +dependent. For the currently supported SoCs (Exynos 4210 and Exynos 4212) it
> +is as follows:
> +  0 - USB device,
> +  1 - USB host,
> +  2 - HSIC0,
> +  3 - HSIC1,

HSIC is supposedly to be transceiver less no? You have to program something in
the digital side?
You have a single IP that have all these functionalities?
> +
> +Example:
> +
> +For Exynos 4412 (compatible with Exynos 4212):
> +
> +exynos_usbphy: exynos-usbphy@125B0000 {
> +	compatible = "samsung,exynos4212-usbphy";
> +	reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
> +	ranges;
> +	#address-cells = <1>;
> +	#size-cells = <1>;

The above 3 properties aren't documented? Are they needed here?
> +	clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> +							<&clock 2>;
> +	clock-names = "phy", "device", "host", "hsic0", "hsic1";
> +	status = "okay";
> +	#phy-cells = <1>;
> +};
> +
> +Then the PHY can be used in other nodes such as:
> +
> +ehci@12580000 {
> +	status = "okay";
> +	phys = <&exynos_usbphy 2>;
> +	phy-names = "hsic0";
> +};
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 349bef2..2f7ac0a 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -15,4 +15,25 @@ config GENERIC_PHY
>  	  phy users can obtain reference to the PHY. All the users of this
>  	  framework should select this config.
>  
> +config PHY_EXYNOS_USB
> +	tristate "Samsung USB PHY driver (using the Generic PHY Framework)"
Mentioning *Generic PHY Framework* is not necessary.
*select GENERIC_PHY* here
> +	help
> +	  Enable this to support Samsung USB phy helper driver for Samsung SoCs.
> +	  This driver provides common interface to interact, for Samsung
> +	  USB 2.0 PHY driver.

If it's going to be used only for usb2, name it PHY_EXYNOS_USB2.
> +
> +config PHY_EXYNOS4210_USB
> +	bool "Support for Exynos 4210"
> +	depends on PHY_EXYNOS_USB
> +	depends on CPU_EXYNOS4210
> +	help
> +	  Enable USB PHY support for Exynos 4210
> +
> +config PHY_EXYNOS4212_USB
> +	bool "Support for Exynos 4212"
> +	depends on PHY_EXYNOS_USB
> +	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> +	help
> +	  Enable USB PHY support for Exynos 4212

How difference is USB PHY in Exynos 4212 from Exynos 4210? If there isn't much,
I would suggest to use a single driver.
> +
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9e9560f..ca3dc82 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -3,3 +3,6 @@
>  #
>  
>  obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
> +obj-$(CONFIG_PHY_EXYNOS_USB)		+= phy-exynos-usb.o
> +obj-$(CONFIG_PHY_EXYNOS4210_USB)	+= phy-exynos4210-usb.o
> +obj-$(CONFIG_PHY_EXYNOS4212_USB)	+= phy-exynos4212-usb.o
> diff --git a/drivers/phy/phy-exynos-usb.c b/drivers/phy/phy-exynos-usb.c
> new file mode 100644
> index 0000000..d4a26df
> --- /dev/null
> +++ b/drivers/phy/phy-exynos-usb.c

phy-exynos-usb2.c?
> @@ -0,0 +1,245 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "phy-exynos-usb.h"
> +
> +static int exynos_uphy_power_on(struct phy *phy)

exynos_usb2_phy here and everywhere below.
> +{
> +	struct uphy_instance *inst = phy_get_drvdata(phy);
> +	struct uphy_driver *drv = inst->drv;
> +	int ret;
> +
> +	dev_info(drv->dev, "Request to power_on \"%s\" usb phy\n",
> +							inst->cfg->label);

make it dev_dbg if it's necessary.
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		return ret;
> +	if (inst->cfg->power_on) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_on(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +	clk_disable_unprepare(drv->clk);

hmm.. don't you need the clock to be on during the duration of the PHY operation?
> +	return ret;
> +}
> +
> +static int exynos_uphy_power_off(struct phy *phy)
> +{
> +	struct uphy_instance *inst = phy_get_drvdata(phy);
> +	struct uphy_driver *drv = inst->drv;
> +	int ret;
> +
> +	dev_info(drv->dev, "Request to power_off \"%s\" usb phy\n",
> +							inst->cfg->label);

dev_dbg?
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		return ret;
> +	if (inst->cfg->power_off) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_off(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +	clk_disable_unprepare(drv->clk);
> +	return ret;
> +}
> +
> +static struct phy_ops exynos_uphy_ops = {
> +	.power_on	= exynos_uphy_power_on,
> +	.power_off	= exynos_uphy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *exynos_uphy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct uphy_driver *drv;
> +
> +	drv = dev_get_drvdata(dev);
> +	if (!drv)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (WARN_ON(args->args[0] >= drv->cfg->num_phys))
> +		return ERR_PTR(-ENODEV);
> +
> +	return drv->uphy_instances[args->args[0]].phy;
> +}
> +
> +static const struct of_device_id exynos_uphy_of_match[];
> +
> +static int exynos_uphy_probe(struct platform_device *pdev)
> +{
> +	struct uphy_driver *drv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *mem;
> +	struct phy_provider *phy_provider;
> +
> +	const struct of_device_id *match;
> +	const struct uphy_config *cfg;
> +	struct clk *clk;
> +
> +	int i;
> +
> +	match = of_match_node(exynos_uphy_of_match, pdev->dev.of_node);
> +	if (!match) {
> +		dev_err(dev, "of_match_node() failed\n");
> +		return -EINVAL;
> +	}
> +	cfg = match->data;
> +	if (!cfg) {
> +		dev_err(dev, "Failed to get configuration\n");
> +		return -EINVAL;
> +	}
> +
> +	drv = devm_kzalloc(dev, sizeof(struct uphy_driver) +
> +		cfg->num_phys * sizeof(struct uphy_instance), GFP_KERNEL);
> +
> +	if (!drv) {
> +		dev_err(dev, "Failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(dev, drv);
> +	spin_lock_init(&drv->lock);
> +
> +	drv->cfg = cfg;
> +	drv->dev = dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
empty blank line.
> +	drv->reg_phy = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(drv->reg_phy)) {
> +		dev_err(dev, "Failed to map register memory (phy)\n");
> +		return PTR_ERR(drv->reg_phy);
> +	}
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	drv->reg_isol = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(drv->reg_isol)) {
> +		dev_err(dev, "Failed to map register memory (isolation)\n");
> +		return PTR_ERR(drv->reg_isol);
> +	}
> +
> +	switch (drv->cfg->cpu) {
> +	case TYPE_EXYNOS4210:
> +	case TYPE_EXYNOS4212:

Lets not add such cpu checks inside driver.
> +		mem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		drv->reg_mode = devm_ioremap_resource(dev, mem);
> +		if (IS_ERR(drv->reg_mode)) {
> +			dev_err(dev, "Failed to map register memory (mode switch)\n");
> +			return PTR_ERR(drv->reg_mode);
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev,
> +							exynos_uphy_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(drv->dev, "Failed to register phy provider\n");
> +		return PTR_ERR(phy_provider);
> +	}
> +
> +	drv->clk = devm_clk_get(dev, "phy");
> +	if (IS_ERR(drv->clk)) {
> +		dev_err(dev, "Failed to get clock of phy controller\n");
> +		return PTR_ERR(drv->clk);
> +	}
> +
> +	for (i = 0; i < drv->cfg->num_phys; i++) {
> +		char *label = drv->cfg->phys[i].label;
> +		struct uphy_instance *p = &drv->uphy_instances[i];
> +
> +		dev_info(dev, "Creating phy \"%s\"\n", label);
> +		p->phy = devm_phy_create(dev, &exynos_uphy_ops, NULL);
> +		if (IS_ERR(p->phy)) {
> +			dev_err(drv->dev, "Failed to create uphy \"%s\"\n",
> +						label);
> +			return PTR_ERR(p->phy);
> +		}
> +
> +		p->cfg = &drv->cfg->phys[i];
> +		p->drv = drv;
> +		phy_set_drvdata(p->phy, p);
> +
> +		clk = clk_get(dev, p->cfg->label);
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "Failed to get clock of \"%s\" phy\n",
> +								p->cfg->label);
> +			return PTR_ERR(clk);
> +		}
> +
> +		p->rate = clk_get_rate(clk);
> +
> +		if (p->cfg->rate_to_clk) {
> +			p->clk = p->cfg->rate_to_clk(p->rate);
> +			if (p->clk == CLKSEL_ERROR) {
> +				dev_err(dev, "Clock rate (%ld) not supported\n",
> +								p->rate);
> +				clk_put(clk);
> +				return -EINVAL;
> +			}
> +		}
> +		clk_put(clk);
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PHY_EXYNOS4210_USB
Do we really need this?

> +extern const struct uphy_config exynos4210_uphy_config;
> +#endif
> +
> +#ifdef CONFIG_PHY_EXYNOS4212_USB

Same here.
> +extern const struct uphy_config exynos4212_uphy_config;
> +#endif
> +
> +static const struct of_device_id exynos_uphy_of_match[] = {
> +#ifdef CONFIG_PHY_EXYNOS4210_USB

#if not needed here.
> +	{
> +		.compatible = "samsung,exynos4210-usbphy",
> +		.data = &exynos4210_uphy_config,
> +	},
> +#endif
> +#ifdef CONFIG_PHY_EXYNOS4212_USB

here too.
> +	{
> +		.compatible = "samsung,exynos4212-usbphy",
> +		.data = &exynos4212_uphy_config,
> +	},
> +#endif
> +	{ },
> +};
> +
> +static struct platform_driver exynos_uphy_driver = {
> +	.probe	= exynos_uphy_probe,
> +	.driver = {
> +		.of_match_table	= exynos_uphy_of_match,
> +		.name		= "exynos-usbphy-new",
"exynos-usb2-phy".
> +		.owner		= THIS_MODULE,
> +	}
> +};
> +
> +module_platform_driver(exynos_uphy_driver);
> +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> +MODULE_AUTHOR("Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-uphy-new");
> +
> diff --git a/drivers/phy/phy-exynos-usb.h b/drivers/phy/phy-exynos-usb.h
> new file mode 100644
> index 0000000..f45cb3c
> --- /dev/null
> +++ b/drivers/phy/phy-exynos-usb.h
> @@ -0,0 +1,94 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _PHY_SAMSUNG_NEW_H
> +#define _PHY_SAMSUNG_NEW_H
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define CLKSEL_ERROR                       -1
> +
> +#ifndef KHZ
> +#define KHZ 1000
> +#endif
> +
> +#ifndef MHZ
> +#define MHZ (KHZ * KHZ)
> +#endif
> +
> +enum phy_type {
> +	PHY_DEVICE,
> +	PHY_HOST,
> +};
> +
> +enum samsung_cpu_type {
> +	TYPE_S3C64XX,
> +	TYPE_EXYNOS4210,
> +	TYPE_EXYNOS4212,

No *cpu_type* inside driver files.
> +};
> +
> +enum uphy_state {
> +	STATE_OFF,
> +	STATE_ON,
> +};
> +
> +struct uphy_driver;
> +struct uphy_instance;
> +struct uphy_config;
> +
> +struct uphy_instance {
> +	struct uphy_driver *drv;
> +	struct phy *phy;
> +	const struct common_phy *cfg;
> +	enum uphy_state state;
> +	int ref_cnt;
> +	u32 clk;
> +	unsigned long rate;
> +};
> +
> +struct uphy_driver {
> +	struct device *dev;
> +	spinlock_t lock;
> +	void __iomem *reg_phy;
> +	void __iomem *reg_isol;
> +	void __iomem *reg_mode;
> +	const struct uphy_config *cfg;
> +	struct clk *clk;
> +	struct uphy_instance uphy_instances[0];

you might have more than one phy instance right?
> +};
> +
> +struct common_phy {
> +	char *label;
> +	enum phy_type type;
> +	unsigned int id;
> +	u32 (*rate_to_clk)(unsigned long);
> +	int (*power_on)(struct uphy_instance*);
> +	int (*power_off)(struct uphy_instance*);
> +};
> +
> +
> +struct uphy_config {
> +	enum samsung_cpu_type cpu;
> +	int num_phys;
> +	const struct common_phy *phys;
> +};
> +
> +#endif
> +
> diff --git a/drivers/phy/phy-exynos4210-usb.c b/drivers/phy/phy-exynos4210-usb.c
> new file mode 100644
> index 0000000..6cf74f7
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4210-usb.c
> @@ -0,0 +1,295 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "phy-exynos-usb.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4210_UPHYPWR			0x0
> +
> +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
> +#define EXYNOS_4210_UPHYPWR_PHY0_PWR		(1 << 3)
> +#define EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR	(1 << 4)
> +#define EXYNOS_4210_UPHYPWR_PHY0_SLEEP		(1 << 5)
> +#define EXYNOS_4210_UPHYPWR_PHY0	( \
> +	EXYNOS_4210_UPHYPWR_PHY0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY0_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_PHY1_SUSPEND	(1 << 6)
> +#define EXYNOS_4210_UPHYPWR_PHY1_PWR		(1 << 7)
> +#define EXYNOS_4210_UPHYPWR_PHY1_SLEEP		(1 << 8)
> +#define EXYNOS_4210_UPHYPWR_PHY1 ( \
> +	EXYNOS_4210_UPHYPWR_PHY1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY1_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY1_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SLEEP		(1 << 10)
> +#define EXYNOS_4210_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND	(1 << 11)
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SLEEP		(1 << 12)
> +#define EXYNOS_4210_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4210_UPHYCLK			0x4
> +
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_MASK	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ	(0x0 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +
> +#define EXYNOS_4210_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 2)
> +#define EXYNOS_4210_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4210_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +/* PHY reset control */
> +#define EXYNOS_4210_UPHYRST			0x8
> +
> +#define EXYNOS_4210_URSTCON_PHY0		(1 << 0)
> +#define EXYNOS_4210_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4210_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4210_URSTCON_PHY1_ALL		(1 << 3)
> +#define EXYNOS_4210_URSTCON_PHY1_P0		(1 << 4)
> +#define EXYNOS_4210_URSTCON_PHY1_P1P2		(1 << 5)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_ALL	(1 << 6)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P0	(1 << 7)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P1	(1 << 8)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P2	(1 << 9)But 
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET	0x0
> +#define EXYNOS_4210_USB_ISOL_DEVICE		(1 << 0)
> +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET	0x4
> +#define EXYNOS_4210_USB_ISOL_HOST		(1 << 0)
> +
> +/* USBYPHY1 Floating prevention */
> +#define EXYNOS_4210_UPHY1CON			0x34
> +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION	0x1
> +
> +enum exynos4210_phy_id {
> +	EXYNOS4210_DEVICE,
> +	EXYNOS4210_HOST,
> +	EXYNOS4210_HSIC0,
> +	EXYNOS4210_HSIC1,
> +	EXYNOS4210_NUM_PHYS,
> +};
> +
> +/* exynos4210_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register. */

use proper commenting styles Documentation/CodingStyle
> +static u32 exynos4210_rate_to_clk(unsigned long rate)
> +{
> +	unsigned int clksel;
> +
> +	switch (rate) {
> +	case 12 * MHZ:
> +		clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 24 * MHZ:
> +		clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 48 * MHZ:
> +		clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;
> +		break;
> +	default:
> +		clksel = CLKSEL_ERROR;
> +	}
> +
> +	return clksel;
> +}
> +
> +static void exynos4210_isol(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +	u32 tmp;
> +
> +	if (!drv->reg_isol)
> +		return;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		offset = EXYNOS_4210_USB_ISOL_DEVICE_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_DEVICE;
> +		break;
> +	case EXYNOS4210_HOST:
> +		offset = EXYNOS_4210_USB_ISOL_HOST_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_HOST;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	tmp = readl(drv->reg_isol + offset);
> +	if (on)
> +		tmp &= ~mask;
> +	else
> +		tmp |= mask;
> +	writel(tmp, drv->reg_isol + offset);
> +}
> +
> +static void exynos4210_phy_pwr(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY0;
> +		break;
> +	case EXYNOS4210_HOST:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_ALL |
> +				EXYNOS_4210_URSTCON_PHY1_P0 |
> +				EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_ALL |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P0;
> +		writel(on, drv->reg_phy + EXYNOS_4210_UPHY1CON);
> +		break;
> +	case EXYNOS4210_HSIC0:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P1;
> +		break;
> +	case EXYNOS4210_HSIC1:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P2;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk, drv->reg_phy + EXYNOS_4210_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		udelay(10);

usleep_range?
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4210_power_on(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_ON) {
> +		dev_err(drv->dev, "usb phy \"%s\" already on",
> +							inst->cfg->label);
> +		return -ENODEV;
> +	}
> +	inst->state = STATE_ON;
> +	inst->ref_cnt++;
> +	if (inst->ref_cnt > 1)
> +		return 0;

reference counting shouldn't be needed. It's been handled by the PHY framework.
> +
> +	/* Order of initialisation is important - first power then isolation */
> +	exynos4210_phy_pwr(inst, 1);
> +	exynos4210_isol(inst, 0);
> +
> +	return 0;
> +}
> +
> +static int exynos4210_power_off(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_OFF) {
> +		dev_err(drv->dev, "usb phy \"%s\" already off",
> +							inst->cfg->label);
> +		return -EINVAL;
> +	}
> +
> +	inst->state = STATE_OFF;
> +	inst->ref_cnt--;
> +	if (inst->ref_cnt > 0)
> +		return 0;

same here.
> +
> +	exynos4210_isol(inst, 1);
> +	exynos4210_phy_pwr(inst, 0);
> +
> +	return 0;
> +}
> +
> +
> +static const struct common_phy exynos4210_phys[] = {
> +	{
> +		.label		= "device",
> +		.type		= PHY_DEVICE,
> +		.id		= EXYNOS4210_DEVICE,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4210_HOST,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4210_HSIC0,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4210_HSIC1,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{},
> +};
> +
> +const struct uphy_config exynos4210_uphy_config = {
> +	.cpu		= TYPE_EXYNOS4210,
> +	.num_phys	= EXYNOS4210_NUM_PHYS,
> +	.phys		= exynos4210_phys,
> +};
> +
> diff --git a/drivers/phy/phy-exynos4212-usb.c b/drivers/phy/phy-exynos4212-usb.c
> new file mode 100644
> index 0000000..c07ae8e
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4212-usb.c
> @@ -0,0 +1,343 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "phy-exynos-usb.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4212_UPHYPWR			0x0
> +
> +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND		(1 << 0)
> +#define EXYNOS_4212_UPHYPWR_DEV_PWR		(1 << 3)
> +#define EXYNOS_4212_UPHYPWR_DEV_OTG_PWR		(1 << 4)
> +#define EXYNOS_4212_UPHYPWR_DEV_SLEEP		(1 << 5)
> +#define EXYNOS_4212_UPHYPWR_DEV	( \
> +	EXYNOS_4212_UPHYPWR_DEV_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_DEV_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_OTG_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HOST_SUSPEND	(1 << 6)
> +#define EXYNOS_4212_UPHYPWR_HOST_PWR		(1 << 7)
> +#define EXYNOS_4212_UPHYPWR_HOST_SLEEP		(1 << 8)
> +#define EXYNOS_4212_UPHYPWR_HOST ( \
> +	EXYNOS_4212_UPHYPWR_HOST_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HOST_PWR | \
> +	EXYNOS_4212_UPHYPWR_HOST_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_PWR		(1 << 10)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SLEEP		(1 << 11)
> +#define EXYNOS_4212_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND	(1 << 12)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_PWR		(1 << 13)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SLEEP		(1 << 14)
> +#define EXYNOS_4212_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4212_UPHYCLK			0x4
> +
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_MASK	(0x7 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6	(0x0 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ	(0x1 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2	(0x3 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ	(0x4 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ	(0x5 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ	(0x7 << 0)
> +
> +#define EXYNOS_4212_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 3)
> +#define EXYNOS_4212_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4212_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_MASK	(0x7f << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_12MHZ	(0x24 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_15MHZ	(0x1c << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_16MHZ	(0x1a << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_19MHZ2	(0x15 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_20MHZ	(0x14 << 10)
> +
> +/* PHY reset control */
> +#define EXYNOS_4212_UPHYRST			0x8
> +
> +#define EXYNOS_4212_URSTCON_DEVICE		(1 << 0)
> +#define EXYNOS_4212_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4212_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4212_URSTCON_HOST_PHY		(1 << 3)
> +#define EXYNOS_4212_URSTCON_PHY1		(1 << 4)
> +#define EXYNOS_4212_URSTCON_HSIC0		(1 << 5)
> +#define EXYNOS_4212_URSTCON_HSIC1		(1 << 6)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_ALL	(1 << 7)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P0	(1 << 8)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P1	(1 << 9)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P2	(1 << 10)
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4212_USB_ISOL_OFFSET		0x0
> +#define EXYNOS_4212_USB_ISOL_OTG		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET	0x4
> +#define EXYNOS_4212_USB_ISOL_HSIC0		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET	0x8
> +#define EXYNOS_4212_USB_ISOL_HSIC1		(1 << 0)
> +
> +enum exynos4x12_phy_id {
> +	EXYNOS4212_DEVICE,
> +	EXYNOS4212_HOST,
> +	EXYNOS4212_HSIC0,
> +	EXYNOS4212_HSIC1,
> +	EXYNOS4212_NUM_PHYS,
> +};
> +
> +/* exynos4212_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register. */
> +static u32 exynos4212_rate_to_clk(unsigned long rate)
> +{
> +	unsigned int clksel;
> +
> +	/* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> +
> +	switch (rate) {
> +	case 9600 * KHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> +		break;
> +	case 10 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> +		break;
> +	case 12 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 19200 * KHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> +		break;
> +	case 20 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> +		break;
> +	case 24 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 50 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> +		break;
> +	default:
> +		clksel = CLKSEL_ERROR;
> +	}
> +
> +	return clksel;
> +}
> +
> +static void exynos4212_isol(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +	u32 tmp;
> +
> +	if (!drv->reg_isol)
> +		return;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_OTG;
> +		break;
> +	case EXYNOS4212_HOST:
> +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_OTG;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC0_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC0;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC1_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC1;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	tmp = readl(drv->reg_isol + offset);
> +	if (on)
> +		tmp &= ~mask;
> +	else
> +		tmp |= mask;
> +	writel(tmp, drv->reg_isol + offset);
> +}
> +
> +static void exynos4212_phy_pwr(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +		phypwr =	EXYNOS_4212_UPHYPWR_DEV;
> +		rstbits =	EXYNOS_4212_URSTCON_DEVICE;
> +		break;
> +	case EXYNOS4212_HOST:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HOST;
> +		rstbits =	EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P0 |
> +				EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P1;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk, drv->reg_phy + EXYNOS_4212_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		udelay(10);
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4212_power_on(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_ON) {
> +		dev_err(drv->dev, "usb phy \"%s\" already on",
> +							inst->cfg->label);
> +		return -ENODEV;
> +	}
> +
> +	inst->state = STATE_ON;
> +	inst->ref_cnt++;
> +	if (inst->ref_cnt > 1)
> +		return 0;
> +
> +	exynos4212_phy_pwr(inst, 1);
> +	exynos4212_isol(inst, 0);
> +
> +	/* Power on the device, as it is necessary for HSIC to work */
> +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> +		struct uphy_instance *device =
> +					&drv->uphy_instances[EXYNOS4212_DEVICE];
> +		exynos4212_phy_pwr(device, 1);
> +		exynos4212_isol(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int exynos4212_power_off(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_OFF) {
> +		dev_err(drv->dev, "usb phy \"%s\" already off",
> +							inst->cfg->label);
> +		return -EINVAL;
> +	}
> +
> +	inst->state = STATE_OFF;
> +	inst->ref_cnt--;
> +
> +	if (inst->ref_cnt > 0)
> +		return 0;
> +
> +	exynos4212_isol(inst, 1);
> +	exynos4212_phy_pwr(inst, 0);
> +
> +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> +		struct uphy_instance *device =
> +					&drv->uphy_instances[EXYNOS4212_DEVICE];
> +		exynos4212_isol(device, 1);
> +		exynos4212_phy_pwr(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static const struct common_phy exynos4212_phys[] = {
> +	{
> +		.label		= "device",
> +		.type		= PHY_DEVICE,
> +		.id		= EXYNOS4212_DEVICE,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4212_HOST,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4212_HSIC0,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4212_HSIC1,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{},
> +};
> +
> +const struct uphy_config exynos4212_uphy_config = {
> +	.cpu		= TYPE_EXYNOS4212,
> +	.num_phys	= EXYNOS4212_NUM_PHYS,
> +	.phys		= exynos4212_phys,
> +};

I see quite an amount of similarities between the two PHY drivers. It would be
better if we can have a single driver to handle it.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID (diff)
From: Kishon Vijay Abraham I <kishon@ti.com>
To: Kamil Debski <k.debski@samsung.com>
Cc: <linux-kernel@vger.kernel.org>,
	<linux-samsung-soc@vger.kernel.org>, <linux-usb@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-arm@vger.kernel.org>,
	<kyungmin.park@samsung.com>, <t.figa@samsung.com>,
	<s.nawrocki@samsung.com>, <m.szyprowski@samsung.com>,
	<gautam.vivek@samsung.com>, <mat.krawczuk@gmail.com>
Subject: Re: [PATCH v2 1/5] phy: Add new Exynos USB PHY driver
Date: Fri, 25 Oct 2013 21:09:37 +0530	[thread overview]
Message-ID: <526A90B9.3040501@ti.com> (raw)
In-Reply-To: <1382710529-12082-2-git-send-email-k.debski@samsung.com>

Hi,

On Friday 25 October 2013 07:45 PM, Kamil Debski wrote:
> Add a new driver for the Exynos USB PHY. The new driver uses the generic
> PHY framework. The driver includes support for the Exynos 4x10 and 4x12
> SoC families.
> 
> Signed-off-by: Kamil Debski <k.debski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/phy/samsung-usbphy.txt     |   51 +++
>  drivers/phy/Kconfig                                |   21 ++
>  drivers/phy/Makefile                               |    3 +
>  drivers/phy/phy-exynos-usb.c                       |  245 ++++++++++++++
>  drivers/phy/phy-exynos-usb.h                       |   94 ++++++
>  drivers/phy/phy-exynos4210-usb.c                   |  295 +++++++++++++++++
>  drivers/phy/phy-exynos4212-usb.c                   |  343 ++++++++++++++++++++
>  7 files changed, 1052 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/samsung-usbphy.txt
>  create mode 100644 drivers/phy/phy-exynos-usb.c
>  create mode 100644 drivers/phy/phy-exynos-usb.h
>  create mode 100644 drivers/phy/phy-exynos4210-usb.c
>  create mode 100644 drivers/phy/phy-exynos4212-usb.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> new file mode 100644
> index 0000000..f112b37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
> @@ -0,0 +1,51 @@
> +Samsung S5P/EXYNOS SoC series USB PHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the listed compatibles:
> +	- "samsung,exynos4210-usbphy"
> +	- "samsung,exynos4212-usbphy"
> +- reg : a list of registers used by phy driver
> +	- first and obligatory is the location of phy modules registers
> +	- second and also required is the location of isolation registers
> +	  (isolation registers control the physical connection between the in
> +	  SoC modules and outside of the SoC, this also can be called enable
> +	  control in the documentation of the SoC)
> +	- third is the location of the mode switch register, this only applies
> +	  to SoCs that have such a feature; mode switching enables to have
> +	  both host and device used the same SoC pins and is commonly used
> +	  when OTG is supported
> +- #phy-cells : from the generic phy bindings, must be 1;
> +
> +The second cell in the PHY specifier identifies the PHY its meaning is SoC
> +dependent. For the currently supported SoCs (Exynos 4210 and Exynos 4212) it
> +is as follows:
> +  0 - USB device,
> +  1 - USB host,
> +  2 - HSIC0,
> +  3 - HSIC1,

HSIC is supposedly to be transceiver less no? You have to program something in
the digital side?
You have a single IP that have all these functionalities?
> +
> +Example:
> +
> +For Exynos 4412 (compatible with Exynos 4212):
> +
> +exynos_usbphy: exynos-usbphy@125B0000 {
> +	compatible = "samsung,exynos4212-usbphy";
> +	reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>;
> +	ranges;
> +	#address-cells = <1>;
> +	#size-cells = <1>;

The above 3 properties aren't documented? Are they needed here?
> +	clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>,
> +							<&clock 2>;
> +	clock-names = "phy", "device", "host", "hsic0", "hsic1";
> +	status = "okay";
> +	#phy-cells = <1>;
> +};
> +
> +Then the PHY can be used in other nodes such as:
> +
> +ehci@12580000 {
> +	status = "okay";
> +	phys = <&exynos_usbphy 2>;
> +	phy-names = "hsic0";
> +};
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 349bef2..2f7ac0a 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -15,4 +15,25 @@ config GENERIC_PHY
>  	  phy users can obtain reference to the PHY. All the users of this
>  	  framework should select this config.
>  
> +config PHY_EXYNOS_USB
> +	tristate "Samsung USB PHY driver (using the Generic PHY Framework)"
Mentioning *Generic PHY Framework* is not necessary.
*select GENERIC_PHY* here
> +	help
> +	  Enable this to support Samsung USB phy helper driver for Samsung SoCs.
> +	  This driver provides common interface to interact, for Samsung
> +	  USB 2.0 PHY driver.

If it's going to be used only for usb2, name it PHY_EXYNOS_USB2.
> +
> +config PHY_EXYNOS4210_USB
> +	bool "Support for Exynos 4210"
> +	depends on PHY_EXYNOS_USB
> +	depends on CPU_EXYNOS4210
> +	help
> +	  Enable USB PHY support for Exynos 4210
> +
> +config PHY_EXYNOS4212_USB
> +	bool "Support for Exynos 4212"
> +	depends on PHY_EXYNOS_USB
> +	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> +	help
> +	  Enable USB PHY support for Exynos 4212

How difference is USB PHY in Exynos 4212 from Exynos 4210? If there isn't much,
I would suggest to use a single driver.
> +
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9e9560f..ca3dc82 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -3,3 +3,6 @@
>  #
>  
>  obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
> +obj-$(CONFIG_PHY_EXYNOS_USB)		+= phy-exynos-usb.o
> +obj-$(CONFIG_PHY_EXYNOS4210_USB)	+= phy-exynos4210-usb.o
> +obj-$(CONFIG_PHY_EXYNOS4212_USB)	+= phy-exynos4212-usb.o
> diff --git a/drivers/phy/phy-exynos-usb.c b/drivers/phy/phy-exynos-usb.c
> new file mode 100644
> index 0000000..d4a26df
> --- /dev/null
> +++ b/drivers/phy/phy-exynos-usb.c

phy-exynos-usb2.c?
> @@ -0,0 +1,245 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "phy-exynos-usb.h"
> +
> +static int exynos_uphy_power_on(struct phy *phy)

exynos_usb2_phy here and everywhere below.
> +{
> +	struct uphy_instance *inst = phy_get_drvdata(phy);
> +	struct uphy_driver *drv = inst->drv;
> +	int ret;
> +
> +	dev_info(drv->dev, "Request to power_on \"%s\" usb phy\n",
> +							inst->cfg->label);

make it dev_dbg if it's necessary.
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		return ret;
> +	if (inst->cfg->power_on) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_on(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +	clk_disable_unprepare(drv->clk);

hmm.. don't you need the clock to be on during the duration of the PHY operation?
> +	return ret;
> +}
> +
> +static int exynos_uphy_power_off(struct phy *phy)
> +{
> +	struct uphy_instance *inst = phy_get_drvdata(phy);
> +	struct uphy_driver *drv = inst->drv;
> +	int ret;
> +
> +	dev_info(drv->dev, "Request to power_off \"%s\" usb phy\n",
> +							inst->cfg->label);

dev_dbg?
> +	ret = clk_prepare_enable(drv->clk);
> +	if (ret)
> +		return ret;
> +	if (inst->cfg->power_off) {
> +		spin_lock(&drv->lock);
> +		ret = inst->cfg->power_off(inst);
> +		spin_unlock(&drv->lock);
> +	}
> +	clk_disable_unprepare(drv->clk);
> +	return ret;
> +}
> +
> +static struct phy_ops exynos_uphy_ops = {
> +	.power_on	= exynos_uphy_power_on,
> +	.power_off	= exynos_uphy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *exynos_uphy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct uphy_driver *drv;
> +
> +	drv = dev_get_drvdata(dev);
> +	if (!drv)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (WARN_ON(args->args[0] >= drv->cfg->num_phys))
> +		return ERR_PTR(-ENODEV);
> +
> +	return drv->uphy_instances[args->args[0]].phy;
> +}
> +
> +static const struct of_device_id exynos_uphy_of_match[];
> +
> +static int exynos_uphy_probe(struct platform_device *pdev)
> +{
> +	struct uphy_driver *drv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *mem;
> +	struct phy_provider *phy_provider;
> +
> +	const struct of_device_id *match;
> +	const struct uphy_config *cfg;
> +	struct clk *clk;
> +
> +	int i;
> +
> +	match = of_match_node(exynos_uphy_of_match, pdev->dev.of_node);
> +	if (!match) {
> +		dev_err(dev, "of_match_node() failed\n");
> +		return -EINVAL;
> +	}
> +	cfg = match->data;
> +	if (!cfg) {
> +		dev_err(dev, "Failed to get configuration\n");
> +		return -EINVAL;
> +	}
> +
> +	drv = devm_kzalloc(dev, sizeof(struct uphy_driver) +
> +		cfg->num_phys * sizeof(struct uphy_instance), GFP_KERNEL);
> +
> +	if (!drv) {
> +		dev_err(dev, "Failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(dev, drv);
> +	spin_lock_init(&drv->lock);
> +
> +	drv->cfg = cfg;
> +	drv->dev = dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
empty blank line.
> +	drv->reg_phy = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(drv->reg_phy)) {
> +		dev_err(dev, "Failed to map register memory (phy)\n");
> +		return PTR_ERR(drv->reg_phy);
> +	}
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	drv->reg_isol = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(drv->reg_isol)) {
> +		dev_err(dev, "Failed to map register memory (isolation)\n");
> +		return PTR_ERR(drv->reg_isol);
> +	}
> +
> +	switch (drv->cfg->cpu) {
> +	case TYPE_EXYNOS4210:
> +	case TYPE_EXYNOS4212:

Lets not add such cpu checks inside driver.
> +		mem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		drv->reg_mode = devm_ioremap_resource(dev, mem);
> +		if (IS_ERR(drv->reg_mode)) {
> +			dev_err(dev, "Failed to map register memory (mode switch)\n");
> +			return PTR_ERR(drv->reg_mode);
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev,
> +							exynos_uphy_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(drv->dev, "Failed to register phy provider\n");
> +		return PTR_ERR(phy_provider);
> +	}
> +
> +	drv->clk = devm_clk_get(dev, "phy");
> +	if (IS_ERR(drv->clk)) {
> +		dev_err(dev, "Failed to get clock of phy controller\n");
> +		return PTR_ERR(drv->clk);
> +	}
> +
> +	for (i = 0; i < drv->cfg->num_phys; i++) {
> +		char *label = drv->cfg->phys[i].label;
> +		struct uphy_instance *p = &drv->uphy_instances[i];
> +
> +		dev_info(dev, "Creating phy \"%s\"\n", label);
> +		p->phy = devm_phy_create(dev, &exynos_uphy_ops, NULL);
> +		if (IS_ERR(p->phy)) {
> +			dev_err(drv->dev, "Failed to create uphy \"%s\"\n",
> +						label);
> +			return PTR_ERR(p->phy);
> +		}
> +
> +		p->cfg = &drv->cfg->phys[i];
> +		p->drv = drv;
> +		phy_set_drvdata(p->phy, p);
> +
> +		clk = clk_get(dev, p->cfg->label);
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "Failed to get clock of \"%s\" phy\n",
> +								p->cfg->label);
> +			return PTR_ERR(clk);
> +		}
> +
> +		p->rate = clk_get_rate(clk);
> +
> +		if (p->cfg->rate_to_clk) {
> +			p->clk = p->cfg->rate_to_clk(p->rate);
> +			if (p->clk == CLKSEL_ERROR) {
> +				dev_err(dev, "Clock rate (%ld) not supported\n",
> +								p->rate);
> +				clk_put(clk);
> +				return -EINVAL;
> +			}
> +		}
> +		clk_put(clk);
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PHY_EXYNOS4210_USB
Do we really need this?

> +extern const struct uphy_config exynos4210_uphy_config;
> +#endif
> +
> +#ifdef CONFIG_PHY_EXYNOS4212_USB

Same here.
> +extern const struct uphy_config exynos4212_uphy_config;
> +#endif
> +
> +static const struct of_device_id exynos_uphy_of_match[] = {
> +#ifdef CONFIG_PHY_EXYNOS4210_USB

#if not needed here.
> +	{
> +		.compatible = "samsung,exynos4210-usbphy",
> +		.data = &exynos4210_uphy_config,
> +	},
> +#endif
> +#ifdef CONFIG_PHY_EXYNOS4212_USB

here too.
> +	{
> +		.compatible = "samsung,exynos4212-usbphy",
> +		.data = &exynos4212_uphy_config,
> +	},
> +#endif
> +	{ },
> +};
> +
> +static struct platform_driver exynos_uphy_driver = {
> +	.probe	= exynos_uphy_probe,
> +	.driver = {
> +		.of_match_table	= exynos_uphy_of_match,
> +		.name		= "exynos-usbphy-new",
"exynos-usb2-phy".
> +		.owner		= THIS_MODULE,
> +	}
> +};
> +
> +module_platform_driver(exynos_uphy_driver);
> +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver");
> +MODULE_AUTHOR("Kamil Debski <k.debski@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:exynos-uphy-new");
> +
> diff --git a/drivers/phy/phy-exynos-usb.h b/drivers/phy/phy-exynos-usb.h
> new file mode 100644
> index 0000000..f45cb3c
> --- /dev/null
> +++ b/drivers/phy/phy-exynos-usb.h
> @@ -0,0 +1,94 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _PHY_SAMSUNG_NEW_H
> +#define _PHY_SAMSUNG_NEW_H
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define CLKSEL_ERROR                       -1
> +
> +#ifndef KHZ
> +#define KHZ 1000
> +#endif
> +
> +#ifndef MHZ
> +#define MHZ (KHZ * KHZ)
> +#endif
> +
> +enum phy_type {
> +	PHY_DEVICE,
> +	PHY_HOST,
> +};
> +
> +enum samsung_cpu_type {
> +	TYPE_S3C64XX,
> +	TYPE_EXYNOS4210,
> +	TYPE_EXYNOS4212,

No *cpu_type* inside driver files.
> +};
> +
> +enum uphy_state {
> +	STATE_OFF,
> +	STATE_ON,
> +};
> +
> +struct uphy_driver;
> +struct uphy_instance;
> +struct uphy_config;
> +
> +struct uphy_instance {
> +	struct uphy_driver *drv;
> +	struct phy *phy;
> +	const struct common_phy *cfg;
> +	enum uphy_state state;
> +	int ref_cnt;
> +	u32 clk;
> +	unsigned long rate;
> +};
> +
> +struct uphy_driver {
> +	struct device *dev;
> +	spinlock_t lock;
> +	void __iomem *reg_phy;
> +	void __iomem *reg_isol;
> +	void __iomem *reg_mode;
> +	const struct uphy_config *cfg;
> +	struct clk *clk;
> +	struct uphy_instance uphy_instances[0];

you might have more than one phy instance right?
> +};
> +
> +struct common_phy {
> +	char *label;
> +	enum phy_type type;
> +	unsigned int id;
> +	u32 (*rate_to_clk)(unsigned long);
> +	int (*power_on)(struct uphy_instance*);
> +	int (*power_off)(struct uphy_instance*);
> +};
> +
> +
> +struct uphy_config {
> +	enum samsung_cpu_type cpu;
> +	int num_phys;
> +	const struct common_phy *phys;
> +};
> +
> +#endif
> +
> diff --git a/drivers/phy/phy-exynos4210-usb.c b/drivers/phy/phy-exynos4210-usb.c
> new file mode 100644
> index 0000000..6cf74f7
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4210-usb.c
> @@ -0,0 +1,295 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "phy-exynos-usb.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4210_UPHYPWR			0x0
> +
> +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND	(1 << 0)
> +#define EXYNOS_4210_UPHYPWR_PHY0_PWR		(1 << 3)
> +#define EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR	(1 << 4)
> +#define EXYNOS_4210_UPHYPWR_PHY0_SLEEP		(1 << 5)
> +#define EXYNOS_4210_UPHYPWR_PHY0	( \
> +	EXYNOS_4210_UPHYPWR_PHY0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY0_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_OTG_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_PHY1_SUSPEND	(1 << 6)
> +#define EXYNOS_4210_UPHYPWR_PHY1_PWR		(1 << 7)
> +#define EXYNOS_4210_UPHYPWR_PHY1_SLEEP		(1 << 8)
> +#define EXYNOS_4210_UPHYPWR_PHY1 ( \
> +	EXYNOS_4210_UPHYPWR_PHY1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_PHY1_PWR | \
> +	EXYNOS_4210_UPHYPWR_PHY1_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4210_UPHYPWR_HSCI0_SLEEP		(1 << 10)
> +#define EXYNOS_4210_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND	(1 << 11)
> +#define EXYNOS_4210_UPHYPWR_HSCI1_SLEEP		(1 << 12)
> +#define EXYNOS_4210_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4210_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4210_UPHYCLK			0x4
> +
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_MASK	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ	(0x0 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ	(0x3 << 0)
> +#define EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +
> +#define EXYNOS_4210_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 2)
> +#define EXYNOS_4210_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4210_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +/* PHY reset control */
> +#define EXYNOS_4210_UPHYRST			0x8
> +
> +#define EXYNOS_4210_URSTCON_PHY0		(1 << 0)
> +#define EXYNOS_4210_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4210_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4210_URSTCON_PHY1_ALL		(1 << 3)
> +#define EXYNOS_4210_URSTCON_PHY1_P0		(1 << 4)
> +#define EXYNOS_4210_URSTCON_PHY1_P1P2		(1 << 5)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_ALL	(1 << 6)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P0	(1 << 7)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P1	(1 << 8)
> +#define EXYNOS_4210_URSTCON_HOST_LINK_P2	(1 << 9)But 
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET	0x0
> +#define EXYNOS_4210_USB_ISOL_DEVICE		(1 << 0)
> +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET	0x4
> +#define EXYNOS_4210_USB_ISOL_HOST		(1 << 0)
> +
> +/* USBYPHY1 Floating prevention */
> +#define EXYNOS_4210_UPHY1CON			0x34
> +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION	0x1
> +
> +enum exynos4210_phy_id {
> +	EXYNOS4210_DEVICE,
> +	EXYNOS4210_HOST,
> +	EXYNOS4210_HSIC0,
> +	EXYNOS4210_HSIC1,
> +	EXYNOS4210_NUM_PHYS,
> +};
> +
> +/* exynos4210_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register. */

use proper commenting styles Documentation/CodingStyle
> +static u32 exynos4210_rate_to_clk(unsigned long rate)
> +{
> +	unsigned int clksel;
> +
> +	switch (rate) {
> +	case 12 * MHZ:
> +		clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 24 * MHZ:
> +		clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 48 * MHZ:
> +		clksel = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ;
> +		break;
> +	default:
> +		clksel = CLKSEL_ERROR;
> +	}
> +
> +	return clksel;
> +}
> +
> +static void exynos4210_isol(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +	u32 tmp;
> +
> +	if (!drv->reg_isol)
> +		return;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		offset = EXYNOS_4210_USB_ISOL_DEVICE_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_DEVICE;
> +		break;
> +	case EXYNOS4210_HOST:
> +		offset = EXYNOS_4210_USB_ISOL_HOST_OFFSET;
> +		mask = EXYNOS_4210_USB_ISOL_HOST;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	tmp = readl(drv->reg_isol + offset);
> +	if (on)
> +		tmp &= ~mask;
> +	else
> +		tmp |= mask;
> +	writel(tmp, drv->reg_isol + offset);
> +}
> +
> +static void exynos4210_phy_pwr(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4210_DEVICE:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY0;
> +		break;
> +	case EXYNOS4210_HOST:
> +		phypwr =	EXYNOS_4210_UPHYPWR_PHY1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_ALL |
> +				EXYNOS_4210_URSTCON_PHY1_P0 |
> +				EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_ALL |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P0;
> +		writel(on, drv->reg_phy + EXYNOS_4210_UPHY1CON);
> +		break;
> +	case EXYNOS4210_HSIC0:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P1;
> +		break;
> +	case EXYNOS4210_HSIC1:
> +		phypwr =	EXYNOS_4210_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4210_URSTCON_PHY1_P1P2 |
> +				EXYNOS_4210_URSTCON_HOST_LINK_P2;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk, drv->reg_phy + EXYNOS_4210_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +		udelay(10);

usleep_range?
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST);
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4210_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4210_power_on(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_ON) {
> +		dev_err(drv->dev, "usb phy \"%s\" already on",
> +							inst->cfg->label);
> +		return -ENODEV;
> +	}
> +	inst->state = STATE_ON;
> +	inst->ref_cnt++;
> +	if (inst->ref_cnt > 1)
> +		return 0;

reference counting shouldn't be needed. It's been handled by the PHY framework.
> +
> +	/* Order of initialisation is important - first power then isolation */
> +	exynos4210_phy_pwr(inst, 1);
> +	exynos4210_isol(inst, 0);
> +
> +	return 0;
> +}
> +
> +static int exynos4210_power_off(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_OFF) {
> +		dev_err(drv->dev, "usb phy \"%s\" already off",
> +							inst->cfg->label);
> +		return -EINVAL;
> +	}
> +
> +	inst->state = STATE_OFF;
> +	inst->ref_cnt--;
> +	if (inst->ref_cnt > 0)
> +		return 0;

same here.
> +
> +	exynos4210_isol(inst, 1);
> +	exynos4210_phy_pwr(inst, 0);
> +
> +	return 0;
> +}
> +
> +
> +static const struct common_phy exynos4210_phys[] = {
> +	{
> +		.label		= "device",
> +		.type		= PHY_DEVICE,
> +		.id		= EXYNOS4210_DEVICE,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4210_HOST,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4210_HSIC0,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4210_HSIC1,
> +		.rate_to_clk	= exynos4210_rate_to_clk,
> +		.power_on	= exynos4210_power_on,
> +		.power_off	= exynos4210_power_off,
> +	},
> +	{},
> +};
> +
> +const struct uphy_config exynos4210_uphy_config = {
> +	.cpu		= TYPE_EXYNOS4210,
> +	.num_phys	= EXYNOS4210_NUM_PHYS,
> +	.phys		= exynos4210_phys,
> +};
> +
> diff --git a/drivers/phy/phy-exynos4212-usb.c b/drivers/phy/phy-exynos4212-usb.c
> new file mode 100644
> index 0000000..c07ae8e
> --- /dev/null
> +++ b/drivers/phy/phy-exynos4212-usb.c
> @@ -0,0 +1,343 @@
> +/*
> + * Samsung S5P/EXYNOS SoC series USB PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Kamil Debski <k.debski@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include "phy-exynos-usb.h"
> +
> +/* Exynos USB PHY registers */
> +
> +/* PHY power control */
> +#define EXYNOS_4212_UPHYPWR			0x0
> +
> +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND		(1 << 0)
> +#define EXYNOS_4212_UPHYPWR_DEV_PWR		(1 << 3)
> +#define EXYNOS_4212_UPHYPWR_DEV_OTG_PWR		(1 << 4)
> +#define EXYNOS_4212_UPHYPWR_DEV_SLEEP		(1 << 5)
> +#define EXYNOS_4212_UPHYPWR_DEV	( \
> +	EXYNOS_4212_UPHYPWR_DEV_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_DEV_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_OTG_PWR | \
> +	EXYNOS_4212_UPHYPWR_DEV_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HOST_SUSPEND	(1 << 6)
> +#define EXYNOS_4212_UPHYPWR_HOST_PWR		(1 << 7)
> +#define EXYNOS_4212_UPHYPWR_HOST_SLEEP		(1 << 8)
> +#define EXYNOS_4212_UPHYPWR_HOST ( \
> +	EXYNOS_4212_UPHYPWR_HOST_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HOST_PWR | \
> +	EXYNOS_4212_UPHYPWR_HOST_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND	(1 << 9)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_PWR		(1 << 10)
> +#define EXYNOS_4212_UPHYPWR_HSCI0_SLEEP		(1 << 11)
> +#define EXYNOS_4212_UPHYPWR_HSCI0 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI0_SLEEP)
> +
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND	(1 << 12)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_PWR		(1 << 13)
> +#define EXYNOS_4212_UPHYPWR_HSCI1_SLEEP		(1 << 14)
> +#define EXYNOS_4212_UPHYPWR_HSCI1 ( \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SUSPEND | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_PWR | \
> +	EXYNOS_4212_UPHYPWR_HSCI1_SLEEP)
> +
> +/* PHY clock control */
> +#define EXYNOS_4212_UPHYCLK			0x4
> +
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_MASK	(0x7 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6	(0x0 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ	(0x1 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ	(0x2 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2	(0x3 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ	(0x4 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ	(0x5 << 0)
> +#define EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ	(0x7 << 0)
> +
> +#define EXYNOS_4212_UPHYCLK_PHY0_ID_PULLUP	(0x1 << 3)
> +#define EXYNOS_4212_UPHYCLK_PHY0_COMMON_ON	(0x1 << 4)
> +#define EXYNOS_4212_UPHYCLK_PHY1_COMMON_ON	(0x1 << 7)
> +
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_MASK	(0x7f << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_12MHZ	(0x24 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_15MHZ	(0x1c << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_16MHZ	(0x1a << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_19MHZ2	(0x15 << 10)
> +#define EXYNOS_4212_UPHYCLK_HSIC_REFCLK_20MHZ	(0x14 << 10)
> +
> +/* PHY reset control */
> +#define EXYNOS_4212_UPHYRST			0x8
> +
> +#define EXYNOS_4212_URSTCON_DEVICE		(1 << 0)
> +#define EXYNOS_4212_URSTCON_OTG_HLINK		(1 << 1)
> +#define EXYNOS_4212_URSTCON_OTG_PHYLINK		(1 << 2)
> +#define EXYNOS_4212_URSTCON_HOST_PHY		(1 << 3)
> +#define EXYNOS_4212_URSTCON_PHY1		(1 << 4)
> +#define EXYNOS_4212_URSTCON_HSIC0		(1 << 5)
> +#define EXYNOS_4212_URSTCON_HSIC1		(1 << 6)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_ALL	(1 << 7)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P0	(1 << 8)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P1	(1 << 9)
> +#define EXYNOS_4212_URSTCON_HOST_LINK_P2	(1 << 10)
> +
> +/* Isolation, configured in the power management unit */
> +#define EXYNOS_4212_USB_ISOL_OFFSET		0x0
> +#define EXYNOS_4212_USB_ISOL_OTG		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET	0x4
> +#define EXYNOS_4212_USB_ISOL_HSIC0		(1 << 0)
> +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET	0x8
> +#define EXYNOS_4212_USB_ISOL_HSIC1		(1 << 0)
> +
> +enum exynos4x12_phy_id {
> +	EXYNOS4212_DEVICE,
> +	EXYNOS4212_HOST,
> +	EXYNOS4212_HSIC0,
> +	EXYNOS4212_HSIC1,
> +	EXYNOS4212_NUM_PHYS,
> +};
> +
> +/* exynos4212_rate_to_clk() converts the supplied clock rate to the value that
> + * can be written to the phy register. */
> +static u32 exynos4212_rate_to_clk(unsigned long rate)
> +{
> +	unsigned int clksel;
> +
> +	/* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */
> +
> +	switch (rate) {
> +	case 9600 * KHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6;
> +		break;
> +	case 10 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ;
> +		break;
> +	case 12 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ;
> +		break;
> +	case 19200 * KHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2;
> +		break;
> +	case 20 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ;
> +		break;
> +	case 24 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ;
> +		break;
> +	case 50 * MHZ:
> +		clksel = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ;
> +		break;
> +	default:
> +		clksel = CLKSEL_ERROR;
> +	}
> +
> +	return clksel;
> +}
> +
> +static void exynos4212_isol(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 offset;
> +	u32 mask;
> +	u32 tmp;
> +
> +	if (!drv->reg_isol)
> +		return;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_OTG;
> +		break;
> +	case EXYNOS4212_HOST:
> +		offset = EXYNOS_4212_USB_ISOL_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_OTG;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC0_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC0;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		offset = EXYNOS_4212_USB_ISOL_HSIC1_OFFSET;
> +		mask = EXYNOS_4212_USB_ISOL_HSIC1;
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	tmp = readl(drv->reg_isol + offset);
> +	if (on)
> +		tmp &= ~mask;
> +	else
> +		tmp |= mask;
> +	writel(tmp, drv->reg_isol + offset);
> +}
> +
> +static void exynos4212_phy_pwr(struct uphy_instance *inst, bool on)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +	u32 rstbits = 0;
> +	u32 phypwr = 0;
> +	u32 rst;
> +	u32 pwr;
> +
> +	switch (inst->cfg->id) {
> +	case EXYNOS4212_DEVICE:
> +		phypwr =	EXYNOS_4212_UPHYPWR_DEV;
> +		rstbits =	EXYNOS_4212_URSTCON_DEVICE;
> +		break;
> +	case EXYNOS4212_HOST:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HOST;
> +		rstbits =	EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC0:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI0;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P0 |
> +				EXYNOS_4212_URSTCON_HOST_PHY;
> +		break;
> +	case EXYNOS4212_HSIC1:
> +		phypwr =	EXYNOS_4212_UPHYPWR_HSCI1;
> +		rstbits =	EXYNOS_4212_URSTCON_HSIC1 |
> +				EXYNOS_4212_URSTCON_HOST_LINK_P1;
> +		break;
> +	};
> +
> +	if (on) {
> +		writel(inst->clk, drv->reg_phy + EXYNOS_4212_UPHYCLK);
> +
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr &= ~phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +
> +		rst = readl(drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		rst |= rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +		udelay(10);
> +		rst &= ~rstbits;
> +		writel(rst, drv->reg_phy + EXYNOS_4212_UPHYRST);
> +	} else {
> +		pwr = readl(drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +		pwr |= phypwr;
> +		writel(pwr, drv->reg_phy + EXYNOS_4212_UPHYPWR);
> +	}
> +}
> +
> +static int exynos4212_power_on(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_ON) {
> +		dev_err(drv->dev, "usb phy \"%s\" already on",
> +							inst->cfg->label);
> +		return -ENODEV;
> +	}
> +
> +	inst->state = STATE_ON;
> +	inst->ref_cnt++;
> +	if (inst->ref_cnt > 1)
> +		return 0;
> +
> +	exynos4212_phy_pwr(inst, 1);
> +	exynos4212_isol(inst, 0);
> +
> +	/* Power on the device, as it is necessary for HSIC to work */
> +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> +		struct uphy_instance *device =
> +					&drv->uphy_instances[EXYNOS4212_DEVICE];
> +		exynos4212_phy_pwr(device, 1);
> +		exynos4212_isol(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int exynos4212_power_off(struct uphy_instance *inst)
> +{
> +	struct uphy_driver *drv = inst->drv;
> +
> +	if (inst->state == STATE_OFF) {
> +		dev_err(drv->dev, "usb phy \"%s\" already off",
> +							inst->cfg->label);
> +		return -EINVAL;
> +	}
> +
> +	inst->state = STATE_OFF;
> +	inst->ref_cnt--;
> +
> +	if (inst->ref_cnt > 0)
> +		return 0;
> +
> +	exynos4212_isol(inst, 1);
> +	exynos4212_phy_pwr(inst, 0);
> +
> +	if (inst->cfg->id == EXYNOS4212_HSIC0) {
> +		struct uphy_instance *device =
> +					&drv->uphy_instances[EXYNOS4212_DEVICE];
> +		exynos4212_isol(device, 1);
> +		exynos4212_phy_pwr(device, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static const struct common_phy exynos4212_phys[] = {
> +	{
> +		.label		= "device",
> +		.type		= PHY_DEVICE,
> +		.id		= EXYNOS4212_DEVICE,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "host",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4212_HOST,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic0",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4212_HSIC0,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{
> +		.label		= "hsic1",
> +		.type		= PHY_HOST,
> +		.id		= EXYNOS4212_HSIC1,
> +		.rate_to_clk	= exynos4212_rate_to_clk,
> +		.power_on	= exynos4212_power_on,
> +		.power_off	= exynos4212_power_off,
> +	},
> +	{},
> +};
> +
> +const struct uphy_config exynos4212_uphy_config = {
> +	.cpu		= TYPE_EXYNOS4212,
> +	.num_phys	= EXYNOS4212_NUM_PHYS,
> +	.phys		= exynos4212_phys,
> +};

I see quite an amount of similarities between the two PHY drivers. It would be
better if we can have a single driver to handle it.

Thanks
Kishon

  parent reply	other threads:[~2013-10-25 15:39 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-25 14:15 [PATCH 0/5] phy: Add new Exynos USB PHY driver Kamil Debski
2013-10-25 14:15 ` Kamil Debski
     [not found] ` <1382710529-12082-1-git-send-email-k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-25 14:15   ` [PATCH v2 1/5] " Kamil Debski
2013-10-25 14:15     ` Kamil Debski
     [not found]     ` <1382710529-12082-2-git-send-email-k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-25 15:39       ` Kishon Vijay Abraham I [this message]
2013-10-25 15:39         ` Kishon Vijay Abraham I
     [not found]         ` <526A90B9.3040501-l0cyMroinI0@public.gmane.org>
2013-10-28 13:52           ` Kamil Debski
2013-10-28 13:52             ` Kamil Debski
2013-10-28 20:00             ` Tomasz Figa
2013-10-29 10:16               ` Kamil Debski
     [not found]             ` <025b01ced3e4$ec685db0$c5391910$%debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-29  9:52               ` Kishon Vijay Abraham I
2013-10-29  9:52                 ` Kishon Vijay Abraham I
2013-10-25 21:36       ` Kumar Gala
2013-10-25 21:36         ` Kumar Gala
2013-10-28 13:52         ` Kamil Debski
2013-10-25 14:15 ` [RFC PATCH 2/5] phy: Add WIP Exynos 5250 support to the " Kamil Debski
     [not found]   ` <1382710529-12082-3-git-send-email-k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-25 15:43     ` Kishon Vijay Abraham I
2013-10-25 15:43       ` Kishon Vijay Abraham I
     [not found]       ` <526A91B9.1060901-l0cyMroinI0@public.gmane.org>
2013-10-28 13:52         ` Kamil Debski
2013-10-28 13:52           ` Kamil Debski
2013-10-28 14:41       ` Vivek Gautam
2013-10-29  9:55         ` Kishon Vijay Abraham I
2013-10-29  9:55           ` Kishon Vijay Abraham I
2013-10-29 10:14           ` Kamil Debski
2013-10-29 10:51             ` Kishon Vijay Abraham I
2013-10-29 10:51               ` Kishon Vijay Abraham I
2013-10-25 14:15 ` [PATCH 3/5] phy: Add support for S5PV210 " Kamil Debski
     [not found]   ` <1382710529-12082-4-git-send-email-k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-25 15:50     ` Kishon Vijay Abraham I
2013-10-25 15:50       ` Kishon Vijay Abraham I
     [not found]       ` <526A933C.4020904-l0cyMroinI0@public.gmane.org>
2013-10-26  1:40         ` Jingoo Han
2013-10-26  1:40           ` Jingoo Han
2013-10-25 14:15 ` [PATCH 4/5] usb: ehci-s5p: Change to use phy provided by the generic phy framework Kamil Debski
2013-10-25 15:52   ` Kishon Vijay Abraham I
2013-10-25 15:52     ` Kishon Vijay Abraham I
2013-10-26  1:27   ` Jingoo Han
     [not found]     ` <003101ced1ea$85f7e010$91e7a030$%han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-28 13:52       ` Kamil Debski
2013-10-28 13:52         ` Kamil Debski
     [not found]   ` <1382710529-12082-5-git-send-email-k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-26  9:41     ` Vivek Gautam
2013-10-26  9:41       ` Vivek Gautam
2013-10-28 13:53       ` Kamil Debski
     [not found]         ` <025f01ced3e5$0660ecf0$1322c6d0$%debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2013-10-28 14:36           ` Vivek Gautam
2013-10-28 14:36             ` Vivek Gautam
2013-10-25 14:15 ` [PATCH 5/5] usb: s3c-hsotg: Use the new Exynos USB phy driver with " Kamil Debski
2013-10-25 15:53   ` Kishon Vijay Abraham I
2013-10-25 15:53     ` Kishon Vijay Abraham I

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=526A90B9.3040501@ti.com \
    --to=kishon-l0cymroini0@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=linux-arm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=mat.krawczuk-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.