Re: [PATCH] [POWERPC] 8xx: mpc885ads pcmcia support

On Fri, 04 May 2007 03:57:51 +0400
Vitaly Bordug [off-list ref] wrote:

Adds support for PowerQuicc on-chip PCMCIA. The driver is implemented as
of_device, so only arch/powerpc stuff is capable to use it, which now
implies only mpc885ads reference board.

To cope with the code that should be hooked inside driver, but is really
board specific (like set_voltage), global structure mpc8xx_pcmcia_ops
holds necessary function pointers that are filled in the BSP code.

argh.

akpm:/home/akpm> grep '^.*        ' x | wc -l 
72

please, Linux uses hard-tabs, not spacespacespacespacespacespacespacespace
everywhere.

Please declare this in a header file.

+/* Some internal interrupt registers use an 8-bit mask for the interrupt
+ * level instead of a number.
+ */

Standard Linux commenting style is:

/*
 * Some internal interrupt registers use an 8-bit mask for the interrupt
 * level instead of a number.
 */

+#define mk_int_int_mask(IL) (1 << (7 - (IL/2)))

Insufficiently parenthesised?

static inline functions are preferred.

+#ifdef CONFIG_PCMCIA_M8XX
+extern struct mpc8xx_pcmcia_ops m8xx_pcmcia_ops;

Please don't ever put extern declarations in C files.  Declare it in a
header, include that header in the C file which contains the definition as
well as within all C files which use the symbol.

Missing a tab.

+	iounmap(bcsr_io);
+}
+
+static int pcmcia_set_voltage(int slot, int vcc, int vpp)
+{
+        u32 reg = 0;
+        unsigned *bcsr_io;
+
+        bcsr_io = ioremap(BCSR1, sizeof(unsigned long));
+
+        switch(vcc) {
+                case 0:
+                        break;
+                case 33:
+                        reg |= BCSR1_PCCVCC0;
+                        break;
+                case 50:
+                        reg |= BCSR1_PCCVCC1;
+                        break;
+                default:
+                        return 1;
+        }

Standard Linux layout for switch statements is:

        switch(vcc) {
	case 0:
		break;
	case 33:
		reg |= BCSR1_PCCVCC0;
		break;
	case 50:
		reg |= BCSR1_PCCVCC1;
		break;
	default:
		return 1;
        }

+        switch(vpp) {
+                case 0:
+                        break;
+                case 33:
+                case 50:
+                        if(vcc == vpp)
+                                reg |= BCSR1_PCCVPP1;
+                        else
+                                return 1;
+                        break;
+                case 120:
+                        if ((vcc == 33) || (vcc == 50))
+                                reg |= BCSR1_PCCVPP0;
+                        else
+                                return 1;
+                default:
+                        return 1;
+        }

Ditto.

please try to fit code into 80 columns.

 
-static DEFINE_SPINLOCK(events_lock);
+static spinlock_t events_lock = SPIN_LOCK_UNLOCKED;

This is wrong.  DEFINE_SPINLOCK is required for correct lockdep operation.

+#define hardware_enable(_slot_) m8xx_pcmcia_ops.hw_ctrl(_slot_, 1)
+#define hardware_disable(_slot_) m8xx_pcmcia_ops.hw_ctrl(_slot_, 0)
+#define voltage_set(slot, vcc, vpp) m8xx_pcmcia_ops.voltage_set(slot, vcc, vpp)

static inline functions are preferred.  One reason is that people are more
inclined to add comments to them than to macros.

-static DEFINE_SPINLOCK(pending_event_lock);
+static spinlock_t pending_event_lock = SPIN_LOCK_UNLOCKED;

again, you added a bug.

+	for(i = 0; i < PCMCIA_SOCKETS_NO; i++){

No, we format `for' statements as:

	for (i = 0; i < PCMCIA_SOCKETS_NO; i++) {

+		w = (void *) &pcmcia->pcmc_pbr0;
 
+		out_be32(&pcmcia->pcmc_pscr, M8XX_PCMCIA_MASK(i));
+		out_be32(&pcmcia->pcmc_per, in_be32(&pcmcia->pcmc_per) & ~M8XX_PCMCIA_MASK(i));

80-cols

+
+		/* turn off interrupt and disable CxOE */
+		out_be32(M8XX_PGCRX(i), M8XX_PGCRX_CXOE);
+
+		/* turn off memory windows */
+		for(m = 0; m < PCMCIA_MEM_WIN_NO; m++) {

formatting

+			out_be32(&w->or, 0); /* set to not valid */
+			w++;
+		}
+
+		/* turn off voltage */
+		voltage_set(i, 0, 0);
+
+		/* disable external hardware */
+		hardware_disable(i);
+	}
 	for (i = 0; i < PCMCIA_SOCKETS_NO; i++)
 		pcmcia_unregister_socket(&socket[i].socket);
 
-	m8xx_shutdown();
+	free_irq(pcmcia_schlvl, NULL);
+
+	return 0;
+}

-	platform_device_unregister(&m8xx_device);
-	driver_unregister(&m8xx_driver);
+#ifdef CONFIG_PM
+static int m8xx_suspend(struct platform_device *pdev, pm_message_t state)
+{
+        return pcmcia_socket_dev_suspend(&pdev->dev, state);
+}
+
+static int m8xx_resume(struct platform_device *pdev)
+{
+        return pcmcia_socket_dev_resume(&pdev->dev);
+}
+#endif

Here, use

#else
#define m8xx_suspend NULL
#define m8xx_resume NULL
#endif

+static struct of_device_id m8xx_pcmcia_match[] = {
+	{
+		.type = "pcmcia",
+		.compatible = "fsl,pq-pcmcia",
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, m8xx_pcmcia_match);
+
+static struct of_platform_driver m8xx_pcmcia_driver = {
+	.name		= (char *) driver_name,
+	.match_table	= m8xx_pcmcia_match,
+	.probe		= m8xx_probe,
+	.remove		= m8xx_remove,
+#ifdef CONFIG_PM
+	.suspend	= m8xx_suspend,
+	.resume		= m8xx_resume,
+#endif

then remove this ifdef.