[PATCH v2 0/2] hwmon: Add attributes to enable/disable sensors

[PATCH v2 0/2] hwmon: Add attributes to enable/disable sensors

[Shilpasri G Bhat]

This patch series adds new attribute to enable or disable a sensor in
runtime.

v1 : https://lkml.org/lkml/2018/3/22/214

Shilpasri G Bhat (2):
  powernv:opal-sensor-groups: Add support to enable sensor groups
  hwmon: ibmpowernv: Add attributes to enable/disable sensor groups

 Documentation/hwmon/sysfs-interface                |  22 ++
 arch/powerpc/include/asm/opal-api.h                |   1 +
 arch/powerpc/include/asm/opal.h                    |   2 +
 .../powerpc/platforms/powernv/opal-sensor-groups.c |  28 ++
 arch/powerpc/platforms/powernv/opal-wrappers.S     |   1 +
 drivers/hwmon/ibmpowernv.c                         | 281 ++++++++++++++++++---
 6 files changed, 296 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] powernv:opal-sensor-groups: Add support to enable sensor groups

[Shilpasri G Bhat]

Adds support to enable/disable a sensor group at runtime. This
can be used to select the sensor groups that needs to be copied to
main memory by OCC. Sensor groups like power, temperature, current,
voltage, frequency, utilization can be enabled/disabled at runtime.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
- Rebased on master. No changes from v1.

 arch/powerpc/include/asm/opal-api.h                |  1 +
 arch/powerpc/include/asm/opal.h                    |  2 ++
 .../powerpc/platforms/powernv/opal-sensor-groups.c | 28 ++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal-wrappers.S     |  1 +
 4 files changed, 32 insertions(+)

diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index 3bab299..56a94a1 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -206,6 +206,7 @@
 #define OPAL_NPU_SPA_CLEAR_CACHE		160
 #define OPAL_NPU_TL_SET				161
 #define OPAL_SENSOR_READ_U64			162
+#define OPAL_SENSOR_GROUP_ENABLE		163
 #define OPAL_PCI_GET_PBCQ_TUNNEL_BAR		164
 #define OPAL_PCI_SET_PBCQ_TUNNEL_BAR		165
 #define OPAL_LAST				165
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index e1b2910..fc0550e 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -292,6 +292,7 @@ int64_t opal_imc_counters_init(uint32_t type, uint64_t address,
 int opal_get_power_shift_ratio(u32 handle, int token, u32 *psr);
 int opal_set_power_shift_ratio(u32 handle, int token, u32 psr);
 int opal_sensor_group_clear(u32 group_hndl, int token);
+int opal_sensor_group_enable(u32 group_hndl, int token, bool enable);
 
 s64 opal_signal_system_reset(s32 cpu);
 s64 opal_quiesce(u64 shutdown_type, s32 cpu);
@@ -326,6 +327,7 @@ extern int opal_async_wait_response_interruptible(uint64_t token,
 		struct opal_msg *msg);
 extern int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data);
 extern int opal_get_sensor_data_u64(u32 sensor_hndl, u64 *sensor_data);
+extern int sensor_group_enable(u32 grp_hndl, bool enable);
 
 struct rtc_time;
 extern time64_t opal_get_boot_time(void);
diff --git a/arch/powerpc/platforms/powernv/opal-sensor-groups.c b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
index 541c9ea..f7d04b6 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor-groups.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor-groups.c
@@ -32,6 +32,34 @@ struct sg_attr {
 	struct sg_attr *sgattrs;
 } *sgs;
 
+int sensor_group_enable(u32 handle, bool enable)
+{
+	struct opal_msg msg;
+	int token, ret;
+
+	token = opal_async_get_token_interruptible();
+	if (token < 0)
+		return token;
+
+	ret = opal_sensor_group_enable(handle, token, enable);
+	if (ret == OPAL_ASYNC_COMPLETION) {
+		ret = opal_async_wait_response(token, &msg);
+		if (ret) {
+			pr_devel("Failed to wait for the async response\n");
+			ret = -EIO;
+			goto out;
+		}
+		ret = opal_error_code(opal_get_async_rc(msg));
+	} else {
+		ret = opal_error_code(ret);
+	}
+
+out:
+	opal_async_release_token(token);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(sensor_group_enable);
+
 static ssize_t sg_store(struct kobject *kobj, struct kobj_attribute *attr,
 			const char *buf, size_t count)
 {
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index a8d9b40..8268a1e 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -327,3 +327,4 @@ OPAL_CALL(opal_npu_tl_set,			OPAL_NPU_TL_SET);
 OPAL_CALL(opal_pci_get_pbcq_tunnel_bar,		OPAL_PCI_GET_PBCQ_TUNNEL_BAR);
 OPAL_CALL(opal_pci_set_pbcq_tunnel_bar,		OPAL_PCI_SET_PBCQ_TUNNEL_BAR);
 OPAL_CALL(opal_sensor_read_u64,			OPAL_SENSOR_READ_U64);
+OPAL_CALL(opal_sensor_group_enable,		OPAL_SENSOR_GROUP_ENABLE);
-- 
1.8.3.1

[PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups

[Shilpasri G Bhat]

On-Chip-Controller(OCC) is an embedded micro-processor in POWER9 chip
which measures various system and chip level sensors. These sensors
comprises of environmental sensors (like power, temperature, current
and voltage) and performance sensors (like utilization, frequency).
All these sensors are copied to main memory at a regular interval of
100ms. OCC provides a way to select a group of sensors that is copied
to the main memory to increase the update frequency of selected sensor
groups. When a sensor-group is disabled, OCC will not copy it to main
memory and those sensors read 0 values.

This patch provides support for enabling/disabling the sensor groups
like power, temperature, current and voltage. This patch adds new
per-senor sysfs attribute to disable and enable them.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
Changes from v1:
- Add per-sensor 'enable' attribute
- Return -ENODATA when sensor is disabled

 Documentation/hwmon/sysfs-interface |  22 +++
 drivers/hwmon/ibmpowernv.c          | 281 +++++++++++++++++++++++++++++++-----
 2 files changed, 264 insertions(+), 39 deletions(-)

diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
index fc337c3..38ab05c 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -184,6 +184,11 @@ vrm		Voltage Regulator Module version number.
 		Affects the way the driver calculates the CPU core reference
 		voltage from the vid pins.
 
+in[0-*]_enable	Enable or disable the sensor
+		1 : Enable
+		0 : Disable
+		RW
+
 Also see the Alarms section for status flags associated with voltages.
 
 
@@ -409,6 +414,12 @@ temp_reset_history
 		Reset temp_lowest and temp_highest for all sensors
 		WO
 
+temp[1-*]_enable
+		Enable or disable the sensor
+		1 : Enable
+		0 : Disable
+		RW
+
 Some chips measure temperature using external thermistors and an ADC, and
 report the temperature measurement as a voltage. Converting this voltage
 back to a temperature (or the other way around for limits) requires
@@ -468,6 +479,12 @@ curr_reset_history
 		Reset currX_lowest and currX_highest for all sensors
 		WO
 
+curr[1-*]_enable
+		Enable or disable the sensor
+		1 : Enable
+		0 : Disable
+		RW
+
 Also see the Alarms section for status flags associated with currents.
 
 *********
@@ -566,6 +583,11 @@ power[1-*]_crit			Critical maximum power.
 				Unit: microWatt
 				RW
 
+power[1-*]_enable		Enable or disable the sensor
+				1 : Enable
+				0 : Disable
+				RW
+
 Also see the Alarms section for status flags associated with power readings.
 
 **********
diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index f829dad..61e04cf 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -90,8 +90,28 @@ struct sensor_data {
 	char label[MAX_LABEL_LEN];
 	char name[MAX_ATTR_LEN];
 	struct device_attribute dev_attr;
+	struct sensor_group_data *sgdata;
+	struct sensor_data *sdata[3];
+	bool enable;
 };
 
+static struct sensor_group_data {
+	u32 gid;
+	u32 nr_phandle;
+	u32 nr_sensor;
+	enum sensors type;
+	const __be32 *phandles;
+	struct sensor_data **sensors;
+	bool enable;
+} *sg_data;
+
+/*
+ * To synchronise writes to struct sensor_data.enable and
+ * struct sensor_group_data.enable
+ */
+DEFINE_MUTEX(sensor_groups_mutex);
+static int nr_sensor_groups;
+
 struct platform_data {
 	const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
 	u32 sensors_count; /* Total count of sensors from each group */
@@ -105,6 +125,9 @@ static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
 	ssize_t ret;
 	u64 x;
 
+	if (sdata->sgdata && !sdata->enable)
+		return -ENODATA;
+
 	ret =  opal_get_sensor_data_u64(sdata->id, &x);
 
 	if (ret)
@@ -120,6 +143,74 @@ static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
 	return sprintf(buf, "%llu\n", x);
 }
 
+static ssize_t show_enable(struct device *dev,
+			   struct device_attribute *devattr, char *buf)
+{
+	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
+						 dev_attr);
+
+	return sprintf(buf, "%u\n", sdata->enable);
+}
+
+static ssize_t store_enable(struct device *dev,
+			    struct device_attribute *devattr,
+			    const char *buf, size_t count)
+{
+	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
+						 dev_attr);
+	struct sensor_group_data *sg = sdata->sgdata;
+	u32 data;
+	int ret, i;
+	bool send_command;
+
+	ret = kstrtoint(buf, 0, &data);
+	if (ret)
+		return ret;
+
+	if (data != 0 && data != 1)
+		return -EIO;
+
+	ret = mutex_lock_interruptible(&sensor_groups_mutex);
+	if (ret)
+		return ret;
+
+	sdata->enable = data;
+	if ((data && sg->enable) || (!data && !sg->enable)) {
+		send_command = false;
+	} else if (data && !sg->enable) {
+		/* Enable if first sensor in the group */
+		send_command = true;
+	} else if (!data && sg->enable) {
+		/* Disable if last sensor in the group */
+		send_command = true;
+		for (i = 0; i < sg->nr_sensor; i++) {
+			struct sensor_data *sd = sg->sensors[i];
+
+			if (sd->enable) {
+				send_command = false;
+				break;
+			}
+		}
+	}
+
+	if (send_command) {
+		ret =  sensor_group_enable(sg->gid, data);
+		if (!ret)
+			sg->enable = data;
+	}
+
+	if (!ret) {
+		for (i = 0; i < ARRAY_SIZE(sdata->sdata); i++)
+			sdata->sdata[i]->enable = data;
+		ret = count;
+	} else {
+		ret = -EIO;
+	}
+
+	mutex_unlock(&sensor_groups_mutex);
+	return ret;
+}
+
 static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
 			  char *buf)
 {
@@ -292,6 +383,90 @@ static u32 get_sensor_hwmon_index(struct sensor_data *sdata,
 	return ++sensor_groups[sdata->type].hwmon_index;
 }
 
+static int init_sensor_group_data(struct platform_device *pdev)
+{
+	struct device_node *sensor_groups, *sg;
+	enum sensors type;
+	int count = 0, ret = 0;
+
+	sensor_groups = of_find_node_by_path("/ibm,opal/sensor-groups");
+	if (!sensor_groups)
+		return ret;
+
+	for_each_child_of_node(sensor_groups, sg) {
+		type = get_sensor_type(sg);
+		if (type != MAX_SENSOR_TYPE)
+			nr_sensor_groups++;
+	}
+
+	if (!nr_sensor_groups)
+		goto out;
+
+	sg_data = devm_kzalloc(&pdev->dev, nr_sensor_groups * sizeof(*sg_data),
+			       GFP_KERNEL);
+	if (!sg_data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	for_each_child_of_node(sensor_groups, sg) {
+		const __be32 *phandles;
+		int len, gid;
+
+		type = get_sensor_type(sg);
+		if (type == MAX_SENSOR_TYPE)
+			continue;
+
+		if (of_property_read_u32(sg, "sensor-group-id", &gid))
+			continue;
+
+		phandles = of_get_property(sg, "sensors", &len);
+		if (!phandles)
+			continue;
+
+		len /= sizeof(u32);
+		if (!len)
+			continue;
+
+		sg_data[count].sensors = devm_kzalloc(&pdev->dev,
+					    len * sizeof(struct sensor_data *),
+					    GFP_KERNEL);
+		if (!sg_data[count].sensors) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		sg_data[count].nr_sensor = 0;
+		sg_data[count].gid = gid;
+		sg_data[count].type = type;
+		sg_data[count].nr_phandle = len;
+		sg_data[count].phandles = phandles;
+		sg_data[count++].enable = true;
+	}
+out:
+	of_node_put(sensor_groups);
+	return ret;
+}
+
+static struct sensor_group_data *get_sensor_group(struct device_node *np,
+						  enum sensors type)
+{
+	int i, j;
+
+	for (i = 0; i < nr_sensor_groups; i++) {
+		const __be32 *phandles = sg_data[i].phandles;
+
+		if (type != sg_data[i].type)
+			continue;
+
+		for (j = 0; j < sg_data[i].nr_phandle; j++)
+			if (be32_to_cpu(phandles[j]) == np->phandle)
+				return &sg_data[i];
+	}
+
+	return NULL;
+}
+
 static int populate_attr_groups(struct platform_device *pdev)
 {
 	struct platform_data *pdata = platform_get_drvdata(pdev);
@@ -299,6 +474,9 @@ static int populate_attr_groups(struct platform_device *pdev)
 	struct device_node *opal, *np;
 	enum sensors type;
 
+	if (init_sensor_group_data(pdev))
+		return -ENOMEM;
+
 	opal = of_find_node_by_path("/ibm,opal/sensors");
 	for_each_child_of_node(opal, np) {
 		const char *label;
@@ -313,7 +491,7 @@ static int populate_attr_groups(struct platform_device *pdev)
 		sensor_groups[type].attr_count++;
 
 		/*
-		 * add attributes for labels, min and max
+		 * add attributes for labels, min, max and enable
 		 */
 		if (!of_property_read_string(np, "label", &label))
 			sensor_groups[type].attr_count++;
@@ -321,6 +499,8 @@ static int populate_attr_groups(struct platform_device *pdev)
 			sensor_groups[type].attr_count++;
 		if (of_find_property(np, "sensor-data-max", NULL))
 			sensor_groups[type].attr_count++;
+		if (get_sensor_group(np, type))
+			sensor_groups[type].attr_count++;
 	}
 
 	of_node_put(opal);
@@ -344,7 +524,10 @@ static int populate_attr_groups(struct platform_device *pdev)
 static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
 			      ssize_t (*show)(struct device *dev,
 					      struct device_attribute *attr,
-					      char *buf))
+					      char *buf),
+			    ssize_t (*store)(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count))
 {
 	snprintf(sdata->name, MAX_ATTR_LEN, "%s%d_%s",
 		 sensor_groups[sdata->type].name, sdata->hwmon_index,
@@ -352,23 +535,34 @@ static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
 
 	sysfs_attr_init(&sdata->dev_attr.attr);
 	sdata->dev_attr.attr.name = sdata->name;
-	sdata->dev_attr.attr.mode = S_IRUGO;
 	sdata->dev_attr.show = show;
+	if (store) {
+		sdata->dev_attr.store = store;
+		sdata->dev_attr.attr.mode = 0664;
+	} else {
+		sdata->dev_attr.attr.mode = 0444;
+	}
 }
 
 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,
+			    struct sensor_group_data *sgdata,
 			    ssize_t (*show)(struct device *dev,
 					    struct device_attribute *attr,
-					    char *buf))
+					    char *buf),
+			    ssize_t (*store)(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count))
 {
 	sdata->id = sid;
 	sdata->type = type;
 	sdata->opal_index = od;
 	sdata->hwmon_index = hd;
-	create_hwmon_attr(sdata, attr_name, show);
+	create_hwmon_attr(sdata, attr_name, show, store);
 	pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
+	sdata->enable = true;
+	sdata->sgdata = sgdata;
 }
 
 static char *get_max_attr(enum sensors type)
@@ -408,18 +602,17 @@ static int create_device_attrs(struct platform_device *pdev)
 	u32 count = 0;
 	int err = 0;
 
-	opal = of_find_node_by_path("/ibm,opal/sensors");
 	sdata = devm_kcalloc(&pdev->dev,
 			     pdata->sensors_count, sizeof(*sdata),
 			     GFP_KERNEL);
-	if (!sdata) {
-		err = -ENOMEM;
-		goto exit_put_node;
-	}
+	if (!sdata)
+		return -ENOMEM;
 
+	opal = of_find_node_by_path("/ibm,opal/sensors");
 	for_each_child_of_node(opal, np) {
+		struct sensor_group_data *sgdata;
 		const char *attr_name;
-		u32 opal_index;
+		u32 opal_index, hw_id;
 		const char *label;
 
 		if (np->name == NULL)
@@ -456,14 +649,43 @@ 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);
+		hw_id = get_sensor_hwmon_index(&sdata[count], sdata, count);
+		sgdata = get_sensor_group(np, type);
+		populate_sensor(&sdata[count], opal_index, hw_id, sensor_id,
+				attr_name, type, pgroups[type], sgdata,
+				show_sensor, NULL);
+		count++;
+
+		if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
+			attr_name = get_max_attr(type);
+			populate_sensor(&sdata[count], opal_index, hw_id,
+					sensor_id, attr_name, type,
+					pgroups[type], sgdata, show_sensor,
+					NULL);
+			count++;
+		}
+
+		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
+			attr_name = get_min_attr(type);
+			populate_sensor(&sdata[count], opal_index, hw_id,
+					sensor_id, attr_name, type,
+					pgroups[type], sgdata, show_sensor,
+					NULL);
+			count++;
+		}
 
-		create_hwmon_attr(&sdata[count], attr_name, show_sensor);
+		if (sgdata) {
+			int i;
 
-		pgroups[type]->attrs[sensor_groups[type].attr_count++] =
-				&sdata[count++].dev_attr.attr;
+			sgdata->sensors[sgdata->nr_sensor++] = &sdata[count];
+			populate_sensor(&sdata[count], opal_index, hw_id,
+					sgdata->gid, "enable", type,
+					pgroups[type], sgdata, show_enable,
+					store_enable);
+			for (i = 0; i < ARRAY_SIZE(sdata[count].sdata); i++)
+				sdata[count].sdata[i] = &sdata[count - i - 1];
+			count++;
+		}
 
 		if (!of_property_read_string(np, "label", &label)) {
 			/*
@@ -474,34 +696,15 @@ static int create_device_attrs(struct platform_device *pdev)
 			 */
 
 			make_sensor_label(np, &sdata[count], label);
-			populate_sensor(&sdata[count], opal_index,
-					sdata[count - 1].hwmon_index,
+			populate_sensor(&sdata[count], opal_index, hw_id,
 					sensor_id, "label", type, pgroups[type],
-					show_label);
-			count++;
-		}
-
-		if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
-			attr_name = get_max_attr(type);
-			populate_sensor(&sdata[count], opal_index,
-					sdata[count - 1].hwmon_index,
-					sensor_id, attr_name, type,
-					pgroups[type], show_sensor);
-			count++;
-		}
-
-		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
-			attr_name = get_min_attr(type);
-			populate_sensor(&sdata[count], opal_index,
-					sdata[count - 1].hwmon_index,
-					sensor_id, attr_name, type,
-					pgroups[type], show_sensor);
+					NULL, show_label, NULL);
 			count++;
 		}
 	}
 
-exit_put_node:
 	of_node_put(opal);
+
 	return err;
 }
 
-- 
1.8.3.1

Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups

[Guenter Roeck]

On 07/04/2018 02:16 AM, Shilpasri G Bhat wrote:
> On-Chip-Controller(OCC) is an embedded micro-processor in POWER9 chip
> which measures various system and chip level sensors. These sensors
> comprises of environmental sensors (like power, temperature, current
> and voltage) and performance sensors (like utilization, frequency).
> All these sensors are copied to main memory at a regular interval of
> 100ms. OCC provides a way to select a group of sensors that is copied
> to the main memory to increase the update frequency of selected sensor
> groups. When a sensor-group is disabled, OCC will not copy it to main
> memory and those sensors read 0 values.
> 
> This patch provides support for enabling/disabling the sensor groups
> like power, temperature, current and voltage. This patch adds new
> per-senor sysfs attribute to disable and enable them.
> 
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> Changes from v1:
> - Add per-sensor 'enable' attribute
> - Return -ENODATA when sensor is disabled
> 
>   Documentation/hwmon/sysfs-interface |  22 +++
>   drivers/hwmon/ibmpowernv.c          | 281 +++++++++++++++++++++++++++++++-----
>   2 files changed, 264 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> index fc337c3..38ab05c 100644
> --- a/Documentation/hwmon/sysfs-interface
> +++ b/Documentation/hwmon/sysfs-interface

Separate patch please.

> @@ -184,6 +184,11 @@ vrm		Voltage Regulator Module version number.
>   		Affects the way the driver calculates the CPU core reference
>   		voltage from the vid pins.
>   
> +in[0-*]_enable	Enable or disable the sensor
> +		1 : Enable
> +		0 : Disable
> +		RW
> +
>   Also see the Alarms section for status flags associated with voltages.
>   
>   
> @@ -409,6 +414,12 @@ temp_reset_history
>   		Reset temp_lowest and temp_highest for all sensors
>   		WO
>   
> +temp[1-*]_enable
> +		Enable or disable the sensor
> +		1 : Enable
> +		0 : Disable > +		RW
> +
>   Some chips measure temperature using external thermistors and an ADC, and
>   report the temperature measurement as a voltage. Converting this voltage
>   back to a temperature (or the other way around for limits) requires
> @@ -468,6 +479,12 @@ curr_reset_history
>   		Reset currX_lowest and currX_highest for all sensors
>   		WO
>   
> +curr[1-*]_enable
> +		Enable or disable the sensor
> +		1 : Enable
> +		0 : Disable
> +		RW
> +
>   Also see the Alarms section for status flags associated with currents.
>   
>   *********
> @@ -566,6 +583,11 @@ power[1-*]_crit			Critical maximum power.
>   				Unit: microWatt
>   				RW
>   
> +power[1-*]_enable		Enable or disable the sensor
> +				1 : Enable
> +				0 : Disable
> +				RW
> +
>   Also see the Alarms section for status flags associated with power readings.
>   

Any reason for excluding fan, energy, humidity ?

>   **********
> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
> index f829dad..61e04cf 100644
> --- a/drivers/hwmon/ibmpowernv.c
> +++ b/drivers/hwmon/ibmpowernv.c
> @@ -90,8 +90,28 @@ struct sensor_data {
>   	char label[MAX_LABEL_LEN];
>   	char name[MAX_ATTR_LEN];
>   	struct device_attribute dev_attr;
> +	struct sensor_group_data *sgdata;
> +	struct sensor_data *sdata[3];
> +	bool enable;
>   };
>   
> +static struct sensor_group_data {
> +	u32 gid;
> +	u32 nr_phandle;
> +	u32 nr_sensor;
> +	enum sensors type;
> +	const __be32 *phandles;
> +	struct sensor_data **sensors;
> +	bool enable;
> +} *sg_data;
> +
> +/*
> + * To synchronise writes to struct sensor_data.enable and
> + * struct sensor_group_data.enable
> + */
> +DEFINE_MUTEX(sensor_groups_mutex);

Not as global variable, please.

> +static int nr_sensor_groups;
> +

Do those have to be static variables ? Why not in struct platform_data ?

>   struct platform_data {
>   	const struct attribute_group *attr_groups[MAX_SENSOR_TYPE + 1];
>   	u32 sensors_count; /* Total count of sensors from each group */
> @@ -105,6 +125,9 @@ static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
>   	ssize_t ret;
>   	u64 x;
>   
> +	if (sdata->sgdata && !sdata->enable)
> +		return -ENODATA;
> +

This return code should be documented in the ABI.

>   	ret =  opal_get_sensor_data_u64(sdata->id, &x);
>   
>   	if (ret)
> @@ -120,6 +143,74 @@ static ssize_t show_sensor(struct device *dev, struct device_attribute *devattr,
>   	return sprintf(buf, "%llu\n", x);
>   }
>   
> +static ssize_t show_enable(struct device *dev,
> +			   struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
> +						 dev_attr);
> +
> +	return sprintf(buf, "%u\n", sdata->enable);
> +}
> +
> +static ssize_t store_enable(struct device *dev,
> +			    struct device_attribute *devattr,
> +			    const char *buf, size_t count)
> +{
> +	struct sensor_data *sdata = container_of(devattr, struct sensor_data,
> +						 dev_attr);
> +	struct sensor_group_data *sg = sdata->sgdata;
> +	u32 data;
> +	int ret, i;
> +	bool send_command;
> +
> +	ret = kstrtoint(buf, 0, &data);

Use kstrtobool(), please. Then you won't need the additional range check below.

> +	if (ret)
> +		return ret;
> +
> +	if (data != 0 && data != 1)
> +		return -EIO;

This is not an IO error.

> +
> +	ret = mutex_lock_interruptible(&sensor_groups_mutex);
> +	if (ret)
> +		return ret;
> +
> +	sdata->enable = data;
> +	if ((data && sg->enable) || (!data && !sg->enable)) {
> +		send_command = false;

If data is bool you can use a simple comparison.

> +	} else if (data && !sg->enable) {

!sg->enable is unnecessary.

> +		/* Enable if first sensor in the group */
> +		send_command = true; > +	} else if (!data && sg->enable) {

This if statement is always true and thus unnecessary.

Overall, you could use

	send_command = data != sg->enable;
	if (send_command && !data) {
		for (...) {

> +		/* Disable if last sensor in the group */
> +		send_command = true;
> +		for (i = 0; i < sg->nr_sensor; i++) {
> +			struct sensor_data *sd = sg->sensors[i];
> +
> +			if (sd->enable) {
> +				send_command = false;
> +				break;
> +			}

This is weird. So there are situations where a request to disable
a sensor is accepted, but effectively ignored ? Shouldn't that
return, say, -EBUSY ?

> +		}
> +	}
> +
> +	if (send_command) {
> +		ret =  sensor_group_enable(sg->gid, data);
> +		if (!ret)
> +			sg->enable = data;
> +	}
> +
> +	if (!ret) {
> +		for (i = 0; i < ARRAY_SIZE(sdata->sdata); i++)
> +			sdata->sdata[i]->enable = data;
> +		ret = count;
> +	} else {
> +		ret = -EIO;

No overriding error codes, please.

> +	}
> +
> +	mutex_unlock(&sensor_groups_mutex);
> +	return ret;
> +}
> +
>   static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
>   			  char *buf)
>   {
> @@ -292,6 +383,90 @@ static u32 get_sensor_hwmon_index(struct sensor_data *sdata,
>   	return ++sensor_groups[sdata->type].hwmon_index;
>   }
>   
> +static int init_sensor_group_data(struct platform_device *pdev)
> +{
> +	struct device_node *sensor_groups, *sg;
> +	enum sensors type;
> +	int count = 0, ret = 0;
> +
> +	sensor_groups = of_find_node_by_path("/ibm,opal/sensor-groups");
> +	if (!sensor_groups)
> +		return ret;
> +
> +	for_each_child_of_node(sensor_groups, sg) {
> +		type = get_sensor_type(sg);
> +		if (type != MAX_SENSOR_TYPE)
> +			nr_sensor_groups++;
> +	}
> +
> +	if (!nr_sensor_groups)
> +		goto out;
> +
> +	sg_data = devm_kzalloc(&pdev->dev, nr_sensor_groups * sizeof(*sg_data),
> +			       GFP_KERNEL);
> +	if (!sg_data) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for_each_child_of_node(sensor_groups, sg) {
> +		const __be32 *phandles;
> +		int len, gid;
> +
> +		type = get_sensor_type(sg);
> +		if (type == MAX_SENSOR_TYPE)
> +			continue;
> +
> +		if (of_property_read_u32(sg, "sensor-group-id", &gid))
> +			continue;
> +
> +		phandles = of_get_property(sg, "sensors", &len);
> +		if (!phandles)
> +			continue;
> +
> +		len /= sizeof(u32);
> +		if (!len)
> +			continue;
> +
> +		sg_data[count].sensors = devm_kzalloc(&pdev->dev,
> +					    len * sizeof(struct sensor_data *),
> +					    GFP_KERNEL);
> +		if (!sg_data[count].sensors) {
> +			ret = -ENOMEM;
> +			break;

Doesn't this result in a missing of_node_put() on sg ?

> +		}
> +
> +		sg_data[count].nr_sensor = 0;
> +		sg_data[count].gid = gid;
> +		sg_data[count].type = type;
> +		sg_data[count].nr_phandle = len;
> +		sg_data[count].phandles = phandles;
> +		sg_data[count++].enable = true;
> +	}
> +out:
> +	of_node_put(sensor_groups);
> +	return ret;
> +}
> +
> +static struct sensor_group_data *get_sensor_group(struct device_node *np,
> +						  enum sensors type)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < nr_sensor_groups; i++) {
> +		const __be32 *phandles = sg_data[i].phandles;
> +
> +		if (type != sg_data[i].type)
> +			continue;
> +
> +		for (j = 0; j < sg_data[i].nr_phandle; j++)
> +			if (be32_to_cpu(phandles[j]) == np->phandle)
> +				return &sg_data[i];
> +	}
> +
> +	return NULL;
> +}
> +
>   static int populate_attr_groups(struct platform_device *pdev)
>   {
>   	struct platform_data *pdata = platform_get_drvdata(pdev);
> @@ -299,6 +474,9 @@ static int populate_attr_groups(struct platform_device *pdev)
>   	struct device_node *opal, *np;
>   	enum sensors type;
>   
> +	if (init_sensor_group_data(pdev))
> +		return -ENOMEM;
> +

init_sensor_group_data() returns an error code which you should pass on.

>   	opal = of_find_node_by_path("/ibm,opal/sensors");
>   	for_each_child_of_node(opal, np) {
>   		const char *label;
> @@ -313,7 +491,7 @@ static int populate_attr_groups(struct platform_device *pdev)
>   		sensor_groups[type].attr_count++;
>   
>   		/*
> -		 * add attributes for labels, min and max
> +		 * add attributes for labels, min, max and enable
>   		 */
>   		if (!of_property_read_string(np, "label", &label))
>   			sensor_groups[type].attr_count++;
> @@ -321,6 +499,8 @@ static int populate_attr_groups(struct platform_device *pdev)
>   			sensor_groups[type].attr_count++;
>   		if (of_find_property(np, "sensor-data-max", NULL))
>   			sensor_groups[type].attr_count++;
> +		if (get_sensor_group(np, type))
> +			sensor_groups[type].attr_count++;
>   	}
>   
>   	of_node_put(opal);
> @@ -344,7 +524,10 @@ static int populate_attr_groups(struct platform_device *pdev)
>   static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
>   			      ssize_t (*show)(struct device *dev,
>   					      struct device_attribute *attr,
> -					      char *buf))
> +					      char *buf),
> +			    ssize_t (*store)(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf, size_t count))
>   {
>   	snprintf(sdata->name, MAX_ATTR_LEN, "%s%d_%s",
>   		 sensor_groups[sdata->type].name, sdata->hwmon_index,
> @@ -352,23 +535,34 @@ static void create_hwmon_attr(struct sensor_data *sdata, const char *attr_name,
>   
>   	sysfs_attr_init(&sdata->dev_attr.attr);
>   	sdata->dev_attr.attr.name = sdata->name;
> -	sdata->dev_attr.attr.mode = S_IRUGO;
>   	sdata->dev_attr.show = show;
> +	if (store) {
> +		sdata->dev_attr.store = store;
> +		sdata->dev_attr.attr.mode = 0664;
> +	} else {
> +		sdata->dev_attr.attr.mode = 0444;
> +	}
>   }
>   
>   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,
> +			    struct sensor_group_data *sgdata,
>   			    ssize_t (*show)(struct device *dev,
>   					    struct device_attribute *attr,
> -					    char *buf))
> +					    char *buf),
> +			    ssize_t (*store)(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf, size_t count))
>   {
>   	sdata->id = sid;
>   	sdata->type = type;
>   	sdata->opal_index = od;
>   	sdata->hwmon_index = hd;
> -	create_hwmon_attr(sdata, attr_name, show);
> +	create_hwmon_attr(sdata, attr_name, show, store);
>   	pgroup->attrs[sensor_groups[type].attr_count++] = &sdata->dev_attr.attr;
> +	sdata->enable = true;
> +	sdata->sgdata = sgdata;
>   }
>   
>   static char *get_max_attr(enum sensors type)
> @@ -408,18 +602,17 @@ static int create_device_attrs(struct platform_device *pdev)
>   	u32 count = 0;
>   	int err = 0;
>   
> -	opal = of_find_node_by_path("/ibm,opal/sensors");
>   	sdata = devm_kcalloc(&pdev->dev,
>   			     pdata->sensors_count, sizeof(*sdata),
>   			     GFP_KERNEL);
> -	if (!sdata) {
> -		err = -ENOMEM;
> -		goto exit_put_node;
> -	}
> +	if (!sdata)
> +		return -ENOMEM;
>   
> +	opal = of_find_node_by_path("/ibm,opal/sensors");
>   	for_each_child_of_node(opal, np) {
> +		struct sensor_group_data *sgdata;
>   		const char *attr_name;
> -		u32 opal_index;
> +		u32 opal_index, hw_id;
>   		const char *label;
>   
>   		if (np->name == NULL)
> @@ -456,14 +649,43 @@ 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);
> +		hw_id = get_sensor_hwmon_index(&sdata[count], sdata, count);
> +		sgdata = get_sensor_group(np, type);
> +		populate_sensor(&sdata[count], opal_index, hw_id, sensor_id,
> +				attr_name, type, pgroups[type], sgdata,
> +				show_sensor, NULL);
> +		count++;
> +
> +		if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
> +			attr_name = get_max_attr(type);
> +			populate_sensor(&sdata[count], opal_index, hw_id,
> +					sensor_id, attr_name, type,
> +					pgroups[type], sgdata, show_sensor,
> +					NULL);
> +			count++;
> +		}
> +
> +		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
> +			attr_name = get_min_attr(type);
> +			populate_sensor(&sdata[count], opal_index, hw_id,
> +					sensor_id, attr_name, type,
> +					pgroups[type], sgdata, show_sensor,
> +					NULL);
> +			count++;
> +		}
>   
> -		create_hwmon_attr(&sdata[count], attr_name, show_sensor);
> +		if (sgdata) {
> +			int i;
>   
> -		pgroups[type]->attrs[sensor_groups[type].attr_count++] =
> -				&sdata[count++].dev_attr.attr;
> +			sgdata->sensors[sgdata->nr_sensor++] = &sdata[count];
> +			populate_sensor(&sdata[count], opal_index, hw_id,
> +					sgdata->gid, "enable", type,
> +					pgroups[type], sgdata, show_enable,
> +					store_enable);
> +			for (i = 0; i < ARRAY_SIZE(sdata[count].sdata); i++)
> +				sdata[count].sdata[i] = &sdata[count - i - 1];
> +			count++;
> +		}
>   
>   		if (!of_property_read_string(np, "label", &label)) {
>   			/*
> @@ -474,34 +696,15 @@ static int create_device_attrs(struct platform_device *pdev)
>   			 */
>   
>   			make_sensor_label(np, &sdata[count], label);
> -			populate_sensor(&sdata[count], opal_index,
> -					sdata[count - 1].hwmon_index,
> +			populate_sensor(&sdata[count], opal_index, hw_id,
>   					sensor_id, "label", type, pgroups[type],
> -					show_label);
> -			count++;
> -		}
> -
> -		if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) {
> -			attr_name = get_max_attr(type);
> -			populate_sensor(&sdata[count], opal_index,
> -					sdata[count - 1].hwmon_index,
> -					sensor_id, attr_name, type,
> -					pgroups[type], show_sensor);
> -			count++;
> -		}
> -
> -		if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) {
> -			attr_name = get_min_attr(type);
> -			populate_sensor(&sdata[count], opal_index,
> -					sdata[count - 1].hwmon_index,
> -					sensor_id, attr_name, type,
> -					pgroups[type], show_sensor);
> +					NULL, show_label, NULL);
>   			count++;
>   		}
>   	}
>   
> -exit_put_node:
>   	of_node_put(opal);
> +
>   	return err;
>   }
>   
> 

Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups

[Shilpasri G Bhat]

Hi Guenter,

Thanks for reviewing the patch.
On 07/04/2018 08:16 PM, Guenter Roeck wrote:
>> +        /* Disable if last sensor in the group */
>> +        send_command = true;
>> +        for (i = 0; i < sg->nr_sensor; i++) {
>> +            struct sensor_data *sd = sg->sensors[i];
>> +
>> +            if (sd->enable) {
>> +                send_command = false;
>> +                break;
>> +            }
> 
> This is weird. So there are situations where a request to disable
> a sensor is accepted, but effectively ignored ? Shouldn't that
> return, say, -EBUSY ?

This is because we do not support per-sensor enable/disable. We can only
enable/disable at a sensor-group level.

This patch follows the semantic to disable a sensor group iff all the sensors
belonging to that group have been disabled. Otherwise the sensor alone is marked
to be disabled and returns -ENODATA on reading it.

And a sensor group will be enabled if any of the sensor in that group is enabled.

I will make changes to the remaining code according to your suggestion.

Thanks and Regards,
Shilpa

Re: [PATCH v2 2/2] hwmon: ibmpowernv: Add attributes to enable/disable sensor groups

[Guenter Roeck]

On 07/04/2018 09:53 AM, Shilpasri G Bhat wrote:
> Hi Guenter,
> 
> Thanks for reviewing the patch.
> On 07/04/2018 08:16 PM, Guenter Roeck wrote:
>>> +        /* Disable if last sensor in the group */
>>> +        send_command = true;
>>> +        for (i = 0; i < sg->nr_sensor; i++) {
>>> +            struct sensor_data *sd = sg->sensors[i];
>>> +
>>> +            if (sd->enable) {
>>> +                send_command = false;
>>> +                break;
>>> +            }
>>
>> This is weird. So there are situations where a request to disable
>> a sensor is accepted, but effectively ignored ? Shouldn't that
>> return, say, -EBUSY ?
> 
> This is because we do not support per-sensor enable/disable. We can only
> enable/disable at a sensor-group level.
> 
> This patch follows the semantic to disable a sensor group iff all the sensors
> belonging to that group have been disabled. Otherwise the sensor alone is marked
> to be disabled and returns -ENODATA on reading it.
> 
> And a sensor group will be enabled if any of the sensor in that group is enabled.
> 

In similar situations, where setting one attribute affects others, a common solution
is to make only the first attribute writable and have it affect all the others.
I think that would make sense here as well, and it would be much simpler to implement.

Guenter

> I will make changes to the remaining code according to your suggestion.
> 
> Thanks and Regards,
> Shilpa
> 
> --
> 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
>