* [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors
@ 2017-04-28 5:59 Shilpasri G Bhat
2017-04-28 13:29 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Shilpasri G Bhat @ 2017-04-28 5:59 UTC (permalink / raw)
To: jdelvare, linux, paulus, mpe
Cc: linux-hwmon, linuxppc-dev, linux-kernel, svaidy, ego,
akshay.adiga, andrew, clg, 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>
---
Changes from V1:
- Add functions to get min and max attribute strings
- Add function 'populate_sensor' to fill in the 'struct sensor_data'
for each sensor.
drivers/hwmon/ibmpowernv.c | 96 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 77 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index 6d2e660..d59262c 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -50,6 +50,7 @@ enum sensors {
TEMP,
POWER_SUPPLY,
POWER_INPUT,
+ CURRENT,
MAX_SENSOR_TYPE,
};
@@ -65,7 +66,8 @@ enum sensors {
{"fan", "ibm,opal-sensor-cooling-fan"},
{"temp", "ibm,opal-sensor-amb-temp"},
{"in", "ibm,opal-sensor-power-supply"},
- {"power", "ibm,opal-sensor-power"}
+ {"power", "ibm,opal-sensor-power"},
+ {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
};
struct sensor_data {
@@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
opal = of_find_node_by_path("/ibm,opal/sensors");
for_each_child_of_node(opal, np) {
const char *label;
+ int len;
if (np->name == NULL)
continue;
@@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev)
sensor_groups[type].attr_count++;
/*
- * add a new attribute for labels
+ * add attributes for labels, min and max
*/
if (!of_property_read_string(np, "label", &label))
sensor_groups[type].attr_count++;
+ if (of_find_property(np, "sensor-data-min", &len))
+ sensor_groups[type].attr_count++;
+ if (of_find_property(np, "sensor-data-max", &len))
+ sensor_groups[type].attr_count++;
}
of_node_put(opal);
@@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
sdata->dev_attr.show = show;
}
+static void populate_sensor(struct sensor_data *sdata, int od, int hd, int sid,
+ const char *attr_name, enum sensors type,
+ const struct attribute_group *pgroup,
+ ssize_t (*show)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf))
+{
+ sdata->id = sid;
+ sdata->type = type;
+ sdata->opal_index = od;
+ sdata->hwmon_index = hd;
+ create_hwmon_attr(sdata, attr_name, show);
+ pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
+}
+
+static char *get_max_attr(enum sensors type)
+{
+ switch (type) {
+ case POWER_INPUT:
+ return "input_highest";
+ case TEMP:
+ return "max";
+ default:
+ break;
+ }
+
+ return "highest";
+}
+
+static char *get_min_attr(enum sensors type)
+{
+ switch (type) {
+ case POWER_INPUT:
+ return "input_lowest";
+ case TEMP:
+ return "min";
+ default:
+ break;
+ }
+
+ return "lowest";
+}
+
/*
* Iterate through the device tree for each child of 'sensors' node, create
* a sysfs attribute file, the file is named by translating the DT node name
@@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device *pdev)
for_each_child_of_node(opal, np) {
const char *attr_name;
u32 opal_index;
+ u32 hwmon_index;
const char *label;
if (np->name == NULL)
@@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device *pdev)
continue;
}
- sdata[count].id = sensor_id;
- sdata[count].type = type;
-
/*
* If we can not parse the node name, it means we are
* running on a newer device tree. We can just forget
@@ -401,14 +449,12 @@ static int create_device_attrs(struct platform_device *pdev)
opal_index = INVALID_INDEX;
}
- sdata[count].opal_index = opal_index;
- sdata[count].hwmon_index =
- get_sensor_hwmon_index(&sdata[count], sdata, count);
-
- create_hwmon_attr(&sdata[count], attr_name, show_sensor);
-
- pgroups[type]->attrs[sensor_groups[type].attr_count++] =
- &sdata[count++].dev_attr.attr;
+ hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata,
+ count);
+ populate_sensor(&sdata[count], opal_index, hwmon_index,
+ sensor_id, attr_name, type, pgroups[type],
+ show_sensor);
+ count++;
if (!of_property_read_string(np, "label", &label)) {
/*
@@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device *pdev)
* attribute. They are related to the same
* sensor.
*/
- sdata[count].type = type;
- sdata[count].opal_index = sdata[count - 1].opal_index;
- sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
make_sensor_label(np, &sdata[count], label);
+ populate_sensor(&sdata[count], opal_index, hwmon_index,
+ sensor_id, "label", type, pgroups[type],
+ show_label);
+ count++;
+ }
- create_hwmon_attr(&sdata[count], "label", show_label);
+ if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
+ attr_name = get_max_attr(type);
+ populate_sensor(&sdata[count], opal_index, hwmon_index,
+ sensor_id, attr_name, type,
+ pgroups[type], show_sensor);
+ count++;
+ }
- pgroups[type]->attrs[sensor_groups[type].attr_count++] =
- &sdata[count++].dev_attr.attr;
+ if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
+ attr_name = get_min_attr(type);
+ populate_sensor(&sdata[count], opal_index, hwmon_index,
+ sensor_id, attr_name, type,
+ pgroups[type], show_sensor);
+ count++;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors
2017-04-28 5:59 [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors Shilpasri G Bhat
@ 2017-04-28 13:29 ` Guenter Roeck
2017-05-02 4:35 ` Shilpasri G Bhat
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2017-04-28 13:29 UTC (permalink / raw)
To: Shilpasri G Bhat, jdelvare, paulus, mpe
Cc: linux-hwmon, linuxppc-dev, linux-kernel, svaidy, ego,
akshay.adiga, andrew, clg
On 04/27/2017 10:59 PM, Shilpasri G Bhat wrote:
> Add support for adding min/max values for the inband sensors copied by
> OCC to main memory. And also add current(mA) sensors to the list.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> Changes from V1:
> - Add functions to get min and max attribute strings
> - Add function 'populate_sensor' to fill in the 'struct sensor_data'
> for each sensor.
>
> drivers/hwmon/ibmpowernv.c | 96 +++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 77 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index 6d2e660..d59262c 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -50,6 +50,7 @@ enum sensors {
> TEMP,
> POWER_SUPPLY,
> POWER_INPUT,
> + CURRENT,
> MAX_SENSOR_TYPE,
> };
>
> @@ -65,7 +66,8 @@ enum sensors {
> {"fan", "ibm,opal-sensor-cooling-fan"},
> {"temp", "ibm,opal-sensor-amb-temp"},
> {"in", "ibm,opal-sensor-power-supply"},
> - {"power", "ibm,opal-sensor-power"}
> + {"power", "ibm,opal-sensor-power"},
> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
Following up on a previous e-mail, this really _is_ odd. Any chance to fix it
in the firmware and have current sensors return "ibm,opal-sensor-current" ?
> };
>
> struct sensor_data {
> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
> opal = of_find_node_by_path("/ibm,opal/sensors");
> for_each_child_of_node(opal, np) {
> const char *label;
> + int len;
>
> if (np->name == NULL)
> continue;
> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev)
> sensor_groups[type].attr_count++;
>
> /*
> - * add a new attribute for labels
> + * add attributes for labels, min and max
> */
> if (!of_property_read_string(np, "label", &label))
> sensor_groups[type].attr_count++;
> + if (of_find_property(np, "sensor-data-min", &len))
> + sensor_groups[type].attr_count++;
> + if (of_find_property(np, "sensor-data-max", &len))
> + sensor_groups[type].attr_count++;
> }
>
> of_node_put(opal);
> @@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
> sdata->dev_attr.show = show;
> }
>
> +static void populate_sensor(struct sensor_data *sdata, int od, int hd, int sid,
> + const char *attr_name, enum sensors type,
> + const struct attribute_group *pgroup,
> + ssize_t (*show)(struct device *dev,
> + struct device_attribute *attr,
> + char *buf))
> +{
> + sdata->id = sid;
> + sdata->type = type;
> + sdata->opal_index = od;
> + sdata->hwmon_index = hd;
> + create_hwmon_attr(sdata, attr_name, show);
> + pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
> +}
> +
> +static char *get_max_attr(enum sensors type)
> +{
> + switch (type) {
> + case POWER_INPUT:
> + return "input_highest";
> + case TEMP:
> + return "max";
> + default:
> + break;
> + }
> +
> + return "highest";
This is a bit confusing. Why not 'return "highest";' in the default case above ?
Also, is this correct for type == POWER_SUPPLY, ie is it "highest" vs. "max" ?
Kind of odd that the firmware reports "highest/lowest" in some cases
and "max/min" in others. Guess there is nothing we can do about that,
just a note.
> +}
> +
> +static char *get_min_attr(enum sensors type)
> +{
> + switch (type) {
> + case POWER_INPUT:
> + return "input_lowest";
> + case TEMP:
> + return "min";
> + default:
> + break;
> + }
> +
> + return "lowest";
Same here.
> +}
> +
> /*
> * Iterate through the device tree for each child of 'sensors' node, create
> * a sysfs attribute file, the file is named by translating the DT node name
> @@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device *pdev)
> for_each_child_of_node(opal, np) {
> const char *attr_name;
> u32 opal_index;
> + u32 hwmon_index;
> const char *label;
>
> if (np->name == NULL)
> @@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device *pdev)
> continue;
> }
>
> - sdata[count].id = sensor_id;
> - sdata[count].type = type;
> -
> /*
> * If we can not parse the node name, it means we are
> * running on a newer device tree. We can just forget
> @@ -401,14 +449,12 @@ static int create_device_attrs(struct platform_device *pdev)
> opal_index = INVALID_INDEX;
> }
>
> - sdata[count].opal_index = opal_index;
> - sdata[count].hwmon_index =
> - get_sensor_hwmon_index(&sdata[count], sdata, count);
> -
> - create_hwmon_attr(&sdata[count], attr_name, show_sensor);
> -
> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> - &sdata[count++].dev_attr.attr;
> + hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata,
> + count);
> + populate_sensor(&sdata[count], opal_index, hwmon_index,
> + sensor_id, attr_name, type, pgroups[type],
> + show_sensor);
> + count++;
>
> if (!of_property_read_string(np, "label", &label)) {
> /*
> @@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device *pdev)
> * attribute. They are related to the same
> * sensor.
> */
> - sdata[count].type = type;
> - sdata[count].opal_index = sdata[count - 1].opal_index;
> - sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>
> make_sensor_label(np, &sdata[count], label);
> + populate_sensor(&sdata[count], opal_index, hwmon_index,
> + sensor_id, "label", type, pgroups[type],
> + show_label);
> + count++;
> + }
>
> - create_hwmon_attr(&sdata[count], "label", show_label);
> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
> + attr_name = get_max_attr(type);
> + populate_sensor(&sdata[count], opal_index, hwmon_index,
> + sensor_id, attr_name, type,
> + pgroups[type], show_sensor);
> + count++;
> + }
>
> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> - &sdata[count++].dev_attr.attr;
> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
> + attr_name = get_min_attr(type);
> + populate_sensor(&sdata[count], opal_index, hwmon_index,
> + sensor_id, attr_name, type,
> + pgroups[type], show_sensor);
> + count++;
> }
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors
2017-04-28 13:29 ` Guenter Roeck
@ 2017-05-02 4:35 ` Shilpasri G Bhat
2017-05-02 5:04 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Shilpasri G Bhat @ 2017-05-02 4:35 UTC (permalink / raw)
To: Guenter Roeck, jdelvare, paulus, mpe
Cc: linux-hwmon, linuxppc-dev, linux-kernel, svaidy, ego,
akshay.adiga, andrew, clg
Hi Guenter,
On 04/28/2017 06:59 PM, Guenter Roeck wrote:
> On 04/27/2017 10:59 PM, Shilpasri G Bhat wrote:
>> Add support for adding min/max values for the inband sensors copied by
>> OCC to main memory. And also add current(mA) sensors to the list.
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>> Changes from V1:
>> - Add functions to get min and max attribute strings
>> - Add function 'populate_sensor' to fill in the 'struct sensor_data'
>> for each sensor.
>>
>> drivers/hwmon/ibmpowernv.c | 96 +++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 77 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>> index 6d2e660..d59262c 100644
>> --- a/drivers/hwmon/ibmpowernv.c
>> +++ b/drivers/hwmon/ibmpowernv.c
>> @@ -50,6 +50,7 @@ enum sensors {
>> TEMP,
>> POWER_SUPPLY,
>> POWER_INPUT,
>> + CURRENT,
>> MAX_SENSOR_TYPE,
>> };
>>
>> @@ -65,7 +66,8 @@ enum sensors {
>> {"fan", "ibm,opal-sensor-cooling-fan"},
>> {"temp", "ibm,opal-sensor-amb-temp"},
>> {"in", "ibm,opal-sensor-power-supply"},
>> - {"power", "ibm,opal-sensor-power"}
>> + {"power", "ibm,opal-sensor-power"},
>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>
> Following up on a previous e-mail, this really _is_ odd. Any chance to fix it
> in the firmware and have current sensors return "ibm,opal-sensor-current" ?
Okay will drop "curr' sensor till we resolve this in the firmware.
>
>> };
>>
>> struct sensor_data {
>> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
>> opal = of_find_node_by_path("/ibm,opal/sensors");
>> for_each_child_of_node(opal, np) {
>> const char *label;
>> + int len;
>>
>> if (np->name == NULL)
>> continue;
>> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device
>> *pdev)
>> sensor_groups[type].attr_count++;
>>
>> /*
>> - * add a new attribute for labels
>> + * add attributes for labels, min and max
>> */
>> if (!of_property_read_string(np, "label", &label))
>> sensor_groups[type].attr_count++;
>> + if (of_find_property(np, "sensor-data-min", &len))
>> + sensor_groups[type].attr_count++;
>> + if (of_find_property(np, "sensor-data-max", &len))
>> + sensor_groups[type].attr_count++;
>> }
>>
>> of_node_put(opal);
>> @@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata,
>> const char *attr_name,
>> sdata->dev_attr.show = show;
>> }
>>
>> +static void populate_sensor(struct sensor_data *sdata, int od, int hd, int sid,
>> + const char *attr_name, enum sensors type,
>> + const struct attribute_group *pgroup,
>> + ssize_t (*show)(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf))
>> +{
>> + sdata->id = sid;
>> + sdata->type = type;
>> + sdata->opal_index = od;
>> + sdata->hwmon_index = hd;
>> + create_hwmon_attr(sdata, attr_name, show);
>> + pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
>> +}
>> +
>> +static char *get_max_attr(enum sensors type)
>> +{
>> + switch (type) {
>> + case POWER_INPUT:
>> + return "input_highest";
>> + case TEMP:
>> + return "max";
>> + default:
>> + break;
>> + }
>> +
>> + return "highest";
>
> This is a bit confusing. Why not 'return "highest";' in the default case above ?
>
> Also, is this correct for type == POWER_SUPPLY, ie is it "highest" vs. "max" ?
>
> Kind of odd that the firmware reports "highest/lowest" in some cases
> and "max/min" in others. Guess there is nothing we can do about that,
> just a note.
Firmware has min/max values for all sensors, but I had mapped them as
highest/lowest to add reset_history attribute in the later patches.
Will change "TEMP" to highest/lowest to keep consistency.
Thanks and Regards,
Shilpa
>
>> +}
>> +
>> +static char *get_min_attr(enum sensors type)
>> +{
>> + switch (type) {
>> + case POWER_INPUT:
>> + return "input_lowest";
>> + case TEMP:
>> + return "min";
>> + default:
>> + break;
>> + }
>> +
>> + return "lowest";
>
> Same here.
>
>> +}
>> +
>> /*
>> * Iterate through the device tree for each child of 'sensors' node, create
>> * a sysfs attribute file, the file is named by translating the DT node name
>> @@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device *pdev)
>> for_each_child_of_node(opal, np) {
>> const char *attr_name;
>> u32 opal_index;
>> + u32 hwmon_index;
>> const char *label;
>>
>> if (np->name == NULL)
>> @@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device *pdev)
>> continue;
>> }
>>
>> - sdata[count].id = sensor_id;
>> - sdata[count].type = type;
>> -
>> /*
>> * If we can not parse the node name, it means we are
>> * running on a newer device tree. We can just forget
>> @@ -401,14 +449,12 @@ static int create_device_attrs(struct platform_device
>> *pdev)
>> opal_index = INVALID_INDEX;
>> }
>>
>> - sdata[count].opal_index = opal_index;
>> - sdata[count].hwmon_index =
>> - get_sensor_hwmon_index(&sdata[count], sdata, count);
>> -
>> - create_hwmon_attr(&sdata[count], attr_name, show_sensor);
>> -
>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>> - &sdata[count++].dev_attr.attr;
>> + hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata,
>> + count);
>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>> + sensor_id, attr_name, type, pgroups[type],
>> + show_sensor);
>> + count++;
>>
>> if (!of_property_read_string(np, "label", &label)) {
>> /*
>> @@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device
>> *pdev)
>> * attribute. They are related to the same
>> * sensor.
>> */
>> - sdata[count].type = type;
>> - sdata[count].opal_index = sdata[count - 1].opal_index;
>> - sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>>
>> make_sensor_label(np, &sdata[count], label);
>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>> + sensor_id, "label", type, pgroups[type],
>> + show_label);
>> + count++;
>> + }
>>
>> - create_hwmon_attr(&sdata[count], "label", show_label);
>> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
>> + attr_name = get_max_attr(type);
>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>> + sensor_id, attr_name, type,
>> + pgroups[type], show_sensor);
>> + count++;
>> + }
>>
>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>> - &sdata[count++].dev_attr.attr;
>> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
>> + attr_name = get_min_attr(type);
>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>> + sensor_id, attr_name, type,
>> + pgroups[type], show_sensor);
>> + count++;
>> }
>> }
>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors
2017-05-02 4:35 ` Shilpasri G Bhat
@ 2017-05-02 5:04 ` Guenter Roeck
2017-05-02 5:48 ` Shilpasri G Bhat
0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2017-05-02 5:04 UTC (permalink / raw)
To: Shilpasri G Bhat, jdelvare, paulus, mpe
Cc: linux-hwmon, linuxppc-dev, linux-kernel, svaidy, ego,
akshay.adiga, andrew, clg
On 05/01/2017 09:35 PM, Shilpasri G Bhat wrote:
> Hi Guenter,
>
> On 04/28/2017 06:59 PM, Guenter Roeck wrote:
>> On 04/27/2017 10:59 PM, Shilpasri G Bhat wrote:
>>> Add support for adding min/max values for the inband sensors copied by
>>> OCC to main memory. And also add current(mA) sensors to the list.
>>>
>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>> ---
>>> Changes from V1:
>>> - Add functions to get min and max attribute strings
>>> - Add function 'populate_sensor' to fill in the 'struct sensor_data'
>>> for each sensor.
>>>
>>> drivers/hwmon/ibmpowernv.c | 96 +++++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 77 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>> index 6d2e660..d59262c 100644
>>> --- a/drivers/hwmon/ibmpowernv.c
>>> +++ b/drivers/hwmon/ibmpowernv.c
>>> @@ -50,6 +50,7 @@ enum sensors {
>>> TEMP,
>>> POWER_SUPPLY,
>>> POWER_INPUT,
>>> + CURRENT,
>>> MAX_SENSOR_TYPE,
>>> };
>>>
>>> @@ -65,7 +66,8 @@ enum sensors {
>>> {"fan", "ibm,opal-sensor-cooling-fan"},
>>> {"temp", "ibm,opal-sensor-amb-temp"},
>>> {"in", "ibm,opal-sensor-power-supply"},
>>> - {"power", "ibm,opal-sensor-power"}
>>> + {"power", "ibm,opal-sensor-power"},
>>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>>
>> Following up on a previous e-mail, this really _is_ odd. Any chance to fix it
>> in the firmware and have current sensors return "ibm,opal-sensor-current" ?
>
> Okay will drop "curr' sensor till we resolve this in the firmware.
>
>>
>>> };
>>>
>>> struct sensor_data {
>>> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev)
>>> opal = of_find_node_by_path("/ibm,opal/sensors");
>>> for_each_child_of_node(opal, np) {
>>> const char *label;
>>> + int len;
>>>
>>> if (np->name == NULL)
>>> continue;
>>> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device
>>> *pdev)
>>> sensor_groups[type].attr_count++;
>>>
>>> /*
>>> - * add a new attribute for labels
>>> + * add attributes for labels, min and max
>>> */
>>> if (!of_property_read_string(np, "label", &label))
>>> sensor_groups[type].attr_count++;
>>> + if (of_find_property(np, "sensor-data-min", &len))
>>> + sensor_groups[type].attr_count++;
>>> + if (of_find_property(np, "sensor-data-max", &len))
>>> + sensor_groups[type].attr_count++;
>>> }
>>>
>>> of_node_put(opal);
>>> @@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata,
>>> const char *attr_name,
>>> sdata->dev_attr.show = show;
>>> }
>>>
>>> +static void populate_sensor(struct sensor_data *sdata, int od, int hd, int sid,
>>> + const char *attr_name, enum sensors type,
>>> + const struct attribute_group *pgroup,
>>> + ssize_t (*show)(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf))
>>> +{
>>> + sdata->id = sid;
>>> + sdata->type = type;
>>> + sdata->opal_index = od;
>>> + sdata->hwmon_index = hd;
>>> + create_hwmon_attr(sdata, attr_name, show);
>>> + pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
>>> +}
>>> +
>>> +static char *get_max_attr(enum sensors type)
>>> +{
>>> + switch (type) {
>>> + case POWER_INPUT:
>>> + return "input_highest";
>>> + case TEMP:
>>> + return "max";
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return "highest";
>>
>> This is a bit confusing. Why not 'return "highest";' in the default case above ?
>>
>> Also, is this correct for type == POWER_SUPPLY, ie is it "highest" vs. "max" ?
>>
>> Kind of odd that the firmware reports "highest/lowest" in some cases
>> and "max/min" in others. Guess there is nothing we can do about that,
>> just a note.
>
> Firmware has min/max values for all sensors, but I had mapped them as
> highest/lowest to add reset_history attribute in the later patches.
>
Not sure I am parsing that correctly. Does the firmware report highest/lowest
or min/max ? reset_history only makes sense if the chip supports highest/lowest.
In case you plan to implement highest/lowest in software ... please don't.
Highest/lowest attributes must only be provided if the history is delivered
by the chip. If the chip does not support highest/lowest values, user
space can implement it, but we definitely don't want any workers or
similar in the kernel to do it.
Thanks,
Guenter
> Will change "TEMP" to highest/lowest to keep consistency.
>
> Thanks and Regards,
> Shilpa
>
>>
>>> +}
>>> +
>>> +static char *get_min_attr(enum sensors type)
>>> +{
>>> + switch (type) {
>>> + case POWER_INPUT:
>>> + return "input_lowest";
>>> + case TEMP:
>>> + return "min";
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return "lowest";
>>
>> Same here.
>>
>>> +}
>>> +
>>> /*
>>> * Iterate through the device tree for each child of 'sensors' node, create
>>> * a sysfs attribute file, the file is named by translating the DT node name
>>> @@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device *pdev)
>>> for_each_child_of_node(opal, np) {
>>> const char *attr_name;
>>> u32 opal_index;
>>> + u32 hwmon_index;
>>> const char *label;
>>>
>>> if (np->name == NULL)
>>> @@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device *pdev)
>>> continue;
>>> }
>>>
>>> - sdata[count].id = sensor_id;
>>> - sdata[count].type = type;
>>> -
>>> /*
>>> * If we can not parse the node name, it means we are
>>> * running on a newer device tree. We can just forget
>>> @@ -401,14 +449,12 @@ static int create_device_attrs(struct platform_device
>>> *pdev)
>>> opal_index = INVALID_INDEX;
>>> }
>>>
>>> - sdata[count].opal_index = opal_index;
>>> - sdata[count].hwmon_index =
>>> - get_sensor_hwmon_index(&sdata[count], sdata, count);
>>> -
>>> - create_hwmon_attr(&sdata[count], attr_name, show_sensor);
>>> -
>>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>>> - &sdata[count++].dev_attr.attr;
>>> + hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata,
>>> + count);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, attr_name, type, pgroups[type],
>>> + show_sensor);
>>> + count++;
>>>
>>> if (!of_property_read_string(np, "label", &label)) {
>>> /*
>>> @@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device
>>> *pdev)
>>> * attribute. They are related to the same
>>> * sensor.
>>> */
>>> - sdata[count].type = type;
>>> - sdata[count].opal_index = sdata[count - 1].opal_index;
>>> - sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>>>
>>> make_sensor_label(np, &sdata[count], label);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, "label", type, pgroups[type],
>>> + show_label);
>>> + count++;
>>> + }
>>>
>>> - create_hwmon_attr(&sdata[count], "label", show_label);
>>> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
>>> + attr_name = get_max_attr(type);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, attr_name, type,
>>> + pgroups[type], show_sensor);
>>> + count++;
>>> + }
>>>
>>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>>> - &sdata[count++].dev_attr.attr;
>>> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
>>> + attr_name = get_min_attr(type);
>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>> + sensor_id, attr_name, type,
>>> + pgroups[type], show_sensor);
>>> + count++;
>>> }
>>> }
>>>
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors
2017-05-02 5:04 ` Guenter Roeck
@ 2017-05-02 5:48 ` Shilpasri G Bhat
0 siblings, 0 replies; 5+ messages in thread
From: Shilpasri G Bhat @ 2017-05-02 5:48 UTC (permalink / raw)
To: Guenter Roeck, jdelvare, paulus, mpe
Cc: linux-hwmon, linuxppc-dev, linux-kernel, svaidy, ego,
akshay.adiga, andrew, clg
On 05/02/2017 10:34 AM, Guenter Roeck wrote:
> On 05/01/2017 09:35 PM, Shilpasri G Bhat wrote:
>> Hi Guenter,
>>
>> On 04/28/2017 06:59 PM, Guenter Roeck wrote:
>>> On 04/27/2017 10:59 PM, Shilpasri G Bhat wrote:
>>>> Add support for adding min/max values for the inband sensors copied by
>>>> OCC to main memory. And also add current(mA) sensors to the list.
>>>>
>>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>>> ---
>>>> Changes from V1:
>>>> - Add functions to get min and max attribute strings
>>>> - Add function 'populate_sensor' to fill in the 'struct sensor_data'
>>>> for each sensor.
>>>>
>>>> drivers/hwmon/ibmpowernv.c | 96 +++++++++++++++++++++++++++++++++++++---------
>>>> 1 file changed, 77 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
>>>> index 6d2e660..d59262c 100644
>>>> --- a/drivers/hwmon/ibmpowernv.c
>>>> +++ b/drivers/hwmon/ibmpowernv.c
>>>> @@ -50,6 +50,7 @@ enum sensors {
>>>> TEMP,
>>>> POWER_SUPPLY,
>>>> POWER_INPUT,
>>>> + CURRENT,
>>>> MAX_SENSOR_TYPE,
>>>> };
>>>>
>>>> @@ -65,7 +66,8 @@ enum sensors {
>>>> {"fan", "ibm,opal-sensor-cooling-fan"},
>>>> {"temp", "ibm,opal-sensor-amb-temp"},
>>>> {"in", "ibm,opal-sensor-power-supply"},
>>>> - {"power", "ibm,opal-sensor-power"}
>>>> + {"power", "ibm,opal-sensor-power"},
>>>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */
>>>
>>> Following up on a previous e-mail, this really _is_ odd. Any chance to fix it
>>> in the firmware and have current sensors return "ibm,opal-sensor-current" ?
>>
>> Okay will drop "curr' sensor till we resolve this in the firmware.
>>
>>>
>>>> };
>>>>
>>>> struct sensor_data {
>>>> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device
>>>> *pdev)
>>>> opal = of_find_node_by_path("/ibm,opal/sensors");
>>>> for_each_child_of_node(opal, np) {
>>>> const char *label;
>>>> + int len;
>>>>
>>>> if (np->name == NULL)
>>>> continue;
>>>> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device
>>>> *pdev)
>>>> sensor_groups[type].attr_count++;
>>>>
>>>> /*
>>>> - * add a new attribute for labels
>>>> + * add attributes for labels, min and max
>>>> */
>>>> if (!of_property_read_string(np, "label", &label))
>>>> sensor_groups[type].attr_count++;
>>>> + if (of_find_property(np, "sensor-data-min", &len))
>>>> + sensor_groups[type].attr_count++;
>>>> + if (of_find_property(np, "sensor-data-max", &len))
>>>> + sensor_groups[type].attr_count++;
>>>> }
>>>>
>>>> of_node_put(opal);
>>>> @@ -337,6 +344,49 @@ static void create_hwmon_attr(struct sensor_data *sdata,
>>>> const char *attr_name,
>>>> sdata->dev_attr.show = show;
>>>> }
>>>>
>>>> +static void populate_sensor(struct sensor_data *sdata, int od, int hd, int
>>>> sid,
>>>> + const char *attr_name, enum sensors type,
>>>> + const struct attribute_group *pgroup,
>>>> + ssize_t (*show)(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf))
>>>> +{
>>>> + sdata->id = sid;
>>>> + sdata->type = type;
>>>> + sdata->opal_index = od;
>>>> + sdata->hwmon_index = hd;
>>>> + create_hwmon_attr(sdata, attr_name, show);
>>>> + pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
>>>> +}
>>>> +
>>>> +static char *get_max_attr(enum sensors type)
>>>> +{
>>>> + switch (type) {
>>>> + case POWER_INPUT:
>>>> + return "input_highest";
>>>> + case TEMP:
>>>> + return "max";
>>>> + default:
>>>> + break;
>>>> + }
>>>> +
>>>> + return "highest";
>>>
>>> This is a bit confusing. Why not 'return "highest";' in the default case above ?
>>>
>>> Also, is this correct for type == POWER_SUPPLY, ie is it "highest" vs. "max" ?
>>>
>>> Kind of odd that the firmware reports "highest/lowest" in some cases
>>> and "max/min" in others. Guess there is nothing we can do about that,
>>> just a note.
>>
>> Firmware has min/max values for all sensors, but I had mapped them as
>> highest/lowest to add reset_history attribute in the later patches.
>>
>
> Not sure I am parsing that correctly. Does the firmware report highest/lowest
> or min/max ? reset_history only makes sense if the chip supports highest/lowest.
Firmware currently records the minimum and maximum value for the sensor readings
while sampling the sensor. In the current boot it gives the min/max values of
the sensor readings. Firmware also supports mechanism to reset these min/max
values for the sensor.
So does this make a good candidate for highest/lowest ?
>
> In case you plan to implement highest/lowest in software ... please don't.
> Highest/lowest attributes must only be provided if the history is delivered
> by the chip. If the chip does not support highest/lowest values, user
> space can implement it, but we definitely don't want any workers or
> similar in the kernel to do it.
>
> Thanks,
> Guenter
>
>> Will change "TEMP" to highest/lowest to keep consistency.
>>
>> Thanks and Regards,
>> Shilpa
>>
>>>
>>>> +}
>>>> +
>>>> +static char *get_min_attr(enum sensors type)
>>>> +{
>>>> + switch (type) {
>>>> + case POWER_INPUT:
>>>> + return "input_lowest";
>>>> + case TEMP:
>>>> + return "min";
>>>> + default:
>>>> + break;
>>>> + }
>>>> +
>>>> + return "lowest";
>>>
>>> Same here.
>>>
>>>> +}
>>>> +
>>>> /*
>>>> * Iterate through the device tree for each child of 'sensors' node, create
>>>> * a sysfs attribute file, the file is named by translating the DT node name
>>>> @@ -365,6 +415,7 @@ static int create_device_attrs(struct platform_device
>>>> *pdev)
>>>> for_each_child_of_node(opal, np) {
>>>> const char *attr_name;
>>>> u32 opal_index;
>>>> + u32 hwmon_index;
>>>> const char *label;
>>>>
>>>> if (np->name == NULL)
>>>> @@ -386,9 +437,6 @@ static int create_device_attrs(struct platform_device
>>>> *pdev)
>>>> continue;
>>>> }
>>>>
>>>> - sdata[count].id = sensor_id;
>>>> - sdata[count].type = type;
>>>> -
>>>> /*
>>>> * If we can not parse the node name, it means we are
>>>> * running on a newer device tree. We can just forget
>>>> @@ -401,14 +449,12 @@ static int create_device_attrs(struct platform_device
>>>> *pdev)
>>>> opal_index = INVALID_INDEX;
>>>> }
>>>>
>>>> - sdata[count].opal_index = opal_index;
>>>> - sdata[count].hwmon_index =
>>>> - get_sensor_hwmon_index(&sdata[count], sdata, count);
>>>> -
>>>> - create_hwmon_attr(&sdata[count], attr_name, show_sensor);
>>>> -
>>>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>>>> - &sdata[count++].dev_attr.attr;
>>>> + hwmon_index = get_sensor_hwmon_index(&sdata[count], sdata,
>>>> + count);
>>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>>> + sensor_id, attr_name, type, pgroups[type],
>>>> + show_sensor);
>>>> + count++;
>>>>
>>>> if (!of_property_read_string(np, "label", &label)) {
>>>> /*
>>>> @@ -417,16 +463,28 @@ static int create_device_attrs(struct platform_device
>>>> *pdev)
>>>> * attribute. They are related to the same
>>>> * sensor.
>>>> */
>>>> - sdata[count].type = type;
>>>> - sdata[count].opal_index = sdata[count - 1].opal_index;
>>>> - sdata[count].hwmon_index = sdata[count - 1].hwmon_index;
>>>>
>>>> make_sensor_label(np, &sdata[count], label);
>>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>>> + sensor_id, "label", type, pgroups[type],
>>>> + show_label);
>>>> + count++;
>>>> + }
>>>>
>>>> - create_hwmon_attr(&sdata[count], "label", show_label);
>>>> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
>>>> + attr_name = get_max_attr(type);
>>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>>> + sensor_id, attr_name, type,
>>>> + pgroups[type], show_sensor);
>>>> + count++;
>>>> + }
>>>>
>>>> - pgroups[type]->attrs[sensor_groups[type].attr_count++] =
>>>> - &sdata[count++].dev_attr.attr;
>>>> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
>>>> + attr_name = get_min_attr(type);
>>>> + populate_sensor(&sdata[count], opal_index, hwmon_index,
>>>> + sensor_id, attr_name, type,
>>>> + pgroups[type], show_sensor);
>>>> + count++;
>>>> }
>>>> }
>>>>
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-02 5:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-28 5:59 [PATCH V2] hwmon: (ibmpowernv) Add min/max attributes and current sensors Shilpasri G Bhat
2017-04-28 13:29 ` Guenter Roeck
2017-05-02 4:35 ` Shilpasri G Bhat
2017-05-02 5:04 ` Guenter Roeck
2017-05-02 5:48 ` Shilpasri G Bhat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).