Skip to contentSkip to conversation

omap: mailbox: bunch of cleanups

PATCH Superseded Felipe Contreras 6 participants 38 messages May 22, 2010 – Jun 11, 2010

Subsystems (4)
Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP1 support

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Felipe Contreras

author
#0 lore raw

v3 cover omap: mailbox: bunch of cleanups

Hi,

These are hopefully non-functional changes. Just shuffling code around, and removing unecessary stuff.

In v2 I tried to split platform_device and also make mailbox_mach built-in. Not any more.

Comments from Felipe Balbi, Tony Lindgren, Hiroshi DOYU, and Russell King.

Felipe Contreras (14):
omap: mailbox: trivial whitespace cleanups
omap: mailbox: trivial cleanups
omap: mailbox: reorganize structures
omap: mailbox: 2420 should be detected at run-time
omap: mailbox: use correct config for omap1
omap: mailbox: update omap1 probing
omap: mailbox: don't export unecessary symbols
omap: mailbox: remove unecessary fields
omap: mailbox: add IRQ names
omap: mailbox: reorganize registering
omap: mailbox: only compile for configured archs
omap: mailbox: standarize on 'omap-mailbox'
omap: mailbox: simplify omap_mbox_register()
omap: mailbox: reorganize headers

 arch/arm/mach-omap1/devices.c             |   11 +-
 arch/arm/mach-omap1/mailbox.c             |   52 ++++-----
 arch/arm/mach-omap2/devices.c             |    6 +-
 arch/arm/mach-omap2/mailbox.c             |  177 +++++++++++++----------------
 arch/arm/plat-omap/include/plat/mailbox.h |   16 +--
 arch/arm/plat-omap/mailbox.c              |  136 +++++++++-------------
 6 files changed, 168 insertions(+), 230 deletions(-)
author
#1 lore raw

PATCH v3 1/14 omap: mailbox: trivial whitespace cleanups

Signed-off-by Felipe Contreras <[email protected]>

---

Hiroshi: I think there's something wrong with your editor =/

 arch/arm/plat-omap/mailbox.c |   44 +++++++++++++++++++++---------------------
 1 files changed, 22 insertions(+), 22 deletions(-)
arch/arm/plat-omap/mailbox.c +22 -22
index 36b3aa2..38a6cb1 100644
@@ -142,14 +142,14 @@ static void mbox_rx_work(struct work_struct *work)
 	struct omap_mbox_queue *mq =
 			container_of(work, struct omap_mbox_queue, work);
 	mbox_msg_t msg;
- 	int len;
-  
- 	while (kfifo_len(&mq->fifo) >= sizeof(msg)) {
- 		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
- 		WARN_ON(len != sizeof(msg));
-
- 		if (mq->callback)
- 			mq->callback((void *)msg);
+	int len;
+
+	while (kfifo_len(&mq->fifo) >= sizeof(msg)) {
+		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+		WARN_ON(len != sizeof(msg));
+
+		if (mq->callback)
+			mq->callback((void *)msg);
 	}
 }
 

@@ -165,12 +165,12 @@ static void __mbox_tx_interrupt(struct omap_mbox *mbox)
 
 static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 {
- 	struct omap_mbox_queue *mq = mbox->rxq;
+	struct omap_mbox_queue *mq = mbox->rxq;
 	mbox_msg_t msg;
- 	int len;
+	int len;
 
 	while (!mbox_fifo_empty(mbox)) {
- 		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
+		if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
 			omap_mbox_disable_irq(mbox, IRQ_RX);
 			rq_full = true;
 			goto nomem;

@@ -178,8 +178,8 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox)
 
 		msg = mbox_fifo_read(mbox);
 
- 		len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
- 		WARN_ON(len != sizeof(msg));
+		len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+		WARN_ON(len != sizeof(msg));
 
 		if (mbox->ops->type == OMAP_MBOX_TYPE1)
 			break;

@@ -278,11 +278,11 @@ static int omap_mbox_startup(struct omap_mbox *mbox)
 
 	return 0;
 
- fail_alloc_rxq:
+fail_alloc_rxq:
 	mbox_queue_free(mbox->txq);
- fail_alloc_txq:
+fail_alloc_txq:
 	free_irq(mbox->irq, mbox);
- fail_request_irq:
+fail_request_irq:
 	if (mbox->ops->shutdown)
 		mbox->ops->shutdown(mbox);
 

@@ -359,10 +359,10 @@ int omap_mbox_register(struct device *parent, struct omap_mbox *mbox)
 	if (mbox->next)
 		return -EBUSY;
 
- 	mbox->dev = device_create(&omap_mbox_class,
- 				  parent, 0, mbox, "%s", mbox->name);
- 	if (IS_ERR(mbox->dev))
- 		return PTR_ERR(mbox->dev);
+	mbox->dev = device_create(&omap_mbox_class,
+				  parent, 0, mbox, "%s", mbox->name);
+	if (IS_ERR(mbox->dev))
+		return PTR_ERR(mbox->dev);
 
 	spin_lock(&mboxes_lock);
 	tmp = find_mboxes(mbox->name);

@@ -405,7 +405,7 @@ EXPORT_SYMBOL(omap_mbox_unregister);
 
 static int __init omap_mbox_init(void)
 {
- 	int err;
+	int err;
 
 	err = class_register(&omap_mbox_class);
 	if (err)

@@ -426,7 +426,7 @@ subsys_initcall(omap_mbox_init);
 static void __exit omap_mbox_exit(void)
 {
 	destroy_workqueue(mboxd);
- 	class_unregister(&omap_mbox_class);
+	class_unregister(&omap_mbox_class);
 }
 module_exit(omap_mbox_exit);
Signature · 1 line
1.7.1
#2 lore raw

PATCH v3 1/14 omap: mailbox: trivial whitespace cleanups

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 01/14] omap: mailbox: trivial whitespace cleanups
Date: Sat, 22 May 2010 19:14:12 +0200

Quoted 4 lines
Signed-off-by Felipe Contreras <[email protected]>

---

Hiroshi: I think there's something wrong with your editor =/

Hm...I did something wrong to save them. I'll fix. Thanks.

author
#3 lore raw

PATCH v3 2/14 omap: mailbox: trivial cleanups

In reply to Felipe Contreras

And fix a few compilation warnings.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/devices.c |    6 ++----
 arch/arm/mach-omap1/mailbox.c |    2 +-
 arch/arm/mach-omap2/mailbox.c |   10 ++++++----
 3 files changed, 9 insertions(+), 9 deletions(-)
arch/arm/mach-omap1/devices.c +2 -4
index 379100c..462b59c 100644
@@ -73,12 +73,10 @@ static inline void omap_init_rtc(void) {}
 #  define INT_DSP_MAILBOX1	INT_1610_DSP_MAILBOX1
 #endif
 
-#define OMAP1_MBOX_BASE		OMAP16XX_MAILBOX_BASE
-
 static struct resource mbox_resources[] = {
 	{
-		.start		= OMAP1_MBOX_BASE,
-		.end		= OMAP1_MBOX_BASE + OMAP1_MBOX_SIZE,
+		.start		= OMAP16XX_MAILBOX_BASE,
+		.end		= OMAP16XX_MAILBOX_BASE + OMAP1_MBOX_SIZE,
 		.flags		= IORESOURCE_MEM,
 	},
 	{
arch/arm/mach-omap1/mailbox.c +1 -1
index 4f5b3da..15bf2a2 100644
@@ -83,7 +83,7 @@ static int omap1_mbox_fifo_full(struct omap_mbox *mbox)
 	struct omap_mbox1_fifo *fifo =
 		&((struct omap_mbox1_priv *)mbox->priv)->rx_fifo;
 
-	return (mbox_read_reg(fifo->flag));
+	return mbox_read_reg(fifo->flag);
 }
 
 /* irq */
arch/arm/mach-omap2/mailbox.c +6 -4
index 763272c..8c1070c 100644
@@ -131,7 +131,7 @@ static int omap2_mbox_startup(struct omap_mbox *mbox)
 	}
 
 	l = mbox_read_reg(MAILBOX_REVISION);
-	pr_info("omap mailbox rev %d.%d\n", (l & 0xf0) >> 4, (l & 0x0f));
+	pr_debug("omap mailbox rev %d.%d\n", (l & 0xf0) >> 4, (l & 0x0f));
 
 	if (cpu_is_omap44xx())
 		l = OMAP4_SMARTIDLE;

@@ -300,8 +300,6 @@ static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
 	.irqdisable	= MAILBOX_IRQENABLE(0),
 };
 
-
-
 /* OMAP4 specific data structure. Use the cpu_is_omap4xxx()
 to use this*/
 static struct omap_mbox2_priv omap2_mbox_1_priv = {

@@ -357,7 +355,6 @@ struct omap_mbox mbox_2_info = {
 };
 EXPORT_SYMBOL(mbox_2_info);
 
-
 #if defined(CONFIG_ARCH_OMAP2420) /* IVA */
 static struct omap_mbox2_priv omap2_mbox_iva_priv = {
 	.tx_fifo = {

@@ -443,6 +440,11 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 #endif
 	return 0;
 
+#if defined(CONFIG_ARCH_OMAP2420) /* IVA */
+err_iva1:
+	omap_mbox_unregister(&mbox_dsp_info);
+#endif
+
 err_dsp:
 	iounmap(mbox_base);
 	return ret;
Signature · 1 line
1.7.1
author
#4 lore raw

PATCH v3 3/14 omap: mailbox: reorganize structures

In reply to Felipe Contreras

OMAP4 ones messed up the organization.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap2/mailbox.c |   68 +++++++++++++++++++++--------------------
 1 files changed, 35 insertions(+), 33 deletions(-)
arch/arm/mach-omap2/mailbox.c +35 -33
index 8c1070c..edcfec6 100644
@@ -283,6 +283,7 @@ static struct omap_mbox_ops omap2_mbox_ops = {
  */
 
 /* FIXME: the following structs should be filled automatically by the user id */
+
 /* DSP */
 static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
 	.tx_fifo = {

@@ -300,8 +301,40 @@ static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
 	.irqdisable	= MAILBOX_IRQENABLE(0),
 };
 
-/* OMAP4 specific data structure. Use the cpu_is_omap4xxx()
-to use this*/
+struct omap_mbox mbox_dsp_info = {
+	.name	= "dsp",
+	.ops	= &omap2_mbox_ops,
+	.priv	= &omap2_mbox_dsp_priv,
+};
+EXPORT_SYMBOL(mbox_dsp_info);
+
+#if defined(CONFIG_ARCH_OMAP2420)
+
+/* IVA */
+static struct omap_mbox2_priv omap2_mbox_iva_priv = {
+	.tx_fifo = {
+		.msg		= MAILBOX_MESSAGE(2),
+		.fifo_stat	= MAILBOX_FIFOSTATUS(2),
+	},
+	.rx_fifo = {
+		.msg		= MAILBOX_MESSAGE(3),
+		.msg_stat	= MAILBOX_MSGSTATUS(3),
+	},
+	.irqenable	= MAILBOX_IRQENABLE(3),
+	.irqstatus	= MAILBOX_IRQSTATUS(3),
+	.notfull_bit	= MAILBOX_IRQ_NOTFULL(2),
+	.newmsg_bit	= MAILBOX_IRQ_NEWMSG(3),
+	.irqdisable	= MAILBOX_IRQENABLE(3),
+};
+
+static struct omap_mbox mbox_iva_info = {
+	.name	= "iva",
+	.ops	= &omap2_mbox_ops,
+	.priv	= &omap2_mbox_iva_priv,
+};
+#endif
+
+/* OMAP4 */
 static struct omap_mbox2_priv omap2_mbox_1_priv = {
 	.tx_fifo = {
 		.msg		= MAILBOX_MESSAGE(0),

@@ -325,13 +358,6 @@ struct omap_mbox mbox_1_info = {
 };
 EXPORT_SYMBOL(mbox_1_info);
 
-struct omap_mbox mbox_dsp_info = {
-	.name	= "dsp",
-	.ops	= &omap2_mbox_ops,
-	.priv	= &omap2_mbox_dsp_priv,
-};
-EXPORT_SYMBOL(mbox_dsp_info);
-
 static struct omap_mbox2_priv omap2_mbox_2_priv = {
 	.tx_fifo = {
 		.msg		= MAILBOX_MESSAGE(3),

@@ -355,30 +381,6 @@ struct omap_mbox mbox_2_info = {
 };
 EXPORT_SYMBOL(mbox_2_info);
 
-#if defined(CONFIG_ARCH_OMAP2420) /* IVA */
-static struct omap_mbox2_priv omap2_mbox_iva_priv = {
-	.tx_fifo = {
-		.msg		= MAILBOX_MESSAGE(2),
-		.fifo_stat	= MAILBOX_FIFOSTATUS(2),
-	},
-	.rx_fifo = {
-		.msg		= MAILBOX_MESSAGE(3),
-		.msg_stat	= MAILBOX_MSGSTATUS(3),
-	},
-	.irqenable	= MAILBOX_IRQENABLE(3),
-	.irqstatus	= MAILBOX_IRQSTATUS(3),
-	.notfull_bit	= MAILBOX_IRQ_NOTFULL(2),
-	.newmsg_bit	= MAILBOX_IRQ_NEWMSG(3),
-	.irqdisable	= MAILBOX_IRQENABLE(3),
-};
-
-static struct omap_mbox mbox_iva_info = {
-	.name	= "iva",
-	.ops	= &omap2_mbox_ops,
-	.priv	= &omap2_mbox_iva_priv,
-};
-#endif
-
 static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
Signature · 1 line
1.7.1
author
#5 lore raw

PATCH v3 4/14 omap: mailbox: 2420 should be detected at run-time

In reply to Felipe Contreras
Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap2/mailbox.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/mailbox.c +2 -1
index edcfec6..f67cb2c 100644
@@ -455,7 +455,8 @@ err_dsp:
 static int __devexit omap2_mbox_remove(struct platform_device *pdev)
 {
 #if defined(CONFIG_ARCH_OMAP2420)
-	omap_mbox_unregister(&mbox_iva_info);
+	if (cpu_is_omap2420())
+		omap_mbox_unregister(&mbox_iva_info);
 #endif
 
 	if (cpu_is_omap44xx()) {
Signature · 1 line
1.7.1
author
#6 lore raw

PATCH v3 5/14 omap: mailbox: use correct config for omap1

In reply to Felipe Contreras

CONFIG_OMAP_DSP is not in mainline, CONFIG_OMAP_MBOX_FWK is.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/devices.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap1/devices.c +1 -1
index 462b59c..da796f2 100644
@@ -63,7 +63,7 @@ static void omap_init_rtc(void)
 static inline void omap_init_rtc(void) {}
 #endif
 
-#if defined(CONFIG_OMAP_DSP) || defined(CONFIG_OMAP_DSP_MODULE)
+#if defined(CONFIG_OMAP_MBOX_FWK) || defined(CONFIG_OMAP_MBOX_FWK_MODULE)
 
 #if defined(CONFIG_ARCH_OMAP15XX)
 #  define OMAP1_MBOX_SIZE	0x23
Signature · 1 line
1.7.1
author
#7 lore raw

PATCH v3 6/14 omap: mailbox: update omap1 probing

In reply to Felipe Contreras

Based on omap2 code.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)
arch/arm/mach-omap1/mailbox.c +14 -14
index 15bf2a2..211b9fc 100644
@@ -146,12 +146,7 @@ EXPORT_SYMBOL(mbox_dsp_info);
 static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
-
-	if (pdev->num_resources != 2) {
-		dev_err(&pdev->dev, "invalid number of resources: %d\n",
-			pdev->num_resources);
-		return -ENODEV;
-	}
+	int ret;
 
 	/* MBOX base */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -161,27 +156,32 @@ static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 	}
 
 	mbox_base = ioremap(res->start, resource_size(res));
-	if (!mbox_base) {
-		dev_err(&pdev->dev, "ioremap failed\n");
-		return -ENODEV;
-	}
+	if (!mbox_base)
+		return -ENOMEM;
 
 	/* DSP IRQ */
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (unlikely(!res)) {
 		dev_err(&pdev->dev, "invalid irq resource\n");
-		iounmap(mbox_base);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err_out;
 	}
 	mbox_dsp_info.irq = res->start;
 
-	return omap_mbox_register(&pdev->dev, &mbox_dsp_info);
+	ret = omap_mbox_register(&pdev->dev, &mbox_dsp_info);
+	if (ret)
+		goto err_out;
+	return 0;
+
+err_out:
+	iounmap(mbox_base);
+	return ret;
 }
 
 static int __devexit omap1_mbox_remove(struct platform_device *pdev)
 {
 	omap_mbox_unregister(&mbox_dsp_info);
-
+	iounmap(mbox_base);
 	return 0;
 }
Signature · 1 line
1.7.1
author
#8 lore raw

PATCH v3 7/14 omap: mailbox: don't export unecessary symbols

In reply to Felipe Contreras
Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c |    1 -
 arch/arm/mach-omap2/mailbox.c |    3 ---
 2 files changed, 0 insertions(+), 4 deletions(-)
arch/arm/mach-omap1/mailbox.c +0 -1
index 211b9fc..590ac66 100644
@@ -141,7 +141,6 @@ struct omap_mbox mbox_dsp_info = {
 	.ops	= &omap1_mbox_ops,
 	.priv	= &omap1_mbox_dsp_priv,
 };
-EXPORT_SYMBOL(mbox_dsp_info);
 
 static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 {
arch/arm/mach-omap2/mailbox.c +0 -3
index f67cb2c..3ff1ad5 100644
@@ -306,7 +306,6 @@ struct omap_mbox mbox_dsp_info = {
 	.ops	= &omap2_mbox_ops,
 	.priv	= &omap2_mbox_dsp_priv,
 };
-EXPORT_SYMBOL(mbox_dsp_info);
 
 #if defined(CONFIG_ARCH_OMAP2420)
 

@@ -356,7 +355,6 @@ struct omap_mbox mbox_1_info = {
 	.ops	= &omap2_mbox_ops,
 	.priv	= &omap2_mbox_1_priv,
 };
-EXPORT_SYMBOL(mbox_1_info);
 
 static struct omap_mbox2_priv omap2_mbox_2_priv = {
 	.tx_fifo = {

@@ -379,7 +377,6 @@ struct omap_mbox mbox_2_info = {
 	.ops	= &omap2_mbox_ops,
 	.priv	= &omap2_mbox_2_priv,
 };
-EXPORT_SYMBOL(mbox_2_info);
 
 static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {
Signature · 1 line
1.7.1
author
#9 lore raw

PATCH v3 8/14 omap: mailbox: remove unecessary fields

In reply to Felipe Contreras

Nobody is using them.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/plat-omap/include/plat/mailbox.h |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)
arch/arm/plat-omap/include/plat/mailbox.h +0 -8
index 0c3c4a5..aad8bf8 100644
@@ -52,19 +52,11 @@ struct omap_mbox_queue {
 struct omap_mbox {
 	char			*name;
 	unsigned int		irq;
-
 	struct omap_mbox_queue	*txq, *rxq;
-
 	struct omap_mbox_ops	*ops;
-
-	mbox_msg_t		seq_snd, seq_rcv;
-
 	struct device		*dev;
-
 	struct omap_mbox	*next;
 	void			*priv;
-
-	void			(*err_notify)(void);
 };
 
 int omap_mbox_msg_send(struct omap_mbox *, mbox_msg_t msg);
Signature · 1 line
1.7.1
author
#10 lore raw

PATCH v3 9/14 omap: mailbox: add IRQ names

In reply to Felipe Contreras

Will be useful to identify them later.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/devices.c |    1 +
 arch/arm/mach-omap2/devices.c |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)
arch/arm/mach-omap1/devices.c +1 -0
index da796f2..314fea3 100644
@@ -82,6 +82,7 @@ static struct resource mbox_resources[] = {
 	{
 		.start		= INT_DSP_MAILBOX1,
 		.flags		= IORESOURCE_IRQ,
+		.name		= "dsp",
 	},
 };
arch/arm/mach-omap2/devices.c +4 -0
index 2271b9b..b621476 100644
@@ -150,10 +150,12 @@ static struct resource omap2_mbox_resources[] = {
 	{
 		.start		= INT_24XX_MAIL_U0_MPU,
 		.flags		= IORESOURCE_IRQ,
+		.name		= "dsp",
 	},
 	{
 		.start		= INT_24XX_MAIL_U3_MPU,
 		.flags		= IORESOURCE_IRQ,
+		.name		= "iva",
 	},
 };
 static int omap2_mbox_resources_sz = ARRAY_SIZE(omap2_mbox_resources);

@@ -172,6 +174,7 @@ static struct resource omap3_mbox_resources[] = {
 	{
 		.start		= INT_24XX_MAIL_U0_MPU,
 		.flags		= IORESOURCE_IRQ,
+		.name		= "dsp",
 	},
 };
 static int omap3_mbox_resources_sz = ARRAY_SIZE(omap3_mbox_resources);

@@ -193,6 +196,7 @@ static struct resource omap4_mbox_resources[] = {
 	{
 		.start          = OMAP44XX_IRQ_MAIL_U0,
 		.flags          = IORESOURCE_IRQ,
+		.name		= "mbox",
 	},
 };
 static int omap4_mbox_resources_sz = ARRAY_SIZE(omap4_mbox_resources);
Signature · 1 line
1.7.1
author
#11 lore raw

PATCH v3 10/14 omap: mailbox: reorganize registering

In reply to Felipe Contreras

It's more extensible this way.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c |   41 +++++++++-------
 arch/arm/mach-omap2/mailbox.c |  106 +++++++++++++++++------------------------
 2 files changed, 66 insertions(+), 81 deletions(-)
arch/arm/mach-omap1/mailbox.c +22 -19
index 590ac66..38c21a2 100644
@@ -29,6 +29,8 @@
 
 static void __iomem *mbox_base;
 
+static struct omap_mbox **list;
+
 struct omap_mbox1_fifo {
 	unsigned long cmd;
 	unsigned long data;

@@ -142,44 +144,45 @@ struct omap_mbox mbox_dsp_info = {
 	.priv	= &omap1_mbox_dsp_priv,
 };
 
+struct omap_mbox *omap1_mboxes[] = { &mbox_dsp_info, NULL };
+
 static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	int ret;
+	int i;
 
-	/* MBOX base */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid mem resource\n");
-		return -ENODEV;
-	}
+	res = pdev->resource;
+
+	list = omap1_mboxes;
 
-	mbox_base = ioremap(res->start, resource_size(res));
+	list[0]->irq = platform_get_irq_byname(pdev, "dsp");
+
+	mbox_base = ioremap(res[0].start, resource_size(&res[0]));
 	if (!mbox_base)
 		return -ENOMEM;
 
-	/* DSP IRQ */
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid irq resource\n");
-		ret = -ENODEV;
-		goto err_out;
+	for (i = 0; list[i]; i++) {
+		ret = omap_mbox_register(&pdev->dev, list[i]);
+		if (ret)
+			goto err_out;
 	}
-	mbox_dsp_info.irq = res->start;
-
-	ret = omap_mbox_register(&pdev->dev, &mbox_dsp_info);
-	if (ret)
-		goto err_out;
 	return 0;
 
 err_out:
+	while (i--)
+		omap_mbox_unregister(list[i]);
 	iounmap(mbox_base);
 	return ret;
 }
 
 static int __devexit omap1_mbox_remove(struct platform_device *pdev)
 {
-	omap_mbox_unregister(&mbox_dsp_info);
+	int i;
+
+	for (i = 0; list[i]; i++)
+		omap_mbox_unregister(list[i]);
+
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/mach-omap2/mailbox.c +44 -62

151 lines not on this page · raw message

Signature · 1 line
1.7.1
#12 lore raw

PATCH v3 10/14 omap: mailbox: reorganize registering

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 10/14] omap: mailbox: reorganize registering
Date: Sat, 22 May 2010 19:14:21 +0200

arch/arm/mach-omap1/mailbox.c struct omap_mbox mbox_dsp_info = { · line 155

Quoted 6 lines

platform_get_resource(pdev, IORESOURCE_MEM, 0);

arch/arm/mach-omap1/mailbox.c +1 -4
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid mem resource\n");
-		return -ENODEV;
-	}
+	res = pdev->resource;

why not?

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
Quoted 33 lines
arch/arm/mach-omap1/mailbox.c +11 -12
+
+	list = omap1_mboxes;
 
-	mbox_base = ioremap(res->start, resource_size(res));
+	list[0]->irq = platform_get_irq_byname(pdev, "dsp");
+
+	mbox_base = ioremap(res[0].start, resource_size(&res[0]));
 	if (!mbox_base)
 		return -ENOMEM;
 
-	/* DSP IRQ */
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid irq resource\n");
-		ret = -ENODEV;
-		goto err_out;
+	for (i = 0; list[i]; i++) {
+		ret = omap_mbox_register(&pdev->dev, list[i]);
+		if (ret)
+			goto err_out;
 	}
-	mbox_dsp_info.irq = res->start;
-
-	ret = omap_mbox_register(&pdev->dev, &mbox_dsp_info);
-	if (ret)
-		goto err_out;
 	return 0;
 
 err_out:
+	while (i--)
+		omap_mbox_unregister(list[i]);
 	iounmap(mbox_base);
 	return ret;
#13 lore raw

PATCH v3 10/14 omap: mailbox: reorganize registering

Quoted 13 lines · Hiroshi DOYU

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 10/14] omap: mailbox: reorganize registering
Date: Sat, 22 May 2010 19:14:21 +0200

arch/arm/mach-omap1/mailbox.c struct omap_mbox mbox_dsp_info = { · line 155

Quoted 6 lines

platform_get_resource(pdev, IORESOURCE_MEM, 0);

arch/arm/mach-omap1/mailbox.c +1 -4
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid mem resource\n");
-		return -ENODEV;
-	}
+	res = pdev->resource;

why not?

Quoted 1 line
	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

I agree - always use the accessors provided rather than going beneath the covers and fiddling directly with stuff inside objects.

author
#14 lore raw

PATCH v3 10/14 omap: mailbox: reorganize registering

Quoted 12 lines · Russell King - ARM Linux
Quoted 11 lines · Hiroshi DOYU

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 10/14] omap: mailbox: reorganize registering
Date: Sat, 22 May 2010 19:14:21 +0200

Quoted 6 lines

platform_get_resource(pdev, IORESOURCE_MEM, 0);
- ? if (unlikely(!res)) {
- ? ? ? ? ? dev_err(&pdev->dev, "invalid mem resource\n");
- ? ? ? ? ? return -ENODEV;
- ? }
+ ? res = pdev->resource;

Quoted 6 lines
Quoted 3 lines

why not?

? ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

I agree - always use the accessors provided rather than going beneath the covers and fiddling directly with stuff inside objects.

Yes, this patch slipped from the previous series where resources were defined in the same file and some tricky assumptions were being made. Now platform_get_resource() makes more sense.

Signature · 1 line
Felipe Contreras
author
#15 lore raw

PATCH v3 11/14 omap: mailbox: only compile for configured archs

In reply to Felipe Contreras
Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap2/mailbox.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/mailbox.c +12 -2
index f55fa84..1f0c5cd 100644
@@ -286,6 +286,7 @@ static struct omap_mbox_ops omap2_mbox_ops = {
 
 /* FIXME: the following structs should be filled automatically by the user id */
 
+#if defined(CONFIG_ARCH_OMAP3430) || defined(CONFIG_ARCH_OMAP2420)
 /* DSP */
 static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
 	.tx_fifo = {

@@ -308,11 +309,13 @@ struct omap_mbox mbox_dsp_info = {
 	.ops	= &omap2_mbox_ops,
 	.priv	= &omap2_mbox_dsp_priv,
 };
+#endif
 
+#if defined(CONFIG_ARCH_OMAP3430)
 struct omap_mbox *omap3_mboxes[] = { &mbox_dsp_info, NULL };
+#endif
 
 #if defined(CONFIG_ARCH_OMAP2420)
-
 /* IVA */
 static struct omap_mbox2_priv omap2_mbox_iva_priv = {
 	.tx_fifo = {

@@ -339,6 +342,7 @@ static struct omap_mbox mbox_iva_info = {
 struct omap_mbox *omap2_mboxes[] = { &mbox_iva_info, &mbox_dsp_info, NULL };
 #endif
 
+#if defined(CONFIG_ARCH_OMAP4)
 /* OMAP4 */
 static struct omap_mbox2_priv omap2_mbox_1_priv = {
 	.tx_fifo = {

@@ -385,6 +389,7 @@ struct omap_mbox mbox_2_info = {
 };
 
 struct omap_mbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info, NULL };
+#endif
 
 static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {

@@ -394,11 +399,14 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 
 	res = pdev->resource;
 
-	if (cpu_is_omap3430()) {
+	if (false);
+#if defined(CONFIG_ARCH_OMAP3430)
+	else if (cpu_is_omap3430()) {
 		list = omap3_mboxes;
 
 		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 	}
+#endif
 #if defined(CONFIG_ARCH_OMAP2420)
 	else if (cpu_is_omap2420()) {
 		list = omap2_mboxes;

@@ -407,12 +415,14 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 		list[1]->irq = platform_get_irq_byname(pdev, "iva");
 	}
 #endif
+#if defined(CONFIG_ARCH_OMAP4)
 	else if (cpu_is_omap44xx()) {
 		list = omap4_mboxes;
 
 		list[0]->irq = list[1]->irq =
 			platform_get_irq_byname(pdev, "mbox");
 	}
+#endif
 	else {
 		pr_err("%s: platform not supported\n", __func__);
 		return -ENODEV;
Signature · 1 line
1.7.1
#16 lore raw

PATCH v3 11/14 omap: mailbox: only compile for configured archs

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 11/14] omap: mailbox: only compile for configured archs
Date: Sat, 22 May 2010 19:14:22 +0200

Quoted 49 lines
Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap2/mailbox.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/mailbox.c +6 -1
index f55fa84..1f0c5cd 100644
@@ -286,6 +286,7 @@ static struct omap_mbox_ops omap2_mbox_ops = {
 
 /* FIXME: the following structs should be filled automatically by the user id */
 
+#if defined(CONFIG_ARCH_OMAP3430) || defined(CONFIG_ARCH_OMAP2420)
 /* DSP */
 static struct omap_mbox2_priv omap2_mbox_dsp_priv = {
 	.tx_fifo = {

@@ -308,11 +309,13 @@ struct omap_mbox mbox_dsp_info = {
 	.ops	= &omap2_mbox_ops,
 	.priv	= &omap2_mbox_dsp_priv,
 };
+#endif
 
+#if defined(CONFIG_ARCH_OMAP3430)
 struct omap_mbox *omap3_mboxes[] = { &mbox_dsp_info, NULL };
+#endif
 
 #if defined(CONFIG_ARCH_OMAP2420)
-
 /* IVA */
 static struct omap_mbox2_priv omap2_mbox_iva_priv = {
 	.tx_fifo = {

@@ -339,6 +342,7 @@ static struct omap_mbox mbox_iva_info = {
 struct omap_mbox *omap2_mboxes[] = { &mbox_iva_info, &mbox_dsp_info, NULL };
 #endif
 
+#if defined(CONFIG_ARCH_OMAP4)
 /* OMAP4 */
 static struct omap_mbox2_priv omap2_mbox_1_priv = {
 	.tx_fifo = {

@@ -385,6 +389,7 @@ struct omap_mbox mbox_2_info = {
 };
 
 struct omap_mbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info, NULL };
+#endif
 
 static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {

arch/arm/mach-omap2/mailbox.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 402

Quoted 5 lines
arch/arm/mach-omap2/mailbox.c +1 -1
@@ -394,11 +399,14 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 
 	res = pdev->resource;
 
-	if (cpu_is_omap3430()) {
+	if (false);

nitpcik:
The above may look better as below:

	if (false)
		;

"checkpatch.pl" also doesn't complain.

Quoted 11 lines
arch/arm/mach-omap2/mailbox.c +3 -0
+#if defined(CONFIG_ARCH_OMAP3430)
+	else if (cpu_is_omap3430()) {
 		list = omap3_mboxes;
 
 		list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 	}
+#endif
 #if defined(CONFIG_ARCH_OMAP2420)
 	else if (cpu_is_omap2420()) {
 		list = omap2_mboxes;

arch/arm/mach-omap2/mailbox.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 423

Quoted 9 lines
arch/arm/mach-omap2/mailbox.c +1 -0
@@ -407,12 +415,14 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 		list[1]->irq = platform_get_irq_byname(pdev, "iva");
 	}
 #endif
+#if defined(CONFIG_ARCH_OMAP4)
 	else if (cpu_is_omap44xx()) {
 		list = omap4_mboxes;
 
 		list[0]->irq = list[1]->irq =
 			platform_get_irq_byname(pdev, "mbox");

For checkpatch.pl,

  		list[0]->irq =
  			platform_get_irq_byname(pdev, "mbox");
  		list[1]->irq = list[0]->irq;
Quoted 8 lines
arch/arm/mach-omap2/mailbox.c +1 -0
 	}
+#endif
 	else {
 		pr_err("%s: platform not supported\n", __func__);
 		return -ENODEV;
Signature · 1 line
1.7.1
author
#17 lore raw

PATCH v3 11/14 omap: mailbox: only compile for configured archs

Quoted 11 lines · Hiroshi DOYU

From: ext Felipe Contreras <[email protected]>

Quoted 1 line

+ if (false);

nitpcik:
The above may look better as below:

? ? ? ?if (false)
? ? ? ? ? ? ? ?;

"checkpatch.pl" also doesn't complain.

Personally I think it looks weird and it's a checkpatch bug, but fine by me.

Quoted 18 lines
Quoted 18 lines

+#if defined(CONFIG_ARCH_OMAP3430)
+ ? ? else if (cpu_is_omap3430()) {
? ? ? ? ? ? ? list = omap3_mboxes;

? ? ? ? ? ? ? list[0]->irq = platform_get_irq_byname(pdev, "dsp"); ? ? ? }
+#endif
?#if defined(CONFIG_ARCH_OMAP2420)
? ? ? else if (cpu_is_omap2420()) {
? ? ? ? ? ? ? list = omap2_mboxes;

(quoted diff) +0 -0
@@ -407,12 +415,14 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 ? ? ? ? ? ? ? list[1]->irq = platform_get_irq_byname(pdev, "iva");
 ? ? ? }

?#endif
+#if defined(CONFIG_ARCH_OMAP4)
? ? ? else if (cpu_is_omap44xx()) {
? ? ? ? ? ? ? list = omap4_mboxes;

Quoted 8 lines
Quoted 2 lines

? ? ? ? ? ? ? list[0]->irq = list[1]->irq =
? ? ? ? ? ? ? ? ? ? ? platform_get_irq_byname(pdev, "mbox");

For checkpatch.pl,

? ? ? ? ? ? ? ?list[0]->irq =
? ? ? ? ? ? ? ? ? ? ? ?platform_get_irq_byname(pdev, "mbox"); ? ? ? ? ? ? ? ?list[1]->irq = list[0]->irq;

I don't see checkpatch (0.30) complaining about that.

Cheers.

Signature · 1 line
Felipe Contreras
#18 lore raw

PATCH v3 11/14 omap: mailbox: only compile for configured archs

Quoted 8 lines · Felipe Contreras
Quoted 7 lines · Hiroshi DOYU

From: ext Felipe Contreras <[email protected]>

Quoted 1 line

+ if (false);

nitpcik:
The above may look better as below:

Quoted 6 lines
Quoted 4 lines

? ? ? ?if (false)
? ? ? ? ? ? ? ?;

"checkpatch.pl" also doesn't complain.

Personally I think it looks weird and it's a checkpatch bug, but fine by me.

No, I'd say it's intentional.

	if (false);

looks too much like an error - think about:

	if (false);
		do_something();

which would be very easy to miss while reviewing.

author
#19 lore raw

PATCH v3 12/14 omap: mailbox: standarize on 'omap-mailbox'

In reply to Felipe Contreras

omap{1,2}-mailbox are modules that provide the 'omap-mailbox' driver.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/devices.c |    2 +-
 arch/arm/mach-omap1/mailbox.c |    4 ++--
 arch/arm/mach-omap2/devices.c |    2 +-
 arch/arm/mach-omap2/mailbox.c |    6 ++----
 4 files changed, 6 insertions(+), 8 deletions(-)
arch/arm/mach-omap1/devices.c +1 -1
index 314fea3..16e43e5 100644
@@ -87,7 +87,7 @@ static struct resource mbox_resources[] = {
 };
 
 static struct platform_device mbox_device = {
-	.name		= "omap1-mailbox",
+	.name		= "omap-mailbox",
 	.id		= -1,
 	.num_resources	= ARRAY_SIZE(mbox_resources),
 	.resource	= mbox_resources,
arch/arm/mach-omap1/mailbox.c +2 -2
index 38c21a2..4210896 100644
@@ -1,5 +1,5 @@
 /*
- * Mailbox reservation modules for DSP
+ * Mailbox reservation modules for OMAP1
  *
  * Copyright (C) 2006-2009 Nokia Corporation
  * Written by: Hiroshi DOYU <[email protected]>

@@ -191,7 +191,7 @@ static struct platform_driver omap1_mbox_driver = {
 	.probe	= omap1_mbox_probe,
 	.remove	= __devexit_p(omap1_mbox_remove),
 	.driver	= {
-		.name	= "omap1-mailbox",
+		.name	= "omap-mailbox",
 	},
 };
arch/arm/mach-omap2/devices.c +1 -1
index b621476..d9ac853 100644
@@ -206,7 +206,7 @@ static int omap4_mbox_resources_sz = ARRAY_SIZE(omap4_mbox_resources);
 #endif
 
 static struct platform_device mbox_device = {
-	.name		= "omap2-mailbox",
+	.name		= "omap-mailbox",
 	.id		= -1,
 };
arch/arm/mach-omap2/mailbox.c +2 -4
index 1f0c5cd..a433ca7 100644
@@ -18,8 +18,6 @@
 #include <plat/mailbox.h>
 #include <mach/irqs.h>
 
-#define DRV_NAME "omap2-mailbox"
-
 #define MAILBOX_REVISION		0x000
 #define MAILBOX_SYSCONFIG		0x010
 #define MAILBOX_SYSSTATUS		0x014

@@ -461,7 +459,7 @@ static struct platform_driver omap2_mbox_driver = {
 	.probe = omap2_mbox_probe,
 	.remove = __devexit_p(omap2_mbox_remove),
 	.driver = {
-		.name = DRV_NAME,
+		.name = "omap-mailbox",
 	},
 };
 

@@ -482,4 +480,4 @@ MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("omap mailbox: omap2/3/4 architecture specific functions");
 MODULE_AUTHOR("Hiroshi DOYU <[email protected]>");
 MODULE_AUTHOR("Paul Mundt");
-MODULE_ALIAS("platform:"DRV_NAME);
+MODULE_ALIAS("platform:omap2-mailbox");
Signature · 1 line
1.7.1
author
#20 lore raw

PATCH v3 13/14 omap: mailbox: simplify omap_mbox_register()

In reply to Felipe Contreras

No need to dynamically register mailboxes one by one.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c             |   25 ++------
 arch/arm/mach-omap2/mailbox.c             |   22 ++-----
 arch/arm/plat-omap/include/plat/mailbox.h |    5 +-
 arch/arm/plat-omap/mailbox.c              |   95 +++++++++++------------------
 4 files changed, 50 insertions(+), 97 deletions(-)
arch/arm/mach-omap1/mailbox.c +7 -18
index 4210896..e50b3c2 100644
@@ -29,8 +29,6 @@
 
 static void __iomem *mbox_base;
 
-static struct omap_mbox **list;
-
 struct omap_mbox1_fifo {
 	unsigned long cmd;
 	unsigned long data;

@@ -151,38 +149,29 @@ static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 	int i;
+	struct omap_mbox **list;
 
 	res = pdev->resource;
 
 	list = omap1_mboxes;
-
 	list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 
 	mbox_base = ioremap(res[0].start, resource_size(&res[0]));
 	if (!mbox_base)
 		return -ENOMEM;
 
-	for (i = 0; list[i]; i++) {
-		ret = omap_mbox_register(&pdev->dev, list[i]);
-		if (ret)
-			goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);
+		return ret;
 	}
-	return 0;
 
-err_out:
-	while (i--)
-		omap_mbox_unregister(list[i]);
-	iounmap(mbox_base);
-	return ret;
+	return 0;
 }
 
 static int __devexit omap1_mbox_remove(struct platform_device *pdev)
 {
-	int i;
-
-	for (i = 0; list[i]; i++)
-		omap_mbox_unregister(list[i]);
-
+	omap_mbox_unregister();
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/mach-omap2/mailbox.c +6 -16
index a433ca7..66d366d 100644
@@ -54,8 +54,6 @@
 
 static void __iomem *mbox_base;
 
-static struct omap_mbox **list;
-
 struct omap_mbox2_fifo {
 	unsigned long msg;
 	unsigned long fifo_stat;

@@ -393,7 +391,7 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	int ret;
-	int i;
+	struct omap_mbox **list;
 
 	res = pdev->resource;
 

@@ -430,27 +428,19 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 	if (!mbox_base)
 		return -ENOMEM;
 
-	for (i = 0; list[i]; i++) {
-		ret = omap_mbox_register(&pdev->dev, list[i]);
-		if (ret)
-			goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);
+		return ret;
 	}
 	return 0;
 
-err_out:
-	while (i--)
-		omap_mbox_unregister(list[i]);
-	iounmap(mbox_base);
 	return ret;
 }
 
 static int __devexit omap2_mbox_remove(struct platform_device *pdev)
 {
-	int i;
-
-	for (i = 0; list[i]; i++)
-		omap_mbox_unregister(list[i]);
-
+	omap_mbox_unregister();
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/plat-omap/include/plat/mailbox.h +2 -3
index aad8bf8..c44fde3 100644
@@ -55,7 +55,6 @@ struct omap_mbox {
 	struct omap_mbox_queue	*txq, *rxq;
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
-	struct omap_mbox	*next;
 	void			*priv;
 };
 

@@ -65,8 +64,8 @@ void omap_mbox_init_seq(struct omap_mbox *);
 struct omap_mbox *omap_mbox_get(const char *);
 void omap_mbox_put(struct omap_mbox *);
 
-int omap_mbox_register(struct device *parent, struct omap_mbox *);
-int omap_mbox_unregister(struct omap_mbox *);
+int omap_mbox_register(struct device *parent, struct omap_mbox **);
+int omap_mbox_unregister(void);
 
 static inline void omap_mbox_save_ctx(struct omap_mbox *mbox)
 {
arch/arm/plat-omap/mailbox.c +35 -60

131 lines not on this page · raw message

Signature · 1 line
1.7.1
#21 lore raw

PATCH v3 13/14 omap: mailbox: simplify omap_mbox_register()

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 13/14] omap: mailbox: simplify omap_mbox_register()
Date: Sat, 22 May 2010 19:14:24 +0200

Quoted 1 line

No need to dynamically register mailboxes one by one.

Can you squash this into [PATCH 10/14]?

I think that the "struct omap_mbox **list" would make more sense without those reworking of modifications in this patch.

Quoted 287 lines
Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c             |   25 ++------
 arch/arm/mach-omap2/mailbox.c             |   22 ++-----
 arch/arm/plat-omap/include/plat/mailbox.h |    5 +-
 arch/arm/plat-omap/mailbox.c              |   95 +++++++++++------------------
 4 files changed, 50 insertions(+), 97 deletions(-)
arch/arm/mach-omap1/mailbox.c +7 -18
index 4210896..e50b3c2 100644
@@ -29,8 +29,6 @@
 
 static void __iomem *mbox_base;
 
-static struct omap_mbox **list;
-
 struct omap_mbox1_fifo {
 	unsigned long cmd;
 	unsigned long data;

@@ -151,38 +149,29 @@ static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 	int i;
+	struct omap_mbox **list;
 
 	res = pdev->resource;
 
 	list = omap1_mboxes;
-
 	list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 
 	mbox_base = ioremap(res[0].start, resource_size(&res[0]));
 	if (!mbox_base)
 		return -ENOMEM;
 
-	for (i = 0; list[i]; i++) {
-		ret = omap_mbox_register(&pdev->dev, list[i]);
-		if (ret)
-			goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);
+		return ret;
 	}
-	return 0;
 
-err_out:
-	while (i--)
-		omap_mbox_unregister(list[i]);
-	iounmap(mbox_base);
-	return ret;
+	return 0;
 }
 
 static int __devexit omap1_mbox_remove(struct platform_device *pdev)
 {
-	int i;
-
-	for (i = 0; list[i]; i++)
-		omap_mbox_unregister(list[i]);
-
+	omap_mbox_unregister();
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/mach-omap2/mailbox.c +6 -16
index a433ca7..66d366d 100644
@@ -54,8 +54,6 @@
 
 static void __iomem *mbox_base;
 
-static struct omap_mbox **list;
-
 struct omap_mbox2_fifo {
 	unsigned long msg;
 	unsigned long fifo_stat;

@@ -393,7 +391,7 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	int ret;
-	int i;
+	struct omap_mbox **list;
 
 	res = pdev->resource;
 

@@ -430,27 +428,19 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 	if (!mbox_base)
 		return -ENOMEM;
 
-	for (i = 0; list[i]; i++) {
-		ret = omap_mbox_register(&pdev->dev, list[i]);
-		if (ret)
-			goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);
+		return ret;
 	}
 	return 0;
 
-err_out:
-	while (i--)
-		omap_mbox_unregister(list[i]);
-	iounmap(mbox_base);
 	return ret;
 }
 
 static int __devexit omap2_mbox_remove(struct platform_device *pdev)
 {
-	int i;
-
-	for (i = 0; list[i]; i++)
-		omap_mbox_unregister(list[i]);
-
+	omap_mbox_unregister();
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/plat-omap/include/plat/mailbox.h +2 -3
index aad8bf8..c44fde3 100644
@@ -55,7 +55,6 @@ struct omap_mbox {
 	struct omap_mbox_queue	*txq, *rxq;
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
-	struct omap_mbox	*next;
 	void			*priv;
 };
 

@@ -65,8 +64,8 @@ void omap_mbox_init_seq(struct omap_mbox *);
 struct omap_mbox *omap_mbox_get(const char *);
 void omap_mbox_put(struct omap_mbox *);
 
-int omap_mbox_register(struct device *parent, struct omap_mbox *);
-int omap_mbox_unregister(struct omap_mbox *);
+int omap_mbox_register(struct device *parent, struct omap_mbox **);
+int omap_mbox_unregister(void);
 
 static inline void omap_mbox_save_ctx(struct omap_mbox *mbox)
 {
arch/arm/plat-omap/mailbox.c +35 -60
index 38a6cb1..a8e22e1 100644
@@ -33,8 +33,7 @@
 #include <plat/mailbox.h>
 
 static struct workqueue_struct *mboxd;
-static struct omap_mbox *mboxes;
-static DEFINE_SPINLOCK(mboxes_lock);
+static struct omap_mbox **mboxes;
 static bool rq_full;
 
 static int mbox_configured;

@@ -307,31 +306,20 @@ static void omap_mbox_fini(struct omap_mbox *mbox)
 	}
 }
 
-static struct omap_mbox **find_mboxes(const char *name)
-{
-	struct omap_mbox **p;
-
-	for (p = &mboxes; *p; p = &(*p)->next) {
-		if (strcmp((*p)->name, name) == 0)
-			break;
-	}
-
-	return p;
-}
-
 struct omap_mbox *omap_mbox_get(const char *name)
 {
 	struct omap_mbox *mbox;
 	int ret;
 
-	spin_lock(&mboxes_lock);
-	mbox = *(find_mboxes(name));
-	if (mbox == NULL) {
-		spin_unlock(&mboxes_lock);
-		return ERR_PTR(-ENOENT);
-	}
+	if (!mboxes)
+		return ERR_PTR(-EINVAL);
 
-	spin_unlock(&mboxes_lock);
+	for (mbox = *mboxes; mbox; mbox++)
+		if (!strcmp(mbox->name, name))
+			break;
+
+	if (!mbox)
+		return ERR_PTR(-ENOENT);
 
 	ret = omap_mbox_startup(mbox);
 	if (ret)

@@ -349,57 +337,44 @@ EXPORT_SYMBOL(omap_mbox_put);
 
 static struct class omap_mbox_class = { .name = "mbox", };
 
-int omap_mbox_register(struct device *parent, struct omap_mbox *mbox)
+int omap_mbox_register(struct device *parent, struct omap_mbox **list)
 {
-	int ret = 0;
-	struct omap_mbox **tmp;
+	int ret;
+	int i;
 
-	if (!mbox)
+	mboxes = list;
+	if (!mboxes)
 		return -EINVAL;
-	if (mbox->next)
-		return -EBUSY;
-
-	mbox->dev = device_create(&omap_mbox_class,
-				  parent, 0, mbox, "%s", mbox->name);
-	if (IS_ERR(mbox->dev))
-		return PTR_ERR(mbox->dev);
-
-	spin_lock(&mboxes_lock);
-	tmp = find_mboxes(mbox->name);
-	if (*tmp) {
-		ret = -EBUSY;
-		spin_unlock(&mboxes_lock);
-		goto err_find;
-	}
-	*tmp = mbox;
-	spin_unlock(&mboxes_lock);
 
+	for (i = 0; mboxes[i]; i++) {
+		struct omap_mbox *mbox = mboxes[i];
+		mbox->dev = device_create(&omap_mbox_class,
+				parent, 0, mbox, "%s", mbox->name);
+		if (IS_ERR(mbox->dev)) {
+			ret = PTR_ERR(mbox->dev);
+			goto err_out;
+		}
+	}
 	return 0;
 
-err_find:
+err_out:
+	while (i--)
+		device_unregister(mboxes[i]->dev);
 	return ret;
 }
 EXPORT_SYMBOL(omap_mbox_register);
 
-int omap_mbox_unregister(struct omap_mbox *mbox)
+int omap_mbox_unregister(void)
 {
-	struct omap_mbox **tmp;
-
-	spin_lock(&mboxes_lock);
-	tmp = &mboxes;
-	while (*tmp) {
-		if (mbox == *tmp) {
-			*tmp = mbox->next;
-			mbox->next = NULL;
-			spin_unlock(&mboxes_lock);
-			device_unregister(mbox->dev);
-			return 0;
-		}
-		tmp = &(*tmp)->next;
-	}
-	spin_unlock(&mboxes_lock);
+	int i;
 
-	return -EINVAL;
+	if (!mboxes)
+		return -EINVAL;
+
+	for (i = 0; mboxes[i]; i++)
+		device_unregister(mboxes[i]->dev);
+	mboxes = NULL;
+	return 0;
 }
 EXPORT_SYMBOL(omap_mbox_unregister);
Signature · 1 line
1.7.1
author
#22 lore raw

PATCH v3 13/14 omap: mailbox: simplify omap_mbox_register()

Quoted 10 lines · Hiroshi DOYU

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 13/14] omap: mailbox: simplify omap_mbox_register()
Date: Sat, 22 May 2010 19:14:24 +0200

Quoted 1 line

No need to dynamically register mailboxes one by one.

Can you squash this into [PATCH 10/14]?

I think that the "struct omap_mbox **list" would make more sense without those reworking of modifications in this patch.

I don't know, I'm looking at the patch and I think it's much more difficult to figure out what's happening.

See:

arch/arm/mach-omap1/mailbox.c +13 -22
index 590ac66..3df1c28 100644
@@ -142,44 +142,35 @@ struct omap_mbox mbox_dsp_info = {
 	.priv	= &omap1_mbox_dsp_priv,
 };
 
+struct omap_mbox *omap1_mboxes[] = { &mbox_dsp_info, NULL };
+
 static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	int ret;
+	int i;
+	struct omap_mbox **list;
 
-	/* MBOX base */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid mem resource\n");
-		return -ENODEV;
-	}
+	list = omap1_mboxes;
+	list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 
-	mbox_base = ioremap(res->start, resource_size(res));
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mbox_base = ioremap(res[0].start, resource_size(&res[0]));
 	if (!mbox_base)
 		return -ENOMEM;
 
-	/* DSP IRQ */
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (unlikely(!res)) {
-		dev_err(&pdev->dev, "invalid irq resource\n");
-		ret = -ENODEV;
-		goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);
+		return ret;
 	}
-	mbox_dsp_info.irq = res->start;
 
-	ret = omap_mbox_register(&pdev->dev, &mbox_dsp_info);
-	if (ret)
-		goto err_out;
 	return 0;
-
-err_out:
-	iounmap(mbox_base);
-	return ret;
 }
 
 static int __devexit omap1_mbox_remove(struct platform_device *pdev)
 {
-	omap_mbox_unregister(&mbox_dsp_info);
+	omap_mbox_unregister();
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/mach-omap2/mailbox.c +35 -64

136 lines not on this page · raw message

(quoted diff) +0 -0

b/arch/arm/plat-omap/include/plat/mailbox.h
index aad8bf8..c44fde3 100644

arch/arm/plat-omap/include/plat/mailbox.h +2 -3
@@ -55,7 +55,6 @@ struct omap_mbox {
 	struct omap_mbox_queue	*txq, *rxq;
 	struct omap_mbox_ops	*ops;
 	struct device		*dev;
-	struct omap_mbox	*next;
 	void			*priv;
 };
 

@@ -65,8 +64,8 @@ void omap_mbox_init_seq(struct omap_mbox *);
 struct omap_mbox *omap_mbox_get(const char *);
 void omap_mbox_put(struct omap_mbox *);
 
-int omap_mbox_register(struct device *parent, struct omap_mbox *);
-int omap_mbox_unregister(struct omap_mbox *);
+int omap_mbox_register(struct device *parent, struct omap_mbox **);
+int omap_mbox_unregister(void);
 
 static inline void omap_mbox_save_ctx(struct omap_mbox *mbox)
 {
arch/arm/plat-omap/mailbox.c +35 -60

131 lines not on this page · raw message

Signature · 1 line
Felipe Contreras
author
#23 lore raw

PATCH v3 13/14 omap: mailbox: simplify omap_mbox_register()

Quoted 7 lines · Felipe Contreras
Quoted 6 lines · Hiroshi DOYU

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 13/14] omap: mailbox: simplify omap_mbox_register()
Date: Sat, 22 May 2010 19:14:24 +0200

Quoted 1 line

No need to dynamically register mailboxes one by one.

Quoted 7 lines
Quoted 4 lines

Can you squash this into [PATCH 10/14]?

I think that the "struct omap_mbox **list" would make more sense without those reworking of modifications in this patch.

I don't know, I'm looking at the patch and I think it's much more difficult to figure out what's happening.

I forgot to mention: what I will do is put one patch after the other.

Signature · 1 line
Felipe Contreras
#24 lore raw

PATCH v3 13/14 omap: mailbox: simplify omap_mbox_register()

In reply to Felipe Contreras

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 13/14] omap: mailbox: simplify omap_mbox_register()
Date: Sat, 22 May 2010 19:14:24 +0200

Quoted 102 lines

No need to dynamically register mailboxes one by one.

Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c             |   25 ++------
 arch/arm/mach-omap2/mailbox.c             |   22 ++-----
 arch/arm/plat-omap/include/plat/mailbox.h |    5 +-
 arch/arm/plat-omap/mailbox.c              |   95 +++++++++++------------------
 4 files changed, 50 insertions(+), 97 deletions(-)
arch/arm/mach-omap1/mailbox.c +7 -18
index 4210896..e50b3c2 100644
@@ -29,8 +29,6 @@
 
 static void __iomem *mbox_base;
 
-static struct omap_mbox **list;
-
 struct omap_mbox1_fifo {
 	unsigned long cmd;
 	unsigned long data;

@@ -151,38 +149,29 @@ static int __devinit omap1_mbox_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 	int i;
+	struct omap_mbox **list;
 
 	res = pdev->resource;
 
 	list = omap1_mboxes;
-
 	list[0]->irq = platform_get_irq_byname(pdev, "dsp");
 
 	mbox_base = ioremap(res[0].start, resource_size(&res[0]));
 	if (!mbox_base)
 		return -ENOMEM;
 
-	for (i = 0; list[i]; i++) {
-		ret = omap_mbox_register(&pdev->dev, list[i]);
-		if (ret)
-			goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);
+		return ret;
 	}
-	return 0;
 
-err_out:
-	while (i--)
-		omap_mbox_unregister(list[i]);
-	iounmap(mbox_base);
-	return ret;
+	return 0;
 }
 
 static int __devexit omap1_mbox_remove(struct platform_device *pdev)
 {
-	int i;
-
-	for (i = 0; list[i]; i++)
-		omap_mbox_unregister(list[i]);
-
+	omap_mbox_unregister();
 	iounmap(mbox_base);
 	return 0;
 }
arch/arm/mach-omap2/mailbox.c +4 -7
index a433ca7..66d366d 100644
@@ -54,8 +54,6 @@
 
 static void __iomem *mbox_base;
 
-static struct omap_mbox **list;
-
 struct omap_mbox2_fifo {
 	unsigned long msg;
 	unsigned long fifo_stat;

@@ -393,7 +391,7 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	int ret;
-	int i;
+	struct omap_mbox **list;
 
 	res = pdev->resource;
 

@@ -430,27 +428,19 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 	if (!mbox_base)
 		return -ENOMEM;
 
-	for (i = 0; list[i]; i++) {
-		ret = omap_mbox_register(&pdev->dev, list[i]);
-		if (ret)
-			goto err_out;
+	ret = omap_mbox_register(&pdev->dev, list);
+	if (ret) {
+		iounmap(mbox_base);

arch/arm/mach-omap2/mailbox.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 439

Quoted 10 lines
arch/arm/mach-omap2/mailbox.c +1 -4
+		return ret;
 	}
 	return 0;
 
-err_out:
-	while (i--)
-		omap_mbox_unregister(list[i]);
-	iounmap(mbox_base);
 	return ret;
 }

The above result in:

    ....
	ret = omap_mbox_register(&pdev->dev, list);
	if (ret) {
		iounmap(mbox_base);
		return ret;
	}
	return 0;

	return ret;
}
author
#25 lore raw

PATCH v3 14/14 omap: mailbox: reorganize headers

In reply to Felipe Contreras
Signed-off-by Felipe Contreras <[email protected]>
---
 arch/arm/mach-omap1/mailbox.c             |    3 ---
 arch/arm/mach-omap2/mailbox.c             |    1 -
 arch/arm/plat-omap/include/plat/mailbox.h |    3 ++-
 arch/arm/plat-omap/mailbox.c              |    5 ++---
 4 files changed, 4 insertions(+), 8 deletions(-)
arch/arm/mach-omap1/mailbox.c +0 -3
index e50b3c2..fdd6cc9 100644
@@ -9,13 +9,10 @@
  * for more details.
  */
 
-#include <linux/kernel.h>
-#include <linux/resource.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <plat/mailbox.h>
-#include <mach/irqs.h>
 
 #define MAILBOX_ARM2DSP1		0x00
 #define MAILBOX_ARM2DSP1b		0x04
arch/arm/mach-omap2/mailbox.c +0 -1
index 66d366d..d46e439 100644
@@ -10,7 +10,6 @@
  * for more details.
  */
 
-#include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/platform_device.h>
arch/arm/plat-omap/include/plat/mailbox.h +2 -1
index c44fde3..9976565 100644
@@ -3,9 +3,10 @@
 #ifndef MAILBOX_H
 #define MAILBOX_H
 
-#include <linux/wait.h>
+#include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/interrupt.h>
+#include <linux/device.h>
 #include <linux/kfifo.h>
 
 typedef u32 mbox_msg_t;
arch/arm/plat-omap/mailbox.c +2 -3
index a8e22e1..8d4c7be 100644
@@ -21,10 +21,9 @@
  *
  */
 
-#include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/interrupt.h>
-#include <linux/device.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/kfifo.h>
Signature · 1 line
1.7.1
#26 lore raw

PATCH v3 14/14 omap: mailbox: reorganize headers

Hi,

Quoted 61 lines · ext Felipe Contreras
Signed-off-by Felipe Contreras <[email protected]>

---

arch/arm/mach-omap1/mailbox.c             |    3 ---
arch/arm/mach-omap2/mailbox.c             |    1 -
arch/arm/plat-omap/include/plat/mailbox.h |    3 ++-
arch/arm/plat-omap/mailbox.c              |    5 ++---
4 files changed, 4 insertions(+), 8 deletions(-)
arch/arm/mach-omap1/mailbox.c +0 -3
index e50b3c2..fdd6cc9 100644
@@ -9,13 +9,10 @@
 * for more details.
 */
 
-#include <linux/kernel.h>
-#include <linux/resource.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <plat/mailbox.h>
-#include <mach/irqs.h>
 
 #define MAILBOX_ARM2DSP1		0x00
 #define MAILBOX_ARM2DSP1b		0x04
arch/arm/mach-omap2/mailbox.c +0 -1
index 66d366d..d46e439 100644
@@ -10,7 +10,6 @@
 * for more details.
 */
 
-#include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/platform_device.h>
arch/arm/plat-omap/include/plat/mailbox.h +2 -1
index c44fde3..9976565 100644
@@ -3,9 +3,10 @@
 #ifndef MAILBOX_H
 #define MAILBOX_H
 
-#include <linux/wait.h>
+#include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/interrupt.h>
+#include <linux/device.h>
 #include <linux/kfifo.h>
 
 typedef u32 mbox_msg_t;
arch/arm/plat-omap/mailbox.c +0 -0
index a8e22e1..8d4c7be 100644
@@ -21,10 +21,9 @@
 *
 */

arch/arm/plat-omap/mailbox.c line 29

Quoted 10 lines
arch/arm/plat-omap/mailbox.c +2 -3
 
-#include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/interrupt.h>
-#include <linux/device.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/kfifo.h>

personally I don't like this patch since it makes the C-sources rely on indirect header inclusion by another header. That's really error prone and will cause build failures if someone decides to remove the include <linux/device.h> from plat/mailbox.h.

Those headers are safe against multiple inclusions and I think they should be included directly in all source files (header or not) that need any symbols from it.

Signature · 3 lines
balbi

DefectiveByDesign.org
author
#27 lore raw

PATCH v3 14/14 omap: mailbox: reorganize headers

Quoted 4 lines · Felipe Balbi

personally I don't like this patch since it makes the C-sources rely on indirect header inclusion by another header. That's really error prone and will cause build failures if someone decides to remove the include <linux/device.h> from plat/mailbox.h.

First of all, you cannot expect each and every source file to include each and every header it's using directly. It would be nice, but it's not happening now, and probably will not happen ever.

Now, with a few exceptions, I didn't assume somebody else would be loading anything, in fact, the patch is mostly _adding_ headers that were missing.

If you take a look at the modified files regarding kernel.h and module.h:

* arch/arm/mach-omap1/mailbox.c:
Not really using kernel.h, missing module.h

* arch/arm/mach-omap2/mailbox.c:
Missing module.h

* arch/arm/plat-omap/mailbox.c
Missing kernel.h

So it was clear to me that these were not carefully added, and probably were redundant, so I choose to remove them completely. If you have something against that, I can add them where it makes sense but the current patch is not really making the situation much worst.

Now, regarding device.h:

* arch/arm/mach-omap2/mailbox.c
Using it, but missing

* arch/arm/plat-omap/include/plat/mailbox.h Was using it, but a 'struct device' would suffice

* arch/arm/plat-omap/mailbox.c
Using it correctly

Now, I am pretty sure that platform_device.h will always depend on device.h that's why I removed them, but I can add them again if somebody wants to, but again, the situation is already bad.

Signature · 1 line
Felipe Contreras
#28 lore raw

PATCH v3 14/14 omap: mailbox: reorganize headers

Hi,

Quoted 1 line · ext Felipe Contreras

Now, regarding device.h:

which was my only concern

Quoted 3 lines

Now, I am pretty sure that platform_device.h will always depend on device.h that's why I removed them, but I can add them again if somebody wants to, but again, the situation is already bad.

so there's platform_device.h on that file, a little comment on the changelog would suffice to avoid this kind of mis-understanding.

Signature · 3 lines
balbi

DefectiveByDesign.org
author
#29 lore raw

PATCH v3 14/14 omap: mailbox: reorganize headers

Hello,

Quoted 7 lines · Felipe Balbi
Quoted 3 lines · ext Felipe Contreras

Now, I am pretty sure that platform_device.h will always depend on device.h that's why I removed them, but I can add them again if somebody wants to, but again, the situation is already bad.

so there's platform_device.h on that file, a little comment on the changelog would suffice to avoid this kind of mis-understanding.

Sure, will do.

Signature · 1 line
Felipe Contreras
#30 lore raw

v3 cover omap: mailbox: bunch of cleanups

In reply to Felipe Contreras

Hi Felipe,

From: ext Felipe Contreras <[email protected]>
Subject: [PATCH v3 00/14] omap: mailbox: bunch of cleanups
Date: Sat, 22 May 2010 19:14:11 +0200

Quoted 10 lines

Hi,

These are hopefully non-functional changes. Just shuffling code around, and removing unecessary stuff.

In v2 I tried to split platform_device and also make mailbox_mach built-in. Not any more.

Comments from Felipe Balbi, Tony Lindgren, Hiroshi DOYU, and Russell King.

Have you had a chance to update them?

Quoted 24 lines

Felipe Contreras (14):
omap: mailbox: trivial whitespace cleanups
omap: mailbox: trivial cleanups
omap: mailbox: reorganize structures
omap: mailbox: 2420 should be detected at run-time
omap: mailbox: use correct config for omap1
omap: mailbox: update omap1 probing
omap: mailbox: don't export unecessary symbols
omap: mailbox: remove unecessary fields
omap: mailbox: add IRQ names
omap: mailbox: reorganize registering
omap: mailbox: only compile for configured archs
omap: mailbox: standarize on 'omap-mailbox'
omap: mailbox: simplify omap_mbox_register()
omap: mailbox: reorganize headers

 arch/arm/mach-omap1/devices.c             |   11 +-
 arch/arm/mach-omap1/mailbox.c             |   52 ++++-----
 arch/arm/mach-omap2/devices.c             |    6 +-
 arch/arm/mach-omap2/mailbox.c             |  177 +++++++++++++----------------
 arch/arm/plat-omap/include/plat/mailbox.h |   16 +--
 arch/arm/plat-omap/mailbox.c              |  136 +++++++++-------------
 6 files changed, 168 insertions(+), 230 deletions(-)
author
#31 lore raw

v3 cover omap: mailbox: bunch of cleanups

Quoted 1 line · Hiroshi DOYU

Have you had a chance to update them?

Sorry, no. I've been busy with user-space stuff. Probably this weekend.

There's no merge-window rush or anything, right?

Signature · 1 line
Felipe Contreras
#32 lore raw

v3 cover omap: mailbox: bunch of cleanups

From: ext Felipe Contreras <[email protected]>
Subject: Re: [PATCH v3 00/14] omap: mailbox: bunch of cleanups
Date: Thu, 3 Jun 2010 12:23:43 +0200

Quoted 6 lines · Hiroshi DOYU
Quoted 1 line

Have you had a chance to update them?

Sorry, no. I've been busy with user-space stuff. Probably this weekend.

There's no merge-window rush or anything, right?

No rush. Sorry for just ping.

#34 lore raw

v3 cover omap: mailbox: bunch of cleanups

Hi,

Quoted 6 lines · ext Russell King - ARM Linux

I think folk need to read this:

	http://lkml.org/lkml/2010/6/2/472

and consider what it means for the future, and what can be done to reduce the amount of "churn".

I believe Linus is considering more the amount of files not the amount of patches. Personally, I consider trivial cleanup patches the best way to start off with a new code (not saying that Felipe C. is new to mbox driver) and those patches are as needed as a brand new feature.

One thing to decrease the amount of lines of code in arch/arm is to move the drivers to they're proper locations on the tree. Only on omap we have 6 platform_drivers that could (should) be moved to proper locations after a 'trivial cleanups'.

gpio.c mcbsp.c timer drivers (for clocksource) etc, which alone would mean a decrease of 6237 lines out of arch/arm/*omap* only. And I'm not even counting the drivers which aren't platform_drivers.

The thing is that ARM is really different than x86 and there's no standardization among different SoCs, so other than moving drivers out of arch/arm, there isn't much we can do. Or do you have any ideas ?

Signature · 3 lines
balbi

DefectiveByDesign.org
#35 lore raw

v3 cover omap: mailbox: bunch of cleanups

In reply to Russell King - ARM Linux

Hi Russell,

Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Hiroshi DOYU
Quoted 3 lines

Felipe Contreras (14):
omap: mailbox: trivial whitespace cleanups
omap: mailbox: trivial cleanups

I think folk need to read this:

	http://lkml.org/lkml/2010/6/2/472

and consider what it means for the future, and what can be done to reduce the amount of "churn".

Can you elaborate a bit? I guess you generally agree with Linus on this. What should be done in your opinion for the ARM arch to produce less "churn".

baruch

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
author
#36 lore raw

v3 cover omap: mailbox: bunch of cleanups

In reply to Russell King - ARM Linux
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Hiroshi DOYU
Quoted 3 lines

Felipe Contreras (14):
? omap: mailbox: trivial whitespace cleanups
? omap: mailbox: trivial cleanups

I think folk need to read this:

? ? ? ?http://lkml.org/lkml/2010/6/2/472

and consider what it means for the future, and what can be done to reduce the amount of "churn".

If I have to decipher that rant, my best guess is that he is complaining about the amount of code. If that's the case, this patch series is only doing good:

6 files changed, 168 insertions(+), 230 deletions(-)

Another part of the rant is the fact that Linus cannot follow the huge amount of changes. I think "trivial whitespace cleanups" pretty much
says: don't look at me if you want to be productive, while "trivial cleanups" says, look at me if you feel like looking at easy changes. If I were to squash these two, I think the patch series would become harder to follow.

Something that helps in other projects, like git, is that merges are clearly identified.

Merge branch 'linux-arm'
Merge branch 'linux-omap'
Merge branch 'linux-omap-mbox'
Merge branch 'fc-cleanups'
Merge branch 'ob-kfifo'

Then it's a bit clearer what each patch series is doing in the mailbox tree without looking at individual patches.

Cheers.

Signature · 1 line
Felipe Contreras
maintainer
#37 lore raw

v3 cover omap: mailbox: bunch of cleanups

Quoted 29 lines · Felipe Contreras
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Hiroshi DOYU
Quoted 3 lines

Felipe Contreras (14):
? omap: mailbox: trivial whitespace cleanups
? omap: mailbox: trivial cleanups

I think folk need to read this:

? ? ? ?http://lkml.org/lkml/2010/6/2/472

and consider what it means for the future, and what can be done to reduce the amount of "churn".

If I have to decipher that rant, my best guess is that he is complaining about the amount of code. If that's the case, this patch series is only doing good:

6 files changed, 168 insertions(+), 230 deletions(-)

Another part of the rant is the fact that Linus cannot follow the huge amount of changes. I think "trivial whitespace cleanups" pretty much
says: don't look at me if you want to be productive, while "trivial cleanups" says, look at me if you feel like looking at easy changes. If I were to squash these two, I think the patch series would become harder to follow.

Something that helps in other projects, like git, is that merges are clearly identified.

Quoted 8 lines

Merge branch 'linux-arm'
Merge branch 'linux-omap'
Merge branch 'linux-omap-mbox'
Merge branch 'fc-cleanups'
Merge branch 'ob-kfifo'

Then it's a bit clearer what each patch series is doing in the mailbox tree without looking at individual patches.

Also we need to make sure all the code we merge is really tested well before the merge window. Too much of the code needs to be fixed after the merge window. For the upcoming merge window, we will only merge things that have been sitting in the for-next for days before the merge window opens.

Tony