Messages in this thread | | | Date | Thu, 22 Apr 2010 14:16:11 +0100 | From | Alan Cox <> | Subject | Re: [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
| |