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
Karol Herbst
2016-Mar-25 11:19 UTC
[Nouveau] [PATCH 3/4] iccsense: split sensor into own struct
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
Karol Herbst
2016-Mar-25 11:19 UTC
[Nouveau] [PATCH 4/4] iccsense: configure sensors like nvidia does
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;
Martin Peres
2016-Mar-28 10:36 UTC
[Nouveau] [PATCH 3/4] iccsense: split sensor into own struct
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; > }; >
Martin Peres
2016-Mar-28 10:40 UTC
[Nouveau] [PATCH 4/4] iccsense: configure sensors like nvidia does
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>