On Tue, Oct 07, 2014 at 08:44:05AM -0700, Stephen Warren wrote:
> On 10/07/2014 03:27 AM, Vidya Sagar wrote:
> > Enables root port to advertise its ASPM-L1 capability
> > resulting in possible link entry to L1 when an ASPM-L1 capable
> > device is connected
> > Enables per-controller & per-TMS clock clamping by default
> > Enabling above features result in more power saving
> >
> > It also avoids PM message truncation by waiting for DLLP to finish
> > before entering into L1 or L2
> >
> > Also, it adds helper functions to access root port registers
>
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>
> > @@ -1870,8 +1920,10 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
> >
> > tegra_pcie_port_enable(port);
> >
> > - if (tegra_pcie_port_check_link(port))
> > + if (tegra_pcie_port_check_link(port)) {
> > + tegra_pcie_enable_rp_features(port);
> > continue;
> > + }
> >
> > dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
>
> Wouldn't it be better to have the error case inside the if block; most
> error-handling is that way. For example:
>
> if (!tegra_pcie_port_check_link(port)) {
> dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
> tegra_pcie_port_disable(port);
> tegra_pcie_port_free(port);
> continue;
> }
>
> tegra_pcie_enable_rp_features(port);
> ... any other "good case" code (of which there admittedly is none at
> present)Yes, that's a little more idiomatic. I think it could be a separate cleanup patch, though. Thierry
pgpNSREqQoqPo.pgp
Description: PGP signature
