[Nouveau] [PATCH 14/14] drm/nouveau/therm: Rework nouveau_therm_create()

[Ben Skeggs]

On Mon, May 21, 2012 at 12:15:03AM +0100, Emil Velikov wrote:
> It contains a few changes mainly targeting the following
>  * Therm table is present in BIT vbios
>  * Parse the vbios table before hooking temp_get(), as it contains the therm
> sensor calibration data
>  * Add dummy_therm_temp_get() function to prevent multiple null dereff's

I didn't take this patch at all yet.  I'll let Martin put his input into
this instead.  I didn't really touch the thermal stuff aside from matching
APIs because he's got an overhaul pending anyway.

Comments on specific pieces inline as they may be useful.

> 
> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_pm.c    |    2 +-
>  drivers/gpu/drm/nouveau/nouveau_therm.c |   63 ++++++++++++++++++++++++-------
>  2 files changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_pm.c b/drivers/gpu/drm/nouveau/nouveau_pm.c
> index 9dd34fe..1b4422b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_pm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_pm.c
> @@ -693,7 +693,7 @@ nouveau_hwmon_init(struct nouveau_device *ndev)
>  	}
>  
>  	/* if the card can read the fan rpm */
> -	if (nouveau_gpio_func_valid(ndev, DCB_GPIO_FAN_SENSE)) {
> +	if (pfan && pfan->sense(pfan) >= 0) {
>  		ret = sysfs_create_group(&dev->pdev->dev.kobj,
>  					 &hwmon_fan_rpm_attrgroup);
>  		if (ret)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_therm.c b/drivers/gpu/drm/nouveau/nouveau_therm.c
> index acf81a9..91095be 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_therm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_therm.c
> @@ -30,6 +30,12 @@
>  #include "nouveau_pm.h"
>  #include "nouveau_therm.h"
>  
> +static inline int
> +dummy_therm_temp_get(struct nouveau_therm *ptherm)
> +{
> +	return 0;
> +}
> +
I don't really like this, if we can't expose any thermal data I think we
just shouldn't create a thermal subdev?

>  static int
>  nv40_sensor_setup(struct nouveau_therm *ptherm)
>  {
> @@ -181,7 +187,7 @@ nouveau_therm_vbios_parse(struct nouveau_therm *ptherm, u8 *temp)
>  	temp = temp + headerlen;
>  
>  	/* Read the entries from the table */
> -	for (i = 0; i < entries; i++) {
> +	for (i = 0; i < entries; i++, temp += recordlen) {
>  		s16 value = ROM16(temp[1]);
>  
>  		switch (temp[0]) {
> @@ -228,7 +234,6 @@ nouveau_therm_vbios_parse(struct nouveau_therm *ptherm, u8 *temp)
>  			ptherm->fan.pwm_freq = value;
>  			break;
>  		}
> -		temp += recordlen;
>  	}
>  
>  	nouveau_therm_safety_checks(ptherm);
> @@ -299,6 +304,12 @@ nouveau_therm_probe_i2c(struct nouveau_device *ndev)
>  			     probe_monitoring_device, NV_I2C_DEFAULT(0));
>  }
>  
> +static void
> +nouveau_ptherm_destroy(struct nouveau_device *ndev, int subdev)
> +{
> +// XXX: Undo probe_monitoring_device
> +}
> +
>  int
>  nouveau_therm_create(struct nouveau_device *ndev, int subdev)
>  {
> @@ -307,29 +318,53 @@ nouveau_therm_create(struct nouveau_device *ndev, int subdev)
>  	u8 *temp = NULL;
>  	int ret;
>  
> -	ret = nouveau_subdev_create(ndev, subdev, "THERM", "thermal", &ptherm);
> -	if (ret)
> -		return ret;
> +	if (bios->type == NVBIOS_BIT) {
> +		if (bit_table(ndev, 'P', &P))
> +			return 0;
The BIT check isn't necessary, bit_table() will fail if it's not a BIT BIOS.

>  
> -	if (ndev->chipset >= 0x40 && ndev->chipset < 0x84)
> -		ptherm->temp_get = nv40_therm_temp_get;
> -	else
> -	if (ndev->chipset <= 0xd9)
> -		ptherm->temp_get = nv84_therm_temp_get;
> -
> -	if (bit_table(ndev, 'P', &P) == 0) {
>  		if (P.version == 1)
>  			temp = ROMPTR(ndev, P.data[12]);
>  		else
>  		if (P.version == 2)
>  			temp = ROMPTR(ndev, P.data[16]);
> -		else
> +		else {
>  			NV_WARN(ndev, "unknown temp for BIT P %d\n", P.version);
> +		}
> +	} else {
> +		return 0;
> +	}
>  
> -		nouveau_therm_vbios_parse(ptherm, temp);
> +	if (!temp) {
> +		NV_DEBUG(ndev, "temp table pointer invalid\n");
> +		return 0;
>  	}
>  
> +	ret = nouveau_subdev_create(ndev, subdev, "THERM", "thermal", &ptherm);
> +	if (ret)
> +		return ret;
> +
> +	nouveau_therm_vbios_parse(ptherm, temp);
>  	nouveau_therm_probe_i2c(ndev);
>  
> +	ptherm->base.destroy = nouveau_ptherm_destroy;
> +	switch (ndev->card_type) {
> +	case NV_40:
> +	case NV_50:
> +	case NV_C0:
> +	case NV_D0:
> +	case NV_E0:
> +		if (ndev->chipset < 0x84) {
> +			ptherm->temp_get = nv40_therm_temp_get;
> +			break;
> +		} else
> +		if (ndev->chipset <= 0xd9) {
> +			ptherm->temp_get = nv84_therm_temp_get;
> +			break;
> +		}
> +	default:
> +		ptherm->temp_get = dummy_therm_temp_get;
> +		break;
> +	}
> +
>  	return nouveau_subdev_init(ndev, subdev, ret);
>  }
> -- 
> 1.7.10.2
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau