Re: [RFC PATCHv2 1/2] Export SoC info through sysfs

[Eduardo Valentin <eduardo.valentin@nokia.com>]

Hello,

On Fri, Mar 11, 2011 at 01:35:10PM +0100, ext Maxime Coquelin wrote:
> Common base to export System-on-Chip related informations through sysfs.
> 
> Creation of a "socinfo" directory under /sys/.
> Creation of SoC information entries.
> 
> Signed-off-by: Maxime COQUELIN <maxime.coquelin-nonst@stericsson.com>
> ---
>  Documentation/ABI/testing/sysfs-socinfo |   16 ++++++
>  drivers/base/Kconfig                    |    3 +
>  drivers/base/Makefile                   |    1 +
>  drivers/base/soc.c                      |   79 +++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h                 |   50 +++++++++++++++++++
>  5 files changed, 149 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-socinfo
>  create mode 100644 drivers/base/soc.c
>  create mode 100644 include/linux/sys_soc.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-socinfo b/Documentation/ABI/testing/sysfs-socinfo
> new file mode 100644
> index 0000000..afd9da2
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-socinfo
> @@ -0,0 +1,16 @@
> +What:		/sys/socinfo
> +Date:		March 2011
> +contact:	Maxime Coquelin <maxime.coquelin-nonst@stericsson.com>
> +Description:
> +		The /sys/socinfo directory contains information about the
> +		System-on-Chip.	It is only available if platform implements it.
> +		This directory contains two kind of attributes :
> +			- common attributes: 
> +				* machine: the name of the machine.
> +				* family: the family name of the SoC
> +			- SoC-specific attributes: The SoC vendor can declare attributes
> +			  to export some strings to user-space, like the serial-number for
> +			  example.
> +
> +Users:
> +		User-space applications which needs these kind of attributes.
> 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 5f51c3b..f3bcfb3 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..046b43b
> --- /dev/null
> +++ b/drivers/base/soc.c
> @@ -0,0 +1,79 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> 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 kobject *soc_object;
> +
> +ssize_t show_soc_info(struct kobject *kobj,
> +			struct kobj_attribute *attr, char *buf)
> +{
> +	struct sysfs_soc_info *si = container_of(attr,
> +				struct sysfs_soc_info, attr);
> +
> +	if (si->info)
> +		return sprintf(buf, "%s\n", si->info);
> +
> +	return si->get_info(buf, si);
> +}
> +
> +int __init register_sysfs_soc_info(struct sysfs_soc_info *info, int nb_info)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < nb_info; i++) {
> +		ret = sysfs_create_file(soc_object, &info[i].attr.attr);
> +		if (ret) {
> +			for (i -= 1; i >= 0; i--)
> +				sysfs_remove_file(soc_object, &info[i].attr.attr);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}

>From functional perspective, this looks like a sysfs_create_group.
Now it makes me wonder if this thing would make sense. Maybe it's
better to create a node under platform and then add attributes to it, as suggested
in V1 thread. I don't know, this could still be done by the socinfo code.
I mean, location is still an issue it seams :-)

Another thing, what could be done is, instead of creating new data structures to hold
the attributes, a struct  attribute_group could be pass instead during registration time.
What do you think??

> +
> +static struct attribute *soc_attrs[] = {
> +	NULL,
> +};
> +
> +static struct attribute_group soc_attr_group = {
> +	.attrs = soc_attrs,
> +};


What is the point of the above two ?

> +
> +int __init register_sysfs_soc(struct sysfs_soc_info *info, size_t num)
> +{
> +	int ret;
> +
> +	soc_object = kobject_create_and_add("socinfo", NULL);
> +	if (!soc_object) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	ret = sysfs_create_group(soc_object, &soc_attr_group);
> +	if (ret)
> +		goto kset_exit;

You add an empty group here.

> +
> +	ret = register_sysfs_soc_info(info, num);
> +	if (ret)
> +		goto group_exit;

But the real thing happens here.

> +
> +	return 0;
> +
> +group_exit:
> +	sysfs_remove_group(soc_object, &soc_attr_group);
> +kset_exit:
> +	kobject_put(soc_object);
> +exit:
> +	return ret;
> +}
> +
> diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
> new file mode 100644
> index 0000000..05e5529
> --- /dev/null
> +++ b/include/linux/sys_soc.h
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2011
> + * Author: Maxime Coquelin <maxime.coquelin-nonst@stericsson.com> for ST-Ericsson.
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#ifndef __SYS_SOC_H
> +#define __SYS_SOC_H
> +
> +#include <linux/kobject.h>
> +
> +/**
> + * struct sys_soc_info - SoC exports related informations
> + * @name: name of the export
> + * @info: pointer on the key to export
> + * @get_info: callback to retrieve key if info field is NULL
> + * @attr: export's sysdev class attribute
> + */
> +struct sysfs_soc_info {
> +	const char *info;
> +	ssize_t (*get_info)(char *buf, struct sysfs_soc_info *);
> +	struct kobj_attribute attr;
> +};
> +
> +ssize_t show_soc_info(struct kobject *, struct kobj_attribute *, char *);
> +
> +#define SYSFS_SOC_ATTR_VALUE(_name, _value) {		\
> +	.attr.attr.name = _name,			\
> +	.attr.attr.mode = S_IRUGO,			\
> +	.attr.show	= show_soc_info,		\
> +	.info           = _value,			\
> +}
> +
> +#define SYSFS_SOC_ATTR_CALLBACK(_name, _callback) {	\
> +	.attr.attr.name = _name,			\
> +	.attr.attr.mode = S_IRUGO,			\
> +	.attr.show	= show_soc_info,		\
> +	.get_info       = _callback,			\
> +}
> +
> +/**
> + * register_sys_soc - register the soc information
> + * @name: name of the machine
> + * @info: pointer on the info table to export
> + * @num: number of info to export
> + *
> + * NOTE: This function must only be called once
> + */
> +int register_sysfs_soc(struct sysfs_soc_info *info, size_t num);
> +
> +#endif /* __SYS_SOC_H */
> -- 
> 1.7.1

-- 
Eduardo Valentin