lkml.org 
[lkml]   [2005]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-08-08 19:29    [W:1.380 / U:0.748 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site