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

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

[Shilpasri G Bhat]

Add support for adding min/max values for the inband sensors copied by
OCC to main memory. And also add current(mA) sensors to the list.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
 drivers/hwmon/ibmpowernv.c | 55 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index 6d2e660..1f329fa8 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -50,6 +50,7 @@ enum sensors {
 	TEMP,
 	POWER_SUPPLY,
 	POWER_INPUT,
+	CURRENT,
 	MAX_SENSOR_TYPE,
 };
 
@@ -65,7 +66,8 @@ enum sensors {
 	{"fan", "ibm,opal-sensor-cooling-fan"},
 	{"temp", "ibm,opal-sensor-amb-temp"},
 	{"in", "ibm,opal-sensor-power-supply"},
-	{"power", "ibm,opal-sensor-power"}
+	{"power", "ibm,opal-sensor-power"},
+	{"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
 };
 
 struct sensor_data {
@@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
 	opal = of_find_node_by_path("/ibm,opal/sensors");
 	for_each_child_of_node(opal, np) {
 		const char *label;
+		int len;
 
 		if (np->name == NULL)
 			continue;
@@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev)
 		sensor_groups[type].attr_count++;
 
 		/*
-		 * add a new attribute for labels
+		 * add attributes for labels, min and max
 		 */
 		if (!of_property_read_string(np, "label", &label))
 			sensor_groups[type].attr_count++;
+		if (of_find_property(np, "sensor-data-min", &len))
+			sensor_groups[type].attr_count++;
+		if (of_find_property(np, "sensor-data-max", &len))
+			sensor_groups[type].attr_count++;
 	}
 
 	of_node_put(opal);
@@ -428,6 +435,50 @@ 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_u32(np, "sensor-data-max", &sensor_id)) {
+			switch (type) {
+			case POWER_INPUT:
+				attr_name = "input_highest";
+				break;
+			case TEMP:
+				attr_name = "max";
+				break;
+			default:
+				attr_name = "highest";
+				break;
+			}
+
+			sdata[count].id = sensor_id;
+			sdata[count].type = type;
+			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
+			create_hwmon_attr(&sdata[count], attr_name,
+					  show_sensor);
+			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
+				&sdata[count++].dev_attr.attr;
+		}
+
+		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
+			switch (type) {
+			case POWER_INPUT:
+				attr_name = "input_lowest";
+				break;
+			case TEMP:
+				attr_name = "min";
+				break;
+			default:
+				attr_name = "lowest";
+				break;
+			}
+
+			sdata[count].id = sensor_id;
+			sdata[count].type = type;
+			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
+			create_hwmon_attr(&sdata[count], attr_name,
+					  show_sensor);
+			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
+				&sdata[count++].dev_attr.attr;
+		}
 	}
 
 exit_put_node:
-- 
1.8.3.1

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

[Cédric Le Goater]

Hello Shilpasri,

On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote:
> Add support for adding min/max values for the inband sensors copied by
> OCC to main memory. And also add current(mA) sensors to the list.
> 
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
>  drivers/hwmon/ibmpowernv.c | 55 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index 6d2e660..1f329fa8 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -50,6 +50,7 @@ enum sensors {
>  	TEMP,
>  	POWER_SUPPLY,
>  	POWER_INPUT,
> +	CURRENT,
>  	MAX_SENSOR_TYPE,
>  };
>  
> @@ -65,7 +66,8 @@ enum sensors {
>  	{"fan", "ibm,opal-sensor-cooling-fan"},
>  	{"temp", "ibm,opal-sensor-amb-temp"},
>  	{"in", "ibm,opal-sensor-power-supply"},
> -	{"power", "ibm,opal-sensor-power"}
> +	{"power", "ibm,opal-sensor-power"},
> +	{"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */

why isn't there a compatible string ? 

>  };
>  
>  struct sensor_data {
> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
>  	opal = of_find_node_by_path("/ibm,opal/sensors");
>  	for_each_child_of_node(opal, np) {
>  		const char *label;
> +		int len;
>  
>  		if (np->name == NULL)
>  			continue;
> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev)
>  		sensor_groups[type].attr_count++;
>  
>  		/*
> -		 * add a new attribute for labels
> +		 * add attributes for labels, min and max
>  		 */
>  		if (!of_property_read_string(np, "label", &label))
>  			sensor_groups[type].attr_count++;

We are negating of_property_read_string() above, but not below.
I wonder why ? 
 
> +		if (of_find_property(np, "sensor-data-min", &len))
> +			sensor_groups[type].attr_count++;
> +		if (of_find_property(np, "sensor-data-max", &len))
> +			sensor_groups[type].attr_count++;
>  	}
>  
>  	of_node_put(opal);
> @@ -428,6 +435,50 @@ 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_u32(np, "sensor-data-max", &sensor_id)) {
> +			switch (type) {
> +			case POWER_INPUT:
> +				attr_name = "input_highest";
> +				break;
> +			case TEMP:
> +				attr_name = "max";
> +				break;
> +			default:
> +				attr_name = "highest";
> +				break;
> +			}

May be we could use a converting routine here ? create_device_attrs()
is getting big.

> +			sdata[count].id = sensor_id;
> +			sdata[count].type = type;
> +			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
> +			create_hwmon_attr(&sdata[count], attr_name,
> +					  show_sensor);
> +			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> +				&sdata[count++].dev_attr.attr;
> +		}
> +
> +		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
> +			switch (type) {
> +			case POWER_INPUT:
> +				attr_name = "input_lowest";
> +				break;
> +			case TEMP:
> +				attr_name = "min";
> +				break;
> +			default:
> +				attr_name = "lowest";
> +				break;
> +			}

same here.

Thanks,

C.
> +			sdata[count].id = sensor_id;
> +			sdata[count].type = type;
> +			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
> +			create_hwmon_attr(&sdata[count], attr_name,
> +					  show_sensor);
> +			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> +				&sdata[count++].dev_attr.attr;
> +		}
>  	}
>  
>  exit_put_node:
> 

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

[Shilpasri G Bhat]

Hi Cedric,

On 04/21/2017 05:17 PM, Cédric Le Goater wrote:
> Hello Shilpasri,
> 
> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote:
>> Add support for adding min/max values for the inband sensors copied by
>> OCC to main memory. And also add current(mA) sensors to the list.
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>>  drivers/hwmon/ibmpowernv.c | 55 ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>> index 6d2e660..1f329fa8 100644
>> --- a/drivers/hwmon/ibmpowernv.c
>> +++ b/drivers/hwmon/ibmpowernv.c
>> @@ -50,6 +50,7 @@ enum sensors {
>>  	TEMP,
>>  	POWER_SUPPLY,
>>  	POWER_INPUT,
>> +	CURRENT,
>>  	MAX_SENSOR_TYPE,
>>  };
>>  
>> @@ -65,7 +66,8 @@ enum sensors {
>>  	{"fan", "ibm,opal-sensor-cooling-fan"},
>>  	{"temp", "ibm,opal-sensor-amb-temp"},
>>  	{"in", "ibm,opal-sensor-power-supply"},
>> -	{"power", "ibm,opal-sensor-power"}
>> +	{"power", "ibm,opal-sensor-power"},
>> +	{"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
> 
> why isn't there a compatible string ? 
> 

Skiboot exports "ibm,opal-sensor" as compatible string for all the inband
sensors. Now if I define that as the compatible string here, then all the
sensors would get identified as "curr" type of sensor by the driver.

>>  };
>>  
>>  struct sensor_data {
>> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
>>  	opal = of_find_node_by_path("/ibm,opal/sensors");
>>  	for_each_child_of_node(opal, np) {
>>  		const char *label;
>> +		int len;
>>  
>>  		if (np->name == NULL)
>>  			continue;
>> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev)
>>  		sensor_groups[type].attr_count++;
>>  
>>  		/*
>> -		 * add a new attribute for labels
>> +		 * add attributes for labels, min and max
>>  		 */
>>  		if (!of_property_read_string(np, "label", &label))
>>  			sensor_groups[type].attr_count++;
> 
> We are negating of_property_read_string() above, but not below.
> I wonder why ? 

of_find_property() returns 'struct property *' while of_property_read_string()
returns 0 on success.

> 
>> +		if (of_find_property(np, "sensor-data-min", &len))
>> +			sensor_groups[type].attr_count++;
>> +		if (of_find_property(np, "sensor-data-max", &len))
>> +			sensor_groups[type].attr_count++;
>>  	}
>>  
>>  	of_node_put(opal);
>> @@ -428,6 +435,50 @@ 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_u32(np, "sensor-data-max", &sensor_id)) {
>> +			switch (type) {
>> +			case POWER_INPUT:
>> +				attr_name = "input_highest";
>> +				break;
>> +			case TEMP:
>> +				attr_name = "max";
>> +				break;
>> +			default:
>> +				attr_name = "highest";
>> +				break;
>> +			}
> 
> May be we could use a converting routine here ? create_device_attrs()
> is getting big.

Okay will do.

> 
>> +			sdata[count].id = sensor_id;
>> +			sdata[count].type = type;
>> +			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>> +			create_hwmon_attr(&sdata[count], attr_name,
>> +					  show_sensor);
>> +			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>> +				&sdata[count++].dev_attr.attr;
>> +		}
>> +
>> +		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
>> +			switch (type) {
>> +			case POWER_INPUT:
>> +				attr_name = "input_lowest";
>> +				break;
>> +			case TEMP:
>> +				attr_name = "min";
>> +				break;
>> +			default:
>> +				attr_name = "lowest";
>> +				break;
>> +			}
> 
> same here.

Sure.

Thanks and Regards,
Shilpa

> 
> Thanks,
> 
> C.
>> +			sdata[count].id = sensor_id;
>> +			sdata[count].type = type;
>> +			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>> +			create_hwmon_attr(&sdata[count], attr_name,
>> +					  show_sensor);
>> +			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>> +				&sdata[count++].dev_attr.attr;
>> +		}
>>  	}
>>  
>>  exit_put_node:
>>
> 

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

[Michael Ellerman]

Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> On 04/21/2017 05:17 PM, Cédric Le Goater wrote:
>> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote:
>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>> index 6d2e660..1f329fa8 100644
>>> --- a/drivers/hwmon/ibmpowernv.c
>>> +++ b/drivers/hwmon/ibmpowernv.c
>>> @@ -65,7 +66,8 @@ enum sensors {
>>>  	{"fan", "ibm,opal-sensor-cooling-fan"},
>>>  	{"temp", "ibm,opal-sensor-amb-temp"},
>>>  	{"in", "ibm,opal-sensor-power-supply"},
>>> -	{"power", "ibm,opal-sensor-power"}
>>> +	{"power", "ibm,opal-sensor-power"},
>>> +	{"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>> 
>> why isn't there a compatible string ? 
>
> Skiboot exports "ibm,opal-sensor" as compatible string for all the inband
> sensors. Now if I define that as the compatible string here, then all the
> sensors would get identified as "curr" type of sensor by the driver.

So fix it in skiboot?

cheers

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

[Cédric Le Goater]

On 04/22/2017 08:11 AM, Michael Ellerman wrote:
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>> On 04/21/2017 05:17 PM, Cédric Le Goater wrote:
>>> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote:
>>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>>> index 6d2e660..1f329fa8 100644
>>>> --- a/drivers/hwmon/ibmpowernv.c
>>>> +++ b/drivers/hwmon/ibmpowernv.c
>>>> @@ -65,7 +66,8 @@ enum sensors {
>>>>  	{"fan", "ibm,opal-sensor-cooling-fan"},
>>>>  	{"temp", "ibm,opal-sensor-amb-temp"},
>>>>  	{"in", "ibm,opal-sensor-power-supply"},
>>>> -	{"power", "ibm,opal-sensor-power"}
>>>> +	{"power", "ibm,opal-sensor-power"},
>>>> +	{"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>>>
>>> why isn't there a compatible string ? 
>>
>> Skiboot exports "ibm,opal-sensor" as compatible string for all the inband
>> sensors. Now if I define that as the compatible string here, then all the
>> sensors would get identified as "curr" type of sensor by the driver.
> 
> So fix it in skiboot?


After a memory refresh, this table is to maintain compatibility with 
the support of the FSP sensors in old firmware. These have peculiar
device node names and properties.  

So What the code is doing looks correct. But, you should also modify 
the 'enum sensors' to include a CURRENT value.

Thanks,

C.

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

[Cédric Le Goater]

On 04/25/2017 04:28 PM, Cédric Le Goater wrote:
> On 04/22/2017 08:11 AM, Michael Ellerman wrote:
>> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>>> On 04/21/2017 05:17 PM, Cédric Le Goater wrote:
>>>> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote:
>>>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>>>> index 6d2e660..1f329fa8 100644
>>>>> --- a/drivers/hwmon/ibmpowernv.c
>>>>> +++ b/drivers/hwmon/ibmpowernv.c
>>>>> @@ -65,7 +66,8 @@ enum sensors {
>>>>>  	{"fan", "ibm,opal-sensor-cooling-fan"},
>>>>>  	{"temp", "ibm,opal-sensor-amb-temp"},
>>>>>  	{"in", "ibm,opal-sensor-power-supply"},
>>>>> -	{"power", "ibm,opal-sensor-power"}
>>>>> +	{"power", "ibm,opal-sensor-power"},
>>>>> +	{"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>>>>
>>>> why isn't there a compatible string ? 
>>>
>>> Skiboot exports "ibm,opal-sensor" as compatible string for all the inband
>>> sensors. Now if I define that as the compatible string here, then all the
>>> sensors would get identified as "curr" type of sensor by the driver.
>>
>> So fix it in skiboot?
> 
> 
> After a memory refresh, this table is to maintain compatibility with 
> the support of the FSP sensors in old firmware. These have peculiar
> device node names and properties.  
> 
> So What the code is doing looks correct. But, you should also modify 
> the 'enum sensors' to include a CURRENT value.

But the patch does that already. I was missing context. This is fine
then.

Thanks,

C.

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

[Cédric Le Goater]

...

> +			sdata[count].id = sensor_id;
> +			sdata[count].type = type;
> +			sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
> +			create_hwmon_attr(&sdata[count], attr_name,
> +					  show_sensor);
> +			pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> +				&sdata[count++].dev_attr.attr;
> +		}

We are duplicating these lines at least three times. I wonder if we 
could make a routine for them. Don't bother doing so if the number
of arguments is too large.

Thanks,

C.