| Message ID | 1429779066-21406-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
2015-04-23 10:51 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > Just because of_clk_get() doesn't mean it should be used. Use devm_clk_get > which is the correct function when a struct device * is available. Also > since it's a devm function we can drop the calls to clk_put(). While at > it also fix a wrong debug message. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/tty/serial/8250/8250_mtk.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > index 7a11fac..c6901dc 100644 > --- a/drivers/tty/serial/8250/8250_mtk.c > +++ b/drivers/tty/serial/8250/8250_mtk.c > @@ -131,18 +131,16 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > struct mtk8250_data *data) > { > int err; > - struct device_node *np = pdev->dev.of_node; > > - data->uart_clk = of_clk_get(np, 0); > + data->uart_clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(data->uart_clk)) { > - dev_warn(&pdev->dev, "Can't get timer clock\n"); > + dev_warn(&pdev->dev, "Can't get uart clock\n"); > return PTR_ERR(data->uart_clk); > } > > err = clk_prepare_enable(data->uart_clk); > if (err) { > dev_warn(&pdev->dev, "Can't prepare clock\n"); > - clk_put(data->uart_clk); > return err; > } > p->uartclk = clk_get_rate(data->uart_clk); > @@ -216,7 +214,6 @@ static int mtk8250_remove(struct platform_device *pdev) > serial8250_unregister_port(data->line); > if (!IS_ERR(data->uart_clk)) { > clk_disable_unprepare(data->uart_clk); > - clk_put(data->uart_clk); > } Please delete the braces.
On Fri, Apr 24, 2015 at 12:28:33PM +0200, Matthias Brugger wrote: > 2015-04-23 10:51 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > > Just because of_clk_get() doesn't mean it should be used. Use devm_clk_get > > which is the correct function when a struct device * is available. Also > > since it's a devm function we can drop the calls to clk_put(). While at > > it also fix a wrong debug message. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/tty/serial/8250/8250_mtk.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > > index 7a11fac..c6901dc 100644 > > --- a/drivers/tty/serial/8250/8250_mtk.c > > +++ b/drivers/tty/serial/8250/8250_mtk.c > > @@ -131,18 +131,16 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > > struct mtk8250_data *data) > > { > > int err; > > - struct device_node *np = pdev->dev.of_node; > > > > - data->uart_clk = of_clk_get(np, 0); > > + data->uart_clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(data->uart_clk)) { > > - dev_warn(&pdev->dev, "Can't get timer clock\n"); > > + dev_warn(&pdev->dev, "Can't get uart clock\n"); > > return PTR_ERR(data->uart_clk); > > } > > > > err = clk_prepare_enable(data->uart_clk); > > if (err) { > > dev_warn(&pdev->dev, "Can't prepare clock\n"); > > - clk_put(data->uart_clk); > > return err; > > } > > p->uartclk = clk_get_rate(data->uart_clk); > > @@ -216,7 +214,6 @@ static int mtk8250_remove(struct platform_device *pdev) > > serial8250_unregister_port(data->line); > > if (!IS_ERR(data->uart_clk)) { > > clk_disable_unprepare(data->uart_clk); > > - clk_put(data->uart_clk); > > } > > Please delete the braces. The if() is removed completly in the next patch anyway. Sascha
2015-04-24 12:31 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > On Fri, Apr 24, 2015 at 12:28:33PM +0200, Matthias Brugger wrote: >> 2015-04-23 10:51 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: >> > Just because of_clk_get() doesn't mean it should be used. Use devm_clk_get >> > which is the correct function when a struct device * is available. Also >> > since it's a devm function we can drop the calls to clk_put(). While at >> > it also fix a wrong debug message. >> > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> > --- >> > drivers/tty/serial/8250/8250_mtk.c | 7 ++----- >> > 1 file changed, 2 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c >> > index 7a11fac..c6901dc 100644 >> > --- a/drivers/tty/serial/8250/8250_mtk.c >> > +++ b/drivers/tty/serial/8250/8250_mtk.c >> > @@ -131,18 +131,16 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, >> > struct mtk8250_data *data) >> > { >> > int err; >> > - struct device_node *np = pdev->dev.of_node; >> > >> > - data->uart_clk = of_clk_get(np, 0); >> > + data->uart_clk = devm_clk_get(&pdev->dev, NULL); >> > if (IS_ERR(data->uart_clk)) { >> > - dev_warn(&pdev->dev, "Can't get timer clock\n"); >> > + dev_warn(&pdev->dev, "Can't get uart clock\n"); >> > return PTR_ERR(data->uart_clk); >> > } >> > >> > err = clk_prepare_enable(data->uart_clk); >> > if (err) { >> > dev_warn(&pdev->dev, "Can't prepare clock\n"); >> > - clk_put(data->uart_clk); >> > return err; >> > } >> > p->uartclk = clk_get_rate(data->uart_clk); >> > @@ -216,7 +214,6 @@ static int mtk8250_remove(struct platform_device *pdev) >> > serial8250_unregister_port(data->line); >> > if (!IS_ERR(data->uart_clk)) { >> > clk_disable_unprepare(data->uart_clk); >> > - clk_put(data->uart_clk); >> > } >> >> Please delete the braces. > > The if() is removed completly in the next patch anyway. You are right. I propose to merge patch 2 and 3, so you get a clean code. On the way, can you have a look on the commit message? For me it is not clear what you want to say. Cheers, Matthias
2015-04-24 12:53 GMT+02:00 Matthias Brugger <matthias.bgg@gmail.com>: > 2015-04-24 12:31 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: >> On Fri, Apr 24, 2015 at 12:28:33PM +0200, Matthias Brugger wrote: >>> 2015-04-23 10:51 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: >>> > Just because of_clk_get() doesn't mean it should be used. Use devm_clk_get >>> > which is the correct function when a struct device * is available. Also >>> > since it's a devm function we can drop the calls to clk_put(). While at >>> > it also fix a wrong debug message. >>> > >>> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >>> > --- >>> > drivers/tty/serial/8250/8250_mtk.c | 7 ++----- >>> > 1 file changed, 2 insertions(+), 5 deletions(-) >>> > >>> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c >>> > index 7a11fac..c6901dc 100644 >>> > --- a/drivers/tty/serial/8250/8250_mtk.c >>> > +++ b/drivers/tty/serial/8250/8250_mtk.c >>> > @@ -131,18 +131,16 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, >>> > struct mtk8250_data *data) >>> > { >>> > int err; >>> > - struct device_node *np = pdev->dev.of_node; >>> > >>> > - data->uart_clk = of_clk_get(np, 0); >>> > + data->uart_clk = devm_clk_get(&pdev->dev, NULL); >>> > if (IS_ERR(data->uart_clk)) { >>> > - dev_warn(&pdev->dev, "Can't get timer clock\n"); >>> > + dev_warn(&pdev->dev, "Can't get uart clock\n"); >>> > return PTR_ERR(data->uart_clk); >>> > } >>> > >>> > err = clk_prepare_enable(data->uart_clk); >>> > if (err) { >>> > dev_warn(&pdev->dev, "Can't prepare clock\n"); >>> > - clk_put(data->uart_clk); >>> > return err; >>> > } >>> > p->uartclk = clk_get_rate(data->uart_clk); >>> > @@ -216,7 +214,6 @@ static int mtk8250_remove(struct platform_device *pdev) >>> > serial8250_unregister_port(data->line); >>> > if (!IS_ERR(data->uart_clk)) { >>> > clk_disable_unprepare(data->uart_clk); >>> > - clk_put(data->uart_clk); >>> > } >>> >>> Please delete the braces. >> >> The if() is removed completly in the next patch anyway. > > You are right. I propose to merge patch 2 and 3, so you get a clean code. > On the way, can you have a look on the commit message? For me it is > not clear what you want to say. Actually I meant patch 1 and 2 of the series. Sorry for the confusion.
2015-04-23 10:51 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > The mtk 8250 needs two clocks, one for providing the baudrate and > one that needs to be enabled for register accesses. The latter has > not been supported, this patch adds support for it. It is optional > for now since not all SoCs provide a bus clock. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > Documentation/devicetree/bindings/serial/mtk-uart.txt | 9 +++++++-- > drivers/tty/serial/8250/8250_mtk.c | 14 +++++++++++++- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt > index 4415226..369b5c3 100644 > --- a/Documentation/devicetree/bindings/serial/mtk-uart.txt > +++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt > @@ -14,7 +14,11 @@ Required properties: > > - interrupts: A single interrupt specifier. > > -- clocks: Clock driving the hardware. > +- clocks : Must contain an entry for each entry in clock-names. > + See ../clocks/clock-bindings.txt for details. > +- clock-names: > + - "baud": The clock the baudrate is derived from > + - "bus": The bus clock for register accesses (optional) > > Example: > > @@ -22,5 +26,6 @@ Example: > compatible = "mediatek,mt6589-uart", "mediatek,mt6577-uart"; > reg = <0x11006000 0x400>; > interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_LOW>; > - clocks = <&uart_clk>; > + clocks = <&uart_clk>, <&bus_clk>; > + clock-names = "baud", "bus"; > }; > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > index 2f28bd0..20c27f0 100644 > --- a/drivers/tty/serial/8250/8250_mtk.c > +++ b/drivers/tty/serial/8250/8250_mtk.c > @@ -34,6 +34,7 @@ > struct mtk8250_data { > int line; > struct clk *uart_clk; > + struct clk *bus_clk; > }; > > static void > @@ -120,6 +121,7 @@ static int mtk8250_runtime_suspend(struct device *dev) > struct mtk8250_data *data = dev_get_drvdata(dev); > > clk_disable_unprepare(data->uart_clk); > + clk_disable_unprepare(data->bus_clk); > > return 0; > } > @@ -135,6 +137,12 @@ static int mtk8250_runtime_resume(struct device *dev) > return err; > } > > + err = clk_prepare_enable(data->bus_clk); > + if (err) { > + dev_warn(dev, "Can't enable bus clock\n"); > + return err; > + } > + > return 0; > } > > @@ -153,7 +161,7 @@ mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) > static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > struct mtk8250_data *data) > { > - data->uart_clk = devm_clk_get(&pdev->dev, NULL); > + data->uart_clk = devm_clk_get(&pdev->dev, "baud"); > if (IS_ERR(data->uart_clk)) { > dev_warn(&pdev->dev, "Can't get uart clock\n"); > return PTR_ERR(data->uart_clk); > @@ -161,6 +169,10 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > > p->uartclk = clk_get_rate(data->uart_clk); > > + data->bus_clk = devm_clk_get(&pdev->dev, "bus"); > + if (IS_ERR(data->bus_clk)) > + data->bus_clk = NULL; > + We have to take into account the old DTS definition. This means, that if devm_clk_get(&pdev->dev, "baud") fails, we have to look for devm_clk_get(&pdev->dev, NULL). Only in the case I when clock "baud" does exist, do we check for clock "bus". Does this make sense? The backwards compatibility should be reflected in the bindings documentation as well. Cheers, Matthias
On Fri, Apr 24, 2015 at 12:53:08PM +0200, Matthias Brugger wrote: > 2015-04-24 12:31 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > > On Fri, Apr 24, 2015 at 12:28:33PM +0200, Matthias Brugger wrote: > >> 2015-04-23 10:51 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>: > >> > Just because of_clk_get() doesn't mean it should be used. Use devm_clk_get > >> > which is the correct function when a struct device * is available. Also > >> > since it's a devm function we can drop the calls to clk_put(). While at > >> > it also fix a wrong debug message. > >> > > >> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > >> > --- > >> > drivers/tty/serial/8250/8250_mtk.c | 7 ++----- > >> > 1 file changed, 2 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > >> > index 7a11fac..c6901dc 100644 > >> > --- a/drivers/tty/serial/8250/8250_mtk.c > >> > +++ b/drivers/tty/serial/8250/8250_mtk.c > >> > @@ -131,18 +131,16 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p, > >> > struct mtk8250_data *data) > >> > { > >> > int err; > >> > - struct device_node *np = pdev->dev.of_node; > >> > > >> > - data->uart_clk = of_clk_get(np, 0); > >> > + data->uart_clk = devm_clk_get(&pdev->dev, NULL); > >> > if (IS_ERR(data->uart_clk)) { > >> > - dev_warn(&pdev->dev, "Can't get timer clock\n"); > >> > + dev_warn(&pdev->dev, "Can't get uart clock\n"); > >> > return PTR_ERR(data->uart_clk); > >> > } > >> > > >> > err = clk_prepare_enable(data->uart_clk); > >> > if (err) { > >> > dev_warn(&pdev->dev, "Can't prepare clock\n"); > >> > - clk_put(data->uart_clk); > >> > return err; > >> > } > >> > p->uartclk = clk_get_rate(data->uart_clk); > >> > @@ -216,7 +214,6 @@ static int mtk8250_remove(struct platform_device *pdev) > >> > serial8250_unregister_port(data->line); > >> > if (!IS_ERR(data->uart_clk)) { > >> > clk_disable_unprepare(data->uart_clk); > >> > - clk_put(data->uart_clk); > >> > } > >> > >> Please delete the braces. > > > > The if() is removed completly in the next patch anyway. > > You are right. I propose to merge patch 2 and 3, so you get a clean code. Patches 1 and 2 are different topics, I see no reason merging them. I changed the order of the two patches, maybe this makes it better readable. > On the way, can you have a look on the commit message? For me it is > not clear what you want to say. Changed to: When a struct device * is present clk_get should be used rather than of_clk_get. Use the devm variant of this function to be able to drop the clk_put in the error and remove pathes. While at it fix a wrong error message. Sascha
The following changes since commit 39a8804455fb23f09157341d3ba7db6d7ae6ee76: Linux 4.0 (2015-04-12 15:12:50 -0700) are available in the git repository at: git://git.pengutronix.de/git/sha/linux-2.6.git tags/v4.0-mtk-uart-clk for you to fetch changes up to 91bb93e93bf0916f9ebf571e8ab7cb671322ebc8: tty: serial: 8250_mtk: Add support for bus clock (2015-04-23 10:36:04 +0200) ---------------------------------------------------------------- This series adds support for the second clock on the Mediatek UART. This is necessary for SoCs needing a bus clock to be enabled before the UART can be accessed. ---------------------------------------------------------------- Sascha Hauer (4): tty: serial: 8250_mtk: Use devm_clk_get tty: serial: 8250_mtk: remove unnecessary test tty: serial: 8250_mtk: use pm_runtime callbacks for enabling tty: serial: 8250_mtk: Add support for bus clock .../devicetree/bindings/serial/mtk-uart.txt | 9 ++- drivers/tty/serial/8250/8250_mtk.c | 92 ++++++++++++---------- 2 files changed, 57 insertions(+), 44 deletions(-)