Hi, > From: Kamil Debski [mailto:k.debski@xxxxxxxxxxx] > Sent: Friday, December 06, 2013 5:28 PM > > Hi Kishon, > > Thank you for the review. > > > From: Kishon Vijay Abraham I [mailto:kishon@xxxxxx] > > Sent: Friday, December 06, 2013 11:59 AM > > > > Hi, > > > > On Thursday 05 December 2013 05:59 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@xxxxxxxxxxx> > > > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > > > --- > > > .../devicetree/bindings/phy/samsung-usbphy.txt | 54 ++++ > > > drivers/phy/Kconfig | 20 ++ > > > drivers/phy/Makefile | 3 + > > > drivers/phy/phy-exynos4210-usb2.c | 264 > > +++++++++++++++++ > > > drivers/phy/phy-exynos4212-usb2.c | 312 > > ++++++++++++++++++++ > > > drivers/phy/phy-samsung-usb2.c | 228 > > ++++++++++++++ > > > drivers/phy/phy-samsung-usb2.h | 72 +++++ > > > 7 files changed, 953 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/phy/samsung-usbphy.txt > > > create mode 100644 drivers/phy/phy-exynos4210-usb2.c create mode > > > 100644 drivers/phy/phy-exynos4212-usb2.c create mode 100644 > > > drivers/phy/phy-samsung-usb2.c create mode 100644 > > > drivers/phy/phy-samsung-usb2.h > > > > > > diff --git > > > a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt > > > b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt > > > new file mode 100644 > > > index 0000000..cadbf70 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt > > > > use the existing samsung-phy.txt. > > Ok. > > > > @@ -0,0 +1,54 @@ > > > +Samsung S5P/EXYNOS SoC series USB PHY > > > +------------------------------------------------- > > > + > > > +Required properties: > > > +- compatible : should be one of the listed compatibles: > > > + - "samsung,exynos4210-usb2-phy" > > > + - "samsung,exynos4212-usb2-phy" > > > +- reg : a list of registers used by phy driver > > > + - first and obligatory is the location of phy modules registers > > > +- samsung,sysreg-phandle - handle to syscon used to control the > > > +system registers > > > +- samsung,pmureg-phandle - handle to syscon used to control PMU > > > +registers > > > +- #phy-cells : from the generic phy bindings, must be 1; > > > +- clocks and clock-names: > > > + - the "phy" clocks is required by the phy module > > > + - next for each of the phys a clock has to be assidned, this > > clock > > > > %s/assidned/assigned/ > > Thank you for spotting this. > > > > + will be used to determine clocking frequency for the phys > > > + (the labels are specified in the paragraph below) > > > + > > > +The first phandle argument in the PHY specifier identifies the PHY, > > > +its meaning is compatible dependent. For the currently supported > > SoCs > > > +(Exynos 4210 and Exynos 4212) it is as follows: > > > + 0 - USB device ("device"), > > > + 1 - USB host ("host"), > > > + 2 - HSIC0 ("hsic0"), > > > + 3 - HSIC1 ("hsic1"), > > > + > > > +Exynos 4210 and Exynos 4212 use mode switching and require that > > > +mode switch register is supplied. > > > + > > > +Example: > > > + > > > +For Exynos 4412 (compatible with Exynos 4212): > > > + > > > +usbphy: phy@125B0000 { > > > > use lower case for address here... > > Ok. > > > > + compatible = "samsung,exynos4212-usb2-phy"; > > > + reg = <0x125B0000 0x100 0x10020704 0x0c 0x1001021c 0x4>; > > and here.. > > > + clocks = <&clock 305>, <&clock 2>, <&clock 2>, <&clock 2>, > > > + <&clock 2>; > > > + clock-names = "phy", "device", "host", "hsic0", "hsic1"; > > > + status = "okay"; > > > + #phy-cells = <1>; > > > + samsung,sysreg-phandle = <&sys_reg>; > > > + samsung,pmureg-phandle = <&pmu_reg>; }; > > > + > > > +Then the PHY can be used in other nodes such as: > > > + > > > +phy-consumer@12340000 { > > > + phys = <&usbphy 2>; > > > + phy-names = "phy"; > > > +}; > > > + > > > +Refer to DT bindings documentation of particular PHY consumer > > devices > > > +for more information about required PHYs and the way of > > specification. > > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index > > > a344f3d..b29018f 100644 > > > --- a/drivers/phy/Kconfig > > > +++ b/drivers/phy/Kconfig > > > @@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO > > > help > > > Support for Display Port PHY found on Samsung EXYNOS SoCs. > > > > > > +config PHY_SAMSUNG_USB2 > > > + tristate "Samsung USB 2.0 PHY driver" > > > + 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. > > > + > > > +config PHY_EXYNOS4210_USB2 > > > + bool "Support for Exynos 4210" > > > + depends on PHY_SAMSUNG_USB2 > > > + depends on CPU_EXYNOS4210 > > > > select GENERIC_PHY here? > > I think that depends on PHY_SAMSUNG_USB2 is better in this place. > However, I agree that I should add select GENERIC_PHY to > PHY_SAMSUNG_USB2. > > The reason why I am saying this is that I like how it looks in > menuconfig. Selecting PHY_SAMSUNG_USB2 expands more options and if > unselected the menu looks tidier. > > > > + help > > > + Enable USB PHY support for Exynos 4210 > > > > Add more explanation here and make checkpatch happy. > > Here I think we should not treat checkpatch as an oracle. I also > noticed these warnings, but I think that writing a four line > description for SoC specific options in this menu is an overkill. In > addition, checkpatch also complains for the following entries in > Kconfig file: > - PHY_EXYNOS_MIPI_VIDEO > - PHY_EXYNOS_DP_VIDEO > - PHY_SAMSUNG_USB2 (here expanding the description may be justified, > but > I think the current description is enough) > > But thank you for directing my attention to the Kconfig file. I noticed > That SoC specific Kconfig entries should have "USB 2.0" instead of just > "USB". > > > > + > > > +config PHY_EXYNOS4212_USB2 > > > + bool "Support for Exynos 4212" > > > + depends on PHY_SAMSUNG_USB2 > > > + depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412) > > > > select GENERIC_PHY. > > Please check my reply above. > > > > + help > > > + Enable USB PHY support for Exynos 4212 > > > > more explanation here too.. > > Please check my reply above. > > > > endmenu > > > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index > > > d0caae9..9f4befd 100644 > > > --- a/drivers/phy/Makefile > > > +++ b/drivers/phy/Makefile > > > @@ -7,3 +7,6 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy- > exynos-dp- > > video.o > > > obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o > > > obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o > > > obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o > > > +obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-samsung-usb2.o > > > +obj-$(CONFIG_PHY_EXYNOS4210_USB2) += phy-exynos4210-usb2.o > > > +obj-$(CONFIG_PHY_EXYNOS4212_USB2) += phy-exynos4212-usb2.o > > > diff --git a/drivers/phy/phy-exynos4210-usb2.c > > > b/drivers/phy/phy-exynos4210-usb2.c > > > new file mode 100644 > > > index 0000000..a02e5c2 > > > --- /dev/null > > > +++ b/drivers/phy/phy-exynos4210-usb2.c > > > @@ -0,0 +1,264 @@ > > > +/* > > > + * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support > > > + * > > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd. > > > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx> > > > + * > > > + * 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/regmap.h> > > > +#include <linux/spinlock.h> > > > > You've included most of the above header files in phy-samsung-usb2.h > > which you are including below. > > I agree that includes in phy-samsung-usb2.h could use a cleanup. On the > other hand my opinion is that a .c file should include all .h files > that are used in this .c file. Relaying on .h file to include > another .h doesn't seem good to me. > > > > +#include "phy-samsung-usb2.h" > > > + > > > +/* Exynos USB PHY registers */ > > > + > > > +/* PHY power control */ > > > +#define EXYNOS_4210_UPHYPWR 0x0 > > > + > > > +#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND (1 << 0) > > > > use BIT() here and everywhere below. > > All right. > > > > +#define EXYNOS_4210_UPHYPWR_PHY0_PWR (1 << 3) > > > > replace PHY0 here with DEV so it looks similar to EXYNOS_4212. > > > +#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) > > > > replace PHY0 here with HOST so it looks similar to EXYNOS_4212. > > Thank you for spotting this inconsistence. I will fix that. But I > rather incline to use PHY0/1 instead of DEVICE/HOST as it will be then > consistent with the date sheet. > > > > +#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) > > > + > > > +/* Isolation, configured in the power management unit */ > > > +#define EXYNOS_4210_USB_ISOL_DEVICE_OFFSET 0x704 > > > +#define EXYNOS_4210_USB_ISOL_DEVICE (1 << 0) > > > +#define EXYNOS_4210_USB_ISOL_HOST_OFFSET 0x708 > > > +#define EXYNOS_4210_USB_ISOL_HOST (1 << 0) > > > + > > > +/* USBYPHY1 Floating prevention */ > > > +#define EXYNOS_4210_UPHY1CON 0x34 > > > +#define EXYNOS_4210_UPHY1CON_FLOAT_PREVENTION 0x1 > > > + > > > +/* Mode switching SUB Device <-> Host */ > > > +#define EXYNOS_4210_MODE_SWITCH_OFFSET 0x21c > > > +#define EXYNOS_4210_MODE_SWITCH_MASK 1 > > > +#define EXYNOS_4210_MODE_SWITCH_DEVICE 0 > > > +#define EXYNOS_4210_MODE_SWITCH_HOST 1 > > > + > > > +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. > > > + */ > > > +static int exynos4210_rate_to_clk(unsigned long rate, u32 *reg) { > > > + switch (rate) { > > > + case 12 * MHZ: > > > + *reg = EXYNOS_4210_UPHYCLK_PHYFSEL_12MHZ; > > > + break; > > > + case 24 * MHZ: > > > + *reg = EXYNOS_4210_UPHYCLK_PHYFSEL_24MHZ; > > > + break; > > > + case 48 * MHZ: > > > + reg = EXYNOS_4210_UPHYCLK_PHYFSEL_48MHZ; > > > > %s/reg/*Reg > > Thank you. > > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void exynos4210_isol(struct samsung_usb2_phy_instance *inst, > > > +bool on) { > > > + struct samsung_usb2_phy_driver *drv = inst->drv; > > > + u32 offset; > > > + u32 mask; > > > + > > > + 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; > > > + }; > > > + > > > + regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask); } > > > + > > > +static void exynos4210_phy_pwr(struct samsung_usb2_phy_instance > > > +*inst, bool on) { > > > + struct samsung_usb2_phy_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_reg_val, 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); > > > + rst &= ~rstbits; > > > + writel(rst, drv->reg_phy + EXYNOS_4210_UPHYRST); > > > > Do you have to reset during every power on? IMO this reset should be > > done once in phy_init. > > It seems that this reset is neded by the hardware. > > > > + } 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 samsung_usb2_phy_instance > > > +*inst) { > > > + /* 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 samsung_usb2_phy_instance > > > +*inst) { > > > + exynos4210_isol(inst, 1); > > > + exynos4210_phy_pwr(inst, 0); > > > + > > > + return 0; > > > +} > > > + > > > + > > > +static const struct samsung_usb2_common_phy exynos4210_phys[] = { > > > + { > > > + .label = "device", > > > + .id = EXYNOS4210_DEVICE, > > > + .rate_to_clk = exynos4210_rate_to_clk, > > > + .power_on = exynos4210_power_on, > > > + .power_off = exynos4210_power_off, > > > + }, > > > + { > > > + .label = "host", > > > + .id = EXYNOS4210_HOST, > > > + .rate_to_clk = exynos4210_rate_to_clk, > > > + .power_on = exynos4210_power_on, > > > + .power_off = exynos4210_power_off, > > > + }, > > > + { > > > + .label = "hsic0", > > > + .id = EXYNOS4210_HSIC0, > > > + .rate_to_clk = exynos4210_rate_to_clk, > > > + .power_on = exynos4210_power_on, > > > + .power_off = exynos4210_power_off, > > > + }, > > > + { > > > + .label = "hsic1", > > > + .id = EXYNOS4210_HSIC1, > > > + .rate_to_clk = exynos4210_rate_to_clk, > > > + .power_on = exynos4210_power_on, > > > + .power_off = exynos4210_power_off, > > > + }, > > > + {}, > > > +}; > > > + > > > +const struct samsung_usb2_phy_config exynos4210_usb2_phy_config = > { > > > + .num_phys = EXYNOS4210_NUM_PHYS, > > > + .phys = exynos4210_phys, > > > + .has_mode_switch = 1, > > > +}; > > > + > > > diff --git a/drivers/phy/phy-exynos4212-usb2.c > > > b/drivers/phy/phy-exynos4212-usb2.c > > > new file mode 100644 > > > index 0000000..375ece0 > > > --- /dev/null > > > +++ b/drivers/phy/phy-exynos4212-usb2.c > > > @@ -0,0 +1,312 @@ > > > +/* > > > + * Samsung S5P/EXYNOS SoC series USB PHY driver - Exynos 4212 > > support > > > + * > > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd. > > > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx> > > > + * > > > + * 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/regmap.h> > > > +#include <linux/spinlock.h> > > > > You've included most of the above header files in phy-samsung-usb2.h > > which you are including below. > > Please see my reply above. > > > > +#include "phy-samsung-usb2.h" > > > + > > > +/* Exynos USB PHY registers */ > > > + > > > +/* PHY power control */ > > > +#define EXYNOS_4212_UPHYPWR 0x0 > > > + > > > +#define EXYNOS_4212_UPHYPWR_DEV_SUSPEND (1 << 0) > > > > use BIT() here and below.. > > All right. > > > > +#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 0x704 > > > +#define EXYNOS_4212_USB_ISOL_OTG (1 << 0) > > > +#define EXYNOS_4212_USB_ISOL_HSIC0_OFFSET 0x708 > > > +#define EXYNOS_4212_USB_ISOL_HSIC0 (1 << 0) > > > +#define EXYNOS_4212_USB_ISOL_HSIC1_OFFSET 0x70c > > > +#define EXYNOS_4212_USB_ISOL_HSIC1 (1 << 0) > > > + > > > +/* Mode switching SUB Device <-> Host */ > > > +#define EXYNOS_4212_MODE_SWITCH_OFFSET 0x21c > > > +#define EXYNOS_4212_MODE_SWITCH_MASK 1 > > > +#define EXYNOS_4212_MODE_SWITCH_DEVICE 0 > > > +#define EXYNOS_4212_MODE_SWITCH_HOST 1 > > > + > > > +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 int exynos4212_rate_to_clk(unsigned long rate, u32 *reg) { > > > + /* EXYNOS_4212_UPHYCLK_PHYFSEL_MASK */ > > > + > > > + switch (rate) { > > > + case 9600 * KHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_9MHZ6; > > > + break; > > > + case 10 * MHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_10MHZ; > > > + break; > > > + case 12 * MHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_12MHZ; > > > + break; > > > + case 19200 * KHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_19MHZ2; > > > + break; > > > + case 20 * MHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_20MHZ; > > > + break; > > > + case 24 * MHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_24MHZ; > > > + break; > > > + case 50 * MHZ: > > > + *reg = EXYNOS_4212_UPHYCLK_PHYFSEL_50MHZ; > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void exynos4212_isol(struct samsung_usb2_phy_instance *inst, > > > +bool on) { > > > + struct samsung_usb2_phy_driver *drv = inst->drv; > > > + u32 offset; > > > + u32 mask; > > > + > > > + switch (inst->cfg->id) { > > > + case EXYNOS4212_DEVICE: > > > + 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; > > > + }; > > > + > > > + regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask); } > > > + > > > +static void exynos4212_phy_pwr(struct samsung_usb2_phy_instance > > > +*inst, bool on) { > > > + struct samsung_usb2_phy_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_reg_val, 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); > > > > reset should be done once in init? > > The hardware seems to need this reset. > > > > + } 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 samsung_usb2_phy_instance > > > +*inst) { > > > + struct samsung_usb2_phy_driver *drv = inst->drv; > > > + > > > + inst->enabled = 1; > > > + exynos4212_phy_pwr(inst, 1); > > > + exynos4212_isol(inst, 0); > > > + > > > + /* Power on the device, as it is necessary for HSIC to work */ > > > > This looks weird. How powering on the 'device PHY' makes 'HSIC PHY' > to > > work? > > I agree with you that it looks weird, but I found this necessary to be > done. > > > > + if (inst->cfg->id == EXYNOS4212_HSIC0) { > > > + struct samsung_usb2_phy_instance *device = > > > + &drv->instances[EXYNOS4212_DEVICE]; > > > + exynos4212_phy_pwr(device, 1); > > > + exynos4212_isol(device, 0); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int exynos4212_power_off(struct samsung_usb2_phy_instance > > > +*inst) { > > > + struct samsung_usb2_phy_driver *drv = inst->drv; > > > + struct samsung_usb2_phy_instance *device = > > > +&drv->instances[EXYNOS4212_DEVICE]; > > > + > > > + inst->enabled = 0; > > > + exynos4212_isol(inst, 1); > > > + exynos4212_phy_pwr(inst, 0); > > > + > > > + if (inst->cfg->id == EXYNOS4212_HSIC0 && !device->enabled) { > > > + exynos4212_isol(device, 1); > > > + exynos4212_phy_pwr(device, 0); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > + > > > +static const struct samsung_usb2_common_phy exynos4212_phys[] = { > > > + { > > > + .label = "device", > > > + .id = EXYNOS4212_DEVICE, > > > + .rate_to_clk = exynos4212_rate_to_clk, > > > + .power_on = exynos4212_power_on, > > > + .power_off = exynos4212_power_off, > > > + }, > > > + { > > > + .label = "host", > > > + .id = EXYNOS4212_HOST, > > > + .rate_to_clk = exynos4212_rate_to_clk, > > > + .power_on = exynos4212_power_on, > > > + .power_off = exynos4212_power_off, > > > + }, > > > + { > > > + .label = "hsic0", > > > + .id = EXYNOS4212_HSIC0, > > > + .rate_to_clk = exynos4212_rate_to_clk, > > > + .power_on = exynos4212_power_on, > > > + .power_off = exynos4212_power_off, > > > + }, > > > + { > > > + .label = "hsic1", > > > + .id = EXYNOS4212_HSIC1, > > > + .rate_to_clk = exynos4212_rate_to_clk, > > > + .power_on = exynos4212_power_on, > > > + .power_off = exynos4212_power_off, > > > + }, > > > + {}, > > > +}; > > > + > > > +const struct samsung_usb2_phy_config exynos4212_usb2_phy_config = > { > > > + .num_phys = EXYNOS4212_NUM_PHYS, > > > + .phys = exynos4212_phys, > > > + .has_mode_switch = 1, > > > +}; > > > + > > > diff --git a/drivers/phy/phy-samsung-usb2.c > > > b/drivers/phy/phy-samsung-usb2.c new file mode 100644 index > > > 0000000..804ec77 > > > --- /dev/null > > > +++ b/drivers/phy/phy-samsung-usb2.c > > > @@ -0,0 +1,228 @@ > > > +/* > > > + * Samsung SoC USB 1.1/2.0 PHY driver > > > + * > > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd. > > > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx> > > > + * > > > + * 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/mfd/syscon.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> > > > > You've included most of the above header files in phy-samsung-usb2.h > > which you are including below. > > Please see my answer above. > > > > +#include "phy-samsung-usb2.h" > > > + > > > +static int samsung_usb2_phy_power_on(struct phy *phy) { > > > + struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy); > > > + struct samsung_usb2_phy_driver *drv = inst->drv; > > > + int ret; > > > + > > > + dev_dbg(drv->dev, "Request to power_on \"%s\" usb phy\n", > > > + inst->cfg->label); > > > + ret = clk_prepare_enable(drv->clk); > > > + if (ret) > > > + goto err_main_clk; > > > + ret = clk_prepare_enable(inst->clk); > > > + if (ret) > > > + goto err_instance_clk; > > > + inst->rate = clk_get_rate(inst->clk); > > > + if (inst->cfg->rate_to_clk) { > > > + ret = inst->cfg->rate_to_clk(inst->rate, &inst- > > >clk_reg_val); > > > + if (ret) > > > + goto err_get_rate; > > > + } > > > + if (inst->cfg->power_on) { > > > + spin_lock(&drv->lock); > > > + ret = inst->cfg->power_on(inst); > > > + spin_unlock(&drv->lock); > > > + } > > > + > > > + return 0; > > > + > > > +err_get_rate: > > > + clk_disable_unprepare(inst->clk); > > > +err_instance_clk: > > > + clk_disable_unprepare(drv->clk); > > > +err_main_clk: > > > + return ret; > > > +} > > > + > > > +static int samsung_usb2_phy_power_off(struct phy *phy) { > > > + struct samsung_usb2_phy_instance *inst = phy_get_drvdata(phy); > > > + struct samsung_usb2_phy_driver *drv = inst->drv; > > > + int ret = 0; > > > + > > > + dev_dbg(drv->dev, "Request to power_off \"%s\" usb phy\n", > > > + inst->cfg->label); > > > + if (inst->cfg->power_off) { > > > + spin_lock(&drv->lock); > > > + ret = inst->cfg->power_off(inst); > > > + spin_unlock(&drv->lock); > > > + } > > > + clk_disable_unprepare(inst->clk); > > > + clk_disable_unprepare(drv->clk); > > > + return ret; > > > +} > > > + > > > +static struct phy_ops samsung_usb2_phy_ops = { > > > + .power_on = samsung_usb2_phy_power_on, > > > + .power_off = samsung_usb2_phy_power_off, > > > + .owner = THIS_MODULE, > > > +}; > > > + > > > +static struct phy *samsung_usb2_phy_xlate(struct device *dev, > > > + struct of_phandle_args *args) > > > +{ > > > + struct samsung_usb2_phy_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->instances[args->args[0]].phy; > > > +} > > > + > > > +static const struct of_device_id samsung_usb2_phy_of_match[] = { > > > +#ifdef CONFIG_PHY_EXYNOS4210_USB2 > > > + { > > > + .compatible = "samsung,exynos4210-usb2-phy", > > > + .data = &exynos4210_usb2_phy_config, > > > + }, > > > +#endif > > > +#ifdef CONFIG_PHY_EXYNOS4212_USB2 > > > + { > > > + .compatible = "samsung,exynos4212-usb2-phy", > > > + .data = &exynos4212_usb2_phy_config, > > > + }, > > > +#endif > > > + { }, > > > +}; > > > > I think we've had enough discussion about this approach. Let's get > the > > opinion of others too. Felipe? Greg? > > Good idea. > > > Summary: > > We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with > > almost similar register map [1] and a samsung helper driver for these > > two drivers. > > I would not call them separate drivers. It's a single USB 2.0 driver > with the option to include support for various SoCs. This patchset adds: > Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that another > person is working on supporting S3C6410. > > > These two PHY drivers populate the function pointers in the helper > > driver. So any phy_ops will first invoke the helper driver which will > > then invoke the corresponding phy driver. > > > > [1] -> http://www.diffchecker.com/7yno1uvk > > Come on, this diff only includes the registers part of the file. > The following functions are also different: > - exynos421*_rate_to_clk > - exynos421*_isol > - exynos421*_phy_pwr > - exynos421*_power_on > - exynos421*_power_on > > It seems that the file is too large for the tool. But still this makes > a false impression that only registers are different. > > > Advantages: > > * (more) clean and readable > > * helper driver can be used with other PHY drivers which will be > added > > soon > > > > Disadvantages: > > * code duplication > > I would say that actually in this case less code is duplicated. Having > Separate drivers would mean that most of the phy-samsung-usb2.c file > has to be repeated. That is 240 times 4 (and surely more in the future, > as this patchset adds support for 4 SoCs). Which is almost 1000 lines > more. > > > > > Maybe having a helper driver makes sense when we have other samsung > > PHY drivers added but not sure if it's needed here for > EXYNOS4210_USB2 > > and > > EXYNOS4212_USB2 > > > > Need your inputs on what you think about this. > > Yes, I would also welcome other people's opinions. > > > > + > > > +static int samsung_usb2_phy_probe(struct platform_device *pdev) { > > > + const struct of_device_id *match; > > > + const struct samsung_usb2_phy_config *cfg; > > > + struct clk *clk; > > > + struct device *dev = &pdev->dev; > > > + struct phy_provider *phy_provider; > > > + struct resource *mem; > > > + struct samsung_usb2_phy_driver *drv; > > > + int i; > > > + > > > + if (!pdev->dev.of_node) { > > > + dev_err(dev, "This driver is required to be instantiated > > from device tree\n"); > > > + return -EINVAL; > > > + } > > > + > > > + match = of_match_node(samsung_usb2_phy_of_match, pdev- > > >dev.of_node); > > > + if (!match) { > > > + dev_err(dev, "of_match_node() failed\n"); > > > + return -EINVAL; > > > + } > > > + cfg = match->data; > > > + > > > + drv = devm_kzalloc(dev, sizeof(struct samsung_usb2_phy_driver) + > > > + cfg->num_phys * sizeof(struct samsung_usb2_phy_instance), > > GFP_KERNEL); > > > + if (!drv) > > > + 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); > > > + 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); > > > + } > > > + > > > + drv->reg_pmu = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > > > + "samsung,pmureg-phandle"); > > > + if (IS_ERR(drv->reg_pmu)) { > > > + dev_err(dev, "Failed to map PMU registers (via syscon)\n"); > > > + return PTR_ERR(drv->reg_pmu); > > > + } > > > + > > > + if (drv->cfg->has_mode_switch) { > > > + drv->reg_sys = syscon_regmap_lookup_by_phandle( > > > + pdev->dev.of_node, "samsung,sysreg-phandle"); > > > + if (IS_ERR(drv->reg_sys)) { > > > + dev_err(dev, "Failed to map system registers (via > > syscon)\n"); > > > + return PTR_ERR(drv->reg_sys); > > > + } > > > + } > > > + > > > + 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 samsung_usb2_phy_instance *p = &drv->instances[i]; > > > + > > > + dev_dbg(dev, "Creating phy \"%s\"\n", label); > > > + p->phy = devm_phy_create(dev, &samsung_usb2_phy_ops, NULL); > > > + if (IS_ERR(p->phy)) { > > > + dev_err(drv->dev, "Failed to create usb2_phy > > \"%s\"\n", > > > + label); > > > + return PTR_ERR(p->phy); > > > + } > > > + > > > + p->cfg = &drv->cfg->phys[i]; > > > + p->drv = drv; > > > + phy_set_drvdata(p->phy, p); > > > + > > > + clk = devm_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->clk = clk; > > > + } > > > + > > > + phy_provider = devm_of_phy_provider_register(dev, > > > + samsung_usb2_phy_xlate); > > > + if (IS_ERR(phy_provider)) { > > > + dev_err(drv->dev, "Failed to register phy provider\n"); > > > + return PTR_ERR(phy_provider); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static struct platform_driver samsung_usb2_phy_driver = { > > > + .probe = samsung_usb2_phy_probe, > > > + .driver = { > > > + .of_match_table = samsung_usb2_phy_of_match, > > > + .name = "samsung-usb2-phy", > > > + .owner = THIS_MODULE, > > > + } > > > +}; > > > + > > > +module_platform_driver(samsung_usb2_phy_driver); > > > +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC USB PHY driver"); > > > +MODULE_AUTHOR("Kamil Debski <k.debski@xxxxxxxxxxx>"); > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_ALIAS("platform:samsung-usb2-phy"); > > > + > > > diff --git a/drivers/phy/phy-samsung-usb2.h > > > b/drivers/phy/phy-samsung-usb2.h new file mode 100644 index > > > 0000000..cd12477 > > > --- /dev/null > > > +++ b/drivers/phy/phy-samsung-usb2.h > > > @@ -0,0 +1,72 @@ > > > +/* > > > + * Samsung SoC USB 1.1/2.0 PHY driver > > > + * > > > + * Copyright (C) 2013 Samsung Electronics Co., Ltd. > > > + * Author: Kamil Debski <k.debski@xxxxxxxxxxx> > > > + * > > > + * 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_EXYNOS_USB2_H > > > +#define _PHY_EXYNOS_USB2_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 KHZ 1000 > > > +#define MHZ (KHZ * KHZ) > > > + > > > +struct samsung_usb2_phy_driver; > > > +struct samsung_usb2_phy_instance; > > > +struct samsung_usb2_phy_config; > > > + > > > +struct samsung_usb2_phy_instance { > > > + struct samsung_usb2_phy_driver *drv; > > > + struct phy *phy; > > > + const struct samsung_usb2_common_phy *cfg; > > > + char enabled; > > > + struct clk *clk; > > > + u32 clk_reg_val; > > > + unsigned long rate; > > > +}; > > > + > > > +struct samsung_usb2_phy_driver { > > > + struct device *dev; > > > + spinlock_t lock; > > > + void __iomem *reg_phy; > > > + struct regmap *reg_sys; > > > + struct regmap *reg_pmu; > > > + const struct samsung_usb2_phy_config *cfg; > > > + struct clk *clk; > > > + struct samsung_usb2_phy_instance instances[0]; }; > > > + > > > +struct samsung_usb2_common_phy { > > > + char *label; > > > + unsigned int id; > > > + int (*rate_to_clk)(unsigned long, u32 *); > > > + int (*power_on)(struct samsung_usb2_phy_instance *); > > > + int (*power_off)(struct samsung_usb2_phy_instance *); }; > > > + > > > + > > > +struct samsung_usb2_phy_config { > > > + int num_phys; > > > + const struct samsung_usb2_common_phy *phys; > > > + char has_mode_switch; > > > > u8 instead? > > Do we really need to specify that we need 8bits? Why do you think char > is wrong? > > Please read this paragraph from LDD3: > "Sometimes kernel code requires data items of a specific size, perhaps > to match predefined binary structures,* to communicate with user space, > or to align data within structures by inserting "padding" fields (but > refer to the section "Data Alignment" for information about alignment > issues)." > Chapter 11, page 290 http://lwn.net/images/pdf/LDD3/ch11.pdf > > has_mode_switch is only a flag 0 or 1. Never written anywhere in > hardware registers. Used in an if somewhere in code. Give me a good > reason why do you think it should be explicitly 8 bit long. > On the second thought, we both are wrong as bool would be the best choice here. Have a good weekend. Best wishes, -- Kamil Debski Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
