diff mbox

[v2,1/2] Create eeprom_dev hardware class for EEPROM devices

Message ID 1390504562-20333-1-git-send-email-curt@cumulusnetworks.com
State Superseded
Headers show

Commit Message

Curt Brune Jan. 23, 2014, 7:16 p.m. UTC
Create a new hardware class under /sys/class/eeprom_dev

EEPROM drivers can register their devices with the eeprom_dev class
during instantiation.

The registered devices show up as:

  /sys/class/eeprom_dev/eeprom0
  /sys/class/eeprom_dev/eeprom1
  ...
  /sys/class/eeprom_dev/eeprom[N]

Each member of the eeprom class exports a sysfs file called "label",
containing the label property from the corresponding device tree node.

Example:

  /sys/class/eeprom_dev/eeprom0/label

If the device tree node property "label" does not exist the value
"unknown" is used.

Note: The class cannot be called 'eeprom' as that is the name of the
I/O file created by the driver.  The class name appears as a
sub-directory within the main device directory.  Hence the class name
'eeprom_dev'.

Userspace can use the label to identify what the EEPROM is for.

The real device is available from the class device via the "device"
link:

  /sys/class/eeprom_dev/eeprom0/device

Signed-off-by: Curt Brune <curt@cumulusnetworks.com>
---
 Documentation/misc-devices/eeprom_hw_class.txt |   81 ++++++++++++
 drivers/misc/eeprom/Kconfig                    |   11 ++
 drivers/misc/eeprom/Makefile                   |    1 +
 drivers/misc/eeprom/eeprom_class.c             |  159 ++++++++++++++++++++++++
 include/linux/eeprom_class.h                   |   35 ++++++
 5 files changed, 287 insertions(+)
 create mode 100644 Documentation/misc-devices/eeprom_hw_class.txt
 create mode 100644 drivers/misc/eeprom/eeprom_class.c
 create mode 100644 include/linux/eeprom_class.h

Comments

Laszlo Papp Jan. 24, 2014, 6:42 p.m. UTC | #1
> Note: The class cannot be called 'eeprom' as that is the name of the
> I/O file created by the driver.  The class name appears as a
> sub-directory within the main device directory.  Hence the class name
> 'eeprom_dev'.

I am not sure I follow the reasoning here, but it is possibly because
I lack some knowledge. Could you please describe bad thing would
happen if "/sys/class/eeprom/eeprom0/label" would be used as opposed
to "/sys/class/eeprom_dev/eeprom0/label"?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Curt Brune Jan. 24, 2014, 7:27 p.m. UTC | #2
On Fri Jan 24 18:42, Laszlo Papp wrote:
> > Note: The class cannot be called 'eeprom' as that is the name of the
> > I/O file created by the driver.  The class name appears as a
> > sub-directory within the main device directory.  Hence the class name
> > 'eeprom_dev'.
> 
> I am not sure I follow the reasoning here, but it is possibly because
> I lack some knowledge. Could you please describe bad thing would
> happen if "/sys/class/eeprom/eeprom0/label" would be used as opposed
> to "/sys/class/eeprom_dev/eeprom0/label"?

By way of example -- let's say I have an at24 device on i2c bus 2,
with address 0x54.  In sysfs the device can be found by its bus
address as:

  $ cd /sys/bus/i2c/devices/2-0054
  $ ls -l

  total 0
  lrwxrwxrwx 1 root root    0 Jan 24 19:11 driver ->  ../../../../../../bus/i2c/drivers/at24
  -rw------- 1 root root  256 Jan 23 23:33 eeprom
  drwxr-xr-x 3 root root    0 Jan 23 23:33 eeprom_dev
  -r--r--r-- 1 root root 4096 Jan 24 19:11 modalias
  -r--r--r-- 1 root root 4096 Jan 24 19:11 name
  lrwxrwxrwx 1 root root    0 Jan 24 19:11 subsystem ->  ../../../../../../bus/i2c
  -rw-r--r-- 1 root root 4096 Jan 24 19:11 uevent

The file "/sys/bus/i2c/devices/2-0054/eeprom" comes from the at24
driver.  That is the file name the EEPROM driver exports for I/O to
the device.  User space applications read/write this file to
read/write the physical EEPROM via the at24 driver.

The directory "/sys/bus/i2c/devices/2-0054/eeprom_dev" comes from the
sysfs class name "eeprom_dev".  All sysfs class names appear as
directories with the corresponding device directory.

See the conflict?  If the class was also called "eeprom" it would
clash with the existing "eeprom" file.  There cannot be two things
named /sys/bus/i2c/devices/2-0054/eeprom.

The files under /sys/class/eeprom_dev are symlinks to the "eeprom_dev"
directories of the physical devices.  For this example:

  $ cd /sys/class/eeprom_dev
  $ ls -l eeprom0
  lrwxrwxrwx 1 root root 0 Jan 23 23:33 eeprom0 -> ../../devices/soc.0/ffe03000.i2c/i2c-0/i2c-2/2-0054/eeprom_dev/eeprom0

Believe me I wanted to use "eeprom" as the class name originally, as
it makes a lot of sense.  But the sysfs file creation failed due to
the duplicate name.

I was not about to change the at24 driver as user space expects the
"eeprom" name.

Hence the class name is eeprom_dev.

Hope that helps.

Cheers,
Curt
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laszlo Papp Jan. 24, 2014, 8:19 p.m. UTC | #3
On Fri, Jan 24, 2014 at 7:27 PM, Curt Brune <curt@cumulusnetworks.com> wrote:
> On Fri Jan 24 18:42, Laszlo Papp wrote:
>> > Note: The class cannot be called 'eeprom' as that is the name of the
>> > I/O file created by the driver.  The class name appears as a
>> > sub-directory within the main device directory.  Hence the class name
>> > 'eeprom_dev'.
>>
>> I am not sure I follow the reasoning here, but it is possibly because
>> I lack some knowledge. Could you please describe bad thing would
>> happen if "/sys/class/eeprom/eeprom0/label" would be used as opposed
>> to "/sys/class/eeprom_dev/eeprom0/label"?
>
> By way of example -- let's say I have an at24 device on i2c bus 2,
> with address 0x54.  In sysfs the device can be found by its bus
> address as:
>
>   $ cd /sys/bus/i2c/devices/2-0054
>   $ ls -l
>
>   total 0
>   lrwxrwxrwx 1 root root    0 Jan 24 19:11 driver ->  ../../../../../../bus/i2c/drivers/at24
>   -rw------- 1 root root  256 Jan 23 23:33 eeprom
>   drwxr-xr-x 3 root root    0 Jan 23 23:33 eeprom_dev
>   -r--r--r-- 1 root root 4096 Jan 24 19:11 modalias
>   -r--r--r-- 1 root root 4096 Jan 24 19:11 name
>   lrwxrwxrwx 1 root root    0 Jan 24 19:11 subsystem ->  ../../../../../../bus/i2c
>   -rw-r--r-- 1 root root 4096 Jan 24 19:11 uevent
>
> The file "/sys/bus/i2c/devices/2-0054/eeprom" comes from the at24
> driver.  That is the file name the EEPROM driver exports for I/O to
> the device.  User space applications read/write this file to
> read/write the physical EEPROM via the at24 driver.
>
> The directory "/sys/bus/i2c/devices/2-0054/eeprom_dev" comes from the
> sysfs class name "eeprom_dev".  All sysfs class names appear as
> directories with the corresponding device directory.
>
> See the conflict?  If the class was also called "eeprom" it would
> clash with the existing "eeprom" file.  There cannot be two things
> named /sys/bus/i2c/devices/2-0054/eeprom.

Yes, this is more comprehensive for a newcomer, thanks.

> The files under /sys/class/eeprom_dev are symlinks to the "eeprom_dev"
> directories of the physical devices.  For this example:
>
>   $ cd /sys/class/eeprom_dev
>   $ ls -l eeprom0
>   lrwxrwxrwx 1 root root 0 Jan 23 23:33 eeprom0 -> ../../devices/soc.0/ffe03000.i2c/i2c-0/i2c-2/2-0054/eeprom_dev/eeprom0
>
> Believe me I wanted to use "eeprom" as the class name originally, as
> it makes a lot of sense.  But the sysfs file creation failed due to
> the duplicate name.
>
> I was not about to change the at24 driver as user space expects the
> "eeprom" name.
>
> Hence the class name is eeprom_dev.
>
> Hope that helps.

Yes, it does and I most certainly believe you.

I am not the maintainer of this code, nor do I have any knowledge
about the API promise in the kernel, but this case seems to be a major
upgrade to the Linux eeprom stack, and hence I would not personally
worry about compatibility.

If the API is kept, the Linux kernel will have an IMHO broken stack
for many upcoming years. IMO, the benefits of the different name does
not outweigh the disadvantages, but I will leave it with the
corresponding maintainer...
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Lutomirski Jan. 25, 2014, 12:23 a.m. UTC | #4
On 01/23/2014 11:16 AM, Curt Brune wrote:
> Create a new hardware class under /sys/class/eeprom_dev
> 
> EEPROM drivers can register their devices with the eeprom_dev class
> during instantiation.
> 
> The registered devices show up as:
> 
>   /sys/class/eeprom_dev/eeprom0
>   /sys/class/eeprom_dev/eeprom1
>   ...
>   /sys/class/eeprom_dev/eeprom[N]
> 
> Each member of the eeprom class exports a sysfs file called "label",
> containing the label property from the corresponding device tree node.
> 
> Example:
> 
>   /sys/class/eeprom_dev/eeprom0/label
> 
> If the device tree node property "label" does not exist the value
> "unknown" is used.
> 
> Note: The class cannot be called 'eeprom' as that is the name of the
> I/O file created by the driver.  The class name appears as a
> sub-directory within the main device directory.  Hence the class name
> 'eeprom_dev'.
> 
> Userspace can use the label to identify what the EEPROM is for.

Since my previous email [1] seems to have vanished into the void, I'll
try again more succinctly:

How will this work on non device tree / openfirmware systems?

Is there a better way to expose topology information (i.e. that the
eeprom belongs to another device that might not live on the i2c bus at all)?

Can we expose type information?  There's a big difference between SPD
EEPROMs, EDID EEPROMs, and nic mac-address-containing EEPROMs, for example.

--Andy

> 
> The real device is available from the class device via the "device"
> link:
> 
>   /sys/class/eeprom_dev/eeprom0/device
> 
> Signed-off-by: Curt Brune <curt-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
> ---
>  Documentation/misc-devices/eeprom_hw_class.txt |   81 ++++++++++++
>  drivers/misc/eeprom/Kconfig                    |   11 ++
>  drivers/misc/eeprom/Makefile                   |    1 +
>  drivers/misc/eeprom/eeprom_class.c             |  159 ++++++++++++++++++++++++
>  include/linux/eeprom_class.h                   |   35 ++++++
>  5 files changed, 287 insertions(+)
>  create mode 100644 Documentation/misc-devices/eeprom_hw_class.txt
>  create mode 100644 drivers/misc/eeprom/eeprom_class.c
>  create mode 100644 include/linux/eeprom_class.h
> 
> diff --git a/Documentation/misc-devices/eeprom_hw_class.txt b/Documentation/misc-devices/eeprom_hw_class.txt
> new file mode 100644
> index 0000000..b5cbc35
> --- /dev/null
> +++ b/Documentation/misc-devices/eeprom_hw_class.txt
> @@ -0,0 +1,81 @@
> +EEPROM Device Hardware Class
> +============================
> +
> +This feature is enabled by CONFIG_EEPROM_CLASS.
> +
> +The original problem:
> +
> +We work on several different switching platforms, each of which has
> +about 64 EEPROMs, one for each of the 10G SFP+ modules.  In addition
> +the systems typically have a board info EEPROM, SPD and power supply
> +EEPROMs.  It is difficult to map the device tree entries for the
> +EEPROMs to the appropriate sysfs device needed for I/O in a generic
> +way.
> +
> +Also mappings are further complicated by some systems using custom i2c
> +buses implemented in FPGAs.
> +
> +The solution is two fold:
> +
> +1. Create an EEPROM class for all EEPROM devices.  Each EEPROM driver,
> +at24 for example, would register with the class during probe().
> +
> +2. Create a mapping in the .dts file by adding a property called
> +'label' to each EEPROM entry.  The EEPROM class will expose this label
> +property for all EEPROMs.
> +
> +For example, for all the EEPROM devices in the system you would see
> +directories in sysfs like:
> +
> +  /sys/class/eeprom_dev/eeprom0
> +  /sys/class/eeprom_dev/eeprom1
> +  /sys/class/eeprom_dev/eeprom2
> +  ...
> +  /sys/class/eeprom_dev/eeprom<N>
> +
> +Within each eepromN directory you would find:
> +
> +  root@switch:/sys/class/eeprom_dev# ls -l eeprom2/
> +  total 0
> +  lrwxrwxrwx 1 root root    0 Sep  3 22:08 device -> ../../../1-0050
> +  -r--r--r-- 1 root root 4096 Sep  3 22:08 label
> +  lrwxrwxrwx 1 root root    0 Sep  4 17:18 subsystem ->  ../../../../../../../class/eeprom_dev
> +
> +device -- this is a symlink to the physical device.  For example to
> +dump the EEPROM data of eeprom2 you could do:
> +
> +  hexdump -C /sys/class/eeprom_dev/eeprom2/device/eeprom
> +
> +As an example the device tree entry corresponding to eeprom2 could
> +look like:
> +
> +	sfp_eeprom@50 {
> +		compatible = "at,24c04";
> +		reg = <0x50>;
> +		label = "port6";
> +	};
> +
> +From the original problem, imagine 64 similar entries for all the
> +other ports.  Plus a few more entries for board EEPROM and power
> +supply EEPROMs.
> +
> +From user space if I wanted to know the device corresponding to port6
> +I could do something as simple as:
> +
> +root@switch:~# grep port6 /sys/class/eeprom_dev/eeprom*/label
> +/sys/class/eeprom_dev/eeprom2/label:port6
> +
> +Then I could access the information via
> +/sys/class/eeprom_dev/eeprom2/device/eeprom.
> +
> +It is nice that it keeps the mapping all in one place, in the .dts
> +file.  It is not spread around in the device tree and some other
> +platform specific configuration file.
> +
> +Note: For devices without a 'label' property the label file is still
> +created, however, its contents would be set to 'unknown'.
> +
> +Note2: The class cannot be called 'eeprom' as that is the name of the
> +I/O file created by the driver.  The class name appears as a
> +sub-directory within the main device directory.  Hence the class name
> +'eeprom_dev'.
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index 9536852f..be6b727 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -1,7 +1,18 @@
>  menu "EEPROM support"
>  
> +config EEPROM_CLASS
> +	tristate "EEPROM Hardware Class support"
> +	depends on SYSFS
> +	default y
> +	help
> +	  Creates a hardware class in sysfs called "eeprom_dev",
> +	  providing a common place to register EEPROM devices.
> +
> +	  This support can also be built as a module.  If so, the module
> +	  will be called eeprom_class.
> +
>  config EEPROM_AT24
>  	tristate "I2C EEPROMs / RAMs / ROMs from most vendors"
>  	depends on I2C && SYSFS
>  	help
>  	  Enable this driver to get read/write support to most I2C EEPROMs
> diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
> index 9507aec..d2369ca 100644
> --- a/drivers/misc/eeprom/Makefile
> +++ b/drivers/misc/eeprom/Makefile
> @@ -1,5 +1,6 @@
> +obj-$(CONFIG_EEPROM_CLASS)	+= eeprom_class.o
>  obj-$(CONFIG_EEPROM_AT24)	+= at24.o
>  obj-$(CONFIG_EEPROM_AT25)	+= at25.o
>  obj-$(CONFIG_EEPROM_LEGACY)	+= eeprom.o
>  obj-$(CONFIG_EEPROM_MAX6875)	+= max6875.o
>  obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
> diff --git a/drivers/misc/eeprom/eeprom_class.c b/drivers/misc/eeprom/eeprom_class.c
> new file mode 100644
> index 0000000..c934ba6e
> --- /dev/null
> +++ b/drivers/misc/eeprom/eeprom_class.c
> @@ -0,0 +1,159 @@
> +/*
> + * eeprom_class.c
> + *
> + * This file defines the sysfs class "eeprom", for use by EEPROM
> + * drivers.
> + *
> + * Copyright (C) 2013 Cumulus Networks, Inc.
> + * Author: Curt Brune <curt-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
> + *
> + * Ideas and structure graciously borrowed from the hwmon class:
> + * Copyright (C) 2005 Mark M. Hoffman <mhoffman-xQSgfq/1h4JiLUuM0BA3LQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kdev_t.h>
> +#include <linux/idr.h>
> +#include <linux/eeprom_class.h>
> +#include <linux/gfp.h>
> +#include <linux/spinlock.h>
> +#include <linux/pci.h>
> +
> +/* Root eeprom "class" object (corresponds to '/<sysfs>/class/eeprom_dev/') */
> +static struct class *eeprom_class;
> +
> +#define EEPROM_CLASS_NAME "eeprom_dev"
> +#define EEPROM_ID_PREFIX "eeprom"
> +#define EEPROM_ID_FORMAT EEPROM_ID_PREFIX "%d"
> +
> +static DEFINE_IDA(eeprom_ida);
> +
> +/**
> + * eeprom_device_register - register w/ eeprom class
> + * @dev: the device to register
> + *
> + * eeprom_device_unregister() must be called when the device is no
> + * longer needed.
> + *
> + * Creates a new eeprom class device that is a child of @dev.  Also
> + * creates a symlink in /<sysfs>/class/eeprom_dev/eeprom[N] pointing
> + * to the new device.
> + *
> + * Returns the pointer to the new device.
> + */
> +struct device *eeprom_device_register(struct device *dev)
> +{
> +	struct device *eeprom_dev;
> +	int id;
> +
> +	id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0)
> +		return ERR_PTR(id);
> +
> +	eeprom_dev = device_create(eeprom_class, dev, MKDEV(0, 0), NULL,
> +				   EEPROM_ID_FORMAT, id);
> +
> +	if (IS_ERR(eeprom_dev))
> +		ida_simple_remove(&eeprom_ida, id);
> +
> +	return eeprom_dev;
> +}
> +
> +/**
> + * eeprom_device_unregister - removes the previously registered class device
> + *
> + * @dev: the class device to destroy
> + */
> +void eeprom_device_unregister(struct device *dev)
> +{
> +	int id;
> +
> +	if (likely(sscanf(dev_name(dev), EEPROM_ID_FORMAT, &id) == 1)) {
> +		device_unregister(dev);
> +		ida_simple_remove(&eeprom_ida, id);
> +	} else
> +		dev_dbg(dev->parent,
> +			"eeprom_device_unregister() failed: bad class ID!\n");
> +}
> +
> +/**
> + * Each member of the eeprom class exports a sysfs file called
> + * "label", containing the label property from the corresponding
> + * device tree node.
> + *
> + *  Userspace can use the label to identify what the EEPROM is for.
> + */
> +static ssize_t label_show(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	const char* cp = NULL;
> +	int len = 0;
> +
> +	/*
> +	 * The class device is a child of the original device,
> +	 * i.e. dev->parent points to the original device.
> +	 */
> +	if (dev->parent && dev->parent->of_node)
> +		cp = of_get_property(dev->parent->of_node, "label", &len);
> +
> +	if ((cp == NULL) || (len == 0)) {
> +		cp = "unknown";
> +		len = strlen(cp) + 1;
> +	}
> +
> +	strncpy(buf, cp, len - 1);
> +	buf[len - 1] = '\n';
> +	buf[len] = '\0';
> +
> +	return len;
> +}
> +
> +struct device_attribute eeprom_class_dev_attrs[] = {
> +	__ATTR_RO(label),
> +	__ATTR_NULL,
> +};
> +
> +static int __init eeprom_init(void)
> +{
> +	eeprom_class = class_create(THIS_MODULE, EEPROM_CLASS_NAME);
> +	if (IS_ERR(eeprom_class)) {
> +		pr_err("couldn't create sysfs class\n");
> +		return PTR_ERR(eeprom_class);
> +	}
> +
> +	eeprom_class->dev_attrs = eeprom_class_dev_attrs;
> +
> +	return 0;
> +}
> +
> +static void __exit eeprom_exit(void)
> +{
> +	class_destroy(eeprom_class);
> +}
> +
> +subsys_initcall(eeprom_init);
> +module_exit(eeprom_exit);
> +
> +EXPORT_SYMBOL_GPL(eeprom_device_register);
> +EXPORT_SYMBOL_GPL(eeprom_device_unregister);
> +
> +MODULE_AUTHOR("Curt Brune <curt-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>");
> +MODULE_DESCRIPTION("eeprom sysfs/class support");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/eeprom_class.h b/include/linux/eeprom_class.h
> new file mode 100644
> index 0000000..389ac16
> --- /dev/null
> +++ b/include/linux/eeprom_class.h
> @@ -0,0 +1,35 @@
> +/*
> + * eeprom_class.c
> + *
> + * This file exports interface functions for the sysfs class "eeprom",
> + * for use by EEPROM drivers.
> + *
> + * Copyright (C) 2013 Cumulus Networks, Inc.
> + * Author: Curt Brune <curt-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + */
> +
> +#ifndef EEPROM_CLASS_H__
> +#define EEPROM_CLASS_H__
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +
> +struct device *eeprom_device_register(struct device *dev);
> +
> +void eeprom_device_unregister(struct device *dev);
> +
> +#endif /* EEPROM_CLASS_H__ */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laszlo Papp Feb. 11, 2014, 1:03 p.m. UTC | #5
On Sat, Jan 25, 2014 at 12:23 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On 01/23/2014 11:16 AM, Curt Brune wrote:
>> Create a new hardware class under /sys/class/eeprom_dev
>>
>> EEPROM drivers can register their devices with the eeprom_dev class
>> during instantiation.
>>
>> The registered devices show up as:
>>
>>   /sys/class/eeprom_dev/eeprom0
>>   /sys/class/eeprom_dev/eeprom1
>>   ...
>>   /sys/class/eeprom_dev/eeprom[N]
>>
>> Each member of the eeprom class exports a sysfs file called "label",
>> containing the label property from the corresponding device tree node.
>>
>> Example:
>>
>>   /sys/class/eeprom_dev/eeprom0/label
>>
>> If the device tree node property "label" does not exist the value
>> "unknown" is used.
>>
>> Note: The class cannot be called 'eeprom' as that is the name of the
>> I/O file created by the driver.  The class name appears as a
>> sub-directory within the main device directory.  Hence the class name
>> 'eeprom_dev'.
>>
>> Userspace can use the label to identify what the EEPROM is for.
>
> Since my previous email [1] seems to have vanished into the void, I'll
> try again more succinctly:
>
> How will this work on non device tree / openfirmware systems?
>
> Is there a better way to expose topology information (i.e. that the
> eeprom belongs to another device that might not live on the i2c bus at all)?
>
> Can we expose type information?  There's a big difference between SPD
> EEPROMs, EDID EEPROMs, and nic mac-address-containing EEPROMs, for example.

I personally do not see major issues for improving that, but it might
be just me.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard April 7, 2014, 9:09 a.m. UTC | #6
Hi,

On Thu, Jan 23, 2014 at 11:16:01AM -0800, Curt Brune wrote:
> Create a new hardware class under /sys/class/eeprom_dev
> 
> EEPROM drivers can register their devices with the eeprom_dev class
> during instantiation.
> 
> The registered devices show up as:
> 
>   /sys/class/eeprom_dev/eeprom0
>   /sys/class/eeprom_dev/eeprom1
>   ...
>   /sys/class/eeprom_dev/eeprom[N]
> 
> Each member of the eeprom class exports a sysfs file called "label",
> containing the label property from the corresponding device tree node.
> 
> Example:
> 
>   /sys/class/eeprom_dev/eeprom0/label
> 
> If the device tree node property "label" does not exist the value
> "unknown" is used.
> 
> Note: The class cannot be called 'eeprom' as that is the name of the
> I/O file created by the driver.  The class name appears as a
> sub-directory within the main device directory.  Hence the class name
> 'eeprom_dev'.
> 
> Userspace can use the label to identify what the EEPROM is for.
> 
> The real device is available from the class device via the "device"
> link:
> 
>   /sys/class/eeprom_dev/eeprom0/device

Sorry for stepping up this late, but I've been working on an eeprom
framework recently to move all the duplication that exists in the
eeprom drivers (such as sysfs file creation, in kernel accessors) to
common code. And part of that work was to create a eeprom class.

The only feature that's missing from your code is the label, but I
guess it can easily be added, and makes sense.

I'm not quite fond of some of the API yet, particularly on the in
kernel side, but I guess I can post what I have so far as an RFC and
get the discussion started.

Maxime
diff mbox

Patch

diff --git a/Documentation/misc-devices/eeprom_hw_class.txt b/Documentation/misc-devices/eeprom_hw_class.txt
new file mode 100644
index 0000000..b5cbc35
--- /dev/null
+++ b/Documentation/misc-devices/eeprom_hw_class.txt
@@ -0,0 +1,81 @@ 
+EEPROM Device Hardware Class
+============================
+
+This feature is enabled by CONFIG_EEPROM_CLASS.
+
+The original problem:
+
+We work on several different switching platforms, each of which has
+about 64 EEPROMs, one for each of the 10G SFP+ modules.  In addition
+the systems typically have a board info EEPROM, SPD and power supply
+EEPROMs.  It is difficult to map the device tree entries for the
+EEPROMs to the appropriate sysfs device needed for I/O in a generic
+way.
+
+Also mappings are further complicated by some systems using custom i2c
+buses implemented in FPGAs.
+
+The solution is two fold:
+
+1. Create an EEPROM class for all EEPROM devices.  Each EEPROM driver,
+at24 for example, would register with the class during probe().
+
+2. Create a mapping in the .dts file by adding a property called
+'label' to each EEPROM entry.  The EEPROM class will expose this label
+property for all EEPROMs.
+
+For example, for all the EEPROM devices in the system you would see
+directories in sysfs like:
+
+  /sys/class/eeprom_dev/eeprom0
+  /sys/class/eeprom_dev/eeprom1
+  /sys/class/eeprom_dev/eeprom2
+  ...
+  /sys/class/eeprom_dev/eeprom<N>
+
+Within each eepromN directory you would find:
+
+  root@switch:/sys/class/eeprom_dev# ls -l eeprom2/
+  total 0
+  lrwxrwxrwx 1 root root    0 Sep  3 22:08 device -> ../../../1-0050
+  -r--r--r-- 1 root root 4096 Sep  3 22:08 label
+  lrwxrwxrwx 1 root root    0 Sep  4 17:18 subsystem ->  ../../../../../../../class/eeprom_dev
+
+device -- this is a symlink to the physical device.  For example to
+dump the EEPROM data of eeprom2 you could do:
+
+  hexdump -C /sys/class/eeprom_dev/eeprom2/device/eeprom
+
+As an example the device tree entry corresponding to eeprom2 could
+look like:
+
+	sfp_eeprom@50 {
+		compatible = "at,24c04";
+		reg = <0x50>;
+		label = "port6";
+	};
+
+From the original problem, imagine 64 similar entries for all the
+other ports.  Plus a few more entries for board EEPROM and power
+supply EEPROMs.
+
+From user space if I wanted to know the device corresponding to port6
+I could do something as simple as:
+
+root@switch:~# grep port6 /sys/class/eeprom_dev/eeprom*/label
+/sys/class/eeprom_dev/eeprom2/label:port6
+
+Then I could access the information via
+/sys/class/eeprom_dev/eeprom2/device/eeprom.
+
+It is nice that it keeps the mapping all in one place, in the .dts
+file.  It is not spread around in the device tree and some other
+platform specific configuration file.
+
+Note: For devices without a 'label' property the label file is still
+created, however, its contents would be set to 'unknown'.
+
+Note2: The class cannot be called 'eeprom' as that is the name of the
+I/O file created by the driver.  The class name appears as a
+sub-directory within the main device directory.  Hence the class name
+'eeprom_dev'.
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 9536852f..be6b727 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -1,7 +1,18 @@ 
 menu "EEPROM support"
 
+config EEPROM_CLASS
+	tristate "EEPROM Hardware Class support"
+	depends on SYSFS
+	default y
+	help
+	  Creates a hardware class in sysfs called "eeprom_dev",
+	  providing a common place to register EEPROM devices.
+
+	  This support can also be built as a module.  If so, the module
+	  will be called eeprom_class.
+
 config EEPROM_AT24
 	tristate "I2C EEPROMs / RAMs / ROMs from most vendors"
 	depends on I2C && SYSFS
 	help
 	  Enable this driver to get read/write support to most I2C EEPROMs
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index 9507aec..d2369ca 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -1,5 +1,6 @@ 
+obj-$(CONFIG_EEPROM_CLASS)	+= eeprom_class.o
 obj-$(CONFIG_EEPROM_AT24)	+= at24.o
 obj-$(CONFIG_EEPROM_AT25)	+= at25.o
 obj-$(CONFIG_EEPROM_LEGACY)	+= eeprom.o
 obj-$(CONFIG_EEPROM_MAX6875)	+= max6875.o
 obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
diff --git a/drivers/misc/eeprom/eeprom_class.c b/drivers/misc/eeprom/eeprom_class.c
new file mode 100644
index 0000000..c934ba6e
--- /dev/null
+++ b/drivers/misc/eeprom/eeprom_class.c
@@ -0,0 +1,159 @@ 
+/*
+ * eeprom_class.c
+ *
+ * This file defines the sysfs class "eeprom", for use by EEPROM
+ * drivers.
+ *
+ * Copyright (C) 2013 Cumulus Networks, Inc.
+ * Author: Curt Brune <curt@cumulusnetworks.com>
+ *
+ * Ideas and structure graciously borrowed from the hwmon class:
+ * Copyright (C) 2005 Mark M. Hoffman <mhoffman@lightlink.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kdev_t.h>
+#include <linux/idr.h>
+#include <linux/eeprom_class.h>
+#include <linux/gfp.h>
+#include <linux/spinlock.h>
+#include <linux/pci.h>
+
+/* Root eeprom "class" object (corresponds to '/<sysfs>/class/eeprom_dev/') */
+static struct class *eeprom_class;
+
+#define EEPROM_CLASS_NAME "eeprom_dev"
+#define EEPROM_ID_PREFIX "eeprom"
+#define EEPROM_ID_FORMAT EEPROM_ID_PREFIX "%d"
+
+static DEFINE_IDA(eeprom_ida);
+
+/**
+ * eeprom_device_register - register w/ eeprom class
+ * @dev: the device to register
+ *
+ * eeprom_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Creates a new eeprom class device that is a child of @dev.  Also
+ * creates a symlink in /<sysfs>/class/eeprom_dev/eeprom[N] pointing
+ * to the new device.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *eeprom_device_register(struct device *dev)
+{
+	struct device *eeprom_dev;
+	int id;
+
+	id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL);
+	if (id < 0)
+		return ERR_PTR(id);
+
+	eeprom_dev = device_create(eeprom_class, dev, MKDEV(0, 0), NULL,
+				   EEPROM_ID_FORMAT, id);
+
+	if (IS_ERR(eeprom_dev))
+		ida_simple_remove(&eeprom_ida, id);
+
+	return eeprom_dev;
+}
+
+/**
+ * eeprom_device_unregister - removes the previously registered class device
+ *
+ * @dev: the class device to destroy
+ */
+void eeprom_device_unregister(struct device *dev)
+{
+	int id;
+
+	if (likely(sscanf(dev_name(dev), EEPROM_ID_FORMAT, &id) == 1)) {
+		device_unregister(dev);
+		ida_simple_remove(&eeprom_ida, id);
+	} else
+		dev_dbg(dev->parent,
+			"eeprom_device_unregister() failed: bad class ID!\n");
+}
+
+/**
+ * Each member of the eeprom class exports a sysfs file called
+ * "label", containing the label property from the corresponding
+ * device tree node.
+ *
+ *  Userspace can use the label to identify what the EEPROM is for.
+ */
+static ssize_t label_show(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	const char* cp = NULL;
+	int len = 0;
+
+	/*
+	 * The class device is a child of the original device,
+	 * i.e. dev->parent points to the original device.
+	 */
+	if (dev->parent && dev->parent->of_node)
+		cp = of_get_property(dev->parent->of_node, "label", &len);
+
+	if ((cp == NULL) || (len == 0)) {
+		cp = "unknown";
+		len = strlen(cp) + 1;
+	}
+
+	strncpy(buf, cp, len - 1);
+	buf[len - 1] = '\n';
+	buf[len] = '\0';
+
+	return len;
+}
+
+struct device_attribute eeprom_class_dev_attrs[] = {
+	__ATTR_RO(label),
+	__ATTR_NULL,
+};
+
+static int __init eeprom_init(void)
+{
+	eeprom_class = class_create(THIS_MODULE, EEPROM_CLASS_NAME);
+	if (IS_ERR(eeprom_class)) {
+		pr_err("couldn't create sysfs class\n");
+		return PTR_ERR(eeprom_class);
+	}
+
+	eeprom_class->dev_attrs = eeprom_class_dev_attrs;
+
+	return 0;
+}
+
+static void __exit eeprom_exit(void)
+{
+	class_destroy(eeprom_class);
+}
+
+subsys_initcall(eeprom_init);
+module_exit(eeprom_exit);
+
+EXPORT_SYMBOL_GPL(eeprom_device_register);
+EXPORT_SYMBOL_GPL(eeprom_device_unregister);
+
+MODULE_AUTHOR("Curt Brune <curt@cumulusnetworks.com>");
+MODULE_DESCRIPTION("eeprom sysfs/class support");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/eeprom_class.h b/include/linux/eeprom_class.h
new file mode 100644
index 0000000..389ac16
--- /dev/null
+++ b/include/linux/eeprom_class.h
@@ -0,0 +1,35 @@ 
+/*
+ * eeprom_class.c
+ *
+ * This file exports interface functions for the sysfs class "eeprom",
+ * for use by EEPROM drivers.
+ *
+ * Copyright (C) 2013 Cumulus Networks, Inc.
+ * Author: Curt Brune <curt@cumulusnetworks.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#ifndef EEPROM_CLASS_H__
+#define EEPROM_CLASS_H__
+
+#include <linux/device.h>
+#include <linux/err.h>
+
+struct device *eeprom_device_register(struct device *dev);
+
+void eeprom_device_unregister(struct device *dev);
+
+#endif /* EEPROM_CLASS_H__ */