Re: [PATCH] base: add sysfs socs info

[Ryan Mallon]

On 12/16/2010 09:17 AM, Ryan Mallon wrote:
> On 12/15/2010 01:40 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> this provide an easy way to register soc information
> 
> This idea has been kicked around a few times in various forms, both as a
> proc file and as sysfs files. Cc'ed the arm-linux and embedded-linux
> lists since this patch mainly affects them.

Gah, my email client still has the old arm-linux address cached. Cc'ed
the new one.

~Ryan

>> arch, family, model, id, revision
> 
> Some SoCs want to expose additional information also. This patch doesn't
> appear to provide any standard way of extending the information
> available in sysfs.
> 
>> as this for at91sam9g20
>>
>> $ cat /sys/devices/system/soc/soc0/arch
>> current
> 
> What does this mean? Shouldn't it be ARM? Also, we already have ways to
> determine the architecture/cpu type.
> 
>> $ cat /sys/devices/system/soc/soc0/family
>> at91
>> $ cat /sys/devices/system/soc/soc0/id
>> at91sam9g20
>> $ cat /sys/devices/system/soc/soc0/model
>> 0x00000000019905a0
>> $ cat /sys/devices/system/soc/soc0/revision
>> 1.1
> 
> What is the difference between model and revision? Do these fields make
> sense for all SoCs?
> 
> What userspace tools actually need this information? Some of the
> extended information for various SoCs may be useful, but I can't think
> of many good reasons for a userspace application to care about the SoC
> family or revision.
> 
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx>
>> Cc: Nicolas Ferre <nicolas.ferre@xxxxxxxxx>
>> Cc: Patrice VILCHEZ <patrice.vilchez@xxxxxxxxx>
>> ---
>>  drivers/base/Makefile |    3 +-
>>  drivers/base/base.h   |    1 +
>>  drivers/base/init.c   |    1 +
>>  drivers/base/soc.c    |  124 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/soc.h   |   27 +++++++++++
>>  5 files changed, 155 insertions(+), 1 deletions(-)
>>  create mode 100644 drivers/base/soc.c
>>  create mode 100644 include/linux/soc.h
>>
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index 5f51c3b..cf3e59f 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -3,7 +3,8 @@
>>  obj-y			:= core.o sys.o bus.o dd.o \
>>  			   driver.o class.o platform.o \
>>  			   cpu.o firmware.o init.o map.o devres.o \
>> -			   attribute_container.o transport_class.o
>> +			   attribute_container.o transport_class.o \
>> +			   soc.o
>>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>>  obj-y			+= power/
>>  obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 2ca7f5b..e2daaf6 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -107,6 +107,7 @@ static inline int hypervisor_init(void) { return 0; }
>>  extern int platform_bus_init(void);
>>  extern int system_bus_init(void);
>>  extern int cpu_dev_init(void);
>> +extern int soc_dev_init(void);
>>  
>>  extern int bus_add_device(struct device *dev);
>>  extern void bus_probe_device(struct device *dev);
>> diff --git a/drivers/base/init.c b/drivers/base/init.c
>> index c8a934e..f908faa 100644
>> --- a/drivers/base/init.c
>> +++ b/drivers/base/init.c
>> @@ -33,5 +33,6 @@ void __init driver_init(void)
>>  	platform_bus_init();
>>  	system_bus_init();
>>  	cpu_dev_init();
>> +	soc_dev_init();
>>  	memory_dev_init();
>>  }
>> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> new file mode 100644
>> index 0000000..c24bb41
>> --- /dev/null
>> +++ b/drivers/base/soc.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * drivers/base/soc.c - basic SOC class support
>> + *
>> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@xxxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/sysdev.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/soc.h>
>> +#include <linux/device.h>
>> +
>> +#include "base.h"
>> +
>> +static int nb_socs;
>> +
>> +struct sysdev_class soc_sysdev_class = {
>> +	.name = "soc",
>> +};
>> +EXPORT_SYMBOL_GPL(soc_sysdev_class);
>> +
>> +#define print_u64_attr(field)							\
>> +static ssize_t print_socs_##field(struct sys_device *dev,			\
>> +				  struct sysdev_attribute *attr, char *buf)	\
>> +{										\
>> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
>> +										\
>> +	return sprintf(buf, "0x%016Lx\n", soc->field);				\
>> +}										\
>> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
>> +
>> +#define print_str_attr(field)							\
>> +static ssize_t print_socs_##field(struct sys_device *dev,			\
>> +				  struct sysdev_attribute *attr, char *buf)	\
>> +{										\
>> +	struct soc *soc = container_of(dev, struct soc, sysdev);		\
>> +										\
>> +	return sprintf(buf, "%s\n", soc->field);				\
>> +}										\
>> +static SYSDEV_ATTR(field, 0444, print_socs_##field, NULL);			\
> 
> At first glance this looks like two functions with the same name because
> of the identical print_socs##field bit. I intuitively expect field to
> just be the name of the sysfs file.
> 
>> +print_u64_attr(id)
>> +print_str_attr(arch)
>> +print_str_attr(family)
>> +print_str_attr(model)
>> +print_str_attr(revision)
> 
> These should have semicolons at the end (drop the final one from the
> macro name). Also I think the names should be in caps and should be
> renamed to better reflect what the do, i.e. SOC_SYSFS_U64_ATTR and
> SOC_SYSFS_STRING_ATTR.
> 
>> +static char *arch_current = "current";
> 
> Should be const.
> 
>> +/*
>> + * register_soc - Setup a sysfs device for a SOC.
>> + *
>> + * Initialize and register the SOC device.
>> + */
>> +int register_soc(struct soc *soc)
>> +{
> 
> This name implies that it does much more than just adding some sysfs
> files :-).
> 
>> +	int err;
>> +
>> +	if (!soc)
>> +		return -EINVAL;
> 
> Wouldn't bother with this check. Just crash so that we can catch buggy code.
> 
>> +	soc->sysdev.id = nb_socs;
>> +	soc->sysdev.cls = &soc_sysdev_class;
>> +
>> +	if (!soc->arch)
>> +		soc->arch = arch_current;
>> +
>> +	err = sysdev_register(&soc->sysdev);
>> +
>> +	if (err)
>> +		return err;
> 
> Why all the additional whitespace?
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_arch);
>> +
>> +	if (err)
>> +		goto end;
> 
> You can use sysfs_create_group to register a bunch of files which will
> greatly simply the code here.
> 
>> +	err = sysdev_create_file(&soc->sysdev, &attr_family);
>> +
>> +	if (err)
>> +		goto end0;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_model);
>> +
>> +	if (err)
>> +		goto end1;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_id);
>> +
>> +	if (err)
>> +		goto end2;
>> +
>> +	err = sysdev_create_file(&soc->sysdev, &attr_revision);
>> +
>> +	if (err)
>> +		goto end3;
>> +
>> +	nb_socs++;
> 
> If there is more than one SoC (SMP machine?) then how do you guarantee
> the order of registration? Should the registration function take id as a
> parameter?
> 
>> +	return 0;
>> +
>> +end3:
>> +	sysdev_remove_file(&soc->sysdev, &attr_id);
>> +end2:
>> +	sysdev_remove_file(&soc->sysdev, &attr_model);
>> +end1:
>> +	sysdev_remove_file(&soc->sysdev, &attr_family);
>> +end0:
>> +	sysdev_remove_file(&soc->sysdev, &attr_arch);
>> +end:
>> +	sysdev_unregister(&soc->sysdev);
>> +
>> +	return err;
>> +}
> 
> EXPORT_SYMBOL(register_soc)?
> 
>> +
>> +int __init soc_dev_init(void)
>> +{
>> +	nb_socs = 0;
>> +
>> +	return sysdev_class_register(&soc_sysdev_class);
>> +}
> 
> EXPORT_SYMBOL(soc_dev_init)?
> 
>> diff --git a/include/linux/soc.h b/include/linux/soc.h
>> new file mode 100644
>> index 0000000..55e6ea2
>> --- /dev/null
>> +++ b/include/linux/soc.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * include/linux/soc.h - generic soc definition
>> + *
>> + * Copyright (C) 2010 Jean-Chrisotphe PLAGNIOL-VILLARD * <plagnioj@xxxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef _LINUX_SOC_H_
>> +#define _LINUX_OSC_H_
>> +
>> +#include <linux/sysdev.h>
>> +
>> +struct soc {
>> +	u64 id;
>> +	char *arch;
>> +	char *family;
>> +	char *model;
>> +	char *revision;
>> +	struct sys_device sysdev;
>> +};
>> +
>> +extern int register_soc(struct soc *soc);
>> +extern struct sysdev_class soc_sysdev_class;
>> +
>> +#endif /* _LINUX_SOC_H_ */
> 
> ~Ryan
> 


-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@xxxxxxxxxxxxxxxx         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/