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

[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 <shilpa.bhat@xxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>