Messages in this thread Patch in this message | | | Date | Mon, 8 Aug 2005 07:55:44 -0700 | From | Greg KH <> | Subject | Re: [PATCH] spi |
| |
On Mon, Aug 08, 2005 at 01:12:18PM +0400, dmitry pervushin wrote: > +config SPI > + default Y > + tristate "SPI support" > + default false > + help > + Say Y if you need to enable SPI support on your kernel
What about the "the module will be called..." text for this?
> +obj-$(CONFIG_SPI) += spi-core.o helpers.o
Hm, this will not build :(
> +static LIST_HEAD( spi_busses );
No spaces after ( or before ) please. You do this all over the place in the code, please fix it up.
> + if (NULL == dev || NULL == driver) {
Put the variable on the left side, gcc will complain if you incorrectly put a "=" instead of a "==" here, which is all that you are defending against with this style.
> + printk(KERN_ERR > + "%s: error - both dev and driver should not be NULL !\n", > + __FUNCTION__); > + found = 0; > + goto spi_match_done; > + } > + > + if (NULL == spidrv->supported_ids) { > + printk > + ("%s: driver has no ids of devices to support, assuming ALL\n", > + __FUNCTION__);
No KERN_ level here. Also, please use dev_dbg() and friends if you possibly can, it's much better for log messages.
> +int spi_bus_register( struct spi_bus* bus, char* name ) > +{ > + int err = -EINVAL; > + static int count = 0; > + char busname[ BUS_ID_SIZE ]; > + char fullname[ BUS_ID_SIZE ]; > + > + ENTER();
Tracing? ick, not really needed...
> + if( bus ) { > + init_MUTEX( &bus->lock ); > + > + bus->platform_device.name = NULL; > + bus->the_bus.name = NULL; > + > + strncpy( busname, name ? name : "SPI", sizeof( busname ) ); > + bus->platform_device.id = count++ % 100; > + sprintf( fullname, "%s_%02d", busname, bus->platform_device.id); > + bus->the_bus.name = kmalloc( strlen( fullname )+1, GFP_KERNEL ); > + if( bus->the_bus.name ) { > + strcpy( bus->the_bus.name, fullname ); > + } > +
{ } are not needed for 1 line if statements.
> +/** > + * spi_add_adapter - register a new SPI bus adapter > + * @spidev: spi_device structure for the registering adapter > + * > + * Make the adapter available for use by clients using name adap->name. > + * The adap->adapters list is initialised by this function. > + * > + * Returns error code ( 0 on success ) ; > + */ > +struct spi_bus* spi_bus_find( char* id )
Wrong comment.
> +{ > + struct bus_type* the_bus = find_bus( id ); > + > + return the_bus ? container_of( the_bus, struct spi_bus, the_bus ) : NULL; > +} > + > +EXPORT_SYMBOL( spi_bus_find );
EXPORT_SYMBOL_GPL ?
> +int spi_device_add( struct spi_bus* bus, struct spi_device *dev, char* name)
No space between that and the next function?
No comment for a public function ike this?
> +/** > + * spi_del_adapter - unregister a SPI bus adapter > + * @dev: spi_device structure to unregister > + * > + * Remove an adapter from the list of available SPI Bus adapters. > + * > + * Returns error code (0 on success); > + */ > + > +void spi_device_del(struct spi_device *dev)
Again, comments out of sync with the code.
> +void spi_driver_del( struct spi_driver* drv ) > +{ > + driver_unregister( &drv->driver ); > +} > +/** > + * spi_transfer - transfer information on an SPI bus
Please, just 1 line of whitespace between functions, not none.
> +/** > + * spi_write - send data to a device on an SPI bus > + * @client: registered client structure > + * @addr: SPI bus address > + * @buf: buffer for bytes to send > + * @len: number of bytes to send > + * > + * Send len bytes pointed to by buf to device address addr on the SPI bus > + * described by client. > + * > + * Returns the number of bytes transferred, or negative error code. > + */ > +int spi_write(struct spi_device *dev, int addr, const char *buf, int len)
Comment doesn't match function. You will catch all of these errors if you add the files to the kernel api docbook document.
> =================================================================== > --- linux-2.6.10.orig/drivers/spi/spi-dev.c 1970-01-01 00:00:00.000000000 +0000 > +++ linux-2.6.10/drivers/spi/spi-dev.c 2005-07-15 06:57:39.000000000 +0000 > @@ -0,0 +1,303 @@ > +/*#ifdef CONFIG_DEVFS_FS What's with the #ifdef here?
> + > +/* $Id: common_spi_core-2.patch,v 1.1.2.6 2005/07/15 07:24:40 tpoynor Exp $ */
No CVS ids in kernel code please.
> +/* struct file_operations changed too often in the 2.1 series for nice code */
I don't think this comment is necessary anymore :)
> +static ssize_t spidev_read(struct file *file, char *buf, size_t count, > + loff_t * offset); > +static ssize_t spidev_write(struct file *file, const char *buf, size_t count, > + loff_t * offset);
You didn't run this through sparse :(
That's enough to start with...
thanks,
greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |