Hi Guenter, On 04/28/2017 06:59 PM, Guenter Roeck wrote: > On 04/27/2017 10:59 PM, Shilpasri G Bhat wrote: >> Add support for adding min/max values for the inband sensors copied by >> OCC to main memory. And also add current(mA) sensors to the list. >> >> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com> >> --- >> Changes from V1: >> - Add functions to get min and max attribute strings >> - Add function 'populate_sensor' to fill in the 'struct sensor_data' >> for each sensor. >> >> drivers/hwmon/ibmpowernv.c | 96 >> +++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 77 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c >> index 6d2e660..d59262c 100644 >> --- a/drivers/hwmon/ibmpowernv.c >> +++ b/drivers/hwmon/ibmpowernv.c >> @@ -50,6 +50,7 @@ enum sensors { >> TEMP, >> POWER_SUPPLY, >> POWER_INPUT, >> + CURRENT, >> MAX_SENSOR_TYPE, >> }; >> >> @@ -65,7 +66,8 @@ enum sensors { >> {"fan", "ibm,opal-sensor-cooling-fan"}, >> {"temp", "ibm,opal-sensor-amb-temp"}, >> {"in", "ibm,opal-sensor-power-supply"}, >> - {"power", "ibm,opal-sensor-power"} >> + {"power", "ibm,opal-sensor-power"}, >> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ > > Following up on a previous e-mail, this really _is_ odd. Any chance to fix it > in the firmware and have current sensors return "ibm,opal-sensor-current" ?
Okay will drop "curr' sensor till we resolve this in the firmware. > >> }; >> >> struct sensor_data { >> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device >> *pdev) >> opal = of_find_node_by_path("/ibm,opal/sensors"); >> for_each_child_of_node(opal, np) { >> const char *label; >> + int len; >> >> if (np->name == NULL) >> continue; >> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device >> *pdev) >> sensor_groups[type].attr_count++; >> >> /* >> - * add a new attribute for labels >> + * add attributes for labels, min and max >> */ >> if (!of_property_read_string(np, "label", &label)) >> sensor_groups[type].attr_count++; >> + if (of_find_property(np, "sensor-data-min", &len)) >> + sensor_groups[type].attr_count++; >> + if (of_find_property(np, "sensor-data-max", &len)) >> + sensor_groups[type].attr_count++; >> } >> >> of_node_put(opal); >> @@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata, >> const char *attr_name, >> sdata->dev_attr.show = show; >> } >> >> +static void populate_sensor(struct sensor_data *sdata, int od, int hd, int >> sid, >> + const char *attr_name, enum sensors type, >> + const struct attribute_group *pgroup, >> + ssize_t (*show)(struct device *dev, >> + struct device_attribute *attr, >> + char *buf)) >> +{ >> + sdata->id = sid; >> + sdata->type = type; >> + sdata->opal_index = od; >> + sdata->hwmon_index = hd; >> + create_hwmon_attr(sdata, attr_name, show); >> + pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr; >> +} >> + >> +static char *get_max_attr(enum sensors type) >> +{ >> + switch (type) { >> + case POWER_INPUT: >> + return "input_highest"; >> + case TEMP: >> + return "max"; >> + default: >> + break; >> + } >> + >> + return "highest"; > > This is a bit confusing. Why not 'return "highest";' in the default case > above ? > > Also, is this correct for type == POWER_SUPPLY, ie is it "highest" vs. "max" ? > > Kind of odd that the firmware reports "highest/lowest" in some cases > and "max/min" in others. Guess there is nothing we can do about that, > just a note. Firmware has min/max values for all sensors, but I had mapped them as highest/lowest to add reset_history attribute in the later patches. Will change "TEMP" to highest/lowest to keep consistency. Thanks and Regards, Shilpa > >> +} >> + >> +static char *get_min_attr(enum sensors type) >> +{ >> + switch (type) { >> + case POWER_INPUT: >> + return "input_lowest"; >> + case TEMP: >> + return "min"; >> + default: >> + break; >> + } >> + >> + return "lowest"; > > Same here. > >> +} >> + >> /* >> * Iterate through the device tree for each child of 'sensors' node, create >> * a sysfs attribute file, the file is named by translating the DT node name >> @@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device >> *pdev) >> for_each_child_of_node(opal, np) { >> const char *attr_name; >> u32 opal_index; >> + u32 hwmon_index; >> const char *label; >> >> if (np->name == NULL) >> @@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device >> *pdev) >> continue; >> } >> >> - sdata[count].id = sensor_id; >> - sdata[count].type = type; >> - >> /* >> * If we can not parse the node name, it means we are >> * running on a newer device tree. We can just forget >> @@ -401,14 +449,12 @@ static int create_device_attrs(struct platform_device >> *pdev) >> opal_index = INVALID_INDEX; >> } >> >> - sdata[count].opal_index = opal_index; >> - sdata[count].hwmon_index = >> - get_sensor_hwmon_index(&sdata[count], sdata, count); >> - >> - create_hwmon_attr(&sdata[count], attr_name, show_sensor); >> - >> - pgroups[type]->attrs[sensor_groups[type].attr_count++] = >> - &sdata[count++].dev_attr.attr; >> + hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata, >> + count); >> + populate_sensor(&sdata[count], opal_index, hwmon_index, >> + sensor_id, attr_name, type, pgroups[type], >> + show_sensor); >> + count++; >> >> if (!of_property_read_string(np, "label", &label)) { >> /* >> @@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device >> *pdev) >> * attribute. They are related to the same >> * sensor. >> */ >> - sdata[count].type = type; >> - sdata[count].opal_index = sdata[count - 1].opal_index; >> - sdata[count].hwmon_index = sdata[count - 1].hwmon_index; >> >> make_sensor_label(np, &sdata[count], label); >> + populate_sensor(&sdata[count], opal_index, hwmon_index, >> + sensor_id, "label", type, pgroups[type], >> + show_label); >> + count++; >> + } >> >> - create_hwmon_attr(&sdata[count], "label", show_label); >> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) { >> + attr_name = get_max_attr(type); >> + populate_sensor(&sdata[count], opal_index, hwmon_index, >> + sensor_id, attr_name, type, >> + pgroups[type], show_sensor); >> + count++; >> + } >> >> - pgroups[type]->attrs[sensor_groups[type].attr_count++] = >> - &sdata[count++].dev_attr.attr; >> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) { >> + attr_name = get_min_attr(type); >> + populate_sensor(&sdata[count], opal_index, hwmon_index, >> + sensor_id, attr_name, type, >> + pgroups[type], show_sensor); >> + count++; >> } >> } >> >> >