Re: [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors

From: Guenter Roeck
Date: Tue May 02 2017 - 01:04:34 EST


On 05/01/2017 09:35 PM, Shilpasri G Bhat wrote:
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.bhat@xxxxxxxxxxxxxxxxxx>
---
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.


Not sure I am parsing that correctly. Does the firmware report highest/lowest
or min/max ? reset_history only makes sense if the chip supports highest/lowest.

In case you plan to implement highest/lowest in software ... please don't.
Highest/lowest attributes must only be provided if the history is delivered
by the chip. If the chip does not support highest/lowest values, user
space can implement it, but we definitely don't want any workers or
similar in the kernel to do it.

Thanks,
Guenter

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++;
}
}




--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html