Re: [lm-sensors] [PATCH] hwmon: (thmc50) Convert to a new-style i2c driver

[Hans de Goede <j.w.r.degoede-fbo2DhPpy/Q@public.gmane.org>]

Jean Delvare wrote:
> The new-style thmc50 driver implements the optional detect()
> callback to cover the use cases of the legacy driver.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Krzysztof Helt <krzysztof.h1-5tc4TXWwyLM@public.gmane.org>

> @@ -307,21 +304,22 @@ static int thmc50_detect(struct i2c_adap
>  	}
>  	if (err == -ENODEV) {
>  		pr_debug("thmc50: Detection of THMC50/ADM1022 failed\n");
> -		goto exit_free;
> +		return err;
>  	}
> -	data->type = kind;
>  
>  	if (kind == adm1022) {
>  		int id = i2c_adapter_id(client->adapter);
>  		int i;
>  
>  		type_name = "adm1022";
> -		data->has_temp3 = (config >> 7) & 1;	/* config MSB */
>  		for (i = 0; i + 1 < adm1022_temp3_num; i += 2)
>  			if (adm1022_temp3[i] == id &&
> -			    adm1022_temp3[i + 1] == address) {
> +			    adm1022_temp3[i + 1] == client->addr) {
>  				/* enable 2nd remote temp */
> -				data->has_temp3 = 1;
> +				config |= (1 << 7);
> +				i2c_smbus_write_byte_data(client,
> +							  THMC50_REG_CONF,
> +							  config);
>  				break;
>  			}
>  	} else {

Hmm, This does not seem right, you are _writing_ to the device in a the detect 
function changing its configuration. I understand you can no longer set 
data->has_temp3 as there is no data yet in the detect, instead the entire loop 
which checks if the device has temp3 forced through a module option should be 
moved to the probe function. Writing to a config register in the detect 
function feels wrong to me.

Regards,

Hans

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c