| Message ID | 1310476090-9807-1-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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]
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
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.
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
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
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
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 */
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