On Fri, Apr 08, 2016 at 01:05:28PM -0500, Bjorn Helgaas wrote:
> Hi Thierry,
>
> I think there are a couple typos (one in a message and one that
> actually looks important), and one question below.
>
> On Fri, Apr 08, 2016 at 06:13:14PM +0200, Thierry Reding wrote:
[...]
> > +static int tegra_pcie_phy_power_off(struct tegra_pcie *pcie)
> > +{
> > + struct tegra_pcie_port *port;
> > + int err;
> > +
> > + if (pcie->legacy_phy) {
> > + if (pcie->phy)
> > + err = phy_power_on(pcie->phy);
>
> s/phy_power_on/phy_power_off/
Good catch. Fixed.
> > @@ -899,13 +1025,9 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> > afi_writel(pcie, value, AFI_FUSE);
> > }
> >
> > - if (!pcie->phy)
> > - err = tegra_pcie_phy_enable(pcie);
> > - else
> > - err = phy_power_on(pcie->phy);
> > -
> > + err = tegra_pcie_phy_power_on(pcie);
> > if (err < 0) {
> > - dev_err(pcie->dev, "failed to power on PHY: %d\n", err);
> > + dev_err(pcie->dev, "failed to power off PHY(s): %d\n", err);
>
> s/off/on/
Fixed.
> > +static int tegra_pcie_phys_get(struct tegra_pcie *pcie)
> > +{
> > + struct tegra_pcie_port *port;
> > + int err;
> > +
> > + if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL)
> > + return tegra_pcie_phys_get_legacy(pcie);
> > +
> > + list_for_each_entry(port, &pcie->ports, list) {
> > + err = tegra_pcie_port_get_phys(port);
> > + if (err < 0) {
> > + return err;
> > + }
> > + }
>
> This seems backwards: if I'm reading this right, you first check for
> the legacy property ("phys") and use it if you find it. If there is
> no legacy property, you look for the new per-lane PHYs.
>
> The usual pattern would be "look for the new stuff, and if you don't
> find it, fall back to the old stuff." Is there a configuration that
> could be described either way, e.g., something with only one lane and
> only one PHY?
>
> I'm not sure whether it matters, but if it *could* use the "new, fall
> back to old" pattern, that would be nice and would keep people from
> wondering whether it's safe to do it backwards.
The reason why I wrote it this way is to special case the legacy code.
The alternative would be to do:
if (of_get_property(...) == NULL) {
list_for_each_entry(port, &pcie->ports, list) {
err = tegra_pcie_port_get_phys(port);
if (err < 0)
return err;
}
}
return tegra_pcie_phys_get_legacy(pcie);
Which is better in the way you describe (fall back to legacy if new
binding is not found). But like I said, it makes, from the code flow,
the new binding the exception, which looks odd to me as well.
Perhaps this could be somewhat mitigated by wrapping the new code into
a separate function:
if (of_get_property(...) == NULL)
return tegra_pcie_phys_get_per_lane(pcie);
return tegra_pcie_phys_get_legacy(pcie);
I don't feel very strongly in either direction. Do you have a
preference?
Thierry
Attachment:
signature.asc
Description: PGP signature
