Messages in this thread | | | Date | Tue, 7 Oct 2003 01:27:27 -0700 | From | Jeremy Higdon <> | Subject | Re: Patch to add support for SGI's IOC4 chipset |
| |
Hello Bartlomiej,
I have a few questions. I'm going to be submitting the rest of the patches that Aniket was working on, as he started at a new company today (he was attempting to finish the submission before he left). Please forgive a lack of expertise on my part.
On Sat, Oct 04, 2003 at 07:30:15PM +0200, Bartlomiej Zolnierkiewicz wrote: > > > + return p - buffer; > > +} > > > > >Do you really need /proc/ide/sgiioc4? > > >You can print revision number during init. > > > > It has been helpful to be able to see the firmware revision num anytime > > during system operation. > > So the new patch still creates the above entry. > > I don't buy this, lspci can be used :-).
lspci gives the version number. /proc/ide/sgiioc4 gives you:
SGI IOC4 Chipset rev 79. Chipset has 1 IDE channel and supports 2 devices on that channel. Chipset supports DMA in MultiMode-2 data transfer protocol.
Is the # of IDE channels/devices and the DMA mode also available elsewhere?
> > + int ddir); > > +static unsigned int __init pci_init_sgiioc4(struct pci_dev > > *dev,ide_pci_device_t *d); > > > > >Most of this declarations are not needed as sgiioc4.h is only included > > > from shiioc4.c. > > > > The sgiioc4.h file has been removed in the new patch. > > sgiioc4.h was removed, but declarations weren't. > You can shuffle code around to get rid of them.
How important is this to you? It seems more a style issue. I agree with you, by the way. When I write code, I try to minimize forward declarations. If I can get rid of some easily, will that be good enough?
> There are no .enablebits on SGI IOC4? Please add a comment about it.
What are they used for (i.e. what are you looking for in the comment)?
> sgiioc4_init_one(): > + pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev); > + class_rev &= 0xff; > > Access to PCI devices before pci_enable_device() > (it is called later in pci_init_sgiioc4).
Is pci_enable_device required before a config space access?
I will make changes in accordance with the other comments and based on your responses to this.
Thanks
jeremy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |