Hello, On 2014-05-20 13:57, Mark Brown wrote:
On Tue, May 20, 2014 at 01:53:42PM +0200, Marek Szyprowski wrote:> + hub->clk = devm_clk_get(dev, "refclk"); > + if (!IS_ERR(hub->clk)) { This won't handle deferred probe - the driver should error out if it gets -EPROBE_DEFER since it means the clock exists and might be provided later on.
Ok, I will add such case here.
> + unsigned long rate; > + > + clk_prepare_enable(hub->clk); > + rate = clk_get_rate(hub->clk); No error checking here. > + > + if (rate == 38400000 || rate == 26000000 || > + rate == 19200000 || rate == 12000000) > + hub->secondary_ref_clk = 0; > + else if (rate == 24000000 || rate == 27000000 || > + rate == 25000000 || rate == 50000000) > + hub->secondary_ref_clk = 1; > + else > + dev_err(dev, > + "unsupported reference clock rate (%d)\n", > + rate); This looks like a switch statement. Should the driver not try to set the clock to a supported rate if it's not already at one rather than error out - it seems like a more constructive thing to do?
Hmm, the problem here is that you cannot determine the right rate. The rate is encoded on REF_SEL[1:0] pins (usually soldered to some resistors) and cannot be read via i2c commands. To set add support for rate setting, I would need to add
one more property with correct ref clock rate. Is this okay? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
