ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus.

PATCH New Jochen Friedrich 3 participants 10 messages Nov 27, 2011 – Jan 20, 2012

Subsystems (4)

Arm (32-bit) Arm sub-architectures Kbuild Multifunction devices (MFD)

lore

Share & tools

lore

Jochen Friedrich

author

2011-11-27 21:00 UTC #0 lore raw

PATCH 1/2 ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus.

Signed-off-by Jochen Friedrich <[email protected]>

---
 arch/arm/mach-sa1100/assabet.c          |    1 +
 arch/arm/mach-sa1100/cerf.c             |    1 +
 arch/arm/mach-sa1100/collie.c           |    8 ++++-
 arch/arm/mach-sa1100/include/mach/mcp.h |    2 +
 arch/arm/mach-sa1100/lart.c             |    1 +
 arch/arm/mach-sa1100/shannon.c          |    1 +
 arch/arm/mach-sa1100/simpad.c           |    8 ++++-
 drivers/mfd/mcp-core.c                  |   44 +++++++++++++++++++++++++++-
 drivers/mfd/mcp-sa11x0.c                |    7 +++-
 drivers/mfd/ucb1x00-core.c              |   48 +++++++++++++++++++++++-------
 drivers/mfd/ucb1x00-ts.c                |    2 +-
 include/linux/mfd/mcp.h                 |    7 +++-
 include/linux/mfd/ucb1x00.h             |    5 ++-
 include/linux/mod_devicetable.h         |   11 +++++++
 scripts/mod/file2alias.c                |   13 ++++++++
 15 files changed, 138 insertions(+), 21 deletions(-)

arch/arm/mach-sa1100/assabet.c +1 -0

index 3dd133f..14b31f1 100644
@@ -202,6 +202,7 @@ static struct irda_platform_data assabet_irda_data = {
 static struct mcp_plat_data assabet_mcp_data = {
 	.mccr0		= MCCR0_ADM,
 	.sclk_rate	= 11981000,
+	.codec		= "ucb1x00",
 };
 
 static void __init assabet_init(void)

arch/arm/mach-sa1100/cerf.c +1 -0

index a626790..73d4f1d 100644
@@ -127,6 +127,7 @@ static void __init cerf_map_io(void)
 static struct mcp_plat_data cerf_mcp_data = {
 	.mccr0		= MCCR0_ADM,
 	.sclk_rate	= 11981000,
+	.codec		= "ucb1x00",
 };
 
 static void __init cerf_init(void)

arch/arm/mach-sa1100/collie.c +7 -1

index 7a17703..65165f1 100644
@@ -27,6 +27,7 @@
 #include <linux/timer.h>
 #include <linux/gpio.h>
 #include <linux/pda_power.h>
+#include <linux/mfd/ucb1x00.h>
 
 #include <mach/hardware.h>
 #include <asm/mach-types.h>

@@ -85,10 +86,15 @@ static struct scoop_pcmcia_config collie_pcmcia_config = {
 	.num_devs	= 1,
 };
 
+static struct ucb1x00_plat_data collie_ucb1x00_data = {
+	.gpio_base	= COLLIE_TC35143_GPIO_BASE,
+};
+
 static struct mcp_plat_data collie_mcp_data = {
 	.mccr0		= MCCR0_ADM | MCCR0_ExtClk,
 	.sclk_rate	= 9216000,
-	.gpio_base	= COLLIE_TC35143_GPIO_BASE,
+	.codec		= "ucb1x00",
+	.codec_pdata	= &collie_ucb1x00_data,
 };
 
 /*

arch/arm/mach-sa1100/include/mach/mcp.h +2 -0

index ed1a331..586cec8 100644
@@ -17,6 +17,8 @@ struct mcp_plat_data {
 	u32 mccr1;
 	unsigned int sclk_rate;
 	int gpio_base;
+	const char *codec;
+	void *codec_pdata;
 };
 
 #endif

arch/arm/mach-sa1100/lart.c +1 -0

index a8fae5b..6235f39 100644
@@ -24,6 +24,7 @@
 static struct mcp_plat_data lart_mcp_data = {
 	.mccr0		= MCCR0_ADM,
 	.sclk_rate	= 11981000,
+	.codec		= "ucb1x00",
 };
 
 static void __init lart_init(void)

arch/arm/mach-sa1100/shannon.c +1 -0

index 84f5b14..b227368 100644
@@ -55,6 +55,7 @@ static struct resource shannon_flash_resource = {
 static struct mcp_plat_data shannon_mcp_data = {
 	.mccr0		= MCCR0_ADM,
 	.sclk_rate	= 11981000,
+	.codec		= "ucb1x00",
 };
 
 static void __init shannon_init(void)

arch/arm/mach-sa1100/simpad.c +7 -1

index d3c9d2b..c05b2e9 100644
@@ -14,6 +14,7 @@
 #include <linux/mtd/partitions.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/mfd/ucb1x00.h>
 
 #include <asm/irq.h>
 #include <mach/hardware.h>

@@ -187,10 +188,15 @@ static struct resource simpad_flash_resources [] = {
 	}
 };
 
+static struct ucb1x00_plat_data simpad_ucb1x00_data = {
+	.gpio_base	= SIMPAD_UCB1X00_GPIO_BASE,
+};
+
 static struct mcp_plat_data simpad_mcp_data = {
 	.mccr0		= MCCR0_ADM,
 	.sclk_rate	= 11981000,
-	.gpio_base	= SIMPAD_UCB1X00_GPIO_BASE,
+	.codec		= "ucb1300",
+	.codec_pdata	= &simpad_ucb1x00_data,
 };

drivers/mfd/mcp-core.c +42 -2

69 lines not on this page · raw message

drivers/mfd/mcp-sa11x0.c +5 -2

index 2dab02d..9cd38f1 100644
@@ -146,6 +146,9 @@ static int mcp_sa11x0_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENODEV;
 
+	if (!data->codec)
+		return -ENODEV;
+
 	if (!request_mem_region(0x80060000, 0x60, "sa11x0-mcp"))
 		return -EBUSY;

@@ -162,7 +165,7 @@ static int mcp_sa11x0_probe(struct platform_device *pdev)
 	mcp->dma_audio_wr	= DMA_Ser4MCP0Wr;
 	mcp->dma_telco_rd	= DMA_Ser4MCP1Rd;
 	mcp->dma_telco_wr	= DMA_Ser4MCP1Wr;
-	mcp->gpio_base		= data->gpio_base;
+	mcp->codec		= data->codec;
 
 	platform_set_drvdata(pdev, mcp);

@@ -195,7 +198,7 @@ static int mcp_sa11x0_probe(struct platform_device *pdev)
 	mcp->rw_timeout = (64 * 3 * 1000000 + mcp->sclk_rate - 1) /
 			  mcp->sclk_rate;
 
-	ret = mcp_host_register(mcp);
+	ret = mcp_host_register(mcp, data->codec_pdata);
 	if (ret == 0)
 		goto out;

drivers/mfd/ucb1x00-core.c +37 ~~-11~~

104 lines not on this page · raw message

drivers/mfd/ucb1x00-ts.c +1 -1

index 38ffbd5..40ec3c1 100644
@@ -382,7 +382,7 @@ static int ucb1x00_ts_add(struct ucb1x00_dev *dev)
 	ts->adcsync = adcsync ? UCB_SYNC : UCB_NOSYNC;
 
 	idev->name       = "Touchscreen panel";
-	idev->id.product = ts->ucb->id;
+	idev->id.product = ts->ucb->id->driver_data;
 	idev->open       = ucb1x00_ts_open;
 	idev->close      = ucb1x00_ts_close;

include/linux/mfd/mcp.h +5 -2

index ee496708..1515e64 100644
@@ -10,6 +10,7 @@
 #ifndef MCP_H
 #define MCP_H
 
+#include <linux/mod_devicetable.h>
 #include <mach/dma.h>
 
 struct mcp_ops;

@@ -26,7 +27,7 @@ struct mcp {
 	dma_device_t	dma_telco_rd;
 	dma_device_t	dma_telco_wr;
 	struct device	attached_device;
-	int		gpio_base;
+	const char	*codec;
 };
 
 struct mcp_ops {

@@ -44,10 +45,11 @@ void mcp_reg_write(struct mcp *, unsigned int, unsigned int);
 unsigned int mcp_reg_read(struct mcp *, unsigned int);
 void mcp_enable(struct mcp *);
 void mcp_disable(struct mcp *);
+const struct mcp_device_id *mcp_get_device_id(const struct mcp *mcp);
 #define mcp_get_sclk_rate(mcp)	((mcp)->sclk_rate)
 
 struct mcp *mcp_host_alloc(struct device *, size_t);
-int mcp_host_register(struct mcp *);
+int mcp_host_register(struct mcp *, void *);
 void mcp_host_unregister(struct mcp *);
 
 struct mcp_driver {

@@ -56,6 +58,7 @@ struct mcp_driver {
 	void (*remove)(struct mcp *);
 	int (*suspend)(struct mcp *, pm_message_t);
 	int (*resume)(struct mcp *);
+	const struct mcp_device_id *id_table;
 };
 
 int mcp_driver_register(struct mcp_driver *);

include/linux/mfd/ucb1x00.h +4 -1

index 4321f04..bc19e5f 100644
@@ -104,6 +104,9 @@
 #define UCB_MODE_DYN_VFLAG_ENA	(1 << 12)
 #define UCB_MODE_AUD_OFF_CAN	(1 << 13)
 
+struct ucb1x00_plat_data {
+	int		gpio_base;
+};
 
 struct ucb1x00_irq {
 	void *devid;

@@ -116,7 +119,7 @@ struct ucb1x00 {
 	unsigned int		irq;
 	struct semaphore	adc_sem;
 	spinlock_t		io_lock;
-	u16			id;
+	const struct mcp_device_id *id;
 	u16			io_dir;
 	u16			io_out;
 	u16			adc_cr;

include/linux/mod_devicetable.h +11 -0

index 468819c..bc50d9a 100644
@@ -436,6 +436,17 @@ struct spi_device_id {
 			__attribute__((aligned(sizeof(kernel_ulong_t))));
 };
 
+/* mcp */
+
+#define MCP_NAME_SIZE	20
+#define MCP_MODULE_PREFIX "mcp:"
+
+struct mcp_device_id {
+	char name[MCP_NAME_SIZE];
+	kernel_ulong_t driver_data	/* Data private to the driver */
+			__attribute__((aligned(sizeof(kernel_ulong_t))));
+};
+
 /* dmi */
 enum dmi_field {
 	DMI_NONE,

scripts/mod/file2alias.c +13 -0

index f936d1f..e8c7eb1 100644
@@ -774,6 +774,15 @@ static int do_spi_entry(const char *filename, struct spi_device_id *id,
 	return 1;
 }
 
+/* Looks like: mcp:S */
+static int do_mcp_entry(const char *filename, struct mcp_device_id *id,
+			char *alias)
+{
+	sprintf(alias, MCP_MODULE_PREFIX "%s", id->name);
+
+	return 1;
+}
+
 static const struct dmifield {
 	const char *prefix;
 	int field;

@@ -1027,6 +1036,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
 		do_table(symval, sym->st_size,
 			 sizeof(struct spi_device_id), "spi",
 			 do_spi_entry, mod);
+	else if (sym_is(symname, "__mod_mcp_device_table"))
+		do_table(symval, sym->st_size,
+			 sizeof(struct mcp_device_id), "mcp",
+			 do_mcp_entry, mod);
 	else if (sym_is(symname, "__mod_dmi_device_table"))
 		do_table(symval, sym->st_size,
 			 sizeof(struct dmi_system_id), "dmi",

Signature · 1 line

1.7.7.3

Jochen Friedrich

author

2011-11-27 21:00 UTC #1 lore raw

PATCH 2/2 ARM: sa1100: Refactor mcp-sa11x0 to use platform resources.

Make use of memory resources rather than hardcoded IO adresses. This is a first step towards DT support.

Signed-off-by Jochen Friedrich <[email protected]>

---
 arch/arm/mach-sa1100/assabet.c |   11 +++
 arch/arm/mach-sa1100/cerf.c    |   10 +++
 arch/arm/mach-sa1100/collie.c  |   10 +++
 arch/arm/mach-sa1100/generic.c |    7 ++-
 arch/arm/mach-sa1100/lart.c    |    9 ++
 arch/arm/mach-sa1100/shannon.c |   10 +++
 arch/arm/mach-sa1100/simpad.c  |   10 +++
 drivers/mfd/mcp-sa11x0.c       |  162 +++++++++++++++++++++++++++-------------
 8 files changed, 176 insertions(+), 53 deletions(-)

arch/arm/mach-sa1100/assabet.c +11 -0

index 14b31f1..96a162c 100644
@@ -253,6 +253,17 @@ static void __init assabet_init(void)
 	sa11x0_register_mtd(&assabet_flash_data, assabet_flash_resources,
 			    ARRAY_SIZE(assabet_flash_resources));
 	sa11x0_register_irda(&assabet_irda_data);
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
+	ASSABET_BCR_set(ASSABET_BCR_CODEC_RST);
 	sa11x0_register_mcp(&assabet_mcp_data);
 }

arch/arm/mach-sa1100/cerf.c +10 -0

index 73d4f1d..3e025df2a 100644
@@ -134,6 +134,16 @@ static void __init cerf_init(void)
 {
 	platform_add_devices(cerf_devices, ARRAY_SIZE(cerf_devices));
 	sa11x0_register_mtd(&cerf_flash_data, &cerf_flash_resource, 1);
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&cerf_mcp_data);
 }

arch/arm/mach-sa1100/collie.c +10 -0

index 65165f1..326e44b 100644
@@ -359,6 +359,16 @@ static void __init collie_init(void)
 
 	sa11x0_register_mtd(&collie_flash_data, collie_flash_resources,
 			    ARRAY_SIZE(collie_flash_resources));
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&collie_mcp_data);
 
 	sharpsl_save_param();

arch/arm/mach-sa1100/generic.c +6 -1

index 465bcfc..83717b2 100644
@@ -226,10 +226,15 @@ static struct platform_device sa11x0uart3_device = {
 static struct resource sa11x0mcp_resources[] = {
 	[0] = {
 		.start	= __PREG(Ser4MCCR0),
-		.end	= __PREG(Ser4MCCR0) + 0xffff,
+		.end	= __PREG(Ser4MCCR0) + 0x1C - 1,
 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
+		.start	= __PREG(Ser4MCCR1),
+		.end	= __PREG(Ser4MCCR1) + 0x4 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[2] = {
 		.start	= IRQ_Ser4MCP,
 		.end	= IRQ_Ser4MCP,
 		.flags	= IORESOURCE_IRQ,

arch/arm/mach-sa1100/lart.c +9 -0

index 6235f39..c4c3018 100644
@@ -29,6 +29,15 @@ static struct mcp_plat_data lart_mcp_data = {
 
 static void __init lart_init(void)
 {
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&lart_mcp_data);
 }

arch/arm/mach-sa1100/shannon.c +10 -0

index b227368..9f36335 100644
@@ -61,6 +61,16 @@ static struct mcp_plat_data shannon_mcp_data = {
 static void __init shannon_init(void)
 {
 	sa11x0_register_mtd(&shannon_flash_data, &shannon_flash_resource, 1);
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&shannon_mcp_data);
 }

arch/arm/mach-sa1100/simpad.c +10 -0

index c05b2e9..c9f2f41 100644
@@ -387,6 +387,16 @@ static int __init simpad_init(void)
 
 	sa11x0_register_mtd(&simpad_flash_data, simpad_flash_resources,
 			      ARRAY_SIZE(simpad_flash_resources));
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&simpad_mcp_data);
 
 	ret = platform_add_devices(devices, ARRAY_SIZE(devices));

drivers/mfd/mcp-sa11x0.c +110 ~~-52~~

290 lines not on this page · raw message

Signature · 1 line

1.7.7.3

Russell King - ARM Linux

2012-01-14 09:21 UTC #2 lore raw

PATCH 2/2 ARM: sa1100: Refactor mcp-sa11x0 to use platform resources.

Quoted 2 lines · Jochen Friedrich

Make use of memory resources rather than hardcoded IO adresses. This is a first step towards DT support.

I should have spotted this patch earlier.

This is the obviously wrong approach - if you're having to add the same code to almost every board, then you're doing something wrong.

Not only that, but...

Quoted 69 lines

arch/arm/mach-sa1100/assabet.c +11 -0

index 14b31f1..96a162c 100644
@@ -253,6 +253,17 @@ static void __init assabet_init(void)
 	sa11x0_register_mtd(&assabet_flash_data, assabet_flash_resources,
 			    ARRAY_SIZE(assabet_flash_resources));
 	sa11x0_register_irda(&assabet_irda_data);
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
+	ASSABET_BCR_set(ASSABET_BCR_CODEC_RST);
 	sa11x0_register_mcp(&assabet_mcp_data);
 }

arch/arm/mach-sa1100/cerf.c +10 -0

index 73d4f1d..3e025df2a 100644
@@ -134,6 +134,16 @@ static void __init cerf_init(void)
 {
 	platform_add_devices(cerf_devices, ARRAY_SIZE(cerf_devices));
 	sa11x0_register_mtd(&cerf_flash_data, &cerf_flash_resource, 1);
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&cerf_mcp_data);
 }

arch/arm/mach-sa1100/collie.c +10 -0

index 65165f1..326e44b 100644
@@ -359,6 +359,16 @@ static void __init collie_init(void)
 
 	sa11x0_register_mtd(&collie_flash_data, collie_flash_resources,
 			    ARRAY_SIZE(collie_flash_resources));
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&collie_mcp_data);
 
 	sharpsl_save_param();

arch/arm/mach-sa1100/generic.c +0 -0

index 465bcfc..83717b2 100644

arch/arm/mach-sa1100/generic.c static struct platform_device sa11x0uart3_device = { · line 229

Quoted 5 lines

arch/arm/mach-sa1100/generic.c +1 -1

@@ -226,10 +226,15 @@ static struct platform_device sa11x0uart3_device = {
 static struct resource sa11x0mcp_resources[] = {
 	[0] = {
 		.start	= __PREG(Ser4MCCR0),
-		.end	= __PREG(Ser4MCCR0) + 0xffff,
+		.end	= __PREG(Ser4MCCR0) + 0x1C - 1,

Please leave this as +0xffff - we treat all devices has having 64K resources on sa11x0 platforms. (It's possible that the registers repeat throughout the memory space.)

Quoted 96 lines

arch/arm/mach-sa1100/generic.c +5 -0

 		.flags	= IORESOURCE_MEM,
 	},
 	[1] = {
+		.start	= __PREG(Ser4MCCR1),
+		.end	= __PREG(Ser4MCCR1) + 0x4 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[2] = {
 		.start	= IRQ_Ser4MCP,
 		.end	= IRQ_Ser4MCP,
 		.flags	= IORESOURCE_IRQ,

arch/arm/mach-sa1100/lart.c +9 -0

index 6235f39..c4c3018 100644
@@ -29,6 +29,15 @@ static struct mcp_plat_data lart_mcp_data = {
 
 static void __init lart_init(void)
 {
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&lart_mcp_data);
 }

arch/arm/mach-sa1100/shannon.c +10 -0

index b227368..9f36335 100644
@@ -61,6 +61,16 @@ static struct mcp_plat_data shannon_mcp_data = {
 static void __init shannon_init(void)
 {
 	sa11x0_register_mtd(&shannon_flash_data, &shannon_flash_resource, 1);
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&shannon_mcp_data);
 }

arch/arm/mach-sa1100/simpad.c +10 -0

index c05b2e9..c9f2f41 100644
@@ -387,6 +387,16 @@ static int __init simpad_init(void)
 
 	sa11x0_register_mtd(&simpad_flash_data, simpad_flash_resources,
 			      ARRAY_SIZE(simpad_flash_resources));
+
+	/*
+	 * Setup the PPC unit correctly.
+	 */
+	PPDR &= ~PPC_RXD4;
+	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
+	PSDR |= PPC_RXD4;
+	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
+
 	sa11x0_register_mcp(&simpad_mcp_data);
 
 	ret = platform_add_devices(devices, ARRAY_SIZE(devices));

drivers/mfd/mcp-sa11x0.c +6 -2

index 9cd38f1..965b2f7 100644
@@ -19,6 +19,7 @@
 #include <linux/spinlock.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/mcp.h>
+#include <linux/io.h>
 
 #include <mach/dma.h>
 #include <mach/hardware.h>

@@ -26,12 +27,19 @@
 #include <asm/system.h>
 #include <mach/mcp.h>
 
-#include <mach/assabet.h>
-
+/* Register offsets */
+#define MCCR0	0x00
+#define MCDR0	0x08
+#define MCDR1	0x0C
+#define MCDR2	0x10

drivers/mfd/mcp-sa11x0.c line 42

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +6 -2

+#define MCSR	0x18
+#define MCCR1	0x00
 
 struct mcp_sa11x0 {
-	u32	mccr0;
-	u32	mccr1;
+	u32		mccr0;
+	u32		mccr1;
+	unsigned char	*mccr0_base;
+	unsigned char	*mccr1_base;

Basic rule of kernel programming: the return type of ioremap is:

void __iomem *

Note the '__iomem'. That must stay with the cookie. Check your code with sparse.

Quoted 8 lines

drivers/mfd/mcp-sa11x0.c +0 -1

 };
 
 #define priv(mcp)	((struct mcp_sa11x0 *)mcp_priv(mcp))

@@ -39,25 +47,25 @@ struct mcp_sa11x0 {
 static void
 mcp_sa11x0_set_telecom_divisor(struct mcp *mcp, unsigned int divisor)
 {
-	unsigned int mccr0;

drivers/mfd/mcp-sa11x0.c struct mcp_sa11x0 { · line 56

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +4 -3

+	struct mcp_sa11x0 *priv = priv(mcp);
 
 	divisor /= 32;
 
-	mccr0 = Ser4MCCR0 & ~0x00007f00;
-	mccr0 |= divisor << 8;
-	Ser4MCCR0 = mccr0;
+	priv->mccr0 &= ~0x00007f00;
+	priv->mccr0 |= divisor << 8;
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);

writel? writel_relaxed?

Quoted 6 lines

drivers/mfd/mcp-sa11x0.c +0 -1

 }
 
 static void
 mcp_sa11x0_set_audio_divisor(struct mcp *mcp, unsigned int divisor)
 {
-	unsigned int mccr0;

drivers/mfd/mcp-sa11x0.c struct mcp_sa11x0 { · line 68

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +4 -3

+	struct mcp_sa11x0 *priv = priv(mcp);
 
 	divisor /= 32;
 
-	mccr0 = Ser4MCCR0 & ~0x0000007f;
-	mccr0 |= divisor;
-	Ser4MCCR0 = mccr0;
+	priv->mccr0 &= ~0x0000007f;
+	priv->mccr0 |= divisor;
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);

Ditto.

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_write(struct mcp *mcp, unsigned int reg, unsigned int val) · line 86

Quoted 13 lines

drivers/mfd/mcp-sa11x0.c +4 -1

 }
 
 /*

@@ -71,12 +79,16 @@ mcp_sa11x0_write(struct mcp *mcp, unsigned int reg, unsigned int val)
 {
 	int ret = -ETIME;
 	int i;
+	u32 mcpreg;
+	struct mcp_sa11x0 *priv = priv(mcp);
 
-	Ser4MCDR2 = reg << 17 | MCDR2_Wr | (val & 0xffff);
+	mcpreg = reg << 17 | MCDR2_Wr | (val & 0xffff);
+	__raw_writel(mcpreg, priv->mccr0_base + MCDR2);

Ditto.

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_write(struct mcp *mcp, unsigned int reg, unsigned int val) · line 90

Quoted 5 lines

drivers/mfd/mcp-sa11x0.c +1 -1

 	for (i = 0; i < 2; i++) {
 		udelay(mcp->rw_timeout);
-		if (Ser4MCSR & MCSR_CWC) {
+		mcpreg = __raw_readl(priv->mccr0_base + MCSR);

readl or readl_relaxed?

Quoted 5 lines

drivers/mfd/mcp-sa11x0.c +1 -0

+		if (mcpreg & MCSR_CWC) {
 			ret = 0;
 			break;
 		}

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_read(struct mcp *mcp, unsigned int reg) · line 116

Quoted 9 lines

drivers/mfd/mcp-sa11x0.c +4 -1

@@ -97,13 +109,18 @@ mcp_sa11x0_read(struct mcp *mcp, unsigned int reg)
 {
 	int ret = -ETIME;
 	int i;
+	u32 mcpreg;
+	struct mcp_sa11x0 *priv = priv(mcp);
 
-	Ser4MCDR2 = reg << 17 | MCDR2_Rd;
+	mcpreg = reg << 17 | MCDR2_Rd;
+	__raw_writel(mcpreg, priv->mccr0_base + MCDR2);

writel or writel_relaxed?

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_read(struct mcp *mcp, unsigned int reg) · line 120

Quoted 6 lines

drivers/mfd/mcp-sa11x0.c +1 -2

 	for (i = 0; i < 2; i++) {
 		udelay(mcp->rw_timeout);
-		if (Ser4MCSR & MCSR_CRC) {
-			ret = Ser4MCDR2 & 0xffff;
+		mcpreg = __raw_readl(priv->mccr0_base + MCSR);

readl or readl_relaxed?

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_read(struct mcp *mcp, unsigned int reg) · line 122

Quoted 2 lines

drivers/mfd/mcp-sa11x0.c +2 -0

+		if (mcpreg & MCSR_CRC) {
+			ret = __raw_readl(priv->mccr0_base + MCDR2)

Ditto.

Quoted 5 lines

drivers/mfd/mcp-sa11x0.c +1 -0

+				& 0xffff;
 			break;
 		}
 	}

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_read(struct mcp *mcp, unsigned int reg) · line 140

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +5 -2

@@ -116,13 +133,19 @@ mcp_sa11x0_read(struct mcp *mcp, unsigned int reg)
 
 static void mcp_sa11x0_enable(struct mcp *mcp)
 {
-	Ser4MCSR = -1;
-	Ser4MCCR0 |= MCCR0_MCE;
+	struct mcp_sa11x0 *priv = priv(mcp);
+
+	__raw_writel(-1, priv->mccr0_base + MCSR);
+	priv->mccr0 |= MCCR0_MCE;
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);

etc.

drivers/mfd/mcp-sa11x0.c mcp_sa11x0_read(struct mcp *mcp, unsigned int reg) · line 148

Quoted 9 lines

drivers/mfd/mcp-sa11x0.c +4 -1

 }
 
 static void mcp_sa11x0_disable(struct mcp *mcp)
 {
-	Ser4MCCR0 &= ~MCCR0_MCE;
+	struct mcp_sa11x0 *priv = priv(mcp);
+
+	priv->mccr0 &= ~MCCR0_MCE;
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);

etc.

Quoted 14 lines

drivers/mfd/mcp-sa11x0.c +3 -0

 }
 
 /*

@@ -142,6 +165,9 @@ static int mcp_sa11x0_probe(struct platform_device *pdev)
 	struct mcp_plat_data *data = pdev->dev.platform_data;
 	struct mcp *mcp;
 	int ret;
+	struct mcp_sa11x0 *priv;
+	struct resource *res_mem0, *res_mem1;
+	u32 size0, size1;
 
 	if (!data)
 		return -ENODEV;

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 181

Quoted 8 lines

drivers/mfd/mcp-sa11x0.c +4 -1

@@ -149,46 +175,59 @@ static int mcp_sa11x0_probe(struct platform_device *pdev)
 	if (!data->codec)
 		return -ENODEV;
 
-	if (!request_mem_region(0x80060000, 0x60, "sa11x0-mcp"))
+	res_mem0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res_mem0)
+		return -ENODEV;
+	size0 = res_mem0->end - res_mem0->start + 1;

resource_size() ?

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 186

Quoted 5 lines

drivers/mfd/mcp-sa11x0.c +5 -0

+
+	res_mem1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res_mem1)
+		return -ENODEV;
+	size1 = res_mem1->end - res_mem1->start + 1;

resource_size?

Quoted 49 lines

drivers/mfd/mcp-sa11x0.c +18 ~~-17~~

+
+	if (!request_mem_region(res_mem0->start, size0, "sa11x0-mcp"))
 		return -EBUSY;
 
+	if (!request_mem_region(res_mem1->start, size1, "sa11x0-mcp")) {
+		ret = -EBUSY;
+		goto release;
+	}
+
 	mcp = mcp_host_alloc(&pdev->dev, sizeof(struct mcp_sa11x0));
 	if (!mcp) {
 		ret = -ENOMEM;
-		goto release;
+		goto release2;
 	}
 
+	priv = priv(mcp);
+
 	mcp->owner		= THIS_MODULE;
 	mcp->ops		= &mcp_sa11x0;
 	mcp->sclk_rate		= data->sclk_rate;
-	mcp->dma_audio_rd	= DMA_Ser4MCP0Rd;
-	mcp->dma_audio_wr	= DMA_Ser4MCP0Wr;
-	mcp->dma_telco_rd	= DMA_Ser4MCP1Rd;
-	mcp->dma_telco_wr	= DMA_Ser4MCP1Wr;
+	mcp->dma_audio_rd	= DDAR_DevAdd(res_mem0->start + MCDR0)
+				+ DDAR_DevRd + DDAR_Brst4 + DDAR_8BitDev;
+	mcp->dma_audio_wr	= DDAR_DevAdd(res_mem0->start + MCDR0)
+				+ DDAR_DevWr + DDAR_Brst4 + DDAR_8BitDev;
+	mcp->dma_telco_rd	= DDAR_DevAdd(res_mem0->start + MCDR1)
+				+ DDAR_DevRd + DDAR_Brst4 + DDAR_8BitDev;
+	mcp->dma_telco_wr	= DDAR_DevAdd(res_mem0->start + MCDR1)
+				+ DDAR_DevWr + DDAR_Brst4 + DDAR_8BitDev;
 	mcp->codec		= data->codec;
 
 	platform_set_drvdata(pdev, mcp);
 
-	if (machine_is_assabet()) {
-		ASSABET_BCR_set(ASSABET_BCR_CODEC_RST);
-	}
-
-	/*
-	 * Setup the PPC unit correctly.
-	 */
-	PPDR &= ~PPC_RXD4;
-	PPDR |= PPC_TXD4 | PPC_SCLK | PPC_SFRM;
-	PSDR |= PPC_RXD4;
-	PSDR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);
-	PPSR &= ~(PPC_TXD4 | PPC_SCLK | PPC_SFRM);

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 224

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +2 -4

-
 	/*
 	 * Initialise device.  Note that we initially
 	 * set the sampling rate to minimum.
 	 */
-	Ser4MCSR = -1;
-	Ser4MCCR1 = data->mccr1;
-	Ser4MCCR0 = data->mccr0 | 0x7f7f;
+	priv->mccr0_base = ioremap(res_mem0->start, size0);
+	priv->mccr1_base = ioremap(res_mem1->start, size1);

You want to oops the kernel? Where's the error checking?

Quoted 31 lines

drivers/mfd/mcp-sa11x0.c +13 -3

+
+	__raw_writel(-1, priv->mccr0_base + MCSR);
+	priv->mccr1 = data->mccr1;
+	priv->mccr0 = data->mccr0 | 0x7f7f;
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);
+	__raw_writel(priv->mccr1, priv->mccr1_base + MCCR1);
 
 	/*
 	 * Calculate the read/write timeout (us) from the bit clock

@@ -202,32 +241,49 @@ static int mcp_sa11x0_probe(struct platform_device *pdev)
 	if (ret == 0)
 		goto out;
 
+ release2:
+	release_mem_region(res_mem1->start, size1);
  release:
-	release_mem_region(0x80060000, 0x60);
+	release_mem_region(res_mem0->start, size0);
 	platform_set_drvdata(pdev, NULL);
 
  out:
 	return ret;
 }
 
-static int mcp_sa11x0_remove(struct platform_device *dev)
+static int mcp_sa11x0_remove(struct platform_device *pdev)
 {
-	struct mcp *mcp = platform_get_drvdata(dev);
+	struct mcp *mcp = platform_get_drvdata(pdev);
+	struct mcp_sa11x0 *priv = priv(mcp);
+	struct resource *res_mem;

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 266

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +5 -2

+	u32 size;
 
-	platform_set_drvdata(dev, NULL);
+	platform_set_drvdata(pdev, NULL);
 	mcp_host_unregister(mcp);
-	release_mem_region(0x80060000, 0x60);
 
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res_mem) {
+		size = res_mem->end - res_mem->start + 1;

resource_size ?

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 271

Quoted 5 lines

drivers/mfd/mcp-sa11x0.c +5 -0

+		release_mem_region(res_mem->start, size);
+	}
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res_mem) {
+		size = res_mem->end - res_mem->start + 1;

resource_size?

Quoted 8 lines

drivers/mfd/mcp-sa11x0.c +4 -0

+		release_mem_region(res_mem->start, size);
+	}
+	iounmap(priv->mccr0_base);
+	iounmap(priv->mccr1_base);
 	return 0;
 }
 
 static int mcp_sa11x0_suspend(struct platform_device *dev, pm_message_t state)

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 286

Quoted 10 lines

drivers/mfd/mcp-sa11x0.c +4 -3

 {
 	struct mcp *mcp = platform_get_drvdata(dev);
+	struct mcp_sa11x0 *priv = priv(mcp);
+	u32 mccr0;
 
-	priv(mcp)->mccr0 = Ser4MCCR0;
-	priv(mcp)->mccr1 = Ser4MCCR1;
-	Ser4MCCR0 &= ~MCCR0_MCE;
+	mccr0 = priv->mccr0 & ~MCCR0_MCE;
+	__raw_writel(mccr0, priv->mccr0_base + MCCR0);

writel or writel_relaxed?

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_suspend(struct platform_device *dev, pm_message_t state) · line 297

Quoted 13 lines

drivers/mfd/mcp-sa11x0.c +3 -2

 	return 0;
 }

@@ -235,9 +291,10 @@ static int mcp_sa11x0_suspend(struct platform_device *dev, pm_message_t state)
 static int mcp_sa11x0_resume(struct platform_device *dev)
 {
 	struct mcp *mcp = platform_get_drvdata(dev);
+	struct mcp_sa11x0 *priv = priv(mcp);
 
-	Ser4MCCR1 = priv(mcp)->mccr1;
-	Ser4MCCR0 = priv(mcp)->mccr0;
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);
+	__raw_writel(priv->mccr1, priv->mccr1_base + MCCR1);

writel or writel_relaxed?

Quoted 19 lines

drivers/mfd/mcp-sa11x0.c +1 -0

 	return 0;
 }

@@ -254,6 +311,7 @@ static struct platform_driver mcp_sa11x0_driver = {
 	.resume		= mcp_sa11x0_resume,
 	.driver		= {
 		.name	= "sa11x0-mcp",
+		.owner  = THIS_MODULE,
 	},
 };

Signature · 7 lines

1.7.7.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Jochen Friedrich

author

2012-01-17 09:41 UTC #3 lore raw

PATCH 2/2 ARM: sa1100: Refactor mcp-sa11x0 to use platform resources.

Hi Russell,

Quoted 1 line · Russell King - ARM Linux

I should have spotted this patch earlier.

Sorry for my late response as well.

Quoted 2 lines

This is the obviously wrong approach - if you're having to add the same code to almost every board, then you're doing something wrong.

Accessing some random SoC registers and in the assabet case even board specific registers from device drivers is even worse IMHO. We don't get a clean solution until we have a pinmux/pinctrl device driver owning these peripheral control registers. At least having the setup in the board files avoids a potential race condition accessing PPDR and PPSR registers from multiple drivers.

Quoted 12 lines

Not only that, but...
arch/arm/mach-sa1100/generic.c static struct platform_device sa11x0uart3_device = { · line 229
Quoted 5 lines
  static struct resource sa11x0mcp_resources[] = {
  	[0] = {
  		.start	= __PREG(Ser4MCCR0),
arch/arm/mach-sa1100/generic.c +1 -1
-		.end	= __PREG(Ser4MCCR0) + 0xffff,
+		.end	= __PREG(Ser4MCCR0) + 0x1C - 1,
Please leave this as +0xffff - we treat all devices has having 64K resources on sa11x0 platforms. (It's possible that the registers repeat throughout the memory space.)

OK, but...

Quoted 7 lines

arch/arm/mach-sa1100/generic.c static struct platform_device sa11x0uart3_device = { · line 236
Quoted 7 lines
  		.flags	= IORESOURCE_MEM,
  	},
  	[1] = {
arch/arm/mach-sa1100/generic.c +4 -0
+		.start	= __PREG(Ser4MCCR1),
+		.end	= __PREG(Ser4MCCR1) + 0x4 - 1,
+		.flags	= IORESOURCE_MEM,
+	},

How to treat this case then? Here one register is "borrowed" from a different 64K block.
The old driver version didn't even register access to this memory space.

Quoted 4 lines

void __iomem *
Note the '__iomem'. That must stay with the cookie. Check your code with sparse.

OK, will do.

Quoted 3 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+	__raw_writel(priv->mccr0, priv->mccr0_base + MCCR0);
writel? writel_relaxed?

writel_relaxed should be fine. The old version didn't use any barriers, so I assume that's OK. I'll check if reorders would cause any problem.

Quoted 3 lines

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 181
Quoted 1 line
drivers/mfd/mcp-sa11x0.c +1 -0
+	size0 = res_mem0->end - res_mem0->start + 1;
resource_size() ?

Jamie pointed me to this already. I'll change this.

Quoted 4 lines

drivers/mfd/mcp-sa11x0.c static int mcp_sa11x0_probe(struct platform_device *pdev) · line 224
Quoted 2 lines
drivers/mfd/mcp-sa11x0.c +2 -0
+	priv->mccr0_base = ioremap(res_mem0->start, size0);
+	priv->mccr1_base = ioremap(res_mem1->start, size1);
You want to oops the kernel? Where's the error checking?

Good catch!

Thanks for the review,
Jochen

Russell King - ARM Linux

2012-01-17 20:12 UTC #4 lore raw

PATCH 2/2 ARM: sa1100: Refactor mcp-sa11x0 to use platform resources.

Quoted 6 lines · Jochen Friedrich

Accessing some random SoC registers and in the assabet case even board specific registers from device drivers is even worse IMHO. We don't get a clean solution until we have a pinmux/pinctrl device driver owning these peripheral control registers. At least having the setup in the board files avoids a potential race condition accessing PPDR and PPSR registers from multiple drivers.

Yes, but do we really need the exact same code repeated time and time again in each different board file? Could we not have a function which sets the pins up for MCP?

Quoted 14 lines

Quoted 11 lines
Not only that, but...
arch/arm/mach-sa1100/generic.c static struct platform_device sa11x0uart3_device = { · line 229
Quoted 5 lines
  static struct resource sa11x0mcp_resources[] = {
  	[0] = {
  		.start	= __PREG(Ser4MCCR0),
arch/arm/mach-sa1100/generic.c +1 -1
-		.end	= __PREG(Ser4MCCR0) + 0xffff,
+		.end	= __PREG(Ser4MCCR0) + 0x1C - 1,
Please leave this as +0xffff - we treat all devices has having 64K resources on sa11x0 platforms. (It's possible that the registers repeat throughout the memory space.)
OK, but...

Quoted 11 lines

Quoted 7 lines
arch/arm/mach-sa1100/generic.c static struct platform_device sa11x0uart3_device = { · line 236
Quoted 7 lines
  		.flags	= IORESOURCE_MEM,
  	},
  	[1] = {
arch/arm/mach-sa1100/generic.c +4 -0
+		.start	= __PREG(Ser4MCCR1),
+		.end	= __PREG(Ser4MCCR1) + 0x4 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
How to treat this case then? Here one register is "borrowed" from a different 64K block.
The old driver version didn't even register access to this memory space.

I'm not complaining about this one - the MCCR1 shares its 64K space with the peripheral pin controller, and I suspect we won't get to the point where that's controlled by a driver. So it's probably going to remain a feature dealt with primerily by arch/arm/mach-sa1100 code.

Jochen Friedrich

author

2012-01-18 11:30 UTC #5 lore raw

PATCH 2/2 ARM: sa1100: Refactor mcp-sa11x0 to use platform resources.

Hi Russell,

Quoted 10 lines

Quoted 6 lines
Accessing some random SoC registers and in the assabet case even board specific registers from device drivers is even worse IMHO. We don't get a clean solution until we have a pinmux/pinctrl device driver owning these peripheral control registers. At least having the setup in the board files avoids a potential race condition accessing PPDR and PPSR registers from multiple drivers.
Yes, but do we really need the exact same code repeated time and time again in each different board file? Could we not have a function which sets the pins up for MCP?

Yes, this would be a good idea. One should do the same for the sa1100 serial driver. Here we even have the bug that the driver unconditionally sets up pins for UART1 and 3 no matter what platform devices are present.

Quoted 3 lines

I'm not complaining about this one - the MCCR1 shares its 64K space with the peripheral pin controller, and I suspect we won't get to the point where that's controlled by a driver.

Why not? gpio-generic should be able to handle the PPC registers out of the box.

Thanks,
Jochen

Russell King - ARM Linux

2012-01-18 11:48 UTC #6 lore raw

PATCH 2/2 ARM: sa1100: Refactor mcp-sa11x0 to use platform resources.

Quoted 9 lines · Jochen Friedrich

Hi Russell,
Quoted 7 lines
Quoted 6 lines
Accessing some random SoC registers and in the assabet case even board specific registers from device drivers is even worse IMHO. We don't get a clean solution until we have a pinmux/pinctrl device driver owning these peripheral control registers. At least having the setup in the board files avoids a potential race condition accessing PPDR and PPSR registers from multiple drivers.

Quoted 7 lines

Quoted 3 lines
Yes, but do we really need the exact same code repeated time and time again in each different board file? Could we not have a function which sets the pins up for MCP?
Yes, this would be a good idea. One should do the same for the sa1100 serial driver. Here we even have the bug that the driver unconditionally sets up pins for UART1 and 3 no matter what platform devices are present.

If you'd care to pay attention to the code, you'll notice that UARTs 1 and 3 are always unconditionally registered. Therefore, this is not a bug. It's a question of code organization, one which may be sub-standard by todays standards.

It was decided _long_ ago that the _drivers_ would setup the PPC configuration, because on some platforms the PPC configuration depends on what drivers you've configured into the kernel and loaded (which depends on how you jumper some of these early boards) rather than what board is being used.

So, unless you're being extremely careful about moving stuff around, you're going to break stuff.

Quoted 6 lines

Quoted 3 lines
I'm not complaining about this one - the MCCR1 shares its 64K space with the peripheral pin controller, and I suspect we won't get to the point where that's controlled by a driver.
Why not? gpio-generic should be able to handle the PPC registers out of the box.

And then we're going to see a whole slew of additional gpio calls needing to be added throughout the code... this kind of large scale modification sounds like a recipe for things going wrong.

And no, I don't agree that SA-1100 stuff should be pushed towards device tree. Leave it be, it's legacy stuff, and it's not providing much pain as it is. And some of the problems within there are extremely difficult to solve (hell, some of them can't be solved using platform data alone - a lot are procedural problems which can't be expressed in data.)

Samuel Ortiz

Applied

2011-12-12 17:44 UTC #7 lore raw

PATCH 1/2 ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus.

In reply to Jochen Friedrich

Hi Jochen,

Quoted 10 lines · Jochen Friedrich

Signed-off-by Jochen Friedrich <[email protected]>

---
 arch/arm/mach-sa1100/assabet.c          |    1 +
 arch/arm/mach-sa1100/cerf.c             |    1 +
 arch/arm/mach-sa1100/collie.c           |    8 ++++-
 arch/arm/mach-sa1100/include/mach/mcp.h |    2 +
 arch/arm/mach-sa1100/lart.c             |    1 +
 arch/arm/mach-sa1100/shannon.c          |    1 +
 arch/arm/mach-sa1100/simpad.c           |    8 ++++-
 drivers/mfd/mcp-core.c                  |   44 +++++++++++++++++++++++++++-

Quoted 8 lines

 drivers/mfd/mcp-sa11x0.c                |    7 +++-
 drivers/mfd/ucb1x00-core.c              |   48 +++++++++++++++++++++++-------
 drivers/mfd/ucb1x00-ts.c                |    2 +-
 include/linux/mfd/mcp.h                 |    7 +++-
 include/linux/mfd/ucb1x00.h             |    5 ++-
 include/linux/mod_devicetable.h         |   11 +++++++
 scripts/mod/file2alias.c                |   13 ++++++++
 15 files changed, 138 insertions(+), 21 deletions(-)

I applied your 2 patches, thanks.

Cheers,
Samuel.

Signature · 2 lines

Intel Open Source Technology Centre
http://oss.intel.com/

Russell King - ARM Linux

2012-01-18 15:21 UTC #8 lore raw

PATCH 1/2 ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus.

In reply to Jochen Friedrich

On Sun, Nov 27, 2011 at 10:00:54PM +0100, Jochen Friedrich wrote:

No commit log.

Do you actually have a platform which connects something other than a UCB1x00 to the MCP, or are you just fiddling about in this code making changes only to 'bring it up to latest standards' but without any real purpose?

arch/arm/mach-sa1100/include/mach/mcp.h struct mcp_plat_data { · line 19

Quoted 8 lines

arch/arm/mach-sa1100/include/mach/mcp.h +0 -0

index ed1a331..586cec8 100644
@@ -17,6 +17,8 @@ struct mcp_plat_data {
 	u32 mccr1;
 	unsigned int sclk_rate;
 	int gpio_base;

This was added with the addition of GPIO, but now doesn't seem to be used after your patch.

Quoted 43 lines

drivers/mfd/ucb1x00-core.c +17 -2

index b281217..91c4f25 100644
@@ -36,6 +36,15 @@ static DEFINE_MUTEX(ucb1x00_mutex);
 static LIST_HEAD(ucb1x00_drivers);
 static LIST_HEAD(ucb1x00_devices);
 
+static struct mcp_device_id ucb1x00_id[] = {
+	{ "ucb1x00", 0 },  /* auto-detection */
+	{ "ucb1200", UCB_ID_1200 },
+	{ "ucb1300", UCB_ID_1300 },
+	{ "tc35143", UCB_ID_TC35143 },
+	{ }
+};
+MODULE_DEVICE_TABLE(mcp, ucb1x00_id);
+
 /**
  *	ucb1x00_io_set_dir - set IO direction
  *	@ucb: UCB1x00 structure describing chip

@@ -527,17 +536,33 @@ static struct class ucb1x00_class = {
 
 static int ucb1x00_probe(struct mcp *mcp)
 {
+	const struct mcp_device_id *mid;
 	struct ucb1x00 *ucb;
 	struct ucb1x00_driver *drv;
+	struct ucb1x00_plat_data *pdata;
 	unsigned int id;
 	int ret = -ENODEV;
 	int temp;
 
 	mcp_enable(mcp);
 	id = mcp_reg_read(mcp, UCB_ID);
+	mid = mcp_get_device_id(mcp);
 
-	if (id != UCB_ID_1200 && id != UCB_ID_1300 && id != UCB_ID_TC35143) {
-		printk(KERN_WARNING "UCB1x00 ID not found: %04x\n", id);
+	if (mid && mid->driver_data) {
+		if (id != mid->driver_data) {
+			printk(KERN_WARNING "%s wrong ID %04x found: %04x\n",
+				mid->name, (unsigned int) mid->driver_data, id);
+			goto err_disable;

drivers/mfd/ucb1x00-core.c static struct class ucb1x00_class = { · line 565

Quoted 10 lines

drivers/mfd/ucb1x00-core.c +10 -0

+		}
+	} else {
+		mid = &ucb1x00_id[1];
+		while (mid->driver_data) {
+			if (id == mid->driver_data)
+				break;
+			mid++;
+		}
+		printk(KERN_WARNING "%s ID not found: %04x\n",
+			ucb1x00_id[0].name, id);

Why do we need this complexity when we can detect the device from its ID in hardware? This, to me, is just code for the sake of code.

I'm far from impressed by this patch, and had it not already gone in, I'd be NAKing it.

Russell King - ARM Linux

2012-01-20 17:11 UTC #9 lore raw

PATCH 1/2 ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus.

Quoted 20 lines · Russell King - ARM Linux

Quoted 5 lines · Jochen Friedrich
drivers/mfd/ucb1x00-core.c +5 -0
+	if (mid && mid->driver_data) {
+		if (id != mid->driver_data) {
+			printk(KERN_WARNING "%s wrong ID %04x found: %04x\n",
+				mid->name, (unsigned int) mid->driver_data, id);
+			goto err_disable;
drivers/mfd/ucb1x00-core.c static struct class ucb1x00_class = { · line 565
Quoted 10 lines
drivers/mfd/ucb1x00-core.c +10 -0
+		}
+	} else {
+		mid = &ucb1x00_id[1];
+		while (mid->driver_data) {
+			if (id == mid->driver_data)
+				break;
+			mid++;
+		}
+		printk(KERN_WARNING "%s ID not found: %04x\n",
+			ucb1x00_id[0].name, id);
Why do we need this complexity when we can detect the device from its ID in hardware? This, to me, is just code for the sake of code.

Quoted 2 lines

I'm far from impressed by this patch, and had it not already gone in, I'd be NAKing it.

Right, I'm even more convinced that the above is over the top. What it's doing is requiring either:

(a) the autodetect option to be given, in which case we will match using the ID from the device
(b) use a specific option, in which case the ID must match

which is absolutely silly and completely unnecessary. If we require the ID to be correct from the hardware, there's absolutely no point what so ever passing some kind of identifier in to the driver. It just creates an additional unnecessary reason for things to fail - in other words, the only thing it adds is _fragility_.

And to prove the point that it's fragile, on the Assabet I get this:

ucb1x00 ID not found: 1005

and it fails to initialize - 0x1005 is a valid id for a UCB1300 device.

So clearly this code hasn't even been tested either.

Samuel, please revert this change. It's broken and does more than what it says in the change log.