Re: [PATCH 07/16] ARM: mvebu: Make the CPU idle initialization more generic

Gregory CLEMENT
2014-07-03T08:54:20Z

Hi Thomas,

struct device_node *np;
>> + const struct of_device_id *match;
>> +
>> + np = of_find_matching_node_and_match(NULL, of_cpuidle_table,
>> + &match);
>
> I'm not sure I like using of_find_matching_node_and_match() on the
> entire tree to find the root node compatible string. Wouldn't it be
> simpler and shorter to just do:
>
> if (of_machine_is_compatible("marvell,armadaxp"))
> ret = armadaxp_cpuidle_init();
> else if (of_machine_is_compatible("marvell,armada370"))
> ret = armada370_cpuidle_init();
> else if (of_machine_is_compaitble("marvell,armada380"))
> ret = armada38x_cpuidle_init();
>
> if (ret)
> return ret;

Indeed, as I don't use any resource from the device tree here, using
of_find_matching_node_and_match is overkill. And your alternative
will be enough.

>
> Also, using a boolean to return a success/error status is not the
> kernel way of doing things, you should use an int instead and use
> proper error codes or return 0 on success.

OK

Thanks,

Gregory

--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.