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

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

2017-05-01 Thread Shilpasri G Bhat



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 
 ---
 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", ))
  sensor_groups[type].attr_count++;
 +if (of_find_property(np, "sensor-data-min", ))
 +sensor_groups[type].attr_count++;
 +if (of_find_property(np, "sensor-data-max", ))
 +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++] = 
 >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

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

2017-05-01 Thread Guenter Roeck


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 
---
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", ))
 sensor_groups[type].attr_count++;
+if (of_find_property(np, "sensor-data-min", ))
+sensor_groups[type].attr_count++;
+if (of_find_property(np, "sensor-data-max", ))
+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++] = >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;

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

2017-05-01 Thread Shilpasri G Bhat

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 
>> ---
>> 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", ))
>>  sensor_groups[type].attr_count++;
>> +if (of_find_property(np, "sensor-data-min", ))
>> +sensor_groups[type].attr_count++;
>> +if (of_find_property(np, "sensor-data-max", ))
>> +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++] = >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;
>>

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

2017-04-28 Thread Guenter Roeck


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 
---
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", ))
sensor_groups[type].attr_count++;
+   if (of_find_property(np, "sensor-data-min", ))
+   sensor_groups[type].attr_count++;
+   if (of_find_property(np, "sensor-data-max", ))
+   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++] = >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)

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information