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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |