| Message ID | 1386246579-25141-2-git-send-email-k.debski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote: > Previously the of_phy_get function took a struct device * and > was declared static. It was impossible to call it from > another driver and thus it was impossible to get phy defined It was never intended to be called from other drivers. What's up with the wrapper of of_phy_get, phy_get()/devm_phy_get()? Why isn't that enough? Thanks Kishon > for a given node. > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/phy/phy-core.c | 12 +++++------- > include/linux/phy/phy.h | 1 + > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 03cf8fb..7fb3474 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -250,20 +250,17 @@ EXPORT_SYMBOL_GPL(phy_power_off); > * not yet loaded. This function uses of_xlate call back function provided > * while registering the phy_provider to find the phy instance. > */ > -static struct phy *of_phy_get(struct device *dev, int index) > +struct phy *of_phy_get(struct device_node *np, int index) > { > int ret; > struct phy_provider *phy_provider; > struct phy *phy = NULL; > struct of_phandle_args args; > > - ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells", > + ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", > index, &args); > - if (ret) { > - dev_dbg(dev, "failed to get phy in %s node\n", > - dev->of_node->full_name); > + if (ret) > return ERR_PTR(-ENODEV); > - } > > mutex_lock(&phy_provider_mutex); > phy_provider = of_phy_provider_lookup(args.np); > @@ -281,6 +278,7 @@ err0: > > return phy; > } > +EXPORT_SYMBOL_GPL(of_phy_get); > > /** > * phy_put() - release the PHY > @@ -370,7 +368,7 @@ struct phy *phy_get(struct device *dev, const char *string) > if (dev->of_node) { > index = of_property_match_string(dev->of_node, "phy-names", > string); > - phy = of_phy_get(dev, index); > + phy = of_phy_get(dev->of_node, index); > if (IS_ERR(phy)) { > dev_err(dev, "unable to find phy\n"); > return phy; > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 6d72269..169f572 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -131,6 +131,7 @@ struct phy *phy_get(struct device *dev, const char *string); > struct phy *devm_phy_get(struct device *dev, const char *string); > void phy_put(struct phy *phy); > void devm_phy_put(struct device *dev, struct phy *phy); > +struct phy *of_phy_get(struct device_node *np, int index); > struct phy *of_phy_simple_xlate(struct device *dev, > struct of_phandle_args *args); > struct phy *phy_create(struct device *dev, const struct phy_ops *ops, > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Kishon Vijay Abraham I [mailto:kishon@ti.com] > Sent: Friday, December 06, 2013 6:31 AM > > Hi, > > On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote: > > Previously the of_phy_get function took a struct device * and was > > declared static. It was impossible to call it from another driver and > > thus it was impossible to get phy defined > > It was never intended to be called from other drivers. What's up with > the wrapper of of_phy_get, phy_get()/devm_phy_get()? Why isn't that > enough? Implementing support for multiple phys in the ehci driver is a bit tricky. Especially when we want to do it right. Please have a look at this part of the dts file: + ehci@12580000 { + compatible = "samsung,exynos4210-ehci"; + reg = <0x12580000 0x20000>; + interrupts = <0 70 0>; + clocks = <&clock 304>, <&clock 305>; + clock-names = "usbhost", "otg"; + status = "disabled"; + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + phys = <&usb2phy 1>; + phy-names = "host"; + status = "disabled"; + }; + port@1 { + reg = <1>; + phys = <&usb2phy 2>; + phy-names = "hsic0"; + status = "disabled"; + }; + port@2 { + reg = <2>; + phys = <&usb2phy 3>; + phy-names = "hsic1"; + status = "disabled"; + }; + }; With the above we have a clear specification of ports and their respective phys. But to do this properly the ehci driver has to iterate over port nodes. It is much easier to use devm_of_phy_get by giving the node as its argument. [snip] Best wishes,
On Friday 06 December 2013 04:22 PM, Kamil Debski wrote: > Hi, > >> From: Kishon Vijay Abraham I [mailto:kishon@ti.com] >> Sent: Friday, December 06, 2013 6:31 AM >> >> Hi, >> >> On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote: >>> Previously the of_phy_get function took a struct device * and was >>> declared static. It was impossible to call it from another driver and >>> thus it was impossible to get phy defined >> >> It was never intended to be called from other drivers. What's up with >> the wrapper of of_phy_get, phy_get()/devm_phy_get()? Why isn't that >> enough? > > Implementing support for multiple phys in the ehci driver is a bit tricky. > Especially when we want to do it right. Please have a look at this part of > the dts file: > > + ehci@12580000 { > + compatible = "samsung,exynos4210-ehci"; > + reg = <0x12580000 0x20000>; > + interrupts = <0 70 0>; > + clocks = <&clock 304>, <&clock 305>; > + clock-names = "usbhost", "otg"; > + status = "disabled"; > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + phys = <&usb2phy 1>; > + phy-names = "host"; > + status = "disabled"; > + }; > + port@1 { > + reg = <1>; > + phys = <&usb2phy 2>; > + phy-names = "hsic0"; > + status = "disabled"; > + }; > + port@2 { > + reg = <2>; > + phys = <&usb2phy 3>; > + phy-names = "hsic1"; > + status = "disabled"; > + }; > + }; > > With the above we have a clear specification of ports and their respective > phys. But to do this properly the ehci driver has to iterate over port > nodes. It is much easier to use devm_of_phy_get by giving the node as its > argument. I see. There are a couple of more things we do in the wrapper that gets missed while exporting of_phy_get (get_device and try_module_get). You might want to re-work that one. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: Kishon Vijay Abraham I [mailto:kishon@ti.com] > Sent: Monday, December 09, 2013 8:23 AM > > On Friday 06 December 2013 04:22 PM, Kamil Debski wrote: > > Hi, > > > >> From: Kishon Vijay Abraham I [mailto:kishon@ti.com] > >> Sent: Friday, December 06, 2013 6:31 AM > >> > >> Hi, > >> > >> On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote: > >>> Previously the of_phy_get function took a struct device * and was > >>> declared static. It was impossible to call it from another driver > >>> and thus it was impossible to get phy defined > >> > >> It was never intended to be called from other drivers. What's up > with > >> the wrapper of of_phy_get, phy_get()/devm_phy_get()? Why isn't that > >> enough? > > > > Implementing support for multiple phys in the ehci driver is a bit > tricky. > > Especially when we want to do it right. Please have a look at this > > part of the dts file: > > > > + ehci@12580000 { > > + compatible = "samsung,exynos4210-ehci"; > > + reg = <0x12580000 0x20000>; > > + interrupts = <0 70 0>; > > + clocks = <&clock 304>, <&clock 305>; > > + clock-names = "usbhost", "otg"; > > + status = "disabled"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + port@0 { > > + reg = <0>; > > + phys = <&usb2phy 1>; > > + phy-names = "host"; > > + status = "disabled"; > > + }; > > + port@1 { > > + reg = <1>; > > + phys = <&usb2phy 2>; > > + phy-names = "hsic0"; > > + status = "disabled"; > > + }; > > + port@2 { > > + reg = <2>; > > + phys = <&usb2phy 3>; > > + phy-names = "hsic1"; > > + status = "disabled"; > > + }; > > + }; > > > > With the above we have a clear specification of ports and their > > respective phys. But to do this properly the ehci driver has to > > iterate over port nodes. It is much easier to use devm_of_phy_get by > > giving the node as its argument. > > I see. There are a couple of more things we do in the wrapper that gets > missed while exporting of_phy_get (get_device and try_module_get). You > might want to re-work that one. Thank you for the review. I have just sent an updated version of the core patches. Best wishes,
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 03cf8fb..7fb3474 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -250,20 +250,17 @@ EXPORT_SYMBOL_GPL(phy_power_off); * not yet loaded. This function uses of_xlate call back function provided * while registering the phy_provider to find the phy instance. */ -static struct phy *of_phy_get(struct device *dev, int index) +struct phy *of_phy_get(struct device_node *np, int index) { int ret; struct phy_provider *phy_provider; struct phy *phy = NULL; struct of_phandle_args args; - ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells", + ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", index, &args); - if (ret) { - dev_dbg(dev, "failed to get phy in %s node\n", - dev->of_node->full_name); + if (ret) return ERR_PTR(-ENODEV); - } mutex_lock(&phy_provider_mutex); phy_provider = of_phy_provider_lookup(args.np); @@ -281,6 +278,7 @@ err0: return phy; } +EXPORT_SYMBOL_GPL(of_phy_get); /** * phy_put() - release the PHY @@ -370,7 +368,7 @@ struct phy *phy_get(struct device *dev, const char *string) if (dev->of_node) { index = of_property_match_string(dev->of_node, "phy-names", string); - phy = of_phy_get(dev, index); + phy = of_phy_get(dev->of_node, index); if (IS_ERR(phy)) { dev_err(dev, "unable to find phy\n"); return phy; diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 6d72269..169f572 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -131,6 +131,7 @@ struct phy *phy_get(struct device *dev, const char *string); struct phy *devm_phy_get(struct device *dev, const char *string); void phy_put(struct phy *phy); void devm_phy_put(struct device *dev, struct phy *phy); +struct phy *of_phy_get(struct device_node *np, int index); struct phy *of_phy_simple_xlate(struct device *dev, struct of_phandle_args *args); struct phy *phy_create(struct device *dev, const struct phy_ops *ops,