Nouveau - Mar 2016 - [PATCH 0/4] Configure Power Sensors

The power sensors can be configured to sample the readout values over time.
Nvidia does this too, so nouveau should probably do that too.

Karol Herbst (4):
  iccsense: remove read function
  iccsense: convert to linked list
  iccsense: split sensor into own struct
  iccsense: configure sensors like nvidia does

 drm/nouveau/include/nvkm/subdev/iccsense.h |   6 +-
 drm/nouveau/nouveau_hwmon.c                |   2 +-
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 246 +++++++++++++++++++++--------
 drm/nouveau/nvkm/subdev/iccsense/priv.h    |  16 +-
 4 files changed, 198 insertions(+), 72 deletions(-)

-- 
2.7.4

Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
---
 drm/nouveau/include/nvkm/subdev/iccsense.h |  1 -
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 23 ++++++++++-------------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/iccsense.h
b/drm/nouveau/include/nvkm/subdev/iccsense.h
index 530c621..c3defcd 100644
--- a/drm/nouveau/include/nvkm/subdev/iccsense.h
+++ b/drm/nouveau/include/nvkm/subdev/iccsense.h
@@ -12,6 +12,5 @@ struct nvkm_iccsense {
 };
 
 int gf100_iccsense_new(struct nvkm_device *, int index, struct nvkm_iccsense
**);
-int nvkm_iccsense_read(struct nvkm_iccsense *iccsense, u8 idx);
 int nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense);
 #endif
diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
index c44a852..bf1b94e 100644
--- a/drm/nouveau/nvkm/subdev/iccsense/base.c
+++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
@@ -96,26 +96,23 @@ nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
 }
 
 int
-nvkm_iccsense_read(struct nvkm_iccsense *iccsense, u8 idx)
+nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
 {
-	struct nvkm_iccsense_rail *rail;
+	int result = 0, i;
 
-	if (!iccsense || idx >= iccsense->rail_count)
+	if (!iccsense)
 		return -EINVAL;
 
-	rail = &iccsense->rails[idx];
-	if (!rail->read)
+	if (iccsense->rail_count == 0)
 		return -ENODEV;
 
-	return rail->read(iccsense, rail);
-}
-
-int
-nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
-{
-	int result = 0, i;
 	for (i = 0; i < iccsense->rail_count; ++i) {
-		int res = nvkm_iccsense_read(iccsense, i);
+		int res;
+		struct nvkm_iccsense_rail *rail = &iccsense->rails[i];
+		if (!rail->read)
+			return -ENODEV;
+
+		res = rail->read(iccsense, rail);
 		if (res >= 0)
 			result += res;
 		else
-- 
2.7.4

Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
---
 drm/nouveau/include/nvkm/subdev/iccsense.h |  4 +---
 drm/nouveau/nouveau_hwmon.c                |  2 +-
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 32 +++++++++++++-----------------
 drm/nouveau/nvkm/subdev/iccsense/priv.h    |  1 +
 4 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/iccsense.h
b/drm/nouveau/include/nvkm/subdev/iccsense.h
index c3defcd..a4c0da0 100644
--- a/drm/nouveau/include/nvkm/subdev/iccsense.h
+++ b/drm/nouveau/include/nvkm/subdev/iccsense.h
@@ -3,12 +3,10 @@
 
 #include <core/subdev.h>
 
-struct nkvm_iccsense_rail;
 struct nvkm_iccsense {
 	struct nvkm_subdev subdev;
-	u8 rail_count;
 	bool data_valid;
-	struct nvkm_iccsense_rail *rails;
+	struct list_head rails;
 };
 
 int gf100_iccsense_new(struct nvkm_device *, int index, struct nvkm_iccsense
**);
diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
index 67edd2f..74f237b 100644
--- a/drm/nouveau/nouveau_hwmon.c
+++ b/drm/nouveau/nouveau_hwmon.c
@@ -689,7 +689,7 @@ nouveau_hwmon_init(struct drm_device *dev)
 			goto error;
 	}
 
-	if (iccsense && iccsense->data_valid &&
iccsense->rail_count) {
+	if (iccsense && iccsense->data_valid &&
!list_empty(&iccsense->rails)) {
 		ret = sysfs_create_group(&hwmon_dev->kobj,
 					 &hwmon_power_attrgroup);
 		if (ret)
diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
index bf1b94e..6fde68d 100644
--- a/drm/nouveau/nvkm/subdev/iccsense/base.c
+++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
@@ -98,25 +98,21 @@ nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
 int
 nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
 {
-	int result = 0, i;
+	int result = 0;
+	struct nvkm_iccsense_rail *rail;
 
 	if (!iccsense)
 		return -EINVAL;
 
-	if (iccsense->rail_count == 0)
-		return -ENODEV;
-
-	for (i = 0; i < iccsense->rail_count; ++i) {
+	list_for_each_entry(rail, &iccsense->rails, head) {
 		int res;
-		struct nvkm_iccsense_rail *rail = &iccsense->rails[i];
 		if (!rail->read)
 			return -ENODEV;
 
 		res = rail->read(iccsense, rail);
-		if (res >= 0)
-			result += res;
-		else
+		if (res < 0)
 			return res;
+		result += res;
 	}
 	return result;
 }
@@ -125,9 +121,10 @@ static void *
 nvkm_iccsense_dtor(struct nvkm_subdev *subdev)
 {
 	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
+	struct nvkm_iccsense_rail *rail, *tmp;
 
-	if (iccsense->rails)
-		kfree(iccsense->rails);
+	list_for_each_entry_safe(rail, tmp, &iccsense->rails, head)
+		kfree(rail);
 
 	return iccsense;
 }
@@ -145,11 +142,6 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 	    || !stbl.nr_entry)
 		return 0;
 
-	iccsense->rails = kmalloc(sizeof(*iccsense->rails) * stbl.nr_entry,
-	                          GFP_KERNEL);
-	if (!iccsense->rails)
-		return -ENOMEM;
-
 	iccsense->data_valid = true;
 	for (i = 0; i < stbl.nr_entry; ++i) {
 		struct pwr_rail_t *r = &stbl.rail[i];
@@ -184,7 +176,10 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 			continue;
 		}
 
-		rail = &iccsense->rails[iccsense->rail_count];
+		rail = kmalloc(sizeof(*rail), GFP_KERNEL);
+		if (!rail)
+			return -ENOMEM;
+
 		switch (extdev.type) {
 		case NVBIOS_EXTDEV_INA209:
 			rail->read = nvkm_iccsense_ina209_read;
@@ -201,7 +196,7 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 		rail->rail = r->rail;
 		rail->mohm = r->resistor_mohm;
 		rail->i2c = &i2c_bus->i2c;
-		++iccsense->rail_count;
+		list_add_tail(&rail->head, &iccsense->rails);
 	}
 	return 0;
 }
@@ -224,6 +219,7 @@ nvkm_iccsense_new_(struct nvkm_device *device, int index,
 {
 	if (!(*iccsense = kzalloc(sizeof(**iccsense), GFP_KERNEL)))
 		return -ENOMEM;
+	INIT_LIST_HEAD(&(*iccsense)->rails);
 	nvkm_iccsense_ctor(device, index, *iccsense);
 	return 0;
 }
diff --git a/drm/nouveau/nvkm/subdev/iccsense/priv.h
b/drm/nouveau/nvkm/subdev/iccsense/priv.h
index ed398b8..e479128 100644
--- a/drm/nouveau/nvkm/subdev/iccsense/priv.h
+++ b/drm/nouveau/nvkm/subdev/iccsense/priv.h
@@ -4,6 +4,7 @@
 #include <subdev/iccsense.h>
 
 struct nvkm_iccsense_rail {
+	struct list_head head;
 	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
 	struct i2c_adapter *i2c;
 	u8 addr;
-- 
2.7.4

Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
---
 drm/nouveau/include/nvkm/subdev/iccsense.h |   1 +
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 141 ++++++++++++++++++++---------
 drm/nouveau/nvkm/subdev/iccsense/priv.h    |  15 ++-
 3 files changed, 112 insertions(+), 45 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/iccsense.h
b/drm/nouveau/include/nvkm/subdev/iccsense.h
index a4c0da0..3c2ddd9 100644
--- a/drm/nouveau/include/nvkm/subdev/iccsense.h
+++ b/drm/nouveau/include/nvkm/subdev/iccsense.h
@@ -6,6 +6,7 @@
 struct nvkm_iccsense {
 	struct nvkm_subdev subdev;
 	bool data_valid;
+	struct list_head sensors;
 	struct list_head rails;
 };
 
diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
index 6fde68d..b6f6222 100644
--- a/drm/nouveau/nvkm/subdev/iccsense/base.c
+++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
@@ -30,15 +30,14 @@
 
 static bool
 nvkm_iccsense_validate_device(struct i2c_adapter *i2c, u8 addr,
-			      enum nvbios_extdev_type type, u8 rail)
+			      enum nvbios_extdev_type type)
 {
 	switch (type) {
 	case NVBIOS_EXTDEV_INA209:
 	case NVBIOS_EXTDEV_INA219:
-		return rail == 0 && nv_rd16i2cr(i2c, addr, 0x0) >= 0;
+		return nv_rd16i2cr(i2c, addr, 0x0) >= 0;
 	case NVBIOS_EXTDEV_INA3221:
-		return rail <= 3 &&
-		       nv_rd16i2cr(i2c, addr, 0xff) == 0x3220 &&
+		return nv_rd16i2cr(i2c, addr, 0xff) == 0x3220 &&
 		       nv_rd16i2cr(i2c, addr, 0xfe) == 0x5449;
 	default:
 		return false;
@@ -67,8 +66,9 @@ nvkm_iccsense_ina2x9_read(struct nvkm_iccsense *iccsense,
                           struct nvkm_iccsense_rail *rail,
 			  u8 shunt_reg, u8 bus_reg)
 {
-	return nvkm_iccsense_poll_lane(rail->i2c, rail->addr, shunt_reg, 0,
-				       bus_reg, 3, rail->mohm, 10 * 4);
+	return nvkm_iccsense_poll_lane(rail->sensor->i2c,
rail->sensor->addr,
+				       shunt_reg, 0, bus_reg, 3, rail->mohm,
+				       10 * 4);
 }
 
 static int
@@ -89,9 +89,9 @@ static int
 nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
 			   struct nvkm_iccsense_rail *rail)
 {
-	return nvkm_iccsense_poll_lane(rail->i2c, rail->addr,
-				       1 + (rail->rail * 2), 3,
-				       2 + (rail->rail * 2), 3, rail->mohm,
+	return nvkm_iccsense_poll_lane(rail->sensor->i2c,
rail->sensor->addr,
+				       1 + (rail->idx * 2), 3,
+				       2 + (rail->idx * 2), 3, rail->mohm,
 				       40 * 8);
 }
 
@@ -121,81 +121,137 @@ static void *
 nvkm_iccsense_dtor(struct nvkm_subdev *subdev)
 {
 	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
-	struct nvkm_iccsense_rail *rail, *tmp;
+	struct nvkm_iccsense_sensor *sensor, *tmps;
+	struct nvkm_iccsense_rail *rail, *tmpr;
 
-	list_for_each_entry_safe(rail, tmp, &iccsense->rails, head)
+	list_for_each_entry_safe(sensor, tmps, &iccsense->sensors, head)
+		kfree(sensor);
+	list_for_each_entry_safe(rail, tmpr, &iccsense->rails, head)
 		kfree(rail);
 
 	return iccsense;
 }
 
+static struct nvkm_iccsense_sensor*
+nvkm_iccsense_create_sensor(struct nvkm_iccsense *iccsense, u8 id)
+{
+
+	struct nvkm_subdev *subdev = &iccsense->subdev;
+	struct nvkm_bios *bios = subdev->device->bios;
+	struct nvkm_i2c *i2c = subdev->device->i2c;
+	struct nvbios_extdev_func extdev;
+	struct nvkm_i2c_bus *i2c_bus;
+	struct nvkm_iccsense_sensor *sensor;
+	u8 addr;
+
+	if (!i2c || !bios || nvbios_extdev_parse(bios, id, &extdev))
+		return NULL;
+
+	if (extdev.type == 0xff)
+		return NULL;
+
+	if (extdev.type != NVBIOS_EXTDEV_INA209 &&
+	    extdev.type != NVBIOS_EXTDEV_INA219 &&
+	    extdev.type != NVBIOS_EXTDEV_INA3221) {
+		iccsense->data_valid = false;
+		nvkm_error(subdev, "found unknown sensor type %x, "
+			   "power reading might be invalid\n",
+			   extdev.type);
+		return NULL;
+	}
+
+	if (extdev.bus)
+		i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_SEC);
+	else
+		i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_PRI);
+	if (!i2c_bus)
+		return NULL;
+
+	addr = extdev.addr >> 1;
+	if (!nvkm_iccsense_validate_device(&i2c_bus->i2c, addr,
+					   extdev.type)) {
+		iccsense->data_valid = false;
+		nvkm_warn(subdev, "found invalid sensor id: %i, power reading"
+			  "might be invalid\n", id);
+		return NULL;
+	}
+
+	sensor = kmalloc(sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return NULL;
+
+	list_add_tail(&sensor->head, &iccsense->sensors);
+	sensor->id = id;
+	sensor->type = extdev.type;
+	sensor->i2c = &i2c_bus->i2c;
+	sensor->addr = addr;
+	sensor->rail_mask = 0x0;
+	return sensor;
+}
+
+static struct nvkm_iccsense_sensor*
+nvkm_iccsense_get_sensor(struct nvkm_iccsense *iccsense, u8 id)
+{
+	struct nvkm_iccsense_sensor *sensor;
+	list_for_each_entry(sensor, &iccsense->sensors, head) {
+		if (sensor->id == id)
+			return sensor;
+	}
+	return nvkm_iccsense_create_sensor(iccsense, id);
+}
+
 static int
 nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 {
 	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
 	struct nvkm_bios *bios = subdev->device->bios;
-	struct nvkm_i2c *i2c = subdev->device->i2c;
 	struct nvbios_iccsense stbl;
 	int i;
 
-	if (!i2c || !bios || nvbios_iccsense_parse(bios, &stbl)
-	    || !stbl.nr_entry)
+	if (!bios || nvbios_iccsense_parse(bios, &stbl) || !stbl.nr_entry)
 		return 0;
 
 	iccsense->data_valid = true;
 	for (i = 0; i < stbl.nr_entry; ++i) {
 		struct pwr_rail_t *r = &stbl.rail[i];
-		struct nvbios_extdev_func extdev;
 		struct nvkm_iccsense_rail *rail;
-		struct nvkm_i2c_bus *i2c_bus;
-		u8 addr;
+		struct nvkm_iccsense_sensor *sensor;
 
 		if (!r->mode || r->resistor_mohm == 0)
 			continue;
 
-		if (nvbios_extdev_parse(bios, r->extdev_id, &extdev))
-			continue;
-
-		if (extdev.type == 0xff)
-			continue;
-
-		if (extdev.bus)
-			i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_SEC);
-		else
-			i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_PRI);
-		if (!i2c_bus)
+		sensor = nvkm_iccsense_get_sensor(iccsense, r->extdev_id);
+		if (!sensor)
 			continue;
 
-		addr = extdev.addr >> 1;
-		if (!nvkm_iccsense_validate_device(&i2c_bus->i2c, addr,
-						   extdev.type, r->rail)) {
-			iccsense->data_valid = false;
-			nvkm_warn(subdev, "found unknown or invalid rail entry"
-				  " type 0x%x rail %i, power reading might be"
-				  " invalid\n", extdev.type, r->rail);
-			continue;
-		}
-
 		rail = kmalloc(sizeof(*rail), GFP_KERNEL);
 		if (!rail)
 			return -ENOMEM;
 
-		switch (extdev.type) {
+		switch (sensor->type) {
 		case NVBIOS_EXTDEV_INA209:
+			if (r->rail != 0)
+				continue;
 			rail->read = nvkm_iccsense_ina209_read;
 			break;
 		case NVBIOS_EXTDEV_INA219:
+			if (r->rail != 0)
+				continue;
 			rail->read = nvkm_iccsense_ina219_read;
 			break;
 		case NVBIOS_EXTDEV_INA3221:
+			if (r->rail >= 3)
+				continue;
 			rail->read = nvkm_iccsense_ina3221_read;
 			break;
+		default:
+			continue;
 		}
 
-		rail->addr = addr;
-		rail->rail = r->rail;
+		sensor->rail_mask |= 1 << r->rail;
+		rail->sensor = sensor;
+		rail->idx = r->rail;
 		rail->mohm = r->resistor_mohm;
-		rail->i2c = &i2c_bus->i2c;
 		list_add_tail(&rail->head, &iccsense->rails);
 	}
 	return 0;
@@ -219,6 +275,7 @@ nvkm_iccsense_new_(struct nvkm_device *device, int index,
 {
 	if (!(*iccsense = kzalloc(sizeof(**iccsense), GFP_KERNEL)))
 		return -ENOMEM;
+	INIT_LIST_HEAD(&(*iccsense)->sensors);
 	INIT_LIST_HEAD(&(*iccsense)->rails);
 	nvkm_iccsense_ctor(device, index, *iccsense);
 	return 0;
diff --git a/drm/nouveau/nvkm/subdev/iccsense/priv.h
b/drm/nouveau/nvkm/subdev/iccsense/priv.h
index e479128..b72c31d 100644
--- a/drm/nouveau/nvkm/subdev/iccsense/priv.h
+++ b/drm/nouveau/nvkm/subdev/iccsense/priv.h
@@ -2,13 +2,22 @@
 #define __NVKM_ICCSENSE_PRIV_H__
 #define nvkm_iccsense(p) container_of((p), struct nvkm_iccsense, subdev)
 #include <subdev/iccsense.h>
+#include <subdev/bios/extdev.h>
 
-struct nvkm_iccsense_rail {
+struct nvkm_iccsense_sensor {
 	struct list_head head;
-	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
+	int id;
+	enum nvbios_extdev_type type;
 	struct i2c_adapter *i2c;
 	u8 addr;
-	u8 rail;
+	u8 rail_mask;
+};
+
+struct nvkm_iccsense_rail {
+	struct list_head head;
+	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
+	struct nvkm_iccsense_sensor *sensor;
+	u8 idx;
 	u8 mohm;
 };
 
-- 
2.7.4

Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
---
 drm/nouveau/nvkm/subdev/iccsense/base.c | 68 +++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
index b6f6222..6f3709e 100644
--- a/drm/nouveau/nvkm/subdev/iccsense/base.c
+++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
@@ -95,6 +95,63 @@ nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
 				       40 * 8);
 }
 
+static void
+nvkm_iccsense_ina2x9_config(struct nvkm_iccsense *iccsense,
+			    struct nvkm_iccsense_sensor *sensor)
+{
+	struct nvkm_subdev *subdev = &iccsense->subdev;
+	/* configuration:
+	 * 0x0007: 0x0007 shunt and bus continous
+	 * 0x0078: 0x0078 128 samples shunt
+	 * 0x0780: 0x0780 128 samples bus
+	 * 0x1800: 0x0000 +-40 mV shunt range
+	 * 0x2000: 0x0000 16V FSR
+         */
+	u16 value = 0x07ff;
+	nvkm_debug(subdev, "config for sensor id %i: 0x%x\n", sensor->id,
value);
+	nv_wr16i2cr(sensor->i2c, sensor->addr, 0x00, value);
+}
+
+static void
+nvkm_iccsense_ina3221_config(struct nvkm_iccsense *iccsense,
+			     struct nvkm_iccsense_sensor *sensor)
+{
+	struct nvkm_subdev *subdev = &iccsense->subdev;
+	/* configuration:
+	 * 0x0007: 0x0007 shunt and bus continous
+	 * 0x0031: 0x0000 140 us conversion time shunt
+	 * 0x01c0: 0x0000 140 us conversion time bus
+	 * 0x0f00: 0x0f00 1024 samples
+	 * 0x7000: 0x?000 channels
+         */
+	u16 value = 0x0e07;
+	if (sensor->rail_mask & 0x1)
+		value |= 0x1 << 14;
+	if (sensor->rail_mask & 0x2)
+		value |= 0x1 << 13;
+	if (sensor->rail_mask & 0x4)
+		value |= 0x1 << 12;
+	nvkm_debug(subdev, "config for sensor id %i: 0x%x\n", sensor->id,
value);
+	nv_wr16i2cr(sensor->i2c, sensor->addr, 0x00, value);
+}
+
+static void
+nvkm_iccsense_sensor_config(struct nvkm_iccsense *iccsense,
+		            struct nvkm_iccsense_sensor *sensor)
+{
+	switch (sensor->type) {
+	case NVBIOS_EXTDEV_INA209:
+	case NVBIOS_EXTDEV_INA219:
+		nvkm_iccsense_ina2x9_config(iccsense, sensor);
+		break;
+	case NVBIOS_EXTDEV_INA3221:
+		nvkm_iccsense_ina3221_config(iccsense, sensor);
+		break;
+	default:
+		break;
+	}
+}
+
 int
 nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
 {
@@ -257,8 +314,19 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 	return 0;
 }
 
+static int
+nvkm_iccsense_init(struct nvkm_subdev *subdev)
+{
+	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
+	struct nvkm_iccsense_sensor *sensor;
+	list_for_each_entry(sensor, &iccsense->sensors, head)
+		nvkm_iccsense_sensor_config(iccsense, sensor);
+	return 0;
+}
+
 struct nvkm_subdev_func iccsense_func = {
 	.oneinit = nvkm_iccsense_oneinit,
+	.init = nvkm_iccsense_init,
 	.dtor = nvkm_iccsense_dtor,
 };
 
-- 
2.7.4

On 25/03/16 13:19, Karol Herbst wrote:
> Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
> ---
>   drm/nouveau/include/nvkm/subdev/iccsense.h |  4 +---
>   drm/nouveau/nouveau_hwmon.c                |  2 +-
>   drm/nouveau/nvkm/subdev/iccsense/base.c    | 32
+++++++++++++-----------------
>   drm/nouveau/nvkm/subdev/iccsense/priv.h    |  1 +
>   4 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/iccsense.h
b/drm/nouveau/include/nvkm/subdev/iccsense.h
> index c3defcd..a4c0da0 100644
> --- a/drm/nouveau/include/nvkm/subdev/iccsense.h
> +++ b/drm/nouveau/include/nvkm/subdev/iccsense.h
> @@ -3,12 +3,10 @@
>   
>   #include <core/subdev.h>
>   
> -struct nkvm_iccsense_rail;
>   struct nvkm_iccsense {
>   	struct nvkm_subdev subdev;
> -	u8 rail_count;
>   	bool data_valid;
> -	struct nvkm_iccsense_rail *rails;
> +	struct list_head rails;
>   };
>   
>   int gf100_iccsense_new(struct nvkm_device *, int index, struct
nvkm_iccsense **);
> diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
> index 67edd2f..74f237b 100644
> --- a/drm/nouveau/nouveau_hwmon.c
> +++ b/drm/nouveau/nouveau_hwmon.c
> @@ -689,7 +689,7 @@ nouveau_hwmon_init(struct drm_device *dev)
>   			goto error;
>   	}
>   
> -	if (iccsense && iccsense->data_valid &&
iccsense->rail_count) {
> +	if (iccsense && iccsense->data_valid &&
!list_empty(&iccsense->rails)) {
>   		ret = sysfs_create_group(&hwmon_dev->kobj,
>   					 &hwmon_power_attrgroup);
>   		if (ret)
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
> index bf1b94e..6fde68d 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/base.c
> +++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
> @@ -98,25 +98,21 @@ nvkm_iccsense_ina3221_read(struct nvkm_iccsense
*iccsense,
>   int
>   nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
>   {
> -	int result = 0, i;
> +	int result = 0;
> +	struct nvkm_iccsense_rail *rail;
>   
>   	if (!iccsense)
>   		return -EINVAL;
>   
> -	if (iccsense->rail_count == 0)
> -		return -ENODEV;
> -
> -	for (i = 0; i < iccsense->rail_count; ++i) {
> +	list_for_each_entry(rail, &iccsense->rails, head) {
>   		int res;
> -		struct nvkm_iccsense_rail *rail = &iccsense->rails[i];
>   		if (!rail->read)
>   			return -ENODEV;
>   
>   		res = rail->read(iccsense, rail);
> -		if (res >= 0)
> -			result += res;
> -		else
> +		if (res < 0)
>   			return res;
> +		result += res;
>   	}
>   	return result;
>   }
> @@ -125,9 +121,10 @@ static void *
>   nvkm_iccsense_dtor(struct nvkm_subdev *subdev)
>   {
>   	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
> +	struct nvkm_iccsense_rail *rail, *tmp;
>   
> -	if (iccsense->rails)
> -		kfree(iccsense->rails);
> +	list_for_each_entry_safe(rail, tmp, &iccsense->rails, head)
> +		kfree(rail);
The rails list is filled with invalid pointers at this point. Please add 
list_del(rail); before kfree(rail);

It has the added benefit of adding poisonous pointers that will show any 
access after freeing.
>   
>   	return iccsense;
>   }
> @@ -145,11 +142,6 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   	    || !stbl.nr_entry)
>   		return 0;
>   
> -	iccsense->rails = kmalloc(sizeof(*iccsense->rails) * stbl.nr_entry,
> -	                          GFP_KERNEL);
> -	if (!iccsense->rails)
> -		return -ENOMEM;
> -
>   	iccsense->data_valid = true;
>   	for (i = 0; i < stbl.nr_entry; ++i) {
>   		struct pwr_rail_t *r = &stbl.rail[i];
> @@ -184,7 +176,10 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   			continue;
>   		}
>   
> -		rail = &iccsense->rails[iccsense->rail_count];
> +		rail = kmalloc(sizeof(*rail), GFP_KERNEL);
> +		if (!rail)
> +			return -ENOMEM;
> +
>   		switch (extdev.type) {
>   		case NVBIOS_EXTDEV_INA209:
>   			rail->read = nvkm_iccsense_ina209_read;
> @@ -201,7 +196,7 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   		rail->rail = r->rail;
>   		rail->mohm = r->resistor_mohm;
>   		rail->i2c = &i2c_bus->i2c;
> -		++iccsense->rail_count;
> +		list_add_tail(&rail->head, &iccsense->rails);
>   	}
>   	return 0;
>   }
> @@ -224,6 +219,7 @@ nvkm_iccsense_new_(struct nvkm_device *device, int
index,
>   {
>   	if (!(*iccsense = kzalloc(sizeof(**iccsense), GFP_KERNEL)))
>   		return -ENOMEM;
> +	INIT_LIST_HEAD(&(*iccsense)->rails);
>   	nvkm_iccsense_ctor(device, index, *iccsense);
>   	return 0;
>   }
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/priv.h
b/drm/nouveau/nvkm/subdev/iccsense/priv.h
> index ed398b8..e479128 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/priv.h
> +++ b/drm/nouveau/nvkm/subdev/iccsense/priv.h
> @@ -4,6 +4,7 @@
>   #include <subdev/iccsense.h>
>   
>   struct nvkm_iccsense_rail {
> +	struct list_head head;
>   	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
>   	struct i2c_adapter *i2c;
>   	u8 addr;

On 25/03/16 13:19, Karol Herbst wrote:
> Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
> ---
>   drm/nouveau/include/nvkm/subdev/iccsense.h |   1 +
>   drm/nouveau/nvkm/subdev/iccsense/base.c    | 141
++++++++++++++++++++---------
>   drm/nouveau/nvkm/subdev/iccsense/priv.h    |  15 ++-
>   3 files changed, 112 insertions(+), 45 deletions(-)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/iccsense.h
b/drm/nouveau/include/nvkm/subdev/iccsense.h
> index a4c0da0..3c2ddd9 100644
> --- a/drm/nouveau/include/nvkm/subdev/iccsense.h
> +++ b/drm/nouveau/include/nvkm/subdev/iccsense.h
> @@ -6,6 +6,7 @@
>   struct nvkm_iccsense {
>   	struct nvkm_subdev subdev;
>   	bool data_valid;
> +	struct list_head sensors;
>   	struct list_head rails;
>   };
>   
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
> index 6fde68d..b6f6222 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/base.c
> +++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
> @@ -30,15 +30,14 @@
>   
>   static bool
>   nvkm_iccsense_validate_device(struct i2c_adapter *i2c, u8 addr,
> -			      enum nvbios_extdev_type type, u8 rail)
> +			      enum nvbios_extdev_type type)
>   {
>   	switch (type) {
>   	case NVBIOS_EXTDEV_INA209:
>   	case NVBIOS_EXTDEV_INA219:
> -		return rail == 0 && nv_rd16i2cr(i2c, addr, 0x0) >= 0;
> +		return nv_rd16i2cr(i2c, addr, 0x0) >= 0;
>   	case NVBIOS_EXTDEV_INA3221:
> -		return rail <= 3 &&
> -		       nv_rd16i2cr(i2c, addr, 0xff) == 0x3220 &&
> +		return nv_rd16i2cr(i2c, addr, 0xff) == 0x3220 &&
>   		       nv_rd16i2cr(i2c, addr, 0xfe) == 0x5449;
>   	default:
>   		return false;
> @@ -67,8 +66,9 @@ nvkm_iccsense_ina2x9_read(struct nvkm_iccsense *iccsense,
>                             struct nvkm_iccsense_rail *rail,
>   			  u8 shunt_reg, u8 bus_reg)
>   {
> -	return nvkm_iccsense_poll_lane(rail->i2c, rail->addr, shunt_reg, 0,
> -				       bus_reg, 3, rail->mohm, 10 * 4);
> +	return nvkm_iccsense_poll_lane(rail->sensor->i2c,
rail->sensor->addr,
> +				       shunt_reg, 0, bus_reg, 3, rail->mohm,
> +				       10 * 4);
>   }
>   
>   static int
> @@ -89,9 +89,9 @@ static int
>   nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
>   			   struct nvkm_iccsense_rail *rail)
>   {
> -	return nvkm_iccsense_poll_lane(rail->i2c, rail->addr,
> -				       1 + (rail->rail * 2), 3,
> -				       2 + (rail->rail * 2), 3, rail->mohm,
> +	return nvkm_iccsense_poll_lane(rail->sensor->i2c,
rail->sensor->addr,
> +				       1 + (rail->idx * 2), 3,
> +				       2 + (rail->idx * 2), 3, rail->mohm,
>   				       40 * 8);
>   }
>   
> @@ -121,81 +121,137 @@ static void *
>   nvkm_iccsense_dtor(struct nvkm_subdev *subdev)
>   {
>   	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
> -	struct nvkm_iccsense_rail *rail, *tmp;
> +	struct nvkm_iccsense_sensor *sensor, *tmps;
> +	struct nvkm_iccsense_rail *rail, *tmpr;
>   
> -	list_for_each_entry_safe(rail, tmp, &iccsense->rails, head)
> +	list_for_each_entry_safe(sensor, tmps, &iccsense->sensors, head)
> +		kfree(sensor);
> +	list_for_each_entry_safe(rail, tmpr, &iccsense->rails, head)
>   		kfree(rail);
Same problem as the last patch.
>   
>   	return iccsense;
>   }
>   
> +static struct nvkm_iccsense_sensor*
> +nvkm_iccsense_create_sensor(struct nvkm_iccsense *iccsense, u8 id)
> +{
> +
> +	struct nvkm_subdev *subdev = &iccsense->subdev;
> +	struct nvkm_bios *bios = subdev->device->bios;
> +	struct nvkm_i2c *i2c = subdev->device->i2c;
> +	struct nvbios_extdev_func extdev;
> +	struct nvkm_i2c_bus *i2c_bus;
> +	struct nvkm_iccsense_sensor *sensor;
> +	u8 addr;
> +
> +	if (!i2c || !bios || nvbios_extdev_parse(bios, id, &extdev))
> +		return NULL;
> +
> +	if (extdev.type == 0xff)
> +		return NULL;
> +
> +	if (extdev.type != NVBIOS_EXTDEV_INA209 &&
> +	    extdev.type != NVBIOS_EXTDEV_INA219 &&
> +	    extdev.type != NVBIOS_EXTDEV_INA3221) {
> +		iccsense->data_valid = false;
> +		nvkm_error(subdev, "found unknown sensor type %x, "
> +			   "power reading might be invalid\n",
> +			   extdev.type);
You do not create the sensor if it is unknown, so why do you say the 
report might be invalid?

Please change it to "Unknown sensor type %x".
> +		return NULL;
> +	}
> +
> +	if (extdev.bus)
> +		i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_SEC);
> +	else
> +		i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_PRI);
> +	if (!i2c_bus)
> +		return NULL;
> +
> +	addr = extdev.addr >> 1;
> +	if (!nvkm_iccsense_validate_device(&i2c_bus->i2c, addr,
> +					   extdev.type)) {
> +		iccsense->data_valid = false;
> +		nvkm_warn(subdev, "found invalid sensor id: %i, power reading"
> +			  "might be invalid\n", id);
> +		return NULL;
> +	}
> +
> +	sensor = kmalloc(sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return NULL;
> +
> +	list_add_tail(&sensor->head, &iccsense->sensors);
> +	sensor->id = id;
> +	sensor->type = extdev.type;
> +	sensor->i2c = &i2c_bus->i2c;
> +	sensor->addr = addr;
> +	sensor->rail_mask = 0x0;
> +	return sensor;
> +}
> +
> +static struct nvkm_iccsense_sensor*
> +nvkm_iccsense_get_sensor(struct nvkm_iccsense *iccsense, u8 id)
> +{
> +	struct nvkm_iccsense_sensor *sensor;
> +	list_for_each_entry(sensor, &iccsense->sensors, head) {
> +		if (sensor->id == id)
> +			return sensor;
> +	}
> +	return nvkm_iccsense_create_sensor(iccsense, id);
> +}
> +
>   static int
>   nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   {
>   	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
>   	struct nvkm_bios *bios = subdev->device->bios;
> -	struct nvkm_i2c *i2c = subdev->device->i2c;
>   	struct nvbios_iccsense stbl;
>   	int i;
>   
> -	if (!i2c || !bios || nvbios_iccsense_parse(bios, &stbl)
> -	    || !stbl.nr_entry)
> +	if (!bios || nvbios_iccsense_parse(bios, &stbl) || !stbl.nr_entry)
>   		return 0;
>   
>   	iccsense->data_valid = true;
>   	for (i = 0; i < stbl.nr_entry; ++i) {
>   		struct pwr_rail_t *r = &stbl.rail[i];
> -		struct nvbios_extdev_func extdev;
>   		struct nvkm_iccsense_rail *rail;
> -		struct nvkm_i2c_bus *i2c_bus;
> -		u8 addr;
> +		struct nvkm_iccsense_sensor *sensor;
>   
>   		if (!r->mode || r->resistor_mohm == 0)
>   			continue;
>   
> -		if (nvbios_extdev_parse(bios, r->extdev_id, &extdev))
> -			continue;
> -
> -		if (extdev.type == 0xff)
> -			continue;
> -
> -		if (extdev.bus)
> -			i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_SEC);
> -		else
> -			i2c_bus = nvkm_i2c_bus_find(i2c, NVKM_I2C_BUS_PRI);
> -		if (!i2c_bus)
> +		sensor = nvkm_iccsense_get_sensor(iccsense, r->extdev_id);
> +		if (!sensor)
>   			continue;
>   
> -		addr = extdev.addr >> 1;
> -		if (!nvkm_iccsense_validate_device(&i2c_bus->i2c, addr,
> -						   extdev.type, r->rail)) {
> -			iccsense->data_valid = false;
> -			nvkm_warn(subdev, "found unknown or invalid rail entry"
> -				  " type 0x%x rail %i, power reading might be"
> -				  " invalid\n", extdev.type, r->rail);
> -			continue;
> -		}
> -
>   		rail = kmalloc(sizeof(*rail), GFP_KERNEL);
>   		if (!rail)
>   			return -ENOMEM;
>   
> -		switch (extdev.type) {
> +		switch (sensor->type) {
>   		case NVBIOS_EXTDEV_INA209:
> +			if (r->rail != 0)
> +				continue;
>   			rail->read = nvkm_iccsense_ina209_read;
>   			break;
>   		case NVBIOS_EXTDEV_INA219:
> +			if (r->rail != 0)
> +				continue;
>   			rail->read = nvkm_iccsense_ina219_read;
>   			break;
>   		case NVBIOS_EXTDEV_INA3221:
> +			if (r->rail >= 3)
> +				continue;
>   			rail->read = nvkm_iccsense_ina3221_read;
>   			break;
> +		default:
> +			continue;
>   		}
>   
> -		rail->addr = addr;
> -		rail->rail = r->rail;
> +		sensor->rail_mask |= 1 << r->rail;
> +		rail->sensor = sensor;
> +		rail->idx = r->rail;
>   		rail->mohm = r->resistor_mohm;
> -		rail->i2c = &i2c_bus->i2c;
>   		list_add_tail(&rail->head, &iccsense->rails);
>   	}
>   	return 0;
> @@ -219,6 +275,7 @@ nvkm_iccsense_new_(struct nvkm_device *device, int
index,
>   {
>   	if (!(*iccsense = kzalloc(sizeof(**iccsense), GFP_KERNEL)))
>   		return -ENOMEM;
> +	INIT_LIST_HEAD(&(*iccsense)->sensors);
>   	INIT_LIST_HEAD(&(*iccsense)->rails);
>   	nvkm_iccsense_ctor(device, index, *iccsense);
>   	return 0;
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/priv.h
b/drm/nouveau/nvkm/subdev/iccsense/priv.h
> index e479128..b72c31d 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/priv.h
> +++ b/drm/nouveau/nvkm/subdev/iccsense/priv.h
> @@ -2,13 +2,22 @@
>   #define __NVKM_ICCSENSE_PRIV_H__
>   #define nvkm_iccsense(p) container_of((p), struct nvkm_iccsense, subdev)
>   #include <subdev/iccsense.h>
> +#include <subdev/bios/extdev.h>
>   
> -struct nvkm_iccsense_rail {
> +struct nvkm_iccsense_sensor {
>   	struct list_head head;
> -	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
> +	int id;
> +	enum nvbios_extdev_type type;
>   	struct i2c_adapter *i2c;
>   	u8 addr;
> -	u8 rail;
> +	u8 rail_mask;
> +};
> +
> +struct nvkm_iccsense_rail {
> +	struct list_head head;
> +	int (*read)(struct nvkm_iccsense *, struct nvkm_iccsense_rail *);
> +	struct nvkm_iccsense_sensor *sensor;
> +	u8 idx;
>   	u8 mohm;
>   };
>

On 25/03/16 13:19, Karol Herbst wrote:
> Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
> ---
>   drm/nouveau/nvkm/subdev/iccsense/base.c | 68
+++++++++++++++++++++++++++++++++
>   1 file changed, 68 insertions(+)
>
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c
b/drm/nouveau/nvkm/subdev/iccsense/base.c
> index b6f6222..6f3709e 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/base.c
> +++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
> @@ -95,6 +95,63 @@ nvkm_iccsense_ina3221_read(struct nvkm_iccsense
*iccsense,
>   				       40 * 8);
>   }
>   
> +static void
> +nvkm_iccsense_ina2x9_config(struct nvkm_iccsense *iccsense,
> +			    struct nvkm_iccsense_sensor *sensor)
Maybe calling the sensor ina209 and using it also for 219 would be less 
confusing, especially if a 229 is later released by TI.
> +{
> +	struct nvkm_subdev *subdev = &iccsense->subdev;
> +	/* configuration:
> +	 * 0x0007: 0x0007 shunt and bus continous
> +	 * 0x0078: 0x0078 128 samples shunt
> +	 * 0x0780: 0x0780 128 samples bus
> +	 * 0x1800: 0x0000 +-40 mV shunt range
> +	 * 0x2000: 0x0000 16V FSR
> +         */
> +	u16 value = 0x07ff;
> +	nvkm_debug(subdev, "config for sensor id %i: 0x%x\n",
sensor->id, value);
> +	nv_wr16i2cr(sensor->i2c, sensor->addr, 0x00, value);
> +}
> +
> +static void
> +nvkm_iccsense_ina3221_config(struct nvkm_iccsense *iccsense,
> +			     struct nvkm_iccsense_sensor *sensor)
> +{
> +	struct nvkm_subdev *subdev = &iccsense->subdev;
> +	/* configuration:
> +	 * 0x0007: 0x0007 shunt and bus continous
> +	 * 0x0031: 0x0000 140 us conversion time shunt
> +	 * 0x01c0: 0x0000 140 us conversion time bus
> +	 * 0x0f00: 0x0f00 1024 samples
> +	 * 0x7000: 0x?000 channels
> +         */
> +	u16 value = 0x0e07;
> +	if (sensor->rail_mask & 0x1)
> +		value |= 0x1 << 14;
> +	if (sensor->rail_mask & 0x2)
> +		value |= 0x1 << 13;
> +	if (sensor->rail_mask & 0x4)
> +		value |= 0x1 << 12;
> +	nvkm_debug(subdev, "config for sensor id %i: 0x%x\n",
sensor->id, value);
> +	nv_wr16i2cr(sensor->i2c, sensor->addr, 0x00, value);
> +}
> +
> +static void
> +nvkm_iccsense_sensor_config(struct nvkm_iccsense *iccsense,
> +		            struct nvkm_iccsense_sensor *sensor)
> +{
> +	switch (sensor->type) {
> +	case NVBIOS_EXTDEV_INA209:
> +	case NVBIOS_EXTDEV_INA219:
> +		nvkm_iccsense_ina2x9_config(iccsense, sensor);
> +		break;
> +	case NVBIOS_EXTDEV_INA3221:
> +		nvkm_iccsense_ina3221_config(iccsense, sensor);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>   int
>   nvkm_iccsense_read_all(struct nvkm_iccsense *iccsense)
>   {
> @@ -257,8 +314,19 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>   	return 0;
>   }
>   
> +static int
> +nvkm_iccsense_init(struct nvkm_subdev *subdev)
> +{
> +	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
> +	struct nvkm_iccsense_sensor *sensor;
> +	list_for_each_entry(sensor, &iccsense->sensors, head)
> +		nvkm_iccsense_sensor_config(iccsense, sensor);
> +	return 0;
> +}
> +
>   struct nvkm_subdev_func iccsense_func = {
>   	.oneinit = nvkm_iccsense_oneinit,
> +	.init = nvkm_iccsense_init,
>   	.dtor = nvkm_iccsense_dtor,
>   };
>   
Looks like a good cleanup and improvement to me!

With the free-ing the lists fixed and maybe the change in name for the 
ina2x9, this is:
Martin Peres <martin.peres at free.fr>