diff mbox

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

Message ID 1302792592-17484-2-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones April 14, 2011, 2:49 p.m. UTC
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

Comments

Arnd Bergmann April 17, 2011, 6:36 p.m. UTC | #1
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
Lee Jones April 21, 2011, 9:44 a.m. UTC | #2
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
Arnd Bergmann April 21, 2011, 11:03 a.m. UTC | #3
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
Lee Jones April 21, 2011, 11:56 a.m. UTC | #4
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
Russell King - ARM Linux April 27, 2011, 8:48 p.m. UTC | #5
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?
Lee Jones April 28, 2011, 6:46 a.m. UTC | #6
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
diff mbox

Patch

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 */