Re: [PATCH] spi

[Andrew Morton <akpm@osdl.org>]

dmitry pervushin <dpervushin@gmail.com> wrote:
>

A few coding style nits.

> +#include "spi_locals.h"
> +
> +static LIST_HEAD( spi_busses );

Please don't put spaces after '(' and before ')'.

> +		if (0 == strncmp(*id, SPI_ID_ANY, strlen(SPI_ID_ANY))) {

And this trick isn't really needed.  If you do

	if (whatever = constant)

then the compiler will generate a warning.  Please do these comparisons in
the conventional way, with the constant on the right hand side.

> +	if( bus ) {
> +		init_MUTEX( &bus->lock );
> +
> +		bus->platform_device.name = NULL;
> +		bus->the_bus.name = NULL;
> +
> +		strncpy( busname, name ? name : "SPI", sizeof( busname ) );

Lots more extraneous spaces after '(' and before ')'.

> +		if( bus->the_bus.name ) {
> +			strcpy( bus->the_bus.name, fullname );
> +		}

No braces here.

> +		err = bus_register( &bus->the_bus );
> +		if( err ) {
> +			goto out;
> +		}

And here.

> +		list_add_tail( &bus->bus_list, &spi_busses );
> +		bus->platform_device.name = kmalloc( strlen( busname )+1, GFP_KERNEL );
> +		if( bus->platform_device.name ) {
> +			strcpy( bus->platform_device.name, busname );
> +		}

and here...

> +void spi_bus_unregister( struct spi_bus* bus )
> +{
> +	if( bus ) {

We do put a space after `if', so this line should be

	if (bus) {

> +struct spi_bus* spi_bus_find( char* id )

The asterisk goes with the variable, not with the type.  So the above should be

	struct spi_bus *spi_bus_find(char *id)

> +int spi_device_add( struct spi_bus* bus, struct spi_device *dev, char* name)

Here too.

> +int spi_do_probe( struct device* dev, void* device_driver )

You seem to have an awful lot of non-static functions.  Please check
whether they all really need to have global scope.

> +	if (NULL == dev) {

	if (dev == NULL) {

> +static int spidev_do_open(struct device *the_dev, void *context)
> +{
> +	struct spidev_openclose *o = (struct spidev_openclose *) context;

Don't typecast void* when assigning to and from pointers.  It adds clutter
and defeats typechecking.

> +	struct spi_device *dev = SPI_DEV(the_dev);
> +	struct spidev_driver_data *drvdata;
> +
> +	drvdata = (struct spidev_driver_data *) dev_get_drvdata(the_dev);

Ditto.

> +
> +      out_unreg:

Labels go in column zero.

> +	void *(*alloc) (size_t, int);
> +	void (*free) (const void *);
> +	unsigned long (*copy_from_user) (void *to, const void *from_user,
> +					 unsigned long len);
> +	unsigned long (*copy_to_user) (void *to_user, const void *from,
> +				       unsigned long len);

The above names are risky.  Some platform may implement copy_to_user() as a
macro.