[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |   10 +++++
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   45 +++++++++++++++++++++
 4 files changed, 154 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 d57e8d0..2feab2b 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -168,4 +168,14 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config ARCH_NO_SYSDEV_OPS
+	bool
+	---help---
+	  To be selected by architectures that don't use sysdev class or
+	  sysdev driver power management (suspend/resume) and shutdown
+	  operations.
+
+config SYS_SOC
+	bool
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..a0d246d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_SYS_SOC) += 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..30659f4
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,98 @@
+/*
+ * 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>
+
+struct device_attribute soc_attrs[] = {
+	__ATTR(machine,  S_IRUGO, NULL, NULL),
+	__ATTR(family,   S_IRUGO, NULL, NULL),
+	__ATTR(soc_id,   S_IRUGO, NULL, NULL),
+	__ATTR(revision, S_IRUGO, NULL, NULL),
+	__ATTR_NULL,
+};
+
+const char *soc_names[] = { "1", "2", "3", "4", "5", "6", "7", "8" };
+static int soc_count = 0;
+
+static struct device soc_grandparent = {
+	.init_name = "soc",
+};
+
+static int soc_device_create_files(struct device *soc);
+static void soc_device_remove_files(struct device *soc);
+
+static int __init soc_device_create_files(struct device *soc)
+{
+	int ret = 0;
+	int i   = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(soc, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:
+	soc_device_remove_files(soc);
+	return ret;
+}
+
+static void __exit soc_device_remove_files(struct device *soc)
+{
+	int i = 0;
+
+	while (soc_attrs[i++].attr.name != NULL)
+		device_remove_file(soc, &soc_attrs[i]);
+}
+
+int __init soc_device_register(struct device *soc_parent,
+			struct soc_callback_functions *soc_callbacks)
+{
+	int ret;
+
+	soc_attrs[MACHINE].show = soc_callbacks->get_machine_fn;
+	soc_attrs[FAMILY].show = soc_callbacks->get_family_fn;
+	soc_attrs[SOCID].show = soc_callbacks->get_soc_id_fn;
+	soc_attrs[REVISION].show = soc_callbacks->get_revision_fn;
+
+	if (!soc_count) {
+		/* Register top-level SoC device '/sys/devices/soc' */
+		ret = device_register(&soc_grandparent);
+		if (ret)
+			return ret;
+	}
+
+	soc_parent->init_name = soc_names[soc_count];
+	soc_parent->parent = &soc_grandparent;
+
+	ret = device_register(soc_parent);
+	if (ret)
+		return ret;
+
+	soc_device_create_files(soc_parent);
+
+	soc_count++;
+
+	return ret;
+}
+
+void __exit soc_device_unregister(struct device *soc_parent)
+{
+	/* Unregister and free SoC from sysfs */
+	soc_device_remove_files(soc_parent);
+	device_unregister(soc_parent);
+
+	if (--soc_count == 0)
+		/* Unregister top-level SoC device '/sys/devices/soc' */
+	device_unregister(&soc_grandparent);
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..94d5707
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,45 @@
+/*
+ * 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 __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#define MAX_SOCS 8
+#define MACHINE  0
+#define FAMILY   1
+#define SOCID    2
+#define REVISION 3
+
+struct soc_callback_functions {
+	ssize_t (*get_machine_fn)(struct device *dev,
+				struct device_attribute *attr, char *buf);
+	ssize_t (*get_family_fn)(struct device *dev,
+				struct device_attribute *attr, char *buf);
+	ssize_t (*get_soc_id_fn)(struct device *dev,
+				struct device_attribute *attr, char *buf);
+	ssize_t (*get_revision_fn)(struct device *dev,
+				struct device_attribute *attr, char *buf);
+};
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_attr: Array of sysfs file attributes
+ * @soc: Parent node '/sys/devices/soc'
+ */
+int soc_device_register(struct device *soc_parent,
+			struct soc_callback_functions *soc_callbacks);
+
+/**
+ *  soc_device_unregister - unregister SoC as a device
+ * @soc_attr: Array of sysfs file attributes
+ * @soc: Parent node '/sys/devices/soc'
+ */
+void soc_device_unregister(struct device *soc_parent);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.4.1

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-ux500/Kconfig |    1 +
 arch/arm/mach-ux500/id.c    |  115 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index 4210cb4..4d2f2c2 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -26,6 +26,7 @@ config MACH_U8500
 	bool "U8500 Development platform"
 	depends on UX500_SOC_DB8500
 	select TPS6105X
+	select SYS_SOC
 	help
 	  Include support for the mop500 development platform.
 
diff --git a/arch/arm/mach-ux500/id.c b/arch/arm/mach-ux500/id.c
index d35122e..5156438 100644
--- a/arch/arm/mach-ux500/id.c
+++ b/arch/arm/mach-ux500/id.c
@@ -2,12 +2,16 @@
  * 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
  */
 
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/sys_soc.h>
+#include <linux/slab.h>
+#include <linux/stat.h>
 
 #include <asm/cputype.h>
 #include <asm/tlbflush.h>
@@ -105,3 +109,114 @@ void __init ux500_map_io(void)
 
 	ux500_print_soc_info(asicid);
 }
+
+#ifdef CONFIG_SYS_SOC
+#define U8500_BB_UID_BASE (U8500_BACKUPRAM1_BASE + 0xFC0)
+#define U8500_BB_UID_LENGTH 5
+
+static struct device soc_parent;
+
+static ssize_t ux500_get_family(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "Ux500\n");
+}
+
+static ssize_t ux500_get_machine(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "DB%4x\n", dbx500_partnumber());
+}
+
+static ssize_t ux500_get_soc_id(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	void __iomem *uid_base;
+	int i;
+	ssize_t sz = 0;
+
+	if (dbx500_partnumber() == 0x8500) {
+		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)));
+		sz += sprintf(buf + sz, "\n");
+	} else {
+		/* Don't know where it is located for U5500 */
+		sz = sprintf(buf, "N/A\n");
+	}
+
+	return sz;
+}
+
+static ssize_t ux500_get_revision(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	unsigned int rev = dbx500_revision();
+
+	if (rev == 0x01)
+		return sprintf(buf, "%s\n", "ED");
+	else if (rev >= 0xA0)
+		return sprintf(buf, "%d.%d\n" ,
+			(rev >> 4) - 0xA + 1, rev & 0xf);
+
+	return sprintf(buf, "%s", "Unknown\n");
+}
+
+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);
+}
+
+struct soc_callback_functions soc_callbacks = {
+	.get_machine_fn  = ux500_get_machine,
+	.get_family_fn   = ux500_get_family,
+	.get_soc_id_fn   = ux500_get_soc_id,
+	.get_revision_fn = ux500_get_revision,
+};
+
+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;
+	ret = soc_device_register(&soc_parent,
+				  &soc_callbacks);
+	if (ret >= 0) {
+		while (ux500_soc_attrs[i].attr.name != NULL) {
+			ret = device_create_file(&soc_parent,
+						&ux500_soc_attrs[i++]);
+			if (ret)
+				goto out;
+		}
+	}
+ out:
+	return ret;
+}
+module_init(ux500_soc_sysfs_init);
+
+static void __exit ux500_soc_sysfs_exit(void)
+{
+	int i = 0;
+
+	while (ux500_soc_attrs[i].attr.name != NULL)
+		device_remove_file(&soc_parent, &ux500_soc_attrs[i++]);
+
+	soc_device_unregister(&soc_parent);
+}
+module_exit(ux500_soc_sysfs_exit);
+
+#endif
-- 
1.7.4.1

[PATCH 3/3] Add documenation for new sysfs devices/soc functionallity

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/ABI/testing/sysfs-devices-soc |   16 ++++++++++++++++
 1 files changed, 16 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..fcf7f07
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-soc
@@ -0,0 +1,16 @@
+What:		/sys/devices/soc
+Date:		April 2011
+contact:	Lee Jones <lee.jones@linaro.org>
+Description:
+		The /sys/devices/soc 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
+		functionallity is only available if implemented by the platform.
+		This directory contains two kinds of attributes:
+			- Common attributes:
+				* machine: the name of the machine (e.g. Ux500).
+				* family: the family name of the SoC (e.g. DB8500).
+			- SoC-specific attributes:
+			  	* SoC vendor declared, such as serial and rev numbers.
+Users:
+		Any platform dependent user-space applications or profiling tools.
-- 
1.7.4.1

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Baruch Siach]

Hi Lee,

On Tue, Jul 12, 2011 at 02:08:08PM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/base/Kconfig    |   10 +++++
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   45 +++++++++++++++++++++
>  4 files changed, 154 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/sys_soc.h

I guess you should Cc Greg Kroah-Hartman <gregkh@suse.de> on this series.

baruch

[snip]

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Tue, Jul 12, 2011 at 05:13:39PM +0300, Baruch Siach wrote:
> Hi Lee,
> 
> On Tue, Jul 12, 2011 at 02:08:08PM +0100, Lee Jones wrote:
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/base/Kconfig    |   10 +++++
> >  drivers/base/Makefile   |    1 +
> >  drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sys_soc.h |   45 +++++++++++++++++++++
> >  4 files changed, 154 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/base/soc.c
> >  create mode 100644 include/linux/sys_soc.h
> 
> I guess you should Cc Greg Kroah-Hartman <gregkh@suse.de> on this series.

Only if the submitter wants it applied, obviously, they don't :)

Why do we have things like the MAINTAINERS file and
scripts/get_maintainer.pl if no one uses them...

greg k-h

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Saravana Kannan]

On 07/12/2011 06:08 AM, Lee Jones wrote:
> Signed-off-by: Lee Jones<lee.jones@linaro.org>
> ---
>   arch/arm/mach-ux500/Kconfig |    1 +
>   arch/arm/mach-ux500/id.c    |  115 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 116 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
> index 4210cb4..4d2f2c2 100644

<snip>

> +
> +struct soc_callback_functions soc_callbacks = {
> +	.get_machine_fn  = ux500_get_machine,
> +	.get_family_fn   = ux500_get_family,
> +	.get_soc_id_fn   = ux500_get_soc_id,
> +	.get_revision_fn = ux500_get_revision,
> +};
> +
> +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;
> +	ret = soc_device_register(&soc_parent,
> +				&soc_callbacks);
> +	if (ret>= 0) {
> +		while (ux500_soc_attrs[i].attr.name != NULL) {
> +			ret = device_create_file(&soc_parent,
> +						&ux500_soc_attrs[i++]);
> +			if (ret)
> +				goto out;
> +		}
> +	}

Can you please make this code as part of soc_device_register? Otherwise, 
every SoC that wants to add its own set of attributes will have to 
repeat this code.

> + out:
> +	return ret;
> +}
> +module_init(ux500_soc_sysfs_init);
> +
> +static void __exit ux500_soc_sysfs_exit(void)
> +{
> +	int i = 0;
> +
> +	while (ux500_soc_attrs[i].attr.name != NULL)
> +		device_remove_file(&soc_parent,&ux500_soc_attrs[i++]);

That would also pull this in and make sure the ordering is always correct.

> +
> +	soc_device_unregister(&soc_parent);
> +}
> +module_exit(ux500_soc_sysfs_exit);
> +
> +#endif

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Arnd Bergmann]

On Tuesday 12 July 2011, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Just like last time:

   NAK

You still have all the SoC devices as top-level platform devices.
Make them children of the SoC device you create here, and we can
apply this patch.

	Arnd

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

On 12/07/11 17:08, Greg KH wrote:
> On Tue, Jul 12, 2011 at 05:13:39PM +0300, Baruch Siach wrote:
>> Hi Lee,
>>
>> On Tue, Jul 12, 2011 at 02:08:08PM +0100, Lee Jones wrote:
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>  drivers/base/Kconfig    |   10 +++++
>>>  drivers/base/Makefile   |    1 +
>>>  drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/sys_soc.h |   45 +++++++++++++++++++++
>>>  4 files changed, 154 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/base/soc.c
>>>  create mode 100644 include/linux/sys_soc.h
>>
>> I guess you should Cc Greg Kroah-Hartman <gregkh@suse.de> on this series.

Yes, of course. Thanks Baruch.

> Only if the submitter wants it applied, obviously, they don't :)
> 
> Why do we have things like the MAINTAINERS file and
> scripts/get_maintainer.pl if no one uses them...

Doh! Sorry Greg. I'm playing around with my new send-patches.sh and I've
neglected to add functionality to apply bespoke To, CC and BCC fields on
sending. I'll make the amendments to the script for next time.

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Greg KH]

On Wed, Jul 13, 2011 at 08:16:05AM +0100, Lee Jones wrote:
> On 12/07/11 17:08, Greg KH wrote:
> > On Tue, Jul 12, 2011 at 05:13:39PM +0300, Baruch Siach wrote:
> >> Hi Lee,
> >>
> >> On Tue, Jul 12, 2011 at 02:08:08PM +0100, Lee Jones wrote:
> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>> ---
> >>>  drivers/base/Kconfig    |   10 +++++
> >>>  drivers/base/Makefile   |    1 +
> >>>  drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>  include/linux/sys_soc.h |   45 +++++++++++++++++++++
> >>>  4 files changed, 154 insertions(+), 0 deletions(-)
> >>>  create mode 100644 drivers/base/soc.c
> >>>  create mode 100644 include/linux/sys_soc.h
> >>
> >> I guess you should Cc Greg Kroah-Hartman <gregkh@suse.de> on this series.
> 
> Yes, of course. Thanks Baruch.
> 
> > Only if the submitter wants it applied, obviously, they don't :)
> > 
> > Why do we have things like the MAINTAINERS file and
> > scripts/get_maintainer.pl if no one uses them...
> 
> Doh! Sorry Greg. I'm playing around with my new send-patches.sh and I've
> neglected to add functionality to apply bespoke To, CC and BCC fields on
> sending. I'll make the amendments to the script for next time.

Do you really need another 'send-patches.sh' type script?  What's wrong
with the built in ones from git or quilt that requires a custom one?

greg k-h

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

On 12/07/11 17:29, Saravana Kannan wrote:
>> +struct soc_callback_functions soc_callbacks = {
>> +    .get_machine_fn  = ux500_get_machine,
>> +    .get_family_fn   = ux500_get_family,
>> +    .get_soc_id_fn   = ux500_get_soc_id,
>> +    .get_revision_fn = ux500_get_revision,
>> +};
>> +
>> +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;
>> +    ret = soc_device_register(&soc_parent,
>> +                &soc_callbacks);
>> +    if (ret>= 0) {
>> +        while (ux500_soc_attrs[i].attr.name != NULL) {
>> +            ret = device_create_file(&soc_parent,
>> +                        &ux500_soc_attrs[i++]);
>> +            if (ret)
>> +                goto out;
>> +        }
>> +    }
> 
> Can you please make this code as part of soc_device_register? Otherwise,
> every SoC that wants to add its own set of attributes will have to
> repeat this code.

That's the idea.

I initially had it all as part of soc_device_register, but Arnd told me
to remove it in this patch-set.

See here:

On 17/04/11 19:36, Arnd Bergmann wrote:
> For the nonstandard attributes, I would recommend having the individual
> drivers call device_create_file, in order to discourage the use of 
> device specific attribute names.

Hence all of these are the _standard_ attributes:

>> +    .get_machine_fn  = ux500_get_machine,
>> +    .get_family_fn   = ux500_get_family,
>> +    .get_soc_id_fn   = ux500_get_soc_id,
>> +    .get_revision_fn = ux500_get_revision,

And this is the _non standard_ one:
>> +    __ATTR(process,  S_IRUGO, ux500_get_process,  NULL),

Which is subsequently added by the platform.

Kind regards,
Lee

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

On 12/07/11 17:47, Arnd Bergmann wrote:
> On Tuesday 12 July 2011, Lee Jones wrote:
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Just like last time:
> 
>    NAK
> 
> You still have all the SoC devices as top-level platform devices.
> Make them children of the SoC device you create here, and we can
> apply this patch.

Okay, obviously I'm missing something. I am trying to adhere to your
advice, but there must be some communication break-down somewhere down
the line. Let me attempt to explain what's going on in my head.

You keep asking for the SoC devices to be children of the parent SoC
device and as far as I'm concerned they are. Devices appear like this in
sysfs:

/sys/devices/soc/1   /* 1st SoC */
/sys/devices/soc/2   /* 2nd SoC */
/sys/devices/soc/3   /* 3rd SoC */

etc ...

Surely the parent which you speak of is "soc" and each SoC is
represented by "1|2|3|..."? Under each directory with a digit naming
convention appears that SoC's attributes, namely: "family", "machine",
"process", "revision" and "soc_id".

If this is incorrect, would you be kind enough to tell me why its
incorrect and how you would like it changed/fixed please?

Kind regards,
Lee

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

On 13/07/11 08:53, Greg KH wrote:
> On Wed, Jul 13, 2011 at 08:16:05AM +0100, Lee Jones wrote:
>> On 12/07/11 17:08, Greg KH wrote:
>>> On Tue, Jul 12, 2011 at 05:13:39PM +0300, Baruch Siach wrote:
>>>> Hi Lee,
>>>>
>>>> On Tue, Jul 12, 2011 at 02:08:08PM +0100, Lee Jones wrote:
>>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>>> ---
>>>>>  drivers/base/Kconfig    |   10 +++++
>>>>>  drivers/base/Makefile   |    1 +
>>>>>  drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/linux/sys_soc.h |   45 +++++++++++++++++++++
>>>>>  4 files changed, 154 insertions(+), 0 deletions(-)
>>>>>  create mode 100644 drivers/base/soc.c
>>>>>  create mode 100644 include/linux/sys_soc.h
>>>>
>>>> I guess you should Cc Greg Kroah-Hartman <gregkh@suse.de> on this series.
>>
>> Yes, of course. Thanks Baruch.
>>
>>> Only if the submitter wants it applied, obviously, they don't :)
>>>
>>> Why do we have things like the MAINTAINERS file and
>>> scripts/get_maintainer.pl if no one uses them...
>>
>> Doh! Sorry Greg. I'm playing around with my new send-patches.sh and I've
>> neglected to add functionality to apply bespoke To, CC and BCC fields on
>> sending. I'll make the amendments to the script for next time.
> 
> Do you really need another 'send-patches.sh' type script?  What's wrong
> with the built in ones from git or quilt that requires a custom one?

Yes, I really am that lazy. :)

Firstly, I don't send enough patches upstream to enable me to learn then
retain the knowledge to quickly type out a `git send-email` command. If
I write a script just once and keep it centrally located, it will
inevitably save me time in the long-term. Also, when I draft patches I
may do so from a different email address depending on the hat I'm
wearing at any given moment. Supplying an account flag "--can" or
"--lin" is somewhat easier than typing out different credentials each
time. "--smtp-server", "--smtp-server-port", "--smtp-encryption",
"--smtp-user", "--smtp-pass" and "--from" all differ from account to
account.

With this script I dump the patch-set into a directory and issue
"send-patches.sh --lin --arm <patchdir>" and off goes the patch-set to
the Arm ML. However, as Baruch and yourself kindly noticed, I omitted
functionality to add the maintainer and interested party email address -
I'll fix that now.

Sorry for any inconvenience this may have caused.

Kind regards,
Lee

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Arnd Bergmann]

On Wednesday 13 July 2011, Lee Jones wrote:
> Okay, obviously I'm missing something. I am trying to adhere to your
> advice, but there must be some communication break-down somewhere down
> the line. Let me attempt to explain what's going on in my head.
> 
> You keep asking for the SoC devices to be children of the parent SoC
> device and as far as I'm concerned they are. Devices appear like this in
> sysfs:
> 
> /sys/devices/soc/1   /* 1st SoC */
> /sys/devices/soc/2   /* 2nd SoC */
> /sys/devices/soc/3   /* 3rd SoC */
> 
> etc ...
> 
> Surely the parent which you speak of is "soc" and each SoC is
> represented by "1|2|3|..."? Under each directory with a digit naming
> convention appears that SoC's attributes, namely: "family", "machine",
> "process", "revision" and "soc_id".
> 
> If this is incorrect, would you be kind enough to tell me why its
> incorrect and how you would like it changed/fixed please?
> 

I'm not talking of the root soc devices but the platform_devices that
are part of the soc, i.e. interrupt controller, i2c bus, mmc host,
etc.

Basically all the devices that are currently statically declared
in arch/arm/mach-ux500/board-mop500.c and are missing a parent pointer.

What I'm asking you to do is fix those device declarations to have
their .dev.parent pointer point to the correct soc device. If there
are nested buses, they should ideally also be represented as devices
so you end up with something like

/sys/devices/soc/1/amba/uart1
                       /uart2
                       /i2c/bus1
                           /bus2/tc35892
                                /lp5521
                       /spi/foo
                           /bar
                  /dma
                /2/amba/...

Basically, if you have a soc node, I would expect /sys/devices/platform to
become empty, and all devices properly parented to where they are actually
connected.

	Arnd

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

On 13/07/11 14:42, Arnd Bergmann wrote:
> On Wednesday 13 July 2011, Lee Jones wrote:
>> Okay, obviously I'm missing something. I am trying to adhere to your
>> advice, but there must be some communication break-down somewhere down
>> the line. Let me attempt to explain what's going on in my head.
>>
>> You keep asking for the SoC devices to be children of the parent SoC
>> device and as far as I'm concerned they are. Devices appear like this in
>> sysfs:
>>
>> /sys/devices/soc/1   /* 1st SoC */
>> /sys/devices/soc/2   /* 2nd SoC */
>> /sys/devices/soc/3   /* 3rd SoC */
>>
>> etc ...
>>
>> Surely the parent which you speak of is "soc" and each SoC is
>> represented by "1|2|3|..."? Under each directory with a digit naming
>> convention appears that SoC's attributes, namely: "family", "machine",
>> "process", "revision" and "soc_id".
>>
>> If this is incorrect, would you be kind enough to tell me why its
>> incorrect and how you would like it changed/fixed please?
>>
> 
> I'm not talking of the root soc devices but the platform_devices that
> are part of the soc, i.e. interrupt controller, i2c bus, mmc host,
> etc.
> 
> Basically all the devices that are currently statically declared
> in arch/arm/mach-ux500/board-mop500.c and are missing a parent pointer.
> 
> What I'm asking you to do is fix those device declarations to have
> their .dev.parent pointer point to the correct soc device. If there
> are nested buses, they should ideally also be represented as devices
> so you end up with something like
> 
> /sys/devices/soc/1/amba/uart1
>                        /uart2
>                        /i2c/bus1
>                            /bus2/tc35892
>                                 /lp5521
>                        /spi/foo
>                            /bar
>                   /dma
>                 /2/amba/...
> 
> Basically, if you have a soc node, I would expect /sys/devices/platform to
> become empty, and all devices properly parented to where they are actually
> connected.


Ahhhh, well why didn't you say so? I've been racking my brains about
this for ages.:)

Does this have to be coded and submitted with this patch-set? Is there
any way we can accept this one and I'll take this reorganisation of
platform drivers as an action to be completed when I have some more
spare cycles?

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Arnd Bergmann]

On Wednesday 13 July 2011, Lee Jones wrote:
> Does this have to be coded and submitted with this patch-set? Is there
> any way we can accept this one and I'll take this reorganisation of
> platform drivers as an action to be completed when I have some more
> spare cycles?

I prefer to remain strict on this one. I don't mind the first
patch getting in now, but I wouldn't want to set a precedent
on incorrect usage of the soc nodes. Note that doing this right
will become a lot easier once the platform has been converted
into device tree probing, because that implies a hierarchical
view of the devices by nature.

	Arnd

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Saravana Kannan]

On 07/13/2011 12:55 AM, Lee Jones wrote:
> On 12/07/11 17:29, Saravana Kannan wrote:
>> Can you please make this code as part of soc_device_register? Otherwise,
>> every SoC that wants to add its own set of attributes will have to
>> repeat this code.
>
> That's the idea.
>
> I initially had it all as part of soc_device_register, but Arnd told me
> to remove it in this patch-set.
>
> See here:
>
> On 17/04/11 19:36, Arnd Bergmann wrote:
>> For the nonstandard attributes, I would recommend having the individual
>> drivers call device_create_file, in order to discourage the use of
>> device specific attribute names.

Arnd,

I don't think forcing individual driver to call device_create_file() is 
really going to be much of a discouragement. If someone thinks they need 
to export some info, they are going to do it and end up with repeated 
code. The only real discouragement in my opinion would be code review.

Can we please pull the creation of extra attributes into the common code?

Thanks,
Saravana

>
> Hence all of these are the _standard_ attributes:
>
>>> +    .get_machine_fn  = ux500_get_machine,
>>> +    .get_family_fn   = ux500_get_family,
>>> +    .get_soc_id_fn   = ux500_get_soc_id,
>>> +    .get_revision_fn = ux500_get_revision,
>
> And this is the _non standard_ one:
>>> +    __ATTR(process,  S_IRUGO, ux500_get_process,  NULL),
>
> Which is subsequently added by the platform.
>
> Kind regards,
> Lee
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Greg KH]

On Wed, Jul 13, 2011 at 09:40:35AM -0700, Saravana Kannan wrote:
> On 07/13/2011 12:55 AM, Lee Jones wrote:
> >On 12/07/11 17:29, Saravana Kannan wrote:
> >>Can you please make this code as part of soc_device_register? Otherwise,
> >>every SoC that wants to add its own set of attributes will have to
> >>repeat this code.
> >
> >That's the idea.
> >
> >I initially had it all as part of soc_device_register, but Arnd told me
> >to remove it in this patch-set.
> >
> >See here:
> >
> >On 17/04/11 19:36, Arnd Bergmann wrote:
> >>For the nonstandard attributes, I would recommend having the individual
> >>drivers call device_create_file, in order to discourage the use of
> >>device specific attribute names.
> 
> Arnd,
> 
> I don't think forcing individual driver to call device_create_file()
> is really going to be much of a discouragement.

Yes it is as calling that is broken and wrong and userspace will race
with the kernel and bad things will happen on the wrong phase of the
moon.

But again, I never saw the original series to explain why...

greg k-h

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Arnd Bergmann]

On Wednesday 13 July 2011 22:32:26 Greg KH wrote:
> On Wed, Jul 13, 2011 at 09:40:35AM -0700, Saravana Kannan wrote:
> > On 07/13/2011 12:55 AM, Lee Jones wrote:
> > >On 12/07/11 17:29, Saravana Kannan wrote:
> > >I initially had it all as part of soc_device_register, but Arnd told me
> > >to remove it in this patch-set.
> > >
> > >See here:
> > >
> > >On 17/04/11 19:36, Arnd Bergmann wrote:
> > >>For the nonstandard attributes, I would recommend having the individual
> > >>drivers call device_create_file, in order to discourage the use of
> > >>device specific attribute names.
> > 
> > Arnd,
> > 
> > I don't think forcing individual driver to call device_create_file()
> > is really going to be much of a discouragement.
> 
> Yes it is as calling that is broken and wrong and userspace will race
> with the kernel and bad things will happen on the wrong phase of the
> moon.
> 
> But again, I never saw the original series to explain why...

This is about attributes to the main device that acts as a parent for all
devices on the SoC, so there is no user space at the time.

IIRC, the original patch was documenting a set of standard attributes and
letting the SoC specific driver pass a set of static attributes in the
hope that they do whatever is documented.

In one of my review comments, I asked this to be changed to pass a data
structure with the actual contents of those attributes and to make
the implementation common to the soc independent code, in order to
reduce the amount of code needed in each soc implementation, to enforce
consistent behavior, and to make it more obvious (and painful) when an
soc implementation adds a nonstandard attribute.

It seems the final outcome was to have a data structure of function
pointers to get the attribute contents, which is less nice but still
acceptable IMHO.

	Arnd

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

> It seems the final outcome was to have a data structure of function
> pointers to get the attribute contents, which is less nice but still
> acceptable IMHO.

I did think this was a little neater than passing strings all over the
place. I'm pleased you think this patch is now acceptable. Would you
mind re-enforcing your approval with a Signed-off-by please?

I will endeavor to look at the other patch and attempt to reorganise the
Platform Drivers in sysfs sometime soon after this one has been accepted.

Kind regards,
Lee

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Arnd Bergmann]

On Thursday 14 July 2011, Lee Jones wrote:
> > It seems the final outcome was to have a data structure of function
> > pointers to get the attribute contents, which is less nice but still
> > acceptable IMHO.
> 
> I did think this was a little neater than passing strings all over the
> place.

To give some background on why I think it is not:

The contents of the files are all static, so you can generate them
at compile time from an init function that is then discarded. When you
build a kernel for many different SoCs, you only need the pointers
at run-time, while your approach means that the kernel image will
have to keep all the functions for every SoC that is built in.

Also, in many cases, the strings themselves will be static and not
taken from a specific register.

> I'm pleased you think this patch is now acceptable. Would you
> mind re-enforcing your approval with a Signed-off-by please?

A Signed-off-by is not appropriate because I was not involved in sending
the patch but in reviewing it. I can give you a 'Reviewed-by', which
is the appropriate reply in this case.

> I will endeavor to look at the other patch and attempt to reorganise the
> Platform Drivers in sysfs sometime soon after this one has been accepted.

Ok.

	Arnd

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

On 14/07/11 13:58, Arnd Bergmann wrote:
> On Thursday 14 July 2011, Lee Jones wrote:
>>> It seems the final outcome was to have a data structure of function
>>> pointers to get the attribute contents, which is less nice but still
>>> acceptable IMHO.
>>
>> I did think this was a little neater than passing strings all over the
>> place.
> 
> To give some background on why I think it is not:
> 
> The contents of the files are all static, so you can generate them
> at compile time from an init function that is then discarded. When you
> build a kernel for many different SoCs, you only need the pointers
> at run-time, while your approach means that the kernel image will
> have to keep all the functions for every SoC that is built in.
> 
> Also, in many cases, the strings themselves will be static and not
> taken from a specific register.

I see your point.

>> I'm pleased you think this patch is now acceptable. Would you
>> mind re-enforcing your approval with a Signed-off-by please?
> 
> A Signed-off-by is not appropriate because I was not involved in sending
> the patch but in reviewing it. I can give you a 'Reviewed-by', which
> is the appropriate reply in this case.

That'll do, thanks.

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Saravana Kannan]

On 07/13/2011 01:51 PM, Arnd Bergmann wrote:
> On Wednesday 13 July 2011 22:32:26 Greg KH wrote:
>> On Wed, Jul 13, 2011 at 09:40:35AM -0700, Saravana Kannan wrote:
>>> On 07/13/2011 12:55 AM, Lee Jones wrote:
>>>> On 12/07/11 17:29, Saravana Kannan wrote:
>>>> I initially had it all as part of soc_device_register, but Arnd told me
>>>> to remove it in this patch-set.
>>>>
>>>> See here:
>>>>
>>>> On 17/04/11 19:36, Arnd Bergmann wrote:
>>>>> For the nonstandard attributes, I would recommend having the individual
>>>>> drivers call device_create_file, in order to discourage the use of
>>>>> device specific attribute names.
>>>
>>> Arnd,
>>>
>>> I don't think forcing individual driver to call device_create_file()
>>> is really going to be much of a discouragement.
>>
>> Yes it is as calling that is broken and wrong and userspace will race
>> with the kernel and bad things will happen on the wrong phase of the
>> moon.
>>
>> But again, I never saw the original series to explain why...

Greg,

Your wording was a bit confusing for me (sorry), but it appears that you 
are disagreeing with me. Assuming that to be the case, how will moving a 
couple of lines of code (calls to device_create_file()) from a caller 
function A into caller function B make any functional difference if the 
order of the calls are maintained? The calls to device_create_file() 
will still be done as part of the same call flow/execution flow. I'm 
just asking for code refactoring to avoid repeating the code multiple times.

Thanks,
Saravana

> This is about attributes to the main device that acts as a parent for all
> devices on the SoC, so there is no user space at the time.
>
> IIRC, the original patch was documenting a set of standard attributes and
> letting the SoC specific driver pass a set of static attributes in the
> hope that they do whatever is documented.
>
> In one of my review comments, I asked this to be changed to pass a data
> structure with the actual contents of those attributes and to make
> the implementation common to the soc independent code, in order to
> reduce the amount of code needed in each soc implementation, to enforce
> consistent behavior, and to make it more obvious (and painful) when an
> soc implementation adds a nonstandard attribute.
>
> It seems the final outcome was to have a data structure of function
> pointers to get the attribute contents, which is less nice but still
> acceptable IMHO.
>
> 	Arnd

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] mach-ux500: export System-on-Chip information via sysfs

[Lee Jones]

On 14/07/11 18:25, Saravana Kannan wrote:
> On 07/13/2011 01:51 PM, Arnd Bergmann wrote:
>> On Wednesday 13 July 2011 22:32:26 Greg KH wrote:
>>> On Wed, Jul 13, 2011 at 09:40:35AM -0700, Saravana Kannan wrote:
>>>> On 07/13/2011 12:55 AM, Lee Jones wrote:
>>>>> On 12/07/11 17:29, Saravana Kannan wrote:
>>>>> I initially had it all as part of soc_device_register, but Arnd
>>>>> told me
>>>>> to remove it in this patch-set.
>>>>>
>>>>> See here:
>>>>>
>>>>> On 17/04/11 19:36, Arnd Bergmann wrote:
>>>>>> For the nonstandard attributes, I would recommend having the
>>>>>> individual
>>>>>> drivers call device_create_file, in order to discourage the use of
>>>>>> device specific attribute names.
>>>>
>>>> Arnd,
>>>>
>>>> I don't think forcing individual driver to call device_create_file()
>>>> is really going to be much of a discouragement.
>>>
>>> Yes it is as calling that is broken and wrong and userspace will race
>>> with the kernel and bad things will happen on the wrong phase of the
>>> moon.
>>>
>>> But again, I never saw the original series to explain why...
> 
> Greg,
> 
> Your wording was a bit confusing for me (sorry), but it appears that you
> are disagreeing with me. Assuming that to be the case, how will moving a
> couple of lines of code (calls to device_create_file()) from a caller
> function A into caller function B make any functional difference if the
> order of the calls are maintained? The calls to device_create_file()
> will still be done as part of the same call flow/execution flow. I'm
> just asking for code refactoring to avoid repeating the code multiple
> times.

I believe it is Arnd that's disagreeing with you. For his full
description as to why he thought it was a bad idea to allow users to
_easily_ add nodes, see my first attempts at this patch.

I initially had this:

+struct device_attribute soc_one_attrs[] = {
+	__ATTR(machine,  S_IRUGO, ux500_get_machine,  NULL),
+	__ATTR(family,   S_IRUGO, ux500_get_family,   NULL),
+	__ATTR(soc_id,   S_IRUGO, ux500_get_soc_id,   NULL),
+	__ATTR(revision, S_IRUGO, ux500_get_revision, NULL),
+	__ATTR(process,  S_IRUGO, ux500_get_process,  NULL),
+	__ATTR_NULL,
+};
+
+struct device_attribute *each_soc_attrs[] = {
+	soc_one_attrs,
+	NULL,
+};
+
+static int __init ux500_soc_sysfs_init(void)
+{
+	return soc_device_register(&parent_soc, each_soc_attrs
+                                  ARRAY_SIZE(each_soc_attrs));
+}

Which I loved. It was neat and easy to read. However, Arnd felt that it
would be far too easy for SoC vendors/hackers to add nodes willy-nilly,
which he wanted to prevent. Hence the request for change.

Kind regards,
Lee

>> This is about attributes to the main device that acts as a parent for all
>> devices on the SoC, so there is no user space at the time.
>>
>> IIRC, the original patch was documenting a set of standard attributes and
>> letting the SoC specific driver pass a set of static attributes in the
>> hope that they do whatever is documented.
>>
>> In one of my review comments, I asked this to be changed to pass a data
>> structure with the actual contents of those attributes and to make
>> the implementation common to the soc independent code, in order to
>> reduce the amount of code needed in each soc implementation, to enforce
>> consistent behavior, and to make it more obvious (and painful) when an
>> soc implementation adds a nonstandard attribute.
>>
>> It seems the final outcome was to have a data structure of function
>> pointers to get the attribute contents, which is less nice but still
>> acceptable IMHO.
>>
>>     Arnd
> 


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

On Tuesday 12 July 2011, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/base/Kconfig    |   10 +++++
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |   98 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   45 +++++++++++++++++++++

After looking at the patch again, I do have some important comments
after all. I had only looked at the documentation you posted, not
at the actual patch.

> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d57e8d0..2feab2b 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,14 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config ARCH_NO_SYSDEV_OPS
> +	bool
> +	---help---
> +	  To be selected by architectures that don't use sysdev class or
> +	  sysdev driver power management (suspend/resume) and shutdown
> +	  operations.

You don't seem to be using this anywhere. What is this for?

> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..30659f4
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,98 @@
> +/*
> + * 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>
> +
> +struct device_attribute soc_attrs[] = {
> +	__ATTR(machine,  S_IRUGO, NULL, NULL),
> +	__ATTR(family,   S_IRUGO, NULL, NULL),
> +	__ATTR(soc_id,   S_IRUGO, NULL, NULL),
> +	__ATTR(revision, S_IRUGO, NULL, NULL),
> +	__ATTR_NULL,
> +};

This method does not work. You only set the function pointers
when you call soc_device_register, but they will end up overwriting
the existing function pointers when you have multiple callers of that
function.

It would be better to define a 'struct soc_device' derived from 'struct 
device' that holds a pointer to the actual strings (or operations, if
you insist) and provide a single soc_info_get() function that is used
for all the attributes.

> +const char *soc_names[] = { "1", "2", "3", "4", "5", "6", "7", "8" };
> +static int soc_count = 0;

This is a bit silly and artificially limits the number of SoC devices.
It's much simpler to just use dev_set_name().

> +static struct device soc_grandparent = {
> +	.init_name = "soc",
> +};
>
> +static int soc_device_create_files(struct device *soc);
> +static void soc_device_remove_files(struct device *soc);

No forward declarations for static functions please.

> +int __init soc_device_register(struct device *soc_parent,
> +			struct soc_callback_functions *soc_callbacks)
> +{
> +	int ret;
> +
> +	soc_attrs[MACHINE].show = soc_callbacks->get_machine_fn;
> +	soc_attrs[FAMILY].show = soc_callbacks->get_family_fn;
> +	soc_attrs[SOCID].show = soc_callbacks->get_soc_id_fn;
> +	soc_attrs[REVISION].show = soc_callbacks->get_revision_fn;

It's better to allocate the device here, so you don't have to
do it in each caller.

> +	soc_parent->init_name = soc_names[soc_count];
> +	soc_parent->parent = &soc_grandparent;
> +
> +	ret = device_register(soc_parent);
> +	if (ret)
> +		return ret;
> +
> +	soc_device_create_files(soc_parent);
> +
> +	soc_count++;

This needs some locking to ensure that you don't try to register
two devices with the same number.

> +
> +void __exit soc_device_unregister(struct device *soc_parent)
> +{
> +	/* Unregister and free SoC from sysfs */
> +	soc_device_remove_files(soc_parent);
> +	device_unregister(soc_parent);
> +
> +	if (--soc_count == 0)
> +		/* Unregister top-level SoC device '/sys/devices/soc' */
> +	device_unregister(&soc_grandparent);
> +}

The exit function does not make any sense if you cannot build the soc
support itself as a module, which in turn makes no sense at all.

> +
> +#define MAX_SOCS 8
> +#define MACHINE  0
> +#define FAMILY   1
> +#define SOCID    2
> +#define REVISION 3
> +

I think these defines are used nowhere by the individual drivers, so they
need not be in the header file. More importantly, the names are not put
in a proper namespace, so they can easily collide with other macros
or enums.

	Arnd

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Hi Russell,

>>> Ok, in that case, I'd suggest you use your ST-Ericsson address for
>>> Signed-off-by and the author statement above.
>>
>> I'm not an ST-Ericsson employee, thus do not have an associated address.
>>
>> I work for Linaro, currently on assignment to ST-Ericsson.
> 
> It was my understanding gained last week was that Linaro is a separate
> organization, and that while folk are working for Linaro, stuff they
> create belongs to Linaro and not the company whose office they happen
> to be sitting in.
> 
> I also thought Linaro was supposed to be an organization for solving
> the _common_ problems being experienced by each member organization,
> rather than being a contracting house to any one particular
> organization.  Am I mistaken?

You are not mistaken for the most part. However, you are referring to
the engineering work carried out by the engineers contained in the
Linaro Working Groups. The role I occupy within the company is slightly
different. I lead a group of individuals called a "Landing Team". There
are currently four of these within Linaro; ST-Ericsson (mine), Texas
Instruments, Samsung and Freescale, with a view on increasing that
number over the upcoming months. The Landing Team's main purpose is to
upstream as much code as possible for a previously specified SoC, in our
case the u8500. How the Landing Team spends their time is my
responsibility and as such (unless Linaro or ST-Ericsson management have
a special request) is depicted by me.

With regards to the Copyright label, I will endeavor to find a
definitive answer. We came to the conclusion of "Written by Linaro for
ST-Ericsson" some time ago in a Landing Team meeting we held some months
ago. If you or Arnd see this as an issue clearly we need to have more
meetings and involve more senior people from Linaro to chase a
conclusive answer.

Thanks for your time and input Russell and Arnd, it is appreciated.

Kind regards,
Lee Jones
Team Lead, Linaro ST-Ericsson Landing Team

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Russell King - ARM Linux]

On Thu, Apr 21, 2011 at 12:56:07PM +0100, Lee Jones wrote:
> Hi Arnd,
> 
> </snip>
> 
> >>> (I believe this would be Copyright Linaro Ltd, not ST-Ericsson SA,
> >>> but better ask internally in ST-Ericsson about what your rules are)
> >>
> >> We've had internal discussions about this. I believe this is the correct
> >> thing to do. The Copyright should stay with ST-Ericsson.
> > 
> > Ok, in that case, I'd suggest you use your ST-Ericsson address for
> > Signed-off-by and the author statement above.
> 
> I'm not an ST-Ericsson employee, thus do not have an associated address.
> 
> I work for Linaro, currently on assignment to ST-Ericsson.

It was my understanding gained last week was that Linaro is a separate
organization, and that while folk are working for Linaro, stuff they
create belongs to Linaro and not the company whose office they happen
to be sitting in.

I also thought Linaro was supposed to be an organization for solving
the _common_ problems being experienced by each member organization,
rather than being a contracting house to any one particular
organization.  Am I mistaken?

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Hi Arnd,

</snip>

>>> (I believe this would be Copyright Linaro Ltd, not ST-Ericsson SA,
>>> but better ask internally in ST-Ericsson about what your rules are)
>>
>> We've had internal discussions about this. I believe this is the correct
>> thing to do. The Copyright should stay with ST-Ericsson.
> 
> Ok, in that case, I'd suggest you use your ST-Ericsson address for
> Signed-off-by and the author statement above.

I'm not an ST-Ericsson employee, thus do not have an associated address.

I work for Linaro, currently on assignment to ST-Ericsson.

</snip>

> I would prefer to standardise the attributes as much as possible. Ideally,
> all SOCs should export the same set of attributes, and in no case should
> there be multiple SOCs that have the same attribute name but with a
> slightly different interface (e.g. one writable, or one root-only readable),
> or the same contents in attributes of different names.
> 
> The best way to ensure this is to give less flexiblity to the person
> implementing the individual SOC code. All attributes that are documented
> to be available across SOCs can simply be automatically created and
> filled with the data provided by the platform.
> 
> Having interfaces specific to one SOC should be the absolute exception,
> so I'd try to make that as hard as possible.

Well your word overrides mine.

I'll completely rewrite the driver again. It may be some time before
it's complete (post-UDS/LDS), as I have a lot on 'till then. I would
like to see this to the end though, so leave it with me.

Kind regards,
Lee

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

On Thursday 21 April 2011, Lee Jones wrote:

> >> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> >> new file mode 100644
> >> index 0000000..5e4d6ef
> >> --- /dev/null
> >> +++ b/drivers/base/soc.c
> >> @@ -0,0 +1,127 @@
> >> +/*
> >> + * 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
> >> + */
> > 
> > (I believe this would be Copyright Linaro Ltd, not ST-Ericsson SA,
> > but better ask internally in ST-Ericsson about what your rules are)
> 
> We've had internal discussions about this. I believe this is the correct
> thing to do. The Copyright should stay with ST-Ericsson.

Ok, in that case, I'd suggest you use your ST-Ericsson address for
Signed-off-by and the author statement above.

> >> +int __init soc_device_register(struct device_attribute *soc_attrs[],
> >> +                               int soc_count)
> > 
> > This needs to return the soc device, otherwise there is nothing that
> > a platform can do with the device.
> 
> What do you think the platform would want to do with the device?

Add the child devices, or add more attributes.

> > Passing the soc_count the way you do won't work when you have different
> > SoCs, 
> 
> Why won't the platform know how many SoCs are on a given platform?
>
> > so better require the user to call the register function repeatedly.
> 
> ... and if it truly doesn't know, how will it know how many times to
> call the register function?

You could have a platform that has several SOCs, some of which are optional,
E.g. one SoC for that contains the main CPU, and then another SOC of
a different vendor plugged into an external bus.

> > I think a nicer interface would be to pass a data structure into it
> > with the data you always want to export, and then have the soc
> > core create the necessary attributes, instead of requiring every
> > user to duplicate that code.
> > 
> > A possible interface might be
> > 
> > struct soc_device {
> > 	const char *machine;
> > 	const char *family;
> > 	/* ... */
> > 	struct device dev;
> > };
> 
> Either way, the probing functions would have to be called in order to
> populate the structure. Why is using the struct device_attribute
> show|store callbacks to call them a bad thing to do in this case?

Code is more complex than data, and we want to have the complexity
in a central location, not copied over all subarchitectures. When
you use a flattened device tree, the strings can simply point to
properties of the root device, so there would be very little to
do other than assign them.

> > struct soc_device *soc_device_register(const char *machine, const char *family);
> > 
> > For the nonstandard attributes, I would recommend having the individual
> > drivers call device_create_file, in order to discourage the use of 
> > device specific attribute names.
> 
> I'm not entirely sure what you mean here. I'm assuming you mean calling
> device_create_file from platform code once the device has been
> registered and a pointer passed back.

Right.

> If that's the case then surely the
> driver could set the attribute names to _any_ value still?
> 
> I really like the:
> 
> struct device_attribute soc_one_attrs[] = {
> 	__ATTR(machine,  S_IRUGO, ux500_get_machine,  NULL),
> 	__ATTR(family,   S_IRUGO, ux500_get_family,   NULL),
> 	__ATTR(soc_id,   S_IRUGO, ux500_get_soc_id,   NULL),
> 	/* ... */
> 	__ATTR_NULL,
> };

> ... interface. I think it's neat, and easy to read. Are you suggesting I
> should remove this altogether and replace it with passing const
> arguments for common attributes and insisting the platform code calls
> device_create_file for all non-standard ones? If so, if you would be
> kind enough to explain why this is better, I'd appreciate it.

I would prefer to standardise the attributes as much as possible. Ideally,
all SOCs should export the same set of attributes, and in no case should
there be multiple SOCs that have the same attribute name but with a
slightly different interface (e.g. one writable, or one root-only readable),
or the same contents in attributes of different names.

The best way to ensure this is to give less flexiblity to the person
implementing the individual SOC code. All attributes that are documented
to be available across SOCs can simply be automatically created and
filled with the data provided by the platform.

Having interfaces specific to one SOC should be the absolute exception,
so I'd try to make that as hard as possible.

	Arnd

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Hi Arnd,

> On Thursday 14 April 2011, Lee Jones wrote:
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
> 
> This definitely needs a changelog, explaining what the code is there for,
> and why you chose this interface and not the alternatives we discussed
> earlier.

No problem.

>> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> new file mode 100644
>> index 0000000..5e4d6ef
>> --- /dev/null
>> +++ b/drivers/base/soc.c
>> @@ -0,0 +1,127 @@
>> +/*
>> + * 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
>> + */
> 
> (I believe this would be Copyright Linaro Ltd, not ST-Ericsson SA,
> but better ask internally in ST-Ericsson about what your rules are)

We've had internal discussions about this. I believe this is the correct
thing to do. The Copyright should stay with ST-Ericsson.

>> +struct device *parent_soc;
>> +struct device *soc[MAX_SOCS];
> 
> The array should not be needed here, you can simply iterate all soc
> devices using device_for_each_child() if required.

Joy, another re-write.

(I think you are correct however)

> Global variables should really not have such generic names. Better
> make all variables static and make sure that the code using them
> can at there properly.

Agreed.

>> +int __init soc_device_register(struct device_attribute *soc_attrs[],
>> +                               int soc_count)
> 
> This needs to return the soc device, otherwise there is nothing that
> a platform can do with the device.

What do you think the platform would want to do with the device?

> Passing the soc_count the way you do won't work when you have different
> SoCs, 

Why won't the platform know how many SoCs are on a given platform?

> so better require the user to call the register function repeatedly.

... and if it truly doesn't know, how will it know how many times to
call the register function?

> I think a nicer interface would be to pass a data structure into it
> with the data you always want to export, and then have the soc
> core create the necessary attributes, instead of requiring every
> user to duplicate that code.
> 
> A possible interface might be
> 
> struct soc_device {
> 	const char *machine;
> 	const char *family;
> 	/* ... */
> 	struct device dev;
> };

Either way, the probing functions would have to be called in order to
populate the structure. Why is using the struct device_attribute
show|store callbacks to call them a bad thing to do in this case?

> struct soc_device *soc_device_register(const char *machine, const char *family);
> 
> For the nonstandard attributes, I would recommend having the individual
> drivers call device_create_file, in order to discourage the use of 
> device specific attribute names.

I'm not entirely sure what you mean here. I'm assuming you mean calling
device_create_file from platform code once the device has been
registered and a pointer passed back. If that's the case then surely the
driver could set the attribute names to _any_ value still?

I really like the:

struct device_attribute soc_one_attrs[] = {
	__ATTR(machine,  S_IRUGO, ux500_get_machine,  NULL),
	__ATTR(family,   S_IRUGO, ux500_get_family,   NULL),
	__ATTR(soc_id,   S_IRUGO, ux500_get_soc_id,   NULL),
	/* ... */
	__ATTR_NULL,
};

... interface. I think it's neat, and easy to read. Are you suggesting I
should remove this altogether and replace it with passing const
arguments for common attributes and insisting the platform code calls
device_create_file for all non-standard ones? If so, if you would be
kind enough to explain why this is better, I'd appreciate it.

>> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
>> new file mode 100644
>> index 0000000..988cf6f
>> --- /dev/null
>> +++ b/include/linux/sys_soc.h
>> @@ -0,0 +1,29 @@
>> +/*
>> + * 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 __SYS_SOC_H
>> +#define __SYS_SOC_H
>> +
>> +#include <linux/kobject.h>
>> +#include <linux/device.h>
>> +
>> +#define MAX_SOCS 8
> 
> No need to hardcode the maximum.

No problem.

Kind regards,
Lee

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Arnd Bergmann]

Hi Lee,

On Thursday 14 April 2011, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---

This definitely needs a changelog, explaining what the code is there for,
and why you chose this interface and not the alternatives we discussed
earlier.

> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> new file mode 100644
> index 0000000..5e4d6ef
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,127 @@
> +/*
> + * 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
> + */

(I believe this would be Copyright Linaro Ltd, not ST-Ericsson SA,
but better ask internally in ST-Ericsson about what your rules are)

> +struct device *parent_soc;
> +struct device *soc[MAX_SOCS];

The array should not be needed here, you can simply iterate all soc
devices using device_for_each_child() if required.

Global variables should really not have such generic names. Better
make all variables static and make sure that the code using them
can at there properly.

> +int __init soc_device_register(struct device_attribute *soc_attrs[],
> +                               int soc_count)

This needs to return the soc device, otherwise there is nothing that
a platform can do with the device.

Passing the soc_count the way you do won't work when you have different
SoCs, so better require the user to call the register function repeatedly.


I think a nicer interface would be to pass a data structure into it
with the data you always want to export, and then have the soc
core create the necessary attributes, instead of requiring every
user to duplicate that code.

A possible interface might be

struct soc_device {
	const char *machine;
	const char *family;
	/* ... */
	struct device dev;
};

struct soc_device *soc_device_register(const char *machine, const char *family);

For the nonstandard attributes, I would recommend having the individual
drivers call device_create_file, in order to discourage the use of 
device specific attribute names.

> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..988cf6f
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,29 @@
> +/*
> + * 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 __SYS_SOC_H
> +#define __SYS_SOC_H
> +
> +#include <linux/kobject.h>
> +#include <linux/device.h>
> +
> +#define MAX_SOCS 8

No need to hardcode the maximum.

	Arnd

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  127 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   29 +++++++++++
 4 files changed, 160 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 e9e5238..f381fcc 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -168,6 +168,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 config ARCH_NO_SYSDEV_OPS
 	bool
 	---help---
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..a0d246d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_SYS_SOC) += 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..5e4d6ef
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,127 @@
+/*
+ * 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>
+
+struct device *parent_soc;
+struct device *soc[MAX_SOCS];
+
+static void soc_device_remove_files(struct device *soc,
+                                    struct device_attribute soc_attrs[])
+{
+	int i = 0;
+
+	while (soc_attrs[i++].attr.name != NULL)
+		device_remove_file(soc, &soc_attrs[i]);
+}
+
+static int __init soc_device_create_files(struct device *soc,
+                                          struct device_attribute soc_attrs[])
+{
+	int ret = 0;
+	int i = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(soc, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:
+	soc_device_remove_files(soc, soc_attrs);
+	return ret;
+}
+
+void soc_device_release(struct device *soc)
+{
+	kfree(soc);
+}
+
+int __init soc_device_register(struct device_attribute *soc_attrs[],
+                               int soc_count)
+{
+	int ret, i;
+
+	if (!soc_attrs || soc_count > MAX_SOCS)
+		return -EINVAL;
+
+	/* Register top-level SoC device '/sys/devices/soc'. */
+	parent_soc = kzalloc(sizeof(struct device), GFP_KERNEL);
+	if (!parent_soc)
+		return -ENOMEM;
+
+	ret = dev_set_name(parent_soc, "soc");
+	if (ret)
+		goto soc_parent_free;
+
+	parent_soc->release = soc_device_release;
+
+	ret = device_register(parent_soc);
+	if (ret)
+		goto soc_parent_free;
+
+	/* Register each SoC and populate sysfs with requested attributes. */
+	for (i = 0; i < soc_count - 1; i++) {
+		soc[i] = kzalloc(sizeof(struct device), GFP_KERNEL);
+		if (!soc[i]) {
+			ret = -ENOMEM;
+			goto soc_out_of_memory;
+		}
+
+		ret = dev_set_name(soc[i], "soc%d", i);
+		if (ret)
+			goto soc_free_unreg;
+
+		soc[i]->parent = parent_soc;
+		soc[i]->release = soc_device_release;
+
+		ret = device_register(soc[i]);
+		if (ret)
+			goto soc_free_unreg;
+
+		ret = soc_device_create_files(soc[i], soc_attrs[i]);
+		if (ret)
+			goto soc_free_unreg;
+	}
+	return ret;
+
+soc_free_unreg:
+	kfree(soc[i]);
+soc_out_of_memory:
+	/* Unregister only previously registered SoCs. */
+	soc_device_unregister(soc_attrs, i);
+	return ret;
+
+soc_parent_free:
+	/* Free unregisterable parent SoC device. */
+	kfree(parent_soc);
+	return ret;
+}
+
+void soc_device_unregister(struct device_attribute *soc_attrs[],
+                           int soc_count)
+{
+	int i;
+
+	if (!soc_attrs || soc_count > MAX_SOCS)
+		return;
+
+	/* Unregister and free all SoC from sysfs. */
+	for (i = 0; i < soc_count - 1; i++) {
+		soc_device_remove_files(soc[i], soc_attrs[i]);
+		device_unregister(soc[i]);
+	}
+
+	/* Unregister top-level SoC device '/sys/devices/soc'. */
+	device_unregister(parent_soc);
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..988cf6f
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,29 @@
+/*
+ * 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 __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+
+#define MAX_SOCS 8
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_attrs: Multiple arrays of sysfs file attributes
+ * @num_socs: Amount of SoCs we're attempting to register
+ */
+int soc_device_register(struct device_attribute *soc_attrs[],
+                        int num_socs);
+/**
+ * soc_device_unregister - unregister SoC as a device
+ * @soc_attrs: Multiple arrays of sysfs file attributes
+ * @num_socs: Amount of SoCs we're attempting to register
+ */
+void soc_device_unregister(struct device_attribute *soc_attrs[],
+                           int num_socs);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.4.1

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

Hi James,

Firstly, thank you for taking the time to review my patch.

I hope this is more to your liking:

--------------------------------------------------------------------------------

From: Lee Jones <lee.jones@linaro.org>

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |  127 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   29 +++++++++++
 4 files changed, 160 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 e9e5238..f381fcc 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -168,6 +168,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 config ARCH_NO_SYSDEV_OPS
 	bool
 	---help---
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..a0d246d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_SYS_SOC) += 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..9862a7e
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,127 @@
+/*
+ * 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>
+
+struct device *parent_soc;
+struct device *soc[MAX_SOCS];
+
+static void soc_device_remove_files(struct device *soc,
+                                    struct device_attribute soc_attrs[])
+{
+	int i = 0;
+
+	while (soc_attrs[i++].attr.name != NULL)
+		device_remove_file(soc, &soc_attrs[i]);
+}
+
+static int __init soc_device_create_files(struct device *soc,
+                                          struct device_attribute soc_attrs[])
+{
+	int ret = 0;
+	int i = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(soc, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:
+	soc_device_remove_files(soc, soc_attrs);
+	return ret;
+}
+
+void soc_device_release(struct device *soc)
+{
+	kfree(soc);
+}
+
+int __init soc_device_register(struct device_attribute *soc_attrs[],
+                               int soc_count)
+{
+	int ret, i;
+
+	if (!soc_attrs || soc_count > MAX_SOCS)
+		return -EINVAL;
+
+	/* Register top-level SoC device '/sys/devices/soc'. */
+	parent_soc = kzalloc(sizeof(struct device), GFP_KERNEL);
+	if (!parent_soc)
+		return -ENOMEM;
+
+	ret = dev_set_name(parent_soc, "soc");
+	if (ret)
+		goto soc_parent_free;
+
+	parent_soc->release = soc_device_release;
+
+	ret = device_register(parent_soc);
+	if (ret)
+		goto soc_parent_free;
+
+	/* Register each SoC and populate sysfs with requested attributes. */
+	for (i = 0; i < soc_count - 1; i++) {
+		soc[i] = kzalloc(sizeof(struct device), GFP_KERNEL);
+		if (!soc[i]) {
+			ret = -ENOMEM;
+			goto soc_out_of_memory;
+		}
+
+		ret = dev_set_name(soc[i], "soc%d", i);
+		if (ret)
+			goto soc_free_unreg;
+
+		soc[i]->parent = parent_soc;
+		soc[i]->release = soc_device_release;
+
+		ret = device_register(soc[i]);
+		if (ret)
+			goto soc_free_unreg;
+
+		ret = soc_device_create_files(soc[i], soc_attrs[i]);
+		if (ret)
+			goto soc_free_unreg;
+	}
+	return ret;
+
+soc_free_unreg:
+	kfree(soc[i]);
+soc_out_of_memory:
+	/* Unregister only previously registered SoCs. */
+	soc_device_unregister(soc_attrs, i);
+	return ret;
+
+soc_parent_free:
+	/* Free unregisterable parent SoC device. */
+	kfree(parent_soc);
+	return ret;
+}
+
+void soc_device_unregister(struct device_attribute *soc_attrs[],
+                           int soc_count)
+{
+	int i;
+
+	if (!soc_attrs || soc_count > MAX_SOCS)
+		return;
+
+	/* Unregister and free all SoC from sysfs */
+	for (i = 0; i < soc_count - 1; i++) {
+		soc_device_remove_files(soc[i], soc_attrs[i]);
+		device_unregister(soc[i]);
+	}
+
+	/* Unregister top-level SoC device '/sys/devices/soc'. */
+	device_unregister(parent_soc);
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..529a798
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,29 @@
+/*
+ * 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 __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+
+#define MAX_SOCS 8
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_attr: Array of sysfs file attributes
+ * @num_socs: Amount of SoCs we're attempting to register
+ */
+int soc_device_register(struct device_attribute *soc_attrs[],
+                        int num_socs);
+/**
+ * soc_device_unregister - unregister SoC as a device
+ * @soc_attr: Array of sysfs file attributes
+ * @num_socs: Amount of SoCs we're attempting to register
+ */
+void soc_device_unregister(struct device_attribute *soc_attrs[],
+                           int num_socs);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.4.1

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Jamie Iles]

Hi Lee,

Works fine on my platform, but I have a couple of questions.

On Mon, Apr 11, 2011 at 07:01:16PM +0100, Lee Jones wrote:
> From: Lee Jones <lee.jones@linaro.org>
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/base/Kconfig    |    3 +
>  drivers/base/Makefile   |    1 +
>  drivers/base/soc.c      |   96 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |   29 ++++++++++++++
>  4 files changed, 129 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 e9e5238..f381fcc 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,6 +168,9 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +config SYS_SOC
> +	bool
> +
>  config ARCH_NO_SYSDEV_OPS
>  	bool
>  	---help---
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 4c5701c..a0d246d 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
>  obj-$(CONFIG_MODULES)	+= module.o
>  endif
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
> +obj-$(CONFIG_SYS_SOC) += 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..1a409e2
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,96 @@
> +/*
> + * 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>
> +
> +static char *soc_names[] = {"1", "2", "3", "4", "5", "6", "7", "8" };

Could this be made const?  Actually, could we just use dev_set_name() 
and do it at runtime?

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

I don't think this device can be statically allocated.

> +struct device *soc[MAX_SOCS];
> +
> +static int soc_device_create_files(struct device *soc,
> +                                   struct device_attribute soc_attrs[]);
> +
> +static void soc_device_remove_files(struct device *soc,
> +                                    struct device_attribute soc_attrs[]);

If you move soc_device_remove_files() above soc_device_create_files() 
then I don't think you need these prototypes.

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

This is used in the cleanup path of soc_device_create_files() so can't 
be annotated with __exit:

`soc_device_remove_files' referenced in section `.init.text' of 
drivers/built-in.o: defined in discarded section `.exit.text' of 
drivers/built-in.o

> +{
> +	int i = 0;
> +
> +	while (soc_attrs[i++].attr.name != NULL)
> +		device_remove_file(soc, &soc_attrs[i]);
> +}
> +
> +int __init soc_device_register(struct device_attribute *soc_attrs[],
> +                               int soc_count)
> +{
> +	int ret, i;

Shouldn't this check that soc_count < MAX_SOCS?

> +
> +	/* Register top-level SoC device '/sys/devices/soc' */
> +	ret = device_register(&parent_soc);
> +	if (ret)
> +		return ret;

I think that this device needs to be dynamically allocated and have a 
release function that does a kfree() on it.

> +
> +	/* Register each SoC and populate sysfs with requested attributes */
> +	for (i = 0; i < soc_count - 1; i++) {
> +		soc[i] = kmalloc(sizeof(struct device), GFP_KERNEL);
> +		if (!soc[i])
> +			return -ENOMEM;
> +
> +		memset(soc[i], 0, sizeof(struct device));

kzalloc() and remove the memset()?

> +		soc[i]->init_name = soc_names[i];

It would be good to use dev_set_name() here, but I wonder if perhaps the 
devices should be called soc1, soc2 etc rather than 1, 2?

> +		soc[i]->parent = &parent_soc;
> +
> +		ret = device_register(soc[i]);
> +		if (ret)
> +			return ret;
> +
> +		soc_device_create_files(soc[i], soc_attrs[i]);
> +	}
> +
> +	return ret;
> +}
> +
> +void __exit soc_device_unregister(struct device_attribute *soc_attrs[],
> +                                  int soc_count)
> +{
> +	int i;
> +
> +	/* Unregister and free all SoC from sysfs */
> +	for (i = 0; i < soc_count - 1; i++) {
> +		soc_device_remove_files(soc[i], soc_attrs[i]);
> +		device_unregister(soc[i]);
> +		kfree(soc[i]);

The kfree() should be done in the .release() method otherwise the device 
could be freed whilst someone still holds a reference.

> +	}
> +
> +	/* Unregister top-level SoC device '/sys/devices/soc' */
> +	device_unregister(&parent_soc);
> +}
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..072cd39
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,29 @@
> +/*
> + * 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 __SYS_SOC_H
> +#define __SYS_SOC_H
> +
> +#include <linux/kobject.h>
> +#include <linux/device.h>
> +
> +#define MAX_SOCS 8
> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_attr: Array of sysfs file attributes
> + * @soc: Parent node '/sys/devices/soc'
> + */
> +int soc_device_register(struct device_attribute *soc_attrs[],
> +                        int num_socs);
> +/**
> + *  soc_device_unregister - unregister SoC as a device
> + * @soc_attr: Array of sysfs file attributes
> + * @soc: Parent node '/sys/devices/soc'
> + */
> +void soc_device_unregister(struct device_attribute *soc_attrs[],
> +                           int num_socs);
> +
> +#endif /* __SYS_SOC_H */
> -- 
> 1.7.4.1
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] Framework for exporting System-on-Chip information via sysfs

[Lee Jones]

From: Lee Jones <lee.jones@linaro.org>

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/base/Kconfig    |    3 +
 drivers/base/Makefile   |    1 +
 drivers/base/soc.c      |   96 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sys_soc.h |   29 ++++++++++++++
 4 files changed, 129 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 e9e5238..f381fcc 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -168,6 +168,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config SYS_SOC
+	bool
+
 config ARCH_NO_SYSDEV_OPS
 	bool
 	---help---
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..a0d246d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_SYS_SOC) += 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..1a409e2
--- /dev/null
+++ b/drivers/base/soc.c
@@ -0,0 +1,96 @@
+/*
+ * 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>
+
+static char *soc_names[] = {"1", "2", "3", "4", "5", "6", "7", "8" };
+static struct device parent_soc = { .init_name = "soc", };
+struct device *soc[MAX_SOCS];
+
+static int soc_device_create_files(struct device *soc,
+                                   struct device_attribute soc_attrs[]);
+
+static void soc_device_remove_files(struct device *soc,
+                                    struct device_attribute soc_attrs[]);
+
+static int __init soc_device_create_files(struct device *soc,
+                                          struct device_attribute soc_attrs[])
+{
+	int ret = 0;
+	int i = 0;
+
+	while (soc_attrs[i].attr.name != NULL) {
+		ret = device_create_file(soc, &soc_attrs[i++]);
+		if (ret)
+			goto out;
+	}
+	return ret;
+
+out:
+	soc_device_remove_files(soc, soc_attrs);
+	return ret;
+}
+
+static void __exit soc_device_remove_files(struct device *soc,
+                                           struct device_attribute soc_attrs[])
+{
+	int i = 0;
+
+	while (soc_attrs[i++].attr.name != NULL)
+		device_remove_file(soc, &soc_attrs[i]);
+}
+
+int __init soc_device_register(struct device_attribute *soc_attrs[],
+                               int soc_count)
+{
+	int ret, i;
+
+	/* Register top-level SoC device '/sys/devices/soc' */
+	ret = device_register(&parent_soc);
+	if (ret)
+		return ret;
+
+	/* Register each SoC and populate sysfs with requested attributes */
+	for (i = 0; i < soc_count - 1; i++) {
+		soc[i] = kmalloc(sizeof(struct device), GFP_KERNEL);
+		if (!soc[i])
+			return -ENOMEM;
+
+		memset(soc[i], 0, sizeof(struct device));
+		soc[i]->init_name = soc_names[i];
+		soc[i]->parent = &parent_soc;
+
+		ret = device_register(soc[i]);
+		if (ret)
+			return ret;
+
+		soc_device_create_files(soc[i], soc_attrs[i]);
+	}
+
+	return ret;
+}
+
+void __exit soc_device_unregister(struct device_attribute *soc_attrs[],
+                                  int soc_count)
+{
+	int i;
+
+	/* Unregister and free all SoC from sysfs */
+	for (i = 0; i < soc_count - 1; i++) {
+		soc_device_remove_files(soc[i], soc_attrs[i]);
+		device_unregister(soc[i]);
+		kfree(soc[i]);
+	}
+
+	/* Unregister top-level SoC device '/sys/devices/soc' */
+	device_unregister(&parent_soc);
+}
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
new file mode 100644
index 0000000..072cd39
--- /dev/null
+++ b/include/linux/sys_soc.h
@@ -0,0 +1,29 @@
+/*
+ * 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 __SYS_SOC_H
+#define __SYS_SOC_H
+
+#include <linux/kobject.h>
+#include <linux/device.h>
+
+#define MAX_SOCS 8
+
+/**
+ * soc_device_register - register SoC as a device
+ * @soc_attr: Array of sysfs file attributes
+ * @soc: Parent node '/sys/devices/soc'
+ */
+int soc_device_register(struct device_attribute *soc_attrs[],
+                        int num_socs);
+/**
+ *  soc_device_unregister - unregister SoC as a device
+ * @soc_attr: Array of sysfs file attributes
+ * @soc: Parent node '/sys/devices/soc'
+ */
+void soc_device_unregister(struct device_attribute *soc_attrs[],
+                           int num_socs);
+
+#endif /* __SYS_SOC_H */
-- 
1.7.4.1