| Message ID | 1493359145-6143-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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++; > } > } > > -- 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
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++; >> } >> } >> >> > -- 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
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 > -- 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
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 >> > -- 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
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++; } }
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(-)