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. My guess is that I should remove that if statement
altogether?
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.
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-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
