Hi Joachim, 2015-05-16 7:28 GMT+09:00 Joachim Eastwood <manabian@xxxxxxxxx>: >> + >> +#include <linux/init.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/clk.h> >> +#include <linux/of.h> >> +#include <linux/io.h> >> +#include <linux/serial_core.h> >> +#include <linux/serial_reg.h> >> +#include <linux/serial_8250.h> > > Please put the includes in alphabetic order. OK. > Do you really need init.h? Not really. Will remove it. >> +static int uniphier_of_serial_setup(struct platform_device *pdev, >> + struct uart_port *port, >> + struct uniphier8250_priv *priv) >> +{ >> + int ret; >> + u32 prop; >> + struct device_node *np = pdev->dev.of_node; >> + >> + ret = of_alias_get_id(np, "serial"); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to get alias id\n"); >> + return ret; >> + } >> + port->line = priv->line = ret; >> + >> + /* Get clk rate through clk driver */ >> + priv->clk = of_clk_get(np, 0); >> + if (IS_ERR(priv->clk)) { >> + dev_err(&pdev->dev, "failed to get clock\n"); >> + return PTR_ERR(priv->clk); >> + } > > Use devm_clk_get() if possible. Sure. > >> +static int uniphier_uart_remove(struct platform_device *pdev) >> +{ >> + struct uniphier8250_priv *priv = platform_get_drvdata(pdev); >> + >> + serial8250_unregister_port(priv->line); >> + if (!IS_ERR_OR_NULL(priv->clk)) { >> + clk_disable_unprepare(priv->clk); >> + clk_put(priv->clk); >> + } > > If you use devm_clk_get() in uniphier_of_serial_setup() you only need > to call clk_disable_unprepare() here. > > Calling clk_disable_unprepare() with NULL is allowed and you already > check for IS_ERR in your setup function. Yes. Thank you for your review! -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html