Re: [PATCH v4 3/6] iccsense: implement for ina209, ina219 and ina3221

[Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>]

On 20/02/16 19:11, Karol Herbst wrote:
> based on Martins initial work
>
> v3: fix ina2x9 calculations
> v4: don't kmalloc(0), fix the lsb/pga stuff
>
> Signed-off-by: Karol Herbst <nouveau@karolherbst.de>
> ---
>   drm/nouveau/include/nvkm/subdev/bios/extdev.h |   3 +
>   drm/nouveau/include/nvkm/subdev/i2c.h         |  31 ++++++
>   drm/nouveau/include/nvkm/subdev/iccsense.h    |   5 +
>   drm/nouveau/nvkm/engine/device/base.c         |  20 ++++
>   drm/nouveau/nvkm/subdev/iccsense/Kbuild       |   1 +
>   drm/nouveau/nvkm/subdev/iccsense/base.c       | 150 +++++++++++++++++++++++++-
>   drm/nouveau/nvkm/subdev/iccsense/gf100.c      |  31 ++++++
>   drm/nouveau/nvkm/subdev/iccsense/priv.h       |   8 ++
>   8 files changed, 248 insertions(+), 1 deletion(-)
>   create mode 100644 drm/nouveau/nvkm/subdev/iccsense/gf100.c
>
> diff --git a/drm/nouveau/nvkm/subdev/iccsense/base.c b/drm/nouveau/nvkm/subdev/iccsense/base.c
> index 5dfa2fd..29c6641 100644
> --- a/drm/nouveau/nvkm/subdev/iccsense/base.c
> +++ b/drm/nouveau/nvkm/subdev/iccsense/base.c
> @@ -23,13 +23,161 @@
>    */
>   #include "priv.h"
>   
> -struct nvkm_subdev_func iccsense_func = { 0 };
> +#include <subdev/bios.h>
> +#include <subdev/bios/extdev.h>
> +#include <subdev/bios/iccsense.h>
> +#include <subdev/i2c.h>
> +
> +static int
> +nvkm_iccsense_poll_lane(struct i2c_adapter *i2c, u8 addr, u8 shunt_reg,
> +			u8 shunt_shift, u8 bus_reg, u8 bus_shift, u8 shunt,
> +			u16 lsb)
> +{
> +	int vbus, vshunt;
> +
> +	if (shunt == 0)
> +		return 0;
> +
> +	vshunt = nv_rd16i2cr(i2c, addr, shunt_reg);
> +	vbus = nv_rd16i2cr(i2c, addr, bus_reg);
> +
> +	if (vshunt < 0 || vbus < 0)
> +		return -EINVAL;
> +
> +	vshunt >>= shunt_shift;
> +	vbus >>= bus_shift;
> +
> +	return (vbus * vshunt * lsb) / shunt;
> +}
> +
> +static int
> +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);
> +}
> +
> +static int
> +nvkm_iccsense_ina209_read(struct nvkm_iccsense *iccsense,
> +			  struct nvkm_iccsense_rail *rail)
> +{
> +	return nvkm_iccsense_ina2x9_read(iccsense, rail, 3, 4);
> +}
> +
> +static int
> +nvkm_iccsense_ina219_read(struct nvkm_iccsense *iccsense,
> +			  struct nvkm_iccsense_rail *rail)
> +{
> +	return nvkm_iccsense_ina2x9_read(iccsense, rail, 1, 2);
> +}
> +
> +static int
> +nvkm_iccsense_ina3221_read(struct nvkm_iccsense *iccsense,
> +			   struct nvkm_iccsense_rail *rail)
> +{
> +	if (rail->rail >= 3)
> +		return -EINVAL;
> +
> +	return nvkm_iccsense_poll_lane(rail->i2c, rail->addr,
> +				       1 + (rail->rail * 2), 3,
> +				       2 + (rail->rail * 2), 3, rail->mohm,
> +				       40 * 8);
> +}
> +
> +int
> +nkvm_iccsense_read(struct nvkm_iccsense *iccsense, u8 idx)
> +{
> +	struct nvkm_iccsense_rail *rail;
> +
> +	if (!iccsense || idx >= iccsense->rail_count)
> +		return -EINVAL;
> +
> +	rail = &iccsense->rails[idx];
> +	if (!rail->read)
> +		return -ENODEV;
> +
> +	return rail->read(iccsense, rail);
> +}
> +
> +static void *
> +nvkm_iccsense_dtor(struct nvkm_subdev *subdev)
> +{
> +	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
> +
> +	if (iccsense->rails)
> +		kfree(iccsense->rails);
> +
> +	return iccsense;
> +}
> +
> +struct nvkm_subdev_func iccsense_func = {
> +	.dtor = nvkm_iccsense_dtor,
> +};
>   
>   int
>   nvkm_iccsense_ctor(struct nvkm_device *device, int index,
>   		   struct nvkm_iccsense *iccsense)
>   {
> +	struct nvkm_bios *bios;
> +	struct nvkm_i2c *i2c;
> +	struct nvbios_iccsense stbl;
> +	int i;
> +
>   	nvkm_subdev_ctor(&iccsense_func, device, index, 0, &iccsense->subdev);
> +	bios = device->bios;
> +	i2c = device->i2c;
> +
> +	if (!i2c || !bios || nvbios_iccsense_parse(bios, &stbl)
> +	    || !stbl.nr_entry)
I must say that this line is a bit ugly ... but meh!
> +		return 0;
> +
> +	iccsense->rails = kmalloc(sizeof(*iccsense->rails) * stbl.nr_entry,
> +	                          GFP_KERNEL);
> +	if (!iccsense->rails)
> +		return -ENOMEM;
> +
> +	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;
> +
> +		if (!r->mode)
> +			continue;
> +
> +		if (nvbios_extdev_parse(bios, r->extdev_id, &extdev))
> +			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)
> +			continue;

Wow, this is great! Thanks, I always wondered how I would get the bus 
working
and it seems like you found a nice way.

> +
> +		rail = &iccsense->rails[iccsense->rail_count];
> +		switch (extdev.type) {
> +		case NVBIOS_EXTDEV_INA209:
> +			rail->read = nvkm_iccsense_ina209_read;
> +			break;
> +		case NVBIOS_EXTDEV_INA219:
> +			rail->read = nvkm_iccsense_ina219_read;
> +			break;
> +		case NVBIOS_EXTDEV_INA3221:
> +			rail->read = nvkm_iccsense_ina3221_read;
> +			break;
> +		default:

It would be nice to add a warning here that there is a new sensor type 
we do not know about yet. This is
especially since some rails may be covered by other devices which would 
be supported and an invalid power
reading would be present.

I would say that more than just displaying a warning, we should also 
change a boolean which would say how
trustful the reading is. This would be good when enabling or disabling 
the usage of boost clocks. Don't you think?

> +			continue;
> +		}
> +		rail->addr = extdev.addr >> 1;
> +		rail->rail = r->rail;
> +		rail->mohm = r->resistor_mohm;
> +		rail->i2c = &i2c_bus->i2c;
> +		++iccsense->rail_count;

There is no verification that the device is actually present. The extdev 
table is untrustful and I really wouldn't mind
more validation here :)

Other than that, nice work! I really like the design :)

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau