On Thu, 11 Dec 2014 arun.ramamurthy@xxxxxxxxxxxx wrote:
From: Arun Ramamurthy <arunrama@xxxxxxxxxxxx>
Added support for cases where one controller is connected
to multiple phys
Will this continue to work on systems that use only one PHY? What
about systems that don't use DT?
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 4369299..eef82f1 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -38,7 +38,8 @@
struct ohci_platform_priv {
struct clk *clks[OHCI_MAX_CLKS];
struct reset_control *rst;
- struct phy *phy;
+ struct phy *phys;
Is this supposed to be an array of phy _structures_ or an array of
_pointers_ to phy structures? The code says an array of structures,
which seems very suspicious. Why copy phy structures into your own
private array?
+ int num_phys;
};
static const char hcd_name[] = "ohci-platform";
@@ -61,7 +62,7 @@ static int ohci_platform_power_on(struct platform_device *dev)
{
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
- int clk, ret;
+ int clk, ret, phy_num;
for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) {
ret = clk_prepare_enable(priv->clks[clk]);
@@ -69,20 +70,28 @@ static int ohci_platform_power_on(struct platform_device *dev)
goto err_disable_clks;
}
- if (priv->phy) {
- ret = phy_init(priv->phy);
- if (ret)
- goto err_disable_clks;
-
- ret = phy_power_on(priv->phy);
- if (ret)
+ for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
+ ret = phy_init(&priv->phys[phy_num]);
+ if (ret) {
+ if (phy_num == 0)
+ goto err_disable_clks;
+ else
+ goto err_exit_phy;
You don't need this "if" statement. Just goto err_exit_phy always; it
will do the right thing.
+ }
+ ret = phy_power_on(&priv->phys[phy_num]);
+ if (ret) {
+ phy_exit(&priv->phys[phy_num]);
goto err_exit_phy;
+ }
}
return 0;
err_exit_phy:
- phy_exit(priv->phy);
+ while (--phy_num >= 0) {
+ phy_power_off(&priv->phys[phy_num]);
+ phy_exit(&priv->phys[phy_num]);
+ };
err_disable_clks:
while (--clk >= 0)
clk_disable_unprepare(priv->clks[clk]);
@@ -94,11 +103,11 @@ static void ohci_platform_power_off(struct platform_device *dev)
{
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
- int clk;
+ int clk, phy_num;
- if (priv->phy) {
- phy_power_off(priv->phy);
- phy_exit(priv->phy);
+ for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
+ phy_power_off(&priv->phys[phy_num]);
+ phy_exit(&priv->phys[phy_num]);
}
for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
@@ -127,7 +136,9 @@ static int ohci_platform_probe(struct platform_device *dev)
struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ohci_platform_priv *priv;
struct ohci_hcd *ohci;
- int err, irq, clk = 0;
+ struct phy *temp_phy;
+ const char *phy_name;
+ int err, irq, phy_num, clk = 0;
if (usb_disabled())
return -ENODEV;
@@ -175,12 +186,29 @@ static int ohci_platform_probe(struct platform_device *dev)
if (of_property_read_bool(dev->dev.of_node, "big-endian"))
ohci->flags |= OHCI_QUIRK_BE_MMIO | OHCI_QUIRK_BE_DESC;
- priv->phy = devm_phy_get(&dev->dev, "usb");
- if (IS_ERR(priv->phy)) {
- err = PTR_ERR(priv->phy);
- if (err == -EPROBE_DEFER)
- goto err_put_hcd;
- priv->phy = NULL;
+ priv->num_phys = of_count_phandle_with_args
+ (dev->dev.of_node, "phys", "#phy-cells");
It's things like this that make me question whether this patch will
work correctly. What about systems that don't use OF but do use
devm_phy_get()?
Also, the line break is at a strange place (just before the open paren
of a function call), and the indentation of the continuation line is
unusual.
+ if (priv->num_phys > 0) {
+ priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
+ sizeof(struct phy), GFP_KERNEL);
Here again the indentation is unusual. In this file, continuation
lines are indented by 2 tab stops beyond the statement's opening line.
Not 3.
What happens if the memory allocation fails?
+ for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
+ err = of_property_read_string_index(
+ dev->dev.of_node,
+ "phy-names", phy_num,
+ &phy_name);
+ if (err < 0) {
+ dev_err(&dev->dev, "Phy-names not provided");
+ goto err_put_hcd;
+ }
+
+ temp_phy = devm_of_phy_get(&dev->dev,
+ dev->dev.of_node, phy_name);
+ if (IS_ERR(temp_phy)) {
+ err = PTR_ERR(temp_phy);
+ goto err_put_hcd;
+ } else
+ priv->phys[phy_num] = *temp_phy;
+ }
}
for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
Similar comments apply to the 2/2 patch.
Alan Stern
