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.
> > +static const unsigned int misc_div[] = {
> > + [0] = 1,
> > + [1] = 2,
> > + [2] = 4,
> > + [3] = 8,
> > + [4] = 16,
> > + [5] = 32,
> > + [6] = 64,
> > + [7] = 128,
> > + [8] = 3,
> > + [9] = 6,
> > + [10] = 12,
> > + [11] = 24,
> > + [12] = 48,
> > + [13] = 96,
> > + [14] = 192,
> > + [15] = 384,
> > +};
> > +
> > +static const unsigned int px_div[] = {
> > + [0] = 1,
> > + [1] = 2,
> > + [2] = 4,
> > + [3] = 8,
> > + [4] = 16,
> > + [5] = 32,
> > + [6] = 64,
> > + [7] = 128,
> > + [8] = 3,
> > + [9] = 6,
> > + [10] = 12,
> > + [11] = 24,
> > + [12] = 48,
> > + [13] = 96,
> > + [14] = 192,
> > + [15] = 384,
> > + [16] = 5,
> > + [17] = 10,
> > + [18] = 20,
> > + [19] = 40,
> > + [20] = 80,
> > + [21] = 160,
> > + [22] = 320,
> > + [23] = 604,
> > +};
> > +#endif
> > +
> > +#define decode_div(val, lshft, selbit, mask, dtab) \
> > + ((((val) & (selbit)) ? pll2 : 288 * MHZ) / dtab[(((val) >> (lshft)) &
> > (mask))])
>
> A C function would be nicer here, if poss.
>
> > +#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? Is it going to be necessary to remove this?
> > + (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() ?
> > +EXPORT_SYMBOL_GPL(sm501_misc_control);
>
> The driver exports a lot of symbols. Does it really need to?
Yes, these are used by the drivers for the specific function
blocks in the chip.
> > + down(&sm->clock_lock);
>
> Please convert to a mutex, if possible.
The current callers are the device initialisation and ioctl() calls
from the framebuffer layer. I expect all the other callers to be from
ioctl() calls (to update device parameters) or possibly from sysfs
file interaction, so I think that a mutex may be okay for this.
> > +/* sm501_null_release
> > + *
> > + * A release function for the platform devices we create to keep the
> > + * driver core happy, and stop any crashed when the devices are removed
> > +*/
> > +
> > +static void sm501_null_release(struct device *dev)
> > +{
> > +}
>
> Greg might have an opinion on that ;)
Without this the system OOPses when the driver is removed.
> > +static void sm501_create_mem(struct sm501_devdata *sm,
> > + struct resource *res,
> > + unsigned long *offs,
> > + unsigned long size)
> > +{
> > + *offs -= size; /* adjust memory size */
> > +
> > + res->flags = IORESOURCE_MEM;
> > + res->parent = sm->mem_res;
> > + res->start = sm->mem_res->start + *offs;
> > + res->end = res->start + size - 1;
> > +}
>
> Please use resource_size_t throughout, test with CONFIG_RESOURCES_64BIT on
> and off.
I will go and fix this.5~
> > +static inline void sm501_init_reg(struct sm501_devdata *sm,
> > + unsigned long reg,
> > + struct sm501_reg_init *r)
> > +{
> > + unsigned long tmp;
> > +
> > + tmp = readl(sm->regs + reg);
> > + tmp |= r->set;
> > + tmp &= ~r->mask;
> > + writel(tmp, sm->regs + reg);
> > +}
>
> This might be too large to inline.
possibly.
--
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/
