On Tue, Jan 14, 2014 at 11:43:37AM +0000, Lorenzo Pieralisi wrote:
On Sun, Jan 12, 2014 at 07:20:39PM +0000, Mark Brown wrote:
+static void __init parse_core(struct device_node *core, int cluster_id,
int core_id)+{
- char name[10];
- bool leaf = true;
- int i, cpu;
- struct device_node *t;
- i = 0;
You could initialize i at declaration, I can understand why you are doing that explictly in parse_cluster (two loops, to make code clearer), but here it does not make much sense to add a line for that.
I still find it clearer for do { } while loops to have the start condition required for the loop to function right next to the loop. Yes, you can save a line code but that's about it.
- do {
snprintf(name, sizeof(name), "thread%d", i);
If we wanted to be very picky, you need to copy "thread" just once (same goes for other strings), but we'd better leave code as is IMHO.
That would just make the code more complex, we need to handle tens of cores so just doing i + '0' won't cut it.
t = of_get_child_by_name(core, name);
Should we check the MT bit in MPIDR_EL1 before validating threads as well ?
I do not like the idea because this means reliance on MPIDR_EL1 for MT and DT for topology bits, but it might be a worthwhile check.
It is certainly odd to have a DT with threads and an MPIDR_EL1 with the MT bit clear.
Checking seems counter to the idea of forcing everyone to provide this information from the firmware in the first place - checking that one bit and ignoring the rest of the information even if it's good would seem perverse.