Re: [PATCH] IPC driver for Intel Mobile Internet Device (MID) platforms

[Matthew Garrett <mjg59@srcf.ucam.org>]

On Fri, Apr 23, 2010 at 03:30:59PM +0100, Alan Cox wrote:

Seems basically fine, with a few comments inline. One not especially 
related query in passing - the graphics driver for Moorestown hasn't 
been submitted yet, and the version in Meego doesn't look like it's 
especially upstreamable. I seem to recall that Moorestown was going to 
lack port IO, so does it have VGA compatibility? If not, is there any 
use in mainline Moorestown support until support for the graphics is 
submitted?

> +/* Read single register */
> +int intel_scu_ipc_ioread8(u16 addr, u8 *data);
> +
> +/* Read two registers */
> +int intel_scu_ipc_ioread16(u16 addr, u16 *data);
> +
> +/* Read four registers */
> +int intel_scu_ipc_ioread32(u16 addr, u32 *data);

Are these comments really needed?

> + * Update single register based on the mask
> + * First element of data is register value and second element is mask value
> + */
> +int intel_scu_ipc_update_register(u16 addr, u8 data[2]);

Would seem neater as two separate arguments rather than an array...

> + * SCU runing in ARC processor communicates with other entity running in IA
> + * core through IPC mechanism which in turn messaging between IA core ad SCU.
> + * SCU has two IPC mechanism IPC-1 and IPC-2. IPC-1 is used between IA32 and
> + * SCU where IPC-2 is used between P-Unit and SCU. This driver delas with
> + * IPC-1 Driver provides an API for power control unit registers (e.g. MSIC)
> + * along with other APIs.

This isn't especially clear. I think I understand what it's talking 
about (There's a service processor and an x86, and this driver is 
responsible for handing the communication between the two - but there's 
also another IPC mechanism for talking to P-Unit, which isn't explained 
here but at a guess is some kind of hip-hop group and thus outside the 
scope of this driver?

> +#define IPC_BASE_ADDR     0xFF11C000	/* IPC1 base register address */

The address is guaranteed to be stable? This doesn't need to come from 
SFI or anything?

> +static struct intel_scu_ipc_dev  ipcdev; /* Only one for now */

If this is going to change in future then it might be sane to get it 
right now.

> +static int platform = 1;
> +module_param(platform, int, 0);
> +MODULE_PARM_DESC(platform, "1 for moorestown platform");

What do non-1 values mean? It looks like everything other than 1 
provides some default behaviour. Could you check for invalid values and 
abort in order to avoid breakage if someone depends on this?

> +static inline u8 ipc_readl(u32 offset) /* Read ipc u32 data */

read16 in places and readl in others?

> +int intel_scu_ipc_battery_cc_read(u32 *value)

I'm a bit confused by the cc functions. The actual battery code isn't in 
this driver, so why are these functions not in the battery driver?

> +int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)

Same here. If the i2c functionality is in this device, why not keep the 
full i2c driver code here?

> +	mutex_lock(&ipclock);
> +	cmd = (addr >> 24) & 0xFF;
> +	if (cmd == IPC_I2C_READ) {
> +		writel(addr, ipcdev.i2c_base + IPC_I2C_CNTRL_ADDR);
> +		/* Write not getting updated without delay */

It needs a delay rather than a posting read?

> +/*
> + * Interrupt handler gets called when ioc bit of IPC_COMMAND_REG set to 1
> + * When ioc bit is set to 1, caller api must wait for interrupt handler called
> + * which in turn unlocks the caller api. Currently this is not used

Is this a problem? It looks like ioc can only be set if someone passes 
it in via intel_scu_ipc_simple_command() and that's a pretty obvious 
"Don't do that", but it'd be nice to get rid of the busy looping.

All of these symbols are EXPORT_SYMBOL. Is there a reason they're not 
EXPORT_SYMBOL_GPL?

-- 
Matthew Garrett | mjg59@srcf.ucam.org