Re: [RFC] SPI core -- revisited

Greg KH
2005-06-24T00:38:37Z

On Thu, Jun 23, 2005 at 04:18:54PM +0400, dmitry pervushin wrote:
> +int driver_for_each_dev(struct device_driver *drv, void *data,
> + int (*callback) (struct device * dev, void *data))
> +{
> + _find_t local_data;
> +
> + local_data.drv = drv;
> + local_data.data = data;
> + local_data.callback = callback;
> + return bus_for_each_dev(drv->bus, NULL, &local_data, dfed_callback);
> +}
> +
> +EXPORT_SYMBOL(driver_for_each_dev);

Use the built-in kernel functions, don't create a new one and go around
the _GPL export.

> +#ifdef CONFIG_SPI_DEBUG
> +#define DEBUG
> +#endif /* CONFIG_SPI_DEBUG */

Have the Makefile define this for you, it's not needed in the .c file

> +#include <linux/spi/spi.h>

Why a separate subdirectory? It should just go in include/linux/ like
everything else.

> + if (NULL == dev || NULL == driver) {

dev == NULL instead of the other way around (yes, I know why you do
this, but the rest of the kernel is the other way, and gcc will warn you
if you forget a '=').

> +SPI_IDS_TABLE_BEGIN
> + SPI_ID_ANY
> +SPI_IDS_TABLE_END

Ick, that's a horrible way to define things. Please make it look like
real .c code.

> +static struct spi_driver spidev_driver = {
> + .owner = THIS_MODULE,
> + .driver = {
> + .name = "generic spi",

No spaces in driver names please.

> + drvdata = (spidev_driver_data_t *) kmalloc(sizeof(spidev_driver_data_t),
> + GFP_KERNEL);

Cast is unnecessary.

Also, get rid of all typedefs, they are not acceptable.

> +#ifdef CONFIG_DEVFS_FS
> + devfs_mk_cdev(MKDEV(SPI_MAJOR, drvdata->minor),
> + S_IFCHR | S_IRUSR | S_IWUSR, "spi/%d", drvdata->minor);
> +#endif

Don't put #ifdef in code where it is not needed at all.

> + if (NULL == dev) {
> + printk(KERN_ERR "%s: removing the NULL device\n", __FUNCTION__);
> + }

Extra { } are not needed.

What is this checking for anyway?

Use dev_err() for these things.

> +static ssize_t spidev_read(struct file *file, char *buf, size_t count,
> + loff_t * offset)
> +{
> + char *tmp;
> + int ret = 0;
> +#ifdef DEBUG
> + struct inode *inode = file->f_dentry->d_inode;
> +#endif

#ifdef not nice to have here, it's not needed.

> +#define SPI_STUFF_FOUND -ECANCELED

Just use the raw error number, don't redefine it.

> +struct spi_adapter {
> + /*
> + * This name is used to uniquely identify the adapter.
> + * It should be the same as the module name.
> + */
> + char name[32];

Why not use the device.bus_id instead?

I too would like to see how this code is used, example drivers please.

thanks,

greg k-h