lkml.org 
[lkml]   [2010]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] IPC driver for Intel Mobile Internet Device (MID) platforms
> __packed, please.  (I've requested that this be added to checkpatch,
> but Mr Checkpatch is asleep).

Done


> > +static inline int busy_loop(void) /* Wait till scu status is busy */
> > +{
> > + u32 status = 0;
> > + u32 loop_count = 0;
> > +
> > + status = __raw_readl(IPC_STATUS_REG);
> > + while (status & 1) {
> > + udelay(1); /* scu processing time is in few u secods */
> > + status = __raw_readl(IPC_STATUS_REG);
> > + loop_count++;
> > + /* break if scu doesn't reset busy bit after huge retry */
> > + if (loop_count > 100000)
> > + return -ETIMEDOUT;

> This function has seven-odd callsites and is waaaaaaaay to fat and slow
> to be inlined.

Looking at the asm I'm not convinced.


> > + if (id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
> > + /* Workaround: values are read as 0 without memcpy_fromio */
> > + memcpy_fromio(cbuf, IPC_READ_BUFFER, 16);
>
> Should we still be doing this if busy_loop() failed?
>
> (Lots of dittoes on this question)

Does no harm

> > +}
>
> I wonder if this function would look better if cbuf had type u16[],

No - we do byte aligned access to it. and cbuf[offset + 1.5] is sadly not
supported by C (although I suspect you can make it work in C++ ;))

> > + mutex_lock(&ipclock);
> > + if (ipcdev.pdev == NULL) {
>
> This check happens a lot. Can it really happen?

Yes.

> I think mailbox_base could/should have had type `struct
> fw_update_mailbox __iomem *'. ioremap_nocache() should handle that
> cleanly, and this cast goes away.

Definitely - and the rest them cleans up by magic

Alan


\
 
 \ /
  Last update: 2010-04-22 15:15    [W:0.085 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site