LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Grant Grundler <grundler@parisc-linux.org>
To: Greg KH <greg@kroah.com>
Cc: Grant Grundler <grundler@parisc-linux.org>,
	Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	linux-pci@atrey.karlin.mff.cuni.cz, Andrew Morton <akpm@osdl.org>,
	e1000-devel@lists.sourceforge.net, linux-scsi@vger.kernel.org,
	Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Subject: Re: [PATCH 1/5] Update Documentation/pci.txt
Date: Wed, 6 Dec 2006 20:55:06 -0700	[thread overview]
Message-ID: <20061207035506.GA2283@colo.lackof.org> (raw)
In-Reply-To: <20061206072651.GG17199@kroah.com>

On Tue, Dec 05, 2006 at 11:26:51PM -0800, Greg KH wrote:
> > If this looks good, I'll post a diff for gregkh.
> 
> This looks very good, thanks for doing this work.

welcome - feels like the most significant contribution I've made
to kernel in a long time.

> I do have a few minor comments:

I'll fix up the comments and post a diff in a few days.
Try to answer your questions below.

...
> > 	save_state	Save a device's state before it is suspended.
> 
> There is no such callback.  We have "suspend", "suspend_late",
> "resume_early", and "resume".  You might want to update this.

I'll review all the callbacks and verify we aren't missing any too.
Definitely want to update this. I'll add them but ask someone
else whose used those entry points rewrite my crap with a
seperate patch. I don't want to rathole on this patch.


...
> > 	driver_data	Data private to the driver.
> > 			Most drivers don't need to use driver_data field.
> > 			Best practice is to use driver_data as an index
> > 			into a static list of equivalent device types,
> > 			instead of using it as a pointer.
> 
> Perhaps mention the PCI_DEVICE() and PCI_DEVICE_CLASS() macros to set
> these fields properly?

Yes. Will add that.

> > Have a table entry {PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID}
> 
> PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) is a bit smaller :)

ditto.

> > All fields are passed in as hexadecimal values (no leading 0x).
> > Users need pass only as many fields as necessary:
> > 	ovendor, device, subvendor, and subdevice fields default
> > 	to PCI_ANY_ID (FFFFFFFF),
> > 	oclass and classmask fields default to 0
> > 	odriver_data defaults to 0UL.
> 
> What's with the "o"s here?

Those are supposed to be bullets.

Cute Story: I lost the first blank space on _every_ line when I messed up
my attempt to remove trailing whitespace in vi with ":1,$ s/ *//".
I left out the "$". :(
I didn't realize the mistake until a few minutes later and had to
hand edit every line to fix it up again. *ouch*
Lesson is to be more careful and save state before making global changes.



> 
> > Once added, the driver probe routine will be invoked for any unclaimed
> > PCI devices listed in its (newly updated) pci_ids list.
> > 
> > Device drivers must initialize use_driver_data in the dynids struct
> > in their pci_driver struct prior to calling pci_register_driver in order
> > for the driver_data field to get passed to the driver. Otherwise, only a
> > 0 (zero) is passed in that field.
> 
> Note that _no one_ uses this field, so it might go away soon...

When it's removed, the patch can remove that paragraph.
This is a seperate issue.


...
> > 	o If the driver is not a hotplug driver then use only
> > 	  __init/__exit and __initdata/__exitdata.
> 
> No, don't say this, pci drivers are not "hotplug drivers", and since
> CONFIG_HOTPLUG is really hard to turn off these days, don't confuse
> people with the devinit stuff.  Everyone gets it wrong...

Yeah. I'll drop the "is not a hotplug driver" bullet item.

> > 	o Pointers to functions marked as __devexit must be created using
> > 	  __devexit_p(function_name).  That will generate the function
> > 	  name or NULL if the __devexit function will be discarded.
> 
> I really recommend just not using any of these for almost all PCI
> drivers, as the space savings just really isn't there...

Ok.  fgrep found 321 instances of __devexit_p and thus I think we should
document it's use at a minimum. I'll just add "Most PCI drivers don't need
to use __devexit."

thanks for the comments!
Expect the next rev this weekend sometime.

grant

  reply	other threads:[~2006-12-07  3:55 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-11-22  8:05 Hidetoshi Seto
2006-11-22  8:54 ` Arjan van de Ven
2006-11-22 18:28 ` Grant Grundler
2006-11-24  0:38   ` Hidetoshi Seto
2006-11-24  5:12     ` Grant Grundler
2006-11-24  6:05       ` Hidetoshi Seto
2006-12-06  7:26       ` Greg KH
2006-12-07  3:55         ` Grant Grundler [this message]
2006-12-10  7:25         ` Grant Grundler
2006-12-15 17:02           ` Greg KH
2006-12-18  7:11             ` Grant Grundler
2006-12-22 19:46               ` Randy Dunlap
2006-12-22 21:52                 ` Stefan Richter
2006-12-24  6:11                 ` Grant Grundler
2006-12-24  6:07               ` [PATCH] Update Documentation/pci.txt v7 Grant Grundler
2006-12-24 19:16                 ` Randy Dunlap
2006-12-25  7:59                   ` Grant Grundler
2006-12-25  8:06                 ` Grant Grundler
2006-12-25  8:08                   ` Grant Grundler
2007-01-02 21:45                     ` Greg KH
2007-01-03  7:15                       ` Grant Grundler
2006-12-25  9:04                   ` Kenji Kaneshige
2007-01-16 22:26                   ` patch pci-rework-documentation-pci.txt.patch added to gregkh-2.6 tree gregkh
2007-01-17  9:10                     ` Jiri Slaby
2007-01-17 19:21                       ` Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20061207035506.GA2283@colo.lackof.org \
    --to=grundler@parisc-linux.org \
    --cc=akpm@osdl.org \
    --cc=e1000-devel@lists.sourceforge.net \
    --cc=greg@kroah.com \
    --cc=kaneshige.kenji@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@atrey.karlin.mff.cuni.cz \
    --cc=linux-scsi@vger.kernel.org \
    --cc=seto.hidetoshi@jp.fujitsu.com \
    --subject='Re: [PATCH 1/5] Update Documentation/pci.txt' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).