Messages in this thread Patch in this message | | | Date | Thu, 9 Jun 2005 15:33:20 +0300 | From | Pekka Enberg <> | Subject | Re: [RFC] SPI core |
| |
Hi,
Here are some coding style comments. Sorry for any duplicates, I read other comments only after I reviewed the patch.
Pekka
On 5/31/05, dmitry pervushin <dpervushin@ru.mvista.com> wrote: > diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-core.c linux/drivers/spi/spi-core.c > --- linux-2.6.12-rc4/drivers/spi/spi-core.c 1970-01-01 03:00:00.000000000 +0300 > +++ linux/drivers/spi/spi-core.c 2005-05-31 19:59:12.000000000 +0400 > +int spi_add_adapter(struct spi_adapter *adap) > +{ > + struct list_head *l; > + > + INIT_LIST_HEAD(&adap->clients); > + down(&adapter_lock); > + init_MUTEX(&adap->lock);
adapter_lock protects adapter_list, not adap. Therefore, please move adap->lock initialization outside the critical region.
> +struct spi_adapter *spi_get_adapter(int id) > +{ > + struct list_head *item; > + struct spi_adapter *adapter; > + > + down(&adapter_lock); > + list_for_each(item, &adapter_list) { > + adapter = list_entry(item, struct spi_adapter, adapters); > + if (id == adapter->nr && try_module_get(adapter->owner)) { > + up(&adapter_lock); > + return adapter;
One exit path please. You can use goto here.
> + > +EXPORT_SYMBOL(spi_add_adapter); > +EXPORT_SYMBOL(spi_del_adapter); > +EXPORT_SYMBOL(spi_get_adapter); > +EXPORT_SYMBOL(spi_put_adapter); > + > +EXPORT_SYMBOL(spi_add_driver); > +EXPORT_SYMBOL(spi_del_driver); > +EXPORT_SYMBOL(spi_get_driver); > +EXPORT_SYMBOL(spi_put_driver); > + > +EXPORT_SYMBOL(spi_attach_client); > +EXPORT_SYMBOL(spi_detach_client); > + > +EXPORT_SYMBOL(spi_transfer); > +EXPORT_SYMBOL(spi_write); > +EXPORT_SYMBOL(spi_read);
Preferred location for EXPORT_SYMBOLs is immediately after the function definition.
> diff -uNr -X dontdiff linux-2.6.12-rc4/drivers/spi/spi-dev.c linux/drivers/spi/spi-dev.c > --- linux-2.6.12-rc4/drivers/spi/spi-dev.c 1970-01-01 03:00:00.000000000 +0300 > +++ linux/drivers/spi/spi-dev.c 2005-05-31 19:59:18.000000000 +0400 > @@ -0,0 +1,514 @@ > +#include <linux/init.h> > +#include <asm/uaccess.h> > +#include <linux/spi/spi.h> > + > +#define SPI_TRANSFER_MAX 65535 > +#define SPI_ADAP_MAX 32
Unused define.
> +static struct file_operations spidev_fops = { > + owner:THIS_MODULE, > + llseek:no_llseek, > + read:spidev_read, > + write:spidev_write, > + open:spidev_open, > + release:spidev_release, > + ioctl:spidev_ioctl,
Use C99 struct initializers.
> +}; > + > +static struct spi_driver spidev_driver = { > + name:"spi", > + attach_adapter:spidev_attach_adapter, > + detach_adapter:spidev_detach_adapter, > + owner:THIS_MODULE,
Ditto.
> +}; > + > +static struct spi_client spidev_client_template = { > + driver:&spidev_driver
Same here.
> +static struct spi_dev *get_free_spi_dev(struct spi_adapter *adap) > +{ > + struct spi_dev *spi_dev; > + > + spi_dev = kmalloc(sizeof(*spi_dev), GFP_KERNEL); > + if (!spi_dev) > + return ERR_PTR(-ENOMEM); > + memset(spi_dev, 0x00, sizeof(*spi_dev));
See below.
> + > + spin_lock(&spi_dev_array_lock); > + if (spi_dev_array[adap->nr]) { > + spin_unlock(&spi_dev_array_lock);
One spin_unlock, please. Use gotos here.
> + dev_err(&adap->dev, > + "spi-dev already has a device assigned to this adapter\n"); > + goto error; > + } > + spi_dev->minor = adap->nr;
Instead of memset() followed by assignment, you can use C99 struct initializers like this:
*spi_dev = (struct spi_dev) { .minor = adap->nr; };
> + spi_dev_array[adap->nr] = spi_dev; > + spin_unlock(&spi_dev_array_lock); > + return spi_dev; > + error:
The placement of this label is strange.
> +static int spidev_ioctl(struct inode *inode, struct file *file, > + unsigned int cmd, unsigned long arg) > +{ > + struct spi_client *client = (struct spi_client *)file->private_data;
Please remove the redundant cast.
> + struct spi_rdwr_ioctl_data rdwr_arg; > + struct spi_msg *rdwr_pa; > + int res, i; > + unsigned long (*cpy_to_user) (void *to_user, const void *from, > + unsigned long len); > + unsigned long (*cpy_from_user) (void *to, const void *from_user, > + unsigned long len); > + void *(*alloc) (size_t, int); > + void (*free) (const void *);
No type declarations within function body, please.
> + > + DBG("spi-%d ioctl, cmd: 0x%x, arg: %lx.\n", > + MINOR(inode->i_rdev), cmd, arg); > + > + cpy_to_user = > + client->adapter->copy_to_user ? client->adapter-> > + copy_to_user : copy_to_user; > + cpy_from_user = > + client->adapter->copy_from_user ? client->adapter-> > + copy_from_user : copy_from_user; > + alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc; > + free = client->adapter->free ? client->adapter->free : kfree; > + > + switch (cmd) { > + case SPI_RDWR: > + if (copy_from_user(&rdwr_arg, > + (struct spi_rdwr_ioctl_data *)arg, > + sizeof(rdwr_arg))) > + return -EFAULT; > + > + rdwr_pa = (struct spi_msg *) > + kmalloc(rdwr_arg.nmsgs * sizeof(struct spi_msg), > + GFP_KERNEL);
Redundant cast.
> +static int spidev_attach_adapter(struct spi_adapter *adap) > +{ > + struct spi_dev *spi_dev; > + int retval; > + > + spi_dev = get_free_spi_dev(adap); > + if (IS_ERR(spi_dev)) > + return PTR_ERR(spi_dev); > + > +#if defined( CONFIG_DEVFS_FS ) > + devfs_mk_cdev(MKDEV(SPI_MAJOR, spi_dev->minor), > + S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", spi_dev->minor); > +#endif > + dev_dbg(&adap->dev, "Registered as minor %d\n", spi_dev->minor); > + > + /* register this spi device with the driver core */ > + spi_dev->adap = adap; > + if (adap->dev.parent == &platform_bus) > + spi_dev->class_dev.dev = &adap->dev; > + else > + spi_dev->class_dev.dev = adap->dev.parent; > + spi_dev->class_dev.class = &spi_dev_class; > + snprintf(spi_dev->class_dev.class_id, BUS_ID_SIZE, "spi-%d", > + spi_dev->minor); > + retval = class_device_register(&spi_dev->class_dev); > + if (retval) > + goto error; > + class_device_create_file(&spi_dev->class_dev, &class_device_attr_dev); > + class_device_create_file(&spi_dev->class_dev, &class_device_attr_name); > + return 0; > + error:
The placement of this label is strange.
> +static ssize_t spidev_read(struct file *file, char *buf, size_t count, > + loff_t * offset) > +{ > + char *tmp; > + int ret; > +#ifdef SPIDEV_DEBUG > + struct inode *inode = file->f_dentry->d_inode; > +#endif > + > + struct spi_client *client = (struct spi_client *)file->private_data; > + unsigned long (*cpy_to_user) (void *to_user, const void *from, > + unsigned long len); > + void *(*alloc) (size_t, int); > + void (*free) (const void *);
Please remove the duplicate function pointer type declarations.
> + if (count > SPI_TRANSFER_MAX) > + count = SPI_TRANSFER_MAX; > + > + cpy_to_user = > + client->adapter->copy_to_user ? client->adapter-> > + copy_to_user : copy_to_user; > + alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc; > + free = client->adapter->free ? client->adapter->free : kfree;
More duplicate code.
> + > + /* copy user space data to kernel space. */ > + tmp = alloc(count, GFP_KERNEL); > + if (tmp == NULL) > + return -ENOMEM; > + > + DBG("spi-%d reading %d bytes.\n", MINOR(inode->i_rdev), count); > + > + ret = spi_read(client, 0, tmp, count); > + if (ret >= 0) > + ret = cpy_to_user(buf, tmp, count) ? -EFAULT : ret; > + free(tmp); > + return ret; > +} > + > +static ssize_t spidev_write(struct file *file, const char *buf, size_t count, > + loff_t * offset) > +{ > + int ret; > + char *tmp; > + struct spi_client *client = (struct spi_client *)file->private_data; > +#ifdef SPIDEV_DEBUG > + struct inode *inode = file->f_dentry->d_inode; > +#endif > + unsigned long (*cpy_from_user) (void *to, const void *from_user, > + unsigned long len); > + void *(*alloc) (size_t, int); > + void (*free) (const void *);
Aiie! Duplicates.
> + > + if (count > SPI_TRANSFER_MAX) > + count = SPI_TRANSFER_MAX; > + > + cpy_from_user = > + client->adapter->copy_from_user ? client->adapter-> > + copy_from_user : copy_from_user; > + alloc = client->adapter->alloc ? client->adapter->alloc : kmalloc; > + free = client->adapter->free ? client->adapter->free : kfree;
More of the same.
> +int spidev_open(struct inode *inode, struct file *file) > +{ > + unsigned int minor = iminor(inode); > + struct spi_client *client; > + struct spi_adapter *adap; > + struct spi_dev *spi_dev; > + > + spi_dev = spi_dev_get_by_minor(minor); > + if (!spi_dev) > + return -ENODEV; > + > + adap = spi_get_adapter(spi_dev->adap->nr); > + if (!adap) > + return -ENODEV; > + > + client = kmalloc(sizeof(*client), GFP_KERNEL); > + if (!client) { > + spi_put_adapter(adap); > + return -ENOMEM; > + } > + memcpy(client, &spidev_client_template, sizeof(*client)); > + > + /* registered with adapter, passed as client to user */ > + client->adapter = adap;
Please use C99 struct initializer here.
> +static int __init spi_dev_init(void) > +{ > + int res; > + > + printk(KERN_INFO "spi /dev entries driver\n"); > + > + res = register_chrdev(SPI_MAJOR, "spi", &spidev_fops); > + if (res) > + goto out; > + > + res = class_register(&spi_dev_class); > + if (res) > + goto out_unreg_chrdev; > + > + res = spi_add_driver(&spidev_driver); > + if (res) > + goto out_unreg_class; > + > +#ifdef CONFIG_DEVFS_FS > + devfs_mk_dir("spi"); > +#endif > + return 0; > + > + out_unreg_class:
The label indentation is strange.
> + class_unregister(&spi_dev_class); > + out_unreg_chrdev:
Ditto.
> + unregister_chrdev(SPI_MAJOR, "spi"); > + out:
Here as well.
> diff -uNr -X dontdiff linux-2.6.12-rc4/include/linux/spi/spi.h linux/include/linux/spi/spi.h > --- linux-2.6.12-rc4/include/linux/spi/spi.h 1970-01-01 03:00:00.000000000 +0300 > +++ linux/include/linux/spi/spi.h 2005-05-31 19:59:26.000000000 +0400 > @@ -0,0 +1,265 @@ > +/* > + * linux/include/linux/spi/spi.h > + * > + * Copyright (C) 2001 Russell King, All Rights Reserved. > + * Copyright (C) 2002 Compaq Computer Corporation, All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License. > + * > + * Derived from l3.h by Jamey Hicks > + */ > +#ifndef SPI_H > +#define SPI_H > + > +struct spi_msg { > + unsigned char addr; /* slave address */ > + unsigned char flags; > +#define SPI_M_RD 0x01 > +#define SPI_M_NOADDR 0x02 Please use enums instead.
> + unsigned short len; /* msg length */ > + unsigned char *buf; /* pointer to msg data */ > +}; > + > +/* TODO: should be in a separate header file? */ > + > +/* Commands for the ioctl call */ > +#define SPI_RDWR 0x0801 /* Read/write transfer in a single transaction */ > +#define SPI_CLK_RATE 0x0802 /* Sets SPI clock divisor (int type) */ > +#define SPI_SDMA 0x0803 /* Turns on/off SDMA usage (int type). */
Same here.
> + /* Pass non-zero value to turn SDMA on */ > +struct spi_rdwr_ioctl_data { > + struct spi_msg *msgs; /* pointers to spi_msgs */ > + int nmsgs; /* number of spi_msgs */ > +}; > +/*------------^^^^^^^^^^^^^^^--------------*/
Please drop the above ASCII art.
> + > +#ifdef __KERNEL__ > + > +#include <linux/types.h> > +#include <linux/list.h> > + > +#define SPI_ADAP_MAX 32
Unused constant. - 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/
| |