Re: O2 VICE support

[ilya@theIlya.com]

[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]

> Urgg.  Usually kernel headers aren't supposed to be used in userspace.
> If you want to use a copy anyway it should be done with much less burden
> on the kernel code.
Well, most of manipulation of this device will be done bu user-space
code. Kernel's only job is synchronising access and managing memory.
Thus we would like to keep all definitions in one place.

> > +    if (!filp->private_data) {
> > +	filp->private_data = vice_device;
> > +    }
> 
> filp->private_data can't be set.
???
I can live without using it of course, since it isn't currently possible to have
more then one instance of VICE in same machine, but theoretical possibility does
exist. Where should I pass information about which specific device current
call is?

> 
> > +ssize_t vice_read(struct file * filp, char *buf, size_t count, loff_t * f_pos)
> > +{
> > +    printk(KERN_WARNING
> > +	   "Processing bit streams through reading/writing is not supported yet\n");
> > +    return -ENOSYS;
> > +}
> > +
> > +ssize_t vice_write(struct file * filp, const char *buf, size_t count,
> > +	   loff_t * f_pos)
> > +{
> > +    printk(KERN_WARNING
> > +	   "Processing bit streams through reading/writing is not supported (yet)\n");
> > +    return -ENOSYS;
> > +}
> 
> What about just not implementing the methods instead?
This is more like reminder to myself that I should do that one day :)

> > +static void vice_vma_open(struct vm_area_struct *vma)
> > +{ MOD_INC_USE_COUNT; }
> > +
> > +static void vice_vma_close(struct vm_area_struct *vma)
> > +{ MOD_DEC_USE_COUNT; }
> 
> This is silly.  You get a reference for vma->vm_file as long as you
> have any mmaps.  That's enough for the refcounting.
I was wondering about that too, but this is the way i was in book :)

> 
> > +static struct vm_operations_struct vice_vm_ops = {
> > +    open:  vice_vma_open,
> > +    close: vice_vma_close,
> > +    nopage: vice_vma_nopage,
> > +};
> 
> Please use C99-initializers (i.e. .foo = bar)
> 
> > +void vice_cleanup_module(void)
> > +{
> > +#ifndef CONFIG_DEVFS_FS
> > +    /* cleanup_module is never called if registering failed */
> > +    unregister_chrdev(vice_major, "vice");
> > +#endif
> 
> Umm, just because someone makes the mistake of enabling devfs he
> doesn't have to use it.. :)
I'm not buying that one :)

> 
> > +#ifndef VICE_DEBUG
> > +    EXPORT_NO_SYMBOLS;		/* otherwise, leave global symbols visible */
> > +#endif
> 
> EXPORT_NO_SYMBOLS is a noop on 2.5
Driver was written for 2.4 first.


Thanks a lot for constructive criticism.

	Ilya.


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]