| Message ID | 1400586822-3837-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
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. > + 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?
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
On Tue, May 20, 2014 at 02:18:30PM +0200, Marek Szyprowski wrote: > On 2014-05-20 13:57, Mark Brown wrote: > >> + 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? Sounds reasonable to me, yes.
diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c index a31641e18d19..52cb7549b775 100644 --- a/drivers/usb/misc/usb3503.c +++ b/drivers/usb/misc/usb3503.c @@ -18,6 +18,7 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ +#include <linux/clk.h> #include <linux/i2c.h> #include <linux/gpio.h> #include <linux/delay.h> @@ -57,10 +58,12 @@ struct usb3503 { enum usb3503_mode mode; struct regmap *regmap; struct device *dev; + struct clk *clk; u8 port_off_mask; int gpio_intn; int gpio_reset; int gpio_connect; + bool secondary_ref_clk; }; static int usb3503_reset(struct usb3503 *hub, int state) @@ -186,6 +189,25 @@ static int usb3503_probe(struct usb3503 *hub) } else if (np) { hub->port_off_mask = 0; + hub->clk = devm_clk_get(dev, "refclk"); + if (!IS_ERR(hub->clk)) { + unsigned long rate; + + clk_prepare_enable(hub->clk); + rate = clk_get_rate(hub->clk); + + 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); + } + property = of_get_property(np, "disabled-ports", &len); if (property && (len / sizeof(u32)) > 0) { int i; @@ -213,8 +235,10 @@ static int usb3503_probe(struct usb3503 *hub) dev_err(dev, "Ports disabled with no control interface\n"); if (gpio_is_valid(hub->gpio_intn)) { - err = devm_gpio_request_one(dev, hub->gpio_intn, - GPIOF_OUT_INIT_HIGH, "usb3503 intn"); + int val = hub->secondary_ref_clk ? GPIOF_OUT_INIT_LOW : + GPIOF_OUT_INIT_HIGH; + err = devm_gpio_request_one(dev, hub->gpio_intn, val, + "usb3503 intn"); if (err) { dev_err(dev, "unable to request GPIO %d as connect pin (%d)\n",
USB3503 chip supports 8 values of reference clock. The value is specified by REF_SEL[1:0] pins and INT_N line. This patch add support for getting 'refclk' clock, enabling it and setting INT_N line according to the value of the gathered clock. If no clock has been specified, driver defaults to the old behaviour (assuming that clock has been specified by REF_SEL pins from primary reference clock frequencies table). Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/usb/misc/usb3503.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) Hello, This extension to USB3503 driver is needed to add support for OdroidU3 board, which uses 24MHz reference clock, sourced directly from CLKOUT line from Exynos4412 SoC. Best regards Marek Szyprowski Samsung R&D Institute Poland