[RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors

[RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors

[Shilpasri G Bhat]

Add support to read power and temperature sensors from OCC inband
sensors which are copied to main memory by OCC.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
CC: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
CC: Jean Delvare <jdelvare-IBi9RG/b67k@public.gmane.org>
CC: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
CC: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
CC: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-hwmon-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 .../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.
+ */
+
+#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>
+
+#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;							\
+	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;
+}
+
+static void ibmpowernv_occ_add_sdata(struct occ_hwmon_sensor sensor,
+				     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");
+
+		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;
+	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;
+
+	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;
+
+	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");
+
+	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-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>");
+MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors

[Guenter Roeck]

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 ?

Thanks,
Guenter

> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Mark Rutland <mark.rutland@arm.com>
> CC: Jean Delvare <jdelvare@suse.com>
> CC: Guenter Roeck <linux@roeck-us.net>
> CC: Jonathan Corbet <corbet@lwn.net>
> CC: devicetree@vger.kernel.org
> CC: linux-hwmon@vger.kernel.org
> CC: linux-doc@vger.kernel.org
> ---
>  .../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.
> + */
> +
> +#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>
> +
> +#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;							\
> +	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;
> +}
> +
> +static void ibmpowernv_occ_add_sdata(struct occ_hwmon_sensor sensor,
> +				     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");
> +
> +		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;
> +	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;
> +
> +	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;
> +
> +	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");
> +
> +	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@linux.vnet.ibm.com>");
> +MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.3.1
> 

Re: [RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors

[Shilpasri G Bhat]

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.

Thanks and Regards,
Shilpa

> Thanks,
> Guenter
> 
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> CC: Rob Herring <robh+dt@kernel.org>
>> CC: Mark Rutland <mark.rutland@arm.com>
>> CC: Jean Delvare <jdelvare@suse.com>
>> CC: Guenter Roeck <linux@roeck-us.net>
>> CC: Jonathan Corbet <corbet@lwn.net>
>> CC: devicetree@vger.kernel.org
>> CC: linux-hwmon@vger.kernel.org
>> CC: linux-doc@vger.kernel.org
>> ---
>>  .../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.
>> + */
>> +
>> +#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>
>> +
>> +#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;							\
>> +	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;
>> +}
>> +
>> +static void ibmpowernv_occ_add_sdata(struct occ_hwmon_sensor sensor,
>> +				     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");
>> +
>> +		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;
>> +	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;
>> +
>> +	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;
>> +
>> +	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");
>> +
>> +	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@linux.vnet.ibm.com>");
>> +MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 1.8.3.1
>>
> 

Re: [RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors

[Guenter Roeck]

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@linux.vnet.ibm.com>
>>> CC: Rob Herring <robh+dt@kernel.org>
>>> CC: Mark Rutland <mark.rutland@arm.com>
>>> CC: Jean Delvare <jdelvare@suse.com>
>>> CC: Guenter Roeck <linux@roeck-us.net>
>>> CC: Jonathan Corbet <corbet@lwn.net>
>>> CC: devicetree@vger.kernel.org
>>> CC: linux-hwmon@vger.kernel.orgThat
>>> CC: linux-doc@vger.kernel.org
>>> ---
>>>  .../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@linux.vnet.ibm.com>");
>>> +MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 1.8.3.1
>>>
>>
>
>