From: Ben Dooks <ben-linux@fluff.org> To: Andrew Morton <akpm@linux-foundation.org> Cc: Ben Dooks <ben-linux@fluff.org>, linux-kernel@vger.kernel.org, linux-fbdev-devel@lists.sourceforge.net, Greg KH <greg@kroah.com> Subject: Re: [PATCH] mfd: SM501 core driver Date: Wed, 7 Feb 2007 17:09:49 +0000 [thread overview] Message-ID: <20070207170949.GD21120@fluff.org.uk> (raw) In-Reply-To: <20070207085225.c2371cfd.akpm@linux-foundation.org> On Wed, Feb 07, 2007 at 08:52:25AM -0800, Andrew Morton wrote: > On Wed, 7 Feb 2007 11:48:25 +0000 Ben Dooks <ben-linux@fluff.org> 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 <ben-linux@fluff.org> 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 <ben-linux@fluff.org> > > > > > > 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 (ben@fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes'
WARNING: multiple messages have this Message-ID (diff)
From: Ben Dooks <ben-linux@fluff.org> To: Andrew Morton <akpm@linux-foundation.org> Cc: Greg KH <greg@kroah.com>, linux-fbdev-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, Ben Dooks <ben-linux@fluff.org> Subject: Re: [PATCH] mfd: SM501 core driver Date: Wed, 7 Feb 2007 17:09:49 +0000 [thread overview] Message-ID: <20070207170949.GD21120@fluff.org.uk> (raw) In-Reply-To: <20070207085225.c2371cfd.akpm@linux-foundation.org> On Wed, Feb 07, 2007 at 08:52:25AM -0800, Andrew Morton wrote: > On Wed, 7 Feb 2007 11:48:25 +0000 Ben Dooks <ben-linux@fluff.org> 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 <ben-linux@fluff.org> 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 <ben-linux@fluff.org> > > > > > > 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 (ben@fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' ------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
next prev parent reply other threads:[~2007-02-07 17:10 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2007-02-06 19:26 [PATCH] mfd: SM501 core driver Ben Dooks 2007-02-07 5:09 ` Andrew Morton 2007-02-07 5:09 ` Andrew Morton 2007-02-07 11:48 ` Ben Dooks 2007-02-07 11:48 ` Ben Dooks 2007-02-07 14:24 ` Greg KH 2007-02-07 15:29 ` Ben Dooks 2007-02-07 16:52 ` Andrew Morton 2007-02-07 17:09 ` Ben Dooks [this message] 2007-02-07 17:09 ` Ben Dooks 2007-02-08 17:56 ` James Simmons 2007-02-08 17:56 ` James Simmons 2007-02-12 11:12 ` [Linux-fbdev-devel] " Ben Dooks 2007-02-07 17:38 ` Vincent Sanders 2007-02-07 17:38 ` Vincent Sanders
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=20070207170949.GD21120@fluff.org.uk \ --to=ben-linux@fluff.org \ --cc=akpm@linux-foundation.org \ --cc=greg@kroah.com \ --cc=linux-fbdev-devel@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.