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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
