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
-
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]
