On Tue, Apr 24, 2012 at 07:12:10PM +0530, viresh kumar wrote: > On 4/24/12, Andrew Lunn <andrew@xxxxxxx> wrote: > > Sorry, but still wrong. > > > > The clock is optional. If we can find a clock, turn it on. If not, > > keep going.... > > > > You patch causes the missing clock to become a fatal error. > > > > This sata_mv exists in multiple forms. It can be part of a SoC. It can > > also be on a PCI bus. In the PCI form, it is unlikely to have a clk > > which can be controlled. When built into a SoC, namely one of the > > Orion family, dove, orion5x, mv78xx0 do not have a clock which can be > > controlled. However kirkwood does have a clock. > > > > So, kirkwood will provide a clock and expects that sata_mv will turn > > it on. All the other ways of using sata_mv will not provide a clock, > > but still expect the driver to be happy. > > Hmm. What this code does now is: > If HAVE_CLK is selected, then there must be a clock for the device. Otherwise > it will always pass. > -#if defined(CONFIG_HAVE_CLK) hpriv->clk = clk_get(&pdev->dev, NULL); - if (IS_ERR(hpriv->clk)) - dev_notice(&pdev->dev, "cannot get clkdev\n"); - else - clk_enable(hpriv->clk); -#endif + if (IS_ERR(hpriv->clk)) { + dev_err(&pdev->dev, "cannot get clkdev\n"); + return PTR_ERR(hpriv->clk); + } + + clk_enable(hpriv->clk); There are three use cases.... 1) The platform does not implement HAVE_CLK. So we are using your NOP operations. clk_get() returns NULL. IS_ERR(NULL) is false. So it keeps going, calls clk_enable() which is also NOP and the driver is happy. 2) The platform does have HAVE_CLK. So we are using driver/clk/ code. There is no clk defined for the device, since there is no controllable clk. Its using the PCI clock, or some hard wired internal SoC clock. clk_get() returns ERR_PTR(-ENOENT) IS_ERR(hpriv->clk) is true, you output a dev_err() and the device probe fails. This is wrong. Not having the clk is not fatal. The old code would carefully not call clk_enable(), since it has an error pointer, not have a valid clk, and would also not call clk_disable during removal. 3) The platform does have HAVE_CLK. So we are using driver/clk/ code. There is however a gateable clock driving this lump of silicon. clk_get() returns a valid pointer to a clk. IS_ERR(hpriv->clk) is false, so it keeps going. clk_enable() is called and the driver is happy. Well, actually, his brings up a new issues static int __clk_enable(struct clk *clk) { int ret = 0; if (!clk) return 0; if (WARN_ON(clk->prepare_count == 0)) return -ESHUTDOWN; this should actually be clk_prepare_enable(). Did you see my patches adding generic clk framework support for Orion. I fixed this as part of that patch set. Anyway.... You asked: > You want not to return error if a platform does have HAVE_CLK, but > doesn't have a clock for sata? That would be simple to fix, but want > to confirm if this is actually required. Correct. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html