On Thu, Nov 09, 2000 at 12:02:45PM -0500, Jeff Garzik wrote:
> > I have an (untested) update for the cs46xx driver in Linux 2.4.
> > It includes Nils' 2.2 changes, use of initcalls (so compiled-in
> > should work) and use of the 2.4 PCI interface.
>
> Patch Generally looks ok. Comments:
>
> 1) This code is weird:
> > if (copy_to_user(buffer, dmabuf->rawbuf + swptr, cnt)) {
> >- if (!ret) ret = -EFAULT;
> >- return ret;
> >+ if (!ret)
> >+ ret = -EFAULT;
> >+ break;
> > }
>
> If you have an error condition (ret != 0), then you should not attempt
> the copy_to_user at all.
> If you do not have an error condition, you should unconditionally assign
> -EFAULT to 'ret'.
Makes sense. But I did not change the logic at all ...
Ask the Author (Alan or Nils) what this is about.
> 2) this patch enabled cs_mmap, and removes a check for vma->vm_offset !=
> 0. Also that is clearly 2.2.x code, simply removing the check is
> wrong. You should replace the check with one that checks vma->vm_pgoff
> != 0.
Sorry, forgot to add this back in the patch I sent.
> 3) is there method or madness to the delay changes? they are not
> explained, just changed...
Not shure. This was accepted in 2.2.18pre, so it should be sane.
> 4) cs_probe is marked __init but cs_remove is marked __devexit. on
> hotplug, cs_probe simply doesn't exist in the kernel anymore... boom.
Ok, should be fixed.
> 5) there is no need to appear zeroes to the end of these cs_pci_tbl
> entries. Just end each with "PCI_ANY_ID,"...
> + { PCI_VENDOR_ID_CIRRUS, 0x6001, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0
> },
> + { PCI_VENDOR_ID_CIRRUS, 0x6003, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0
> },
> + { PCI_VENDOR_ID_CIRRUS, 0x6004, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0
> },
That's true. But i think it looks cleaner if all members are present.
> 6) remove check for pci_present(), redundant
Lot's of pci drivers do this. Anyway I will removed this in the next version.
> 7) use pci_module_init for hotplug. quite simply:
>
> init_module() { return pci_module_init(&my_driver); }
Ok.
> of course for cs46xx, you will want to keep the version printk...
>
> 8) xxx_MODULE_NAME was a dumb and overly-lengthy creation of mine. Use
> instead MODNAME.
It doesn't really matter ...
Christoph
-- Always remember that you are unique. Just like everyone else. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed Nov 15 2000 - 21:00:14 EST