lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFp+6iFJ5us7e9wY0USiDYn4XyNsxgLF9GhVYQFM9OY0qnxU_w@mail.gmail.com> Date: Wed, 26 Dec 2012 17:37:10 +0530 From: Vivek Gautam <gautamvivek1987@...il.com> To: Doug Anderson <dianders@...omium.org> Cc: Vivek Gautam <gautam.vivek@...sung.com>, l.majewski@...sung.com, linux-samsung-soc@...r.kernel.org, Praveen Paneri <p.paneri@...sung.com>, gregkh@...uxfoundation.org, devicetree-discuss@...ts.ozlabs.org, linux-usb@...r.kernel.org, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, balbi@...com, kyungmin.park@...sung.com, Kukjin Kim <kgene.kim@...sung.com>, Ben Dooks <ben-linux@...ff.org>, Mark Brown <broonie@...nsource.wolfsonmicro.com>, Sylwester Nawrocki <sylvester.nawrocki@...il.com>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org> Subject: Re: [PATCH v3] usb: phy: samsung: Add support to set pmu isolation Hi Doug, On Fri, Dec 21, 2012 at 10:35 PM, Doug Anderson <dianders@...omium.org> wrote: > 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 <gautam.vivek@...sung.com> wrote: >> 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. Oops !! true it is 1, will amend this. > >> 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. This is actually 'devphy_en_mask' only. We use it this way: When pmu_isolation is true, meaning usbphy is isolated from pmu so we are resetting this bit to disable usbphy, as there in samsung_usbphy_set_isolation(). > >> + 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? > Sure will move this in forthcoming patch for host phy. >> 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 Will make it 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) > Sure will amend this. >> + 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. > Isn't devm_kzallocing the memory for sphy setting sphy->phyctrl_pmureg to NULL ? Then, does checking here for IS_ERR_OR_NULL(sphy->phyctrl_pmureg) has a problem ? Probably i am not getting what is expected here. :-( >> +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. > "samsung_usbphy_get_driver_data()" is returning (struct samsung_usbphy_drvdata *) and the data is actually (void *). So won't we need a cast here. I am actually getting compile time warnings. >> } >> >> - 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. > ditto here, the driver data for non dt is (unsigned long). So ? >> +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? Sure will make them const. no harm. > > > > -Doug > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@...ts.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss -- Thanks & Regards Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists