[PATCH 1/4] Framework for exporting System-on-Chip information via sysfs
Jamie Iles
jamie at jamieiles.com
Wed Aug 10 09:29:56 EDT 2011
Hi Lee,
A few minor comments inline.
Jamie
On Wed, Aug 10, 2011 at 02:03:39PM +0100, Lee Jones wrote:
> This patch introduces a new driver to drivers/base. The
> driver provides a means to export vital SoC data out to
> userspace via sysfs. Standard information applicable to all
> SoCs are exported to:
>
> /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id].
>
> It is possible to create SoC specific items via the
> soc_parent, which is returned post-registration, although
> this should be discouraged.
>
> Signed-off-by: Lee Jones <lee.jones at linaro.org>
> ---
> drivers/base/Kconfig | 3 +
> drivers/base/Makefile | 1 +
> drivers/base/soc.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/sys_soc.h | 41 ++++++++++++++++++
> 4 files changed, 149 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..372ef3a 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -168,4 +168,7 @@ config SYS_HYPERVISOR
> bool
> default n
>
> +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..05944ef
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + *
> + * Author: Lee Jones <lee.jones at 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 DEFINE_SPINLOCK(register_lock);
> +static int soc_count = 0;
> +
> +static struct device soc_grandparent = {
> + .init_name = "soc",
> +};
> +
> +static ssize_t soc_info_get(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct soc_device *soc_dev = dev->platform_data;
> +
> + if (!strncmp(attr->attr.name, "machine", 7))
> + return sprintf(buf, "%s\n", soc_dev->machine);
> + if (!strncmp(attr->attr.name, "family", 6))
> + return sprintf(buf, "%s\n", soc_dev->family);
> + if (!strncmp(attr->attr.name, "soc_id", 6))
> + return sprintf(buf, "%s\n", soc_dev->soc_id);
> + if (!strncmp(attr->attr.name, "revision", 8))
> + return sprintf(buf, "%s\n", soc_dev->revision);
I think strcmp() would be better here rather than strncmp() otherwise
there could be problems if someone adds an attribute that has a previous
one as a starting sub-string.
> +
> + return sprintf(buf, "Unknown attribute name - %s\n", attr->attr.name);
Just return -EINVAL or similar here to propogate the error to the user.
> +}
> +
> +struct device_attribute soc_attrs[] = {
> + __ATTR(machine, S_IRUGO, soc_info_get, NULL),
> + __ATTR(family, S_IRUGO, soc_info_get, NULL),
> + __ATTR(soc_id, S_IRUGO, soc_info_get, NULL),
> + __ATTR(revision, S_IRUGO, soc_info_get, NULL),
> + __ATTR_NULL,
> +};
> +
> +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]);
> +}
> +
> +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);
This should count back from i and unregister the attributes as
soc_device_remove_files may try and remove files that didn't get
registered.
> + return ret;
> +}
> +
> +int __init soc_device_register(struct device *soc_parent,
> + struct soc_device *soc_dev)
> +{
> + int ret;
> +
> + spin_lock_irq(®ister_lock);
> +
> + if (!soc_count) {
> + /* Register top-level SoC device '/sys/devices/soc' */
> + ret = device_register(&soc_grandparent);
> + if (ret)
> + {
> + spin_unlock_irq(®ister_lock);
> + return ret;
> + }
> + }
> +
> + soc_count++;
> + soc_parent->parent = &soc_grandparent;
> + dev_set_name(soc_parent, "%i", soc_count);
> + soc_parent->platform_data = soc_dev;
I don't think platform_data is the right place for this. It's not clear
what soc_parent and soc_dev do here as soc_dev never gets registered.
Should this be:
soc_dev->parent = &soc_grandparent;
dev_set_name(soc_dev, "%i", soc_count);
device_register(soc_dev);
and drop soc_parent and add the files to soc_device? In soc_info_get(),
you could then do:
struct soc_device *soc = container_of(dev, struct soc_device, dev);
> + spin_unlock_irq(®ister_lock);
> +
> + ret = device_register(soc_parent);
> + if (ret)
> + return ret;
> +
> + soc_device_create_files(soc_parent);
> +
> + return ret;
> +}
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..3de6405
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Lee Jones <lee.jones at 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
These defines don't appear to be used anywhere afaict.
> +
> +struct soc_device {
> + struct device dev;
> + const char *machine;
> + const char *family;
> + const char *soc_id;
> + const char *revision;
> +};
> +
> +/**
> + * soc_device_register - register SoC as a device
> + * @soc_parent: Parent node '/sys/devices/soc/X'
> + * @soc_dev: SoC device specific information
> + */
> +int soc_device_register(struct device *soc_parent,
> + struct soc_device *soc_dev);
> +
> +/**
> + * soc_device_unregister - unregister SoC as a device
> + * @soc_parent: Parent node '/sys/devices/soc/X'
> + */
> +void soc_device_unregister(struct device *soc_parent);
This doesn't appear to exist, and probably doesn't need to.
More information about the linux-arm-kernel
mailing list