Christoph Hellwig wrote:
>
> 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.
Now that I have looked at the code around it, it looks ok. It returns
the bytes copied to userspace so far, like a good read function should.
> > 6) remove check for pci_present(), redundant
>
> Lot's of pci drivers do this. Anyway I will removed this in the next version.
It's been redundant since the 2.1.x days, maybe even before that. There
-might- be a rare case where a driver really needs to check for the
presence of a PCI BIOS. Maybe. But for all cases where you have
if (!pci_present()) probe_pci=0;
/* ... */
if (probe_pci) {
pci_register_driver(...)
or
pdev = pci_find_device(...);
}
you can remove the pci_present check. All PCI probe functions return
sane values in the absence of PCI support (or PCI devices).
> > 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 ...
besides perpetuating a bad idea, that is.
Jeff
--
Jeff Garzik |
Building 1024 | Would you like a Twinkie?
MandrakeSoft |
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
