[PATCH v3] usb: phy: samsung: Add support to set pmu isolation
From: dianders@chromium.org (Doug Anderson)
Date: 2012-12-21 17:05:35
Also in:
linux-devicetree, linux-samsung-soc, lkml
Vivek, Nothing really serious below and things look good to me, but figured I'd put a few nits in (sorry!). On Fri, Dec 21, 2012 at 12:16 AM, Vivek Gautam [off-list ref] wrote:
quoted hunk ↗ jump to hunk
diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt index 7b26e2d..09f06f8 100644 --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt@@ -9,3 +9,31 @@ Required properties: - compatible : should be "samsung,exynos4210-usbphy" - reg : base physical address of the phy registers and length of memory mapped region. +- #address-cells: should be 1. +- #size-cells: should be 0.
Doesn't match your example. Probably should be 1.
quoted hunk ↗ jump to hunk
diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c index 5c5e1bb5..2260029 100644 --- a/drivers/usb/phy/samsung-usbphy.c +++ b/drivers/usb/phy/samsung-usbphy.c /* + * struct samsung_usbphy_drvdata - driver data for various SoC variants + * @cpu_type: machine identifier + * @devphy_en_mask: device phy enable mask for PHY CONTROL register + * @hostphy_en_mask: host phy enable mask for PHY CONTROL register + * + * having different mask for host and device type phy + * helps in setting independent masks in case of SoCs like + * S5PV210 in which PHY0 and PHY1 enable bits belong to same + * register placed at [0] and [1] respectively. + * Although for newer SoCs like exynos these bits belong to + * different registers altogether placed at [0]. + */ +struct samsung_usbphy_drvdata { + int cpu_type; + int devphy_en_mask;
This is really a "devphy_dis_mask", isn't it? AKA: setting to 1 disables the phy and setting to 0 enables the phy.
+ int hostphy_en_mask;
Code below always uses devphy and only ever inits devphy. I assume future code will init / use hostphy? Worth moving the hostphy part in that patch?
quoted hunk ↗ jump to hunk
struct samsung_usbphy { struct usb_phy phy;@@ -81,12 +104,66 @@ struct samsung_usbphy { struct device *dev; struct clk *clk; void __iomem *regs; + void __iomem *phyctrl_pmureg; int ref_clk_freq; - int cpu_type; + struct samsung_usbphy_drvdata *drv_data;
nit: const
+static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
+{
+ struct device_node *usbphy_pmu;
+ u32 reg[2];
+ int ret;
+
+ if (!sphy->dev->of_node) {
+ dev_err(sphy->dev, "Can't get usb-phy node\n");
+ return -ENODEV;
+ }
+
+ usbphy_pmu = of_get_child_by_name(sphy->dev->of_node, "usbphy-pmu");
+ if (!usbphy_pmu)
+ dev_warn(sphy->dev, "Can't get usb-phy pmu control node\n");
+
+ ret = of_property_read_u32_array(usbphy_pmu, "reg", reg, 2);nit: use ARRAY_SIZE(reg)
+ if (!ret)
+ sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
+
+ of_node_put(usbphy_pmu);
+
+ if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {
+ dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");I don't think there's any cases where it matters (you'll error out of the driver if you return an error here), but seems like it might be nice to set sphy->phyctrl_pmureg to NULL here since other places test this member against NULL only.
+static inline struct samsung_usbphy_drvdata
+*samsung_usbphy_get_driver_data(struct platform_device *pdev)
{
if (pdev->dev.of_node) {
const struct of_device_id *match;
match = of_match_node(samsung_usbphy_dt_match,
pdev->dev.of_node);
- return (int) match->data;
+ return (struct samsung_usbphy_drvdata *) match->data;nit: no need for a cast here, I believe.
} - return platform_get_device_id(pdev)->driver_data; + return ((struct samsung_usbphy_drvdata *) + platform_get_device_id(pdev)->driver_data);
nit: no need for a cast here, I believe.
+static struct samsung_usbphy_drvdata usbphy_s3c64xx = {
+ .cpu_type = TYPE_S3C64XX,
+ .devphy_en_mask = S3C64XX_USBPHY_ENABLE,
+};
+
+static struct samsung_usbphy_drvdata usbphy_exynos4 = {
+ .cpu_type = TYPE_EXYNOS4210,
+ .devphy_en_mask = EXYNOS_USBPHY_ENABLE,
+};
+nit: static const for these structs? -Doug