Hi Russell,
On Thu, Dec 27, 2012 at 5:56 AM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> On Wed, Dec 26, 2012 at 05:58:32PM +0530, Vivek Gautam wrote:
>> + if (!ret)
>> + sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
>> +
>> + of_node_put(usbphy_pmu);
>> +
>> + if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {
>
> No. Learn what the error return values are from functions. Using the
> wrong ones is buggy. ioremap() only ever returns NULL on error. You
> must check against NULL, and not use the IS_ERR stuff.
>
True, i should have checked the things. :-(
ioremap() won't return error. Will amend this to check against NULL.
>> +/*
>> + * Set isolation here for phy.
>> + * SOCs control this by controlling corresponding PMU registers
>> + */
>> +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on)
>> +{
>> + u32 reg;
>> + int en_mask;
>> +
>> + if (!sphy->phyctrl_pmureg) {
>> + dev_warn(sphy->dev, "Can't set pmu isolation\n");
>> + return;
>> + }
>> +
>> + reg = readl(sphy->phyctrl_pmureg);
>> +
>> + en_mask = sphy->drv_data->devphy_en_mask;
>> +
>> + if (on)
>> + writel(reg & ~en_mask, sphy->phyctrl_pmureg);
>> + else
>> + writel(reg | en_mask, sphy->phyctrl_pmureg);
>
> What guarantees that this read-modify-write sequence of this register safe?
> And, btw, this can't be optimised very well because of the barrier inside
> writel(). This would be better:
>
> if (on)
> reg &= ~en_mask;
> else
> reg |= en_mask;
>
> writel(reg, sphy->phyctrl_pmureg);
>
Sure will amend this.
A similar way suggested by Sylwester in the earlier mail in this thread:
reg = on ? reg & ~mask : reg | mask;
writel(reg, sphy->phyctrl_pmureg);
Does this look fine ?
>> +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;
>
> match->data is a const void pointer. Is there a reason you need this
> cast here? What if you made the returned pointer from this function
> also const and fixed up all its users (no user should modify this
> data.)
>
Right, we won't need this cast since match->data is a void pointer.
Will also make the returned pointer const.
>> #ifdef CONFIG_OF
>> static const struct of_device_id samsung_usbphy_dt_match[] = {
>> {
>> .compatible = "samsung,s3c64xx-usbphy",
>> - .data = (void *)TYPE_S3C64XX,
>> + .data = (void *)&usbphy_s3c64xx,
>
> Why do you need this cast?
>
True we don't need this cast. Will remove this one.
>> }, {
>> .compatible = "samsung,exynos4210-usbphy",
>> - .data = (void *)TYPE_EXYNOS4210,
>> + .data = (void *)&usbphy_exynos4,
>
> Ditto.
True we don't need this cast. Will remove this one.
Thanks for the review :-)
--
Regards
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
