| Message ID | 1314880043-22517-1-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 01, 2011 at 01:27:19PM +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 device, which is returned post-registration, although > this should be discouraged. > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/base/Kconfig | 3 + > drivers/base/Makefile | 1 + > drivers/base/soc.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/sys_soc.h | 27 ++++++++++++ > 4 files changed, 134 insertions(+), 0 deletions(-) > create mode 100644 drivers/base/soc.c > create mode 100644 include/linux/sys_soc.h > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig > index 21cf46f..95f10c5 100644 > --- a/drivers/base/Kconfig > +++ b/drivers/base/Kconfig > @@ -172,6 +172,9 @@ config SYS_HYPERVISOR > bool > default n > > +config SYS_SOC > + bool > + > source "drivers/base/regmap/Kconfig" > > endmenu > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index 99a375a..a67a1e7 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES) += module.o > endif > obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o > obj-$(CONFIG_REGMAP) += regmap/ > +obj-$(CONFIG_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..e9d908b > --- /dev/null > +++ b/drivers/base/soc.c > @@ -0,0 +1,103 @@ > +/* > + * 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 DEFINE_SPINLOCK(register_lock); If this is really needed, please make it a mutex as you end up calling code paths that can sleep. > +static int soc_count = 0; This should not be needed. > + > +static struct device soc_grandparent = { > + .init_name = "soc", > +}; NEVER create a static struct device, this is totally not needed and completly wrong. > +static ssize_t soc_info_get(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > + > + if (!strcmp(attr->attr.name, "machine")) > + return sprintf(buf, "%s\n", soc_dev->machine); > + if (!strcmp(attr->attr.name, "family")) > + return sprintf(buf, "%s\n", soc_dev->family); > + if (!strcmp(attr->attr.name, "revision")) > + return sprintf(buf, "%s\n", soc_dev->revision); > + if (!strcmp(attr->attr.name, "soc_id")) { > + if (soc_dev->pfn_soc_id) > + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); > + else return sprintf(buf, "N/A \n"); Move this line > + } > + > + return -EINVAL; To here? > +} > + > +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 soc_device_remove_files(struct device *soc, int i) > +{ > + while (i > 0) > + device_remove_file(soc, &soc_attrs[--i]); > +} You do know how to add multiple files, right? Oh wait, this is all not the way to do this in the first place. You just raced userspace :( > + > +static int __init soc_device_create_files(struct device *dev) > +{ > + int ret = 0; > + int i = 0; > + > + while (soc_attrs[i].attr.name != NULL) { > + ret = device_create_file(dev, &soc_attrs[i++]); > + if (ret) > + goto out; > + } > + return ret; > + > +out: > + soc_device_remove_files(dev, --i); > + return ret; > +} Yup, you raced userspace. Please use the proper api for defining a group of attributes for a device by default so that the driver core handles creating and destroying them correctly and in a way that will not be racy (unlike this way.) > + > +int __init soc_device_register(struct device *dev) Don't pass a struct device in here, why are you making the caller create the device? This is the function that should create it for you? > +{ > + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > + int ret; > + > + spin_lock_irq(®ister_lock); Ok. > + > + if (!soc_count) { > + /* Register top-level SoC device '/sys/devices/soc' */ > + ret = device_register(&soc_grandparent); device_register can't be called with a spinlock. You must have gotten lucky in your testing, if you tested this. Anyway, this is not needed at all, please don't create "dummy" devices in the sysfs tree, properly hook up your new device to where it should be in the device tree. > + if (ret) { > + spin_unlock_irq(®ister_lock); > + return ret; > + } > + } > + > + soc_count++; Nope, don't use this, again, use the correct kernel interface for providing you with a unique number that increments as needed. Don't roll your own that will end up being wrong in the end (like this is, what happens if you remove a device in the middle?) > + dev->parent = &soc_grandparent; Nope, don't do that. > + dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine); > + > + spin_unlock_irq(®ister_lock); > + > + ret = device_register(dev); > + if (ret) > + return ret; > + > + soc_device_create_files(dev); Nor that. See above for why. > + > + return ret; > +} > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h > new file mode 100644 > index 0000000..811d7fe > --- /dev/null > +++ b/include/linux/sys_soc.h > @@ -0,0 +1,27 @@ > +/* > + * 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> > + > +struct soc_device { > + struct device dev; > + const char *machine; > + const char *family; > + const char *revision; > + const char *(*pfn_soc_id)(void); > +}; Nice structure, but why do you export it, you aren't using it anywhere, nor should you be... > + > +/** > + * soc_device_register - register SoC as a device > + * @dev: Parent node '/sys/devices/soc/X' > + */ > +int soc_device_register(struct device *dev); This whole api needs to be rethunk, please, it's really wrong. What you want is a way to create a soc device, by calling a function, not be responsible for creating it, then call the soc core, and then somehow, removing it. Oh wait, you forgot to have a function to remove anything created with the above call, that seems really broken and wrong. greg k-h
Comments and questions in-line. On 02/09/11 00:34, Greg KH wrote: > On Thu, Sep 01, 2011 at 01:27:19PM +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 device, which is returned post-registration, although >> this should be discouraged. >> >> Signed-off-by: Lee Jones <lee.jones@linaro.org> >> --- >> drivers/base/Kconfig | 3 + >> drivers/base/Makefile | 1 + >> drivers/base/soc.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/sys_soc.h | 27 ++++++++++++ >> 4 files changed, 134 insertions(+), 0 deletions(-) >> create mode 100644 drivers/base/soc.c >> create mode 100644 include/linux/sys_soc.h >> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig >> index 21cf46f..95f10c5 100644 >> --- a/drivers/base/Kconfig >> +++ b/drivers/base/Kconfig >> @@ -172,6 +172,9 @@ config SYS_HYPERVISOR >> bool >> default n >> >> +config SYS_SOC >> + bool >> + >> source "drivers/base/regmap/Kconfig" >> >> endmenu >> diff --git a/drivers/base/Makefile b/drivers/base/Makefile >> index 99a375a..a67a1e7 100644 >> --- a/drivers/base/Makefile >> +++ b/drivers/base/Makefile >> @@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES) += module.o >> endif >> obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o >> obj-$(CONFIG_REGMAP) += regmap/ >> +obj-$(CONFIG_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..e9d908b >> --- /dev/null >> +++ b/drivers/base/soc.c >> @@ -0,0 +1,103 @@ >> +/* >> + * 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 DEFINE_SPINLOCK(register_lock); > > If this is really needed, please make it a mutex as you end up calling > code paths that can sleep. If I use: "the correct kernel interface for providing you with a unique number that increments as needed" I'm I correct in assuming that I don't need to use locking? >> +static int soc_count = 0; > > This should not be needed. > >> + >> +static struct device soc_grandparent = { >> + .init_name = "soc", >> +}; > > NEVER create a static struct device, this is totally not needed and > completly wrong. Noted on the static point, but I believe that it is needed. That's how /sys/devices/platform does it, which this essentially replaces. >> +static ssize_t soc_info_get(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); >> + >> + if (!strcmp(attr->attr.name, "machine")) >> + return sprintf(buf, "%s\n", soc_dev->machine); >> + if (!strcmp(attr->attr.name, "family")) >> + return sprintf(buf, "%s\n", soc_dev->family); >> + if (!strcmp(attr->attr.name, "revision")) >> + return sprintf(buf, "%s\n", soc_dev->revision); >> + if (!strcmp(attr->attr.name, "soc_id")) { >> + if (soc_dev->pfn_soc_id) >> + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); >> + else return sprintf(buf, "N/A \n"); > > Move this line > >> + } >> + >> + return -EINVAL; > > To here? > I initially had invalid requests return a useful message, but I was told to remove it and return -EINVAL instead: "Just return -EINVAL or similar here to propogate the error to the user." Jamie, what say you? >> +} >> + >> +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 soc_device_remove_files(struct device *soc, int i) >> +{ >> + while (i > 0) >> + device_remove_file(soc, &soc_attrs[--i]); >> +} > > You do know how to add multiple files, right? Oh wait, this is all not > the way to do this in the first place. You just raced userspace :( I didn't, but I will soon. >> + >> +static int __init soc_device_create_files(struct device *dev) >> +{ >> + int ret = 0; >> + int i = 0; >> + >> + while (soc_attrs[i].attr.name != NULL) { >> + ret = device_create_file(dev, &soc_attrs[i++]); >> + if (ret) >> + goto out; >> + } >> + return ret; >> + >> +out: >> + soc_device_remove_files(dev, --i); >> + return ret; >> +} > > Yup, you raced userspace. Please use the proper api for defining a > group of attributes for a device by default so that the driver core > handles creating and destroying them correctly and in a way that will > not be racy (unlike this way.) No problem. I'll look it up and make the changes. >> + >> +int __init soc_device_register(struct device *dev) > > Don't pass a struct device in here, why are you making the caller create > the device? This is the function that should create it for you? I guess I can just return it instead, so it will become: struct device * __init soc_device_register(void) Is that more in keeping with what you would expect to see? >> +{ >> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); >> + int ret; >> + >> + spin_lock_irq(®ister_lock); > > Ok. > >> + >> + if (!soc_count) { >> + /* Register top-level SoC device '/sys/devices/soc' */ >> + ret = device_register(&soc_grandparent); > > device_register can't be called with a spinlock. You must have gotten > lucky in your testing, if you tested this. > > Anyway, this is not needed at all, please don't create "dummy" devices > in the sysfs tree, properly hook up your new device to where it should > be in the device tree. We need a /sys/devices/soc, how else can this be done? Would it make you happier if I called it a bus? >> + if (ret) { >> + spin_unlock_irq(®ister_lock); >> + return ret; >> + } >> + } >> + >> + soc_count++; > > Nope, don't use this, again, use the correct kernel interface for > providing you with a unique number that increments as needed. Don't > roll your own that will end up being wrong in the end (like this is, > what happens if you remove a device in the middle?) Noted. >> + dev->parent = &soc_grandparent; > > Nope, don't do that. As above. >> + dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine); >> + >> + spin_unlock_irq(®ister_lock); >> + >> + ret = device_register(dev); >> + if (ret) >> + return ret; >> + >> + soc_device_create_files(dev); > > Nor that. > > See above for why. > >> + >> + return ret; >> +} >> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h >> new file mode 100644 >> index 0000000..811d7fe >> --- /dev/null >> +++ b/include/linux/sys_soc.h >> @@ -0,0 +1,27 @@ >> +/* >> + * 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> >> + >> +struct soc_device { >> + struct device dev; >> + const char *machine; >> + const char *family; >> + const char *revision; >> + const char *(*pfn_soc_id)(void); >> +}; > > Nice structure, but why do you export it, you aren't using it anywhere, > nor should you be... Okay, I'll use the returning struct device * from soc_device_register() register instead. >> + >> +/** >> + * soc_device_register - register SoC as a device >> + * @dev: Parent node '/sys/devices/soc/X' >> + */ >> +int soc_device_register(struct device *dev); > > This whole api needs to be rethunk, please, it's really wrong. > > What you want is a way to create a soc device, by calling a function, > not be responsible for creating it, then call the soc core, and then > somehow, removing it. > > Oh wait, you forgot to have a function to remove anything created with > the above call, that seems really broken and wrong. Again, I did have an unregister function, but I was told to remove it. "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." What if I put it back and removed the __exit syntax and used it as a standard unregister/free function? Would that be more to everyone's taste? Thanks for your time. Kind regards, Lee
On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote: > On 02/09/11 00:34, Greg KH wrote: > > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote: > >> +static ssize_t soc_info_get(struct device *dev, > >> + struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > >> + > >> + if (!strcmp(attr->attr.name, "machine")) > >> + return sprintf(buf, "%s\n", soc_dev->machine); > >> + if (!strcmp(attr->attr.name, "family")) > >> + return sprintf(buf, "%s\n", soc_dev->family); > >> + if (!strcmp(attr->attr.name, "revision")) > >> + return sprintf(buf, "%s\n", soc_dev->revision); > >> + if (!strcmp(attr->attr.name, "soc_id")) { > >> + if (soc_dev->pfn_soc_id) > >> + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); > >> + else return sprintf(buf, "N/A \n"); > > > > Move this line > > > >> + } > >> + > >> + return -EINVAL; > > > > To here? > > > > I initially had invalid requests return a useful message, but I was told > to remove it and return -EINVAL instead: > > "Just return -EINVAL or similar here to propogate the error to the user." > > Jamie, what say you? Well if there isn't an ID for the SoC, then I think it's better for a read() to return an error rather than have to parse for a special string like "N/A ". So rather than returning that string, perhaps something like: if (!strcmp(attr->attr.name, "soc_id")) { if (!soc_dev->pfn_soc_id) return -ENOENT; return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); } or if you don't think this is an error, default to an id of "0" so that it can be parsed by tools? Jamie
On 02/09/11 10:29, Jamie Iles wrote: > On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote: >> On 02/09/11 00:34, Greg KH wrote: >>> On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote: >>>> +static ssize_t soc_info_get(struct device *dev, >>>> + struct device_attribute *attr, >>>> + char *buf) >>>> +{ >>>> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); >>>> + >>>> + if (!strcmp(attr->attr.name, "machine")) >>>> + return sprintf(buf, "%s\n", soc_dev->machine); >>>> + if (!strcmp(attr->attr.name, "family")) >>>> + return sprintf(buf, "%s\n", soc_dev->family); >>>> + if (!strcmp(attr->attr.name, "revision")) >>>> + return sprintf(buf, "%s\n", soc_dev->revision); >>>> + if (!strcmp(attr->attr.name, "soc_id")) { >>>> + if (soc_dev->pfn_soc_id) >>>> + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); >>>> + else return sprintf(buf, "N/A \n"); >>> >>> Move this line >>> >>>> + } >>>> + >>>> + return -EINVAL; >>> >>> To here? >>> >> >> I initially had invalid requests return a useful message, but I was told >> to remove it and return -EINVAL instead: >> >> "Just return -EINVAL or similar here to propogate the error to the user." >> >> Jamie, what say you? > > Well if there isn't an ID for the SoC, then I think it's better for a > read() to return an error rather than have to parse for a special string > like "N/A ". So rather than returning that string, perhaps something > like: > > if (!strcmp(attr->attr.name, "soc_id")) { > if (!soc_dev->pfn_soc_id) > return -ENOENT; > return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); > } > > or if you don't think this is an error, default to an id of "0" so that > it can be parsed by tools? What about at the very end of the function? What should we return if an inappropriate attr->attr.name is used?
On Fri, Sep 02, 2011 at 10:37:47AM +0100, Lee Jones wrote: > On 02/09/11 10:29, Jamie Iles wrote: > > On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote: > >> On 02/09/11 00:34, Greg KH wrote: > >>> On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote: > >>>> +static ssize_t soc_info_get(struct device *dev, > >>>> + struct device_attribute *attr, > >>>> + char *buf) > >>>> +{ > >>>> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > >>>> + > >>>> + if (!strcmp(attr->attr.name, "machine")) > >>>> + return sprintf(buf, "%s\n", soc_dev->machine); > >>>> + if (!strcmp(attr->attr.name, "family")) > >>>> + return sprintf(buf, "%s\n", soc_dev->family); > >>>> + if (!strcmp(attr->attr.name, "revision")) > >>>> + return sprintf(buf, "%s\n", soc_dev->revision); > >>>> + if (!strcmp(attr->attr.name, "soc_id")) { > >>>> + if (soc_dev->pfn_soc_id) > >>>> + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); > >>>> + else return sprintf(buf, "N/A \n"); > >>> > >>> Move this line > >>> > >>>> + } > >>>> + > >>>> + return -EINVAL; > >>> > >>> To here? > >>> > >> > >> I initially had invalid requests return a useful message, but I was told > >> to remove it and return -EINVAL instead: > >> > >> "Just return -EINVAL or similar here to propogate the error to the user." > >> > >> Jamie, what say you? > > > > Well if there isn't an ID for the SoC, then I think it's better for a > > read() to return an error rather than have to parse for a special string > > like "N/A ". So rather than returning that string, perhaps something > > like: > > > > if (!strcmp(attr->attr.name, "soc_id")) { > > if (!soc_dev->pfn_soc_id) > > return -ENOENT; > > return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); > > } > > > > or if you don't think this is an error, default to an id of "0" so that > > it can be parsed by tools? > > What about at the very end of the function? > > What should we return if an inappropriate attr->attr.name is used? That shouldn't happen as it is only ever registered with valid attributes. That said, you do need to return something so -EINVAL seems like a reasonable choice. Jamie
On Friday 02 September 2011, Greg KH wrote: > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote: > > + > > +int __init soc_device_register(struct device *dev) > > Don't pass a struct device in here, why are you making the caller create > the device? This is the function that should create it for you? Well, when we have device tree based probing, the soc device might already have been created by of_bus_probe(), so we just want to pass it to the soc core code in order to create the common interfaces. It's probably not right that soc_device_register actually calls device_register, because when you have a device that you pass in, it will already be registered. > > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h > > new file mode 100644 > > index 0000000..811d7fe > > --- /dev/null > > +++ b/include/linux/sys_soc.h > > @@ -0,0 +1,27 @@ > > +/* > > + * 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> > > + > > +struct soc_device { > > + struct device dev; > > + const char *machine; > > + const char *family; > > + const char *revision; > > + const char *(*pfn_soc_id)(void); > > +}; > > Nice structure, but why do you export it, you aren't using it anywhere, > nor should you be... This gets filled by the soc-specific code with information that is exported through the common callbacks, so it needs to be visible to the platforms. > > + > > +/** > > + * soc_device_register - register SoC as a device > > + * @dev: Parent node '/sys/devices/soc/X' > > + */ > > +int soc_device_register(struct device *dev); > > This whole api needs to be rethunk, please, it's really wrong. > > What you want is a way to create a soc device, by calling a function, > not be responsible for creating it, then call the soc core, and then > somehow, removing it. Yes, it's definitely still not where it ought to be, and we've been discussing this for ages. It all started out as a way to find out information about the system, and I suggested to better make that information part of the interface for the soc node that is required anyway, rather than making up a purely fictional device. Now, we need this to cover two scenarios: 1. The classic setup without device tree where the soc node is created by platform code. This would work great by just calling a function that creates the device and lets the soc code add the children according to compile-time tables of child devices. 2. Probing from OF device-tree, where the linux device tree is populated by walking the devices found in the flat device tree description. This doesn't really fit your suggestion, nor does it fit Lee's patch really, because it would require reparenting the soc node into /sys/devices/soc and registering it again after it's already registered. I think the ideal solution would be to make the soc layer a class that can be set when initially adding the device (one way or another) and that adds the standard attributes. Since you don't really like classes any more, another option would be to just have a function call that adds an attribute group to the soc node, but then it's not easy to find which devices are actually soc root nodes. > Oh wait, you forgot to have a function to remove anything created with > the above call, that seems really broken and wrong. There is no way to remove a soc from the system, just like there is no complementary function to pcibios_scan_root that removes the PCI root bridge. Arnd
On Fri, Sep 02, 2011 at 04:02:14PM +0200, Arnd Bergmann wrote: > On Friday 02 September 2011, Greg KH wrote: > > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote: > > > + > > > +int __init soc_device_register(struct device *dev) > > > > Don't pass a struct device in here, why are you making the caller create > > the device? This is the function that should create it for you? > > Well, when we have device tree based probing, the soc device might already > have been created by of_bus_probe(), so we just want to pass it to the > soc core code in order to create the common interfaces. No, you want to pass the PARENT pointer in here that this new SOC device will hang off of. You can't expect to pass in any old random struct device and expect it to make any kind of sense at all. > It's probably not right that soc_device_register actually calls > device_register, because when you have a device that you pass in, > it will already be registered. And where would it be registered? No, please don't do this, again, have this code create the struct device because it is responsible for it. That's the way the rest of the kernel works, please don't try to create new models to confuse everyone even more than is needed :) > > > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h > > > new file mode 100644 > > > index 0000000..811d7fe > > > --- /dev/null > > > +++ b/include/linux/sys_soc.h > > > @@ -0,0 +1,27 @@ > > > +/* > > > + * 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> > > > + > > > +struct soc_device { > > > + struct device dev; > > > + const char *machine; > > > + const char *family; > > > + const char *revision; > > > + const char *(*pfn_soc_id)(void); > > > +}; > > > > Nice structure, but why do you export it, you aren't using it anywhere, > > nor should you be... > > This gets filled by the soc-specific code with information that is exported > through the common callbacks, so it needs to be visible to the platforms. But this api doesn't allow that to happen, so use it to create the data in the first place somehow, this is really confusing as-is. > > > + > > > +/** > > > + * soc_device_register - register SoC as a device > > > + * @dev: Parent node '/sys/devices/soc/X' > > > + */ > > > +int soc_device_register(struct device *dev); > > > > This whole api needs to be rethunk, please, it's really wrong. > > > > What you want is a way to create a soc device, by calling a function, > > not be responsible for creating it, then call the soc core, and then > > somehow, removing it. > > Yes, it's definitely still not where it ought to be, and we've been > discussing this for ages. It all started out as a way to find out > information about the system, and I suggested to better make that > information part of the interface for the soc node that is required > anyway, rather than making up a purely fictional device. > > Now, we need this to cover two scenarios: > > 1. The classic setup without device tree where the soc node is > created by platform code. This would work great by just calling > a function that creates the device and lets the soc code add > the children according to compile-time tables of child devices. > > 2. Probing from OF device-tree, where the linux device tree is populated > by walking the devices found in the flat device tree description. > This doesn't really fit your suggestion, nor does it fit Lee's > patch really, because it would require reparenting the soc node > into /sys/devices/soc and registering it again after it's already > registered. No it would not, both of these work just fine like we handle with all other subsystems. You pass in the parent, and if it is not present, the device will show up under /sys/devices/virtual/ just fine. Please don't create new type of things here. > I think the ideal solution would be to make the soc layer a > class that can be set when initially adding the device (one way > or another) and that adds the standard attributes. Since you don't > really like classes any more, another option would be to just > have a function call that adds an attribute group to the soc node, > but then it's not easy to find which devices are actually soc > root nodes. Then make it a bus, like it should be, which would solve all of this, right? > > Oh wait, you forgot to have a function to remove anything created with > > the above call, that seems really broken and wrong. > > There is no way to remove a soc from the system, just like there > is no complementary function to pcibios_scan_root that removes > the PCI root bridge. And no one is ever going to remove the module that created the SOC device in the first place? It just really bothers me to see creation code but no removal code, as it feels wrong somehow :) thanks, greg k-h
On Fri, Sep 02, 2011 at 09:44:52AM +0100, Lee Jones wrote: > Comments and questions in-line. As they should be :) > On 02/09/11 00:34, Greg KH wrote: > > On Thu, Sep 01, 2011 at 01:27:19PM +0100, Lee Jones wrote: > >> +static DEFINE_SPINLOCK(register_lock); > > > > If this is really needed, please make it a mutex as you end up calling > > code paths that can sleep. > > If I use: > > "the correct kernel interface for providing you with a unique number > that increments as needed" > > I'm I correct in assuming that I don't need to use locking? Yes, don't you agree? You do know what this interface is, right? :) > >> +static int soc_count = 0; > > > > This should not be needed. > > > >> + > >> +static struct device soc_grandparent = { > >> + .init_name = "soc", > >> +}; > > > > NEVER create a static struct device, this is totally not needed and > > completly wrong. > > Noted on the static point, but I believe that it is needed. > > That's how /sys/devices/platform does it, which this essentially replaces. NO NO NO NO NO NO Again, no. This does NOT replace /sys/devices/platform in any shape, manner, or form. Platform devices are "special" in that they are the "grab-bag of devices that no one seems to know how to hook up to a sane bus so we throw them all in the same heap and try to ignore the ugliness of them." Never repeat the platform code anywhere else, that's not acceptable at all. If you want to make this a "bus" that's probably best, as then you would get to enumerate over all /sys/bus/soc/ devices just fine, no matter where in the device tree they live. > >> +static ssize_t soc_info_get(struct device *dev, > >> + struct device_attribute *attr, > >> + char *buf) > >> +{ > >> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > >> + > >> + if (!strcmp(attr->attr.name, "machine")) > >> + return sprintf(buf, "%s\n", soc_dev->machine); > >> + if (!strcmp(attr->attr.name, "family")) > >> + return sprintf(buf, "%s\n", soc_dev->family); > >> + if (!strcmp(attr->attr.name, "revision")) > >> + return sprintf(buf, "%s\n", soc_dev->revision); > >> + if (!strcmp(attr->attr.name, "soc_id")) { > >> + if (soc_dev->pfn_soc_id) > >> + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); > >> + else return sprintf(buf, "N/A \n"); > > > > Move this line > > > >> + } > >> + > >> + return -EINVAL; > > > > To here? > > > > I initially had invalid requests return a useful message, but I was told > to remove it and return -EINVAL instead: > > "Just return -EINVAL or similar here to propogate the error to the user." No, if a SOC doesn't have one of these attributes, then just don't create the sysfs file. Don't return -EINVAL for something that you know better than to have created in the first place, are you trying to make it hard for userspace? :) > >> +int __init soc_device_register(struct device *dev) > > > > Don't pass a struct device in here, why are you making the caller create > > the device? This is the function that should create it for you? > > I guess I can just return it instead, so it will become: > > struct device * __init soc_device_register(void) > > Is that more in keeping with what you would expect to see? no. How did you pass the attribute data into this core so that it knows how to get to it? You didn't, which is a mess. How about something like: struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr); That would have the proper way to handle lifetime rules, userspace notificatation of userpace, placement in sysfs, and all the other good stuff. I'm feeling like it would be simpler for me to write this code than to keep providing review comments like this telling you to rewrite the whole thing over again each time :) Anyone want to send me a device that this code is needed for so I can do it? > >> +{ > >> + struct soc_device *soc_dev = container_of(dev, struct soc_device, dev); > >> + int ret; > >> + > >> + spin_lock_irq(®ister_lock); > > > > Ok. > > > >> + > >> + if (!soc_count) { > >> + /* Register top-level SoC device '/sys/devices/soc' */ > >> + ret = device_register(&soc_grandparent); > > > > device_register can't be called with a spinlock. You must have gotten > > lucky in your testing, if you tested this. > > > > Anyway, this is not needed at all, please don't create "dummy" devices > > in the sysfs tree, properly hook up your new device to where it should > > be in the device tree. > > We need a /sys/devices/soc, how else can this be done? No you do not not, why would you think so? > Would it make you happier if I called it a bus? Yes, see the other response about creating a bus for these, which would give you /sys/bus/soc/ where you can properly enumerate your devices, which is what I think you want to be able to do, right? thanks, greg k-h
On Friday 02 September 2011, Greg KH wrote: > no. > > How did you pass the attribute data into this core so that it knows how > to get to it? You didn't, which is a mess. > > How about something like: > struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr); > > That would have the proper way to handle lifetime rules, userspace > notificatation of userpace, placement in sysfs, and all the other good > stuff. I early on objected to the individual soc driver providing a set of attributes, because we really want the attributes to follow a common interface and I want to make it *hard* to add arbitrary other attributes that are not documented. > > >> + > > >> + if (!soc_count) { > > >> + /* Register top-level SoC device '/sys/devices/soc' */ > > >> + ret = device_register(&soc_grandparent); > > > > > > device_register can't be called with a spinlock. You must have gotten > > > lucky in your testing, if you tested this. > > > > > > Anyway, this is not needed at all, please don't create "dummy" devices > > > in the sysfs tree, properly hook up your new device to where it should > > > be in the device tree. > > > > We need a /sys/devices/soc, how else can this be done? > > No you do not not, why would you think so? Well, where is all comes from is the desire to have a way from user space to find these devices, based on some path in the device tree. We had discussed putting it into /sys/class/soc /sys/bus/soc /sys/devices/platform/soc /sys/devices/platform/soc%d and a few others and eventually settled on /sys/devices/soc. > > Would it make you happier if I called it a bus? > > Yes, see the other response about creating a bus for these, which would > give you /sys/bus/soc/ where you can properly enumerate your devices, > which is what I think you want to be able to do, right? I think that would be a reasonable interface, but how does this work when the device was created by of_platform_bus_probe? Should we assume that any top-level device in the flattened device tree is a soc? How about just adding a child device in each soc node that is registered to the soc_bus_type? That would end up looking like /sys/devices/db8500 /soc0 # this is the soc device /i2c-0 # and here are the other platform devices below db8500 /spi-0 /bus/soc/devices/soc0 -> ../../../devices/db8500/soc0 /bus/platform/devices/i2c-0 -> ../../../devices/db8500/i2c-0 Arnd
On Friday 02 September 2011, Greg KH wrote: > On Fri, Sep 02, 2011 at 04:02:14PM +0200, Arnd Bergmann wrote: > > On Friday 02 September 2011, Greg KH wrote: > > > > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h > > > > new file mode 100644 > > > > index 0000000..811d7fe > > > > --- /dev/null > > > > +++ b/include/linux/sys_soc.h > > > > @@ -0,0 +1,27 @@ > > > > +/* > > > > + * 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> > > > > + > > > > +struct soc_device { > > > > + struct device dev; > > > > + const char *machine; > > > > + const char *family; > > > > + const char *revision; > > > > + const char *(*pfn_soc_id)(void); > > > > +}; > > > > > > Nice structure, but why do you export it, you aren't using it anywhere, > > > nor should you be... > > > > This gets filled by the soc-specific code with information that is exported > > through the common callbacks, so it needs to be visible to the platforms. > > But this api doesn't allow that to happen, so use it to create the data > in the first place somehow, this is really confusing as-is. Yes, that is true. I hadn't noticed the 'struct device' in the above structure. What really needs to happen is the platform code to fill out a structure with all the attribute data and to pass that into the soc common code as constant data which then gets added to the device data. > > I think the ideal solution would be to make the soc layer a > > class that can be set when initially adding the device (one way > > or another) and that adds the standard attributes. Since you don't > > really like classes any more, another option would be to just > > have a function call that adds an attribute group to the soc node, > > but then it's not easy to find which devices are actually soc > > root nodes. > > Then make it a bus, like it should be, which would solve all of this, > right? I'm not sure if we're talking about the same thing. Do you mean adding an artifical device (like a class_device) under the soc device node and have that be of soc_bus type? Or do you mean putting the entire soc with all its platform_device children on the soc_bus? In the former case, it's a somewhat ugly representation. It would be really nice for the soc to be just one device, not one device to hang all the children off and another one to provide a user-space interface. In the latter case, we would have to rewrite the way we probe SoCs from the device tree so that the root node of the soc becomes a soc_bus device instead of a platform_bus device. > > > Oh wait, you forgot to have a function to remove anything created with > > > the above call, that seems really broken and wrong. > > > > There is no way to remove a soc from the system, just like there > > is no complementary function to pcibios_scan_root that removes > > the PCI root bridge. > > And no one is ever going to remove the module that created the SOC > device in the first place? It just really bothers me to see creation > code but no removal code, as it feels wrong somehow :) SoC code fundamentally cannot be a module, since the soc contains all the basic stuff like irq controller, clocksource etc. If it didn't have those, it would not be a system-on-a-chip but rather some random multifunction device you hang off some bus. Arnd
On Fri, Sep 02, 2011 at 07:22:04PM +0200, Arnd Bergmann wrote: > On Friday 02 September 2011, Greg KH wrote: > > no. > > > > How did you pass the attribute data into this core so that it knows how > > to get to it? You didn't, which is a mess. > > > > How about something like: > > struct soc_device *soc_device_create(struct device *parent, struct soc_device_attributes *attr); > > > > That would have the proper way to handle lifetime rules, userspace > > notificatation of userpace, placement in sysfs, and all the other good > > stuff. > > I early on objected to the individual soc driver providing a set of > attributes, because we really want the attributes to follow a > common interface and I want to make it *hard* to add arbitrary > other attributes that are not documented. That's fine, which is why the struct soc_device_attributes are there, those are common and well defined. We can just do a simple return of 'int' to keep the caller from ever having access to the device at all, which, if these can never be removed, is a great way to keep anyone from messing with them. So the interface gets even simpler: int soc_device_create(struct device *parent, struct soc_device_attributes *attr); Look good? > > > >> + > > > >> + if (!soc_count) { > > > >> + /* Register top-level SoC device '/sys/devices/soc' */ > > > >> + ret = device_register(&soc_grandparent); > > > > > > > > device_register can't be called with a spinlock. You must have gotten > > > > lucky in your testing, if you tested this. > > > > > > > > Anyway, this is not needed at all, please don't create "dummy" devices > > > > in the sysfs tree, properly hook up your new device to where it should > > > > be in the device tree. > > > > > > We need a /sys/devices/soc, how else can this be done? > > > > No you do not not, why would you think so? > > Well, where is all comes from is the desire to have a way from user space > to find these devices, based on some path in the device tree. We had > discussed putting it into > > /sys/class/soc > /sys/bus/soc > /sys/devices/platform/soc > /sys/devices/platform/soc%d > > and a few others and eventually settled on /sys/devices/soc. No, please use /sys/bus/soc as these are all of the same "type". > > > Would it make you happier if I called it a bus? > > > > Yes, see the other response about creating a bus for these, which would > > give you /sys/bus/soc/ where you can properly enumerate your devices, > > which is what I think you want to be able to do, right? > > I think that would be a reasonable interface, but how does this work > when the device was created by of_platform_bus_probe? Should we > assume that any top-level device in the flattened device tree is > a soc? How about just adding a child device in each soc node that > is registered to the soc_bus_type? Yes, that will work with the above soc_device_create() call. > That would end up looking like > > /sys/devices/db8500 > /soc0 # this is the soc device > /i2c-0 # and here are the other platform devices below db8500 > /spi-0 > /bus/soc/devices/soc0 -> ../../../devices/db8500/soc0 > /bus/platform/devices/i2c-0 -> ../../../devices/db8500/i2c-0 Looks good to me. Using a bus, and the above interface, will result in this. thanks, greg k-h
I don't have a great deal of time at the moment, as I'm currently on the train on my way to Plumbers, but I would like to say just one thing. On 02/09/11 17:31, Greg KH wrote: > I'm feeling like it would be simpler for me to write this code than to > keep providing review comments like this telling you to rewrite the > whole thing over again each time :) I'd really rather you didn't. I have put a great deal of my own time into this and I really want to see it through. Up until now I have been finding it quite difficult to please everyone. I have been trying to follow everyone's suggestions and as soon as I think we're getting close, the world flips upside down. When I return, I will sit down and go through all of your (and Arnd's) suggestions and re-write it. Please bear with me. > Anyone want to send me a device that this code is needed for so I can do > it? No. ;) Kind regards, Lee
On Tuesday 06 September 2011 10:41:11 Lee Jones wrote: > I don't have a great deal of time at the moment, as I'm currently on the > train on my way to Plumbers, but I would like to say just one thing. > > On 02/09/11 17:31, Greg KH wrote: > > I'm feeling like it would be simpler for me to write this code than to > > keep providing review comments like this telling you to rewrite the > > whole thing over again each time > > I'd really rather you didn't. > > I have put a great deal of my own time into this and I really want to > see it through. Up until now I have been finding it quite difficult to > please everyone. I have been trying to follow everyone's suggestions and > as soon as I think we're getting close, the world flips upside down. > When I return, I will sit down and go through all of your (and Arnd's) > suggestions and re-write it. Ok. BTW, I feel a bit guilty for making you go through all this work on code that was written by other people and you are not to blame for, but I think we on the right track now. If you have any doubts about how to go on with the next version, please contact on IRC or phone so we can discuss the details. I think that will be more productive than the past email exchanges. Arnd
On Tue, Sep 06, 2011 at 10:41:11AM +0100, Lee Jones wrote: > I don't have a great deal of time at the moment, as I'm currently on the > train on my way to Plumbers, but I would like to say just one thing. > > On 02/09/11 17:31, Greg KH wrote: > > I'm feeling like it would be simpler for me to write this code than to > > keep providing review comments like this telling you to rewrite the > > whole thing over again each time :) > > I'd really rather you didn't. > > I have put a great deal of my own time into this and I really want to > see it through. Up until now I have been finding it quite difficult to > please everyone. I have been trying to follow everyone's suggestions and > as soon as I think we're getting close, the world flips upside down. > When I return, I will sit down and go through all of your (and Arnd's) > suggestions and re-write it. Ok, but expect a very tough code review if you do this. :) greg k-h
On 06/09/11 20:45, Greg KH wrote: > On Tue, Sep 06, 2011 at 10:41:11AM +0100, Lee Jones wrote: >> I don't have a great deal of time at the moment, as I'm currently on the >> train on my way to Plumbers, but I would like to say just one thing. >> >> On 02/09/11 17:31, Greg KH wrote: >>> I'm feeling like it would be simpler for me to write this code than to >>> keep providing review comments like this telling you to rewrite the >>> whole thing over again each time :) >> >> I'd really rather you didn't. >> >> I have put a great deal of my own time into this and I really want to >> see it through. Up until now I have been finding it quite difficult to >> please everyone. I have been trying to follow everyone's suggestions and >> as soon as I think we're getting close, the world flips upside down. >> When I return, I will sit down and go through all of your (and Arnd's) >> suggestions and re-write it. > > Ok, but expect a very tough code review if you do this. :) What's new? ;)
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index 21cf46f..95f10c5 100644 --- a/drivers/base/Kconfig +++ b/drivers/base/Kconfig @@ -172,6 +172,9 @@ config SYS_HYPERVISOR bool default n +config SYS_SOC + bool + source "drivers/base/regmap/Kconfig" endmenu diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 99a375a..a67a1e7 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_MODULES) += module.o endif obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o obj-$(CONFIG_REGMAP) += regmap/ +obj-$(CONFIG_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..e9d908b --- /dev/null +++ b/drivers/base/soc.c @@ -0,0 +1,103 @@ +/* + * 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 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 = container_of(dev, struct soc_device, dev); + + if (!strcmp(attr->attr.name, "machine")) + return sprintf(buf, "%s\n", soc_dev->machine); + if (!strcmp(attr->attr.name, "family")) + return sprintf(buf, "%s\n", soc_dev->family); + if (!strcmp(attr->attr.name, "revision")) + return sprintf(buf, "%s\n", soc_dev->revision); + if (!strcmp(attr->attr.name, "soc_id")) { + if (soc_dev->pfn_soc_id) + return sprintf(buf, "%s\n", soc_dev->pfn_soc_id()); + else return sprintf(buf, "N/A \n"); + } + + return -EINVAL; +} + +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 soc_device_remove_files(struct device *soc, int i) +{ + while (i > 0) + device_remove_file(soc, &soc_attrs[--i]); +} + +static int __init soc_device_create_files(struct device *dev) +{ + int ret = 0; + int i = 0; + + while (soc_attrs[i].attr.name != NULL) { + ret = device_create_file(dev, &soc_attrs[i++]); + if (ret) + goto out; + } + return ret; + +out: + soc_device_remove_files(dev, --i); + return ret; +} + +int __init soc_device_register(struct device *dev) +{ + struct soc_device *soc_dev = container_of(dev, struct soc_device, 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++; + dev->parent = &soc_grandparent; + dev_set_name(dev, "%i_%s", soc_count, soc_dev->machine); + + spin_unlock_irq(®ister_lock); + + ret = device_register(dev); + if (ret) + return ret; + + soc_device_create_files(dev); + + return ret; +} diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h new file mode 100644 index 0000000..811d7fe --- /dev/null +++ b/include/linux/sys_soc.h @@ -0,0 +1,27 @@ +/* + * 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> + +struct soc_device { + struct device dev; + const char *machine; + const char *family; + const char *revision; + const char *(*pfn_soc_id)(void); +}; + +/** + * soc_device_register - register SoC as a device + * @dev: Parent node '/sys/devices/soc/X' + */ +int soc_device_register(struct device *dev); + +#endif /* __SYS_SOC_H */
This patch introduces a new driver to drivers/base. The driver provides a means to export vital SoC data out to userspace via sysfs. Standard information applicable to all SoCs are exported to: /sys/devices/soc/[1|2|3|...]/[family|machine|revision|soc_id]. It is possible to create SoC specific items via the soc device, which is returned post-registration, although this should be discouraged. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/base/Kconfig | 3 + drivers/base/Makefile | 1 + drivers/base/soc.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/sys_soc.h | 27 ++++++++++++ 4 files changed, 134 insertions(+), 0 deletions(-) create mode 100644 drivers/base/soc.c create mode 100644 include/linux/sys_soc.h