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>
Reasonably Related Threads
- [PATCH] nvkm/iccsense: Parse the resistors and config the right way
- [PATCH 3/4] iccsense: split sensor into own struct
- [PATCH 0/4] Configure Power Sensors
- [PATCH v2 0/4] Configure Power Sensors
- [PATCH v4 3/6] iccsense: implement for ina209, ina219 and ina3221