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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
