[RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cédric Le Goater]

Hello !

These patches rework the ibmpowernv driver to support the new device 
tree as proposed by this patchset on the skiboot mailing list :

  https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html

They are based on Linux 3.19 and were tested on IBM Power and Open Power 
systems running trusty.

The main issue is that the new device tree is incompatible with the 
previous ibmpowernv drivers. The consequence is no powernv sensors 
on systems with such a opal/linux configuration.

Cheers,

C.


Cédric Le Goater (3):
  powerpc/powernv: Check OPAL sensor calls exist
  powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read
  hwmon: (ibmpowernv) add DTS support

 arch/powerpc/platforms/powernv/opal-sensor.c |   32 ++-
 drivers/hwmon/ibmpowernv.c                   |  336 ++++++++++++++------------
 2 files changed, 202 insertions(+), 166 deletions(-)

-- 
1.7.10.4

[RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-sensor.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
index 4ab67ef7abc9..544292f2020f 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
 	struct platform_device *pdev;
 	struct device_node *sensor;
 
+	if (!opal_check_token(OPAL_SENSOR_READ))
+		return -ENODEV;
+
 	sensor = of_find_node_by_path("/ibm,opal/sensors");
 	if (!sensor) {
 		pr_err("Opal node 'sensors' not found\n");
-- 
1.7.10.4

[RFC PATCH 2/3] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cédric Le Goater]

Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back. However,
some sensors can be read synchronously (core temperatures for instance)
and don't need to wait for a response.

This patch modifies the opal call to accept an OPAL_SUCCESS return value
and not wait for a response if this is the case.

But, we still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync' call.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-sensor.c |   29 +++++++++++++++++---------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
index 544292f2020f..a4ed83a7dfb4 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -46,18 +46,27 @@ int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data)
 
 	mutex_lock(&opal_sensor_mutex);
 	ret = opal_sensor_read(sensor_hndl, token, &data);
-	if (ret != OPAL_ASYNC_COMPLETION)
-		goto out_token;
+	switch (ret) {
+	case OPAL_ASYNC_COMPLETION:
+		ret = opal_async_wait_response(token, &msg);
+		if (ret) {
+			pr_err("%s: Failed to wait for the async response, %d\n",
+			       __func__, ret);
+			goto out_token;
+		}
 
-	ret = opal_async_wait_response(token, &msg);
-	if (ret) {
-		pr_err("%s: Failed to wait for the async response, %d\n",
-				__func__, ret);
-		goto out_token;
-	}
+		ret = be64_to_cpu(msg.params[1]);
+
+		*sensor_data = be32_to_cpu(data);
+		break;
 
-	*sensor_data = be32_to_cpu(data);
-	ret = be64_to_cpu(msg.params[1]);
+	case OPAL_SUCCESS:
+		*sensor_data = be32_to_cpu(data);
+		break;
+
+	default:
+		break;
+	}
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);
-- 
1.7.10.4

[RFC PATCH 3/3] hwmon: (ibmpowernv) add DTS support

[Cédric Le Goater]

This patch reworks the ibmpowernv driver to support the new device tree
for sensors exposed by OPAL. It also adds new sensors : the core and
memory buffers DTS.

Hopefully, the proposed framework is good enough to easily add sensors
for other resources such as volts, planar temperatures, etc.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |  336 ++++++++++++++++++++++++--------------------
 1 file changed, 180 insertions(+), 156 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index febe8175d36c..9a6ee33f8219 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -32,247 +32,276 @@
 #include <linux/err.h>
 
 #define MAX_ATTR_LEN	32
-
-/* Sensor suffix name from DT */
-#define DT_FAULT_ATTR_SUFFIX		"faulted"
-#define DT_DATA_ATTR_SUFFIX		"data"
-#define DT_THRESHOLD_ATTR_SUFFIX	"thrs"
+#define MAX_LABEL_LEN	64
+#define MAX_ATTRS	3	/* sensor-data, sensor-status, label */
 
 /*
  * Enumerates all the types of sensors in the POWERNV platform and does index
- * into 'struct sensor_group'
+ * into 'struct sensor_type'
  */
 enum sensors {
 	FAN,
-	AMBIENT_TEMP,
+	TEMP,
 	POWER_SUPPLY,
 	POWER_INPUT,
 	MAX_SENSOR_TYPE,
 };
 
-static struct sensor_group {
+static struct sensor_type {
+	enum sensors type;
 	const char *name;
-	const char *compatible;
-	struct attribute_group group;
-	u32 attr_count;
-} sensor_groups[] = {
-	{"fan", "ibm,opal-sensor-cooling-fan"},
-	{"temp", "ibm,opal-sensor-amb-temp"},
-	{"in", "ibm,opal-sensor-power-supply"},
-	{"power", "ibm,opal-sensor-power"}
+	u32 count;
+} sensor_types[] = {
+	{ FAN,			"fan"	},
+	{ TEMP,			"temp"	},
+	{ POWER_SUPPLY,		"power"	},
+	{ POWER_INPUT,		"in"	},
+	{ MAX_SENSOR_TYPE,	NULL	}
 };
 
 struct sensor_data {
-	u32 id; /* An opaque id of the firmware for each sensor */
+	u32 data;
+	u32 status;
+	char label[MAX_LABEL_LEN];
 	enum sensors type;
-	char name[MAX_ATTR_LEN];
-	struct device_attribute dev_attr;
+	char attr_name[MAX_ATTRS][MAX_ATTR_LEN];
+	struct sensor_device_attribute sd_attrs[MAX_ATTRS];
 };
 
 struct platform_data {
-	const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
+	struct attribute_group attr_group;
+	const struct attribute_group *groups[2];
 	u32 sensors_count; /* Total count of sensors from each group */
+	struct attribute **attrs;
+	struct sensor_data **sensors;
 };
 
 static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
 			   char *buf)
 {
-	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
-						 dev_attr);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct sensor_data *sdata = pdata->sensors[attr->index];
 	ssize_t ret;
 	u32 x;
 
-	ret = opal_get_sensor_data(sdata->id, &x);
+	ret = opal_get_sensor_data(sdata->data, &x);
 	if (ret)
 		return ret;
 
 	/* Convert temperature to milli-degrees */
-	if (sdata->type == AMBIENT_TEMP)
+	if (sdata->type == TEMP)
 		x *= 1000;
 	/* Convert power to micro-watts */
-	else if (sdata->type == POWER_INPUT)
+	else if (sdata->type == POWER_SUPPLY)
 		x *= 1000000;
 
 	return sprintf(buf, "%u\n", x);
 }
 
-static int get_sensor_index_attr(const char *name, u32 *index,
-					char *attr)
+static ssize_t show_alarm(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
 {
-	char *hash_pos = strchr(name, '#');
-	char buf[8] = { 0 };
-	char *dash_pos;
-	u32 copy_len;
-	int err;
-
-	if (!hash_pos)
-		return -EINVAL;
-
-	dash_pos = strchr(hash_pos, '-');
-	if (!dash_pos)
-		return -EINVAL;
-
-	copy_len = dash_pos - hash_pos - 1;
-	if (copy_len >= sizeof(buf))
-		return -EINVAL;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct sensor_data *sdata = pdata->sensors[attr->index];
+	ssize_t ret;
+	u32 x;
 
-	strncpy(buf, hash_pos + 1, copy_len);
+	ret = opal_get_sensor_data(sdata->status, &x);
+	if (ret)
+		return ret;
 
-	err = kstrtou32(buf, 10, index);
-	if (err)
-		return err;
+	/*
+	 * Depending on the sensor type, the status bits can have
+	 * different meanings. Let's not be too subtil for the moment,
+	 * testing against 0x6 should raise an alarm.
+	 */
+	return sprintf(buf, "%u\n", x & 0x6);
+}
 
-	strncpy(attr, dash_pos + 1, MAX_ATTR_LEN);
+static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct sensor_data *sdata = pdata->sensors[attr->index];
 
-	return 0;
+	return sprintf(buf, "%s\n", sdata->label);
 }
 
-/*
- * This function translates the DT node name into the 'hwmon' attribute name.
- * IBMPOWERNV device node appear like cooling-fan#2-data, amb-temp#1-thrs etc.
- * which need to be mapped as fan2_input, temp1_max respectively before
- * populating them inside hwmon device class.
- */
-static int create_hwmon_attr_name(struct device *dev, enum sensors type,
-					 const char *node_name,
-					 char *hwmon_attr_name)
+static struct sensor_type *__init get_sensor_type(struct platform_device *pdev,
+	struct device_node *np)
 {
-	char attr_suffix[MAX_ATTR_LEN];
-	char *attr_name;
-	u32 index;
-	int err;
+	const char *type;
+	int i;
 
-	err = get_sensor_index_attr(node_name, &index, attr_suffix);
-	if (err) {
-		dev_err(dev, "Sensor device node name '%s' is invalid\n",
-			node_name);
-		return err;
-	}
+	if (np->name == NULL)
+		return NULL;
+
+	if (!of_device_is_compatible(np, "ibm,opal-sensor"))
+		return NULL;
 
-	if (!strcmp(attr_suffix, DT_FAULT_ATTR_SUFFIX)) {
-		attr_name = "fault";
-	} else if (!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX)) {
-		attr_name = "input";
-	} else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
-		if (type == AMBIENT_TEMP)
-			attr_name = "max";
-		else if (type == FAN)
-			attr_name = "min";
-		else
-			return -ENOENT;
-	} else {
-		return -ENOENT;
+	if (of_property_read_string(np, "sensor-type", &type)) {
+		dev_info(&pdev->dev,
+			 "'sensor-type' missing in the node '%s'\n",
+			 np->name);
+		return NULL;
 	}
 
-	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
-		 sensor_groups[type].name, index, attr_name);
-	return 0;
+	for (i = 0 ; i < MAX_SENSOR_TYPE; i++)
+		if (!strcmp(type, sensor_types[i].name))
+			return &sensor_types[i];
+
+	return NULL;
 }
 
-static int populate_attr_groups(struct platform_device *pdev)
+static void __init make_sensor_label(struct device_node *np,
+		    struct sensor_data *sdata, const char *label)
 {
-	struct platform_data *pdata = platform_get_drvdata(pdev);
-	const struct attribute_group **pgroups = pdata->attr_groups;
-	struct device_node *opal, *np;
-	enum sensors type;
+	u32 id;
+	size_t n;
 
-	opal = of_find_node_by_path("/ibm,opal/sensors");
-	for_each_child_of_node(opal, np) {
-		if (np->name == NULL)
-			continue;
+	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
 
-		for (type = 0; type < MAX_SENSOR_TYPE; type++)
-			if (of_device_is_compatible(np,
-					sensor_groups[type].compatible)) {
-				sensor_groups[type].attr_count++;
-				break;
-			}
-	}
+	/*
+	 * Core temp pretty pretty
+	 */
+	if (!of_property_read_u32(np, "ibm,pir", &id)) {
+		int i;
 
-	of_node_put(opal);
+		for_each_possible_cpu(i)
+			if (paca[i].hw_cpu_id == id)
+				break;
 
-	for (type = 0; type < MAX_SENSOR_TYPE; type++) {
-		sensor_groups[type].group.attrs = devm_kzalloc(&pdev->dev,
-					sizeof(struct attribute *) *
-					(sensor_groups[type].attr_count + 1),
-					GFP_KERNEL);
-		if (!sensor_groups[type].group.attrs)
-			return -ENOMEM;
-
-		pgroups[type] = &sensor_groups[type].group;
-		pdata->sensors_count += sensor_groups[type].attr_count;
-		sensor_groups[type].attr_count = 0;
+		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
+			      " %d-%d", i, i+7);
 	}
 
-	return 0;
+	/*
+	 * Membuffer pretty print
+	 */
+	if (!of_property_read_u32(np, "ibm,chip-id", &id))
+		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
+			      " %d", id & 0xffff);
 }
 
-/*
- * 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
- * to the name required by the higher 'hwmon' driver like fan1_input, temp1_max
- * etc..
- */
-static int create_device_attrs(struct platform_device *pdev)
+static int __init populate_attr_groups(struct platform_device *pdev)
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
-	const struct attribute_group **pgroups = pdata->attr_groups;
 	struct device_node *opal, *np;
-	struct sensor_data *sdata;
-	u32 sensor_id;
-	enum sensors type;
-	u32 count = 0;
 	int err = 0;
+	int i = 0;
 
 	opal = of_find_node_by_path("/ibm,opal/sensors");
-	sdata = devm_kzalloc(&pdev->dev, pdata->sensors_count * sizeof(*sdata),
-			     GFP_KERNEL);
-	if (!sdata) {
+	if (!opal) {
+		dev_dbg(&pdev->dev, "Opal node 'sensors' not found\n");
+		return -ENODEV;
+	}
+
+	for_each_child_of_node(opal, np) {
+		if (of_device_is_compatible(np, "ibm,opal-sensor"))
+			pdata->sensors_count++;
+	}
+
+	pdata->attrs = devm_kzalloc(&pdev->dev,
+	    sizeof(*pdata->attrs) * pdata->sensors_count * MAX_ATTRS + 1,
+	    GFP_KERNEL);
+	if (!pdata->attrs) {
+		err = -ENOMEM;
+		goto exit_put_node;
+	}
+	pdata->sensors = devm_kzalloc(&pdev->dev,
+	    sizeof(*pdata->sensors) * pdata->sensors_count * MAX_ATTRS + 1,
+	    GFP_KERNEL);
+	if (!pdata->sensors) {
 		err = -ENOMEM;
 		goto exit_put_node;
 	}
 
-	for_each_child_of_node(opal, np) {
-		if (np->name == NULL)
-			continue;
 
-		for (type = 0; type < MAX_SENSOR_TYPE; type++)
-			if (of_device_is_compatible(np,
-					sensor_groups[type].compatible))
-				break;
+	for_each_child_of_node(opal, np) {
+		struct sensor_data *sdata;
+		struct sensor_type *stype;
+		u32 sensor_data;
+		const char *label;
 
-		if (type == MAX_SENSOR_TYPE)
+		stype = get_sensor_type(pdev, np);
+		if (!stype)
 			continue;
 
-		if (of_property_read_u32(np, "sensor-id", &sensor_id)) {
+		if (of_property_read_u32(np, "sensor-data", &sensor_data)) {
 			dev_info(&pdev->dev,
-				 "'sensor-id' missing in the node '%s'\n",
+				 "'sensor-data' missing in the node '%s'\n",
 				 np->name);
 			continue;
 		}
 
-		sdata[count].id = sensor_id;
-		sdata[count].type = type;
-		err = create_hwmon_attr_name(&pdev->dev, type, np->name,
-					     sdata[count].name);
-		if (err)
+		sdata = devm_kzalloc(&pdev->dev, sizeof(*sdata), GFP_KERNEL);
+		if (sdata == NULL) {
+			err = -ENOMEM;
 			goto exit_put_node;
+		}
+
+		stype->count++;
+		sdata->data = sensor_data;
+		sdata->type = stype->type;
+
+		snprintf(sdata->attr_name[0], MAX_ATTR_LEN, "%s%d_input",
+			 stype->name, stype->count);
+
+		sysfs_attr_init(&sdata->sd_attrs[0].dev_attr.attr);
+		sdata->sd_attrs[0].dev_attr.attr.name = sdata->attr_name[0];
+		sdata->sd_attrs[0].dev_attr.attr.mode = S_IRUGO;
+		sdata->sd_attrs[0].dev_attr.show = show_sensor;
+		sdata->sd_attrs[0].index = i;
+
+		pdata->attrs[i] = &sdata->sd_attrs[0].dev_attr.attr;
+		pdata->sensors[i++] = sdata;
+
+		if (!of_property_read_u32(np, "sensor-status",
+					  &sdata->status)) {
+			snprintf(sdata->attr_name[1], MAX_ATTR_LEN,
+				 "%s%d_alarm", stype->name, stype->count);
+
+			sysfs_attr_init(&sdata->sd_attrs[1].dev_attr.attr);
+			sdata->sd_attrs[1].dev_attr.attr.name =
+				sdata->attr_name[1];
+			sdata->sd_attrs[1].dev_attr.attr.mode = S_IRUGO;
+			sdata->sd_attrs[1].dev_attr.show = show_alarm;
+			sdata->sd_attrs[1].index = i;
+
+			pdata->attrs[i] = &sdata->sd_attrs[1].dev_attr.attr;
+			pdata->sensors[i++] = sdata;
+		}
+
+		if (!of_property_read_string(np, "label", &label)) {
+			snprintf(sdata->attr_name[2], MAX_ATTR_LEN,
+				 "%s%d_label", stype->name, stype->count);
 
-		sysfs_attr_init(&sdata[count].dev_attr.attr);
-		sdata[count].dev_attr.attr.name = sdata[count].name;
-		sdata[count].dev_attr.attr.mode = S_IRUGO;
-		sdata[count].dev_attr.show = show_sensor;
+			make_sensor_label(np, sdata, label);
 
-		pgroups[type]->attrs[sensor_groups[type].attr_count++] =
-				&sdata[count++].dev_attr.attr;
+			sysfs_attr_init(&sdata->sd_attrs[2].dev_attr.attr);
+			sdata->sd_attrs[2].dev_attr.attr.name =
+				sdata->attr_name[2];
+			sdata->sd_attrs[2].dev_attr.attr.mode = S_IRUGO;
+			sdata->sd_attrs[2].dev_attr.show = show_label;
+			sdata->sd_attrs[2].index = i;
+
+			pdata->attrs[i] = &sdata->sd_attrs[2].dev_attr.attr;
+			pdata->sensors[i++] = sdata;
+		}
 	}
 
+	pdata->attr_group.attrs = pdata->attrs;
+	pdata->groups[0] = &pdata->attr_group;
+
 exit_put_node:
 	of_node_put(opal);
 	return err;
 }
 
-static int ibmpowernv_probe(struct platform_device *pdev)
+static int __init ibmpowernv_probe(struct platform_device *pdev)
 {
 	struct platform_data *pdata;
 	struct device *hwmon_dev;
@@ -288,15 +317,10 @@ static int ibmpowernv_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	/* Create sysfs attribute data for each sensor found in the DT */
-	err = create_device_attrs(pdev);
-	if (err)
-		return err;
-
 	/* Finally, register with hwmon */
 	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev, DRVNAME,
 							   pdata,
-							   pdata->attr_groups);
+							   pdata->groups);
 
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
-- 
1.7.10.4

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Guenter Roeck]

On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
> Hello !
> 
> These patches rework the ibmpowernv driver to support the new device 
> tree as proposed by this patchset on the skiboot mailing list :
> 
>   https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html
> 
> They are based on Linux 3.19 and were tested on IBM Power and Open Power 
> systems running trusty.
> 
> The main issue is that the new device tree is incompatible with the 
> previous ibmpowernv drivers. The consequence is no powernv sensors 
> on systems with such a opal/linux configuration.
> 
I don't think that would be acceptable. There must be lots of such
systems out there. Why does it have to be incompatible ?
Can't it support both the old and new versions ?

Guenter

Re: [RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Guenter Roeck]

On Fri, Feb 20, 2015 at 04:07:35PM +0100, Cédric Le Goater wrote:

You should explain here why this patch is needed.

> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/opal-sensor.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
> index 4ab67ef7abc9..544292f2020f 100644
> --- a/arch/powerpc/platforms/powernv/opal-sensor.c
> +++ b/arch/powerpc/platforms/powernv/opal-sensor.c
> @@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
>  	struct platform_device *pdev;
>  	struct device_node *sensor;
>  
> +	if (!opal_check_token(OPAL_SENSOR_READ))
> +		return -ENODEV;
> +
>  	sensor = of_find_node_by_path("/ibm,opal/sensors");
>  	if (!sensor) {
>  		pr_err("Opal node 'sensors' not found\n");
> -- 
> 1.7.10.4
> 

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 02/20/2015 05:52 PM, Guenter Roeck wrote:
> On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
>> Hello !
>>
>> These patches rework the ibmpowernv driver to support the new device 
>> tree as proposed by this patchset on the skiboot mailing list :
>>
>>   https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html
>>
>> They are based on Linux 3.19 and were tested on IBM Power and Open Power 
>> systems running trusty.
>>
>> The main issue is that the new device tree is incompatible with the 
>> previous ibmpowernv drivers. The consequence is no powernv sensors 
>> on systems with such a opal/linux configuration.
>>
> I don't think that would be acceptable. There must be lots of such
> systems out there. Why does it have to be incompatible ?
> Can't it support both the old and new versions ?

I should have provided more explanation in the Linux patchset. Sorry 
for that. Here is the rationale behind this brutal code change.

The initial ibmpowernv driver was designed in the early days of the
powernv platform and the device tree it is using to expose the sensors 
has some limitations that makes it difficult to add new ones. The current 
layout of the device tree is also tightly coupled to IBM Power systems 
and their service processor (FSP). Open Power systems are different and 
need a different solution.

It is to get more sensors out the P8 (and there are quite a few) that 
the OPAL patchset [1] proposes a new device tree. On the Linux side, it 
feels simpler to make a jump forward and break the compatibility than 
to maintain multiple branches of code just to keep alive an early v1 
of the ibmpowernv driver.    

Open Power systems won't be impacted as ibmpowernv does not support them.


Cheers,

C.


[1] https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html

Re: [RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Cedric Le Goater]

On 02/20/2015 05:53 PM, Guenter Roeck wrote:
> On Fri, Feb 20, 2015 at 04:07:35PM +0100, Cédric Le Goater wrote:
> 
> You should explain here why this patch is needed.

Yes. What it does is to check that the firmware exposes the service
this driver is using (OPAL_SENSOR_READ). I will fix it.

Thanks,

C. 


> 
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>  arch/powerpc/platforms/powernv/opal-sensor.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
>> index 4ab67ef7abc9..544292f2020f 100644
>> --- a/arch/powerpc/platforms/powernv/opal-sensor.c
>> +++ b/arch/powerpc/platforms/powernv/opal-sensor.c
>> @@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
>>  	struct platform_device *pdev;
>>  	struct device_node *sensor;
>>  
>> +	if (!opal_check_token(OPAL_SENSOR_READ))
>> +		return -ENODEV;
>> +
>>  	sensor = of_find_node_by_path("/ibm,opal/sensors");
>>  	if (!sensor) {
>>  		pr_err("Opal node 'sensors' not found\n");
>> -- 
>> 1.7.10.4
>>
> 

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Guenter Roeck]

On 02/20/2015 12:15 PM, Cedric Le Goater wrote:
> On 02/20/2015 05:52 PM, Guenter Roeck wrote:
>> On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
>>> Hello !
>>>
>>> These patches rework the ibmpowernv driver to support the new device
>>> tree as proposed by this patchset on the skiboot mailing list :
>>>
>>>    https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html
>>>
>>> They are based on Linux 3.19 and were tested on IBM Power and Open Power
>>> systems running trusty.
>>>
>>> The main issue is that the new device tree is incompatible with the
>>> previous ibmpowernv drivers. The consequence is no powernv sensors
>>> on systems with such a opal/linux configuration.
>>>
>> I don't think that would be acceptable. There must be lots of such
>> systems out there. Why does it have to be incompatible ?
>> Can't it support both the old and new versions ?
>
> I should have provided more explanation in the Linux patchset. Sorry
> for that. Here is the rationale behind this brutal code change.
>
> The initial ibmpowernv driver was designed in the early days of the
> powernv platform and the device tree it is using to expose the sensors
> has some limitations that makes it difficult to add new ones. The current
> layout of the device tree is also tightly coupled to IBM Power systems
> and their service processor (FSP). Open Power systems are different and
> need a different solution.
>
> It is to get more sensors out the P8 (and there are quite a few) that
> the OPAL patchset [1] proposes a new device tree. On the Linux side, it
> feels simpler to make a jump forward and break the compatibility than
> to maintain multiple branches of code just to keep alive an early v1
> of the ibmpowernv driver.
>

Would it possibly be appropriate to write a different driver for the new
device tree ?

Guenter

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 02/21/2015 12:52 AM, Guenter Roeck wrote:
> On 02/20/2015 12:15 PM, Cedric Le Goater wrote:
>> On 02/20/2015 05:52 PM, Guenter Roeck wrote:
>>> On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
>>>> Hello !
>>>>
>>>> These patches rework the ibmpowernv driver to support the new device
>>>> tree as proposed by this patchset on the skiboot mailing list :
>>>>
>>>>    https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html
>>>>
>>>> They are based on Linux 3.19 and were tested on IBM Power and Open Power
>>>> systems running trusty.
>>>>
>>>> The main issue is that the new device tree is incompatible with the
>>>> previous ibmpowernv drivers. The consequence is no powernv sensors
>>>> on systems with such a opal/linux configuration.
>>>>
>>> I don't think that would be acceptable. There must be lots of such
>>> systems out there. Why does it have to be incompatible ?
>>> Can't it support both the old and new versions ?
>>
>> I should have provided more explanation in the Linux patchset. Sorry
>> for that. Here is the rationale behind this brutal code change.
>>
>> The initial ibmpowernv driver was designed in the early days of the
>> powernv platform and the device tree it is using to expose the sensors
>> has some limitations that makes it difficult to add new ones. The current
>> layout of the device tree is also tightly coupled to IBM Power systems
>> and their service processor (FSP). Open Power systems are different and
>> need a different solution.
>>
>> It is to get more sensors out the P8 (and there are quite a few) that
>> the OPAL patchset [1] proposes a new device tree. On the Linux side, it
>> feels simpler to make a jump forward and break the compatibility than
>> to maintain multiple branches of code just to keep alive an early v1
>> of the ibmpowernv driver.
>>
> 
> Would it possibly be appropriate to write a different driver for the new
> device tree ?

Sure. That is an option. 

There are no conflicts between the trees so we can live with two drivers 
using the same sensors/ root node. With time we will deprecate the initial
one.

Is that the preferred option ? How would we name the new driver ? 

	1. powernv
	2. powernv-hwmon
	3. openpowernv
	4. ibmpowernv2 

Thanks,

C.

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Guenter Roeck]

On 02/20/2015 11:14 PM, Cedric Le Goater wrote:
> On 02/21/2015 12:52 AM, Guenter Roeck wrote:
>> On 02/20/2015 12:15 PM, Cedric Le Goater wrote:
>>> On 02/20/2015 05:52 PM, Guenter Roeck wrote:
>>>> On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
>>>>> Hello !
>>>>>
>>>>> These patches rework the ibmpowernv driver to support the new device
>>>>> tree as proposed by this patchset on the skiboot mailing list :
>>>>>
>>>>>     https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html
>>>>>
>>>>> They are based on Linux 3.19 and were tested on IBM Power and Open Power
>>>>> systems running trusty.
>>>>>
>>>>> The main issue is that the new device tree is incompatible with the
>>>>> previous ibmpowernv drivers. The consequence is no powernv sensors
>>>>> on systems with such a opal/linux configuration.
>>>>>
>>>> I don't think that would be acceptable. There must be lots of such
>>>> systems out there. Why does it have to be incompatible ?
>>>> Can't it support both the old and new versions ?
>>>
>>> I should have provided more explanation in the Linux patchset. Sorry
>>> for that. Here is the rationale behind this brutal code change.
>>>
>>> The initial ibmpowernv driver was designed in the early days of the
>>> powernv platform and the device tree it is using to expose the sensors
>>> has some limitations that makes it difficult to add new ones. The current
>>> layout of the device tree is also tightly coupled to IBM Power systems
>>> and their service processor (FSP). Open Power systems are different and
>>> need a different solution.
>>>
>>> It is to get more sensors out the P8 (and there are quite a few) that
>>> the OPAL patchset [1] proposes a new device tree. On the Linux side, it
>>> feels simpler to make a jump forward and break the compatibility than
>>> to maintain multiple branches of code just to keep alive an early v1
>>> of the ibmpowernv driver.
>>>
>>
>> Would it possibly be appropriate to write a different driver for the new
>> device tree ?
>
> Sure. That is an option.
>
> There are no conflicts between the trees so we can live with two drivers
> using the same sensors/ root node. With time we will deprecate the initial
> one.
>
You lost me a bit. Are you saying you are going to replace the devicetree
properties with something that is incompatible but retain the same
compatible properties ? If so, how do you expect this to work ?
How do you expect to be able to determine which version of devicetree
is loaded, and thus which driver is needed ?

I'll have to understand this way better. The link above doesn't explain
what the differences are, nor how the driver is supposed to know what
to do.

Guenter

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 02/21/2015 12:03 PM, Guenter Roeck wrote:
> On 02/20/2015 11:14 PM, Cedric Le Goater wrote:
>> On 02/21/2015 12:52 AM, Guenter Roeck wrote:
>>> On 02/20/2015 12:15 PM, Cedric Le Goater wrote:
>>>> On 02/20/2015 05:52 PM, Guenter Roeck wrote:
>>>>> On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
>>>>>> Hello !
>>>>>>
>>>>>> These patches rework the ibmpowernv driver to support the new device
>>>>>> tree as proposed by this patchset on the skiboot mailing list :
>>>>>>
>>>>>>     https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html
>>>>>>
>>>>>> They are based on Linux 3.19 and were tested on IBM Power and Open Power
>>>>>> systems running trusty.
>>>>>>
>>>>>> The main issue is that the new device tree is incompatible with the
>>>>>> previous ibmpowernv drivers. The consequence is no powernv sensors
>>>>>> on systems with such a opal/linux configuration.
>>>>>>
>>>>> I don't think that would be acceptable. There must be lots of such
>>>>> systems out there. Why does it have to be incompatible ?
>>>>> Can't it support both the old and new versions ?
>>>>
>>>> I should have provided more explanation in the Linux patchset. Sorry
>>>> for that. Here is the rationale behind this brutal code change.
>>>>
>>>> The initial ibmpowernv driver was designed in the early days of the
>>>> powernv platform and the device tree it is using to expose the sensors
>>>> has some limitations that makes it difficult to add new ones. The current
>>>> layout of the device tree is also tightly coupled to IBM Power systems
>>>> and their service processor (FSP). Open Power systems are different and
>>>> need a different solution.
>>>>
>>>> It is to get more sensors out the P8 (and there are quite a few) that
>>>> the OPAL patchset [1] proposes a new device tree. On the Linux side, it
>>>> feels simpler to make a jump forward and break the compatibility than
>>>> to maintain multiple branches of code just to keep alive an early v1
>>>> of the ibmpowernv driver.
>>>>
>>>
>>> Would it possibly be appropriate to write a different driver for the new
>>> device tree ?
>>
>> Sure. That is an option.
>>
>> There are no conflicts between the trees so we can live with two drivers
>> using the same sensors/ root node. With time we will deprecate the initial
>> one.
>>
> You lost me a bit. Are you saying you are going to replace the devicetree
> properties with something that is incompatible but retain the same
> compatible properties ? If so, how do you expect this to work ?
> How do you expect to be able to determine which version of devicetree
> is loaded, and thus which driver is needed ?
>
> I'll have to understand this way better. The link above doesn't explain
> what the differences are, nor how the driver is supposed to know what
> to do.
 
Sure. My bad. I did not provide enough information in this RFC. Thanks for
your patience ! 


The current hwmon driver ibmpowernv relies on a device tree provided by 
the OPAL firmware. Today, this tree has the following layout :

	ibm,opal/sensors/
	|-- amb-temp#1-data
	|   |-- compatible
	|   |-- linux,phandle
	|   |-- name
	|   |-- phandle
	|   `-- sensor-id
	|-- amb-temp#1-thrs
	|   |-- compatible
	|   |-- linux,phandle
	|   |-- name
	|   |-- phandle
	|   `-- sensor-id
	|-- cooling-fan#10-data
	|   |-- compatible
	|   |-- linux,phandle
	|   |-- name
	|   |-- phandle
	|   `-- sensor-id
	|-- cooling-fan#10-faulted
	|   |-- compatible
	|   |-- linux,phandle
	|   |-- name
	|   |-- phandle
	|   `-- sensor-id
	|-- cooling-fan#10-thrs
	|   |-- compatible
	|   |-- linux,phandle
	|   |-- name
	|   |-- phandle
	|   `-- sensor-id
	...

It has some limitations and we want to change it to something more flexible, 
giving us the ability to add new sensors. The new device tree layout is 
described here :

	https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html


Both layouts use the same root node "ibm,opal/sensors" but the underlying
nodes have nothing in common, which means that the current driver ibmpowernv
will not 'see' any sensors on a system with an OPAL firmware exposing the new 
device tree. One will need new code for it. 

We have a few options to add support for this new tree :

 1. modify the current driver to parse the two trees. It seems overly complex 
    and the resulting code will be a pain to maintain. 

 2. add a new driver. As the two drivers can cohabitate, it is a viable solution.
    We will let the old driver rot in its corner and deprecate it one day. Kind
    of ugly to keep this code around.

 3. replace the current driver with a new one. The simplest and most brutal but 
    it also means we loose sensor support for IBM P8 systems running the old OPAL 
    firmware. I don't think it is a problem as these systems are bound to update 
    their firmware due to the amount of development currently being done on the 
    OPAL side.

    Open Power systems are not impacted.


Is that clearer ? and I would go for option 3.

Thanks,

C.
   






C. 

Re: [RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Michael Ellerman]

On Fri, 2015-02-20 at 16:07 +0100, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/opal-sensor.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
> index 4ab67ef7abc9..544292f2020f 100644
> --- a/arch/powerpc/platforms/powernv/opal-sensor.c
> +++ b/arch/powerpc/platforms/powernv/opal-sensor.c
> @@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
>  	struct platform_device *pdev;
>  	struct device_node *sensor;
>  
> +	if (!opal_check_token(OPAL_SENSOR_READ))
> +		return -ENODEV;
> +
>  	sensor = of_find_node_by_path("/ibm,opal/sensors");
>  	if (!sensor) {
>  		pr_err("Opal node 'sensors' not found\n");

Are you actually seeing this in practice?

It's a bit annoying that we have to check for the token, and then also check
the device tree. It would be nice if one implied the presence of the other.

cheers

Re: [RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Cedric Le Goater]

On 02/24/2015 05:54 AM, Michael Ellerman wrote:
> On Fri, 2015-02-20 at 16:07 +0100, Cédric Le Goater wrote:
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>  arch/powerpc/platforms/powernv/opal-sensor.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
>> index 4ab67ef7abc9..544292f2020f 100644
>> --- a/arch/powerpc/platforms/powernv/opal-sensor.c
>> +++ b/arch/powerpc/platforms/powernv/opal-sensor.c
>> @@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
>>  	struct platform_device *pdev;
>>  	struct device_node *sensor;
>>  
>> +	if (!opal_check_token(OPAL_SENSOR_READ))
>> +		return -ENODEV;
>> +
>>  	sensor = of_find_node_by_path("/ibm,opal/sensors");
>>  	if (!sensor) {
>>  		pr_err("Opal node 'sensors' not found\n");
> 
> Are you actually seeing this in practice?

No. Not this one. I have seen others though. I will send you patches.

> It's a bit annoying that we have to check for the token, and then also check
> the device tree. It would be nice if one implied the presence of the other.

Should we expose the OPAL call token in the device tree ? 

Cheers,

C. 
 

[PATCH 0/5] hwmon: (ibmpowernv) remove dependency on OPAL index

[Cédric Le Goater]

Hello !

The current implementation of the driver uses an index for the hwmon 
attribute which is extracted from the device node name. This index
is calculated by the OPAL firmware and its usage creates a dependency 
with the driver which makes changes a little more complex in OPAL.

This patchset changes the ibmpowernv code to use its own index. It 
starts with a few cleanups, mostly code shuffling around the creation 
of the hwmon sysfs attributes and completes by removing the dependency.

It also prepares ground for future OPAL changes :  

   https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html

which will be addressed in a other small patchset.


The patches are based on Linux 4.0.0-rc4 and were tested on IBM Power 
and Open Power systems running Trusty. 

Cheers,

C.


Cédric Le Goater (5):
  hwmon: (ibmpowernv) replace AMBIENT_TEMP by TEMP
  hwmon: (ibmpowernv) add a get_sensor_type() routine
  hwmon: (ibmpowernv) add a convert_opal_attr_name() routine
  hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype
  hwmon: (ibmpowernv) do not use the OPAL index for hwmon attribute
    names

 drivers/hwmon/ibmpowernv.c |  122 +++++++++++++++++++++++++++++---------------
 1 file changed, 81 insertions(+), 41 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] hwmon: (ibmpowernv) replace AMBIENT_TEMP by TEMP

[Cédric Le Goater]

Ambient is too restrictive as there can be other temperature channels :
core, memory, etc.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index febe8175d36c..f691e18df16b 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -44,7 +44,7 @@
  */
 enum sensors {
 	FAN,
-	AMBIENT_TEMP,
+	TEMP,
 	POWER_SUPPLY,
 	POWER_INPUT,
 	MAX_SENSOR_TYPE,
@@ -87,7 +87,7 @@ static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
 		return ret;
 
 	/* Convert temperature to milli-degrees */
-	if (sdata->type == AMBIENT_TEMP)
+	if (sdata->type == TEMP)
 		x *= 1000;
 	/* Convert power to micro-watts */
 	else if (sdata->type == POWER_INPUT)
@@ -154,7 +154,7 @@ static int create_hwmon_attr_name(struct device *dev, enum sensors type,
 	} else if (!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX)) {
 		attr_name = "input";
 	} else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
-		if (type == AMBIENT_TEMP)
+		if (type == TEMP)
 			attr_name = "max";
 		else if (type == FAN)
 			attr_name = "min";
-- 
1.7.10.4

[PATCH 2/5] hwmon: (ibmpowernv) add a get_sensor_type() routine

[Cédric Le Goater]

It will help in adding different compatible properties, coming from a
new device tree layout for example.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index f691e18df16b..07a8219b7f4e 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -169,6 +169,17 @@ static int create_hwmon_attr_name(struct device *dev, enum sensors type,
 	return 0;
 }
 
+static int get_sensor_type(struct device_node *np)
+{
+	enum sensors type;
+
+	for (type = 0; type < MAX_SENSOR_TYPE; type++) {
+		if (of_device_is_compatible(np, sensor_groups[type].compatible))
+			return type;
+	}
+	return MAX_SENSOR_TYPE;
+}
+
 static int populate_attr_groups(struct platform_device *pdev)
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
@@ -181,12 +192,9 @@ static int populate_attr_groups(struct platform_device *pdev)
 		if (np->name == NULL)
 			continue;
 
-		for (type = 0; type < MAX_SENSOR_TYPE; type++)
-			if (of_device_is_compatible(np,
-					sensor_groups[type].compatible)) {
-				sensor_groups[type].attr_count++;
-				break;
-			}
+		type = get_sensor_type(np);
+		if (type != MAX_SENSOR_TYPE)
+			sensor_groups[type].attr_count++;
 	}
 
 	of_node_put(opal);
@@ -236,11 +244,7 @@ static int create_device_attrs(struct platform_device *pdev)
 		if (np->name == NULL)
 			continue;
 
-		for (type = 0; type < MAX_SENSOR_TYPE; type++)
-			if (of_device_is_compatible(np,
-					sensor_groups[type].compatible))
-				break;
-
+		type = get_sensor_type(np);
 		if (type == MAX_SENSOR_TYPE)
 			continue;
 
-- 
1.7.10.4

[PATCH 3/5] hwmon: (ibmpowernv) add a convert_opal_attr_name() routine

[Cédric Le Goater]

It simplifies the create_hwmon_attr_name() routine and it clearly isolates
the conversion done between the OPAL node names and hwmon attributes names.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index 07a8219b7f4e..edcc4ffa5ad0 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -127,6 +127,28 @@ static int get_sensor_index_attr(const char *name, u32 *index,
 	return 0;
 }
 
+static const char *convert_opal_attr_name(enum sensors type,
+					const char *opal_attr)
+{
+	const char *attr_name = NULL;
+
+	if (!strcmp(opal_attr, DT_FAULT_ATTR_SUFFIX)) {
+		attr_name = "fault";
+	} else if (!strcmp(opal_attr, DT_DATA_ATTR_SUFFIX)) {
+		attr_name = "input";
+	} else if (!strcmp(opal_attr, DT_THRESHOLD_ATTR_SUFFIX)) {
+		if (type == TEMP)
+			attr_name = "max";
+		else if (type == FAN)
+			attr_name = "min";
+		else
+			return NULL;
+	} else {
+		return NULL;
+	}
+	return attr_name;
+}
+
 /*
  * This function translates the DT node name into the 'hwmon' attribute name.
  * IBMPOWERNV device node appear like cooling-fan#2-data, amb-temp#1-thrs etc.
@@ -138,7 +160,7 @@ static int create_hwmon_attr_name(struct device *dev, enum sensors type,
 					 char *hwmon_attr_name)
 {
 	char attr_suffix[MAX_ATTR_LEN];
-	char *attr_name;
+	const char *attr_name;
 	u32 index;
 	int err;
 
@@ -149,20 +171,9 @@ static int create_hwmon_attr_name(struct device *dev, enum sensors type,
 		return err;
 	}
 
-	if (!strcmp(attr_suffix, DT_FAULT_ATTR_SUFFIX)) {
-		attr_name = "fault";
-	} else if (!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX)) {
-		attr_name = "input";
-	} else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
-		if (type == TEMP)
-			attr_name = "max";
-		else if (type == FAN)
-			attr_name = "min";
-		else
-			return -ENOENT;
-	} else {
+	attr_name = convert_opal_attr_name(type, attr_suffix);
+	if (!attr_name)
 		return -ENOENT;
-	}
 
 	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
 		 sensor_groups[type].name, index, attr_name);
-- 
1.7.10.4

[PATCH 4/5] hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype

[Cédric Le Goater]

It simplifies the creation of the hwmon attributes and will help when
support for a new device tree layout is added. The patch also changes
the name of the routine to parse_opal_node_name().

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index edcc4ffa5ad0..570d2360b698 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -155,28 +155,22 @@ static const char *convert_opal_attr_name(enum sensors type,
  * which need to be mapped as fan2_input, temp1_max respectively before
  * populating them inside hwmon device class.
  */
-static int create_hwmon_attr_name(struct device *dev, enum sensors type,
-					 const char *node_name,
-					 char *hwmon_attr_name)
+static int parse_opal_node_name(const char *node_name, enum sensors type,
+	u32 *index, const char **hwmon_attr_name)
 {
 	char attr_suffix[MAX_ATTR_LEN];
 	const char *attr_name;
-	u32 index;
 	int err;
 
-	err = get_sensor_index_attr(node_name, &index, attr_suffix);
-	if (err) {
-		dev_err(dev, "Sensor device node name '%s' is invalid\n",
-			node_name);
+	err = get_sensor_index_attr(node_name, index, attr_suffix);
+	if (err)
 		return err;
-	}
 
 	attr_name = convert_opal_attr_name(type, attr_suffix);
 	if (!attr_name)
 		return -ENOENT;
 
-	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
-		 sensor_groups[type].name, index, attr_name);
+	*hwmon_attr_name = attr_name;
 	return 0;
 }
 
@@ -252,6 +246,9 @@ static int create_device_attrs(struct platform_device *pdev)
 	}
 
 	for_each_child_of_node(opal, np) {
+		const char *attr_name;
+		u32 opal_index;
+
 		if (np->name == NULL)
 			continue;
 
@@ -268,10 +265,17 @@ static int create_device_attrs(struct platform_device *pdev)
 
 		sdata[count].id = sensor_id;
 		sdata[count].type = type;
-		err = create_hwmon_attr_name(&pdev->dev, type, np->name,
-					     sdata[count].name);
-		if (err)
+
+		err = parse_opal_node_name(np->name, type, &opal_index,
+					   &attr_name);
+		if (err) {
+			dev_err(&pdev->dev, "Sensor device node name '%s' is invalid\n",
+				np->name);
 			goto exit_put_node;
+		}
+
+		snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d_%s",
+			 sensor_groups[type].name, opal_index, attr_name);
 
 		sysfs_attr_init(&sdata[count].dev_attr.attr);
 		sdata[count].dev_attr.attr.name = sdata[count].name;
-- 
1.7.10.4

[PATCH 5/5] hwmon: (ibmpowernv) do not use the OPAL index for hwmon attribute names

[Cédric Le Goater]

The current OPAL firmware exposes the different sensors of an IBM Power
system using the node names such as :

	sensors/amb-temp#1-data
	sensors/amb-temp#1-thrs
	cooling-fan#1-data
	cooling-fan#1-faulted
	cooling-fan#1-thrs
	cooling-fan#2-data
	...

The ibmpowernv driver, when loaded, parses these names to extract the
sensor index and the sensor attribute name. Unfortunately, this scheme
makes it difficult to add sensors with a different layout (specially of
the same type, like temperature) as the sensor index calculated in OPAL
is directly used in the hwmon sysfs interface.

What this patch does is add a independent hwmon index for each sensor.
The increment of the hwmon index (temp, fan, power, etc.) is kept per
sensor type in the sensor_group table. The sensor_data table is used
to store the association of the hwmon and OPAL indexes, as we need to
have the same hwmon index for different attributes of a same sensor.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index 570d2360b698..ffcebc9d8bf5 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -55,6 +55,7 @@ static struct sensor_group {
 	const char *compatible;
 	struct attribute_group group;
 	u32 attr_count;
+	u32 hwmon_index;
 } sensor_groups[] = {
 	{"fan", "ibm,opal-sensor-cooling-fan"},
 	{"temp", "ibm,opal-sensor-amb-temp"},
@@ -64,6 +65,8 @@ static struct sensor_group {
 
 struct sensor_data {
 	u32 id; /* An opaque id of the firmware for each sensor */
+	u32 hwmon_index;
+	u32 opal_index;
 	enum sensors type;
 	char name[MAX_ATTR_LEN];
 	struct device_attribute dev_attr;
@@ -185,6 +188,19 @@ static int get_sensor_type(struct device_node *np)
 	return MAX_SENSOR_TYPE;
 }
 
+static u32 get_sensor_hwmon_index(struct sensor_data *sdata,
+	struct sensor_data *sdata_table, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++)
+		if (sdata_table[i].opal_index == sdata->opal_index &&
+		    sdata_table[i].type == sdata->type)
+			return sdata_table[i].hwmon_index;
+
+	return ++sensor_groups[sdata->type].hwmon_index;
+}
+
 static int populate_attr_groups(struct platform_device *pdev)
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
@@ -274,8 +290,13 @@ static int create_device_attrs(struct platform_device *pdev)
 			goto exit_put_node;
 		}
 
+		sdata[count].opal_index = opal_index;
+		sdata[count].hwmon_index =
+			get_sensor_hwmon_index(&sdata[count], sdata, count);
+
 		snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d_%s",
-			 sensor_groups[type].name, opal_index, attr_name);
+			 sensor_groups[type].name, sdata[count].hwmon_index,
+			 attr_name);
 
 		sysfs_attr_init(&sdata[count].dev_attr.attr);
 		sdata[count].dev_attr.attr.name = sdata[count].name;
-- 
1.7.10.4

Re: [PATCH 3/5] hwmon: (ibmpowernv) add a convert_opal_attr_name() routine

[Guenter Roeck]

On 03/18/2015 08:47 AM, Cédric Le Goater wrote:
> It simplifies the create_hwmon_attr_name() routine and it clearly isolates
> the conversion done between the OPAL node names and hwmon attributes names.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   drivers/hwmon/ibmpowernv.c |   39 +++++++++++++++++++++++++--------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index 07a8219b7f4e..edcc4ffa5ad0 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -127,6 +127,28 @@ static int get_sensor_index_attr(const char *name, u32 *index,
>   	return 0;
>   }
>
> +static const char *convert_opal_attr_name(enum sensors type,
> +					const char *opal_attr)

Would be great if you could align all the continuation lines with '('.

> +{
> +	const char *attr_name = NULL;
> +
> +	if (!strcmp(opal_attr, DT_FAULT_ATTR_SUFFIX)) {
> +		attr_name = "fault";
> +	} else if (!strcmp(opal_attr, DT_DATA_ATTR_SUFFIX)) {
> +		attr_name = "input";
> +	} else if (!strcmp(opal_attr, DT_THRESHOLD_ATTR_SUFFIX)) {
> +		if (type == TEMP)
> +			attr_name = "max";
> +		else if (type == FAN)
> +			attr_name = "min";
> +		else
> +			return NULL;
> +	} else {
> +		return NULL;
> +	}

Those else cases returning NULL are unnecessary; attr_name
is already initialized with NULL, so you can just return it.


> +	return attr_name;
> +}
> +
>   /*
>    * This function translates the DT node name into the 'hwmon' attribute name.
>    * IBMPOWERNV device node appear like cooling-fan#2-data, amb-temp#1-thrs etc.
> @@ -138,7 +160,7 @@ static int create_hwmon_attr_name(struct device *dev, enum sensors type,
>   					 char *hwmon_attr_name)
>   {
>   	char attr_suffix[MAX_ATTR_LEN];
> -	char *attr_name;
> +	const char *attr_name;
>   	u32 index;
>   	int err;
>
> @@ -149,20 +171,9 @@ static int create_hwmon_attr_name(struct device *dev, enum sensors type,
>   		return err;
>   	}
>
> -	if (!strcmp(attr_suffix, DT_FAULT_ATTR_SUFFIX)) {
> -		attr_name = "fault";
> -	} else if (!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX)) {
> -		attr_name = "input";
> -	} else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
> -		if (type == TEMP)
> -			attr_name = "max";
> -		else if (type == FAN)
> -			attr_name = "min";
> -		else
> -			return -ENOENT;
> -	} else {
> +	attr_name = convert_opal_attr_name(type, attr_suffix);
> +	if (!attr_name)
>   		return -ENOENT;
> -	}
>
>   	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
>   		 sensor_groups[type].name, index, attr_name);
>

Re: [PATCH 4/5] hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype

[Guenter Roeck]

On 03/18/2015 08:47 AM, Cédric Le Goater wrote:
> It simplifies the creation of the hwmon attributes and will help when
> support for a new device tree layout is added. The patch also changes
> the name of the routine to parse_opal_node_name().
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   drivers/hwmon/ibmpowernv.c |   32 ++++++++++++++++++--------------
>   1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index edcc4ffa5ad0..570d2360b698 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -155,28 +155,22 @@ static const char *convert_opal_attr_name(enum sensors type,
>    * which need to be mapped as fan2_input, temp1_max respectively before
>    * populating them inside hwmon device class.
>    */
> -static int create_hwmon_attr_name(struct device *dev, enum sensors type,
> -					 const char *node_name,
> -					 char *hwmon_attr_name)
> +static int parse_opal_node_name(const char *node_name, enum sensors type,
> +	u32 *index, const char **hwmon_attr_name)
>   {

Please have the function return const char * and merge the error code with ERR_PTR.

>   	char attr_suffix[MAX_ATTR_LEN];
>   	const char *attr_name;
> -	u32 index;
>   	int err;
>
> -	err = get_sensor_index_attr(node_name, &index, attr_suffix);
> -	if (err) {
> -		dev_err(dev, "Sensor device node name '%s' is invalid\n",
> -			node_name);
> +	err = get_sensor_index_attr(node_name, index, attr_suffix);
> +	if (err)
>   		return err;
> -	}
>
>   	attr_name = convert_opal_attr_name(type, attr_suffix);
>   	if (!attr_name)
>   		return -ENOENT;
>
> -	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
> -		 sensor_groups[type].name, index, attr_name);
> +	*hwmon_attr_name = attr_name;
>   	return 0;
>   }
>
> @@ -252,6 +246,9 @@ static int create_device_attrs(struct platform_device *pdev)
>   	}
>
>   	for_each_child_of_node(opal, np) {
> +		const char *attr_name;
> +		u32 opal_index;
> +
>   		if (np->name == NULL)
>   			continue;
>
> @@ -268,10 +265,17 @@ static int create_device_attrs(struct platform_device *pdev)
>
>   		sdata[count].id = sensor_id;
>   		sdata[count].type = type;
> -		err = create_hwmon_attr_name(&pdev->dev, type, np->name,
> -					     sdata[count].name);
> -		if (err)
> +
> +		err = parse_opal_node_name(np->name, type, &opal_index,
> +					   &attr_name);
> +		if (err) {
> +			dev_err(&pdev->dev, "Sensor device node name '%s' is invalid\n",
> +				np->name);
>   			goto exit_put_node;
> +		}
> +
> +		snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d_%s",
> +			 sensor_groups[type].name, opal_index, attr_name);
>
>   		sysfs_attr_init(&sdata[count].dev_attr.attr);
>   		sdata[count].dev_attr.attr.name = sdata[count].name;
>

Re: [PATCH 0/5] hwmon: (ibmpowernv) remove dependency on OPAL index

[Guenter Roeck]

On 03/18/2015 08:47 AM, Cédric Le Goater wrote:
> Hello !
>
> The current implementation of the driver uses an index for the hwmon
> attribute which is extracted from the device node name. This index
> is calculated by the OPAL firmware and its usage creates a dependency
> with the driver which makes changes a little more complex in OPAL.
>
> This patchset changes the ibmpowernv code to use its own index. It
> starts with a few cleanups, mostly code shuffling around the creation
> of the hwmon sysfs attributes and completes by removing the dependency.
>
> It also prepares ground for future OPAL changes :
>
>     https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
>
> which will be addressed in a other small patchset.
>
>
> The patches are based on Linux 4.0.0-rc4 and were tested on IBM Power
> and Open Power systems running Trusty.
>

I commented on two of the patches; the others are ok.

Please re-send the entire series after addressing my comments.

Thanks,
Guenter

[PATCH v2 0/5] hwmon: (ibmpowernv) remove dependency on OPAL index

[Cédric Le Goater]

Hello !

The current implementation of the driver uses an index for the hwmon 
attribute which is extracted from the device node name. This index
is calculated by the OPAL firmware and its usage creates a dependency 
with the driver which makes changes a little more complex in OPAL.

This patchset changes the ibmpowernv code to use its own index. It 
starts with a few cleanups, mostly code shuffling around the creation 
of the hwmon sysfs attributes and completes by removing the dependency.

It also prepares ground for future OPAL changes :  

   https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html

which will be addressed in a other small patchset.


The patches are based on Linux 4.0.0-rc4 and were tested on IBM Power 
and Open Power systems running Trusty. 

Cheers,

C.


Changes since v1:

 - fixed alignment
 - killed a couple of useless "return NULL"
 - changed returned value of parse_opal_node_name()
 - used *_PTR macros to check for errors

Cédric Le Goater (5):
  hwmon: (ibmpowernv) replace AMBIENT_TEMP by TEMP
  hwmon: (ibmpowernv) add a get_sensor_type() routine
  hwmon: (ibmpowernv) add a convert_opal_attr_name() routine
  hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype
  hwmon: (ibmpowernv) do not use the OPAL index for hwmon attribute
    names

 drivers/hwmon/ibmpowernv.c |  122 +++++++++++++++++++++++++++++---------------
 1 file changed, 81 insertions(+), 41 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/5] hwmon: (ibmpowernv) replace AMBIENT_TEMP by TEMP

[Cédric Le Goater]

Ambient is too restrictive as there can be other temperature channels :
core, memory, etc.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux.git/drivers/hwmon/ibmpowernv.c
===================================================================
--- linux.git.orig/drivers/hwmon/ibmpowernv.c
+++ linux.git/drivers/hwmon/ibmpowernv.c
@@ -44,7 +44,7 @@
  */
 enum sensors {
 	FAN,
-	AMBIENT_TEMP,
+	TEMP,
 	POWER_SUPPLY,
 	POWER_INPUT,
 	MAX_SENSOR_TYPE,
@@ -87,7 +87,7 @@ static ssize_t show_sensor(struct device
 		return ret;
 
 	/* Convert temperature to milli-degrees */
-	if (sdata->type == AMBIENT_TEMP)
+	if (sdata->type == TEMP)
 		x *= 1000;
 	/* Convert power to micro-watts */
 	else if (sdata->type == POWER_INPUT)
@@ -154,7 +154,7 @@ static int create_hwmon_attr_name(struct
 	} else if (!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX)) {
 		attr_name = "input";
 	} else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
-		if (type == AMBIENT_TEMP)
+		if (type == TEMP)
 			attr_name = "max";
 		else if (type == FAN)
 			attr_name = "min";

[PATCH v2 2/5] hwmon: (ibmpowernv) add a get_sensor_type() routine

[Cédric Le Goater]

It will help in adding different compatible properties, coming from a
new device tree layout for example.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Index: linux.git/drivers/hwmon/ibmpowernv.c
===================================================================
--- linux.git.orig/drivers/hwmon/ibmpowernv.c
+++ linux.git/drivers/hwmon/ibmpowernv.c
@@ -169,6 +169,17 @@ static int create_hwmon_attr_name(struct
 	return 0;
 }
 
+static int get_sensor_type(struct device_node *np)
+{
+	enum sensors type;
+
+	for (type = 0; type < MAX_SENSOR_TYPE; type++) {
+		if (of_device_is_compatible(np, sensor_groups[type].compatible))
+			return type;
+	}
+	return MAX_SENSOR_TYPE;
+}
+
 static int populate_attr_groups(struct platform_device *pdev)
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
@@ -181,12 +192,9 @@ static int populate_attr_groups(struct p
 		if (np->name == NULL)
 			continue;
 
-		for (type = 0; type < MAX_SENSOR_TYPE; type++)
-			if (of_device_is_compatible(np,
-					sensor_groups[type].compatible)) {
-				sensor_groups[type].attr_count++;
-				break;
-			}
+		type = get_sensor_type(np);
+		if (type != MAX_SENSOR_TYPE)
+			sensor_groups[type].attr_count++;
 	}
 
 	of_node_put(opal);
@@ -236,11 +244,7 @@ static int create_device_attrs(struct pl
 		if (np->name == NULL)
 			continue;
 
-		for (type = 0; type < MAX_SENSOR_TYPE; type++)
-			if (of_device_is_compatible(np,
-					sensor_groups[type].compatible))
-				break;
-
+		type = get_sensor_type(np);
 		if (type == MAX_SENSOR_TYPE)
 			continue;
 

[PATCH v2 3/5] hwmon: (ibmpowernv) add a convert_opal_attr_name() routine

[Cédric Le Goater]

It simplifies the create_hwmon_attr_name() routine and it clearly isolates
the conversion done between the OPAL node names and hwmon attributes names.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---

Changes since v1:

 - fixed alignment
 - killed a couple of useless "return NULL"

 drivers/hwmon/ibmpowernv.c |   36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

Index: linux.git/drivers/hwmon/ibmpowernv.c
===================================================================
--- linux.git.orig/drivers/hwmon/ibmpowernv.c
+++ linux.git/drivers/hwmon/ibmpowernv.c
@@ -127,6 +127,25 @@ static int get_sensor_index_attr(const c
 	return 0;
 }
 
+static const char *convert_opal_attr_name(enum sensors type,
+					  const char *opal_attr)
+{
+	const char *attr_name = NULL;
+
+	if (!strcmp(opal_attr, DT_FAULT_ATTR_SUFFIX)) {
+		attr_name = "fault";
+	} else if (!strcmp(opal_attr, DT_DATA_ATTR_SUFFIX)) {
+		attr_name = "input";
+	} else if (!strcmp(opal_attr, DT_THRESHOLD_ATTR_SUFFIX)) {
+		if (type == TEMP)
+			attr_name = "max";
+		else if (type == FAN)
+			attr_name = "min";
+	}
+
+	return attr_name;
+}
+
 /*
  * This function translates the DT node name into the 'hwmon' attribute name.
  * IBMPOWERNV device node appear like cooling-fan#2-data, amb-temp#1-thrs etc.
@@ -138,7 +157,7 @@ static int create_hwmon_attr_name(struct
 					 char *hwmon_attr_name)
 {
 	char attr_suffix[MAX_ATTR_LEN];
-	char *attr_name;
+	const char *attr_name;
 	u32 index;
 	int err;
 
@@ -149,20 +168,9 @@ static int create_hwmon_attr_name(struct
 		return err;
 	}
 
-	if (!strcmp(attr_suffix, DT_FAULT_ATTR_SUFFIX)) {
-		attr_name = "fault";
-	} else if (!strcmp(attr_suffix, DT_DATA_ATTR_SUFFIX)) {
-		attr_name = "input";
-	} else if (!strcmp(attr_suffix, DT_THRESHOLD_ATTR_SUFFIX)) {
-		if (type == TEMP)
-			attr_name = "max";
-		else if (type == FAN)
-			attr_name = "min";
-		else
-			return -ENOENT;
-	} else {
+	attr_name = convert_opal_attr_name(type, attr_suffix);
+	if (!attr_name)
 		return -ENOENT;
-	}
 
 	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
 		 sensor_groups[type].name, index, attr_name);

[PATCH v2 4/5] hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype

[Cédric Le Goater]

It simplifies the creation of the hwmon attributes and will help when
support for a new device tree layout is added. The patch also changes
the name of the routine to parse_opal_node_name().

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---

Changes since v1:

 - changed returned value of parse_opal_node_name()
 - used *_PTR macros to check for errors

 drivers/hwmon/ibmpowernv.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Index: linux.git/drivers/hwmon/ibmpowernv.c
===================================================================
--- linux.git.orig/drivers/hwmon/ibmpowernv.c
+++ linux.git/drivers/hwmon/ibmpowernv.c
@@ -152,29 +152,22 @@ static const char *convert_opal_attr_nam
  * which need to be mapped as fan2_input, temp1_max respectively before
  * populating them inside hwmon device class.
  */
-static int create_hwmon_attr_name(struct device *dev, enum sensors type,
-					 const char *node_name,
-					 char *hwmon_attr_name)
+static const char *parse_opal_node_name(const char *node_name,
+					enum sensors type, u32 *index)
 {
 	char attr_suffix[MAX_ATTR_LEN];
 	const char *attr_name;
-	u32 index;
 	int err;
 
-	err = get_sensor_index_attr(node_name, &index, attr_suffix);
-	if (err) {
-		dev_err(dev, "Sensor device node name '%s' is invalid\n",
-			node_name);
-		return err;
-	}
+	err = get_sensor_index_attr(node_name, index, attr_suffix);
+	if (err)
+		return ERR_PTR(err);
 
 	attr_name = convert_opal_attr_name(type, attr_suffix);
 	if (!attr_name)
-		return -ENOENT;
+		return ERR_PTR(-ENOENT);
 
-	snprintf(hwmon_attr_name, MAX_ATTR_LEN, "%s%d_%s",
-		 sensor_groups[type].name, index, attr_name);
-	return 0;
+	return attr_name;
 }
 
 static int get_sensor_type(struct device_node *np)
@@ -249,6 +242,9 @@ static int create_device_attrs(struct pl
 	}
 
 	for_each_child_of_node(opal, np) {
+		const char *attr_name;
+		u32 opal_index;
+
 		if (np->name == NULL)
 			continue;
 
@@ -265,10 +261,17 @@ static int create_device_attrs(struct pl
 
 		sdata[count].id = sensor_id;
 		sdata[count].type = type;
-		err = create_hwmon_attr_name(&pdev->dev, type, np->name,
-					     sdata[count].name);
-		if (err)
+
+		attr_name = parse_opal_node_name(np->name, type, &opal_index);
+		if (IS_ERR(attr_name)) {
+			dev_err(&pdev->dev, "Sensor device node name '%s' is invalid\n",
+				np->name);
+			err = IS_ERR(attr_name);
 			goto exit_put_node;
+		}
+
+		snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d_%s",
+			 sensor_groups[type].name, opal_index, attr_name);
 
 		sysfs_attr_init(&sdata[count].dev_attr.attr);
 		sdata[count].dev_attr.attr.name = sdata[count].name;

[PATCH v2 5/5] hwmon: (ibmpowernv) do not use the OPAL index for hwmon attribute names

[Cédric Le Goater]

The current OPAL firmware exposes the different sensors of an IBM Power
system using node names such as :

	sensors/amb-temp#1-data
	sensors/amb-temp#1-thrs
	cooling-fan#1-data
	cooling-fan#1-faulted
	cooling-fan#1-thrs
	cooling-fan#2-data
	...

The ibmpowernv driver, when loaded, parses these names to extract the
sensor index and the sensor attribute name. Unfortunately, this scheme
makes it difficult to add sensors with a different layout (specially of
the same type, like temperature) as the sensor index calculated in OPAL
is directly used in the hwmon sysfs interface.

What this patch does is add a independent hwmon index for each sensor.
The increment of the hwmon index (temp, fan, power, etc.) is kept per
sensor type in the sensor_group table. The sensor_data table is used
to store the association of the hwmon and OPAL indexes, as we need to
have the same hwmon index for different attributes of a same sensor.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Index: linux.git/drivers/hwmon/ibmpowernv.c
===================================================================
--- linux.git.orig/drivers/hwmon/ibmpowernv.c
+++ linux.git/drivers/hwmon/ibmpowernv.c
@@ -55,6 +55,7 @@ static struct sensor_group {
 	const char *compatible;
 	struct attribute_group group;
 	u32 attr_count;
+	u32 hwmon_index;
 } sensor_groups[] = {
 	{"fan", "ibm,opal-sensor-cooling-fan"},
 	{"temp", "ibm,opal-sensor-amb-temp"},
@@ -64,6 +65,8 @@ static struct sensor_group {
 
 struct sensor_data {
 	u32 id; /* An opaque id of the firmware for each sensor */
+	u32 hwmon_index;
+	u32 opal_index;
 	enum sensors type;
 	char name[MAX_ATTR_LEN];
 	struct device_attribute dev_attr;
@@ -181,6 +184,19 @@ static int get_sensor_type(struct device
 	return MAX_SENSOR_TYPE;
 }
 
+static u32 get_sensor_hwmon_index(struct sensor_data *sdata,
+	struct sensor_data *sdata_table, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++)
+		if (sdata_table[i].opal_index == sdata->opal_index &&
+		    sdata_table[i].type == sdata->type)
+			return sdata_table[i].hwmon_index;
+
+	return ++sensor_groups[sdata->type].hwmon_index;
+}
+
 static int populate_attr_groups(struct platform_device *pdev)
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
@@ -270,8 +286,13 @@ static int create_device_attrs(struct pl
 			goto exit_put_node;
 		}
 
+		sdata[count].opal_index = opal_index;
+		sdata[count].hwmon_index =
+			get_sensor_hwmon_index(&sdata[count], sdata, count);
+
 		snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d_%s",
-			 sensor_groups[type].name, opal_index, attr_name);
+			 sensor_groups[type].name, sdata[count].hwmon_index,
+			 attr_name);
 
 		sysfs_attr_init(&sdata[count].dev_attr.attr);
 		sdata[count].dev_attr.attr.name = sdata[count].name;

Re: [PATCH v2 4/5] hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype

[Cedric Le Goater]

[ ... ] 


> @@ -265,10 +261,17 @@ static int create_device_attrs(struct pl
> 
>  		sdata[count].id = sensor_id;
>  		sdata[count].type = type;
> -		err = create_hwmon_attr_name(&pdev->dev, type, np->name,
> -					     sdata[count].name);
> -		if (err)
> +
> +		attr_name = parse_opal_node_name(np->name, type, &opal_index);
> +		if (IS_ERR(attr_name)) {
> +			dev_err(&pdev->dev, "Sensor device node name '%s' is invalid\n",
> +				np->name);
> +			err = IS_ERR(attr_name);
                              ^^^^^^

Arg. Bad copy/paste. This should be PTR_ERR() ... 

Do you want a full patchset resend or just a resend of this patch ? or a fix maybe.

Sorry for the noise ...

C.  

Re: [PATCH v2 0/5] hwmon: (ibmpowernv) remove dependency on OPAL index

[Guenter Roeck]

On Thu, Mar 19, 2015 at 06:44:40PM +0100, Cédric Le Goater wrote:
> Hello !
> 
> The current implementation of the driver uses an index for the hwmon 
> attribute which is extracted from the device node name. This index
> is calculated by the OPAL firmware and its usage creates a dependency 
> with the driver which makes changes a little more complex in OPAL.
> 
> This patchset changes the ibmpowernv code to use its own index. It 
> starts with a few cleanups, mostly code shuffling around the creation 
> of the hwmon sysfs attributes and completes by removing the dependency.
> 
Series applied to hwmon-next (with patch #4 fixed up).

Thanks,
Guenter

Re: [PATCH v2 4/5] hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype

[Guenter Roeck]

On Fri, Mar 20, 2015 at 09:06:38AM +0100, Cedric Le Goater wrote:
> [ ... ] 
> 
> 
> > @@ -265,10 +261,17 @@ static int create_device_attrs(struct pl
> > 
> >  		sdata[count].id = sensor_id;
> >  		sdata[count].type = type;
> > -		err = create_hwmon_attr_name(&pdev->dev, type, np->name,
> > -					     sdata[count].name);
> > -		if (err)
> > +
> > +		attr_name = parse_opal_node_name(np->name, type, &opal_index);
> > +		if (IS_ERR(attr_name)) {
> > +			dev_err(&pdev->dev, "Sensor device node name '%s' is invalid\n",
> > +				np->name);
> > +			err = IS_ERR(attr_name);
>                               ^^^^^^
> 
> Arg. Bad copy/paste. This should be PTR_ERR() ... 
> 
> Do you want a full patchset resend or just a resend of this patch ? or a fix maybe.
> 
No need to resend; I fixed it up.

Thanks,
Guenter

Re: [PATCH v2 0/5] hwmon: (ibmpowernv) remove dependency on OPAL index

[Cedric Le Goater]

On 03/20/2015 04:26 PM, Guenter Roeck wrote:
> On Thu, Mar 19, 2015 at 06:44:40PM +0100, Cédric Le Goater wrote:
>> Hello !
>>
>> The current implementation of the driver uses an index for the hwmon 
>> attribute which is extracted from the device node name. This index
>> is calculated by the OPAL firmware and its usage creates a dependency 
>> with the driver which makes changes a little more complex in OPAL.
>>
>> This patchset changes the ibmpowernv code to use its own index. It 
>> starts with a few cleanups, mostly code shuffling around the creation 
>> of the hwmon sysfs attributes and completes by removing the dependency.
>>
> Series applied to hwmon-next (with patch #4 fixed up).

Thanks !

C.

[PATCH 0/4] hwmon: (ibmpowernv) add DTS support

[Cédric Le Goater]

Hello !

These patches extend the ibmpowernv driver to support the new OPAL firmware 
which now exposes in its device tree the Digital Temperature Sensors of 
a P8 system.

They are based on Linux 4.0.0-rc6 + the initial cleanup serie :

     http://lists.lm-sensors.org/pipermail/lm-sensors/2015-March/043670.html

and were tested on IBM Power and Open Power systems running Trusty.

Cheers,

C.

Cédric Le Goater (4):
  hwmon: (ibmpowernv) add a helper routine create_hwmon_attr
  hwmon: (ibmpowernv) add support for the new device tree
  hwmon: (ibmpowernv) add a label attribute
  hwmon: (ibmpowernv) pretty print labels

 drivers/hwmon/ibmpowernv.c |  143 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 18 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] hwmon: (ibmpowernv) add a helper routine create_hwmon_attr

[Cédric Le Goater]

This should shorten a bit the code necessary to create a hmwon attribute.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index a6e7245f172d..c9aa4d837c6f 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -232,6 +232,20 @@ static int populate_attr_groups(struct platform_device *pdev)
 	return 0;
 }
 
+static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
+	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
+			char *buf))
+{
+	snprintf(sdata->name, MAX_ATTR_LEN, "%s%d_%s",
+		 sensor_groups[sdata->type].name, sdata->hwmon_index,
+		 attr_name);
+
+	sysfs_attr_init(&sdata->dev_attr.attr);
+	sdata->dev_attr.attr.name = sdata->name;
+	sdata->dev_attr.attr.mode = S_IRUGO;
+	sdata->dev_attr.show = show;
+}
+
 /*
  * 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
@@ -290,14 +304,7 @@ static int create_device_attrs(struct platform_device *pdev)
 		sdata[count].hwmon_index =
 			get_sensor_hwmon_index(&sdata[count], sdata, count);
 
-		snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d_%s",
-			 sensor_groups[type].name, sdata[count].hwmon_index,
-			 attr_name);
-
-		sysfs_attr_init(&sdata[count].dev_attr.attr);
-		sdata[count].dev_attr.attr.name = sdata[count].name;
-		sdata[count].dev_attr.attr.mode = S_IRUGO;
-		sdata[count].dev_attr.show = show_sensor;
+		create_hwmon_attr(&sdata[count], attr_name, show_sensor);
 
 		pgroups[type]->attrs[sensor_groups[type].attr_count++] =
 				&sdata[count++].dev_attr.attr;
-- 
1.7.10.4

[PATCH 2/4] hwmon: (ibmpowernv) add support for the new device tree

[Cédric Le Goater]

The new OPAL device tree for sensors has a different layout and uses new
property names, for the type and for the handler used to capture the
sensor data.

This patch modifies the ibmpowernv driver to support such a tree in a
way preserving compatibility with older OPAL firmwares.

This is achieved by changing the error path of the routine parsing
an OPAL node name. The node is simply considered being from the new
device tree layout and fallback values are used.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   47 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index c9aa4d837c6f..f38aa27343f2 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -176,11 +176,26 @@ static const char *parse_opal_node_name(const char *node_name,
 static int get_sensor_type(struct device_node *np)
 {
 	enum sensors type;
+	const char *str;
 
 	for (type = 0; type < MAX_SENSOR_TYPE; type++) {
 		if (of_device_is_compatible(np, sensor_groups[type].compatible))
 			return type;
 	}
+
+	/*
+	 * Let's check if we have a newer device tree
+	 */
+	if (!of_device_is_compatible(np, "ibm,opal-sensor"))
+		return MAX_SENSOR_TYPE;
+
+	if (of_property_read_string(np, "sensor-type", &str))
+		return MAX_SENSOR_TYPE;
+
+	for (type = 0; type < MAX_SENSOR_TYPE; type++)
+		if (!strcmp(str, sensor_groups[type].name))
+			return type;
+
 	return MAX_SENSOR_TYPE;
 }
 
@@ -189,11 +204,16 @@ static u32 get_sensor_hwmon_index(struct sensor_data *sdata,
 {
 	int i;
 
-	for (i = 0; i < count; i++)
-		if (sdata_table[i].opal_index == sdata->opal_index &&
-		    sdata_table[i].type == sdata->type)
-			return sdata_table[i].hwmon_index;
+	/*
+	 * We don't use the OPAL index on newer device trees
+	 */
+	if (sdata->opal_index != -1) {
+		for (i = 0; i < count; i++)
+			if (sdata_table[i].opal_index == sdata->opal_index &&
+			    sdata_table[i].type == sdata->type)
+				return sdata_table[i].hwmon_index;
 
+	}
 	return ++sensor_groups[sdata->type].hwmon_index;
 }
 
@@ -282,7 +302,12 @@ static int create_device_attrs(struct platform_device *pdev)
 		if (type == MAX_SENSOR_TYPE)
 			continue;
 
-		if (of_property_read_u32(np, "sensor-id", &sensor_id)) {
+		/*
+		 * Newer device trees use a "sensor-data" property
+		 * name for input.
+		 */
+		if (of_property_read_u32(np, "sensor-id", &sensor_id) &&
+		    of_property_read_u32(np, "sensor-data", &sensor_id)) {
 			dev_info(&pdev->dev,
 				 "'sensor-id' missing in the node '%s'\n",
 				 np->name);
@@ -292,12 +317,16 @@ static int create_device_attrs(struct platform_device *pdev)
 		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
+		 * about the OPAL index and use a defaut value for the
+		 * hwmon attribute name
+		 */
 		attr_name = parse_opal_node_name(np->name, type, &opal_index);
 		if (IS_ERR(attr_name)) {
-			dev_err(&pdev->dev, "Sensor device node name '%s' is invalid\n",
-				np->name);
-			err = PTR_ERR(attr_name);
-			goto exit_put_node;
+			attr_name = "input";
+			opal_index = -1;
 		}
 
 		sdata[count].opal_index = opal_index;
-- 
1.7.10.4

[PATCH 3/4] hwmon: (ibmpowernv) add a label attribute

[Cédric Le Goater]

Currently, sensors are only identified by their type and index.

The new OPAL device tree can expose extra properties to identify
some sensors by their name or location. This patch adds the creation
of a new hwmon *_label attribute when such properties are detected.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   51 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index f38aa27343f2..be6fe559b52a 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -32,6 +32,7 @@
 #include <linux/err.h>
 
 #define MAX_ATTR_LEN	32
+#define MAX_LABEL_LEN	64
 
 /* Sensor suffix name from DT */
 #define DT_FAULT_ATTR_SUFFIX		"faulted"
@@ -68,6 +69,7 @@ struct sensor_data {
 	u32 hwmon_index;
 	u32 opal_index;
 	enum sensors type;
+	char label[MAX_LABEL_LEN];
 	char name[MAX_ATTR_LEN];
 	struct device_attribute dev_attr;
 };
@@ -99,6 +101,23 @@ static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
 	return sprintf(buf, "%u\n", x);
 }
 
+static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
+			   char *buf)
+{
+	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
+						 dev_attr);
+
+	return sprintf(buf, "%s\n", sdata->label);
+}
+
+static void __init make_sensor_label(struct device_node *np,
+		    struct sensor_data *sdata, const char *label)
+{
+	size_t n;
+
+	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
+}
+
 static int get_sensor_index_attr(const char *name, u32 *index,
 					char *attr)
 {
@@ -226,11 +245,21 @@ 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;
+
 		if (np->name == NULL)
 			continue;
 
 		type = get_sensor_type(np);
-		if (type != MAX_SENSOR_TYPE)
+		if (type == MAX_SENSOR_TYPE)
+			continue;
+
+		sensor_groups[type].attr_count++;
+
+		/*
+		 * add a new attribute for labels
+		 */
+		if (!of_property_read_string(np, "label", &label))
 			sensor_groups[type].attr_count++;
 	}
 
@@ -294,6 +323,7 @@ static int create_device_attrs(struct platform_device *pdev)
 	for_each_child_of_node(opal, np) {
 		const char *attr_name;
 		u32 opal_index;
+		const char *label;
 
 		if (np->name == NULL)
 			continue;
@@ -337,6 +367,25 @@ static int create_device_attrs(struct platform_device *pdev)
 
 		pgroups[type]->attrs[sensor_groups[type].attr_count++] =
 				&sdata[count++].dev_attr.attr;
+
+		if (!of_property_read_string(np, "label", &label)) {
+			/*
+			 * For the label attribute, we can reuse the
+			 * "properties" of the previous "input"
+			 * 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);
+
+			create_hwmon_attr(&sdata[count], "label", show_label);
+
+			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
+				&sdata[count++].dev_attr.attr;
+		}
 	}
 
 exit_put_node:
-- 
1.7.10.4

[PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cédric Le Goater]

The new OPAL device tree adds a few properties which can be used to add
extra information on the sensor label.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 drivers/hwmon/ibmpowernv.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index be6fe559b52a..3e753c215b40 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -113,9 +113,31 @@ static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
 static void __init make_sensor_label(struct device_node *np,
 		    struct sensor_data *sdata, const char *label)
 {
+	u32 id;
 	size_t n;
 
 	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
+
+	/*
+	 * Core temp pretty print
+	 */
+	if (!of_property_read_u32(np, "ibm,pir", &id)) {
+		int i;
+
+		for_each_possible_cpu(i)
+			if (paca[i].hw_cpu_id == id)
+				break;
+
+		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
+			      " %d-%d", i, i+7);
+	}
+
+	/*
+	 * Membuffer pretty print
+	 */
+	if (!of_property_read_u32(np, "ibm,chip-id", &id))
+		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
+			      " %d", id & 0xffff);
 }
 
 static int get_sensor_index_attr(const char *name, u32 *index,
-- 
1.7.10.4

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Guenter Roeck]

On 04/01/2015 03:15 AM, Cédric Le Goater wrote:
> The new OPAL device tree adds a few properties which can be used to add
> extra information on the sensor label.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>   drivers/hwmon/ibmpowernv.c |   22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index be6fe559b52a..3e753c215b40 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -113,9 +113,31 @@ static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
>   static void __init make_sensor_label(struct device_node *np,
>   		    struct sensor_data *sdata, const char *label)
>   {
> +	u32 id;
>   	size_t n;
>
>   	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
> +
> +	/*
> +	 * Core temp pretty print
> +	 */
> +	if (!of_property_read_u32(np, "ibm,pir", &id)) {
> +		int i;
> +
> +		for_each_possible_cpu(i)
> +			if (paca[i].hw_cpu_id == id)
> +				break;
> +
> +		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
> +			      " %d-%d", i, i+7);

If ibm,pir points to a bad/invalid CPU id you just print an invalid value.
Is that what you want ? Also, what relevance does 'i' have for the user ?
It is the index into the paca array, sure, but what is its relevance
outside this code, especially in the context of you printing both i and i+7 ?

Guenter

> +	}
> +
> +	/*
> +	 * Membuffer pretty print
> +	 */
> +	if (!of_property_read_u32(np, "ibm,chip-id", &id))
> +		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
> +			      " %d", id & 0xffff);
>   }
>
>   static int get_sensor_index_attr(const char *name, u32 *index,
>

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

On 04/03/2015 05:49 PM, Guenter Roeck wrote:
> On 04/01/2015 03:15 AM, Cédric Le Goater wrote:
>> The new OPAL device tree adds a few properties which can be used to add
>> extra information on the sensor label.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>   drivers/hwmon/ibmpowernv.c |   22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>> index be6fe559b52a..3e753c215b40 100644
>> --- a/drivers/hwmon/ibmpowernv.c
>> +++ b/drivers/hwmon/ibmpowernv.c
>> @@ -113,9 +113,31 @@ static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
>>   static void __init make_sensor_label(struct device_node *np,
>>               struct sensor_data *sdata, const char *label)
>>   {
>> +    u32 id;
>>       size_t n;
>>
>>       n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
>> +
>> +    /*
>> +     * Core temp pretty print
>> +     */
>> +    if (!of_property_read_u32(np, "ibm,pir", &id)) {
>> +        int i;
>> +
>> +        for_each_possible_cpu(i)
>> +            if (paca[i].hw_cpu_id == id)
>> +                break;
>> +
>> +        n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
>> +                  " %d-%d", i, i+7);
> 
> If ibm,pir points to a bad/invalid CPU id you just print an invalid value.
> Is that what you want ? 

Certainly not :) I am being over optimistic on the underlying layer. 

> Also, what relevance does 'i' have for the user ?
> It is the index into the paca array, sure, but what is its relevance
> outside this code, especially in the context of you printing both i and i+7 ?

It gives a hint on the localization of the core in the system, which
can be useful when developing custom heat sinks. The translation 
from physical to linux cpu id is here to be consistent with the user
layer. The physical id is rarely used at that level.  

I will send a v2 for this patch.

Thanks,

C.
 

> 
> Guenter
> 
>> +    }
>> +
>> +    /*
>> +     * Membuffer pretty print
>> +     */
>> +    if (!of_property_read_u32(np, "ibm,chip-id", &id))
>> +        n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
>> +                  " %d", id & 0xffff);
>>   }
>>
>>   static int get_sensor_index_attr(const char *name, u32 *index,
>>
> 

[PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cédric Le Goater]

The new OPAL device tree adds a few properties which can be used to add
extra information on the sensor label.

In the case of a cpu core sensor, the firmware exposes the physical 
identifier of the core in the "ibm,pir" property. The driver 
translates this identifier in a linux cpu number and prints out a 
range corresponding to the hardware threads of the core (as they
share the same sensor).

The numbering gives a hint on the localization of the core in the 
system (which socket, which chip). 

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---

 Changes since v1:

 - check cpu validity before printing out the attribute label. 
   if invalid, use a "phy" prefix to distinguish a linux cpu 
   number from a physical cpu number. 

 drivers/hwmon/ibmpowernv.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Index: linux.git/drivers/hwmon/ibmpowernv.c
===================================================================
--- linux.git.orig/drivers/hwmon/ibmpowernv.c
+++ linux.git/drivers/hwmon/ibmpowernv.c
@@ -113,9 +113,43 @@ static ssize_t show_label(struct device
 static void __init make_sensor_label(struct device_node *np,
 		    struct sensor_data *sdata, const char *label)
 {
+	u32 id;
 	size_t n;
 
 	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
+
+	/*
+	 * Core temp pretty print
+	 */
+	if (!of_property_read_u32(np, "ibm,pir", &id)) {
+		int i = -1;
+
+		for_each_possible_cpu(i)
+			if (paca[i].hw_cpu_id == id)
+				break;
+
+		if (i != -1)
+			/*
+			 * The digital thermal sensors are associated
+			 * with a core. Let's print out the range of
+			 * cpu ids corresponding to the hardware
+			 * threads of the core.
+			 */
+			n += snprintf(sdata->label + n,
+				      sizeof(sdata->label) - n,
+				      " %d-%d", i, i+7);
+		else
+			n += snprintf(sdata->label + n,
+				      sizeof(sdata->label) - n,
+				      " phy%d", id);
+	}
+
+	/*
+	 * Membuffer pretty print
+	 */
+	if (!of_property_read_u32(np, "ibm,chip-id", &id))
+		n += snprintf(sdata->label + n, sizeof(sdata->label) - n,
+			      " %d", id & 0xffff);
 }
 
 static int get_sensor_index_attr(const char *name, u32 *index,

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Guenter Roeck]

On Tue, Apr 07, 2015 at 04:45:56PM +0200, Cédric Le Goater wrote:
> The new OPAL device tree adds a few properties which can be used to add
> extra information on the sensor label.
> 
> In the case of a cpu core sensor, the firmware exposes the physical 
> identifier of the core in the "ibm,pir" property. The driver 
> translates this identifier in a linux cpu number and prints out a 
> range corresponding to the hardware threads of the core (as they
> share the same sensor).
> 
> The numbering gives a hint on the localization of the core in the 
> system (which socket, which chip). 
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>

Hi Cedric,

Please do not send follow-up patches as reply, but as separate e-mail.

Further comments below.

> ---
> 
>  Changes since v1:
> 
>  - check cpu validity before printing out the attribute label. 
>    if invalid, use a "phy" prefix to distinguish a linux cpu 
>    number from a physical cpu number. 
> 
>  drivers/hwmon/ibmpowernv.c |   34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> Index: linux.git/drivers/hwmon/ibmpowernv.c
> ===================================================================
> --- linux.git.orig/drivers/hwmon/ibmpowernv.c
> +++ linux.git/drivers/hwmon/ibmpowernv.c
> @@ -113,9 +113,43 @@ static ssize_t show_label(struct device
>  static void __init make_sensor_label(struct device_node *np,
>  		    struct sensor_data *sdata, const char *label)
>  {
> +	u32 id;
>  	size_t n;
>  
>  	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
> +
> +	/*
> +	 * Core temp pretty print
> +	 */
> +	if (!of_property_read_u32(np, "ibm,pir", &id)) {
> +		int i = -1;
> +
> +		for_each_possible_cpu(i)
> +			if (paca[i].hw_cpu_id == id)

I think you should consider using get_hard_smp_processor_id() and avoid
direct access to the paca array.

> +				break;
> +
> +		if (i != -1)

I don't think that works. i is initialized by for_each_possible_cpu(),
either to -1 or 0 depending on the state of CONFIG_SMP. So pre-initializing
it won't make a difference. Its last value is NR_CPUS (SMP) or 1 (non-SMP).

You would need a second variable (which you could pre-initialize)
to determine if the code found a matching ID or not.

> +			/*
> +			 * The digital thermal sensors are associated
> +			 * with a core. Let's print out the range of
> +			 * cpu ids corresponding to the hardware
> +			 * threads of the core.
> +			 */
> +			n += snprintf(sdata->label + n,
> +				      sizeof(sdata->label) - n,
> +				      " %d-%d", i, i+7);

I would really like to see how this looks like in practice.

The id in the devicetree entry is the physical CPU. The resulting index
is the logical CPU number. So let's assume that the logical CPU number
for the first physical CPU is 0. Output would be "0-7". If the second
physical CPU matches logical CPU 1, output would be "1-8". 
How does that make any sense ?

Also, how do you know that the range of CPU IDs is always 8 ?

Can you provide some resulting outputs ?

Thanks,
Guenter

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

Hello Guenter,

On 04/07/2015 06:44 PM, Guenter Roeck wrote:
> On Tue, Apr 07, 2015 at 04:45:56PM +0200, Cédric Le Goater wrote:
>> The new OPAL device tree adds a few properties which can be used to add
>> extra information on the sensor label.
>>
>> In the case of a cpu core sensor, the firmware exposes the physical 
>> identifier of the core in the "ibm,pir" property. The driver 
>> translates this identifier in a linux cpu number and prints out a 
>> range corresponding to the hardware threads of the core (as they
>> share the same sensor).
>>
>> The numbering gives a hint on the localization of the core in the 
>> system (which socket, which chip). 
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> 
> Hi Cedric,
> 
> Please do not send follow-up patches as reply, but as separate e-mail.

Ok. I will start a new thread when I resend this patch.

> Further comments below.
> 
>> ---
>>
>>  Changes since v1:
>>
>>  - check cpu validity before printing out the attribute label. 
>>    if invalid, use a "phy" prefix to distinguish a linux cpu 
>>    number from a physical cpu number. 
>>
>>  drivers/hwmon/ibmpowernv.c |   34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> Index: linux.git/drivers/hwmon/ibmpowernv.c
>> ===================================================================
>> --- linux.git.orig/drivers/hwmon/ibmpowernv.c
>> +++ linux.git/drivers/hwmon/ibmpowernv.c
>> @@ -113,9 +113,43 @@ static ssize_t show_label(struct device
>>  static void __init make_sensor_label(struct device_node *np,
>>  		    struct sensor_data *sdata, const char *label)
>>  {
>> +	u32 id;
>>  	size_t n;
>>  
>>  	n = snprintf(sdata->label, sizeof(sdata->label), "%s", label);
>> +
>> +	/*
>> +	 * Core temp pretty print
>> +	 */
>> +	if (!of_property_read_u32(np, "ibm,pir", &id)) {
>> +		int i = -1;
>> +
>> +		for_each_possible_cpu(i)
>> +			if (paca[i].hw_cpu_id == id)
> 
> I think you should consider using get_hard_smp_processor_id() and avoid
> direct access to the paca array.

Yes. It will look better. Thanks.

>> +				break;
>> +
>> +		if (i != -1)
> 
> I don't think that works. i is initialized by for_each_possible_cpu(),
> either to -1 or 0 depending on the state of CONFIG_SMP. So pre-initializing
> it won't make a difference. Its last value is NR_CPUS (SMP) or 1 (non-SMP).
> 
> You would need a second variable (which you could pre-initialize)
> to determine if the code found a matching ID or not.

gloups. I did that patch waaaay too quickly, it is completely bogus ... 
I will cook a new version. sorry for the noise. 

>> +			/*
>> +			 * The digital thermal sensors are associated
>> +			 * with a core. Let's print out the range of
>> +			 * cpu ids corresponding to the hardware
>> +			 * threads of the core.
>> +			 */
>> +			n += snprintf(sdata->label + n,
>> +				      sizeof(sdata->label) - n,
>> +				      " %d-%d", i, i+7);
> 
> I would really like to see how this looks like in practice.
> 
> The id in the devicetree entry is the physical CPU. The resulting index
> is the logical CPU number. So let's assume that the logical CPU number
> for the first physical CPU is 0. Output would be "0-7". If the second
> physical CPU matches logical CPU 1, output would be "1-8". 
> How does that make any sense ?

The logical CPU numbers on PowerPC are initialized looping on the cores
found in the device tree and then on the threads of the core. Something 
like :

	logical_cpu = core_index * max_threads_per_core + thread_index 

which gives on a P8  :

	# ppc64_cpu --info
	Core   0:    0*    1*    2*    3*    4*    5*    6*    7* 
	Core   1:    8*    9*   10*   11*   12*   13*   14*   15* 
	Core   2:   16*   17*   18*   19*   20*   21*   22*   23* 
	Core   3:   24*   25*   26*   27*   28*   29*   30*   31* 
	Core   4:   32*   33*   34*   35*   36*   37*   38*   39* 
	Core   5:   40*   41*   42*   43*   44*   45*   46*   47* 
	Core   6:   48*   49*   50*   51*   52*   53*   54*   55* 
	Core   7:   56*   57*   58*   59*   60*   61*   62*   63* 
	Core   8:   64*   65*   66*   67*   68*   69*   70*   71* 
	Core   9:   72*   73*   74*   75*   76*   77*   78*   79* 
	Core  10:   80*   81*   82*   83*   84*   85*   86*   87* 
	Core  11:   88*   89*   90*   91*   92*   93*   94*   95* 
	Core  12:   96*   97*   98*   99*  100*  101*  102*  103* 
	Core  13:  104*  105*  106*  107*  108*  109*  110*  111* 
	Core  14:  112*  113*  114*  115*  116*  117*  118*  119* 
	Core  15:  120*  121*  122*  123*  124*  125*  126*  127* 
	Core  16:  128*  129*  130*  131*  132*  133*  134*  135* 
	Core  17:  136*  137*  138*  139*  140*  141*  142*  143* 
	Core  18:  144*  145*  146*  147*  148*  149*  150*  151* 
	Core  19:  152*  153*  154*  155*  156*  157*  158*  159* 

on a P7  :

	# ppc64_cpu --info
	Core   0:    0*    1*    2*    3* 
	Core   1:    4*    5*    6*    7* 
	Core   2:    8*    9*   10*   11* 
	Core   3:   12*   13*   14*   15* 
	Core   4:   16*   17*   18*   19* 
	Core   5:   20*   21*   22*   23* 


> Also, how do you know that the range of CPU IDs is always 8 ?

This is a shortcut. The code is for the ibmpowernv platform and assumes 
that we are running on a P8 (8 hardware threads). It would be better to 
use a "maximum threads per core" variable but I am not sure this is 
available, as it is a tunable. I will look into it.

> Can you provide some resulting outputs ?

sure. 

On an Open Power system : 

	# sensors
	ibmpowernv-isa-0000
	Adapter: ISA adapter
	Core 8-15:    +29.0°C  
	Core 16-23:   +29.0°C  
	Core 24-31:   +30.0°C  
	Core 32-39:   +30.0°C  
	Core 40-47:   +32.0°C  
	Core 48-55:   +29.0°C  
	Core 56-63:   +29.0°C  
	Centaur 0:    +28.0°C  
	Centaur 1:    +32.0°C  
	Centaur 4:    +28.0°C  
	Centaur 5:    +27.0°C  
	Core 0-7:     +29.0°C  

The Centaur numbering does not look as good as for the core.

On an IBM Power system :

	# sensors
	ibmpowernv-isa-0000
	Adapter: ISA adapter
	fan1:         5960 RPM  (min =    0 RPM)
	fan2:         5252 RPM  (min =    0 RPM)
	fan3:         5960 RPM  (min =    0 RPM)
	fan4:         5242 RPM  (min =    0 RPM)
	fan5:         5008 RPM  (min =    0 RPM)
	fan6:         5000 RPM  (min =    0 RPM)
	fan7:         5232 RPM  (min =    0 RPM)
	fan8:         5986 RPM  (min =    0 RPM)
	fan9:         5232 RPM  (min =    0 RPM)
	fan10:        6094 RPM  (min =    0 RPM)
	fan11:        5242 RPM  (min =    0 RPM)
	fan12:        5882 RPM  (min =    0 RPM)
	fan13:        5212 RPM  (min =    0 RPM)
	fan14:        5973 RPM  (min =    0 RPM)
	Ambient:       +18.0°C  (high =  +0.0°C)
	Core 0-7:      +40.0°C  
	Core 8-15:     +39.0°C  
	Core 16-23:    +39.0°C  
	Core 24-31:    +38.0°C  
	Core 32-39:    +38.0°C  
	Core 40-47:    +37.0°C  
	Core 48-55:    +37.0°C  
	Core 56-63:    +38.0°C  
	Core 64-71:    +38.0°C  
	Core 72-79:    +39.0°C  
	Core 80-87:    +35.0°C  
	Core 88-95:    +34.0°C  
	Core 96-103:   +33.0°C  
	Core 104-111:  +34.0°C  
	Core 112-119:  +33.0°C  
	Core 120-127:  +31.0°C  
	Core 128-135:  +31.0°C  
	Core 136-143:  +39.0°C  
	Core 144-151:  +39.0°C  
	Core 152-159:  +40.0°C  
	power1:       425.00 W  

The Centaur are not exposed yet.

Thanks,

C.

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Guenter Roeck]

Hi Cedric,

On Tue, Apr 07, 2015 at 08:03:46PM +0200, Cedric Le Goater wrote:
> 
> on a P7  :
> 
> 	# ppc64_cpu --info
> 	Core   0:    0*    1*    2*    3* 
> 	Core   1:    4*    5*    6*    7* 
> 	Core   2:    8*    9*   10*   11* 
> 	Core   3:   12*   13*   14*   15* 
> 	Core   4:   16*   17*   18*   19* 
> 	Core   5:   20*   21*   22*   23* 
> 
How would the 'sensors' output look like on that system ?
Wouldn't it be something like the following ?

 	Core 0-7:    +29.0°C  
 	Core 4-11:   +29.0°C  

> 
> > Also, how do you know that the range of CPU IDs is always 8 ?
> 
> This is a shortcut. The code is for the ibmpowernv platform and assumes 
> that we are running on a P8 (8 hardware threads). It would be better to 
> use a "maximum threads per core" variable but I am not sure this is 
> available, as it is a tunable. I will look into it.
> 
Tunable how ? The core code must have some means to detect this number
when it initialized CPU entries, or am I missing something ?

Thanks,
Guenter

Re: [Skiboot] [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Benjamin Herrenschmidt]

On Tue, 2015-04-07 at 20:03 +0200, Cedric Le Goater wrote:
> 
> This is a shortcut. The code is for the ibmpowernv platform and assumes 
> that we are running on a P8 (8 hardware threads). It would be better to 
> use a "maximum threads per core" variable but I am not sure this is 
> available, as it is a tunable. I will look into it.

Yes, please don't hard code this...

Cheers,
Ben.

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

Hello Guenter,

On 04/07/2015 09:22 PM, Guenter Roeck wrote:
> Hi Cedric,
> 
> On Tue, Apr 07, 2015 at 08:03:46PM +0200, Cedric Le Goater wrote:
>>
>> on a P7  :
>>
>> 	# ppc64_cpu --info
>> 	Core   0:    0*    1*    2*    3* 
>> 	Core   1:    4*    5*    6*    7* 
>> 	Core   2:    8*    9*   10*   11* 
>> 	Core   3:   12*   13*   14*   15* 
>> 	Core   4:   16*   17*   18*   19* 
>> 	Core   5:   20*   21*   22*   23* 
>>
> How would the 'sensors' output look like on that system ?
> Wouldn't it be something like the following ?
> 
>  	Core 0-7:    +29.0°C  
>  	Core 4-11:   +29.0°C  

yep. 

>>> Also, how do you know that the range of CPU IDs is always 8 ?
>>
>> This is a shortcut. The code is for the ibmpowernv platform and assumes 
>> that we are running on a P8 (8 hardware threads). It would be better to 
>> use a "maximum threads per core" variable but I am not sure this is 
>> available, as it is a tunable. I will look into it.
>>
> Tunable how ? 

You can switch on and off threads.

> The core code must have some means to detect this number when it initialized 
> CPU entries, or am I missing something ?

threads_per_core is what the code needs. v3 is on its way.

Thanks !

C.