Re: [RFC/PATCHv2] powerpc: Move CPM command handling into the cpm drivers

[Arnd Bergmann <arnd@arndb.de>]

On Saturday 24 November 2007, Jochen Friedrich wrote:
> This patch moves the CPM command handling into commproc.c
> for CPM1 and cpm2_common.c. This is yet another preparation
> to get rid of drivers accessing the CPM via the global cpmp.

good stuff, just a little nitpicking below:

> +DEFINE_SPINLOCK(cmd_lock);

This should probably be a static variable. The name is too generic
for a global identifier. If every use of the command register
goes through the cpm_command function, you could even make it
a static variable in that function.

> +int cpm_command(u32 command, u8 opcode)
> +{
> +	int i;
> +	unsigned long flags;
> +
> +	if (command & 0xffffff0f)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&cmd_lock, flags);
> +
> +	out_be16(&cpmp->cp_cpcr, command | CPM_CR_FLG | (opcode << 8));
> +	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
> +		if ((in_be16(&cpmp->cp_cpcr) & CPM_CR_FLG) == 0) {
> +			spin_unlock_irqrestore(&cmd_lock, flags);
> +			return 0;
> +		}

Error handling in here is nicer if you do a goto that jumps to the
unlock below. If the code ever gets more complex, this makes it easier
to check if it's correct regarding locking.

> +	printk(KERN_ERR "%s(): Not able to issue CPM command\n", __FUNCTION__);
> +	spin_unlock_irqrestore(&cmd_lock, flags);
> +	return -EIO;
> +}
> +EXPORT_SYMBOL(cpm_command);

Why not EXPORT_SYMBOL_GPL?
  
> +DEFINE_SPINLOCK(cmd_lock);

see above

> +
> +#define MAX_CR_CMD_LOOPS        10000
> +
> +int cpm_command(u32 command, u8 opcode)
> +{
> +	int i;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cmd_lock, flags);
> +
> +	out_be32(&cpmp->cp_cpcr, command | opcode | CPM_CR_FLG);
> +	for (i = 0; i < MAX_CR_CMD_LOOPS; i++)
> +		if ((in_be32(&cpmp->cp_cpcr) & CPM_CR_FLG) == 0) {
> +			spin_unlock_irqrestore(&cmd_lock, flags);
> +			return 0;
> +		}
> +
> +	printk(KERN_ERR "%s(): Not able to issue CPM command\n", __FUNCTION__);
> +	spin_unlock_irqrestore(&cmd_lock, flags);
> +	return -EIO;
> +}
> +EXPORT_SYMBOL(cpm_command);

see above

	Arnd <><