gl518sm chip driver for 2.6 kernel

[Greg KH]

Comments on the code itself:

> +#ifdef __SMP__
> +#include <linux/smp_lock.h>
> +#endif

No, just include it, don't #ifdef it, that's not needed at all.

> +/* Defining this will enable debug messages for the voltage iteration
> +   code used with rev 0 ICs */
> +#undef DEBUG_VIN
> +#ifdef DEBUG
> +#define DEBUG_VIN
> +#endif

As you always tie DEBUG_VIN to DEBUG, why even have it at all?  Just get
rid of it, and everywhere it's defined.

> +static int gl518_update_thread(void *c)
> +{
> +	struct i2c_client *client = c;
> +	struct gl518_data *data = i2c_get_clientdata(client);
> +
> +#ifdef __SMP__
> +	lock_kernel();
> +#endif

No.  Again, no #ifdefs are needed.  And what's with the lock_kernel()
call?  What are you trying to protect here?

It looks like you only kick off this kernel thread if a user opens a
specific sysfs file.  Why?  Why not just work like all other sensor
drivers, and update the chip data when asked for it from the user?

I really do not see the need for this thread here.

> +	exit_mm(current);
> +	current->session = 1;
> +	sigfillset(&current->blocked);
> +	current->fs->umask = 0;
> +	strcpy(current->comm, "gl518sm");
> +
> +	init_waitqueue_head(&(data->wq));
> +	data->thread = current;
> +
> +#ifdef __SMP__
> +	unlock_kernel();
> +#endif

I think there's easier, and more proper ways to create a kernel thread
in 2.6 now.  If you really want to do this (and you can convince me of
it), please look into this (hint, search for daemonize()).


> +	for (j = 0; j < 10 && loop_more; j++) {
> +		for (i = 0; i < 3; i++)
> +			gl518_write_value(client, VIN_REG(i),
> +					  max[i] << 8 | min[i]);
> +
> +		if ((data->thread) &&
> +		    ((data->quit_thread) || signal_pending(current)))
> +			goto finish;
> +
> +		/* we wait now 1.5 seconds before comparing */
> +		current->state = TASK_INTERRUPTIBLE;
> +		schedule_timeout(HZ + HZ / 2);
> +
> +		alarm = gl518_read_value(client, GL518_REG_INT);

So you sleep for 1.5 seconds 10 times?  Why?  That seems very slow...


thanks,

greg k-h