On Thursday, 12 September, 2013 at 3:01 PM, Sascha Hauer wrote: > On Thu, Sep 12, 2013 at 02:44:01PM +0800, Sean Cross wrote: > > > Sean, > > > > > > Several more comments. > > > > > > > +static int pcie_phy_poll_ack(void __iomem *dbi_base, int max_iterations, > > > > + int exp_val) > > > > +{ > > > > + u32 val; > > > > + u32 wait_counter = 0; > > > > + > > > > + do { > > > > + val = readl(dbi_base + PCIE_PHY_STAT); > > > > + val = (val >> PCIE_PHY_STAT_ACK_LOC) & 0x1; > > > > + wait_counter++; > > > > + } while ((wait_counter < max_iterations) && (val != exp_val)); > > > > > > > > > > > > > > > > > > may_iterations is never something else than 100 so you should drop the > > > argument. > > > Also I would prefer a real timeout here rather than a counter. > > > > > > > > Is there a construct to have a real timer? Would a completion work here instead? > > A completion is overkill. Polling is fine, but it should be a well > defined amount of time which is independent of current cpu speed. Adding > a udelay(1) to the loop should be enough. In that case, it makes sense to drop the max_iterations value by a factor of 10 or so. > > > > > > > > > I'm unsure this should be done here. Shawn, should we do this in SoC > > > code? > > > > > > > > A different board could easily use lvds2 to drive the PCIe clock. I think that I should change the variable names to simply "lvds_sel". > > Why should it use a different clock? It's fine to make this > configurable, but only if we actually know the reasons why it should be > configurable. > > Even if it should be configurable I'm still unsure whether this should > be done at pci driver level. I'm a bit wary of including clock setup like this in the SoC code, as different boards might be different. If a board designer wants to run LVDS2 to the PCIe slot, that's entirely acceptable. It might not actually ever get done, but it seems like the sort of board-specific thing that device tree was designed to handle. > > > devm_gpio_request_one can still fail. > > > > It can fail, but it's only critical for the power switch. However, arguably it's a critical error even if these GPIOs can't be allocated, so I'll go ahead and add the same consistency check and error return code to the other three switches. > > > > If you want to make the gpios optional that's fine, but when > of_get_named_gpio succeeds, then devm_gpio_request_one should succeed > aswell, otherwise there's something wrong. Ah, alright. I'll add the errors, then. > > > > + struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev); > > > > + > > > > + clk_disable_unprepare(imx6_pcie->pcie_axi); > > > > + clk_disable_unprepare(imx6_pcie->lvds1_gate); > > > > + clk_disable_unprepare(imx6_pcie->pcie_ref_125m); > > > > > > > > > > > > > > > > > > Still no unregister for the pcie controller? > > The Designware core doesn't support unregistering (or even module unloading). I'm not sure how to handle that case. For that matter, when is remove() even called on a module that can't be unloaded? > > > > Then don't implement the remove callback. That's better than providing a > bogus remove function. I'll cut out the remove() function. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html