Hi Steven,
I realise most of these are not from your code, so please do not
take them as any form of criticism. I'm just listing them here in
case somebody does want to actually go through and clean it up. It
will not stand a chance for merging into mainline until at least
these are (IMHO), and probably more.
Thanks for your work in porting though - it will probably be useful
to others!
Cheers,
Bernard.
> +#define EXTERN extern
Why not just use extern?
> +MODULE_LICENSE("GPL");
Typically goes at the end of the file.
> +struct device_params_t device_config;
device_params_t is a very generic name for a struct that's quite
device-specific. How about just "struct resizer_params" ?
> +inline void rsz_free_pages(unsigned long addr, unsigned long bufsize)
Should be static.
> +int malloc_buff(struct rsz_reqbufs_t *reqbuff,
> + struct channel_config_t *rsz_conf_chan)
Static too. Ditto for basically all of the functions and
declarations in this file.
> +#ifdef CONFIG_PREEMPT_RT
> + wait_for_completion_interruptible
> + (&(rsz_conf_chan->channel_sem));
> +#else
> + down_interruptible(&(rsz_conf_chan->channel_sem));
> +#endif
I can't see any reason why this can't always be a completion. They
were in 2.6.23 too. (This is one of the git checkpatch warnings too).
> +static int rsz_ioctl(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
[...]
> + ret = down_trylock(&(rsz_conf_chan->chanprotection_sem));
> +
> + /* Before decoding check for correctness of cmd */
> + if (_IOC_TYPE(cmd) != RSZ_IOC_BASE) {
> + dev_err(rsz_device, "Bad command Value \n");
> + return -1;
> + }
Locking in this function seems completely broken.
> +
> + /*veryfying access permission of commands */
> + if (_IOC_DIR(cmd) & _IOC_READ)
> + ret = !access_ok(VERIFY_WRITE, (void *)arg, _IOC_SIZE(cmd));
> + else if (_IOC_DIR(cmd) & _IOC_WRITE)
> + ret = !access_ok(VERIFY_READ, (void *)arg, _IOC_SIZE(cmd));
I also feel like arg needs a copy_from_user/copy_to_user for the
various ioctls... maybe I've completely off the mark here though.
> +static struct file_operations rsz_fops = {
> + .owner = THIS_MODULE, .open = rsz_open, .release =
> + rsz_release, .mmap = rsz_mmap, .ioctl = rsz_ioctl,
> +};
Crazy formatting!
> +static struct platform_device resizer_device = {
> + .name = "davinci_resizer", .id = 2, .dev = {
> + .release =
> + resizer_platform_release,}
> +};
More formatting.
> + if (result) {
> + printk("NOtICE \nDaVinciresizer:Error %d adding Davinciresizer\
Should have a KERN_NOTICE, not NOtICE.
> + /* Register the drive as a platform device */
> + if (platform_device_register(&resizer_device) != 0) {
> + driver_unregister(&resizer_driver);
> + unregister_chrdev_region(dev, 1);
> + unregister_chrdev(MAJOR(dev), DRIVER_NAME);
> + cdev_del(&c_dev);
> + return -EINVAL;
> + }
> +
[...]
> + if (!rsz_class) {
> +
> + platform_device_unregister(&resizer_device);
> + cdev_del(&c_dev);
> + unregister_chrdev(MAJOR(dev), DRIVER_NAME);
> +
> + return -EIO;
> + }
Missing cdev_del from this code path - maybe it would be better to
have a common failure path with gotos.
> diff --git a/include/asm-arm/arch-davinci/davinci_resizer.h
> b/include/asm-arm/arch-davinci/davinci_resizer.h
> +#define RESIZER_IOBASE_VADDR IO_ADDRESS(0x01C70C00)
Ideally, this would be split into a platform device.
> +#define ZERO 0
> +#define FIRSTENTRY 0
> +#define SECONDENTRY 1
These constants are gratiutious.
> +#define SUCESS 0
Misspelt, and 0 is a common return code for success in the kernel
anyway.
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
