On 04/13/2016 10:01 AM, Thierry Reding wrote:
On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote:
On 03/08/2016 08:48 AM, Thierry Reding wrote:
From: Thierry Reding <treding@xxxxxxxxxx>
The current XUSB pad controller bindings are insufficient to describe
PHY devices attached to USB controllers. New bindings have been created
to overcome these restrictions. As a side-effect each root port now is
assigned a set of PHY devices, one for each lane associated with the
root port. This has the benefit of allowing fine-grained control of the
power management for each lane.
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
+static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port)
+{
+ struct device *dev = port->pcie->dev;
+ unsigned int i;
+ int err;
+
+ for (i = 0; i < port->lanes; i++) {
+ err = phy_power_on(port->phys[i]);
This assume the number of array entries is precisely the number of lanes.
That seems to contradict the binding update which said the number might not
match. Perhaps there's an expectation that phy_power_on() is a no-op for
some "invalid" values like NULL or an error-pointer value? But...
+static struct phy *devm_of_phy_optional_get_index(struct device *dev,
+ struct device_node *np,
+ const char *consumer,
+ unsigned int index)
+{
+ struct phy *phy;
+ char *name;
+
+ name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index);
+ if (!name)
+ return ERR_PTR(-ENOMEM);
+
+ phy = devm_of_phy_get(dev, np, name);
+ kfree(name);
+
+ if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV)
+ phy = NULL;
+
+ return phy;
+}
The error-handling there looks wrong. The function generally returns either
a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is
returned. Subsystems are supposed to encode their handles as, and functions
are supposed to return, either NULL or an error pointer for error cases, not
both/either. Is the PHY API broken in this regard? If so, then this code is
fine, but if not it might need a fix.
This function mimics phy_optional_get() which similarily returns NULL
for -ENODEV. The remainder of the PHY API treats NULL pointers as
"dummy" PHYs and returns early. I think that's a sensible approach to
handling optional resources.
It might have been more obvious had I implemented this function within
phy-core.c, but I didn't think it universally useful because it uses a
rather uncommon lookup pattern. I did keep a generic name in case it's
ever deemed useful outside of this driver, at which point it could
simply be moved into phy-core.c without requiring this driver to
change.
Ah OK, so if a caller of this function is expected to only use IS_ERR(),
and hence treat NULL as a perfectly valid PHY value, and all the PHY
APIs deal with NULL correctly, then this is probably OK.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
