lkml.org 
[lkml]   [2007]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mfd: SM501 core driver
    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.

    > > > +#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.


    -
    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/

    \
     
     \ /
      Last update: 2007-02-07 17:55    [W:2.185 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site