Hi Roger,
Please answer to the list rather than to me only.
> I have a patch for the vt8231 driver now (attached). It seems OK but I get
> a compilation warning:
>
> drivers/hwmon/vt8231.c:63: warning: `addr_data' defined but not used
>
> This is generated because I am using the macro "I2C_CLIENT_INSMOD_1(vt8231)"
> to register the module, but then never use the addr_data structure that is
> created by it.
>
> I cannot see a reason to use this structure, so I suppose I should be using
> a different macro to register the driver... Any thoughts ?
Don't use I2C_CLIENT_INSMOD_1. The VT8231 is not an I2C chip. It is
attached to the i2c subsystem for historical reasons, but we are trying
to separate such chips from the i2c subsystem now.
A few additional comments about your code:
> +config SENSORS_VT8231
> + tristate "VT8231"
> + depends on HWMON && I2C && PCI
You should make it depend on EXPERIMENTAL too.
> +/* Supports VIA VT8231 South Bridge embedded sensors
> +**
> +** Change log:
> +**
> +** 2005-11-01: Roger Lucas (roger at planbit.co.uk)
> +** Updated to work with the 2.6.14 kernel that I have. This is the first
> +** lm-sensors port that I have done, so there may be nasty mistakes in it.
> +** That having been said, insmod and rmmod seem to work cleanly on my
> +** system and the values reported are reasonable.
> +** I merged the Aaron Marsh vt8231 port with the most recent via686a driver
> +** in the 2.6.14 kernel source to create this code.
> +** VRM doesn't work on the Via Epia 5000 that I have as the kernel call
> +** vid_which_vrm() returns -1. Apparently the Via CPU is unrecognised....
> +*/
Changelog doesn't belong to the code. You can add whatever comment you
want at the top of the patch, and it'll be used in the git commit.
The vid_which_vrm issue is interesting. Rudolf, can you please look
into it?
> +static int force_addr = 0;
Do not explicitely intialize static globals to 0, the compiler does it
for you.
> +MODULE_PARM(force_addr, "i");
Please use module_param instead. MODULE_PARM is deprecated.
> +static unsigned short normal_i2c[] = { I2C_CLIENT_END };
You shouldn't need this one.
> +static DEVICE_ATTR(alarms, S_IRUGO | S_IWUSR, show_alarms, NULL);
S_IWUSR is not correct, that file is read-only.
> +static DEVICE_ATTR(in0_ref, S_IRUGO, show_vid, NULL);
This attribute has been renamed to cpu0_vid.
> + struct vt8231_data *data = vt8231_update_device(dev);
Indentation issue, you use spaces instead of a tab. Same in other
places, please fix them all.
> + return sprintf(buf,"%ld\n", (long)data->vrm);
Please use a space after the comma. Same in other places, please fix
them all.
> + printk("Setting VRM to \"%s\" = %d\n", buf, val );
No space before closing parenthesis.
> + if (!request_region(isa_address, VT8231_EXTENT, "vt8231-sensors")) {
Please use the driver name to request the region.
> + if (!(data = kmalloc(sizeof(struct vt8231_data), GFP_KERNEL))) {
> + err = -ENOMEM;
> + goto exit_release;
> + }
> +
> + memset(data, 0, sizeof(struct vt8231_data));
Please use kzalloc instead of kmalloc + memset.
> + snprintf(new_client->name, I2C_NAME_SIZE, type_name);
strlcpy will do the same, more efficiently.
> + /* TODO: vid and vrm */
> + device_create_file(&new_client->dev, &dev_attr_in0_ref);
> + device_create_file(&new_client->dev, &dev_attr_vrm);
Bad indentation. Why this "TODO" comment?
This was only a quick and partial review, I've been reviewing too much
code today already, I'll go mad if I do more.
One last general comment: please cut long lines so that the code fits
in 80 columns.
Thanks,
--
Jean Delvare
