Framework for exporting System-on-Chip information via sysfs

Here we make use of the new drivers/base/soc 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/[1|2|3|...]/[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 <[email protected]>

}

arch/arm/mach-ux500/id.c +33 -0

+
+struct soc_device *soc_dev;
+
+static const char *ux500_get_machine(void)
+{
+	return kasprintf(GFP_KERNEL, "DB%4x", dbx500_partnumber());
+}
+
+static const char *ux500_get_family(void)
+{
+	return kasprintf(GFP_KERNEL, "Ux500");
+}
+
+static const char *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");

arch/arm/mach-ux500/id.c +15 -0

+struct device_attribute ux500_soc_attrs[] = {
+	__ATTR(process,  S_IRUGO, ux500_get_process,  NULL),
+	__ATTR_NULL,
+};
+
+static int __init ux500_soc_sysfs_init(void)
+{
+	int ret;
+	int i = 0;
+
+	soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
+	if (soc_dev == NULL)
+		return -ENOMEM;
+
+	soc_info_populate(soc_dev);

arch/arm/mach-ux500/cpu-db8500.c static void __init db8500_add_gpios(void) · line 182

Quoted 13 lines · Lee Jones

arch/arm/mach-ux500/cpu-db8500.c +13 -0

+static const char *db8500_get_soc_id(void)
+{
+       void __iomem *uid_base;
+       char buf[1024];
+       ssize_t sz = 0;
+       int i;
+
+       uid_base = __io_address(U8500_BB_UID_BASE);
+       for (i = 0; i < U8500_BB_UID_LENGTH; i++) {
+               sz += sprintf(buf + sz, "%08x", readl(uid_base + i * sizeof(u32)));
+       }
+       return kasprintf(GFP_KERNEL, "%s", buf);
+}

You will get a warning from the stack checker here, about putting a 1024 byte string on the stack. Also, I still think it's bad to just access the U8500_BB_UID_BASE from a compile-time constant. Since this gets called from a function that knows the base address of the DB8500 register area, better pass the device in there so that you end up with something like

static void __devinit db8500_read_soc_id(struct db8500_dev *dev)

{
	u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET;
	snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x",
		readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4]));
}

The style you use here is preexisting in the db8500 code, but you should not keep adding more of that crap.

All the code like

#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)

should never really have been there. What you want to do for this is to call this from the code that initializes the db8500 controller, and pass the board specific pdata into the db8500 init function, along with the i2c client data.

Quoted 6 lines

static void __devinit db8500_read_soc_id(struct db8500_dev *dev)

{
      u32 __iomem *uid = dev->base + U8500_BB_UID_OFFSET;
      snprintf(dev->soc_id, sizeof (dev->soc_id), "%08x%08x%08x%08x%08x",
              readl(uid[0]), readl(uid[1]), readl(uid[2]), readl(uid[3]), readl(uid[4]));
}

No problem.

Where did you get dev->base from though?

Quoted 2 lines

The style you use here is preexisting in the db8500 code, but you should not keep adding more of that crap.

Not meaning to pass the buck at all, but this isn't my code.

It's remnant from when I took this over (starting to wish I hadn't ;))

Quoted 6 lines

All the code like

#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)

Quoted 4 lines

should never really have been there. What you want to do for this is to call this from the code that initializes the db8500 controller, and pass the board specific pdata into the db8500 init function, along with the i2c client data.

Not my domain. Speak to Linus W. :)

This patch introduces a new driver to drivers/base. The driver provides a means to export vital SoC data out to userspace via sysfs. Standard information applicable to all SoCs are exported to:

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

It is possible to create SoC specific items via the soc device, which is returned post-registration, although this should be discouraged.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  103 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   27 ++++++++++++
 4 files changed, 134 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

drivers/base/Kconfig +3 -0

index 21cf46f..95f10c5 100644
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu

drivers/base/Makefile +1 -0

index 99a375a..a67a1e7 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SYS_SOC) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 

drivers/base/soc.c new file +5 -0

new file mode 100644
index 0000000..e9d908b
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2

drivers/base/soc.c new file +6 -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);
+

drivers/base/soc.c new file +5 -0

+}
+
+struct device_attribute soc_attrs[] = {
+	__ATTR(machine,  S_IRUGO, soc_info_get,  NULL),
+	__ATTR(family,   S_IRUGO, soc_info_get,  NULL),

drivers/base/soc.c new file +7 -0

+
+static int __init soc_device_create_files(struct device *dev)
+{
+	int ret = 0;
+	int i   = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {

drivers/base/soc.c new file +3 -0

+
+	return ret;
+}

include/linux/sys_soc.h new file +9 -0

new file mode 100644
index 0000000..811d7fe
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>

Quoted 63 lines · Lee Jones

This patch introduces a new driver to drivers/base. The driver provides a means to export vital SoC data out to userspace via sysfs. Standard information applicable to all SoCs are exported to:

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

It is possible to create SoC specific items via the soc device, which is returned post-registration, although this should be discouraged.

Signed-off-by Lee Jones <[email protected]>

---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  103 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   27 ++++++++++++
 4 files changed, 134 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/soc.c
 create mode 100644 include/linux/sys_soc.h

drivers/base/Kconfig +3 -0

index 21cf46f..95f10c5 100644
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu

drivers/base/Makefile +1 -0

index 99a375a..a67a1e7 100644
@@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
+obj-$(CONFIG_SYS_SOC) += soc.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 

drivers/base/soc.c new file +10 -0

new file mode 100644
index 0000000..e9d908b
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ *
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/sysfs.h>
+#include <linux/module.h>
+#include <linux/init.h>

drivers/base/soc.c line 15

Quoted 5 lines

drivers/base/soc.c new file +5 -0

+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+
+static DEFINE_SPINLOCK(register_lock);

If this is really needed, please make it a mutex as you end up calling code paths that can sleep.

drivers/base/soc.c line 16

Quoted 1 line

drivers/base/soc.c new file +1 -0

+static int soc_count = 0;

This should not be needed.

drivers/base/soc.c line 20

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+
+static struct device soc_grandparent = {
+	.init_name = "soc",
+};

NEVER create a static struct device, this is totally not needed and completly wrong.

Quoted 6 lines

drivers/base/soc.c new file +6 -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);
+

drivers/base/soc.c line 37

Quoted 10 lines

drivers/base/soc.c new file +10 -0

+	if (!strcmp(attr->attr.name, "machine"))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strcmp(attr->attr.name, "family"))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strcmp(attr->attr.name, "revision"))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+	if (!strcmp(attr->attr.name, "soc_id")) {
+		if (soc_dev->pfn_soc_id)
+			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
+		else return sprintf(buf, "N/A \n");

Move this line

drivers/base/soc.c line 40

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	}
+
+	return -EINVAL;

To here?

Quoted 10 lines

drivers/base/soc.c new file +10 -0

+}
+
+struct device_attribute soc_attrs[] = {
+	__ATTR(machine,  S_IRUGO, soc_info_get,  NULL),
+	__ATTR(family,   S_IRUGO, soc_info_get,  NULL),
+	__ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL),
+	__ATTR(revision, S_IRUGO, soc_info_get,  NULL),
+	__ATTR_NULL,
+};
+

drivers/base/soc.c line 55

Quoted 5 lines

drivers/base/soc.c new file +5 -0

+static void soc_device_remove_files(struct device *soc, int i)
+{
+	while (i > 0)
+		device_remove_file(soc, &soc_attrs[--i]);
+}

You do know how to add multiple files, right? Oh wait, this is all not the way to do this in the first place. You just raced userspace :(

Quoted 14 lines

drivers/base/soc.c new file +14 -0

+
+static int __init soc_device_create_files(struct device *dev)
+{
+	int ret = 0;
+	int i   = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(dev, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:

drivers/base/soc.c line 72

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	soc_device_remove_files(dev, --i);
+	return ret;
+}

Yup, you raced userspace. Please use the proper api for defining a group of attributes for a device by default so that the driver core handles creating and destroying them correctly and in a way that will not be racy (unlike this way.)

drivers/base/soc.c line 73

Quoted 2 lines

drivers/base/soc.c new file +2 -0

+
+int __init soc_device_register(struct device *dev)

Don't pass a struct device in here, why are you making the caller create the device? This is the function that should create it for you?

drivers/base/soc.c line 79

Quoted 5 lines

drivers/base/soc.c new file +5 -0

+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+	int ret;
+
+	spin_lock_irq(&register_lock);

Ok.

drivers/base/soc.c line 83

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+
+	if (!soc_count) {
+		/* Register top-level SoC device '/sys/devices/soc' */
+		ret = device_register(&soc_grandparent);

device_register can't be called with a spinlock. You must have gotten lucky in your testing, if you tested this.

Anyway, this is not needed at all, please don't create "dummy" devices in the sysfs tree, properly hook up your new device to where it should be in the device tree.

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+		if (ret) {
+			spin_unlock_irq(&register_lock);
+			return ret;
+		}

drivers/base/soc.c line 90

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	}
+
+	soc_count++;

Nope, don't use this, again, use the correct kernel interface for providing you with a unique number that increments as needed. Don't roll your own that will end up being wrong in the end (like this is, what happens if you remove a device in the middle?)

drivers/base/soc.c line 91

Quoted 1 line

drivers/base/soc.c new file +1 -0

+	dev->parent = &soc_grandparent;

Nope, don't do that.

drivers/base/soc.c line 100

Quoted 9 lines

drivers/base/soc.c new file +9 -0

+	dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine);
+
+	spin_unlock_irq(&register_lock);
+
+	ret = device_register(dev);
+	if (ret)
+		return ret;
+
+	soc_device_create_files(dev);

Nor that.

See above for why.

Quoted 23 lines

drivers/base/soc.c new file +3 -0

+
+	return ret;
+}

include/linux/sys_soc.h new file +14 -0

new file mode 100644
index 0000000..811d7fe
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct soc_device {
+	struct device dev;

include/linux/sys_soc.h line 19

Quoted 5 lines

include/linux/sys_soc.h new file +5 -0

+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *(*pfn_soc_id)(void);
+};

Nice structure, but why do you export it, you aren't using it anywhere, nor should you be...

include/linux/sys_soc.h line 25

Quoted 6 lines

include/linux/sys_soc.h new file +6 -0

+
+/**
+ * soc_device_register - register SoC as a device
+ * @dev: Parent node '/sys/devices/soc/X'
+ */
+int soc_device_register(struct device *dev);

This whole api needs to be rethunk, please, it's really wrong.

What you want is a way to create a soc device, by calling a function, not be responsible for creating it, then call the soc core, and then somehow, removing it.

Oh wait, you forgot to have a function to remove anything created with the above call, that seems really broken and wrong.

Quoted 20 lines · Greg KH

Quoted 6 lines · Lee Jones

drivers/base/soc.c new file +6 -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);
+

drivers/base/soc.c line 37

Quoted 10 lines

drivers/base/soc.c new file +10 -0

+	if (!strcmp(attr->attr.name, "machine"))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strcmp(attr->attr.name, "family"))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strcmp(attr->attr.name, "revision"))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+	if (!strcmp(attr->attr.name, "soc_id")) {
+		if (soc_dev->pfn_soc_id)
+			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
+		else return sprintf(buf, "N/A \n");

Move this line

Quoted 6 lines

drivers/base/soc.c line 40

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	}
+
+	return -EINVAL;

To here?

I initially had invalid requests return a useful message, but I was told to remove it and return -EINVAL instead:

"Just return -EINVAL or similar here to propogate the error to the user."

Jamie, what say you?

Quoted 28 lines · Lee Jones

Quoted 20 lines · Greg KH

Quoted 6 lines · Lee Jones

drivers/base/soc.c new file +6 -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);
+

drivers/base/soc.c line 37

Quoted 10 lines

drivers/base/soc.c new file +10 -0

+	if (!strcmp(attr->attr.name, "machine"))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strcmp(attr->attr.name, "family"))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strcmp(attr->attr.name, "revision"))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+	if (!strcmp(attr->attr.name, "soc_id")) {
+		if (soc_dev->pfn_soc_id)
+			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
+		else return sprintf(buf, "N/A \n");

Move this line

Quoted 6 lines

drivers/base/soc.c line 40

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	}
+
+	return -EINVAL;

To here?

Quoted 6 lines

I initially had invalid requests return a useful message, but I was told to remove it and return -EINVAL instead:

"Just return -EINVAL or similar here to propogate the error to the user."

Jamie, what say you?

Well if there isn't an ID for the SoC, then I think it's better for a read() to return an error rather than have to parse for a special string like "N/A ". So rather than returning that string, perhaps something like:

	if (!strcmp(attr->attr.name, "soc_id")) {
		if (!soc_dev->pfn_soc_id)
			return -ENOENT;
		return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
	}

or if you don't think this is an error, default to an id of "0" so that it can be parsed by tools?

Quoted 47 lines · Jamie Iles

Quoted 28 lines · Lee Jones

Quoted 20 lines · Greg KH

Quoted 6 lines · Lee Jones

drivers/base/soc.c new file +6 -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);
+

drivers/base/soc.c line 37

Quoted 10 lines

drivers/base/soc.c new file +10 -0

+	if (!strcmp(attr->attr.name, "machine"))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strcmp(attr->attr.name, "family"))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strcmp(attr->attr.name, "revision"))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+	if (!strcmp(attr->attr.name, "soc_id")) {
+		if (soc_dev->pfn_soc_id)
+			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
+		else return sprintf(buf, "N/A \n");

Move this line

Quoted 6 lines

drivers/base/soc.c line 40

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	}
+
+	return -EINVAL;

To here?

Quoted 6 lines

I initially had invalid requests return a useful message, but I was told to remove it and return -EINVAL instead:

"Just return -EINVAL or similar here to propogate the error to the user."

Jamie, what say you?

Well if there isn't an ID for the SoC, then I think it's better for a read() to return an error rather than have to parse for a special string like "N/A ". So rather than returning that string, perhaps something like:

	if (!strcmp(attr->attr.name, "soc_id")) {
		if (!soc_dev->pfn_soc_id)
			return -ENOENT;
		return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
	}

Quoted 2 lines

or if you don't think this is an error, default to an id of "0" so that it can be parsed by tools?

What about at the very end of the function?

What should we return if an inappropriate attr->attr.name is used?

Comments and questions in-line.

Quoted 5 lines · Lee Jones

Quoted 1 line

+static DEFINE_SPINLOCK(register_lock);

If this is really needed, please make it a mutex as you end up calling code paths that can sleep.

If I use:

"the correct kernel interface for providing you with a unique number that increments as needed"

I'm I correct in assuming that I don't need to use locking?

Quoted 11 lines

Quoted 1 line

+static int soc_count = 0;

This should not be needed.

drivers/base/soc.c line 20

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+
+static struct device soc_grandparent = {
+	.init_name = "soc",
+};

NEVER create a static struct device, this is totally not needed and completly wrong.

Noted on the static point, but I believe that it is needed.

That's how /sys/devices/platform does it, which this essentially replaces.

Quoted 19 lines

Quoted 6 lines

drivers/base/soc.c new file +6 -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);
+

drivers/base/soc.c line 37

Quoted 10 lines

drivers/base/soc.c new file +10 -0

+	if (!strcmp(attr->attr.name, "machine"))
+		return sprintf(buf, "%s\n", soc_dev->machine);
+	if (!strcmp(attr->attr.name, "family"))
+		return sprintf(buf, "%s\n", soc_dev->family);
+	if (!strcmp(attr->attr.name, "revision"))
+		return sprintf(buf, "%s\n", soc_dev->revision);
+	if (!strcmp(attr->attr.name, "soc_id")) {
+		if (soc_dev->pfn_soc_id)
+			return sprintf(buf, "%s\n", soc_dev->pfn_soc_id());
+		else return sprintf(buf, "N/A \n");

Move this line

Quoted 6 lines

drivers/base/soc.c line 40

Quoted 3 lines

drivers/base/soc.c new file +3 -0

+	}
+
+	return -EINVAL;

To here?

I initially had invalid requests return a useful message, but I was told to remove it and return -EINVAL instead:

"Just return -EINVAL or similar here to propogate the error to the user."

Quoted 4 lines

Quoted 1 line

+int __init soc_device_register(struct device *dev)

Don't pass a struct device in here, why are you making the caller create the device? This is the function that should create it for you?

I guess I can just return it instead, so it will become:

struct device * __init soc_device_register(void)

Is that more in keeping with what you would expect to see?

Quoted 16 lines

drivers/base/soc.c line 79

Quoted 5 lines

drivers/base/soc.c new file +5 -0

+{
+	struct soc_device *soc_dev = container_of(dev, struct soc_device, dev);
+	int ret;
+
+	spin_lock_irq(&register_lock);

Ok.

drivers/base/soc.c line 83

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+
+	if (!soc_count) {
+		/* Register top-level SoC device '/sys/devices/soc' */
+		ret = device_register(&soc_grandparent);

device_register can't be called with a spinlock. You must have gotten lucky in your testing, if you tested this.

Quoted 3 lines

Anyway, this is not needed at all, please don't create "dummy" devices in the sysfs tree, properly hook up your new device to where it should be in the device tree.

We need a /sys/devices/soc, how else can this be done?

Would it make you happier if I called it a bus?

no.

How did you pass the attribute data into this core so that it knows how to get to it? You didn't, which is a mess.

How about something like:

        struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr);

That would have the proper way to handle lifetime rules, userspace notificatation of userpace, placement in sysfs, and all the other good stuff.

Quoted 8 lines

Quoted 8 lines

drivers/base/soc.c line 83

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+
+  if (!soc_count) {
+          /* Register top-level SoC device '/sys/devices/soc' */
+          ret = device_register(&soc_grandparent);

device_register can't be called with a spinlock. You must have gotten lucky in your testing, if you tested this.

Quoted 5 lines

Quoted 3 lines

Anyway, this is not needed at all, please don't create "dummy" devices in the sysfs tree, properly hook up your new device to where it should be in the device tree.

We need a /sys/devices/soc, how else can this be done?

No you do not not, why would you think so?

Quoted 1 line

Would it make you happier if I called it a bus?

Yes, see the other response about creating a bus for these, which would give you /sys/bus/soc/ where you can properly enumerate your devices, which is what I think you want to be able to do, right?

Quoted 8 lines · Greg KH

no.

How did you pass the attribute data into this core so that it knows how to get to it? You didn't, which is a mess.

How about something like:

        struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr);

Quoted 3 lines

That would have the proper way to handle lifetime rules, userspace notificatation of userpace, placement in sysfs, and all the other good stuff.

I early on objected to the individual soc driver providing a set of attributes, because we really want the attributes to follow a common interface and I want to make it *hard* to add arbitrary other attributes that are not documented.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/base/soc.c line 83

Quoted 4 lines

drivers/base/soc.c new file +4 -0

+
+  if (!soc_count) {
+          /* Register top-level SoC device '/sys/devices/soc' */
+          ret = device_register(&soc_grandparent);

device_register can't be called with a spinlock. You must have gotten lucky in your testing, if you tested this.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Anyway, this is not needed at all, please don't create "dummy" devices in the sysfs tree, properly hook up your new device to where it should be in the device tree.

We need a /sys/devices/soc, how else can this be done?

No you do not not, why would you think so?

Well, where is all comes from is the desire to have a way from user space to find these devices, based on some path in the device tree. We had discussed putting it into

/sys/class/soc
/sys/bus/soc
/sys/devices/platform/soc
/sys/devices/platform/soc%d

and a few others and eventually settled on /sys/devices/soc.

Quoted 5 lines

Quoted 1 line

Would it make you happier if I called it a bus?

Yes, see the other response about creating a bus for these, which would give you /sys/bus/soc/ where you can properly enumerate your devices, which is what I think you want to be able to do, right?

I think that would be a reasonable interface, but how does this work when the device was created by of_platform_bus_probe? Should we assume that any top-level device in the flattened device tree is a soc? How about just adding a child device in each soc node that is registered to the soc_bus_type?

That would end up looking like

/sys/devices/db8500

		   /soc0   # this is the soc device
		   /i2c-0  # and here are the other platform devices below db8500 
		   /spi-0
    /bus/soc/devices/soc0 -> ../../../devices/db8500/soc0
    /bus/platform/devices/i2c-0 -> ../../../devices/db8500/i2c-0

I'm feeling like it would be simpler for me to write this code than to keep providing review comments like this telling you to rewrite the whole thing over again each time :)

Anyone want to send me a device that this code is needed for so I can do it?

I don't have a great deal of time at the moment, as I'm currently on the train on my way to Plumbers, but I would like to say just one thing.

Quoted 3 lines · Greg KH

I'm feeling like it would be simpler for me to write this code than to keep providing review comments like this telling you to rewrite the whole thing over again each time

I'd really rather you didn't.

I have put a great deal of my own time into this and I really want to see it through. Up until now I have been finding it quite difficult to please everyone. I have been trying to follow everyone's suggestions and as soon as I think we're getting close, the world flips upside down. When I return, I will sit down and go through all of your (and Arnd's) suggestions and re-write it.

I don't have a great deal of time at the moment, as I'm currently on the train on my way to Plumbers, but I would like to say just one thing.

Quoted 3 lines · Greg KH

I'm feeling like it would be simpler for me to write this code than to keep providing review comments like this telling you to rewrite the whole thing over again each time :)

I'd really rather you didn't.

I have put a great deal of my own time into this and I really want to see it through. Up until now I have been finding it quite difficult to please everyone. I have been trying to follow everyone's suggestions and as soon as I think we're getting close, the world flips upside down. When I return, I will sit down and go through all of your (and Arnd's) suggestions and re-write it.

Quoted 10 lines · Lee Jones

I don't have a great deal of time at the moment, as I'm currently on the train on my way to Plumbers, but I would like to say just one thing.

Quoted 3 lines · Greg KH

I'm feeling like it would be simpler for me to write this code than to keep providing review comments like this telling you to rewrite the whole thing over again each time :)

I'd really rather you didn't.

Quoted 6 lines

I have put a great deal of my own time into this and I really want to see it through. Up until now I have been finding it quite difficult to please everyone. I have been trying to follow everyone's suggestions and as soon as I think we're getting close, the world flips upside down. When I return, I will sit down and go through all of your (and Arnd's) suggestions and re-write it.

Ok, but expect a very tough code review if you do this. :)

Review

Quoted 2 lines · Lee Jones

(quoted diff) +2 -0

+
+int __init soc_device_register(struct device *dev)

Don't pass a struct device in here, why are you making the caller create the device? This is the function that should create it for you?

Quoted 20 lines

include/linux/sys_soc.h new file +14 -0

new file mode 100644
index 0000000..811d7fe
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct soc_device {
+	struct device dev;

include/linux/sys_soc.h line 19

Quoted 5 lines

include/linux/sys_soc.h new file +5 -0

+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *(*pfn_soc_id)(void);
+};

Nice structure, but why do you export it, you aren't using it anywhere, nor should you be...

include/linux/sys_soc.h line 25

Quoted 6 lines

include/linux/sys_soc.h new file +6 -0

+
+/**
+ * soc_device_register - register SoC as a device
+ * @dev: Parent node '/sys/devices/soc/X'
+ */
+int soc_device_register(struct device *dev);

This whole api needs to be rethunk, please, it's really wrong.

What you want is a way to create a soc device, by calling a function, not be responsible for creating it, then call the soc core, and then somehow, removing it.

Oh wait, you forgot to have a function to remove anything created with the above call, that seems really broken and wrong.

Quoted 6 lines · Greg KH

Review

Quoted 2 lines · Lee Jones

(quoted diff) +2 -0

+
+int __init soc_device_register(struct device *dev)

Don't pass a struct device in here, why are you making the caller create the device? This is the function that should create it for you?

Well, when we have device tree based probing, the soc device might already have been created by of_bus_probe(), so we just want to pass it to the soc core code in order to create the common interfaces.

It's probably not right that soc_device_register actually calls device_register, because when you have a device that you pass in, it will already be registered.

Quoted 20 lines

Quoted 20 lines

include/linux/sys_soc.h new file +14 -0

new file mode 100644
index 0000000..811d7fe
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct soc_device {
+	struct device dev;

Quoted 8 lines

include/linux/sys_soc.h line 19

Quoted 5 lines

include/linux/sys_soc.h new file +5 -0

+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *(*pfn_soc_id)(void);
+};

Nice structure, but why do you export it, you aren't using it anywhere, nor should you be...

This gets filled by the soc-specific code with information that is exported through the common callbacks, so it needs to be visible to the platforms.

Quoted 9 lines

include/linux/sys_soc.h line 25

Quoted 6 lines

include/linux/sys_soc.h new file +6 -0

+
+/**
+ * soc_device_register - register SoC as a device
+ * @dev: Parent node '/sys/devices/soc/X'
+ */
+int soc_device_register(struct device *dev);

This whole api needs to be rethunk, please, it's really wrong.

Quoted 3 lines

What you want is a way to create a soc device, by calling a function, not be responsible for creating it, then call the soc core, and then somehow, removing it.

Yes, it's definitely still not where it ought to be, and we've been discussing this for ages. It all started out as a way to find out information about the system, and I suggested to better make that information part of the interface for the soc node that is required anyway, rather than making up a purely fictional device.

Now, we need this to cover two scenarios:

1. The classic setup without device tree where the soc node is created by platform code. This would work great by just calling a function that creates the device and lets the soc code add the children according to compile-time tables of child devices.

2. Probing from OF device-tree, where the linux device tree is populated by walking the devices found in the flat device tree description. This doesn't really fit your suggestion, nor does it fit Lee's patch really, because it would require reparenting the soc node into /sys/devices/soc and registering it again after it's already registered.

I think the ideal solution would be to make the soc layer a class that can be set when initially adding the device (one way or another) and that adds the standard attributes. Since you don't really like classes any more, another option would be to just have a function call that adds an attribute group to the soc node, but then it's not easy to find which devices are actually soc root nodes.

Quoted 2 lines

Oh wait, you forgot to have a function to remove anything created with the above call, that seems really broken and wrong.

There is no way to remove a soc from the system, just like there is no complementary function to pcibios_scan_root that removes the PCI root bridge.

Quoted 30 lines · Arnd Bergmann

Quoted 28 lines · Greg KH

Quoted 15 lines

include/linux/sys_soc.h new file +9 -0

new file mode 100644
index 0000000..811d7fe
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2011
+ * Author: Lee Jones <[email protected]> for ST-Ericsson.
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#ifndef __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>

include/linux/sys_soc.h line 19

Quoted 10 lines

include/linux/sys_soc.h new file +10 -0

+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+struct soc_device {
+	struct device dev;
+	const char *machine;
+	const char *family;
+	const char *revision;
+	const char *(*pfn_soc_id)(void);
+};

Nice structure, but why do you export it, you aren't using it anywhere, nor should you be...

Quoted 2 lines

This gets filled by the soc-specific code with information that is exported through the common callbacks, so it needs to be visible to the platforms.

But this api doesn't allow that to happen, so use it to create the data in the first place somehow, this is really confusing as-is.

Quoted 7 lines

I think the ideal solution would be to make the soc layer a class that can be set when initially adding the device (one way or another) and that adds the standard attributes. Since you don't really like classes any more, another option would be to just have a function call that adds an attribute group to the soc node, but then it's not easy to find which devices are actually soc root nodes.

Then make it a bus, like it should be, which would solve all of this, right?

Quoted 6 lines

Quoted 2 lines

Oh wait, you forgot to have a function to remove anything created with the above call, that seems really broken and wrong.

There is no way to remove a soc from the system, just like there is no complementary function to pcibios_scan_root that removes the PCI root bridge.

And no one is ever going to remove the module that created the SOC device in the first place? It just really bothers me to see creation code but no removal code, as it feels wrong somehow :)