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

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

[Guenter Roeck]

On Wed, Apr 01, 2015 at 12:15:04PM +0200, Cédric Le Goater wrote:
> 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>

Hi Cedric,

I was about to apply the series, but then I found the following problem.

> ---
>  drivers/hwmon/ibmpowernv.c |   47 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 9 deletions(-)
> 
[ ... ]
>  
> @@ -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) {

opal_index is u32, so this won't work (or at least the result is
unpredictable).

Also, in patch 4/4 (v4), get_logical_cpu() takes unsigned int as parameter,
but get_hard_smp_processor_id() returns an int, causing gcc to complain
if the code is built with W=1.

Please fix and resubmit the entire series.

When you do that, please also ensure that continuation lines
are aligned (in patch 3/4).

Thanks,
Guenter

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

[Cedric Le Goater]

On 04/08/2015 05:20 PM, Guenter Roeck wrote:
> On Wed, Apr 01, 2015 at 12:15:04PM +0200, Cédric Le Goater wrote:
>> 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>
> 
> Hi Cedric,
> 
> I was about to apply the series, but then I found the following problem.
> 
>> ---
>>  drivers/hwmon/ibmpowernv.c |   47 +++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 38 insertions(+), 9 deletions(-)
>>
> [ ... ]
>>  
>> @@ -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) {
> 
> opal_index is u32, so this won't work (or at least the result is
> unpredictable).
> 
> Also, in patch 4/4 (v4), get_logical_cpu() takes unsigned int as parameter,
> but get_hard_smp_processor_id() returns an int, causing gcc to complain
> if the code is built with W=1.
> 
> Please fix and resubmit the entire series.
> 
> When you do that, please also ensure that continuation lines
> are aligned (in patch 3/4).

Sure. Working on it right now.

Thanks,

C. 

[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 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