Re: [RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors
From: Guenter Roeck
Date: Sun Mar 12 2017 - 18:57:53 EST
On 03/09/2017 05:30 AM, Shilpasri G Bhat wrote:
Hi Guenter,
On 03/09/2017 05:40 PM, Guenter Roeck wrote:
On Thu, Mar 09, 2017 at 05:19:15PM +0530, Shilpasri G Bhat wrote:
Add support to read power and temperature sensors from OCC inband
sensors which are copied to main memory by OCC.
Is this supposed to be an alternative to the submission from
Eddie James ? If so, is there a reason to consider such an
alternative ?
This is not an alternative submission. Eddie James' hwmon OCC driver is to
export the sensors in BMC which is a service processor for POWER{8,9} servers.
This patch adds a hwmon driver in the POWER9 server.
Both are names ...occ... kind of difficult to understand what is what.
Either case, it appears that there are some dependencies to other code
which I was not copied on. that is not very helpful.
Please use the new hwmon API (devm_hwmon_device_register_with_info); this driver
seems to be a perfect candidate. Also, please do not use function macros;
those tend to obfuscate the code and make it hard to understand.
With the new API, such macros should not be necessary.
Couple of additional comments inline.
Thanks,
Guenter
Thanks and Regards,
Shilpa
Thanks,
Guenter
Signed-off-by: Shilpasri G Bhat <shilpa.bhat@xxxxxxxxxxxxxxxxxx>
CC: Rob Herring <robh+dt@xxxxxxxxxx>
CC: Mark Rutland <mark.rutland@xxxxxxx>
CC: Jean Delvare <jdelvare@xxxxxxxx>
CC: Guenter Roeck <linux@xxxxxxxxxxxx>
CC: Jonathan Corbet <corbet@xxxxxxx>
CC: devicetree@xxxxxxxxxxxxxxx
CC: linux-hwmon@xxxxxxxxxxxxxxxxxxx
CC: linux-doc@xxxxxxxxxxxxxxx
---
.../devicetree/bindings/hwmon/ibmpowernv-occ.txt | 4 +
Documentation/hwmon/ibmpowernv-occ | 24 ++
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/ibmpowernv-occ.c | 302 +++++++++++++++++++++
5 files changed, 342 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt
create mode 100644 Documentation/hwmon/ibmpowernv-occ
create mode 100644 drivers/hwmon/ibmpowernv-occ.c
diff --git a/Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt b/Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt
new file mode 100644
index 0000000..d03f744
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt
@@ -0,0 +1,4 @@
+IBM POWERNV OCC inband platform sensors
+
+Required device-tree property:
+- compatible: "ibm,p9-occ-inband-sensor"
diff --git a/Documentation/hwmon/ibmpowernv-occ b/Documentation/hwmon/ibmpowernv-occ
new file mode 100644
index 0000000..151028b
--- /dev/null
+++ b/Documentation/hwmon/ibmpowernv-occ
@@ -0,0 +1,24 @@
+Kernel driver ibmpowernv-occ
+=============================
+
+Supported systems:
+ * P9 server based on POWERNV platform
+
+Description
+------------
+
+This driver exports the power and temperature sensors from OCC inband
+sensors on P9 POWERNV platforms.
+
+Sysfs attributes
+----------------
+
+powerX_input Latest power reading
+powerX_input_highest Minimum power
+powerX_input_lowest Maximum power
+powerX_label Sensor name
+
+tempX_input Latest temperature reading
+tempX_max Minimum temperature
+tempX_min Maximum temperature
+tempX_label Sensor name
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3..3b1dbb9 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -598,6 +598,17 @@ config SENSORS_IBMPOWERNV
This driver can also be built as a module. If so, the module
will be called ibmpowernv.
+config SENSORS_IBMPOWERNV_OCC
+ tristate "IBM POWERNV OCC Inband platform sensors"
+ depends on PPC_POWERNV
+ default y
+ help
+ If you say yes here you get support for the temperature/power
+ OCC inband sensors on your PowerNV platform.
+
+ This driver can also be built as a module. If so, the module
+ will be called ibmpowernv-occ.
+
config SENSORS_IIO_HWMON
tristate "Hwmon driver that uses channels specified via iio maps"
depends on IIO
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 5509edf..0da2207 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o
obj-$(CONFIG_SENSORS_IBMAEM) += ibmaem.o
obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o
obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o
+obj-$(CONFIG_SENSORS_IBMPOWERNV_OCC)+= ibmpowernv-occ.o
obj-$(CONFIG_SENSORS_IIO_HWMON) += iio_hwmon.o
obj-$(CONFIG_SENSORS_INA209) += ina209.o
obj-$(CONFIG_SENSORS_INA2XX) += ina2xx.o
diff --git a/drivers/hwmon/ibmpowernv-occ.c b/drivers/hwmon/ibmpowernv-occ.c
new file mode 100644
index 0000000..97b1bbe
--- /dev/null
+++ b/drivers/hwmon/ibmpowernv-occ.c
@@ -0,0 +1,302 @@
+/*
+ * IBM PowerNV platform OCC inband sensors for temperature/power
+ * Copyright (C) 2017 IBM
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.
Unnecessary.
+ */
+
+#define DRVNAME "ibmpowernv_occ"
+#define pr_fmt(fmt) DRVNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/hwmon.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
alphabetic order please
+
+#include <asm/opal.h>
+
+#define MAX_HWMON_ATTR_LEN 32
+#define MAX_HWMON_LABEL_LEN (MAX_OCC_SENSOR_NAME_LEN * 2)
+#define HWMON_ATTRS_PER_SENSOR 16
+#define TO_MILLI_UNITS(x) ((x) * 1000)
+#define TO_MICRO_UNITS(x) ((x) * 1000000)
+
+enum sensors {
+ TEMP,
+ POWER,
+ MAX_SENSOR_TYPE,
+};
+
+static struct sensor_type {
+ const char *name;
+ int hwmon_id;
+} sensor_types[] = {
+ { "temp"},
+ { "power"},
+};
+
+static struct sensor_data {
+ u32 occ_id;
+ u64 offset; /* Offset to ping/pong reading buffer */
+ enum sensors type;
+ char label[MAX_HWMON_LABEL_LEN];
+ char name[MAX_HWMON_ATTR_LEN];
+ struct device_attribute attr;
+} *sdata;
+
+static struct attribute_group sensor_attrs_group;
+__ATTRIBUTE_GROUPS(sensor_attrs);
+
+#define show(file_name) \
+static ssize_t ibmpowernv_occ_show_##file_name \
+(struct device *dev, struct device_attribute *dattr, char *buf) \
+{ \
+ struct sensor_data *sdata = container_of(dattr, \
+ struct sensor_data, \
+ attr); \
+ u64 val; \
+ int ret; \
this does or should generate a checkpatch warning.
+ ret = opal_occ_sensor_get_##file_name(sdata->occ_id, \
+ sdata->offset, \
+ &val); \
+ if (ret) \
+ return ret; \
+ if (sdata->type == TEMP) \
+ val = TO_MILLI_UNITS(val); \
+ else if (sdata->type == POWER) \
+ val = TO_MICRO_UNITS(val); \
+ return sprintf(buf, "%llu\n", val); \
+}
+
+show(sample);
+show(max);
+show(min);
+show(js_min);
+show(js_max);
+show(csm_min);
+show(csm_max);
+show(prof_min);
+show(prof_max);
+
+static struct sensor_view_groups {
+ const char *name;
+ ssize_t (*show_sample)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf);
+ ssize_t (*show_min)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf);
+ ssize_t (*show_max)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf);
+} sensor_views[] = {
+ {
+ .name = "",
+ .show_sample = ibmpowernv_occ_show_sample,
+ .show_min = ibmpowernv_occ_show_min,
+ .show_max = ibmpowernv_occ_show_max
+ },
+ {
+ .name = "_JS",
+ .show_sample = ibmpowernv_occ_show_sample,
+ .show_min = ibmpowernv_occ_show_js_min,
+ .show_max = ibmpowernv_occ_show_js_max
+ },
+ { .name = "_CSM",
+ .show_sample = ibmpowernv_occ_show_sample,
+ .show_min = ibmpowernv_occ_show_csm_min,
+ .show_max = ibmpowernv_occ_show_csm_max
+ },
+ { .name = "_Prof",
+ .show_sample = ibmpowernv_occ_show_sample,
+ .show_min = ibmpowernv_occ_show_prof_min,
+ .show_max = ibmpowernv_occ_show_prof_max
+ },
+};
+
+static ssize_t ibmpowernv_occ_show_label(struct device *dev,
+ struct device_attribute *dattr,
+ char *buf)
+{
+ struct sensor_data *sdata = container_of(dattr, struct sensor_data,
+ attr);
+
+ return sprintf(buf, "%s\n", sdata->label);
+}
+
+static int ibmpowernv_occ_get_sensor_type(enum occ_sensor_type type)
+{
+ switch (type) {
+ case OCC_SENSOR_TYPE_POWER:
+ return POWER;
+ case OCC_SENSOR_TYPE_TEMPERATURE:
+ return TEMP;
+ default:
+ return MAX_SENSOR_TYPE;
+ }
+
+ return MAX_SENSOR_TYPE;
Never reached.
+}
+
+static void ibmpowernv_occ_add_sdata(struct occ_hwmon_sensor sensor,
Passing a data structure (instead of a pointer to it) as parameter is quite
unusual. Is this really needed ? I don't really see the point.
+ struct sensor_data *sdata, char *name,
+ int hwmon_id, enum sensors type,
+ ssize_t (*show)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf))
+{
+ sdata->type = type;
+ sdata->occ_id = sensor.occ_id;
+ sdata->offset = sensor.offset;
+ snprintf(sdata->name, MAX_HWMON_ATTR_LEN, "%s%d_%s",
+ sensor_types[type].name, hwmon_id, name);
+ sysfs_attr_init(&sdata->attr.attr);
+ sdata->attr.attr.name = sdata->name;
+ sdata->attr.attr.mode = 0444;
+ sdata->attr.show = show;
+}
+
+static void ibmpowernv_occ_add_sensor_attrs(struct occ_hwmon_sensor sensor,
+ int index)
+{
+ struct attribute **attrs = sensor_attrs_group.attrs;
+ char attr_str[MAX_HWMON_ATTR_LEN];
+ enum sensors type = ibmpowernv_occ_get_sensor_type(sensor.type);
+ int i;
+
+ index *= HWMON_ATTRS_PER_SENSOR;
+ for (i = 0; i < ARRAY_SIZE(sensor_views); i++) {
+ int hid = ++sensor_types[type].hwmon_id;
+
+ /* input */
+ ibmpowernv_occ_add_sdata(sensor, &sdata[index], "input", hid,
+ type, sensor_views[i].show_sample);
+ attrs[index] = &sdata[index].attr.attr;
+ index++;
+
+ /* min */
+ if (type == POWER)
+ snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s",
+ "input_lowest");
+ else
+ snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s", "min");
+
+ ibmpowernv_occ_add_sdata(sensor, &sdata[index], attr_str, hid,
+ type, sensor_views[i].show_min);
+ attrs[index] = &sdata[index].attr.attr;
+ index++;
+
+ /* max */
+ if (type == POWER)
+ snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s",
+ "input_highest");
+ else
+ snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s", "max");
I don't really see the point of those snprintf()s and for having attr_str[]
(instead of char *attr_str).
+
+ ibmpowernv_occ_add_sdata(sensor, &sdata[index], attr_str, hid,
+ type, sensor_views[i].show_max);
+ attrs[index] = &sdata[index].attr.attr;
+ index++;
+
+ /* label */
+ snprintf(sdata[index].label, MAX_HWMON_LABEL_LEN, "%s%s",
+ sensor.name, sensor_views[i].name);
+ ibmpowernv_occ_add_sdata(sensor, &sdata[index], "label", hid,
+ type, ibmpowernv_occ_show_label);
+ attrs[index] = &sdata[index].attr.attr;
+ index++;
+ }
+}
+
+static int ibmpowernv_occ_add_device_attrs(struct platform_device *pdev)
+{
+ struct attribute **attrs;
+ struct occ_hwmon_sensor *slist = NULL;
Unnecessary initialization.
+ int nr_sensors = 0, i;
+ int ret = -ENOMEM;
+
+ slist = opal_occ_sensor_get_hwmon_list(&nr_sensors);
+ if (!nr_sensors)
+ return -ENODEV;
+
+ if (!slist)
+ return ret;
+
is nr_sensors guaranteed to be valid if slist is NULL ?
+ sdata = devm_kzalloc(&pdev->dev, nr_sensors * sizeof(*sdata) *
+ HWMON_ATTRS_PER_SENSOR, GFP_KERNEL);
+ if (!sdata)
+ goto out;
+
+ attrs = devm_kzalloc(&pdev->dev, nr_sensors * sizeof(*attrs) *
+ HWMON_ATTRS_PER_SENSOR, GFP_KERNEL);
+ if (!attrs)
+ goto out;
+
+ sensor_attrs_group.attrs = attrs;
+ for (i = 0; i < nr_sensors; i++)
+ ibmpowernv_occ_add_sensor_attrs(slist[i], i);
+
+ ret = 0;
+out:
+ kfree(slist);
+ return ret;
+}
+
+static int ibmpowernv_occ_probe(struct platform_device *pdev)
+{
+ struct device *hwmon_dev;
+ int err;
+
+ err = ibmpowernv_occ_add_device_attrs(pdev);
+ if (err)
+ goto out;
+
For -ENODEV and -ENOMEM you don't really want/need an error message.
+ hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev, DRVNAME,
+ NULL,
+ sensor_attrs_groups);
+
+ err = PTR_ERR_OR_ZERO(hwmon_dev);
+out:
+ if (err)
+ pr_warn("Failed to initialize Hwmon OCC inband sensors\n");
dev_warn() if this is really neceessary. I would suggest a more specific
error message, though. This doesn't tell if ibmpowernv_occ_add_device_attrs()
or devm_hwmon_device_register_with_groups() failed.
+
+ return err;
+}
+
+static const struct platform_device_id occ_sensor_ids[] = {
+ { .name = "occ-inband-sensor" },
+ { }
+};
+MODULE_DEVICE_TABLE(platform, occ_sensor_ids);
+
+static const struct of_device_id occ_sensor_of_ids[] = {
+ { .compatible = "ibm,p9-occ-inband-sensor" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, occ_sensor_of_ids);
+
+static struct platform_driver ibmpowernv_occ_driver = {
+ .probe = ibmpowernv_occ_probe,
+ .id_table = occ_sensor_ids,
+ .driver = {
+ .name = DRVNAME,
+ .of_match_table = occ_sensor_of_ids,
+ },
+};
+
+module_platform_driver(ibmpowernv_occ_driver);
+
+MODULE_AUTHOR("Shilpasri G Bhat <shilpa.bhat@xxxxxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors");
+MODULE_LICENSE("GPL");
--
1.8.3.1