On Wed, Feb 07, 2007 at 08:52:25AM -0800, Andrew Morton wrote: > On Wed, 7 Feb 2007 11:48:25 +0000 Ben Dooks <[EMAIL PROTECTED]> wrote: > > > On Tue, Feb 06, 2007 at 09:09:30PM -0800, Andrew Morton wrote: > > > On Tue, 6 Feb 2007 19:26:28 +0000 Ben Dooks <[EMAIL PROTECTED]> wrote: > > > > > > > This patch is an update patch, ready for merging > > > > for the Silicon Motion SM501 multi-function device > > > > core. > > > > > > > > This driver handles the core function of the chip, > > > > including the clock, power control and allocation > > > > of resources for drivers. It also exports a series > > > > of platform devices for the function drivers to > > > > attach to. > > > > > > > > This patch supports both platform and PCI bus > > > > attached devices. > > > > > > > > Signed-off-by: Ben Dooks <[EMAIL PROTECTED]> > > > > > > Can we get Vincent's signoff here? > > > > > > > +#ifdef CONFIG_DEBUG > > > > > > This doesn't appear to be defined anywhere, and nor should it be. > > > Something subsystem-specific should be used here? > > > > This protects the code being used by dev_dbg() only from being > > warned as not being used. > > I know what is does, but I query the use of "CONFIG_DEBUG". I don't think > there's a CONFIG_DEBUG defined in existing Kconfig, and your patch doesn't > add a CONFIG_DEBUG and nor should it, because that would be an > inappropriate identifier to use. > > So I'd suggest you just use DEBUG, as many other drivers do. Or call it > CONFIG_SM501_DEBUG and add the Kconfig record to enable it.
You are right, too used to adding CONFIG_ from playing with Kconfig > > > > +#define fmt_freq(x) ((x) / MHZ), ((x) % MHZ), (x) > > > > > > eww. > > > > Do you have a better way of printing a nice formatted MHz with > > fractional parts? > > Nope. > > > Is it going to be necessary to remove this? > > Nope. But ewww. > > > > > + (void)readl(sm->regs); > > > > > > Is there any benefit in all those casts? Generally we prefer to avoid > > > them. > > > > I thoguht they where necessary to stop the compiler optimising > > away the readl() ? > > No, that shuldn't be necessary. If it was, the compiler would optimise > away the first readl() in > > my_local = readl(foo); > my_local = readl(bar); > > which would break stuff. readl() implementations use volatile to prevent > this. Ok, i've moved these into their own function called sm501_sync_regs() to make it more explicit what it is being used for. -- Ben ([EMAIL PROTECTED], http://www.fluff.org/) 'a smiley only costs 4 bytes' - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
