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

Submitted by Shilpasri G Bhat on April 21, 2017, 4:31 a.m.

Details

Message ID 1492749112-32465-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com
State Not Applicable
Headers show

Commit Message

Shilpasri G Bhat April 21, 2017, 4:31 a.m.
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(-)

Comments

Cédric Le Goater April 21, 2017, 11:47 a.m.
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:
>
Shilpasri G Bhat April 21, 2017, 12:39 p.m.
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:
>>
>
Michael Ellerman April 22, 2017, 6:11 a.m.
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
Cédric Le Goater April 25, 2017, 2:28 p.m.
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.
Cédric Le Goater April 25, 2017, 2:32 p.m.
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.
Cédric Le Goater April 25, 2017, 2:34 p.m.
...

> +			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.

Patch hide | download patch | download mbox

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: