Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751872AbbKTPR0 (ORCPT ); Fri, 20 Nov 2015 10:17:26 -0500 Received: from mail-wm0-f46.google.com ([74.125.82.46]:36068 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1162878AbbKTPRP (ORCPT ); Fri, 20 Nov 2015 10:17:15 -0500 MIME-Version: 1.0 In-Reply-To: <2396575.drsjxiqpHR@avalon> References: <1447958344-836-1-git-send-email-geert+renesas@glider.be> <3383903.1R72kZGx62@avalon> <2396575.drsjxiqpHR@avalon> Date: Fri, 20 Nov 2015 16:17:13 +0100 X-Google-Sender-Auth: YWyYr9Se4FOopvjiYerdLMqyhcE Message-ID: Subject: Re: [PATCH 18/25] serial: sh-sci: Prepare for multiple clocks and baud rate generators From: Geert Uytterhoeven To: Laurent Pinchart Cc: Geert Uytterhoeven , Greg Kroah-Hartman , Simon Horman , Magnus Damm , Yoshinori Sato , "linux-serial@vger.kernel.org" , Linux-sh list , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5696 Lines: 131 Hi Laurent, On Fri, Nov 20, 2015 at 3:47 PM, Laurent Pinchart wrote: > On Friday 20 November 2015 08:52:19 Geert Uytterhoeven wrote: >> On Thu, Nov 19, 2015 at 10:04 PM, Laurent Pinchart wrote: >> > On Thursday 19 November 2015 19:38:57 Geert Uytterhoeven wrote: >> >> Refactor the clock and baud rate parameter code to ease adding support >> >> for multiple clocks and baud rate generators later. >> >> sci_scbrr_calc() now returns the bit rate error, so it can be compared >> >> to the bit rate error for other baud rate generators. >> >> >> >> Signed-off-by: Geert Uytterhoeven >> >> --- >> >> >> >> drivers/tty/serial/sh-sci.c | 176 +++++++++++++++++++++++++------------- >> >> 1 file changed, 120 insertions(+), 56 deletions(-) >> >> >> >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> >> index 726c96d5a511c222..12800e52f41953dc 100644 >> >> --- a/drivers/tty/serial/sh-sci.c >> >> +++ b/drivers/tty/serial/sh-sci.c >> >> @@ -2252,33 +2301,48 @@ static struct uart_ops sci_uart_ops = { >> >> static int sci_init_clocks(struct sci_port *sci_port, struct device >> >> *dev) >> >> { >> >> - /* Get the SCI functional clock. It's called "fck" on ARM. */ >> >> - sci_port->fclk = devm_clk_get(dev, "fck"); >> >> - if (PTR_ERR(sci_port->fclk) == -EPROBE_DEFER) >> >> - return -EPROBE_DEFER; >> >> - if (!IS_ERR(sci_port->fclk)) >> >> - return 0; >> >> + const char *clk_names[] = { >> >> + [SCI_FCK] = "fck", >> >> + }; >> >> + struct clk *clk; >> >> + unsigned int i; >> >> >> >> - /* >> >> - * But it used to be called "sci_ick", and we need to maintain DT >> >> - * backward compatibility. >> >> - */ >> >> - sci_port->fclk = devm_clk_get(dev, "sci_ick"); >> >> - if (PTR_ERR(sci_port->fclk) == -EPROBE_DEFER) >> >> - return -EPROBE_DEFER; >> >> - if (!IS_ERR(sci_port->fclk)) >> >> - return 0; >> >> + for (i = 0; i < SCI_NUM_CLKS; i++) { >> >> + clk = devm_clk_get(dev, clk_names[i]); >> >> + if (PTR_ERR(clk) == -EPROBE_DEFER) >> >> + return -EPROBE_DEFER; >> >> >> >> - /* >> >> - * Not all SH platforms declare a clock lookup entry for SCI >> >> devices, >> >> - * in which case we need to get the global "peripheral_clk" clock. >> >> - */ >> >> - sci_port->fclk = devm_clk_get(dev, "peripheral_clk"); >> >> - if (!IS_ERR(sci_port->fclk)) >> >> - return 0; >> >> + if (IS_ERR(clk) && i == SCI_FCK) { >> >> + /* >> >> + * "fck" used to be called "sci_ick", and we need >> >> to >> >> + * maintain DT backward compatibility. >> >> + */ >> >> + clk = devm_clk_get(dev, "sci_ick"); >> >> + if (PTR_ERR(clk) == -EPROBE_DEFER) >> >> + return -EPROBE_DEFER; >> >> + >> >> + if (!IS_ERR(clk)) >> >> + goto found; >> >> + >> >> + /* >> >> + * Not all SH platforms declare a clock lookup >> >> entry >> >> + * for SCI devices, in which case we need to get >> >> the >> >> + * global "peripheral_clk" clock. >> >> + */ >> >> + clk = devm_clk_get(dev, "peripheral_clk"); >> >> + if (!IS_ERR(clk)) >> >> + goto found; >> >> + >> >> + dev_err(dev, "failed to get functional clock\n"); >> >> + return PTR_ERR(clk); >> >> + } >> >> >> >> - dev_err(dev, "failed to get functional clock\n"); >> >> - return PTR_ERR(sci_port->fclk); >> >> +found: >> >> + if (!IS_ERR(clk)) >> >> + dev_dbg(dev, "clk %u is %pC rate %pCr\n", i, clk, >> >> clk); >> >> + sci_port->clks[i] = IS_ERR(clk) ? NULL : clk; >> > >> > Isn't it an issue that we can't tell apart the case where there is no >> > clock specified in DT and the case where we can't get the clock due to >> > another error ? >> >> All failures here are for optional clocks. >> If the real failure is that the clock wasn't specified (or misspelled) in >> DT, it should have been detected during the integration phase. > > There could be cases where the clock is correctly specified in DT but can't be > retrieved due to a runtime error. I suppose that's mostly theoretical in our > case though. Maybe a dev_dbg for the error case could be useful too ? Can we > tell the case where the clock is not specified in DT apart from other errors > (-EPROBE_DEFER aside as that case is already handled) ? If the clock is not in clock/clock-names in DT, the error is definitely -ENOENT. If the clock is specified in DT, it has a phandle to a clock node. If that clock hasn't been instantiated yet, the error is EPROBE_DEFER. Which means there are no other possible error values, right? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/