Re: [PATCH: 2.6.37 1/1] TAOS 258x: Device Driver

[Randy Dunlap]

On Thu, 24 Feb 2011 20:15:18 -0600 Jon Brenner wrote:

> From: J. August Brenner <jbrenner@xxxxxxxxxxx>
> 
> This driver supports the TAOS tsl258x family of ALS sensors.
> This driver provides ALS data (lux), and device configuration via ioctl,
> input_event, sysfs/iio methods.  These methods are provided to support
> varied access requirements and integrated into a single driver.
> Thus, adverting the need for multiple releases of similar code.
> More significantly, this driver provides the capability to be fed 'glass
> coefficients' to accommodate varying system designs (bezels, faceplates,
> etc.).  
> 
> Signed-off-by: Jon August Brenner <jbrenner@xxxxxxxxxxx>
> 
> ---
> diff -Naurp drivers-orig/Kconfig drivers/Kconfig
> --- drivers-orig/Kconfig	2011-01-04 18:50:19.000000000 -0600
> +++ drivers/Kconfig	2011-02-24 16:03:53.536188000 -0600

Wrong source tree level:  diffs should begin with the top-level linux directory.

> @@ -13,6 +13,13 @@ config SENSORS_TSL2563
>  	 This driver can also be built as a module.  If so, the module
>  	 will be called tsl2563.
>  
> +config TAOS_258x
> +	tristate "TAOS TSL258x device driver"
> +	default m

Don't enable unnecessary drivers.

> +	help
> +	  Provides support for the TAOS tsl258x family of devices.
> +	  Access ALS data via iio, sysfs, input_event, and ioctl methods.
> +
>  config SENSORS_ISL29018
>          tristate "ISL 29018 light and proximity sensor"
>          depends on I2C
> diff -Naurp drivers-orig/Makefile drivers/Makefile
> --- drivers-orig/Makefile	2011-01-04 18:50:19.000000000 -0600
> +++ drivers/Makefile	2011-02-24 16:04:31.701642000 -0600

This isn't drivers/Makefile, is it?
This is drivers/hwmon/Makefile or drivers/als/Makefile ?

> @@ -3,4 +3,5 @@
>  #
>  
>  obj-$(CONFIG_SENSORS_TSL2563)	+= tsl2563.o
> +obj-$(CONFIG_TAOS_258x)	+= tsl258x.o
>  obj-$(CONFIG_SENSORS_ISL29018)	+= isl29018.o
> diff -Naurp drivers-orig/tsl258x.c drivers/tsl258x.c
> --- drivers-orig/tsl258x.c	1969-12-31 18:00:00.000000000 -0600
> +++ drivers/tsl258x.c	2011-02-24 14:07:43.416960000 -0600

What is the real path here?

and please include a full diffstat so that we can see what files are being
added/changed.  Hint:  read Documentation/SubmittingPatches.

> @@ -0,0 +1,2055 @@

[snippage]

I didn't read all of Jonathan's comments, but they are not
addressed in this patch.

> +/*........................................................................*/
> +/* Various /misc. emun*/

                     ^^^^ enum */

> +enum {
> +	TAOS_CHIP_UNKNOWN = 0, TAOS_CHIP_WORKING = 1, TAOS_CHIP_SLEEP = 2
> +} TAOS_CHIP_WORKING_STATUS;

> +/* ------ Mutex ------*/
> +DEFINE_MUTEX(als_mutex);

Drop obvious comments. (multiple)

> +struct delayed_work *luxUd;

Strange variable name (no camelcase, please).

> +/*........................................................................*/
> +/*
> + Initial values for device - this values can/will be changed by driver.
> + and applications as needed.
> + These values are dynamic.
> + */

above & below:  Please use the usual kernel multi-line comment style:

/*
 * comment text line 1
 * comment text line 2
 */

> +/*
> + This structure is intentionally large to accommodate updates via
> + ioctl and proc input methods.
> + */


> +struct taos_lux *taos_device_luxP;

ugh

> +/*===========================================================================*/
> +/**@defgroup Initialization Driver Initialization
> + * The sections applies to functions for driver initialization, instantiation,
> + * _exit, \n
> + * and user-land application invoking (ie. open).\n
> + * Also included in this section is the initial interrupt handler (handler
> + * bottom halves are in the respective
> + * sections).
> + * @{
> + */

What kind of doc formatting is that?
Please use kernel-doc for kernel tree doc comments, or just use C-style
comments or plain text, but not that stuff.
For kernel-doc info, see Documentation/kernel-doc-nano-HOWTO.txt.


> +exit_1: device_destroy(taos_class, MKDEV(MAJOR(taos_dev_number), 0));
> +exit_2: cdev_del(&chip->cdev);
> +	kfree(chip);
> +exit_3: class_destroy(taos_class);
> +	unregister_chrdev_region(taos_dev_number, MAX_NUM_DEVICES);
> +

Put labels on a line by themselves (above), like the one below.

> +exit_4:
> +
> +	printk(KERN_INFO "\n\nallocating iio device\n");
> +/**
> + * Driver exit
> + */
> +static void __exit taos_exit(void)
> +{
> +	printk(KERN_INFO "TAOS driver __exit\n");

Drop that printk.

> +}
> +
> +/**
> + * Client probe function - When a valid device is found, the driver's device
> + * data structure is updated, and initialization completes successfully.
> + */

"/**" in the kernel source tree means "the beginning of a kernel-doc comment,"
so please use it for that and only that. (MANY places)


> +static ssize_t taos_interrupt_store(struct device *dev,
> +    struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	unsigned long value;

Please leave an empty line between function local data and code. (multiple)

> +	if (strict_strtoul(buf, 0, &value))
> +		return -EINVAL;


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/