[PATCH 0/6] ux500: Export SoC information and some platform clean-up

[PATCH 0/6] ux500: Export SoC information and some platform clean-up

[Lee Jones]

This patch-set satisfies 4 objectives:
 1. Ensures each platform device can specify a suitable parent
 2. Provides a bus for SoC devices as a means to export information
 3. Allows ux500 to make use of the new SoC bus
 4. Cleans up unnecessary complexity from ux500 code

---
 Documentation/ABI/testing/sysfs-devices-soc    |   58 ++++++++
 arch/arm/mach-ux500/Kconfig                    |    1 +
 arch/arm/mach-ux500/board-mop500-sdi.c         |   31 ++--
 arch/arm/mach-ux500/board-mop500.c             |   74 ++++++----
 arch/arm/mach-ux500/board-mop500.h             |    8 +-
 arch/arm/mach-ux500/board-u5500-sdi.c          |    4 +-
 arch/arm/mach-ux500/board-u5500.c              |   27 ++--
 arch/arm/mach-ux500/cpu-db5500.c               |   36 ++++-
 arch/arm/mach-ux500/cpu-db8500.c               |   44 +++++-
 arch/arm/mach-ux500/cpu.c                      |   75 ++++++++++
 arch/arm/mach-ux500/devices-common.c           |   79 ++--------
 arch/arm/mach-ux500/devices-common.h           |   83 ++++++-----
 arch/arm/mach-ux500/devices-db5500.h           |  116 +++++++++------
 arch/arm/mach-ux500/devices-db8500.h           |  176 +++++++++++++----------
 arch/arm/mach-ux500/dma-db5500.c               |    3 +-
 arch/arm/mach-ux500/include/mach/db8500-regs.h |    3 +
 arch/arm/mach-ux500/include/mach/setup.h       |   10 +-
 arch/arm/mach-ux500/include/mach/usb.h         |    4 +-
 arch/arm/mach-ux500/usb.c                      |    7 +-
 drivers/base/Kconfig                           |    3 +
 drivers/base/Makefile                          |    1 +
 drivers/base/soc.c                             |  186 ++++++++++++++++++++++++
 include/linux/syssoc.h                         |   74 ++++++++++
 23 files changed, 799 insertions(+), 304 deletions(-)

[PATCH 1/6] mach-ux500: pass parent pointer to each platform device

[Lee Jones]

This patch provides a means for any device within ux500
platform code to allocate its own parent. This is particularly
prudent with the introduction of /sys/devices/socX, as a
device can now proclaim to be integral part of an SoC, rather
than a more generic platform device. Latter patches make good
use of this functionality.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500-sdi.c   |   31 +++---
 arch/arm/mach-ux500/board-mop500.c       |   62 ++++++-----
 arch/arm/mach-ux500/board-mop500.h       |    8 +-
 arch/arm/mach-ux500/board-u5500-sdi.c    |    4 +-
 arch/arm/mach-ux500/board-u5500.c        |   23 +++--
 arch/arm/mach-ux500/cpu-db5500.c         |   18 ++--
 arch/arm/mach-ux500/cpu-db8500.c         |   15 ++--
 arch/arm/mach-ux500/devices-common.c     |   13 ++-
 arch/arm/mach-ux500/devices-common.h     |   39 ++++---
 arch/arm/mach-ux500/devices-db5500.h     |  116 ++++++++++++---------
 arch/arm/mach-ux500/devices-db8500.h     |  166 +++++++++++++++++-------------
 arch/arm/mach-ux500/dma-db5500.c         |    3 +-
 arch/arm/mach-ux500/include/mach/setup.h |    8 +-
 arch/arm/mach-ux500/include/mach/usb.h   |    4 +-
 arch/arm/mach-ux500/usb.c                |    4 +-
 15 files changed, 289 insertions(+), 225 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500-sdi.c b/arch/arm/mach-ux500/board-mop500-sdi.c
index 23be34b..163c083 100644
--- a/arch/arm/mach-ux500/board-mop500-sdi.c
+++ b/arch/arm/mach-ux500/board-mop500-sdi.c
@@ -104,7 +104,7 @@ static struct mmci_platform_data mop500_sdi0_data = {
 #endif
 };
 
-static void sdi0_configure(void)
+static void sdi0_configure(struct device *parent)
 {
 	int ret;
 
@@ -123,15 +123,15 @@ static void sdi0_configure(void)
 	gpio_direction_output(sdi0_en, 1);
 
 	/* Add the device, force v2 to subrevision 1 */
-	db8500_add_sdi0(&mop500_sdi0_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi0(parent, &mop500_sdi0_data, U8500_SDI_V2_PERIPHID);
 }
 
-void mop500_sdi_tc35892_init(void)
+void mop500_sdi_tc35892_init(struct device *parent)
 {
 	mop500_sdi0_data.gpio_cd = GPIO_SDMMC_CD;
 	sdi0_en = GPIO_SDMMC_EN;
 	sdi0_vsel = GPIO_SDMMC_1V8_3V_SEL;
-	sdi0_configure();
+	sdi0_configure(parent);
 }
 
 /*
@@ -246,12 +246,13 @@ static struct mmci_platform_data mop500_sdi4_data = {
 #endif
 };
 
-void __init mop500_sdi_init(void)
+void __init mop500_sdi_init(struct device *parent)
 {
 	/* PoP:ed eMMC */
-	db8500_add_sdi2(&mop500_sdi2_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi2(parent, &mop500_sdi2_data, U8500_SDI_V2_PERIPHID);
 	/* On-board eMMC */
-	db8500_add_sdi4(&mop500_sdi4_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi4(parent, &mop500_sdi4_data, U8500_SDI_V2_PERIPHID);
+
 	/*
 	 * On boards with the TC35892 GPIO expander, sdi0 will finally
 	 * be added when the TC35892 initializes and calls
@@ -259,29 +260,29 @@ void __init mop500_sdi_init(void)
 	 */
 }
 
-void __init snowball_sdi_init(void)
+void __init snowball_sdi_init(struct device *parent)
 {
 	/* On-board eMMC */
-	db8500_add_sdi4(&mop500_sdi4_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi4(parent, &mop500_sdi4_data, U8500_SDI_V2_PERIPHID);
 	/* External Micro SD slot */
 	mop500_sdi0_data.gpio_cd = SNOWBALL_SDMMC_CD_GPIO;
 	mop500_sdi0_data.cd_invert = true;
 	sdi0_en = SNOWBALL_SDMMC_EN_GPIO;
 	sdi0_vsel = SNOWBALL_SDMMC_1V8_3V_GPIO;
-	sdi0_configure();
+	sdi0_configure(parent);
 }
 
-void __init hrefv60_sdi_init(void)
+void __init hrefv60_sdi_init(struct device *parent)
 {
 	/* PoP:ed eMMC */
-	db8500_add_sdi2(&mop500_sdi2_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi2(parent, &mop500_sdi2_data, U8500_SDI_V2_PERIPHID);
 	/* On-board eMMC */
-	db8500_add_sdi4(&mop500_sdi4_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi4(parent, &mop500_sdi4_data, U8500_SDI_V2_PERIPHID);
 	/* External Micro SD slot */
 	mop500_sdi0_data.gpio_cd = HREFV60_SDMMC_CD_GPIO;
 	sdi0_en = HREFV60_SDMMC_EN_GPIO;
 	sdi0_vsel = HREFV60_SDMMC_1V8_3V_GPIO;
-	sdi0_configure();
+	sdi0_configure(parent);
 	/* WLAN SDIO channel */
-	db8500_add_sdi1(&mop500_sdi1_data, U8500_SDI_V2_PERIPHID);
+	db8500_add_sdi1(parent, &mop500_sdi1_data, U8500_SDI_V2_PERIPHID);
 }
diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 5c00712..f9ce2a1 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -226,7 +226,12 @@ static struct tps6105x_platform_data mop500_tps61052_data = {
 
 static void mop500_tc35892_init(struct tc3589x *tc3589x, unsigned int base)
 {
-	mop500_sdi_tc35892_init();
+	struct device *parent = NULL;
+#if 0
+	/* FIXME: Is the sdi actually part of tc3589x? */
+	parent = tc3589x->dev;
+#endif
+	mop500_sdi_tc35892_init(parent);
 }
 
 static struct tc3589x_gpio_platform_data mop500_tc35892_gpio_data = {
@@ -353,12 +358,12 @@ U8500_I2C_CONTROLLER(1, 0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
 U8500_I2C_CONTROLLER(2,	0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
 U8500_I2C_CONTROLLER(3,	0xe, 1, 8, 100000, 200, I2C_FREQ_MODE_FAST);
 
-static void __init mop500_i2c_init(void)
+static void __init mop500_i2c_init(struct device *parent)
 {
-	db8500_add_i2c0(&u8500_i2c0_data);
-	db8500_add_i2c1(&u8500_i2c1_data);
-	db8500_add_i2c2(&u8500_i2c2_data);
-	db8500_add_i2c3(&u8500_i2c3_data);
+	db8500_add_i2c0(parent, &u8500_i2c0_data);
+	db8500_add_i2c1(parent, &u8500_i2c1_data);
+	db8500_add_i2c2(parent, &u8500_i2c2_data);
+	db8500_add_i2c3(parent, &u8500_i2c3_data);
 }
 
 static struct gpio_keys_button mop500_gpio_keys[] = {
@@ -451,9 +456,9 @@ static struct pl022_ssp_controller ssp0_platform_data = {
 	.num_chipselect = 5,
 };
 
-static void __init mop500_spi_init(void)
+static void __init mop500_spi_init(struct device *parent)
 {
-	db8500_add_ssp0(&ssp0_platform_data);
+	db8500_add_ssp0(parent, &ssp0_platform_data);
 }
 
 #ifdef CONFIG_STE_DMA40
@@ -587,11 +592,11 @@ static struct amba_pl011_data uart2_plat = {
 #endif
 };
 
-static void __init mop500_uart_init(void)
+static void __init mop500_uart_init(struct device *parent)
 {
-	db8500_add_uart0(&uart0_plat);
-	db8500_add_uart1(&uart1_plat);
-	db8500_add_uart2(&uart2_plat);
+	db8500_add_uart0(parent, &uart0_plat);
+	db8500_add_uart1(parent, &uart1_plat);
+	db8500_add_uart2(parent, &uart2_plat);
 }
 
 static struct platform_device *snowball_platform_devs[] __initdata = {
@@ -603,21 +608,22 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
 
 static void __init mop500_init_machine(void)
 {
+	struct device *parent = NULL;
 	int i2c0_devs;
 
 	mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
-	u8500_init_devices();
+	parent = u8500_init_devices();
 
 	mop500_pins_init();
 
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
 
-	mop500_i2c_init();
-	mop500_sdi_init();
-	mop500_spi_init();
-	mop500_uart_init();
+	mop500_i2c_init(parent);
+	mop500_sdi_init(parent);
+	mop500_spi_init(parent);
+	mop500_uart_init(parent);
 
 	i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 
@@ -631,19 +637,20 @@ static void __init mop500_init_machine(void)
 
 static void __init snowball_init_machine(void)
 {
+	struct device *parent = NULL;
 	int i2c0_devs;
 
-	u8500_init_devices();
+	parent = u8500_init_devices();
 
 	snowball_pins_init();
 
 	platform_add_devices(snowball_platform_devs,
 			ARRAY_SIZE(snowball_platform_devs));
 
-	mop500_i2c_init();
-	snowball_sdi_init();
-	mop500_spi_init();
-	mop500_uart_init();
+	mop500_i2c_init(parent);
+	snowball_sdi_init(parent);
+	mop500_spi_init(parent);
+	mop500_uart_init(parent);
 
 	i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 	i2c_register_board_info(0, mop500_i2c0_devices, i2c0_devs);
@@ -656,6 +663,7 @@ static void __init snowball_init_machine(void)
 
 static void __init hrefv60_init_machine(void)
 {
+	struct device *parent = NULL;
 	int i2c0_devs;
 
 	/*
@@ -665,17 +673,17 @@ static void __init hrefv60_init_machine(void)
 	 */
 	mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
 
-	u8500_init_devices();
+	parent = u8500_init_devices();
 
 	hrefv60_pins_init();
 
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
 
-	mop500_i2c_init();
-	hrefv60_sdi_init();
-	mop500_spi_init();
-	mop500_uart_init();
+	mop500_i2c_init(parent);
+	hrefv60_sdi_init(parent);
+	mop500_spi_init(parent);
+	mop500_uart_init(parent);
 
 	i2c0_devs = ARRAY_SIZE(mop500_i2c0_devices);
 
diff --git a/arch/arm/mach-ux500/board-mop500.h b/arch/arm/mach-ux500/board-mop500.h
index f926d3d..3d594c2 100644
--- a/arch/arm/mach-ux500/board-mop500.h
+++ b/arch/arm/mach-ux500/board-mop500.h
@@ -75,10 +75,10 @@
 
 struct i2c_board_info;
 
-extern void mop500_sdi_init(void);
-extern void snowball_sdi_init(void);
-extern void hrefv60_sdi_init(void);
-extern void mop500_sdi_tc35892_init(void);
+extern void mop500_sdi_init(struct device *parent);
+extern void snowball_sdi_init(struct device *parent);
+extern void hrefv60_sdi_init(struct device *parent);
+extern void mop500_sdi_tc35892_init(struct device *parent);
 void __init mop500_u8500uib_init(void);
 void __init mop500_stuib_init(void);
 void __init mop500_pins_init(void);
diff --git a/arch/arm/mach-ux500/board-u5500-sdi.c b/arch/arm/mach-ux500/board-u5500-sdi.c
index 63c3f80..836112e 100644
--- a/arch/arm/mach-ux500/board-u5500-sdi.c
+++ b/arch/arm/mach-ux500/board-u5500-sdi.c
@@ -66,9 +66,9 @@ static struct mmci_platform_data u5500_sdi0_data = {
 #endif
 };
 
-void __init u5500_sdi_init(void)
+void __init u5500_sdi_init(struct device *parent)
 {
 	nmk_config_pins(u5500_sdi_pins, ARRAY_SIZE(u5500_sdi_pins));
 
-	db5500_add_sdi0(&u5500_sdi0_data);
+	db5500_add_sdi0(parent, &u5500_sdi0_data);
 }
diff --git a/arch/arm/mach-ux500/board-u5500.c b/arch/arm/mach-ux500/board-u5500.c
index 9de9e9c..d7a9596 100644
--- a/arch/arm/mach-ux500/board-u5500.c
+++ b/arch/arm/mach-ux500/board-u5500.c
@@ -97,9 +97,9 @@ static struct i2c_board_info __initdata u5500_i2c2_devices[] = {
 	},
 };
 
-static void __init u5500_i2c_init(void)
+static void __init u5500_i2c_init(struct device *parent)
 {
-	db5500_add_i2c2(&u5500_i2c2_data);
+	db5500_add_i2c2(parent, &u5500_i2c2_data);
 	i2c_register_board_info(2, ARRAY_AND_SIZE(u5500_i2c2_devices));
 }
 
@@ -126,20 +126,23 @@ static struct platform_device *u5500_platform_devices[] __initdata = {
 	&ab5500_device,
 };
 
-static void __init u5500_uart_init(void)
+static void __init u5500_uart_init(struct device *parent)
 {
-	db5500_add_uart0(NULL);
-	db5500_add_uart1(NULL);
-	db5500_add_uart2(NULL);
+	db5500_add_uart0(parent, NULL);
+	db5500_add_uart1(parent, NULL);
+	db5500_add_uart2(parent, NULL);
 }
 
 static void __init u5500_init_machine(void)
 {
-	u5500_init_devices();
+	struct device *parent = NULL;
+
+	parent = u5500_init_devices();
 	nmk_config_pins(u5500_pins, ARRAY_SIZE(u5500_pins));
-	u5500_i2c_init();
-	u5500_sdi_init();
-	u5500_uart_init();
+
+	u5500_i2c_init(parent);
+	u5500_sdi_init(parent);
+	u5500_uart_init(parent);
 
 	platform_add_devices(u5500_platform_devices,
 		ARRAY_SIZE(u5500_platform_devices));
diff --git a/arch/arm/mach-ux500/cpu-db5500.c b/arch/arm/mach-ux500/cpu-db5500.c
index 18aa5c0..c402fd6 100644
--- a/arch/arm/mach-ux500/cpu-db5500.c
+++ b/arch/arm/mach-ux500/cpu-db5500.c
@@ -147,13 +147,13 @@ static resource_size_t __initdata db5500_gpio_base[] = {
 	U5500_GPIOBANK7_BASE,
 };
 
-static void __init db5500_add_gpios(void)
+static void __init db5500_add_gpios(struct device *parent)
 {
 	struct nmk_gpio_platform_data pdata = {
 		/* No custom data yet */
 	};
 
-	dbx500_add_gpios(ARRAY_AND_SIZE(db5500_gpio_base),
+	dbx500_add_gpios(parent, ARRAY_AND_SIZE(db5500_gpio_base),
 			 IRQ_DB5500_GPIO0, &pdata);
 }
 
@@ -212,14 +212,18 @@ static int usb_db5500_tx_dma_cfg[] = {
 	DB5500_DMA_DEV38_USB_OTG_OEP_8
 };
 
-void __init u5500_init_devices(void)
+struct device* __init u5500_init_devices(void)
 {
-	db5500_add_gpios();
+	/* FIXME: First parameter to be a real parent. */
+	db5500_add_gpios(NULL);
 	db5500_pmu_init();
-	db5500_dma_init();
-	db5500_add_rtc();
-	db5500_add_usb(usb_db5500_rx_dma_cfg, usb_db5500_tx_dma_cfg);
+	db5500_dma_init(NULL);
+	db5500_add_rtc(NULL);
+	db5500_add_usb(NULL, usb_db5500_rx_dma_cfg, usb_db5500_tx_dma_cfg);
 
 	platform_add_devices(db5500_platform_devs,
 			     ARRAY_SIZE(db5500_platform_devs));
+
+	/* FIXME: Return value to be a real parent. */
+	return NULL;
 }
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 7176ee7..1e8a2cb 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -132,13 +132,13 @@ static resource_size_t __initdata db8500_gpio_base[] = {
 	U8500_GPIOBANK8_BASE,
 };
 
-static void __init db8500_add_gpios(void)
+static void __init db8500_add_gpios(struct device *parent)
 {
 	struct nmk_gpio_platform_data pdata = {
 		.supports_sleepmode = true,
 	};
 
-	dbx500_add_gpios(ARRAY_AND_SIZE(db8500_gpio_base),
+	dbx500_add_gpios(parent, ARRAY_AND_SIZE(db8500_gpio_base),
 			 IRQ_DB8500_GPIO0, &pdata);
 }
 
@@ -167,14 +167,15 @@ static int usb_db8500_tx_dma_cfg[] = {
 /*
  * This function is called from the board init
  */
-void __init u8500_init_devices(void)
+struct device* __init u8500_init_devices(void)
 {
-	db8500_add_rtc();
-	db8500_add_gpios();
-	db8500_add_usb(usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
+	db8500_add_rtc(NULL);
+	db8500_add_gpios(NULL);
+	db8500_add_usb(NULL, usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
 
 	platform_device_register_simple("cpufreq-u8500", -1, NULL, 0);
 	platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
 
-	return ;
+	/* FIXME: Return value to be a real parent. */
+	return NULL;
 }
diff --git a/arch/arm/mach-ux500/devices-common.c b/arch/arm/mach-ux500/devices-common.c
index c563e54..96be248 100644
--- a/arch/arm/mach-ux500/devices-common.c
+++ b/arch/arm/mach-ux500/devices-common.c
@@ -20,8 +20,9 @@
 #include "devices-common.h"
 
 struct amba_device *
-dbx500_add_amba_device(const char *name, resource_size_t base,
-		       int irq, void *pdata, unsigned int periphid)
+dbx500_add_amba_device(struct device *parent, const char *name,
+		       resource_size_t base, int irq, void *pdata,
+		       unsigned int periphid)
 {
 	struct amba_device *dev;
 	int ret;
@@ -109,7 +110,7 @@ dbx500_add_platform_device_4k1irq(const char *name, int id,
 }
 
 static struct platform_device *
-dbx500_add_gpio(int id, resource_size_t addr, int irq,
+dbx500_add_gpio(struct device *parent, int id, resource_size_t addr, int irq,
 		struct nmk_gpio_platform_data *pdata)
 {
 	struct resource resources[] = {
@@ -130,8 +131,8 @@ dbx500_add_gpio(int id, resource_size_t addr, int irq,
 				pdata, sizeof(*pdata));
 }
 
-void dbx500_add_gpios(resource_size_t *base, int num, int irq,
-		      struct nmk_gpio_platform_data *pdata)
+void dbx500_add_gpios(struct device *parent, resource_size_t *base, int num,
+		      int irq, struct nmk_gpio_platform_data *pdata)
 {
 	int first = 0;
 	int i;
@@ -141,6 +142,6 @@ void dbx500_add_gpios(resource_size_t *base, int num, int irq,
 		pdata->first_irq = NOMADIK_GPIO_TO_IRQ(first);
 		pdata->num_gpio = 32;
 
-		dbx500_add_gpio(i, base[i], irq, pdata);
+		dbx500_add_gpio(parent, i, base[i], irq, pdata);
 	}
 }
diff --git a/arch/arm/mach-ux500/devices-common.h b/arch/arm/mach-ux500/devices-common.h
index 7825705..f8adff8 100644
--- a/arch/arm/mach-ux500/devices-common.h
+++ b/arch/arm/mach-ux500/devices-common.h
@@ -9,7 +9,7 @@
 #define __DEVICES_COMMON_H
 
 extern struct amba_device *
-dbx500_add_amba_device(const char *name, resource_size_t base,
+dbx500_add_amba_device(struct device *parent, const char *name, resource_size_t base,
 		       int irq, void *pdata, unsigned int periphid);
 
 extern struct platform_device *
@@ -20,43 +20,46 @@ dbx500_add_platform_device_4k1irq(const char *name, int id,
 struct spi_master_cntlr;
 
 static inline struct amba_device *
-dbx500_add_msp_spi(const char *name, resource_size_t base, int irq,
+dbx500_add_msp_spi(struct device *parent, const char *name,
+		   resource_size_t base, int irq,
 		   struct spi_master_cntlr *pdata)
 {
-	return dbx500_add_amba_device(name, base, irq, pdata, 0);
+	return dbx500_add_amba_device(parent, name, base, irq,
+				      pdata, 0);
 }
 
 static inline struct amba_device *
-dbx500_add_spi(const char *name, resource_size_t base, int irq,
-	       struct spi_master_cntlr *pdata,
+dbx500_add_spi(struct device *parent, const char *name, resource_size_t base,
+	       int irq, struct spi_master_cntlr *pdata,
 	       u32 periphid)
 {
-	return dbx500_add_amba_device(name, base, irq, pdata, periphid);
+	return dbx500_add_amba_device(parent, name, base, irq,
+				      pdata, periphid);
 }
 
 struct mmci_platform_data;
 
 static inline struct amba_device *
-dbx500_add_sdi(const char *name, resource_size_t base, int irq,
-	       struct mmci_platform_data *pdata,
-	       u32 periphid)
+dbx500_add_sdi(struct device *parent, const char *name, resource_size_t base,
+	       int irq, struct mmci_platform_data *pdata, u32 periphid)
 {
-	return dbx500_add_amba_device(name, base, irq, pdata, periphid);
+	return dbx500_add_amba_device(parent, name, base, irq,
+				      pdata, periphid);
 }
 
 struct amba_pl011_data;
 
 static inline struct amba_device *
-dbx500_add_uart(const char *name, resource_size_t base, int irq,
-		struct amba_pl011_data *pdata)
+dbx500_add_uart(struct device *parent, const char *name, resource_size_t base,
+		int irq, struct amba_pl011_data *pdata)
 {
-	return dbx500_add_amba_device(name, base, irq, pdata, 0);
+	return dbx500_add_amba_device(parent, name, base, irq, pdata, 0);
 }
 
 struct nmk_i2c_controller;
 
 static inline struct platform_device *
-dbx500_add_i2c(int id, resource_size_t base, int irq,
+dbx500_add_i2c(struct device *parent, int id, resource_size_t base, int irq,
 	       struct nmk_i2c_controller *pdata)
 {
 	return dbx500_add_platform_device_4k1irq("nmk-i2c", id, base, irq,
@@ -74,14 +77,14 @@ dbx500_add_msp_i2s(int id, resource_size_t base, int irq,
 }
 
 static inline struct amba_device *
-dbx500_add_rtc(resource_size_t base, int irq)
+dbx500_add_rtc(struct device *parent, resource_size_t base, int irq)
 {
-	return dbx500_add_amba_device("rtc-pl031", base, irq, NULL, 0);
+	return dbx500_add_amba_device(parent, "rtc-pl031", base, irq, NULL, 0);
 }
 
 struct nmk_gpio_platform_data;
 
-void dbx500_add_gpios(resource_size_t *base, int num, int irq,
-		      struct nmk_gpio_platform_data *pdata);
+void dbx500_add_gpios(struct device *parent, resource_size_t *base, int num,
+		      int irq, struct nmk_gpio_platform_data *pdata);
 
 #endif
diff --git a/arch/arm/mach-ux500/devices-db5500.h b/arch/arm/mach-ux500/devices-db5500.h
index 0c4bccd..e709555 100644
--- a/arch/arm/mach-ux500/devices-db5500.h
+++ b/arch/arm/mach-ux500/devices-db5500.h
@@ -10,70 +10,90 @@
 
 #include "devices-common.h"
 
-#define db5500_add_i2c1(pdata) \
-	dbx500_add_i2c(1, U5500_I2C1_BASE, IRQ_DB5500_I2C1, pdata)
-#define db5500_add_i2c2(pdata) \
-	dbx500_add_i2c(2, U5500_I2C2_BASE, IRQ_DB5500_I2C2, pdata)
-#define db5500_add_i2c3(pdata) \
-	dbx500_add_i2c(3, U5500_I2C3_BASE, IRQ_DB5500_I2C3, pdata)
+#define db5500_add_i2c1(parent, pdata) \
+	dbx500_add_i2c(parent, 1, U5500_I2C1_BASE, IRQ_DB5500_I2C1, pdata)
+#define db5500_add_i2c2(parent, pdata) \
+	dbx500_add_i2c(parent, 2, U5500_I2C2_BASE, IRQ_DB5500_I2C2, pdata)
+#define db5500_add_i2c3(parent, pdata) \
+	dbx500_add_i2c(parent, 3, U5500_I2C3_BASE, IRQ_DB5500_I2C3, pdata)
 
-#define db5500_add_msp0_i2s(pdata) \
-	dbx500_add_msp_i2s(0, U5500_MSP0_BASE, IRQ_DB5500_MSP0, pdata)
-#define db5500_add_msp1_i2s(pdata) \
-	dbx500_add_msp_i2s(1, U5500_MSP1_BASE, IRQ_DB5500_MSP1, pdata)
-#define db5500_add_msp2_i2s(pdata) \
-	dbx500_add_msp_i2s(2, U5500_MSP2_BASE, IRQ_DB5500_MSP2, pdata)
+#define db5500_add_msp0_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp0", U5500_MSP0_BASE, \
+			   IRQ_DB5500_MSP0, pdata)
+#define db5500_add_msp1_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp1", U5500_MSP1_BASE, \
+			   IRQ_DB5500_MSP1, pdata)
+#define db5500_add_msp2_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp2", U5500_MSP2_BASE, \
+			   IRQ_DB5500_MSP2, pdata)
 
-#define db5500_add_msp0_spi(pdata) \
-	dbx500_add_msp_spi("msp0", U5500_MSP0_BASE, IRQ_DB5500_MSP0, pdata)
-#define db5500_add_msp1_spi(pdata) \
-	dbx500_add_msp_spi("msp1", U5500_MSP1_BASE, IRQ_DB5500_MSP1, pdata)
-#define db5500_add_msp2_spi(pdata) \
-	dbx500_add_msp_spi("msp2", U5500_MSP2_BASE, IRQ_DB5500_MSP2, pdata)
+#define db5500_add_msp0_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp0", U5500_MSP0_BASE, \
+			  IRQ_DB5500_MSP0, pdata)
+#define db5500_add_msp1_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp1", U5500_MSP1_BASE, \
+			  IRQ_DB5500_MSP1, pdata)
+#define db5500_add_msp2_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp2", U5500_MSP2_BASE, \
+			  IRQ_DB5500_MSP2, pdata)
 
-#define db5500_add_rtc() \
-	dbx500_add_rtc(U5500_RTC_BASE, IRQ_DB5500_RTC);
+#define db5500_add_rtc(parent) \
+	dbx500_add_rtc(parent, U5500_RTC_BASE, IRQ_DB5500_RTC);
 
-#define db5500_add_usb(rx_cfg, tx_cfg) \
-	ux500_add_usb(U5500_USBOTG_BASE, IRQ_DB5500_USBOTG, rx_cfg, tx_cfg)
+#define db5500_add_usb(parent, rx_cfg, tx_cfg) \
+	ux500_add_usb(parent, U5500_USBOTG_BASE, \
+		      IRQ_DB5500_USBOTG, rx_cfg, tx_cfg)
 
-#define db5500_add_sdi0(pdata) \
-	dbx500_add_sdi("sdi0", U5500_SDI0_BASE, IRQ_DB5500_SDMMC0, pdata, \
+#define db5500_add_sdi0(parent, pdata) \
+	dbx500_add_sdi(parent, "sdi0", U5500_SDI0_BASE, \
+		       IRQ_DB5500_SDMMC0, pdata,	\
 		       0x10480180)
-#define db5500_add_sdi1(pdata) \
-	dbx500_add_sdi("sdi1", U5500_SDI1_BASE, IRQ_DB5500_SDMMC1, pdata, \
+#define db5500_add_sdi1(parent, pdata) \
+	dbx500_add_sdi(parent, "sdi1", U5500_SDI1_BASE, \
+		       IRQ_DB5500_SDMMC1, pdata,	\
 		       0x10480180)
-#define db5500_add_sdi2(pdata) \
-	dbx500_add_sdi("sdi2", U5500_SDI2_BASE, IRQ_DB5500_SDMMC2, pdata \
+#define db5500_add_sdi2(parent, pdata) \
+	dbx500_add_sdi(parent, "sdi2", U5500_SDI2_BASE, \
+		       IRQ_DB5500_SDMMC2, pdata		\
 		       0x10480180)
-#define db5500_add_sdi3(pdata) \
-	dbx500_add_sdi("sdi3", U5500_SDI3_BASE, IRQ_DB5500_SDMMC3, pdata \
+#define db5500_add_sdi3(parent, pdata) \
+	dbx500_add_sdi(parent, "sdi3", U5500_SDI3_BASE, \
+		       IRQ_DB5500_SDMMC3, pdata		\
 		       0x10480180)
-#define db5500_add_sdi4(pdata) \
-	dbx500_add_sdi("sdi4", U5500_SDI4_BASE, IRQ_DB5500_SDMMC4, pdata \
+#define db5500_add_sdi4(parent, pdata) \
+	dbx500_add_sdi(parent, "sdi4", U5500_SDI4_BASE, \
+		       IRQ_DB5500_SDMMC4, pdata		\
 		       0x10480180)
 
 /* This one has a bad peripheral ID in the U5500 silicon */
-#define db5500_add_spi0(pdata) \
-	dbx500_add_spi("spi0", U5500_SPI0_BASE, IRQ_DB5500_SPI0, pdata, \
+#define db5500_add_spi0(parent, pdata) \
+	dbx500_add_spi(parent, "spi0", U5500_SPI0_BASE, \
+		       IRQ_DB5500_SPI0, pdata,		\
 		       0x10080023)
-#define db5500_add_spi1(pdata) \
-	dbx500_add_spi("spi1", U5500_SPI1_BASE, IRQ_DB5500_SPI1, pdata, \
+#define db5500_add_spi1(parent, pdata) \
+	dbx500_add_spi(parent, "spi1", U5500_SPI1_BASE, \
+		       IRQ_DB5500_SPI1, pdata,		\
 		       0x10080023)
-#define db5500_add_spi2(pdata) \
-	dbx500_add_spi("spi2", U5500_SPI2_BASE, IRQ_DB5500_SPI2, pdata \
+#define db5500_add_spi2(parent, pdata) \
+	dbx500_add_spi(parent, "spi2", U5500_SPI2_BASE, \
+		       IRQ_DB5500_SPI2, pdata		\
 		       0x10080023)
-#define db5500_add_spi3(pdata) \
-	dbx500_add_spi("spi3", U5500_SPI3_BASE, IRQ_DB5500_SPI3, pdata \
+#define db5500_add_spi3(parent, pdata) \
+	dbx500_add_spi(parent, "spi3", U5500_SPI3_BASE, \
+		       IRQ_DB5500_SPI3, pdata		\
 		       0x10080023)
 
-#define db5500_add_uart0(plat) \
-	dbx500_add_uart("uart0", U5500_UART0_BASE, IRQ_DB5500_UART0, plat)
-#define db5500_add_uart1(plat) \
-	dbx500_add_uart("uart1", U5500_UART1_BASE, IRQ_DB5500_UART1, plat)
-#define db5500_add_uart2(plat) \
-	dbx500_add_uart("uart2", U5500_UART2_BASE, IRQ_DB5500_UART2, plat)
-#define db5500_add_uart3(plat) \
-	dbx500_add_uart("uart3", U5500_UART3_BASE, IRQ_DB5500_UART3, plat)
+#define db5500_add_uart0(parent, plat) \
+	dbx500_add_uart(parent, "uart0", U5500_UART0_BASE, \
+			IRQ_DB5500_UART0, plat)
+#define db5500_add_uart1(parent, plat) \
+	dbx500_add_uart(parent, "uart1", U5500_UART1_BASE, \
+			IRQ_DB5500_UART1, plat)
+#define db5500_add_uart2(parent, plat) \
+	dbx500_add_uart(parent, "uart2", U5500_UART2_BASE, \
+			IRQ_DB5500_UART2, plat)
+#define db5500_add_uart3(parent, plat) \
+	dbx500_add_uart(parent, "uart3", U5500_UART3_BASE, \
+			IRQ_DB5500_UART3, plat)
 
 #endif
diff --git a/arch/arm/mach-ux500/devices-db8500.h b/arch/arm/mach-ux500/devices-db8500.h
index cbd4a9a..9bd08ad 100644
--- a/arch/arm/mach-ux500/devices-db8500.h
+++ b/arch/arm/mach-ux500/devices-db8500.h
@@ -14,7 +14,9 @@ struct ske_keypad_platform_data;
 struct pl022_ssp_controller;
 
 static inline struct platform_device *
-db8500_add_ske_keypad(struct ske_keypad_platform_data *pdata)
+db8500_add_ske_keypad(struct device *parent,
+		      struct ske_keypad_platform_data *pdata,
+		      size_t size)
 {
 	return dbx500_add_platform_device_4k1irq("nmk-ske-keypad", -1,
 						 U8500_SKE_BASE,
@@ -22,80 +24,100 @@ db8500_add_ske_keypad(struct ske_keypad_platform_data *pdata)
 }
 
 static inline struct amba_device *
-db8500_add_ssp(const char *name, resource_size_t base, int irq,
-	       struct pl022_ssp_controller *pdata)
+db8500_add_ssp(struct device *parent, const char *name, resource_size_t base,
+	       int irq, struct pl022_ssp_controller *pdata)
 {
-	return dbx500_add_amba_device(name, base, irq, pdata, 0);
+	return dbx500_add_amba_device(parent, name, base, irq, pdata, 0);
 }
 
 
-#define db8500_add_i2c0(pdata) \
-	dbx500_add_i2c(0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata)
-#define db8500_add_i2c1(pdata) \
-	dbx500_add_i2c(1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata)
-#define db8500_add_i2c2(pdata) \
-	dbx500_add_i2c(2, U8500_I2C2_BASE, IRQ_DB8500_I2C2, pdata)
-#define db8500_add_i2c3(pdata) \
-	dbx500_add_i2c(3, U8500_I2C3_BASE, IRQ_DB8500_I2C3, pdata)
-#define db8500_add_i2c4(pdata) \
-	dbx500_add_i2c(4, U8500_I2C4_BASE, IRQ_DB8500_I2C4, pdata)
-
-#define db8500_add_msp0_i2s(pdata) \
-	dbx500_add_msp_i2s(0, U8500_MSP0_BASE, IRQ_DB8500_MSP0, pdata)
-#define db8500_add_msp1_i2s(pdata) \
-	dbx500_add_msp_i2s(1, U8500_MSP1_BASE, IRQ_DB8500_MSP1, pdata)
-#define db8500_add_msp2_i2s(pdata) \
-	dbx500_add_msp_i2s(2, U8500_MSP2_BASE, IRQ_DB8500_MSP2, pdata)
-#define db8500_add_msp3_i2s(pdata) \
-	dbx500_add_msp_i2s(3, U8500_MSP3_BASE, IRQ_DB8500_MSP1, pdata)
-
-#define db8500_add_msp0_spi(pdata) \
-	dbx500_add_msp_spi("msp0", U8500_MSP0_BASE, IRQ_DB8500_MSP0, pdata)
-#define db8500_add_msp1_spi(pdata) \
-	dbx500_add_msp_spi("msp1", U8500_MSP1_BASE, IRQ_DB8500_MSP1, pdata)
-#define db8500_add_msp2_spi(pdata) \
-	dbx500_add_msp_spi("msp2", U8500_MSP2_BASE, IRQ_DB8500_MSP2, pdata)
-#define db8500_add_msp3_spi(pdata) \
-	dbx500_add_msp_spi("msp3", U8500_MSP3_BASE, IRQ_DB8500_MSP1, pdata)
-
-#define db8500_add_rtc() \
-	dbx500_add_rtc(U8500_RTC_BASE, IRQ_DB8500_RTC);
-
-#define db8500_add_usb(rx_cfg, tx_cfg) \
-	ux500_add_usb(U8500_USBOTG_BASE, IRQ_DB8500_USBOTG, rx_cfg, tx_cfg)
-
-#define db8500_add_sdi0(pdata, pid) \
-	dbx500_add_sdi("sdi0", U8500_SDI0_BASE, IRQ_DB8500_SDMMC0, pdata, pid)
-#define db8500_add_sdi1(pdata, pid) \
-	dbx500_add_sdi("sdi1", U8500_SDI1_BASE, IRQ_DB8500_SDMMC1, pdata, pid)
-#define db8500_add_sdi2(pdata, pid) \
-	dbx500_add_sdi("sdi2", U8500_SDI2_BASE, IRQ_DB8500_SDMMC2, pdata, pid)
-#define db8500_add_sdi3(pdata, pid) \
-	dbx500_add_sdi("sdi3", U8500_SDI3_BASE, IRQ_DB8500_SDMMC3, pdata, pid)
-#define db8500_add_sdi4(pdata, pid) \
-	dbx500_add_sdi("sdi4", U8500_SDI4_BASE, IRQ_DB8500_SDMMC4, pdata, pid)
-#define db8500_add_sdi5(pdata, pid) \
-	dbx500_add_sdi("sdi5", U8500_SDI5_BASE, IRQ_DB8500_SDMMC5, pdata, pid)
-
-#define db8500_add_ssp0(pdata) \
-	db8500_add_ssp("ssp0", U8500_SSP0_BASE, IRQ_DB8500_SSP0, pdata)
-#define db8500_add_ssp1(pdata) \
-	db8500_add_ssp("ssp1", U8500_SSP1_BASE, IRQ_DB8500_SSP1, pdata)
-
-#define db8500_add_spi0(pdata) \
-	dbx500_add_spi("spi0", U8500_SPI0_BASE, IRQ_DB8500_SPI0, pdata, 0)
-#define db8500_add_spi1(pdata) \
-	dbx500_add_spi("spi1", U8500_SPI1_BASE, IRQ_DB8500_SPI1, pdata, 0)
-#define db8500_add_spi2(pdata) \
-	dbx500_add_spi("spi2", U8500_SPI2_BASE, IRQ_DB8500_SPI2, pdata, 0)
-#define db8500_add_spi3(pdata) \
-	dbx500_add_spi("spi3", U8500_SPI3_BASE, IRQ_DB8500_SPI3, pdata, 0)
-
-#define db8500_add_uart0(pdata) \
-	dbx500_add_uart("uart0", U8500_UART0_BASE, IRQ_DB8500_UART0, pdata)
-#define db8500_add_uart1(pdata) \
-	dbx500_add_uart("uart1", U8500_UART1_BASE, IRQ_DB8500_UART1, pdata)
-#define db8500_add_uart2(pdata) \
-	dbx500_add_uart("uart2", U8500_UART2_BASE, IRQ_DB8500_UART2, pdata)
+#define db8500_add_i2c0(parent, pdata) \
+	dbx500_add_i2c(parent, 0, U8500_I2C0_BASE, IRQ_DB8500_I2C0, pdata)
+#define db8500_add_i2c1(parent, pdata) \
+	dbx500_add_i2c(parent, 1, U8500_I2C1_BASE, IRQ_DB8500_I2C1, pdata)
+#define db8500_add_i2c2(parent, pdata) \
+	dbx500_add_i2c(parent, 2, U8500_I2C2_BASE, IRQ_DB8500_I2C2, pdata)
+#define db8500_add_i2c3(parent, pdata) \
+	dbx500_add_i2c(parent, 3, U8500_I2C3_BASE, IRQ_DB8500_I2C3, pdata)
+#define db8500_add_i2c4(parent, pdata) \
+	dbx500_add_i2c(parent, 4, U8500_I2C4_BASE, IRQ_DB8500_I2C4, pdata)
+
+#define db8500_add_msp0_i2s(parent, pdata) \
+	dbx500_add_msp_i2s(parent, 0, U8500_MSP0_BASE, IRQ_DB8500_MSP0, pdata)
+#define db8500_add_msp1_i2s(parent, pdata) \
+	dbx500_add_msp_i2s(parent, 1, U8500_MSP1_BASE, IRQ_DB8500_MSP1, pdata)
+#define db8500_add_msp2_i2s(parent, pdata) \
+	dbx500_add_msp_i2s(parent, 2, U8500_MSP2_BASE, IRQ_DB8500_MSP2, pdata)
+#define db8500_add_msp3_i2s(parent, pdata) \
+	dbx500_add_msp_i2s(parent, 3, U8500_MSP3_BASE, IRQ_DB8500_MSP1, pdata)
+
+#define db8500_add_msp0_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp0", U8500_MSP0_BASE, \
+			   IRQ_DB8500_MSP0, pdata)
+#define db8500_add_msp1_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp1", U8500_MSP1_BASE, \
+			   IRQ_DB8500_MSP1, pdata)
+#define db8500_add_msp2_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp2", U8500_MSP2_BASE, \
+			   IRQ_DB8500_MSP2, pdata)
+#define db8500_add_msp3_spi(parent, pdata) \
+	dbx500_add_msp_spi(parent, "msp3", U8500_MSP3_BASE, \
+			   IRQ_DB8500_MSP1, pdata)
+
+#define db8500_add_rtc(parent) \
+	dbx500_add_rtc(parent, U8500_RTC_BASE, IRQ_DB8500_RTC);
+
+#define db8500_add_usb(parent, rx_cfg, tx_cfg) \
+	ux500_add_usb(parent, U8500_USBOTG_BASE, \
+		      IRQ_DB8500_USBOTG, rx_cfg, tx_cfg)
+
+#define db8500_add_sdi0(parent, pdata, pid) \
+	dbx500_add_sdi(parent, "sdi0", U8500_SDI0_BASE, \
+		       IRQ_DB8500_SDMMC0, pdata, pid)
+#define db8500_add_sdi1(parent, pdata, pid) \
+	dbx500_add_sdi(parent, "sdi1", U8500_SDI1_BASE, \
+		       IRQ_DB8500_SDMMC1, pdata, pid)
+#define db8500_add_sdi2(parent, pdata, pid) \
+	dbx500_add_sdi(parent, "sdi2", U8500_SDI2_BASE, \
+		       IRQ_DB8500_SDMMC2, pdata, pid)
+#define db8500_add_sdi3(parent, pdata, pid) \
+	dbx500_add_sdi(parent, "sdi3", U8500_SDI3_BASE, \
+		       IRQ_DB8500_SDMMC3, pdata, pid)
+#define db8500_add_sdi4(parent, pdata, pid) \
+	dbx500_add_sdi(parent, "sdi4", U8500_SDI4_BASE, \
+		       IRQ_DB8500_SDMMC4, pdata, pid)
+#define db8500_add_sdi5(parent, pdata, pid) \
+	dbx500_add_sdi(parent, "sdi5", U8500_SDI5_BASE, \
+		       IRQ_DB8500_SDMMC5, pdata, pid)
+
+#define db8500_add_ssp0(parent, pdata) \
+	db8500_add_ssp(parent, "ssp0", U8500_SSP0_BASE, \
+		       IRQ_DB8500_SSP0, pdata)
+#define db8500_add_ssp1(parent, pdata) \
+	db8500_add_ssp(parent, "ssp1", U8500_SSP1_BASE, \
+		       IRQ_DB8500_SSP1, pdata)
+
+#define db8500_add_spi0(parent, pdata) \
+	dbx500_add_spi(parent, "spi0", U8500_SPI0_BASE, \
+		       IRQ_DB8500_SPI0, pdata, 0)
+#define db8500_add_spi1(parent, pdata) \
+	dbx500_add_spi(parent, "spi1", U8500_SPI1_BASE, \
+		       IRQ_DB8500_SPI1, pdata, 0)
+#define db8500_add_spi2(parent, pdata) \
+	dbx500_add_spi(parent, "spi2", U8500_SPI2_BASE, \
+		       IRQ_DB8500_SPI2, pdata, 0)
+#define db8500_add_spi3(parent, pdata) \
+	dbx500_add_spi(parent, "spi3", U8500_SPI3_BASE, \
+		       IRQ_DB8500_SPI3, pdata, 0)
+
+#define db8500_add_uart0(parent, pdata) \
+	dbx500_add_uart(parent, "uart0", U8500_UART0_BASE, \
+			IRQ_DB8500_UART0, pdata)
+#define db8500_add_uart1(parent, pdata) \
+	dbx500_add_uart(parent, "uart1", U8500_UART1_BASE, \
+			IRQ_DB8500_UART1, pdata)
+#define db8500_add_uart2(parent, pdata) \
+	dbx500_add_uart(parent, "uart2", U8500_UART2_BASE, \
+			IRQ_DB8500_UART2, pdata)
 
 #endif
diff --git a/arch/arm/mach-ux500/dma-db5500.c b/arch/arm/mach-ux500/dma-db5500.c
index 1cfab68..41e9470 100644
--- a/arch/arm/mach-ux500/dma-db5500.c
+++ b/arch/arm/mach-ux500/dma-db5500.c
@@ -125,10 +125,11 @@ static struct platform_device dma40_device = {
 	.resource	= dma40_resources
 };
 
-void __init db5500_dma_init(void)
+void __init db5500_dma_init(struct device *parent)
 {
 	int ret;
 
+	dma40_device.dev.parent = parent;
 	ret = platform_device_register(&dma40_device);
 	if (ret)
 		dev_err(&dma40_device.dev, "unable to register device: %d\n", ret);
diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
index a7d363f..e46b8b1 100644
--- a/arch/arm/mach-ux500/include/mach/setup.h
+++ b/arch/arm/mach-ux500/include/mach/setup.h
@@ -18,14 +18,14 @@ void __init ux500_map_io(void);
 extern void __init u5500_map_io(void);
 extern void __init u8500_map_io(void);
 
-extern void __init u5500_init_devices(void);
-extern void __init u8500_init_devices(void);
+extern struct device * __init u5500_init_devices(void);
+extern struct device * __init u8500_init_devices(void);
 
 extern void __init ux500_init_irq(void);
 
-extern void __init u5500_sdi_init(void);
+extern void __init u5500_sdi_init(struct device *parent);
 
-extern void __init db5500_dma_init(void);
+extern void __init db5500_dma_init(struct device *parent);
 
 /* We re-use nomadik_timer for this platform */
 extern void nmdk_timer_init(void);
diff --git a/arch/arm/mach-ux500/include/mach/usb.h b/arch/arm/mach-ux500/include/mach/usb.h
index d3739d4..4c1cc50 100644
--- a/arch/arm/mach-ux500/include/mach/usb.h
+++ b/arch/arm/mach-ux500/include/mach/usb.h
@@ -20,6 +20,6 @@ struct ux500_musb_board_data {
 	bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
 };
 
-void ux500_add_usb(resource_size_t base, int irq, int *dma_rx_cfg,
-	int *dma_tx_cfg);
+void ux500_add_usb(struct device *parent, resource_size_t base,
+		   int irq, int *dma_rx_cfg, int *dma_tx_cfg);
 #endif
diff --git a/arch/arm/mach-ux500/usb.c b/arch/arm/mach-ux500/usb.c
index 0a01cbd..bef6a31 100644
--- a/arch/arm/mach-ux500/usb.c
+++ b/arch/arm/mach-ux500/usb.c
@@ -146,8 +146,8 @@ static inline void ux500_usb_dma_update_tx_ch_config(int *dst_dev_type)
 		musb_dma_tx_ch[idx].dst_dev_type = dst_dev_type[idx];
 }
 
-void ux500_add_usb(resource_size_t base, int irq, int *dma_rx_cfg,
-	int *dma_tx_cfg)
+void ux500_add_usb(struct device *parent, resource_size_t base, int irq,
+		   int *dma_rx_cfg, int *dma_tx_cfg)
 {
 	ux500_musb_device.resource[0].start = base;
 	ux500_musb_device.resource[0].end = base + SZ_64K - 1;
-- 
1.7.5.4

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

Traditionally, any System-on-Chip based platform creates a flat list
of platform_devices directly under /sys/devices/platform.

In order to give these some better structure, this introduces a new
bus type for soc_devices that are registered with the new
soc_device_register() function.  All devices that are on the same
chip should then be registered as child devices of the soc device.

The soc bus also exports a few standardised device attributes which
allow user space to query the specific type of soc.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig   |    3 +
 drivers/base/Makefile  |    1 +
 drivers/base/soc.c     |  186 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/syssoc.h |   74 +++++++++++++++++++
 4 files changed, 264 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/syssoc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 7be9f79..9aa618a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
 	bool
 	default n
 
+config SOC_BUS
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 config DMA_SHARED_BUFFER
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 2c8272d..4f302a6 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SOC_BUS) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..29abf6d
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,186 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <linux/spinlock.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+
+static struct ida soc_ida;
+static spinlock_t soc_lock;
+
+/* Required: soc_info_get needs to see the DEVICE_ATTR's, which   */
+/*           in turn need to know about the function's existence. */
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+
+struct soc_device {
+	struct device dev;
+	const struct soc_device_attribute *attr;
+};
+
+static struct bus_type soc_bus_type = {
+	.name  = "soc",
+};
+
+static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+
+struct device *soc_device_to_device(struct soc_device *soc_dev)
+{
+	return &soc_dev->dev;
+}
+
+static mode_t soc_attribute_mode(struct kobject *kobj,
+                                 struct attribute *attr,
+                                 int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if ((attr == &dev_attr_machine.attr)
+	    && (soc_dev->attr->machine != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_family.attr)
+	    && (soc_dev->attr->family != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_revision.attr)
+	    && (soc_dev->attr->revision != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_soc_id.attr)
+	    && (soc_dev->attr->soc_id != NULL))
+	        return attr->mode;
+
+	/* Unknown or unfilled attribute. */
+	return 0;
+}
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if (attr == &dev_attr_machine)
+		return sprintf(buf, "%s\n", soc_dev->attr->machine);
+	if (attr == &dev_attr_family)
+		return sprintf(buf, "%s\n", soc_dev->attr->family);
+	if (attr == &dev_attr_revision)
+		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_soc_id)
+		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+
+	return -EINVAL;
+
+}
+
+static struct attribute *soc_attr[] = {
+	&dev_attr_machine.attr,
+	&dev_attr_family.attr,
+	&dev_attr_soc_id.attr,
+	&dev_attr_revision.attr,
+	NULL,
+};
+
+static const struct attribute_group soc_attr_group = {
+	.attrs = soc_attr,
+	.is_visible = soc_attribute_mode,
+};
+
+static const struct attribute_group *soc_attr_groups[] = {
+	&soc_attr_group,
+	NULL,
+};
+
+static void soc_release(struct device *dev)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	kfree(soc_dev);
+}
+
+struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
+{
+	struct soc_device *soc_dev;
+	int ret;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (!soc_dev) {
+	        ret = -ENOMEM;
+		goto out1;
+	}
+
+	/* Fetch a unique (reclaimable) SOC ID. */
+	do {
+		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
+			ret = -ENOMEM;
+			goto out2;
+		}
+
+		spin_lock(&soc_lock);
+		ret = ida_get_new(&soc_ida, &soc_dev->id);
+		spin_unlock(&soc_lock);
+
+	} while (ret == -EAGAIN);
+
+	if (ret)
+	         goto out2;
+
+	soc_dev->attr = soc_dev_attr;
+	soc_dev->dev.bus = &soc_bus_type;
+	soc_dev->dev.groups = soc_attr_groups;
+	soc_dev->dev.release = soc_release;
+
+	dev_set_name(&soc_dev->dev, "soc%d", soc_dev->id);
+
+	ret = device_register(&soc_dev->dev);
+	if (ret)
+		goto out3;
+
+	return soc_dev;
+
+out3:
+	ida_remove(&soc_ida, soc_dev->id);
+out2:
+	kfree(soc_dev);
+out1:
+	return ERR_PTR(ret);
+}
+
+/* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
+void soc_device_unregister(struct soc_device *soc_dev)
+{
+	ida_remove(&soc_ida, soc_dev->id);
+
+	device_unregister(&soc_dev->dev);
+}
+
+static int __init soc_bus_register(void)
+{
+	spin_lock_init(&soc_lock);
+
+	ida_init(&soc_ida);
+
+	return bus_register(&soc_bus_type);
+}
+core_initcall(soc_bus_register);
+
+static void __exit soc_bus_unregister(void)
+{
+	ida_destroy(&soc_ida);
+
+	bus_unregister(&soc_bus_type);
+}
+module_exit(soc_bus_unregister);
diff --git a/include/linux/syssoc.h b/include/linux/syssoc.h
new file mode 100644
index 0000000..340685c
--- /dev/null
+++ b/include/linux/syssoc.h
@@ -0,0 +1,74 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef __SOC_BUS_H
+#define __SOC_BUS_H
+
+#include <linux/device.h>
+
+struct soc_device_attribute {
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *soc_id;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
+ *
+ * soc_device_register is passed a selection of predefined attributes. Each
+ * detail a particular piece of information pertaining to the System-on-Chip
+ * (SoC) which the code is currently executing on. It is the caller's
+ * responsibility to allocate memory and populate this structure prior to passing
+ * as well as freeing on failure.
+ *
+ * More information surrounding the attributes can be found in:
+ *   'Documentation/ABI/testing/sysfs-devices-soc'.
+ *
+ * Each SoC in the system should call this function once. It will be subsequently
+ * allocated a SoC ID (in order of appearance - so the first SoC on the system
+ * may not necessarily be soc0 in sysfs) and registered.
+ *
+ * If successful, soc_device_register will return a populated soc_device
+ * (definition located in 'drivers/base/soc.c'), which the calling code can then
+ * use as a parent after passing it through soc_device_to_device.
+ *
+ */
+struct soc_device *soc_device_register(
+	const struct soc_device_attribute *soc_plat_dev_attr);
+
+/**
+ * soc_device_unregister - unregister SoC device
+ * @dev: SoC device to be unregistered
+ *
+ * soc_device_unregister only requires the soc_device which was obtained from
+ * soc_device_register. In current architectures this is an unused function call
+ * as SoCs are presently non-removable.
+ */
+void soc_device_unregister(struct soc_device *soc_dev);
+
+/**
+ * soc_device_to_device - helper function to fetch struct device
+ * @soc: Previously registered SoC device container
+ *
+ * Up until the existence of this driver, SoC devices were registered as "grab bag"
+ * platform_devices. Now, once a SoC registers itself with this driver, it will be
+ * provided a soc_device. This has the potential to act as a parent for all SoC
+ * devices and provides a sane bus for SoC devices to be hooked up to.
+ *
+ * When soc_device_to_device is provided with the soc_device returned by
+ * soc_device_register a suitable parent device will be passed back. This may be
+ * used as a suitable parent for all devices located on the current (calling) SoC.
+ *
+ * This function provides a simple way to extract the parent pointer from
+ * soc_device. No reference counter is incremented during the process, so it is
+ * considered safe to call it as many times as required without unnecessary
+ * overhead.
+ */
+struct device *soc_device_to_device(struct soc_device *soc);
+
+#endif /* __SOC_BUS_H */
-- 
1.7.5.4

[PATCH 3/6] Documentation: add information for new sysfs soc bus functionality

[Lee Jones]

This change applies the required documentation for each new
attribute recenty added by the new System-On-Chip (SoC)
information export bus driver.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/ABI/testing/sysfs-devices-soc |   58 +++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-soc

diff --git a/Documentation/ABI/testing/sysfs-devices-soc b/Documentation/ABI/testing/sysfs-devices-soc
new file mode 100644
index 0000000..6d9cc25
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-soc
@@ -0,0 +1,58 @@
+What:		/sys/devices/socX
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		The /sys/devices/ directory contains a sub-directory for each
+		System-on-Chip (SoC) device on a running platform. Information
+		regarding each SoC can be obtained by reading sysfs files. This
+		functionality is only available if implemented by the platform.
+
+		The directory created for each SoC will also house information
+		about devices which are commonly contained in /sys/devices/platform.
+		It has been agreed that if an SoC device exists, its supported
+		devices would be better suited to appear as children of that SoC.
+
+What:		/sys/devices/socX/machine
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		Read-only attribute common to all SoCs. Contains the SoC machine
+		name (e.g. Ux500).
+
+What:		/sys/devices/socX/family
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		Read-only attribute common to all SoCs. Contains SoC family name
+		(e.g. DB8500).
+
+What:		/sys/devices/socX/soc_id
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		Read-only attribute supported by most SoCs. In the case of
+		ST-Ericsson's chips this contains the SoC serial number.
+
+What:		/sys/devices/socX/revision
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		Read-only attribute supported by most SoCs. Contains the SoC's
+		manufacturing revision number.
+
+What:		/sys/devices/socX/process
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		Read-only attribute supported ST-Ericsson's silicon. Contains the
+		the process by which the silicon chip was manufactured.
+
+What:		/sys/bus/soc
+Date:		January 2012
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		The /sys/bus/soc/ directory contains the usual sub-folders
+		expected under most buses. /sys/bus/soc/devices is of particular
+		interest, as it contains a symlink for each SoC device found on
+		the system. Each symlink points back into the aforementioned
+		/sys/devices/socX devices.
-- 
1.7.5.4

[PATCH 4/6] mach-ux500: export System-on-Chip information ux500 via sysfs

[Lee Jones]

Here we make use of the new System-On-Chip bus driver to export
vital SoC information out to userspace via sysfs. This patch
provides a data structure of strings to populate the base
nodes found in:

/sys/devices/soc[0|1|2|...]/[family|machine|revision|soc_id].

It also adds one more node as requested by ST-Ericsson.
'process' depicts the way in which the silicon was manufactured.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig                    |    1 +
 arch/arm/mach-ux500/cpu-db5500.c               |   30 +++++++---
 arch/arm/mach-ux500/cpu-db8500.c               |   33 +++++++++--
 arch/arm/mach-ux500/cpu.c                      |   75 ++++++++++++++++++++++++
 arch/arm/mach-ux500/include/mach/db8500-regs.h |    3 +
 arch/arm/mach-ux500/include/mach/setup.h       |    2 +
 6 files changed, 130 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index a3e0c86..ce8bac2 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -27,6 +27,7 @@ config MACH_U8500
 	bool "U8500 Development platform"
 	depends on UX500_SOC_DB8500
 	select TPS6105X
+	select SOC_BUS
 	help
 	  Include support for the mop500 development platform.
 
diff --git a/arch/arm/mach-ux500/cpu-db5500.c b/arch/arm/mach-ux500/cpu-db5500.c
index c402fd6..8f8acfd 100644
--- a/arch/arm/mach-ux500/cpu-db5500.c
+++ b/arch/arm/mach-ux500/cpu-db5500.c
@@ -212,18 +212,32 @@ static int usb_db5500_tx_dma_cfg[] = {
 	DB5500_DMA_DEV38_USB_OTG_OEP_8
 };
 
-struct device* __init u5500_init_devices(void)
+static const char *db5500_read_soc_id(void)
 {
-	/* FIXME: First parameter to be a real parent. */
-	db5500_add_gpios(NULL);
+	return kasprintf(GFP_KERNEL, "u5500 currently unsupported\n");
+}
+
+static struct device * __init db5500_soc_device_init(void)
+{
+	const char *soc_id = db5500_read_soc_id();
+
+	return ux500_soc_device_init(soc_id);
+}
+
+struct device * __init u5500_init_devices(void)
+{
+	struct device *parent;
+
+	parent = db5500_soc_device_init();
+
+	db5500_add_gpios(parent);
 	db5500_pmu_init();
-	db5500_dma_init(NULL);
-	db5500_add_rtc(NULL);
-	db5500_add_usb(NULL, usb_db5500_rx_dma_cfg, usb_db5500_tx_dma_cfg);
+	db5500_dma_init(parent);
+	db5500_add_rtc(parent);
+	db5500_add_usb(parent, usb_db5500_rx_dma_cfg, usb_db5500_tx_dma_cfg);
 
 	platform_add_devices(db5500_platform_devs,
 			     ARRAY_SIZE(db5500_platform_devs));
 
-	/* FIXME: Return value to be a real parent. */
-	return NULL;
+	return parent;
 }
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index 1e8a2cb..afcde3d 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -24,6 +24,7 @@
 #include <mach/setup.h>
 #include <mach/devices.h>
 #include <mach/usb.h>
+#include <mach/db8500-regs.h>
 
 #include "devices-db8500.h"
 #include "ste-dma40-db8500.h"
@@ -164,18 +165,38 @@ static int usb_db8500_tx_dma_cfg[] = {
 	DB8500_DMA_DEV39_USB_OTG_OEP_8
 };
 
+static const char *db8500_read_soc_id(void)
+{
+	void __iomem *uid = __io_address(U8500_BB_UID_BASE);
+
+	return kasprintf(GFP_KERNEL, "%08x%08x%08x%08x%08x",
+			 readl((u32 *)uid+1),
+			 readl((u32 *)uid+1), readl((u32 *)uid+2),
+			 readl((u32 *)uid+3), readl((u32 *)uid+4));
+}
+
+static struct device * __init db8500_soc_device_init(void)
+{
+	const char *soc_id = db8500_read_soc_id();
+
+	return ux500_soc_device_init(soc_id);
+}
+
 /*
  * This function is called from the board init
  */
-struct device* __init u8500_init_devices(void)
+struct device * __init u8500_init_devices(void)
 {
-	db8500_add_rtc(NULL);
-	db8500_add_gpios(NULL);
-	db8500_add_usb(NULL, usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
+	struct device *parent;
+
+	parent = db8500_soc_device_init();
+
+	db8500_add_rtc(parent);
+	db8500_add_gpios(parent);
+	db8500_add_usb(parent, usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
 
 	platform_device_register_simple("cpufreq-u8500", -1, NULL, 0);
 	platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
 
-	/* FIXME: Return value to be a real parent. */
-	return NULL;
+	return parent;
 }
diff --git a/arch/arm/mach-ux500/cpu.c b/arch/arm/mach-ux500/cpu.c
index f418574..055fb6e 100644
--- a/arch/arm/mach-ux500/cpu.c
+++ b/arch/arm/mach-ux500/cpu.c
@@ -2,6 +2,7 @@
  * Copyright (C) ST-Ericsson SA 2010
  *
  * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson
  * License terms: GNU General Public License (GPL) version 2
  */
 
@@ -11,6 +12,10 @@
 #include <linux/mfd/db8500-prcmu.h>
 #include <linux/mfd/db5500-prcmu.h>
 #include <linux/clksrc-dbx500-prcmu.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/stat.h>
 
 #include <asm/hardware/gic.h>
 #include <asm/mach/map.h>
@@ -50,3 +55,73 @@ void __init ux500_init_irq(void)
 		db8500_prcmu_early_init();
 	clk_init();
 }
+
+static const char * __init ux500_get_machine(void)
+{
+	return kasprintf(GFP_KERNEL, "DB%4x", dbx500_partnumber());
+}
+
+static const char * __init ux500_get_family(void)
+{
+	return kasprintf(GFP_KERNEL, "ux500");
+}
+
+static const char * __init ux500_get_revision(void)
+{
+	unsigned int rev = dbx500_revision();
+
+	if (rev == 0x01)
+		return kasprintf(GFP_KERNEL, "%s", "ED");
+	else if (rev >= 0xA0)
+		return kasprintf(GFP_KERNEL, "%d.%d",
+				 (rev >> 4) - 0xA + 1, rev & 0xf);
+
+	return kasprintf(GFP_KERNEL, "%s", "Unknown");
+}
+
+static ssize_t ux500_get_process(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	if (dbx500_id.process == 0x00)
+		return sprintf(buf, "Standard\n");
+
+	return sprintf(buf, "%02xnm\n", dbx500_id.process);
+}
+
+static void __init soc_info_populate(struct soc_device_attribute *soc_dev_attr,
+				     const char *soc_id)
+{
+	soc_dev_attr->soc_id   = soc_id;
+	soc_dev_attr->machine  = ux500_get_machine();
+	soc_dev_attr->family   = ux500_get_family();
+	soc_dev_attr->revision = ux500_get_revision();
+}
+
+struct device_attribute ux500_soc_attr =
+	__ATTR(process,  S_IRUGO, ux500_get_process,  NULL);
+
+struct device * __init ux500_soc_device_init(const char *soc_id)
+{
+	struct device *parent;
+	struct soc_device *soc_dev;
+	struct soc_device_attribute *soc_dev_attr;
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr)
+		return ERR_PTR(-ENOMEM);
+
+	soc_info_populate(soc_dev_attr, soc_id);
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR_OR_NULL(soc_dev)) {
+	        kfree(soc_dev_attr);
+		return NULL;
+	}
+
+	parent = soc_device_to_device(soc_dev);
+	if (!IS_ERR_OR_NULL(parent))
+		device_create_file(parent, &ux500_soc_attr);
+
+	return parent;
+}
diff --git a/arch/arm/mach-ux500/include/mach/db8500-regs.h b/arch/arm/mach-ux500/include/mach/db8500-regs.h
index 80e10f5..9ec20b9 100644
--- a/arch/arm/mach-ux500/include/mach/db8500-regs.h
+++ b/arch/arm/mach-ux500/include/mach/db8500-regs.h
@@ -161,4 +161,7 @@
 #define U8500_MODEM_BASE	0xe000000
 #define U8500_APE_BASE		0x6000000
 
+/* SoC identification number information */
+#define U8500_BB_UID_BASE      (U8500_BACKUPRAM1_BASE + 0xFC0)
+
 #endif
diff --git a/arch/arm/mach-ux500/include/mach/setup.h b/arch/arm/mach-ux500/include/mach/setup.h
index e46b8b1..74b43bb 100644
--- a/arch/arm/mach-ux500/include/mach/setup.h
+++ b/arch/arm/mach-ux500/include/mach/setup.h
@@ -27,6 +27,8 @@ extern void __init u5500_sdi_init(struct device *parent);
 
 extern void __init db5500_dma_init(struct device *parent);
 
+extern struct device *ux500_soc_device_init(const char *soc_id);
+
 /* We re-use nomadik_timer for this platform */
 extern void nmdk_timer_init(void);
 
-- 
1.7.5.4

[PATCH 5/6] mach-ux500: move top level platform devices in sysfs to /sys/devices/socX

[Lee Jones]

At the request of Arnd Bergmann this patch moves all SoC
platform devices found in sysfs from /sys/devices/platform to
/sys/devices/soc<SoCNum>/. It is believed as the devices are
SoC specific and a /sys/devices/soc node has recently
become available, that this would be a more appropriate place to
display the data.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/board-mop500.c   |   12 ++++++++++++
 arch/arm/mach-ux500/board-u5500.c    |    4 ++++
 arch/arm/mach-ux500/cpu-db5500.c     |    4 ++++
 arch/arm/mach-ux500/cpu-db8500.c     |    8 +++++++-
 arch/arm/mach-ux500/devices-common.c |   13 ++++++++++---
 arch/arm/mach-ux500/usb.c            |    3 +++
 6 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index f9ce2a1..04afcdf 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -610,6 +610,7 @@ static void __init mop500_init_machine(void)
 {
 	struct device *parent = NULL;
 	int i2c0_devs;
+	int i;
 
 	mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
@@ -617,6 +618,9 @@ static void __init mop500_init_machine(void)
 
 	mop500_pins_init();
 
+	for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
+		mop500_platform_devs[i]->dev.parent = parent;
+
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
 
@@ -639,11 +643,15 @@ static void __init snowball_init_machine(void)
 {
 	struct device *parent = NULL;
 	int i2c0_devs;
+	int i;
 
 	parent = u8500_init_devices();
 
 	snowball_pins_init();
 
+	for (i = 0; i < ARRAY_SIZE(snowball_platform_devs); i++)
+		snowball_platform_devs[i]->dev.parent = parent;
+
 	platform_add_devices(snowball_platform_devs,
 			ARRAY_SIZE(snowball_platform_devs));
 
@@ -665,6 +673,7 @@ static void __init hrefv60_init_machine(void)
 {
 	struct device *parent = NULL;
 	int i2c0_devs;
+	int i;
 
 	/*
 	 * The HREFv60 board removed a GPIO expander and routed
@@ -677,6 +686,9 @@ static void __init hrefv60_init_machine(void)
 
 	hrefv60_pins_init();
 
+	for (i = 0; i < ARRAY_SIZE(mop500_platform_devs); i++)
+		mop500_platform_devs[i]->dev.parent = parent;
+
 	platform_add_devices(mop500_platform_devs,
 			ARRAY_SIZE(mop500_platform_devs));
 
diff --git a/arch/arm/mach-ux500/board-u5500.c b/arch/arm/mach-ux500/board-u5500.c
index d7a9596..0ff4be7 100644
--- a/arch/arm/mach-ux500/board-u5500.c
+++ b/arch/arm/mach-ux500/board-u5500.c
@@ -136,6 +136,7 @@ static void __init u5500_uart_init(struct device *parent)
 static void __init u5500_init_machine(void)
 {
 	struct device *parent = NULL;
+	int i;
 
 	parent = u5500_init_devices();
 	nmk_config_pins(u5500_pins, ARRAY_SIZE(u5500_pins));
@@ -144,6 +145,9 @@ static void __init u5500_init_machine(void)
 	u5500_sdi_init(parent);
 	u5500_uart_init(parent);
 
+	for (i = 0; i < ARRAY_SIZE(u5500_platform_devices); i++)
+		u5500_platform_devices[i]->dev.parent = parent;
+
 	platform_add_devices(u5500_platform_devices,
 		ARRAY_SIZE(u5500_platform_devices));
 }
diff --git a/arch/arm/mach-ux500/cpu-db5500.c b/arch/arm/mach-ux500/cpu-db5500.c
index 8f8acfd..bca47f3 100644
--- a/arch/arm/mach-ux500/cpu-db5500.c
+++ b/arch/arm/mach-ux500/cpu-db5500.c
@@ -227,6 +227,7 @@ static struct device * __init db5500_soc_device_init(void)
 struct device * __init u5500_init_devices(void)
 {
 	struct device *parent;
+	int i;
 
 	parent = db5500_soc_device_init();
 
@@ -236,6 +237,9 @@ struct device * __init u5500_init_devices(void)
 	db5500_add_rtc(parent);
 	db5500_add_usb(parent, usb_db5500_rx_dma_cfg, usb_db5500_tx_dma_cfg);
 
+	for (i = 0; i < ARRAY_SIZE(db5500_platform_devs); i++)
+		db5500_platform_devs[i]->dev.parent = parent;
+
 	platform_add_devices(db5500_platform_devs,
 			     ARRAY_SIZE(db5500_platform_devs));
 
diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
index afcde3d..9bd8163 100644
--- a/arch/arm/mach-ux500/cpu-db8500.c
+++ b/arch/arm/mach-ux500/cpu-db8500.c
@@ -188,6 +188,7 @@ static struct device * __init db8500_soc_device_init(void)
 struct device * __init u8500_init_devices(void)
 {
 	struct device *parent;
+	int i;
 
 	parent = db8500_soc_device_init();
 
@@ -195,7 +196,12 @@ struct device * __init u8500_init_devices(void)
 	db8500_add_gpios(parent);
 	db8500_add_usb(parent, usb_db8500_rx_dma_cfg, usb_db8500_tx_dma_cfg);
 
-	platform_device_register_simple("cpufreq-u8500", -1, NULL, 0);
+	platform_device_register_data(parent,
+		"cpufreq-u8500", -1, NULL, 0);
+
+	for (i = 0; i < ARRAY_SIZE(platform_devs); i++)
+		platform_devs[i]->dev.parent = parent;
+
 	platform_add_devices(platform_devs, ARRAY_SIZE(platform_devs));
 
 	return parent;
diff --git a/arch/arm/mach-ux500/devices-common.c b/arch/arm/mach-ux500/devices-common.c
index 96be248..96effbd 100644
--- a/arch/arm/mach-ux500/devices-common.c
+++ b/arch/arm/mach-ux500/devices-common.c
@@ -47,6 +47,8 @@ dbx500_add_amba_device(struct device *parent, const char *name,
 
 	dev->dev.platform_data = pdata;
 
+	dev->dev.parent = parent;
+
 	ret = amba_device_register(dev, &iomem_resource);
 	if (ret) {
 		kfree(dev);
@@ -126,9 +128,14 @@ dbx500_add_gpio(struct device *parent, int id, resource_size_t addr, int irq,
 		}
 	};
 
-	return platform_device_register_resndata(NULL, "gpio", id,
-				resources, ARRAY_SIZE(resources),
-				pdata, sizeof(*pdata));
+	return platform_device_register_resndata(
+		parent,
+		"gpio",
+		id,
+		resources,
+		ARRAY_SIZE(resources),
+		pdata,
+		sizeof(*pdata));
 }
 
 void dbx500_add_gpios(struct device *parent, resource_size_t *base, int num,
diff --git a/arch/arm/mach-ux500/usb.c b/arch/arm/mach-ux500/usb.c
index bef6a31..cdae63e 100644
--- a/arch/arm/mach-ux500/usb.c
+++ b/arch/arm/mach-ux500/usb.c
@@ -7,6 +7,7 @@
 #include <linux/platform_device.h>
 #include <linux/usb/musb.h>
 #include <linux/dma-mapping.h>
+
 #include <plat/ste_dma40.h>
 #include <mach/hardware.h>
 #include <mach/usb.h>
@@ -157,5 +158,7 @@ void ux500_add_usb(struct device *parent, resource_size_t base, int irq,
 	ux500_usb_dma_update_rx_ch_config(dma_rx_cfg);
 	ux500_usb_dma_update_tx_ch_config(dma_tx_cfg);
 
+	ux500_musb_device.dev.parent = parent;
+
 	platform_device_register(&ux500_musb_device);
 }
-- 
1.7.5.4

[PATCH 6/6] mach-ux500: remove intermediary add_platform_device* functions

[Lee Jones]

These are no longer required since a 'parent' pointer is now
passed to each registering device.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/devices-common.c |   53 ----------------------------------
 arch/arm/mach-ux500/devices-common.h |   46 ++++++++++++++++-------------
 arch/arm/mach-ux500/devices-db8500.h |   10 ++++--
 3 files changed, 33 insertions(+), 76 deletions(-)

diff --git a/arch/arm/mach-ux500/devices-common.c b/arch/arm/mach-ux500/devices-common.c
index 96effbd..c3bc094 100644
--- a/arch/arm/mach-ux500/devices-common.c
+++ b/arch/arm/mach-ux500/devices-common.c
@@ -59,59 +59,6 @@ dbx500_add_amba_device(struct device *parent, const char *name,
 }
 
 static struct platform_device *
-dbx500_add_platform_device(const char *name, int id, void *pdata,
-			   struct resource *res, int resnum)
-{
-	struct platform_device *dev;
-	int ret;
-
-	dev = platform_device_alloc(name, id);
-	if (!dev)
-		return ERR_PTR(-ENOMEM);
-
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
-
-	ret = platform_device_add_resources(dev, res, resnum);
-	if (ret)
-		goto out_free;
-
-	dev->dev.platform_data = pdata;
-
-	ret = platform_device_add(dev);
-	if (ret)
-		goto out_free;
-
-	return dev;
-
-out_free:
-	platform_device_put(dev);
-	return ERR_PTR(ret);
-}
-
-struct platform_device *
-dbx500_add_platform_device_4k1irq(const char *name, int id,
-				  resource_size_t base,
-				  int irq, void *pdata)
-{
-	struct resource resources[] = {
-		[0] = {
-			.start	= base,
-			.end	= base + SZ_4K - 1,
-			.flags	= IORESOURCE_MEM,
-		},
-		[1] = {
-			.start	= irq,
-			.end	= irq,
-			.flags	= IORESOURCE_IRQ,
-		}
-	};
-
-	return dbx500_add_platform_device(name, id, pdata, resources,
-					  ARRAY_SIZE(resources));
-}
-
-static struct platform_device *
 dbx500_add_gpio(struct device *parent, int id, resource_size_t addr, int irq,
 		struct nmk_gpio_platform_data *pdata)
 {
diff --git a/arch/arm/mach-ux500/devices-common.h b/arch/arm/mach-ux500/devices-common.h
index f8adff8..39c74ec 100644
--- a/arch/arm/mach-ux500/devices-common.h
+++ b/arch/arm/mach-ux500/devices-common.h
@@ -8,14 +8,15 @@
 #ifndef __DEVICES_COMMON_H
 #define __DEVICES_COMMON_H
 
-extern struct amba_device *
-dbx500_add_amba_device(struct device *parent, const char *name, resource_size_t base,
-		       int irq, void *pdata, unsigned int periphid);
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/sys_soc.h>
+#include <plat/i2c.h>
 
-extern struct platform_device *
-dbx500_add_platform_device_4k1irq(const char *name, int id,
-				  resource_size_t base,
-				  int irq, void *pdata);
+extern struct amba_device *
+dbx500_add_amba_device(struct device *parent, const char *name,
+		       resource_size_t base, int irq, void *pdata,
+		       unsigned int periphid);
 
 struct spi_master_cntlr;
 
@@ -60,20 +61,25 @@ struct nmk_i2c_controller;
 
 static inline struct platform_device *
 dbx500_add_i2c(struct device *parent, int id, resource_size_t base, int irq,
-	       struct nmk_i2c_controller *pdata)
-{
-	return dbx500_add_platform_device_4k1irq("nmk-i2c", id, base, irq,
-						 pdata);
-}
-
-struct msp_i2s_platform_data;
-
-static inline struct platform_device *
-dbx500_add_msp_i2s(int id, resource_size_t base, int irq,
-		   struct msp_i2s_platform_data *pdata)
+	       struct nmk_i2c_controller *data)
 {
-	return dbx500_add_platform_device_4k1irq("MSP_I2S", id, base, irq,
-						 pdata);
+	struct resource res[] = {
+		DEFINE_RES_MEM(base, SZ_4K),
+		DEFINE_RES_IRQ(irq),
+	};
+
+	struct platform_device_info pdevinfo = {
+		.parent = parent,
+		.name = "nmk-i2c",
+		.id = id,
+		.res = res,
+		.num_res = ARRAY_SIZE(res),
+		.data = data,
+		.size_data = sizeof(*data),
+		.dma_mask = DMA_BIT_MASK(32),
+	};
+
+	return platform_device_register_full(&pdevinfo);
 }
 
 static inline struct amba_device *
diff --git a/arch/arm/mach-ux500/devices-db8500.h b/arch/arm/mach-ux500/devices-db8500.h
index 9bd08ad..9fd93e9 100644
--- a/arch/arm/mach-ux500/devices-db8500.h
+++ b/arch/arm/mach-ux500/devices-db8500.h
@@ -18,9 +18,13 @@ db8500_add_ske_keypad(struct device *parent,
 		      struct ske_keypad_platform_data *pdata,
 		      size_t size)
 {
-	return dbx500_add_platform_device_4k1irq("nmk-ske-keypad", -1,
-						 U8500_SKE_BASE,
-						 IRQ_DB8500_KB, pdata);
+	struct resource resources[] = {
+		DEFINE_RES_MEM(U8500_SKE_BASE, SZ_4K),
+		DEFINE_RES_IRQ(IRQ_DB8500_KB),
+	};
+
+	return platform_device_register_resndata(parent, "nmk-ske-keypad", -1,
+						 resources, 2, pdata, size);
 }
 
 static inline struct amba_device *
-- 
1.7.5.4

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

Hi Lee,

I'm not able to give this a full testing, but it looks good to me,  I
have one really minor nit, but I'll look forward to integrating this
into picoxcell as soon as I get chance!

Jamie

On 1 February 2012 04:23, Lee Jones <lee.jones@linaro.org> wrote:
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..29abf6d
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,186 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms: ?GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
> +#include <linux/spinlock.h>
> +#include <linux/sys_soc.h>
> +#include <linux/err.h>
> +
> +static struct ida soc_ida;
> +static spinlock_t soc_lock;

Use DEFINE_IDR() and DEFINE_SPINLOCK() rather than initialising them at runtime?

Jamie

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Wednesday 01 February 2012, Jamie Iles wrote:
> > +static struct ida soc_ida;
> > +static spinlock_t soc_lock;
> 
> Use DEFINE_IDR() and DEFINE_SPINLOCK() rather than initialising them at runtime?
> 

Good point! "static DEFINE_{IDR,SPINLOCK}()" even, in case that wasn't clear.

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

Traditionally, any System-on-Chip based platform creates a flat list
of platform_devices directly under /sys/devices/platform.

In order to give these some better structure, this introduces a new
bus type for soc_devices that are registered with the new
soc_device_register() function.  All devices that are on the same
chip should then be registered as child devices of the soc device.

The soc bus also exports a few standardised device attributes which
allow user space to query the specific type of soc.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  183 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   37 ++++++++++
 4 files changed, 224 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 7be9f79..9aa618a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
 	bool
 	default n
 
+config SOC_BUS
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 config DMA_SHARED_BUFFER
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 2c8272d..4f302a6 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SOC_BUS) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..05f1503
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,183 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <linux/spinlock.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+
+static DEFINE_IDR(soc_ida);
+static DEFINE_SPINLOCK(soc_lock);
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+
+struct soc_device {
+	struct device dev;
+	struct soc_device_attribute *attr;
+	int soc_dev_num;
+};
+
+static struct bus_type soc_bus_type = {
+	.name  = "soc",
+};
+
+static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+
+struct device *soc_device_to_device(struct soc_device *soc_dev)
+{
+	return &soc_dev->dev;
+}
+
+static mode_t soc_attribute_mode(struct kobject *kobj,
+                                 struct attribute *attr,
+                                 int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if ((attr == &dev_attr_machine.attr)
+	    && (soc_dev->attr->machine != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_family.attr)
+	    && (soc_dev->attr->family != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_revision.attr)
+	    && (soc_dev->attr->revision != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_soc_id.attr)
+	    && (soc_dev->attr->soc_id != NULL))
+	        return attr->mode;
+
+	/* Unknown or unfilled attribute. */
+	return 0;
+}
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if (attr == &dev_attr_machine)
+		return sprintf(buf, "%s\n", soc_dev->attr->machine);
+	if (attr == &dev_attr_family)
+		return sprintf(buf, "%s\n", soc_dev->attr->family);
+	if (attr == &dev_attr_revision)
+		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_soc_id)
+		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+
+	return -EINVAL;
+
+}
+
+static struct attribute *soc_attr[] = {
+	&dev_attr_machine.attr,
+	&dev_attr_family.attr,
+	&dev_attr_soc_id.attr,
+	&dev_attr_revision.attr,
+	NULL,
+};
+
+static const struct attribute_group soc_attr_group = {
+	.attrs = soc_attr,
+	.is_visible = soc_attribute_mode,
+};
+
+static const struct attribute_group *soc_attr_groups[] = {
+	&soc_attr_group,
+	NULL,
+};
+
+static void soc_release(struct device *dev)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	kfree(soc_dev);
+}
+
+struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
+{
+	struct soc_device *soc_dev;
+	int ret;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (!soc_dev) {
+	        ret = -ENOMEM;
+		goto out1;
+	}
+
+	/* Fetch a unique (reclaimable) SOC ID. */
+	do {
+		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
+			ret = -ENOMEM;
+			goto out2;
+		}
+
+		spin_lock(&soc_lock);
+		ret = ida_get_new(&soc_ida, &soc_dev->soc_dev_num);
+		spin_unlock(&soc_lock);
+
+	} while (ret == -EAGAIN);
+
+	if (ret)
+	         goto out2;
+
+	soc_dev->attr = soc_dev_attr;
+	soc_dev->dev.bus = &soc_bus_type;
+	soc_dev->dev.groups = soc_attr_groups;
+	soc_dev->dev.release = soc_release;
+
+	dev_set_name(&soc_dev->dev, "soc%d", soc_dev->soc_dev_num);
+
+	ret = device_register(&soc_dev->dev);
+	if (ret)
+		goto out3;
+
+	return soc_dev;
+
+out3:
+	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+out2:
+	kfree(soc_dev);
+out1:
+	return ERR_PTR(ret);
+}
+
+/* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
+void soc_device_unregister(struct soc_device *soc_dev)
+{
+	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+
+	device_unregister(&soc_dev->dev);
+}
+
+static int __init soc_bus_register(void)
+{
+	spin_lock_init(&soc_lock);
+
+	return bus_register(&soc_bus_type);
+}
+core_initcall(soc_bus_register);
+
+static void __exit soc_bus_unregister(void)
+{
+	ida_destroy(&soc_ida);
+
+	bus_unregister(&soc_bus_type);
+}
+module_exit(soc_bus_unregister);
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..2739ccb
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SOC_BUS_H
+#define __SOC_BUS_H
+
+#include <linux/device.h>
+
+struct soc_device_attribute {
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *soc_id;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
+ */
+struct soc_device *soc_device_register(
+	struct soc_device_attribute *soc_plat_dev_attr);
+
+/**
+ * soc_device_unregister - unregister SoC device
+ * @dev: SoC device to be unregistered
+ */
+void soc_device_unregister(struct soc_device *soc_dev);
+
+/**
+ * soc_device_to_device - helper function to fetch struct device
+ * @soc: Previously registered SoC device container
+ */
+struct device *soc_device_to_device(struct soc_device *soc);
+
+#endif /* __SOC_BUS_H */
-- 
1.7.5.4

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Jan 30, 2012 at 05:58:24PM +0000, Arnd Bergmann wrote:
> On Saturday 28 January 2012, Greg KH wrote:
> > On Sat, Jan 21, 2012 at 05:08:03PM +0000, Lee Jones wrote:
> > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> > > index 7be9f79..9aa618a 100644
> > > --- a/drivers/base/Kconfig
> > > +++ b/drivers/base/Kconfig
> > > @@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
> > >  	bool
> > >  	default n
> > >  
> > > +config SOC_BUS
> > > +	bool
> > 
> > That's nice, but you do need some kind of help text here, right?
> 
> Wouldn't hurt, but most silent options have no help text, because that
> would never be visible in the kconfig tools, only in the source code.

Ah, it's being set by others, ok, that's fine.

> > > +static struct ida soc_ida;
> > > +static spinlock_t soc_lock;
> > > +
> > > +static ssize_t soc_info_get(struct device *dev,
> > > +			    struct device_attribute *attr,
> > > +			    char *buf);
> > > +
> > 
> > Why not put the whole function here, well a few lines lower, so no
> > forward declaration is needed, saving a few extra lines.
> 
> You made the same comment in a previous review round and then agreed
> that it's correct after all. A small comment why the forward declaration
> is required here would probably be appropriate here, otherwise the
> next person reading this would have the same thought.

Heh, at least I'm consistant with my requests :)

And yes, you are right, this is fine, a small comment would be good to
have so that I don't make the same review comment the next time around.

thanks,

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Saturday 28 January 2012, Greg KH wrote:
> On Sat, Jan 21, 2012 at 05:08:03PM +0000, Lee Jones wrote:
> > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> > index 7be9f79..9aa618a 100644
> > --- a/drivers/base/Kconfig
> > +++ b/drivers/base/Kconfig
> > @@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
> >  	bool
> >  	default n
> >  
> > +config SOC_BUS
> > +	bool
> 
> That's nice, but you do need some kind of help text here, right?

Wouldn't hurt, but most silent options have no help text, because that
would never be visible in the kconfig tools, only in the source code.

> > +
> > +static struct ida soc_ida;
> > +static spinlock_t soc_lock;
> > +
> > +static ssize_t soc_info_get(struct device *dev,
> > +			    struct device_attribute *attr,
> > +			    char *buf);
> > +
> 
> Why not put the whole function here, well a few lines lower, so no
> forward declaration is needed, saving a few extra lines.

You made the same comment in a previous review round and then agreed
that it's correct after all. A small comment why the forward declaration
is required here would probably be appropriate here, otherwise the
next person reading this would have the same thought.

> > +
> > +static mode_t soc_attribute_mode(struct kobject *kobj,
> > +                                 struct attribute *attr,
> > +                                 int index)
> > +{
> > +	struct device *dev = container_of(kobj, struct device, kobj);
> > +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> > +
> > +	if ((attr == &dev_attr_machine.attr)
> > +	    && (soc_dev->attr->machine != NULL))
> > +		return attr->mode;
> > +	if ((attr == &dev_attr_family.attr)
> > +	    && (soc_dev->attr->family != NULL))
> > +		return attr->mode;
> > +	if ((attr == &dev_attr_revision.attr)
> > +	    && (soc_dev->attr->revision != NULL))
> > +		return attr->mode;
> > +	if ((attr == &dev_attr_soc_id.attr)
> > +	    && (soc_dev->attr->soc_id != NULL))
> > +	        return attr->mode;
> > +

FWIW, the function compares the attribute pointers, so it needs to
see their delarations, while the attribute definition requires the
declaration of the function to be visible.

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Sat, Jan 21, 2012 at 05:08:03PM +0000, Lee Jones wrote:
> Traditionally, any System-on-Chip based platform creates a flat list
> of platform_devices directly under /sys/devices/platform.
> 
> In order to give these some better structure, this introduces a new
> bus type for soc_devices that are registered with the new
> soc_device_register() function.  All devices that are on the same
> chip should then be registered as child devices of the soc device.
> 
> The soc bus also exports a few standardised device attributes which
> allow user space to query the specific type of soc.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/base/Kconfig    |    3 +
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |  185 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   37 ++++++++++
>  4 files changed, 226 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/sys_soc.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 7be9f79..9aa618a 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
>  	bool
>  	default n
>  
> +config SOC_BUS
> +	bool

That's nice, but you do need some kind of help text here, right?

> +
>  source "drivers/base/regmap/Kconfig"
>  
>  config DMA_SHARED_BUFFER
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 2c8272d..4f302a6 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MODULES)	+= module.o
>  endif
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
>  obj-$(CONFIG_REGMAP)	+= regmap/
> +obj-$(CONFIG_SOC_BUS) += soc.o
>  
>  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>  
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..0c9e350
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,185 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
> +#include <linux/spinlock.h>
> +#include <linux/sys_soc.h>
> +#include <linux/err.h>
> +
> +static struct ida soc_ida;
> +static spinlock_t soc_lock;
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf);
> +

Why not put the whole function here, well a few lines lower, so no
forward declaration is needed, saving a few extra lines.

> +struct soc_device {
> +	struct device dev;
> +	struct soc_device_attribute *attr;

const?

> +	int soc_dev_num;

Why is this needed?  What's wrong with dev->id?

> +};
> +
> +static struct bus_type soc_bus_type = {
> +	.name  = "soc",
> +};
> +

Put the function here.

> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> +
> +struct device *soc_device_to_device(struct soc_device *soc_dev)
> +{
> +	return &soc_dev->dev;
> +}

Why is this even needed?  Who cares about this?

> +
> +static mode_t soc_attribute_mode(struct kobject *kobj,
> +                                 struct attribute *attr,
> +                                 int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +
> +	if ((attr == &dev_attr_machine.attr)
> +	    && (soc_dev->attr->machine != NULL))
> +		return attr->mode;
> +	if ((attr == &dev_attr_family.attr)
> +	    && (soc_dev->attr->family != NULL))
> +		return attr->mode;
> +	if ((attr == &dev_attr_revision.attr)
> +	    && (soc_dev->attr->revision != NULL))
> +		return attr->mode;
> +	if ((attr == &dev_attr_soc_id.attr)
> +	    && (soc_dev->attr->soc_id != NULL))
> +	        return attr->mode;
> +
> +	/* Unknown or unfilled attribute. */
> +	return 0;
> +}
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +
> +	if (attr == &dev_attr_machine)
> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
> +	if (attr == &dev_attr_family)
> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
> +	if (attr == &dev_attr_revision)
> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
> +	if (attr == &dev_attr_soc_id)
> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> +
> +	return -EINVAL;
> +
> +}
> +
> +static struct attribute *soc_attr[] = {
> +	&dev_attr_machine.attr,
> +	&dev_attr_family.attr,
> +	&dev_attr_soc_id.attr,
> +	&dev_attr_revision.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group soc_attr_group = {
> +	.attrs = soc_attr,
> +	.is_visible = soc_attribute_mode,

Nice, now wasn't that easier?   :)

> +};
> +
> +static const struct attribute_group *soc_attr_groups[] = {
> +	&soc_attr_group,
> +	NULL,
> +};
> +
> +static void soc_release(struct device *dev)
> +{
> +	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
> +
> +	kfree(soc_dev);
> +}
> +
> +struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	int ret;
> +
> +	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> +	if (!soc_dev) {
> +	        ret = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	/* Fetch a unique (reclaimable) SOC ID. */
> +	do {
> +		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
> +			ret = -ENOMEM;
> +			goto out2;
> +		}
> +
> +		spin_lock(&soc_lock);
> +		ret = ida_get_new(&soc_ida, &soc_dev->soc_dev_num);
> +		spin_unlock(&soc_lock);
> +
> +	} while (ret == -EAGAIN);
> +
> +	if (ret)
> +	         goto out2;
> +
> +	soc_dev->attr = soc_dev_attr;
> +	soc_dev->dev.bus = &soc_bus_type;
> +	soc_dev->dev.groups = soc_attr_groups;
> +	soc_dev->dev.release = soc_release;
> +
> +	dev_set_name(&soc_dev->dev, "soc%d", soc_dev->soc_dev_num);
> +
> +	ret = device_register(&soc_dev->dev);
> +	if (ret)
> +		goto out3;
> +
> +	return soc_dev;
> +
> +out3:
> +	ida_remove(&soc_ida, soc_dev->soc_dev_num);
> +out2:
> +	kfree(soc_dev);
> +out1:
> +	return ERR_PTR(ret);
> +}
> +
> +/* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
> +void soc_device_unregister(struct soc_device *soc_dev)
> +{
> +	ida_remove(&soc_ida, soc_dev->soc_dev_num);
> +
> +	device_unregister(&soc_dev->dev);
> +}
> +
> +static int __init soc_bus_register(void)
> +{
> +	spin_lock_init(&soc_lock);
> +
> +	ida_init(&soc_ida);
> +
> +	return bus_register(&soc_bus_type);
> +}
> +core_initcall(soc_bus_register);
> +
> +static void __exit soc_bus_unregister(void)
> +{
> +	ida_destroy(&soc_ida);
> +
> +	bus_unregister(&soc_bus_type);
> +}
> +module_exit(soc_bus_unregister);
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..2739ccb
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SOC_BUS_H
> +#define __SOC_BUS_H
> +
> +#include <linux/device.h>
> +
> +struct soc_device_attribute {
> +	const char *machine;
> +	const char *family;
> +	const char *revision;
> +	const char *soc_id;
> +};
> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
> + */
> +struct soc_device *soc_device_register(
> +	struct soc_device_attribute *soc_plat_dev_attr);

const?

> +
> +/**
> + * soc_device_unregister - unregister SoC device
> + * @dev: SoC device to be unregistered
> + */
> +void soc_device_unregister(struct soc_device *soc_dev);

We need more documentation for these functions.

> +
> +/**
> + * soc_device_to_device - helper function to fetch struct device
> + * @soc: Previously registered SoC device container
> + */
> +struct device *soc_device_to_device(struct soc_device *soc);

Is the reference count incremented when this is called?  Why is this
needed at all anyway?


Overall, much nicer, but we need more documentation on how to use this
properly, and what exactly it does.

You are almost there :)

thanks,

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

Traditionally, any System-on-Chip based platform creates a flat list
of platform_devices directly under /sys/devices/platform.

In order to give these some better structure, this introduces a new
bus type for soc_devices that are registered with the new
soc_device_register() function.  All devices that are on the same
chip should then be registered as child devices of the soc device.

The soc bus also exports a few standardised device attributes which
allow user space to query the specific type of soc.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  185 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   37 ++++++++++
 4 files changed, 226 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 7be9f79..9aa618a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
 	bool
 	default n
 
+config SOC_BUS
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 config DMA_SHARED_BUFFER
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 2c8272d..4f302a6 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SOC_BUS) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..0c9e350
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,185 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <linux/spinlock.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+
+static struct ida soc_ida;
+static spinlock_t soc_lock;
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+
+struct soc_device {
+	struct device dev;
+	struct soc_device_attribute *attr;
+	int soc_dev_num;
+};
+
+static struct bus_type soc_bus_type = {
+	.name  = "soc",
+};
+
+static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+
+struct device *soc_device_to_device(struct soc_device *soc_dev)
+{
+	return &soc_dev->dev;
+}
+
+static mode_t soc_attribute_mode(struct kobject *kobj,
+                                 struct attribute *attr,
+                                 int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if ((attr == &dev_attr_machine.attr)
+	    && (soc_dev->attr->machine != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_family.attr)
+	    && (soc_dev->attr->family != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_revision.attr)
+	    && (soc_dev->attr->revision != NULL))
+		return attr->mode;
+	if ((attr == &dev_attr_soc_id.attr)
+	    && (soc_dev->attr->soc_id != NULL))
+	        return attr->mode;
+
+	/* Unknown or unfilled attribute. */
+	return 0;
+}
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if (attr == &dev_attr_machine)
+		return sprintf(buf, "%s\n", soc_dev->attr->machine);
+	if (attr == &dev_attr_family)
+		return sprintf(buf, "%s\n", soc_dev->attr->family);
+	if (attr == &dev_attr_revision)
+		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_soc_id)
+		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+
+	return -EINVAL;
+
+}
+
+static struct attribute *soc_attr[] = {
+	&dev_attr_machine.attr,
+	&dev_attr_family.attr,
+	&dev_attr_soc_id.attr,
+	&dev_attr_revision.attr,
+	NULL,
+};
+
+static const struct attribute_group soc_attr_group = {
+	.attrs = soc_attr,
+	.is_visible = soc_attribute_mode,
+};
+
+static const struct attribute_group *soc_attr_groups[] = {
+	&soc_attr_group,
+	NULL,
+};
+
+static void soc_release(struct device *dev)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	kfree(soc_dev);
+}
+
+struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
+{
+	struct soc_device *soc_dev;
+	int ret;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (!soc_dev) {
+	        ret = -ENOMEM;
+		goto out1;
+	}
+
+	/* Fetch a unique (reclaimable) SOC ID. */
+	do {
+		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
+			ret = -ENOMEM;
+			goto out2;
+		}
+
+		spin_lock(&soc_lock);
+		ret = ida_get_new(&soc_ida, &soc_dev->soc_dev_num);
+		spin_unlock(&soc_lock);
+
+	} while (ret == -EAGAIN);
+
+	if (ret)
+	         goto out2;
+
+	soc_dev->attr = soc_dev_attr;
+	soc_dev->dev.bus = &soc_bus_type;
+	soc_dev->dev.groups = soc_attr_groups;
+	soc_dev->dev.release = soc_release;
+
+	dev_set_name(&soc_dev->dev, "soc%d", soc_dev->soc_dev_num);
+
+	ret = device_register(&soc_dev->dev);
+	if (ret)
+		goto out3;
+
+	return soc_dev;
+
+out3:
+	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+out2:
+	kfree(soc_dev);
+out1:
+	return ERR_PTR(ret);
+}
+
+/* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
+void soc_device_unregister(struct soc_device *soc_dev)
+{
+	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+
+	device_unregister(&soc_dev->dev);
+}
+
+static int __init soc_bus_register(void)
+{
+	spin_lock_init(&soc_lock);
+
+	ida_init(&soc_ida);
+
+	return bus_register(&soc_bus_type);
+}
+core_initcall(soc_bus_register);
+
+static void __exit soc_bus_unregister(void)
+{
+	ida_destroy(&soc_ida);
+
+	bus_unregister(&soc_bus_type);
+}
+module_exit(soc_bus_unregister);
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..2739ccb
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SOC_BUS_H
+#define __SOC_BUS_H
+
+#include <linux/device.h>
+
+struct soc_device_attribute {
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *soc_id;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
+ */
+struct soc_device *soc_device_register(
+	struct soc_device_attribute *soc_plat_dev_attr);
+
+/**
+ * soc_device_unregister - unregister SoC device
+ * @dev: SoC device to be unregistered
+ */
+void soc_device_unregister(struct soc_device *soc_dev);
+
+/**
+ * soc_device_to_device - helper function to fetch struct device
+ * @soc: Previously registered SoC device container
+ */
+struct device *soc_device_to_device(struct soc_device *soc);
+
+#endif /* __SOC_BUS_H */
-- 
1.7.5.4

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Fri, Jan 20, 2012 at 05:13:33PM +0000, Lee Jones wrote:

[hint, turn off html in gmail, your messages are not getting through to
the mailing lists...]

> On Fri, Jan 20, 2012 at 4:36 PM, Greg KH <greg@kroah.com> wrote:
> 
> > On Fri, Jan 20, 2012 at 04:10:25PM +0000, Lee Jones wrote:
> > > +struct soc_device *soc_device_register(struct soc_device_attribute
> > *soc_dev_attr)
> > > +{
> > > +     struct soc_device *soc_dev;
> > > +     int ret;
> > > +     int i, j = 0;
> > > +
> > > +     soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> > > +     if (!soc_dev) {
> > > +             ret = -ENOMEM;
> > > +             goto out1;
> > > +     }
> > > +
> > > +     /* Ensure no empty attributes are created. */
> > > +     for (i = 0; i < sizeof(*soc_attr); i++) {
> > > +             if ( soc_attr[i] != NULL ) {
> > > +                     soc_attr[j++] = soc_attr[i];
> > > +             }
> > > +     }
> > > +     for (i = j; i < sizeof(*soc_attr); i++) {
> > > +             soc_attr[i] = NULL;
> > > +     }
> >
> > Huh?
> >
> > Ick, no, don't do this, that's just looney.  Use the api WE ALREADY HAVE
> > for this type of thing.
> >
> > Sometimes I wonder why we even write core code, if people just ignore it
> > and try to do it on their own...
> >
> 
> Sorry. Just another splash of ignorance on my part.
> 
> If I would have known about an API to use, I would have used it (as it
> would have saved me time).
> 
> I'll go mindlessly grepping for things that my be of use and replace it.
> 
> (Of course a hint wouldn't go a miss if you're feeling charitable - every
> day's a school day and all that.) :)

Hey, I offered to write this whole chunk of code myself in the past, and
that was rejected, so I'm not feeling all that generous myself...

But I'll be nice, just this once.

Look at the is_visible() callback in the attribute group structure, that
is what you want to set.

You owe me a beer.

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Fri, Jan 20, 2012 at 05:14:00PM +0000, Lee Jones wrote:
> 
> 
> On Fri, Jan 20, 2012 at 4:39 PM, Greg KH <greg@kroah.com> wrote:
> 
>     On Fri, Jan 20, 2012 at 04:10:25PM +0000, Lee Jones wrote:
>     > +struct soc_device *soc_device_register(struct soc_device_attribute
>     *soc_dev_attr)
>     > +{
>     > + ? ? struct soc_device *soc_dev;
>     > + ? ? int ret;
>     > + ? ? int i, j = 0;
>     > +
>     > + ? ? soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
>     > + ? ? if (!soc_dev) {
>     > + ? ? ? ? ? ? ret = -ENOMEM;
>     > + ? ? ? ? ? ? goto out1;
>     > + ? ? }
> 
>     <snip>
> 
>     > +out1:
>     > + ? ? kfree(soc_dev_attr);
> 
>     You just freed the caller's memory, are you sure you wanted to do that?
> 
> 
> Yes, I think so. We free it if the register fails and in
> the?unregister?function.
> 
> I'd be happy to let the caller handle it if you think that's better.?

Well, you don't document anywhere that the pointer being passed in is
going to be freed, so either document it, or don't do that.

Odds are, you really don't want to do this, as it's going to be a
pointer to a static structure, right?  Calling kfree() on it is not a
good idea.

thanks,

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Fri, Jan 20, 2012 at 04:10:25PM +0000, Lee Jones wrote:
> +struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	int ret;
> +	int i, j = 0;
> +
> +	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> +	if (!soc_dev) {
> +	        ret = -ENOMEM;
> +		goto out1;
> +	}

<snip>

> +out1:
> +	kfree(soc_dev_attr);

You just freed the caller's memory, are you sure you wanted to do that?

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Fri, Jan 20, 2012 at 04:10:25PM +0000, Lee Jones wrote:
> +struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	int ret;
> +	int i, j = 0;
> +
> +	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> +	if (!soc_dev) {
> +	        ret = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	/* Ensure no empty attributes are created. */
> +	for (i = 0; i < sizeof(*soc_attr); i++) {
> +	        if ( soc_attr[i] != NULL ) {
> +		        soc_attr[j++] = soc_attr[i];
> +		}
> +	}
> +	for (i = j; i < sizeof(*soc_attr); i++) {
> +	        soc_attr[i] = NULL;
> +	}

Huh?

Ick, no, don't do this, that's just looney.  Use the api WE ALREADY HAVE
for this type of thing.

Sometimes I wonder why we even write core code, if people just ignore it
and try to do it on their own...

{sigh}

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

Traditionally, any System-on-Chip based platform creates a flat list
of platform_devices directly under /sys/devices/platform.

In order to give these some better structure, this introduces a new
bus type for soc_devices that are registered with the new
soc_device_register() function.  All devices that are on the same
chip should then be registered as child devices of the soc device.

The soc bus also exports a few standardised device attributes which
allow user space to query the specific type of soc.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  175 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   37 ++++++++++
 4 files changed, 216 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 7be9f79..9aa618a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
 	bool
 	default n
 
+config SOC_BUS
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 config DMA_SHARED_BUFFER
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 2c8272d..4f302a6 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SOC_BUS) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..90353b5
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,175 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/idr.h>
+#include <linux/spinlock.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+
+static struct ida soc_ida;
+static spinlock_t soc_lock;
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+
+struct soc_device {
+	struct device dev;
+	struct soc_device_attribute *attr;
+	int soc_dev_num;
+};
+
+static struct bus_type soc_bus_type = {
+	.name  = "soc",
+};
+
+static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+
+static struct attribute *soc_attr[] = {
+	&dev_attr_machine.attr,
+	&dev_attr_family.attr,
+	&dev_attr_soc_id.attr,
+	&dev_attr_revision.attr,
+	NULL,
+};
+
+static const struct attribute_group soc_attr_group = {
+	.attrs = soc_attr,
+};
+
+static const struct attribute_group *soc_attr_groups[] = {
+	&soc_attr_group,
+	NULL,
+};
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	if (attr == &dev_attr_machine)
+		return sprintf(buf, "%s\n", soc_dev->attr->machine);
+	if (attr == &dev_attr_family)
+		return sprintf(buf, "%s\n", soc_dev->attr->family);
+	if (attr == &dev_attr_revision)
+		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_soc_id)
+		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+
+	return -EINVAL;
+
+}
+
+struct device *soc_device_to_device(struct soc_device *soc_dev)
+{
+	return &soc_dev->dev;
+}
+
+static void soc_release(struct device *dev)
+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+
+	kfree(soc_dev);
+}
+
+struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
+{
+	struct soc_device *soc_dev;
+	int ret;
+	int i, j = 0;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (!soc_dev) {
+	        ret = -ENOMEM;
+		goto out1;
+	}
+
+	/* Ensure no empty attributes are created. */
+	for (i = 0; i < sizeof(*soc_attr); i++) {
+	        if ( soc_attr[i] != NULL ) {
+		        soc_attr[j++] = soc_attr[i];
+		}
+	}
+	for (i = j; i < sizeof(*soc_attr); i++) {
+	        soc_attr[i] = NULL;
+	}
+
+	/* Fetch a unique (reclaimable) SOC ID. */
+	do {
+		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
+			ret = -ENOMEM;
+			goto out2;
+		}
+
+		spin_lock(&soc_lock);
+		ret = ida_get_new(&soc_ida, &soc_dev->soc_dev_num);
+		spin_unlock(&soc_lock);
+
+	} while (ret == -EAGAIN);
+
+	if (ret)
+	         goto out2;
+
+	soc_dev->attr = soc_dev_attr;
+	soc_dev->dev.bus = &soc_bus_type;
+	soc_dev->dev.groups = soc_attr_groups;
+	soc_dev->dev.release = soc_release;
+
+	dev_set_name(&soc_dev->dev, "soc%d", soc_dev->soc_dev_num);
+
+	ret = device_register(&soc_dev->dev);
+	if (ret)
+		goto out3;
+
+	return soc_dev;
+
+out3:
+	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+out2:
+	kfree(soc_dev);
+out1:
+	kfree(soc_dev_attr);
+
+	return ERR_PTR(ret);
+}
+
+void soc_device_unregister(struct soc_device *soc_dev)
+{
+	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+
+	device_unregister(&soc_dev->dev);
+
+	kfree(soc_dev->attr);
+	kfree(soc_dev);
+}
+
+static int __init soc_bus_register(void)
+{
+	spin_lock_init(&soc_lock);
+
+	ida_init(&soc_ida);
+
+	return bus_register(&soc_bus_type);
+}
+core_initcall(soc_bus_register);
+
+static void __exit soc_bus_unregister(void)
+{
+	ida_destroy(&soc_ida);
+
+	bus_unregister(&soc_bus_type);
+}
+module_exit(soc_bus_unregister);
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..2739ccb
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SOC_BUS_H
+#define __SOC_BUS_H
+
+#include <linux/device.h>
+
+struct soc_device_attribute {
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *soc_id;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
+ */
+struct soc_device *soc_device_register(
+	struct soc_device_attribute *soc_plat_dev_attr);
+
+/**
+ * soc_device_unregister - unregister SoC device
+ * @dev: SoC device to be unregistered
+ */
+void soc_device_unregister(struct soc_device *soc_dev);
+
+/**
+ * soc_device_to_device - helper function to fetch struct device
+ * @soc: Previously registered SoC device container
+ */
+struct device *soc_device_to_device(struct soc_device *soc);
+
+#endif /* __SOC_BUS_H */
-- 
1.7.5.4

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

On Tue, Oct 18, 2011 at 04:53:09PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 October 2011, Jamie Iles wrote:
> > Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
> > also need to add reference counting of the module when 
> > creating/destroying a soc device to prevent the module and bus 
> > disappearing whilst you had devices with a reference to it?
> 
> No, that's not necessary in a case like this: The bus module remains
> pinned by the reference of the module calling it on each exported
> symbol that is referenced. The reference counting is only needed
> if driver module provides function pointers to the bus module
> that might be in the middle of getting called while the driver is
> removed. We have this case with the ux500_get_process function
> that may be called through sysfs, so that would be a bug if the
> code calling device_create_file() was in a module.

Ahh, okay that makes sense.  Thanks for the explanation Arnd.

Jamie

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

On Tue, Oct 18, 2011 at 04:53:09PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 October 2011, Jamie Iles wrote:
> > Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
> > also need to add reference counting of the module when 
> > creating/destroying a soc device to prevent the module and bus 
> > disappearing whilst you had devices with a reference to it?
> 
> No, that's not necessary in a case like this: The bus module remains
> pinned by the reference of the module calling it on each exported
> symbol that is referenced. The reference counting is only needed
> if driver module provides function pointers to the bus module
> that might be in the middle of getting called while the driver is
> removed. We have this case with the ux500_get_process function
> that may be called through sysfs, so that would be a bug if the
> code calling device_create_file() was in a module.

Ahh, okay that makes sense.  Thanks for the explanation Arnd.

Jamie

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Tuesday 18 October 2011, Jamie Iles wrote:
> I can't think of a system where it make sense to have this as a loadable 
> module so can't we just register the bus_type and never unregister it 
> like the platform and spi busses for example?

Good point. At least if we disallow unregistering soc_devices as I
suggested, there is certainly no point making the bus itself modular.

> Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
> also need to add reference counting of the module when 
> creating/destroying a soc device to prevent the module and bus 
> disappearing whilst you had devices with a reference to it?

No, that's not necessary in a case like this: The bus module remains
pinned by the reference of the module calling it on each exported
symbol that is referenced. The reference counting is only needed
if driver module provides function pointers to the bus module
that might be in the middle of getting called while the driver is
removed. We have this case with the ux500_get_process function
that may be called through sysfs, so that would be a bug if the
code calling device_create_file() was in a module.

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Tuesday 18 October 2011, Jamie Iles wrote:
> I can't think of a system where it make sense to have this as a loadable 
> module so can't we just register the bus_type and never unregister it 
> like the platform and spi busses for example?

Good point. At least if we disallow unregistering soc_devices as I
suggested, there is certainly no point making the bus itself modular.

> Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
> also need to add reference counting of the module when 
> creating/destroying a soc device to prevent the module and bus 
> disappearing whilst you had devices with a reference to it?

No, that's not necessary in a case like this: The bus module remains
pinned by the reference of the module calling it on each exported
symbol that is referenced. The reference counting is only needed
if driver module provides function pointers to the bus module
that might be in the middle of getting called while the driver is
removed. We have this case with the ux500_get_process function
that may be called through sysfs, so that would be a bug if the
code calling device_create_file() was in a module.

	Arnd

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 04:00:03PM +0200, Arnd Bergmann wrote:
> On Monday 17 October 2011, Greg KH wrote:
> > > You also commented that the argument to soc_device_unregister should
> > > be a soc_device (as, consequently, the return type of soc_device_register).
> > > Agree with that comment, but it means that the definition of struct
> > > soc_device needs to remain visible in order to be used as the parent
> > > for other devices.
> > 
> > No it doesn't:
> >         struct device * soc_device_to_device(struct soc device *soc);
> 
> Right, that works of course. I believe the more common way is to
> expose the derived type to its users, and it also simplifies the
> interface.
> 
> > Anyway, what are you using this soc device to be the parent of?
> 
> Basically everything. The SoC is probably about 90% of the system in
> modern embedded systems. Typically, there are on-chip buses like
> AMBA or PLB that contain dozens of internal devices (interrupt
> controller, serial, dmaengine, rtc, timer, watchdog, ...) as well
> as buses (i2c, spi, mmc, usb, pci, ...) that have off-chip child
> devices. You can think of an soc device as a kind of über-MFD
> that holds all of these together.
> 
> If you remember the early discussions about this patch set, I
> specifically asked for making the soc_device be a representation
> of the whole soc with a hierarchical view of its child devices
> under it, as opposed to having an artificial device node that only
> serves to export strings along the lines of /proc/cpuinfo.
> 
> See patch 5/6 for the one that moves all platform devices that
> are part of the dbx500 soc below the soc_device.

Ah, ok, that's nicer, and makes sense.

So yes, you can leave the structure here, or use a helper function, but
either way, you shouldn't be returning a struct device * from the
register function, that doesn't make sense.

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 04:00:03PM +0200, Arnd Bergmann wrote:
> On Monday 17 October 2011, Greg KH wrote:
> > > You also commented that the argument to soc_device_unregister should
> > > be a soc_device (as, consequently, the return type of soc_device_register).
> > > Agree with that comment, but it means that the definition of struct
> > > soc_device needs to remain visible in order to be used as the parent
> > > for other devices.
> > 
> > No it doesn't:
> >         struct device * soc_device_to_device(struct soc device *soc);
> 
> Right, that works of course. I believe the more common way is to
> expose the derived type to its users, and it also simplifies the
> interface.
> 
> > Anyway, what are you using this soc device to be the parent of?
> 
> Basically everything. The SoC is probably about 90% of the system in
> modern embedded systems. Typically, there are on-chip buses like
> AMBA or PLB that contain dozens of internal devices (interrupt
> controller, serial, dmaengine, rtc, timer, watchdog, ...) as well
> as buses (i2c, spi, mmc, usb, pci, ...) that have off-chip child
> devices. You can think of an soc device as a kind of ?ber-MFD
> that holds all of these together.
> 
> If you remember the early discussions about this patch set, I
> specifically asked for making the soc_device be a representation
> of the whole soc with a hierarchical view of its child devices
> under it, as opposed to having an artificial device node that only
> serves to export strings along the lines of /proc/cpuinfo.
> 
> See patch 5/6 for the one that moves all platform devices that
> are part of the dbx500 soc below the soc_device.

Ah, ok, that's nicer, and makes sense.

So yes, you can leave the structure here, or use a helper function, but
either way, you shouldn't be returning a struct device * from the
register function, that doesn't make sense.

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 12:12:41PM +0100, Lee Jones wrote:
> > If you move around things a bit here, you can save 4 lines of code,
> > please do so.
> 
> Telepathy has never been my Forté. Would you mind alluding to which
> lines I can trim please?
> 
> If you mean the placing the DEVICE_ATTR's after soc_info_get and
> removing its prototype; I had that already, but I was told to do it this
> way as it would make the binary smaller.

Yes, you are correct, I am wrong here, nevermind.

> >> +
> >> +struct bus_type soc_bus_type = {
> >> +	.name  = "soc",
> >> +};
> >> +
> >> +static int __init soc_bus_register(void)
> >> +{
> >> +	return bus_register(&soc_bus_type);
> >> +}
> >> +core_initcall(soc_bus_register);
> > 
> > No unregister?
> 
> The unregister is contained in soc_device_unregister, but as you point
> out in your other email this should be removed. I'm guessing we should
> either count the number of SoCs and unregister as the last one leaves,
> or not unregister at all?

No, make it a module exit function and you will be fine.

> >> +struct attribute *soc_attr[] = {
> >> +	&dev_attr_machine.attr,
> >> +	&dev_attr_family.attr,
> >> +	&dev_attr_soc_id.attr,
> >> +	&dev_attr_revision.attr,
> >> +	NULL,
> >> +};
> >> +
> >> +struct attribute_group soc_attr_group = {
> >> +	.attrs = soc_attr,
> >> +};
> >> +
> >> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> >> +{
> >> +	struct soc_device *soc_dev;
> >> +	static atomic_t soc_device_num = ATOMIC_INIT(0);
> > 
> > No, please don't do this, use the proper kernel interface to dynamically
> > handle numbering devices (hint, if you unload a SOC device, you will
> > never reclaim that device number, which isn't that nice.)
> 
> Again, some help would really be appreciated here. I searched the kernel
> last time you mentioned numbering, but this is all I came up with.

ida

> >> +struct soc_device_attribute {
> >> +	const char *machine;
> >> +	const char *family;
> >> +	const char *revision;
> >> +	const char *soc_id;
> >> +};
> > 
> > What happens if one of these attributes is NULL?  Please check for that
> > when you create the attributes so that you don't create an attribute you
> > don't want to.
> 
> By shifting around "struct attribute *soc_attr[]"?
> 
> If one of them is NULL the file will just be empty, is that such a bad
> thing?

Yes.  sysfs files show that if the file is present, then there should be
data in it.  If it isn't present, then that attribute isn't there for
that device.  It is easier for userspace to handle this than by having
to open files and test for a null entry or not.

> >> +
> >> +struct soc_device {
> >> +	struct device dev;
> >> +	struct soc_device_attribute *attr;
> >> +};
> > 
> > Why is this needed to be defined here?  It should be in the .c file as
> > no external code needs to know what it looks like.
> 
> If you want me to pass back a "struct soc_device" instead of a "raw"
> struct device, the calling code will need to know how to separate out
> "struct device dev" won't it?

It depends on what it wants to do with that struct device.  Why would
the caller code care about it?

thanks,

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 12:12:41PM +0100, Lee Jones wrote:
> > If you move around things a bit here, you can save 4 lines of code,
> > please do so.
> 
> Telepathy has never been my Fort?. Would you mind alluding to which
> lines I can trim please?
> 
> If you mean the placing the DEVICE_ATTR's after soc_info_get and
> removing its prototype; I had that already, but I was told to do it this
> way as it would make the binary smaller.

Yes, you are correct, I am wrong here, nevermind.

> >> +
> >> +struct bus_type soc_bus_type = {
> >> +	.name  = "soc",
> >> +};
> >> +
> >> +static int __init soc_bus_register(void)
> >> +{
> >> +	return bus_register(&soc_bus_type);
> >> +}
> >> +core_initcall(soc_bus_register);
> > 
> > No unregister?
> 
> The unregister is contained in soc_device_unregister, but as you point
> out in your other email this should be removed. I'm guessing we should
> either count the number of SoCs and unregister as the last one leaves,
> or not unregister at all?

No, make it a module exit function and you will be fine.

> >> +struct attribute *soc_attr[] = {
> >> +	&dev_attr_machine.attr,
> >> +	&dev_attr_family.attr,
> >> +	&dev_attr_soc_id.attr,
> >> +	&dev_attr_revision.attr,
> >> +	NULL,
> >> +};
> >> +
> >> +struct attribute_group soc_attr_group = {
> >> +	.attrs = soc_attr,
> >> +};
> >> +
> >> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> >> +{
> >> +	struct soc_device *soc_dev;
> >> +	static atomic_t soc_device_num = ATOMIC_INIT(0);
> > 
> > No, please don't do this, use the proper kernel interface to dynamically
> > handle numbering devices (hint, if you unload a SOC device, you will
> > never reclaim that device number, which isn't that nice.)
> 
> Again, some help would really be appreciated here. I searched the kernel
> last time you mentioned numbering, but this is all I came up with.

ida

> >> +struct soc_device_attribute {
> >> +	const char *machine;
> >> +	const char *family;
> >> +	const char *revision;
> >> +	const char *soc_id;
> >> +};
> > 
> > What happens if one of these attributes is NULL?  Please check for that
> > when you create the attributes so that you don't create an attribute you
> > don't want to.
> 
> By shifting around "struct attribute *soc_attr[]"?
> 
> If one of them is NULL the file will just be empty, is that such a bad
> thing?

Yes.  sysfs files show that if the file is present, then there should be
data in it.  If it isn't present, then that attribute isn't there for
that device.  It is easier for userspace to handle this than by having
to open files and test for a null entry or not.

> >> +
> >> +struct soc_device {
> >> +	struct device dev;
> >> +	struct soc_device_attribute *attr;
> >> +};
> > 
> > Why is this needed to be defined here?  It should be in the .c file as
> > no external code needs to know what it looks like.
> 
> If you want me to pass back a "struct soc_device" instead of a "raw"
> struct device, the calling code will need to know how to separate out
> "struct device dev" won't it?

It depends on what it wants to do with that struct device.  Why would
the caller code care about it?

thanks,

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 04:14:02PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 October 2011, Lee Jones wrote:
> > >> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> > >> +{
> > >> +    struct soc_device *soc_dev;
> > >> +    static atomic_t soc_device_num = ATOMIC_INIT(0);
> > > 
> > > No, please don't do this, use the proper kernel interface to dynamically
> > > handle numbering devices (hint, if you unload a SOC device, you will
> > > never reclaim that device number, which isn't that nice.)
> > 
> > Again, some help would really be appreciated here. I searched the kernel
> > last time you mentioned numbering, but this is all I came up with.
> 
> I guess the correct interface to use here would be an "ida", see
> linux/idr.h. However, I'm not convinced that's actually worth the
> extra space for maintaining it here.

Why?  That is the interface for this type of thing, it's very simple to
use, and it handles all of the logic for you for this type of thing.

Please don't roll your own solutions for things when there is already
code in the kernel to do it.

> IMHO, we could also remove the soc_device_unregister() function entirely
> and add a comment along the lines of 
> /*
>  * If you really need to add hot-pluggable soc_devices, add a
>  * soc_device_unregister function and turn the number generation
>  * into an IDA.
>  */

No, please provide this.  I want to see it tested, as when it is, then
the code will be fixed properly as right now it does some things it
shouldn't be doing in this regard.

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 04:14:02PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 October 2011, Lee Jones wrote:
> > >> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> > >> +{
> > >> +    struct soc_device *soc_dev;
> > >> +    static atomic_t soc_device_num = ATOMIC_INIT(0);
> > > 
> > > No, please don't do this, use the proper kernel interface to dynamically
> > > handle numbering devices (hint, if you unload a SOC device, you will
> > > never reclaim that device number, which isn't that nice.)
> > 
> > Again, some help would really be appreciated here. I searched the kernel
> > last time you mentioned numbering, but this is all I came up with.
> 
> I guess the correct interface to use here would be an "ida", see
> linux/idr.h. However, I'm not convinced that's actually worth the
> extra space for maintaining it here.

Why?  That is the interface for this type of thing, it's very simple to
use, and it handles all of the logic for you for this type of thing.

Please don't roll your own solutions for things when there is already
code in the kernel to do it.

> IMHO, we could also remove the soc_device_unregister() function entirely
> and add a comment along the lines of 
> /*
>  * If you really need to add hot-pluggable soc_devices, add a
>  * soc_device_unregister function and turn the number generation
>  * into an IDA.
>  */

No, please provide this.  I want to see it tested, as when it is, then
the code will be fixed properly as right now it does some things it
shouldn't be doing in this regard.

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 03:15:20PM +0100, Jamie Iles wrote:
> Hi Arnd,
> 
> On Tue, Oct 18, 2011 at 04:05:12PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 October 2011, Lee Jones wrote:
> > > On 17/10/11 17:18, Greg KH wrote:
> > > > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> > > >> +
> > > >> +	bus_unregister(&soc_bus_type);
> > > > 
> > > > What happens if you have more than one SOC device?  I think you just
> > > > oopsed.
> > > 
> > > I think you're right.
> > > 
> > > When to you suggest we unregister the bus?
> > 
> > Do it in the same way as registering it, as a module_exit() function
> > below the initcall that instantiates it. These interfaces usually come
> > in pairs, so if something does not look symmetric, you should better
> > have another look.
> 
> I can't think of a system where it make sense to have this as a loadable 
> module so can't we just register the bus_type and never unregister it 
> like the platform and spi busses for example?
> 
> Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
> also need to add reference counting of the module when 
> creating/destroying a soc device to prevent the module and bus 
> disappearing whilst you had devices with a reference to it?

Yes, that is why your register function should take a module pointer,
like USB, PCI, and other bus functions do.

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Tue, Oct 18, 2011 at 03:15:20PM +0100, Jamie Iles wrote:
> Hi Arnd,
> 
> On Tue, Oct 18, 2011 at 04:05:12PM +0200, Arnd Bergmann wrote:
> > On Tuesday 18 October 2011, Lee Jones wrote:
> > > On 17/10/11 17:18, Greg KH wrote:
> > > > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> > > >> +
> > > >> +	bus_unregister(&soc_bus_type);
> > > > 
> > > > What happens if you have more than one SOC device?  I think you just
> > > > oopsed.
> > > 
> > > I think you're right.
> > > 
> > > When to you suggest we unregister the bus?
> > 
> > Do it in the same way as registering it, as a module_exit() function
> > below the initcall that instantiates it. These interfaces usually come
> > in pairs, so if something does not look symmetric, you should better
> > have another look.
> 
> I can't think of a system where it make sense to have this as a loadable 
> module so can't we just register the bus_type and never unregister it 
> like the platform and spi busses for example?
> 
> Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
> also need to add reference counting of the module when 
> creating/destroying a soc device to prevent the module and bus 
> disappearing whilst you had devices with a reference to it?

Yes, that is why your register function should take a module pointer,
like USB, PCI, and other bus functions do.

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

Hi Arnd,

On Tue, Oct 18, 2011 at 04:05:12PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 October 2011, Lee Jones wrote:
> > On 17/10/11 17:18, Greg KH wrote:
> > > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> > >> +
> > >> +	bus_unregister(&soc_bus_type);
> > > 
> > > What happens if you have more than one SOC device?  I think you just
> > > oopsed.
> > 
> > I think you're right.
> > 
> > When to you suggest we unregister the bus?
> 
> Do it in the same way as registering it, as a module_exit() function
> below the initcall that instantiates it. These interfaces usually come
> in pairs, so if something does not look symmetric, you should better
> have another look.

I can't think of a system where it make sense to have this as a loadable 
module so can't we just register the bus_type and never unregister it 
like the platform and spi busses for example?

Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
also need to add reference counting of the module when 
creating/destroying a soc device to prevent the module and bus 
disappearing whilst you had devices with a reference to it?

Jamie

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

Hi Arnd,

On Tue, Oct 18, 2011 at 04:05:12PM +0200, Arnd Bergmann wrote:
> On Tuesday 18 October 2011, Lee Jones wrote:
> > On 17/10/11 17:18, Greg KH wrote:
> > > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> > >> +
> > >> +	bus_unregister(&soc_bus_type);
> > > 
> > > What happens if you have more than one SOC device?  I think you just
> > > oopsed.
> > 
> > I think you're right.
> > 
> > When to you suggest we unregister the bus?
> 
> Do it in the same way as registering it, as a module_exit() function
> below the initcall that instantiates it. These interfaces usually come
> in pairs, so if something does not look symmetric, you should better
> have another look.

I can't think of a system where it make sense to have this as a loadable 
module so can't we just register the bus_type and never unregister it 
like the platform and spi busses for example?

Also, (and I'm right at the edge of my knowledge here!) wouldn't you 
also need to add reference counting of the module when 
creating/destroying a soc device to prevent the module and bus 
disappearing whilst you had devices with a reference to it?

Jamie

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Tuesday 18 October 2011, Lee Jones wrote:
> >> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> >> +{
> >> +    struct soc_device *soc_dev;
> >> +    static atomic_t soc_device_num = ATOMIC_INIT(0);
> > 
> > No, please don't do this, use the proper kernel interface to dynamically
> > handle numbering devices (hint, if you unload a SOC device, you will
> > never reclaim that device number, which isn't that nice.)
> 
> Again, some help would really be appreciated here. I searched the kernel
> last time you mentioned numbering, but this is all I came up with.

I guess the correct interface to use here would be an "ida", see
linux/idr.h. However, I'm not convinced that's actually worth the
extra space for maintaining it here.

An SoC pretty much has the following properties:

* gets probed very early at boot time,
* can never go away, and
* there is only one of them in the system.

The only reason I even asked for the devices to be enumerated is that
there is the possibility that some device will need to have more than
one and we should design every stable user interface in a way that never
changes in a user visible way.

IMHO, we could also remove the soc_device_unregister() function entirely
and add a comment along the lines of 
/*
 * If you really need to add hot-pluggable soc_devices, add a
 * soc_device_unregister function and turn the number generation
 * into an IDA.
 */

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Tuesday 18 October 2011, Lee Jones wrote:
> >> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> >> +{
> >> +    struct soc_device *soc_dev;
> >> +    static atomic_t soc_device_num = ATOMIC_INIT(0);
> > 
> > No, please don't do this, use the proper kernel interface to dynamically
> > handle numbering devices (hint, if you unload a SOC device, you will
> > never reclaim that device number, which isn't that nice.)
> 
> Again, some help would really be appreciated here. I searched the kernel
> last time you mentioned numbering, but this is all I came up with.

I guess the correct interface to use here would be an "ida", see
linux/idr.h. However, I'm not convinced that's actually worth the
extra space for maintaining it here.

An SoC pretty much has the following properties:

* gets probed very early at boot time,
* can never go away, and
* there is only one of them in the system.

The only reason I even asked for the devices to be enumerated is that
there is the possibility that some device will need to have more than
one and we should design every stable user interface in a way that never
changes in a user visible way.

IMHO, we could also remove the soc_device_unregister() function entirely
and add a comment along the lines of 
/*
 * If you really need to add hot-pluggable soc_devices, add a
 * soc_device_unregister function and turn the number generation
 * into an IDA.
 */

	Arnd

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Tuesday 18 October 2011, Lee Jones wrote:
> On 17/10/11 17:18, Greg KH wrote:
> > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> >> +{
> >> +	struct soc_device *soc_dev =
> >> +		container_of(dev, struct soc_device, dev);
> >> +
> >> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
> >> +
> >> +	if (device_is_registered(dev))
> >> +		device_unregister(dev);
> > 
> > Why is this call needed?
> 
> To unregister a previously unregistered device?
> 
> Is that wrong?

See Jamie's excellent explanation: you don't need to check
for device_is_registered() here, but just call device_unregister
unconditionally.

> >> +
> >> +	bus_unregister(&soc_bus_type);
> > 
> > What happens if you have more than one SOC device?  I think you just
> > oopsed.
> 
> I think you're right.
> 
> When to you suggest we unregister the bus?

Do it in the same way as registering it, as a module_exit() function
below the initcall that instantiates it. These interfaces usually come
in pairs, so if something does not look symmetric, you should better
have another look.

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Tuesday 18 October 2011, Lee Jones wrote:
> On 17/10/11 17:18, Greg KH wrote:
> > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> >> +{
> >> +	struct soc_device *soc_dev =
> >> +		container_of(dev, struct soc_device, dev);
> >> +
> >> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
> >> +
> >> +	if (device_is_registered(dev))
> >> +		device_unregister(dev);
> > 
> > Why is this call needed?
> 
> To unregister a previously unregistered device?
> 
> Is that wrong?

See Jamie's excellent explanation: you don't need to check
for device_is_registered() here, but just call device_unregister
unconditionally.

> >> +
> >> +	bus_unregister(&soc_bus_type);
> > 
> > What happens if you have more than one SOC device?  I think you just
> > oopsed.
> 
> I think you're right.
> 
> When to you suggest we unregister the bus?

Do it in the same way as registering it, as a module_exit() function
below the initcall that instantiates it. These interfaces usually come
in pairs, so if something does not look symmetric, you should better
have another look.

	Arnd

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Monday 17 October 2011, Greg KH wrote:
> > You also commented that the argument to soc_device_unregister should
> > be a soc_device (as, consequently, the return type of soc_device_register).
> > Agree with that comment, but it means that the definition of struct
> > soc_device needs to remain visible in order to be used as the parent
> > for other devices.
> 
> No it doesn't:
>         struct device * soc_device_to_device(struct soc device *soc);

Right, that works of course. I believe the more common way is to
expose the derived type to its users, and it also simplifies the
interface.

> Anyway, what are you using this soc device to be the parent of?

Basically everything. The SoC is probably about 90% of the system in
modern embedded systems. Typically, there are on-chip buses like
AMBA or PLB that contain dozens of internal devices (interrupt
controller, serial, dmaengine, rtc, timer, watchdog, ...) as well
as buses (i2c, spi, mmc, usb, pci, ...) that have off-chip child
devices. You can think of an soc device as a kind of über-MFD
that holds all of these together.

If you remember the early discussions about this patch set, I
specifically asked for making the soc_device be a representation
of the whole soc with a hierarchical view of its child devices
under it, as opposed to having an artificial device node that only
serves to export strings along the lines of /proc/cpuinfo.

See patch 5/6 for the one that moves all platform devices that
are part of the dbx500 soc below the soc_device.

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Monday 17 October 2011, Greg KH wrote:
> > You also commented that the argument to soc_device_unregister should
> > be a soc_device (as, consequently, the return type of soc_device_register).
> > Agree with that comment, but it means that the definition of struct
> > soc_device needs to remain visible in order to be used as the parent
> > for other devices.
> 
> No it doesn't:
>         struct device * soc_device_to_device(struct soc device *soc);

Right, that works of course. I believe the more common way is to
expose the derived type to its users, and it also simplifies the
interface.

> Anyway, what are you using this soc device to be the parent of?

Basically everything. The SoC is probably about 90% of the system in
modern embedded systems. Typically, there are on-chip buses like
AMBA or PLB that contain dozens of internal devices (interrupt
controller, serial, dmaengine, rtc, timer, watchdog, ...) as well
as buses (i2c, spi, mmc, usb, pci, ...) that have off-chip child
devices. You can think of an soc device as a kind of ?ber-MFD
that holds all of these together.

If you remember the early discussions about this patch set, I
specifically asked for making the soc_device be a representation
of the whole soc with a hierarchical view of its child devices
under it, as opposed to having an artificial device node that only
serves to export strings along the lines of /proc/cpuinfo.

See patch 5/6 for the one that moves all platform devices that
are part of the dbx500 soc below the soc_device.

	Arnd

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

On 17/10/11 17:18, Greg KH wrote:
> On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
>> +void soc_device_unregister(struct device *dev)
> 
> Ick, no, pass in the struct soc_device, which the register function
> should return, not a "raw" struct device.

Okay.

>> +{
>> +	struct soc_device *soc_dev =
>> +		container_of(dev, struct soc_device, dev);
>> +
>> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
>> +
>> +	if (device_is_registered(dev))
>> +		device_unregister(dev);
> 
> Why is this call needed?

To unregister a previously unregistered device?

Is that wrong?

>> +
>> +	bus_unregister(&soc_bus_type);
> 
> What happens if you have more than one SOC device?  I think you just
> oopsed.

I think you're right.

When to you suggest we unregister the bus?

>> +
>> +	kfree(soc_dev->attr);
>> +	kfree(soc_dev);
> 
> Nope, you just failed again.  I can tell you never tried this code path,
> otherwise you would have noticed the HUGE warnings that the kernel spit
> back at you.  Please fix this.

I'll look into it.

Thanks.

Kind regards,
Lee

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

On 17/10/11 17:18, Greg KH wrote:
> On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
>> +void soc_device_unregister(struct device *dev)
> 
> Ick, no, pass in the struct soc_device, which the register function
> should return, not a "raw" struct device.

Okay.

>> +{
>> +	struct soc_device *soc_dev =
>> +		container_of(dev, struct soc_device, dev);
>> +
>> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
>> +
>> +	if (device_is_registered(dev))
>> +		device_unregister(dev);
> 
> Why is this call needed?

To unregister a previously unregistered device?

Is that wrong?

>> +
>> +	bus_unregister(&soc_bus_type);
> 
> What happens if you have more than one SOC device?  I think you just
> oopsed.

I think you're right.

When to you suggest we unregister the bus?

>> +
>> +	kfree(soc_dev->attr);
>> +	kfree(soc_dev);
> 
> Nope, you just failed again.  I can tell you never tried this code path,
> otherwise you would have noticed the HUGE warnings that the kernel spit
> back at you.  Please fix this.

I'll look into it.

Thanks.

Kind regards,
Lee

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

On 17/10/11 17:16, Greg KH wrote:
> On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
>> Traditionally, any System-on-Chip based platform creates a flat list
>> of platform_devices directly under /sys/devices/platform.
>>
>> In order to give these some better structure, this introduces a new
>> bus type for soc_devices that are registered with the new
>> soc_device_register() function.  All devices that are on the same
>> chip should then be registered as child devices of the soc device.
>>
>> The soc bus also exports a few standardised device attributes which
>> allow user space to query the specific type of soc.
>>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> The code is much better, and smaller, but there's still some issues with
> it:

I'll attempt to address them.

>> +static ssize_t soc_info_get(struct device *dev,
>> +			    struct device_attribute *attr,
>> +			    char *buf);
>> +
>> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
>> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
>> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
>> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
>> +
>> +static ssize_t soc_info_get(struct device *dev,
>> +			    struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	struct soc_device *soc_dev =
>> +		container_of(dev, struct soc_device, dev);
>> +
>> +	if (attr == &dev_attr_machine)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
>> +	if (attr == &dev_attr_family)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
>> +	if (attr == &dev_attr_revision)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
>> +	if (attr == &dev_attr_soc_id)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
>> +
>> +	return -EINVAL;
>> +
>> +}
> 
> If you move around things a bit here, you can save 4 lines of code,
> please do so.

Telepathy has never been my Forté. Would you mind alluding to which
lines I can trim please?

If you mean the placing the DEVICE_ATTR's after soc_info_get and
removing its prototype; I had that already, but I was told to do it this
way as it would make the binary smaller.

>> +
>> +struct bus_type soc_bus_type = {
>> +	.name  = "soc",
>> +};
>> +
>> +static int __init soc_bus_register(void)
>> +{
>> +	return bus_register(&soc_bus_type);
>> +}
>> +core_initcall(soc_bus_register);
> 
> No unregister?

The unregister is contained in soc_device_unregister, but as you point
out in your other email this should be removed. I'm guessing we should
either count the number of SoCs and unregister as the last one leaves,
or not unregister at all?

>> +struct attribute *soc_attr[] = {
>> +	&dev_attr_machine.attr,
>> +	&dev_attr_family.attr,
>> +	&dev_attr_soc_id.attr,
>> +	&dev_attr_revision.attr,
>> +	NULL,
>> +};
>> +
>> +struct attribute_group soc_attr_group = {
>> +	.attrs = soc_attr,
>> +};
>> +
>> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
>> +{
>> +	struct soc_device *soc_dev;
>> +	static atomic_t soc_device_num = ATOMIC_INIT(0);
> 
> No, please don't do this, use the proper kernel interface to dynamically
> handle numbering devices (hint, if you unload a SOC device, you will
> never reclaim that device number, which isn't that nice.)

Again, some help would really be appreciated here. I searched the kernel
last time you mentioned numbering, but this is all I came up with.

>> +struct soc_device_attribute {
>> +	const char *machine;
>> +	const char *family;
>> +	const char *revision;
>> +	const char *soc_id;
>> +};
> 
> What happens if one of these attributes is NULL?  Please check for that
> when you create the attributes so that you don't create an attribute you
> don't want to.

By shifting around "struct attribute *soc_attr[]"?

If one of them is NULL the file will just be empty, is that such a bad
thing?

>> +
>> +struct soc_device {
>> +	struct device dev;
>> +	struct soc_device_attribute *attr;
>> +};
> 
> Why is this needed to be defined here?  It should be in the .c file as
> no external code needs to know what it looks like.

If you want me to pass back a "struct soc_device" instead of a "raw"
struct device, the calling code will need to know how to separate out
"struct device dev" won't it?

> thanks,

No, thank you.

Kind regards,
Lee

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

On 17/10/11 17:16, Greg KH wrote:
> On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
>> Traditionally, any System-on-Chip based platform creates a flat list
>> of platform_devices directly under /sys/devices/platform.
>>
>> In order to give these some better structure, this introduces a new
>> bus type for soc_devices that are registered with the new
>> soc_device_register() function.  All devices that are on the same
>> chip should then be registered as child devices of the soc device.
>>
>> The soc bus also exports a few standardised device attributes which
>> allow user space to query the specific type of soc.
>>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> The code is much better, and smaller, but there's still some issues with
> it:

I'll attempt to address them.

>> +static ssize_t soc_info_get(struct device *dev,
>> +			    struct device_attribute *attr,
>> +			    char *buf);
>> +
>> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
>> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
>> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
>> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
>> +
>> +static ssize_t soc_info_get(struct device *dev,
>> +			    struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	struct soc_device *soc_dev =
>> +		container_of(dev, struct soc_device, dev);
>> +
>> +	if (attr == &dev_attr_machine)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
>> +	if (attr == &dev_attr_family)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
>> +	if (attr == &dev_attr_revision)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
>> +	if (attr == &dev_attr_soc_id)
>> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
>> +
>> +	return -EINVAL;
>> +
>> +}
> 
> If you move around things a bit here, you can save 4 lines of code,
> please do so.

Telepathy has never been my Fort?. Would you mind alluding to which
lines I can trim please?

If you mean the placing the DEVICE_ATTR's after soc_info_get and
removing its prototype; I had that already, but I was told to do it this
way as it would make the binary smaller.

>> +
>> +struct bus_type soc_bus_type = {
>> +	.name  = "soc",
>> +};
>> +
>> +static int __init soc_bus_register(void)
>> +{
>> +	return bus_register(&soc_bus_type);
>> +}
>> +core_initcall(soc_bus_register);
> 
> No unregister?

The unregister is contained in soc_device_unregister, but as you point
out in your other email this should be removed. I'm guessing we should
either count the number of SoCs and unregister as the last one leaves,
or not unregister at all?

>> +struct attribute *soc_attr[] = {
>> +	&dev_attr_machine.attr,
>> +	&dev_attr_family.attr,
>> +	&dev_attr_soc_id.attr,
>> +	&dev_attr_revision.attr,
>> +	NULL,
>> +};
>> +
>> +struct attribute_group soc_attr_group = {
>> +	.attrs = soc_attr,
>> +};
>> +
>> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
>> +{
>> +	struct soc_device *soc_dev;
>> +	static atomic_t soc_device_num = ATOMIC_INIT(0);
> 
> No, please don't do this, use the proper kernel interface to dynamically
> handle numbering devices (hint, if you unload a SOC device, you will
> never reclaim that device number, which isn't that nice.)

Again, some help would really be appreciated here. I searched the kernel
last time you mentioned numbering, but this is all I came up with.

>> +struct soc_device_attribute {
>> +	const char *machine;
>> +	const char *family;
>> +	const char *revision;
>> +	const char *soc_id;
>> +};
> 
> What happens if one of these attributes is NULL?  Please check for that
> when you create the attributes so that you don't create an attribute you
> don't want to.

By shifting around "struct attribute *soc_attr[]"?

If one of them is NULL the file will just be empty, is that such a bad
thing?

>> +
>> +struct soc_device {
>> +	struct device dev;
>> +	struct soc_device_attribute *attr;
>> +};
> 
> Why is this needed to be defined here?  It should be in the .c file as
> no external code needs to know what it looks like.

If you want me to pass back a "struct soc_device" instead of a "raw"
struct device, the calling code will need to know how to separate out
"struct device dev" won't it?

> thanks,

No, thank you.

Kind regards,
Lee

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Oct 17, 2011 at 08:03:42PM +0200, Arnd Bergmann wrote:
> On Monday 17 October 2011 09:16:16 Greg KH wrote:
> > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> 
> > > +static ssize_t soc_info_get(struct device *dev,
> > > +                         struct device_attribute *attr,
> > > +                         char *buf);
> > > +
> > > +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> > > +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> > > +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> > > +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> > > +
> > > +static ssize_t soc_info_get(struct device *dev,
> > > +                         struct device_attribute *attr,
> > > +                         char *buf)
> > > +{
> > > +     struct soc_device *soc_dev =
> > > +             container_of(dev, struct soc_device, dev);
> > > +
> > > +     if (attr == &dev_attr_machine)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->machine);
> > > +     if (attr == &dev_attr_family)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->family);
> > > +     if (attr == &dev_attr_revision)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->revision);
> > > +     if (attr == &dev_attr_soc_id)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> > > +
> > > +     return -EINVAL;
> > > +
> > > +}
> > 
> > If you move around things a bit here, you can save 4 lines of code,
> > please do so.
> 
> I don't think that works: the DEVICE_ATTR definitions require a prototype
> for the function, and the function compares the device attribute.

Ah, yeah, you are right.

> > > +struct soc_device {
> > > +     struct device dev;
> > > +     struct soc_device_attribute *attr;
> > > +};
> > 
> > Why is this needed to be defined here?  It should be in the .c file as
> > no external code needs to know what it looks like.
> 
> You also commented that the argument to soc_device_unregister should
> be a soc_device (as, consequently, the return type of soc_device_register).
> Agree with that comment, but it means that the definition of struct
> soc_device needs to remain visible in order to be used as the parent
> for other devices.

No it doesn't:
	struct device * soc_device_to_device(struct soc device *soc);

Anyway, what are you using this soc device to be the parent of?

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Oct 17, 2011 at 08:03:42PM +0200, Arnd Bergmann wrote:
> On Monday 17 October 2011 09:16:16 Greg KH wrote:
> > On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> 
> > > +static ssize_t soc_info_get(struct device *dev,
> > > +                         struct device_attribute *attr,
> > > +                         char *buf);
> > > +
> > > +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> > > +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> > > +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> > > +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> > > +
> > > +static ssize_t soc_info_get(struct device *dev,
> > > +                         struct device_attribute *attr,
> > > +                         char *buf)
> > > +{
> > > +     struct soc_device *soc_dev =
> > > +             container_of(dev, struct soc_device, dev);
> > > +
> > > +     if (attr == &dev_attr_machine)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->machine);
> > > +     if (attr == &dev_attr_family)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->family);
> > > +     if (attr == &dev_attr_revision)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->revision);
> > > +     if (attr == &dev_attr_soc_id)
> > > +             return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> > > +
> > > +     return -EINVAL;
> > > +
> > > +}
> > 
> > If you move around things a bit here, you can save 4 lines of code,
> > please do so.
> 
> I don't think that works: the DEVICE_ATTR definitions require a prototype
> for the function, and the function compares the device attribute.

Ah, yeah, you are right.

> > > +struct soc_device {
> > > +     struct device dev;
> > > +     struct soc_device_attribute *attr;
> > > +};
> > 
> > Why is this needed to be defined here?  It should be in the .c file as
> > no external code needs to know what it looks like.
> 
> You also commented that the argument to soc_device_unregister should
> be a soc_device (as, consequently, the return type of soc_device_register).
> Agree with that comment, but it means that the definition of struct
> soc_device needs to remain visible in order to be used as the parent
> for other devices.

No it doesn't:
	struct device * soc_device_to_device(struct soc device *soc);

Anyway, what are you using this soc device to be the parent of?

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Monday 17 October 2011 09:16:16 Greg KH wrote:
> On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:

> > +static ssize_t soc_info_get(struct device *dev,
> > +                         struct device_attribute *attr,
> > +                         char *buf);
> > +
> > +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> > +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> > +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> > +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> > +
> > +static ssize_t soc_info_get(struct device *dev,
> > +                         struct device_attribute *attr,
> > +                         char *buf)
> > +{
> > +     struct soc_device *soc_dev =
> > +             container_of(dev, struct soc_device, dev);
> > +
> > +     if (attr == &dev_attr_machine)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->machine);
> > +     if (attr == &dev_attr_family)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->family);
> > +     if (attr == &dev_attr_revision)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->revision);
> > +     if (attr == &dev_attr_soc_id)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> > +
> > +     return -EINVAL;
> > +
> > +}
> 
> If you move around things a bit here, you can save 4 lines of code,
> please do so.

I don't think that works: the DEVICE_ATTR definitions require a prototype
for the function, and the function compares the device attribute.

An earlier version of this patch avoided the forward declaration by doing
a more expensive strcmp instead of the pointer comparison, which avoided
this problem, and I recommended against that.

> > +
> > +struct soc_device {
> > +     struct device dev;
> > +     struct soc_device_attribute *attr;
> > +};
> 
> Why is this needed to be defined here?  It should be in the .c file as
> no external code needs to know what it looks like.

You also commented that the argument to soc_device_unregister should
be a soc_device (as, consequently, the return type of soc_device_register).
Agree with that comment, but it means that the definition of struct
soc_device needs to remain visible in order to be used as the parent
for other devices.

	Arnd

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Arnd Bergmann]

On Monday 17 October 2011 09:16:16 Greg KH wrote:
> On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:

> > +static ssize_t soc_info_get(struct device *dev,
> > +                         struct device_attribute *attr,
> > +                         char *buf);
> > +
> > +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> > +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> > +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> > +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> > +
> > +static ssize_t soc_info_get(struct device *dev,
> > +                         struct device_attribute *attr,
> > +                         char *buf)
> > +{
> > +     struct soc_device *soc_dev =
> > +             container_of(dev, struct soc_device, dev);
> > +
> > +     if (attr == &dev_attr_machine)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->machine);
> > +     if (attr == &dev_attr_family)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->family);
> > +     if (attr == &dev_attr_revision)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->revision);
> > +     if (attr == &dev_attr_soc_id)
> > +             return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> > +
> > +     return -EINVAL;
> > +
> > +}
> 
> If you move around things a bit here, you can save 4 lines of code,
> please do so.

I don't think that works: the DEVICE_ATTR definitions require a prototype
for the function, and the function compares the device attribute.

An earlier version of this patch avoided the forward declaration by doing
a more expensive strcmp instead of the pointer comparison, which avoided
this problem, and I recommended against that.

> > +
> > +struct soc_device {
> > +     struct device dev;
> > +     struct soc_device_attribute *attr;
> > +};
> 
> Why is this needed to be defined here?  It should be in the .c file as
> no external code needs to know what it looks like.

You also commented that the argument to soc_device_unregister should
be a soc_device (as, consequently, the return type of soc_device_register).
Agree with that comment, but it means that the definition of struct
soc_device needs to remain visible in order to be used as the parent
for other devices.

	Arnd

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> +void soc_device_unregister(struct device *dev)

Ick, no, pass in the struct soc_device, which the register function
should return, not a "raw" struct device.


> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
> +
> +	if (device_is_registered(dev))
> +		device_unregister(dev);

Why is this call needed?

> +
> +	bus_unregister(&soc_bus_type);

What happens if you have more than one SOC device?  I think you just
oopsed.

> +
> +	kfree(soc_dev->attr);
> +	kfree(soc_dev);

Nope, you just failed again.  I can tell you never tried this code path,
otherwise you would have noticed the HUGE warnings that the kernel spit
back at you.  Please fix this.

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> +void soc_device_unregister(struct device *dev)

Ick, no, pass in the struct soc_device, which the register function
should return, not a "raw" struct device.


> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
> +
> +	if (device_is_registered(dev))
> +		device_unregister(dev);

Why is this call needed?

> +
> +	bus_unregister(&soc_bus_type);

What happens if you have more than one SOC device?  I think you just
oopsed.

> +
> +	kfree(soc_dev->attr);
> +	kfree(soc_dev);

Nope, you just failed again.  I can tell you never tried this code path,
otherwise you would have noticed the HUGE warnings that the kernel spit
back at you.  Please fix this.

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> Traditionally, any System-on-Chip based platform creates a flat list
> of platform_devices directly under /sys/devices/platform.
> 
> In order to give these some better structure, this introduces a new
> bus type for soc_devices that are registered with the new
> soc_device_register() function.  All devices that are on the same
> chip should then be registered as child devices of the soc device.
> 
> The soc bus also exports a few standardised device attributes which
> allow user space to query the specific type of soc.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

The code is much better, and smaller, but there's still some issues with
it:

> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf);
> +
> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	if (attr == &dev_attr_machine)
> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
> +	if (attr == &dev_attr_family)
> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
> +	if (attr == &dev_attr_revision)
> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
> +	if (attr == &dev_attr_soc_id)
> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> +
> +	return -EINVAL;
> +
> +}

If you move around things a bit here, you can save 4 lines of code,
please do so.

> +
> +struct bus_type soc_bus_type = {
> +	.name  = "soc",
> +};
> +
> +static int __init soc_bus_register(void)
> +{
> +	return bus_register(&soc_bus_type);
> +}
> +core_initcall(soc_bus_register);

No unregister?

> +struct attribute *soc_attr[] = {
> +	&dev_attr_machine.attr,
> +	&dev_attr_family.attr,
> +	&dev_attr_soc_id.attr,
> +	&dev_attr_revision.attr,
> +	NULL,
> +};
> +
> +struct attribute_group soc_attr_group = {
> +	.attrs = soc_attr,
> +};
> +
> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	static atomic_t soc_device_num = ATOMIC_INIT(0);

No, please don't do this, use the proper kernel interface to dynamically
handle numbering devices (hint, if you unload a SOC device, you will
never reclaim that device number, which isn't that nice.)

> +struct soc_device_attribute {
> +	const char *machine;
> +	const char *family;
> +	const char *revision;
> +	const char *soc_id;
> +};

What happens if one of these attributes is NULL?  Please check for that
when you create the attributes so that you don't create an attribute you
don't want to.

> +
> +struct soc_device {
> +	struct device dev;
> +	struct soc_device_attribute *attr;
> +};

Why is this needed to be defined here?  It should be in the .c file as
no external code needs to know what it looks like.

thanks,

greg k-h

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Greg KH]

On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
> Traditionally, any System-on-Chip based platform creates a flat list
> of platform_devices directly under /sys/devices/platform.
> 
> In order to give these some better structure, this introduces a new
> bus type for soc_devices that are registered with the new
> soc_device_register() function.  All devices that are on the same
> chip should then be registered as child devices of the soc device.
> 
> The soc bus also exports a few standardised device attributes which
> allow user space to query the specific type of soc.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

The code is much better, and smaller, but there's still some issues with
it:

> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf);
> +
> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	if (attr == &dev_attr_machine)
> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
> +	if (attr == &dev_attr_family)
> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
> +	if (attr == &dev_attr_revision)
> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
> +	if (attr == &dev_attr_soc_id)
> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> +
> +	return -EINVAL;
> +
> +}

If you move around things a bit here, you can save 4 lines of code,
please do so.

> +
> +struct bus_type soc_bus_type = {
> +	.name  = "soc",
> +};
> +
> +static int __init soc_bus_register(void)
> +{
> +	return bus_register(&soc_bus_type);
> +}
> +core_initcall(soc_bus_register);

No unregister?

> +struct attribute *soc_attr[] = {
> +	&dev_attr_machine.attr,
> +	&dev_attr_family.attr,
> +	&dev_attr_soc_id.attr,
> +	&dev_attr_revision.attr,
> +	NULL,
> +};
> +
> +struct attribute_group soc_attr_group = {
> +	.attrs = soc_attr,
> +};
> +
> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	static atomic_t soc_device_num = ATOMIC_INIT(0);

No, please don't do this, use the proper kernel interface to dynamically
handle numbering devices (hint, if you unload a SOC device, you will
never reclaim that device number, which isn't that nice.)

> +struct soc_device_attribute {
> +	const char *machine;
> +	const char *family;
> +	const char *revision;
> +	const char *soc_id;
> +};

What happens if one of these attributes is NULL?  Please check for that
when you create the attributes so that you don't create an attribute you
don't want to.

> +
> +struct soc_device {
> +	struct device dev;
> +	struct soc_device_attribute *attr;
> +};

Why is this needed to be defined here?  It should be in the .c file as
no external code needs to know what it looks like.

thanks,

greg k-h

Re: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

Hi Lee,

I'll have a proper look and test at this later, but it looks pretty good 
to me, so just a couple of minor nits.

Jamie

On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
[...]
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..1578e4e
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,115 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/err.h>
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf);
> +
> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	if (attr == &dev_attr_machine)
> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
> +	if (attr == &dev_attr_family)
> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
> +	if (attr == &dev_attr_revision)
> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
> +	if (attr == &dev_attr_soc_id)
> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> +
> +	return -EINVAL;
> +
> +}
> +
> +struct bus_type soc_bus_type = {
> +	.name  = "soc",
> +};

static?

> +
> +static int __init soc_bus_register(void)
> +{
> +	return bus_register(&soc_bus_type);
> +}
> +core_initcall(soc_bus_register);
> +
> +struct attribute *soc_attr[] = {
> +	&dev_attr_machine.attr,
> +	&dev_attr_family.attr,
> +	&dev_attr_soc_id.attr,
> +	&dev_attr_revision.attr,
> +	NULL,
> +};

static?

> +
> +struct attribute_group soc_attr_group = {
> +	.attrs = soc_attr,
> +};

static const?

> +
> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	static atomic_t soc_device_num = ATOMIC_INIT(0);
> +	int ret;
> +
> +	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> +	if (!soc_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	soc_dev->attr = soc_dev_attr;
> +	soc_dev->dev.bus = &soc_bus_type;
> +
> +	dev_set_name(&soc_dev->dev, "soc%d",
> +		atomic_inc_return(&soc_device_num) - 1);
> +
> +	ret = device_register(&soc_dev->dev);
> +	if (ret)
> +		goto out1;
> +
> +	ret = sysfs_create_group(&soc_dev->dev.kobj, &soc_attr_group);
> +	if (ret)
> +		goto out2;

If you create an array of struct attribute_group's and set 
soc_dev->dev.groups to that then the driver core will handle creating 
the attributes for you and removing them at cleanup.  It also means the 
attributes will get created at device creation time so there isn't a 
potential race with udev events looking for attributes that haven't been 
created yet.

> +
> +	return &soc_dev->dev;
> +
> +out2:
> +	device_unregister(&soc_dev->dev);
> +out1:
> +	kfree(soc_dev);
> +	kfree(soc_dev->attr);
> +	return ERR_PTR(ret);
> +}
> +
> +void soc_device_unregister(struct device *dev)
> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
> +
> +	if (device_is_registered(dev))
> +		device_unregister(dev);

I don't think you need this - I think it's fine to say that a 
precondition of calling this is that the device is registered (that's 
what the platform_device layer does).

> +
> +	bus_unregister(&soc_bus_type);

I don't think this is quite right - we're freeing the bus on the first 
unregister and we won't be able to register any more soc devices.  As 
this can't sensibly be a loadable module I think it's fine to leave it 
always registered.

> +
> +	kfree(soc_dev->attr);
> +	kfree(soc_dev);

device_unregister() should do this for you.  The device could still be 
hanging around after device_unregister() if there are active references 
to it so we don't want to free the memory too early.  We should have a 
release function in the struct device set in soc_device_register that 
frees the struct soc_device.

> +}
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..2485a0f2
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SOC_BUS_H
> +#define __SOC_BUS_H
> +
> +#include <linux/kobject.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>

I'm not sure you need kobject.h and platform_device.h here.

> +
> +struct soc_device_attribute {
> +	const char *machine;
> +	const char *family;
> +	const char *revision;
> +	const char *soc_id;
> +};
> +
> +struct soc_device {
> +	struct device dev;
> +	struct soc_device_attribute *attr;
> +};

This can be private to the implementation file.

> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
> + */
> +struct device *soc_device_register(
> +	struct soc_device_attribute *soc_plat_dev_attr);
> +
> +/**
> + * soc_device_unregister - unregister SoC device
> + * @dev: SoC device to be unregistered
> + */
> +void soc_device_unregister(struct device *dev);
> +
> +#endif /* __SOC_BUS_H */
> -- 
> 1.7.5.4
> 

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Jamie Iles]

Hi Lee,

I'll have a proper look and test at this later, but it looks pretty good 
to me, so just a couple of minor nits.

Jamie

On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
[...]
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..1578e4e
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,115 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/err.h>
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf);
> +
> +static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
> +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
> +
> +static ssize_t soc_info_get(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	if (attr == &dev_attr_machine)
> +		return sprintf(buf, "%s\n", soc_dev->attr->machine);
> +	if (attr == &dev_attr_family)
> +		return sprintf(buf, "%s\n", soc_dev->attr->family);
> +	if (attr == &dev_attr_revision)
> +		return sprintf(buf, "%s\n", soc_dev->attr->revision);
> +	if (attr == &dev_attr_soc_id)
> +		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
> +
> +	return -EINVAL;
> +
> +}
> +
> +struct bus_type soc_bus_type = {
> +	.name  = "soc",
> +};

static?

> +
> +static int __init soc_bus_register(void)
> +{
> +	return bus_register(&soc_bus_type);
> +}
> +core_initcall(soc_bus_register);
> +
> +struct attribute *soc_attr[] = {
> +	&dev_attr_machine.attr,
> +	&dev_attr_family.attr,
> +	&dev_attr_soc_id.attr,
> +	&dev_attr_revision.attr,
> +	NULL,
> +};

static?

> +
> +struct attribute_group soc_attr_group = {
> +	.attrs = soc_attr,
> +};

static const?

> +
> +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct soc_device *soc_dev;
> +	static atomic_t soc_device_num = ATOMIC_INIT(0);
> +	int ret;
> +
> +	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
> +	if (!soc_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	soc_dev->attr = soc_dev_attr;
> +	soc_dev->dev.bus = &soc_bus_type;
> +
> +	dev_set_name(&soc_dev->dev, "soc%d",
> +		atomic_inc_return(&soc_device_num) - 1);
> +
> +	ret = device_register(&soc_dev->dev);
> +	if (ret)
> +		goto out1;
> +
> +	ret = sysfs_create_group(&soc_dev->dev.kobj, &soc_attr_group);
> +	if (ret)
> +		goto out2;

If you create an array of struct attribute_group's and set 
soc_dev->dev.groups to that then the driver core will handle creating 
the attributes for you and removing them at cleanup.  It also means the 
attributes will get created at device creation time so there isn't a 
potential race with udev events looking for attributes that haven't been 
created yet.

> +
> +	return &soc_dev->dev;
> +
> +out2:
> +	device_unregister(&soc_dev->dev);
> +out1:
> +	kfree(soc_dev);
> +	kfree(soc_dev->attr);
> +	return ERR_PTR(ret);
> +}
> +
> +void soc_device_unregister(struct device *dev)
> +{
> +	struct soc_device *soc_dev =
> +		container_of(dev, struct soc_device, dev);
> +
> +	sysfs_remove_group(&dev->kobj, &soc_attr_group);
> +
> +	if (device_is_registered(dev))
> +		device_unregister(dev);

I don't think you need this - I think it's fine to say that a 
precondition of calling this is that the device is registered (that's 
what the platform_device layer does).

> +
> +	bus_unregister(&soc_bus_type);

I don't think this is quite right - we're freeing the bus on the first 
unregister and we won't be able to register any more soc devices.  As 
this can't sensibly be a loadable module I think it's fine to leave it 
always registered.

> +
> +	kfree(soc_dev->attr);
> +	kfree(soc_dev);

device_unregister() should do this for you.  The device could still be 
hanging around after device_unregister() if there are active references 
to it so we don't want to free the memory too early.  We should have a 
release function in the struct device set in soc_device_register that 
frees the struct soc_device.

> +}
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..2485a0f2
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SOC_BUS_H
> +#define __SOC_BUS_H
> +
> +#include <linux/kobject.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>

I'm not sure you need kobject.h and platform_device.h here.

> +
> +struct soc_device_attribute {
> +	const char *machine;
> +	const char *family;
> +	const char *revision;
> +	const char *soc_id;
> +};
> +
> +struct soc_device {
> +	struct device dev;
> +	struct soc_device_attribute *attr;
> +};

This can be private to the implementation file.

> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
> + */
> +struct device *soc_device_register(
> +	struct soc_device_attribute *soc_plat_dev_attr);
> +
> +/**
> + * soc_device_unregister - unregister SoC device
> + * @dev: SoC device to be unregistered
> + */
> +void soc_device_unregister(struct device *dev);
> +
> +#endif /* __SOC_BUS_H */
> -- 
> 1.7.5.4
> 

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

Traditionally, any System-on-Chip based platform creates a flat list
of platform_devices directly under /sys/devices/platform.

In order to give these some better structure, this introduces a new
bus type for soc_devices that are registered with the new
soc_device_register() function.  All devices that are on the same
chip should then be registered as child devices of the soc device.

The soc bus also exports a few standardised device attributes which
allow user space to query the specific type of soc.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  115 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   38 +++++++++++++++
 4 files changed, 157 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 21cf46f..707aeaf 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SOC_BUS
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 99a375a..6b84b36 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SOC_BUS) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..1578e4e
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+
+static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct soc_device *soc_dev =
+		container_of(dev, struct soc_device, dev);
+
+	if (attr == &dev_attr_machine)
+		return sprintf(buf, "%s\n", soc_dev->attr->machine);
+	if (attr == &dev_attr_family)
+		return sprintf(buf, "%s\n", soc_dev->attr->family);
+	if (attr == &dev_attr_revision)
+		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_soc_id)
+		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+
+	return -EINVAL;
+
+}
+
+struct bus_type soc_bus_type = {
+	.name  = "soc",
+};
+
+static int __init soc_bus_register(void)
+{
+	return bus_register(&soc_bus_type);
+}
+core_initcall(soc_bus_register);
+
+struct attribute *soc_attr[] = {
+	&dev_attr_machine.attr,
+	&dev_attr_family.attr,
+	&dev_attr_soc_id.attr,
+	&dev_attr_revision.attr,
+	NULL,
+};
+
+struct attribute_group soc_attr_group = {
+	.attrs = soc_attr,
+};
+
+struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
+{
+	struct soc_device *soc_dev;
+	static atomic_t soc_device_num = ATOMIC_INIT(0);
+	int ret;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (!soc_dev)
+		return ERR_PTR(-ENOMEM);
+
+	soc_dev->attr = soc_dev_attr;
+	soc_dev->dev.bus = &soc_bus_type;
+
+	dev_set_name(&soc_dev->dev, "soc%d",
+		atomic_inc_return(&soc_device_num) - 1);
+
+	ret = device_register(&soc_dev->dev);
+	if (ret)
+		goto out1;
+
+	ret = sysfs_create_group(&soc_dev->dev.kobj, &soc_attr_group);
+	if (ret)
+		goto out2;
+
+	return &soc_dev->dev;
+
+out2:
+	device_unregister(&soc_dev->dev);
+out1:
+	kfree(soc_dev);
+	kfree(soc_dev->attr);
+	return ERR_PTR(ret);
+}
+
+void soc_device_unregister(struct device *dev)
+{
+	struct soc_device *soc_dev =
+		container_of(dev, struct soc_device, dev);
+
+	sysfs_remove_group(&dev->kobj, &soc_attr_group);
+
+	if (device_is_registered(dev))
+		device_unregister(dev);
+
+	bus_unregister(&soc_bus_type);
+
+	kfree(soc_dev->attr);
+	kfree(soc_dev);
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..2485a0f2
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SOC_BUS_H
+#define __SOC_BUS_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct soc_device_attribute {
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *soc_id;
+};
+
+struct soc_device {
+	struct device dev;
+	struct soc_device_attribute *attr;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
+ */
+struct device *soc_device_register(
+	struct soc_device_attribute *soc_plat_dev_attr);
+
+/**
+ * soc_device_unregister - unregister SoC device
+ * @dev: SoC device to be unregistered
+ */
+void soc_device_unregister(struct device *dev);
+
+#endif /* __SOC_BUS_H */
-- 
1.7.5.4

[PATCH 2/6] drivers/base: add bus for System-on-Chip devices

[Lee Jones]

Traditionally, any System-on-Chip based platform creates a flat list
of platform_devices directly under /sys/devices/platform.

In order to give these some better structure, this introduces a new
bus type for soc_devices that are registered with the new
soc_device_register() function.  All devices that are on the same
chip should then be registered as child devices of the soc device.

The soc bus also exports a few standardised device attributes which
allow user space to query the specific type of soc.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  115 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   38 +++++++++++++++
 4 files changed, 157 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 21cf46f..707aeaf 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SOC_BUS
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 99a375a..6b84b36 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SOC_BUS) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
new file mode 100644
index 0000000..1578e4e
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+
+static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
+
+static ssize_t soc_info_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct soc_device *soc_dev =
+		container_of(dev, struct soc_device, dev);
+
+	if (attr == &dev_attr_machine)
+		return sprintf(buf, "%s\n", soc_dev->attr->machine);
+	if (attr == &dev_attr_family)
+		return sprintf(buf, "%s\n", soc_dev->attr->family);
+	if (attr == &dev_attr_revision)
+		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_soc_id)
+		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
+
+	return -EINVAL;
+
+}
+
+struct bus_type soc_bus_type = {
+	.name  = "soc",
+};
+
+static int __init soc_bus_register(void)
+{
+	return bus_register(&soc_bus_type);
+}
+core_initcall(soc_bus_register);
+
+struct attribute *soc_attr[] = {
+	&dev_attr_machine.attr,
+	&dev_attr_family.attr,
+	&dev_attr_soc_id.attr,
+	&dev_attr_revision.attr,
+	NULL,
+};
+
+struct attribute_group soc_attr_group = {
+	.attrs = soc_attr,
+};
+
+struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
+{
+	struct soc_device *soc_dev;
+	static atomic_t soc_device_num = ATOMIC_INIT(0);
+	int ret;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (!soc_dev)
+		return ERR_PTR(-ENOMEM);
+
+	soc_dev->attr = soc_dev_attr;
+	soc_dev->dev.bus = &soc_bus_type;
+
+	dev_set_name(&soc_dev->dev, "soc%d",
+		atomic_inc_return(&soc_device_num) - 1);
+
+	ret = device_register(&soc_dev->dev);
+	if (ret)
+		goto out1;
+
+	ret = sysfs_create_group(&soc_dev->dev.kobj, &soc_attr_group);
+	if (ret)
+		goto out2;
+
+	return &soc_dev->dev;
+
+out2:
+	device_unregister(&soc_dev->dev);
+out1:
+	kfree(soc_dev);
+	kfree(soc_dev->attr);
+	return ERR_PTR(ret);
+}
+
+void soc_device_unregister(struct device *dev)
+{
+	struct soc_device *soc_dev =
+		container_of(dev, struct soc_device, dev);
+
+	sysfs_remove_group(&dev->kobj, &soc_attr_group);
+
+	if (device_is_registered(dev))
+		device_unregister(dev);
+
+	bus_unregister(&soc_bus_type);
+
+	kfree(soc_dev->attr);
+	kfree(soc_dev);
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..2485a0f2
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SOC_BUS_H
+#define __SOC_BUS_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct soc_device_attribute {
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *soc_id;
+};
+
+struct soc_device {
+	struct device dev;
+	struct soc_device_attribute *attr;
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
+ */
+struct device *soc_device_register(
+	struct soc_device_attribute *soc_plat_dev_attr);
+
+/**
+ * soc_device_unregister - unregister SoC device
+ * @dev: SoC device to be unregistered
+ */
+void soc_device_unregister(struct device *dev);
+
+#endif /* __SOC_BUS_H */
-- 
1.7.5.4