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 <[email protected]> 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 [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]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
