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 <><
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
