On 31.07.2014 15:13, Humberto Naves wrote: > Hi, > > I am bit confused by your response: first you mentioned that I should > remove the NULL check for variable np, but later on you suggested that > I should rearrange the conditional statement to avoid adding more > indentation. That was just a side note. > My guess is that I should remove that if statement > altogether? Yes, that was my intention. > > Regarding the ctx variable, should I still remove the NULL check? As > you said, in the near future samsung_clk_init() won't panic anymore, > and keeping the check in place won't hurt anybody. The rule of thumb for kernel patches is that we want a patch if we know that it is something we need. We don't know yet when and how (which error returning convention, NULL or ERR_PTR() or maybe something else?) samsung_clk_init() gets changed, so right now we shouldn't change its callers. Of course a patch changing samsung_clk_init() and all its callers in one go will be welcome. By the way, please avoid top posting. Here's a good read on Linux mailing lists netiquette: http://www.tux.org/lkml/#s3 . Best regards, Tomasz > > Best, > Humberto > > On Thu, Jul 31, 2014 at 2:34 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: >> Hi Humberto, >> >> Please see my comments inline. >> >> On 31.07.2014 13:22, Humberto Silva Naves wrote: >>> Added NULL pointer checks for device_node input parameter and >>> for the samsung_clk_provider context returned by samsung_clk_init. >>> Even though the *current* samsung_clk_init function never returns >>> a NULL pointer, it is good to keep this check in place to avoid >>> possible problems in the future due to changes in implementation. >>> That way, we also improve the consistency of the code that performs >>> clock initialization across the different SoCs. >>> >>> Signed-off-by: Humberto Silva Naves <hsnaves@xxxxxxxxx> >>> --- >>> drivers/clk/samsung/clk-exynos5410.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c >>> index 231475b..bf57c80 100644 >>> --- a/drivers/clk/samsung/clk-exynos5410.c >>> +++ b/drivers/clk/samsung/clk-exynos5410.c >>> @@ -188,11 +188,17 @@ static void __init exynos5410_clk_init(struct device_node *np) >>> struct samsung_clk_provider *ctx; >>> void __iomem *reg_base; >>> >>> - reg_base = of_iomap(np, 0); >>> - if (!reg_base) >>> - panic("%s: failed to map registers\n", __func__); >>> + if (np) { >> >> Since all Exynos-based boards are always booted using DT, this function >> will never be called if there is no node for the clock controller and so >> there is no way this pointer can end up being NULL. I don't see a point >> in complicating this code with useless checks. >> >>> + reg_base = of_iomap(np, 0); >>> + if (!reg_base) >>> + panic("%s: failed to map registers\n", __func__); >>> + } else { >>> + panic("%s: unable to determine soc\n", __func__); >>> + } >> >> As a side note, since panic() does not return, the code above could be >> changed to follow rest of checks in this function: >> >> if (!np) >> panic("%s: unable to determine soc\n", __func__); >> >> reg_base = of_iomap(np, 0); >> ... >> >> leading to more readable code with less indentation and less changes to >> existing code. >> >>> >>> ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS); >>> + if (!ctx) >>> + panic("%s: unable to allocate context.\n", __func__); >> >> samsung_clk_init() already panics on any error, although now as I think >> of it, it probably should be changed with a patch to just error out and >> let the caller handle the error. However callers don't need to be >> changed before this is done. >> >> Best regards, >> Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
