Subject: Re: [PATCH v2 4/7] bestcomm: core bestcomm support for Freescale MPC5200

From: Matt Sealey <[email-masked]>
Date: Mon, 15 Oct 2007 12:55:07 +0100

My nits:

Grant Likely wrote:
> From: Sylvain Munaut <[email-masked]>
> +static int __devinit
> +bcom_engine_init(void)

Why "bcom" and not "bestcomm"?

> +	/* Disable COMM Bus Prefetch, apparently it's not reliable yet */
> +	/* FIXME: This should be done on 5200 and not 5200B ... */
> +	out_be16(&bcom_eng->regs->PtdCntrl, in_be16(&bcom_eng->regs->PtdCntrl) | 1);

This really, really shouldn't even be here, could it be moved to a platform
init, or switched on a PVR/SVR here?

> +int bcom_sram_init(struct device_node *sram_node, char *owner)
> +{
> +	int rv;
> +	const u32 *regaddr_p;
> +	u64 regaddr64, size64;
> +	unsigned int psize;
> +
> +	/* Create our state struct */
> +	if (bcom_sram) {
> +		printk(KERN_ERR "%s: bcom_sram_init: "
> +			"Already initialiwed !\n", owner);
> +		return -EBUSY;
> +	}

initialised or initialized :)

> +		printk(KERN_ERR "%s: bcom_sram_init: "
> +			"Couln't request region !\n", owner);
> +		rv = -EBUSY;
> +		goto error_free;

Couldn't or could not.

-- 
Matt Sealey <[email-masked]>
Genesi, Manager, Developer Relations