> static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS]; > +static struct nest_pmu *per_nest_pmu_arr[P8_NEST_MAX_PMUS]; > + > +static int nest_event_info(struct property *pp, char *name, > + struct nest_ima_events *p8_events, int string, u32 val) 'int string' is a bit confusing. 'bool is_string' might be clearer, but I think it would be even better still to have different functions for string and non-string cases, especially because you only need val in the non-string case.
That will also allow you to give the functions clearer names. I think the function is populating the event with info from the dt property (in the string case) or the val argument (non-string case) - maybe the names could reflect that somehow? > +{ > + char *buf; > + > + > +static int nest_pmu_create(struct device_node *dev, int pmu_index) > +{ > + struct nest_ima_events **p8_events_arr, *p8_events; > + struct nest_pmu *pmu_ptr; > + struct property *pp; > + char *buf, *start; > + const __be32 *lval; > + u32 val; > + int idx = 0, ret; > + > + if (!dev) > + return -EINVAL; > + > + /* memory for nest pmus */ > + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); > + if (!pmu_ptr) > + return -ENOMEM; > + > + /* Needed for hotplug/migration */ > + per_nest_pmu_arr[pmu_index] = pmu_ptr; > + > + /* memory for nest pmu events */ > + p8_events_arr = kzalloc((sizeof(struct nest_ima_events) * 64), > + GFP_KERNEL); > + if (!p8_events_arr) > + return -ENOMEM; > + p8_events = (struct nest_ima_events *)p8_events_arr; I'm still quite uncomfortable with this. - Why * 64? Should it be * P8_NEST_MAX_EVENTS_SUPPORTED? Or is it a different constant? - p8_events = p8_events_arr[0] would be clearer > + > + /* > + * Loop through each property > + */ > + for_each_property_of_node(dev, pp) { > + start = pp->name; > + > + if (!strcmp(pp->name, "name")) { > + if (!pp->value || > + (strnlen(pp->value, pp->length) == pp->length) || > + (pp->length > P8_NEST_MAX_PMU_NAME_LEN)) > + return -EINVAL; > + > + buf = kzalloc(P8_NEST_MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + /* Save the name to register it later */ > + sprintf(buf, "Nest_%s", (char *)pp->value); > + pmu_ptr->pmu.name = (char *)buf; > + continue; > + } > + > + /* Skip these, we dont need it */ "don't" instead of "dont". > + if (!strcmp(pp->name, "phandle") || > + !strcmp(pp->name, "device_type") || > + !strcmp(pp->name, "linux,phandle")) > + continue; > + > + if (strncmp(pp->name, "unit.", 5) == 0) { > + /* Skip first few chars in the name */ The whole comment is pretty uninformative, as is the similar comment below. If you need a comment at all, maybe something along the lines of "Strip the prefix because <reason we don't need/want the prefix>"? > + start += 5; > + ret = nest_event_info(pp, start, p8_events++, 1, 0); > + } else if (strncmp(pp->name, "scale.", 6) == 0) { > + /* Skip first few chars in the name */ > + start += 6; > + ret = nest_event_info(pp, start, p8_events++, 1, 0); > + } else { > + lval = of_get_property(dev, pp->name, NULL); > + val = (uint32_t)be32_to_cpup(lval); > + > + ret = nest_event_info(pp, start, p8_events++, 0, val); > + } > + > + if (ret) > + return ret; > + > + /* book keeping */ > + idx++; You don't seem to use idx in this function, apart from incrementing it here...? > + } > + > + return 0; > +} -- Regards, Daniel
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev