[PATCH 0/2] Introduce MEN Board Information EEPROM driver

[PATCH 0/2] Introduce MEN Board Information EEPROM driver

[Andreas Werner]

This patchset adds support for the MEN Board Information EEPROM.

The EEPROM is assembled on almost all of the MEN hardware boards and is the
main location to get information about the board.

The driver exports the EEPROM data like production date, board name and serial number
as read-only files as well as a user section for read-write access.

There is also a ABI documentation for sysfs description included.

Andreas Werner (2):
  drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM
    driver
  Documentation/ABI/testing/men_eeprod: Added sysfs description for
    men_eeprod

 .../ABI/testing/sysfs-bus-i2c-devices-men_eeprod   |  69 +++
 MAINTAINERS                                        |   6 +
 drivers/misc/eeprom/Kconfig                        |  10 +
 drivers/misc/eeprom/Makefile                       |   1 +
 drivers/misc/eeprom/men_eeprod.c                   | 560 +++++++++++++++++++++
 5 files changed, 646 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod
 create mode 100644 drivers/misc/eeprom/men_eeprod.c

-- 
2.1.0

[PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

Added driver to support the MEN Board Information EEPROM.
The driver exports the production information as read only sysfs
entries, as well as a user section which is read/write accessible.

Tested on PPC QorIQ and Intel Atom E680.

Tested-by: Johannes Thumshirn <johannes.thumshirn@men.de>
Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 MAINTAINERS                      |   6 +
 drivers/misc/eeprom/Kconfig      |  10 +
 drivers/misc/eeprom/Makefile     |   1 +
 drivers/misc/eeprom/men_eeprod.c | 560 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 577 insertions(+)
 create mode 100644 drivers/misc/eeprom/men_eeprod.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 503da28..88ede76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6029,6 +6029,12 @@ F:	drivers/leds/leds-menf21bmc.c
 F:	drivers/hwmon/menf21bmc_hwmon.c
 F:	Documentation/hwmon/menf21bmc
 
+MEN EEPROD (Board information EEPROM)
+M:	Andreas Werner <andreas.werner@men.de>
+S:	Supported
+F:	drivers/misc/eeprom/men_eeprod.c
+F:	Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod
+
 METAG ARCHITECTURE
 M:	James Hogan <james.hogan@imgtec.com>
 L:	linux-metag@vger.kernel.org
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 9536852f..e087d08 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -62,6 +62,16 @@ config EEPROM_MAX6875
 	  This driver can also be built as a module.  If so, the module
 	  will be called max6875.
 
+config EEPROM_MEN_EEPROD
+	tristate "MEN Board Information EEPROM"
+	depends on I2C && SYSFS
+	help
+	  If you say yes here you get support for the MEN Board Information
+	  EEPROM. This driver supports read-only access to the production
+	  data section, and read-write access to the user section.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called men_eeprod.
 
 config EEPROM_93CX6
 	tristate "EEPROM 93CX6 support"
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index 9507aec..8c70a81 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -2,6 +2,7 @@ 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_MEN_EEPROD) += men_eeprod.o
 obj-$(CONFIG_EEPROM_93CX6)	+= eeprom_93cx6.o
 obj-$(CONFIG_EEPROM_93XX46)	+= eeprom_93xx46.o
 obj-$(CONFIG_EEPROM_SUNXI_SID)	+= sunxi_sid.o
diff --git a/drivers/misc/eeprom/men_eeprod.c b/drivers/misc/eeprom/men_eeprod.c
new file mode 100644
index 0000000..28264df
--- /dev/null
+++ b/drivers/misc/eeprom/men_eeprod.c
@@ -0,0 +1,560 @@
+/*
+ *  men_eeprod - MEN board information EEPROM driver.
+ *
+ *  This is the driver for the Board Information EEPROM on almost
+ *  all of the MEN boards.
+ *  The driver exports each of the predefined eeprom sections as sysfs entries
+ *  including an entry for user data.
+ *
+ *  The EEPROM can be normally found at I2C address 0x57.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *
+ *  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;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ */
+
+/*
+ * Supports the following EEPROM layouts:
+ *
+ * Name		eeprod_id	user_section size
+ * ----------------------------------------------
+ * EEPROD2	0x0E		232 byte
+ *
+ * See Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod
+ * for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/jiffies.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+
+#define EEPROM_ID_ADDR	0x00
+#define EEPROM_SIZE	256
+#define EEPROD2_ID	0x0E
+
+#define DATE_YEAR_BIAS	1990
+
+/*
+ * Internal structure of the Board Information EEPROM
+ * production data section
+ */
+struct eeprom_data {
+	uint8_t eeprod_id;
+
+	uint8_t revision[3];
+	uint32_t serialnr;
+	uint8_t board_model;
+	char hw_name[6];
+
+	uint8_t reserved;
+
+	__be16 prod_date;
+	__be16 rep_date;
+
+	uint8_t reserved2[4];
+};
+
+struct men_eeprod_data {
+	struct bin_attribute user_section;
+	struct i2c_client *client;
+	struct eeprom_data eeprom;
+	struct mutex lock;
+	int smb_access;
+};
+
+static unsigned int i2c_timeout = 25;
+module_param(i2c_timeout, uint, 0);
+MODULE_PARM_DESC(i2c_timeout, "Time (in ms) to try reads and writes (default 25)");
+
+#define OFFSET_TO_USR_SECTION(off) (off + sizeof(struct eeprom_data))
+
+static inline int eeprod_get_day(__be16 eeprod_date)
+{
+	return be16_to_cpu(eeprod_date) & 0x001f;
+}
+
+static inline int eeprod_get_month(__be16 eeprod_date)
+{
+	return (be16_to_cpu(eeprod_date) >> 5) & 0x000f;
+}
+
+static inline int eeprod_get_year(__be16 eeprod_date)
+{
+	return ((be16_to_cpu(eeprod_date) >> 9) & 0x007f) + DATE_YEAR_BIAS;
+}
+
+static ssize_t men_eeprom_read(struct men_eeprod_data *drv_data,
+			       char *buf, loff_t offset, size_t count)
+{
+	struct i2c_client *i2c_client = drv_data->client;
+	unsigned long timeout, read_time;
+	int ret_val;
+
+	/*
+	 * Read fail if the previous write did not copmlete yet.
+	 * Therefore we try to read a few times until this succeed.
+	 */
+	timeout = jiffies + msecs_to_jiffies(i2c_timeout);
+	do {
+		read_time = jiffies;
+
+		/*
+		 * if there is just one byte requested, we use read byte data.
+		 * This will also protect us against a rollover if there is
+		 * just one byte left to read.
+		 */
+		if (count == 1) {
+			ret_val = i2c_smbus_read_byte_data(i2c_client, offset);
+			if (ret_val >= 0) {
+				buf[0] = ret_val;
+				ret_val = 1;
+			}
+			goto err_byte;
+		}
+
+		switch (drv_data->smb_access) {
+		case I2C_SMBUS_I2C_BLOCK_DATA:
+			if (count > I2C_SMBUS_BLOCK_MAX)
+				count = I2C_SMBUS_BLOCK_MAX;
+
+			ret_val = i2c_smbus_read_i2c_block_data(i2c_client,
+								offset, count,
+								buf);
+			if (ret_val >= 0)
+				return count;
+			break;
+		case I2C_SMBUS_WORD_DATA:
+			ret_val = i2c_smbus_read_word_data(i2c_client, offset);
+			if (ret_val >= 0) {
+				buf[0] = ret_val & 0xff;
+				buf[1] = ret_val >> 8;
+				return 2;
+			}
+			break;
+		default:
+			ret_val = i2c_smbus_read_byte_data(i2c_client, offset);
+			if (ret_val >= 0) {
+				buf[0] = ret_val;
+				return 1;
+			}
+			break;
+		}
+
+err_byte:
+		usleep_range(600, 1000);
+	} while (time_before(read_time, timeout));
+
+	return -ETIMEDOUT;
+
+}
+
+static ssize_t men_user_section_read(struct file *filp, struct kobject *kobj,
+				     struct bin_attribute *attr,
+				     char *buf, loff_t off, size_t count)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	int user_section_size = attr->size;
+	ssize_t retval = 0;
+	int bytes_read;
+
+	if (off > user_section_size)
+		return 0;
+
+	if (off + count > user_section_size)
+		count = user_section_size - off;
+
+	off = OFFSET_TO_USR_SECTION(off);
+
+	mutex_lock(&drv_data->lock);
+	while (count) {
+		bytes_read = men_eeprom_read(drv_data, buf, off, count);
+
+		if (bytes_read <= 0) {
+			if (retval == 0)
+				retval = bytes_read;
+			break;
+		}
+
+		buf += bytes_read;
+		off += bytes_read;
+		count -= bytes_read;
+		retval += bytes_read;
+	}
+	mutex_unlock(&drv_data->lock);
+
+	return retval;
+}
+
+static ssize_t men_eeprom_write(struct men_eeprod_data *drv_data,
+				char *buf, loff_t offset, size_t count)
+{
+	struct i2c_client *i2c_client = drv_data->client;
+	unsigned long timeout, write_time;
+	uint16_t word_data;
+	int ret_val;
+
+	/*
+	 * Write fail if the previous write did not copmlete yet.
+	 * Therefore we try to write a few times until this succeed.
+	 */
+	timeout = jiffies + msecs_to_jiffies(i2c_timeout);
+	do {
+		write_time = jiffies;
+
+		/*
+		 * if there is just one byte to write, we use write byte data.
+		 * This will also protect us against a rollover if there is
+		 * just one byte left to write.
+		 */
+		if (count == 1) {
+			ret_val =  i2c_smbus_write_byte_data(i2c_client, offset,
+							     buf[0]);
+			if (!ret_val)
+				return 1;
+			goto err_byte;
+		}
+
+		switch (drv_data->smb_access) {
+		case I2C_SMBUS_I2C_BLOCK_DATA:
+			if (count > I2C_SMBUS_BLOCK_MAX)
+				count = I2C_SMBUS_BLOCK_MAX;
+
+			ret_val = i2c_smbus_write_i2c_block_data(i2c_client,
+								 offset, count,
+								 buf);
+			if (!ret_val)
+				return count;
+			break;
+		case I2C_SMBUS_WORD_DATA:
+			word_data = buf[0] | (buf[1] << 8);
+
+			ret_val = i2c_smbus_write_word_data(i2c_client, offset,
+							    word_data);
+			if (!ret_val)
+				return 2;
+			break;
+		default:
+			ret_val = i2c_smbus_write_byte_data(i2c_client, offset,
+							    buf[0]);
+			if (!ret_val)
+				return 1;
+			break;
+		}
+err_byte:
+		usleep_range(600, 1000);
+	} while (time_before(write_time, timeout));
+
+	return -ETIMEDOUT;
+}
+
+static ssize_t men_user_section_write(struct file *filp, struct kobject *kobj,
+				      struct bin_attribute *attr, char *buf,
+				      loff_t off, size_t count)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	int user_section_size = attr->size;
+	int bytes_written;
+	ssize_t retval = 0;
+
+	if (off > user_section_size)
+		return 0;
+
+	if (off + count > user_section_size)
+		count = user_section_size - off;
+
+	off = OFFSET_TO_USR_SECTION(off);
+
+	mutex_lock(&drv_data->lock);
+	while (count) {
+		bytes_written = men_eeprom_write(drv_data, buf, off, count);
+
+		if (bytes_written <= 0) {
+			if (retval == 0)
+				retval = bytes_written;
+			break;
+		}
+
+		buf += bytes_written;
+		off += bytes_written;
+		count -= bytes_written;
+		retval += bytes_written;
+	}
+	mutex_unlock(&drv_data->lock);
+
+	return retval;
+}
+
+static ssize_t
+show_eeprod_id(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	struct eeprom_data *eeprom = &drv_data->eeprom;
+
+	return sprintf(buf, "0x%02x\n", eeprom->eeprod_id);
+}
+
+static ssize_t
+show_revision(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	struct eeprom_data *eeprom = &drv_data->eeprom;
+
+	return sprintf(buf, "%02d.%02d.%02d\n", eeprom->revision[0],
+		       eeprom->revision[1], eeprom->revision[2]);
+}
+
+static ssize_t
+show_serialnr(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	struct eeprom_data *eeprom = &drv_data->eeprom;
+
+	return sprintf(buf, "%d\n", cpu_to_be32(eeprom->serialnr));
+}
+
+static ssize_t
+show_hw_name(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	struct eeprom_data *eeprom = &drv_data->eeprom;
+
+	return sprintf(buf, "%s%02d\n", eeprom->hw_name, eeprom->board_model);
+}
+
+static ssize_t
+show_prod_date(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	__be16 eeprod_date = drv_data->eeprom.prod_date;
+
+	return sprintf(buf, "%04d-%02d-%02d\n",
+		       eeprod_get_year(eeprod_date),
+		       eeprod_get_month(eeprod_date),
+		       eeprod_get_day(eeprod_date));
+}
+
+static ssize_t
+show_rep_date(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct men_eeprod_data *drv_data = dev_get_drvdata(dev);
+	__be16 eeprod_date = drv_data->eeprom.rep_date;
+
+	/*
+	 * could be empty if the board was never send back
+	 * to the repair department.
+	 */
+	if (eeprod_date == cpu_to_be16(0xffff))
+		return -EINVAL;
+
+	return sprintf(buf, "%04d-%02d-%02d\n",
+		       eeprod_get_year(eeprod_date),
+		       eeprod_get_month(eeprod_date),
+		       eeprod_get_day(eeprod_date));
+}
+
+static DEVICE_ATTR(eeprod_id, S_IRUGO, show_eeprod_id, NULL);
+static DEVICE_ATTR(revision, S_IRUGO, show_revision, NULL);
+static DEVICE_ATTR(serial, S_IRUGO, show_serialnr, NULL);
+static DEVICE_ATTR(hw_name, S_IRUGO, show_hw_name, NULL);
+static DEVICE_ATTR(prod_date, S_IRUGO, show_prod_date, NULL);
+static DEVICE_ATTR(rep_date, S_IRUGO, show_rep_date, NULL);
+
+static struct attribute *eeprod_attrs[] = {
+	&dev_attr_eeprod_id.attr,
+	&dev_attr_revision.attr,
+	&dev_attr_serial.attr,
+	&dev_attr_hw_name.attr,
+	&dev_attr_prod_date.attr,
+	&dev_attr_rep_date.attr,
+	NULL,
+};
+
+static struct attribute_group eeprod_attr_group = {
+	.attrs = eeprod_attrs,
+};
+
+static struct bin_attribute eeprom_user_attr = {
+	.attr = {
+		.name = "user_section",
+		.mode = S_IRUGO | S_IWUSR,
+	},
+	.size = EEPROM_SIZE - sizeof(struct eeprom_data),
+	.read = men_user_section_read,
+	.write = men_user_section_write,
+};
+
+static int men_eeprod_read_prod_data(struct men_eeprod_data *drv_data)
+{
+	struct eeprom_data *eeprom = &drv_data->eeprom;
+	uint8_t *eeprom_byte;
+	int i, ret;
+
+	eeprom_byte = (uint8_t *)eeprom + 1;
+
+	for (i = 1; i < sizeof(*eeprom); i++) {
+		ret = i2c_smbus_read_byte_data(drv_data->client, i);
+		if (ret < 0)
+			return ret;
+
+		*(eeprom_byte++) = ret;
+	}
+	return 0;
+}
+
+static int men_eeprod_calc_parity(struct eeprom_data *eeprom)
+{
+	uint8_t *eeprom_byte;
+	int parity, len;
+
+	eeprom_byte = (uint8_t *)eeprom + 1;
+	len = sizeof(*eeprom) - 1;
+	parity = 0x0f;
+
+	while (len--) {
+		parity ^= (*eeprom_byte >> 4);
+		parity ^= (*eeprom_byte) & 0x0f;
+		eeprom_byte++;
+	}
+
+	return parity;
+}
+
+static int men_eeprod_i2c_functionality(struct men_eeprod_data *drv_data)
+{
+	struct i2c_adapter *i2c_adapter = drv_data->client->adapter;
+	int ret;
+
+	/*
+	 * As the minimum we need read/write byte data
+	 * which every adapter should support
+	 */
+	ret = i2c_check_functionality(i2c_adapter,
+				      I2C_FUNC_SMBUS_BYTE_DATA);
+	if (!ret)
+		return -ENODEV;
+
+	if (i2c_check_functionality(i2c_adapter,
+				    I2C_FUNC_SMBUS_I2C_BLOCK)) {
+		drv_data->smb_access = I2C_SMBUS_I2C_BLOCK_DATA;
+	} else if (i2c_check_functionality(i2c_adapter,
+					   I2C_FUNC_SMBUS_WORD_DATA)) {
+		drv_data->smb_access = I2C_SMBUS_WORD_DATA;
+	} else {
+		drv_data->smb_access = I2C_SMBUS_BYTE_DATA;
+	}
+
+	return 0;
+}
+
+static int
+men_eeprod_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct men_eeprod_data *drv_data;
+	int eeprod_id, eeprod_chksum;
+	int parity, ret;
+
+	drv_data = devm_kzalloc(&client->dev, sizeof(*drv_data), GFP_KERNEL);
+	if (!drv_data)
+		return -ENOMEM;
+
+	drv_data->client = client;
+	mutex_init(&drv_data->lock);
+
+	i2c_set_clientdata(client, drv_data);
+
+	ret = men_eeprod_i2c_functionality(drv_data);
+	if (ret)
+		return ret;
+
+	eeprod_id = i2c_smbus_read_byte_data(client, EEPROM_ID_ADDR);
+	if (eeprod_id < 0)
+		return eeprod_id;
+
+	eeprod_chksum = eeprod_id & 0x0f;
+	eeprod_id >>= 4;
+	drv_data->eeprom.eeprod_id = eeprod_id;
+
+	if (eeprod_id == EEPROD2_ID) {
+		dev_info(&client->dev,
+			 "found MEN Board EEPROM. ID: 0x%02x\n", eeprod_id);
+	} else {
+		dev_err(&client->dev,
+			"board eeprom not supported. ID: 0x%02x\n", eeprod_id);
+		return -ENXIO;
+	}
+
+	ret = men_eeprod_read_prod_data(drv_data);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read EEPROM board data\n");
+		return ret;
+	}
+
+	parity = men_eeprod_calc_parity(&drv_data->eeprom);
+	if (parity != eeprod_chksum) {
+		dev_err(&client->dev, "checksum error. eeprom in invalid state\n");
+		return -EINVAL;
+	}
+
+	drv_data->user_section = eeprom_user_attr;
+	ret = sysfs_create_bin_file(&client->dev.kobj,
+				    &drv_data->user_section);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed to create user_section sysfs entry\n");
+		return ret;
+	}
+
+	ret = sysfs_create_group(&client->dev.kobj, &eeprod_attr_group);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to create sysfs entries\n");
+		goto err_sysfs;
+	}
+
+	dev_info(&client->dev, "MEN Board Information EEPROM registered\n");
+
+	return 0;
+
+err_sysfs:
+	sysfs_remove_bin_file(&client->dev.kobj, &drv_data->user_section);
+	return ret;
+}
+
+static int men_eeprod_remove(struct i2c_client *client)
+{
+	struct men_eeprod_data *drv_data = i2c_get_clientdata(client);
+
+	sysfs_remove_group(&client->dev.kobj, &eeprod_attr_group);
+	sysfs_remove_bin_file(&client->dev.kobj, &drv_data->user_section);
+	return 0;
+}
+
+static const struct i2c_device_id men_eeprod_ids[] = {
+	{ "men_eeprod" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, men_eeprod_ids);
+
+static struct i2c_driver men_eeprod_driver = {
+	.driver = {
+		.name = "men_eeprod",
+		.owner = THIS_MODULE,
+	},
+	.probe = men_eeprod_probe,
+	.remove = men_eeprod_remove,
+	.id_table = men_eeprod_ids,
+};
+
+module_i2c_driver(men_eeprod_driver);
+
+MODULE_DESCRIPTION("MEN Board Information EEPROM driver");
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Greg KH]

On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote:
> +struct eeprom_data {
> +	uint8_t eeprod_id;

Please use the "real" kernel types, "u8" here, and "u32" in other places
you use uint32_t (those are userspace types, not kernel types, sorry.)

> +static DEVICE_ATTR(eeprod_id, S_IRUGO, show_eeprod_id, NULL);
> +static DEVICE_ATTR(revision, S_IRUGO, show_revision, NULL);
> +static DEVICE_ATTR(serial, S_IRUGO, show_serialnr, NULL);
> +static DEVICE_ATTR(hw_name, S_IRUGO, show_hw_name, NULL);
> +static DEVICE_ATTR(prod_date, S_IRUGO, show_prod_date, NULL);
> +static DEVICE_ATTR(rep_date, S_IRUGO, show_rep_date, NULL);

DEVICE_ATTR_RO() please.

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

On Thu, Oct 16, 2014 at 10:44:12AM +0200, Greg KH wrote:
> On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote:
> > +struct eeprom_data {
> > +	uint8_t eeprod_id;
> 
> Please use the "real" kernel types, "u8" here, and "u32" in other places
> you use uint32_t (those are userspace types, not kernel types, sorry.)
> 

You are right i will change it.

> > +static DEVICE_ATTR(eeprod_id, S_IRUGO, show_eeprod_id, NULL);
> > +static DEVICE_ATTR(revision, S_IRUGO, show_revision, NULL);
> > +static DEVICE_ATTR(serial, S_IRUGO, show_serialnr, NULL);
> > +static DEVICE_ATTR(hw_name, S_IRUGO, show_hw_name, NULL);
> > +static DEVICE_ATTR(prod_date, S_IRUGO, show_prod_date, NULL);
> > +static DEVICE_ATTR(rep_date, S_IRUGO, show_rep_date, NULL);
> 
> DEVICE_ATTR_RO() please.

OK no problem, will change it.

Thanks.

Regards
Andy

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote:
> Added driver to support the MEN Board Information EEPROM.
> The driver exports the production information as read only sysfs
> entries, as well as a user section which is read/write accessible.
> 
> Tested on PPC QorIQ and Intel Atom E680.
> 
> Tested-by: Johannes Thumshirn <johannes.thumshirn-csrFAY9JiS4@public.gmane.org>
> Signed-off-by: Andreas Werner <andreas.werner-csrFAY9JiS4@public.gmane.org>

I guess this is just a standard EEPROM with a well defined layout?
Why don't you want to use the standard driver then and parse the thing
in userspace?

Consider how bloated the sysfs-ABI might get if every vendor who uses an
eeprom wants to expose the data this way?

> +struct eeprom_data {
> +	uint8_t eeprod_id;
> +
> +	uint8_t revision[3];
> +	uint32_t serialnr;
> +	uint8_t board_model;
> +	char hw_name[6];
> +
> +	uint8_t reserved;
> +
> +	__be16 prod_date;
> +	__be16 rep_date;
> +
> +	uint8_t reserved2[4];
> +};

And what if the compiler reorders?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Greg KH]

On Thu, Oct 16, 2014 at 10:58:35AM +0200, Wolfram Sang wrote:
> > +struct eeprom_data {
> > +	uint8_t eeprod_id;
> > +
> > +	uint8_t revision[3];
> > +	uint32_t serialnr;
> > +	uint8_t board_model;
> > +	char hw_name[6];
> > +
> > +	uint8_t reserved;
> > +
> > +	__be16 prod_date;
> > +	__be16 rep_date;
> > +
> > +	uint8_t reserved2[4];
> > +};
> 
> And what if the compiler reorders?

It's not allowed to reorder, but it can add padding wherever it wants
to, which if this is a on-device structure, can cause problems.  Use
__packed to prevent that.

thanks,

greg k-h

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Thu, Oct 16, 2014 at 11:34:14AM +0200, Greg KH wrote:
> On Thu, Oct 16, 2014 at 10:58:35AM +0200, Wolfram Sang wrote:
> > > +struct eeprom_data {
> > > +	uint8_t eeprod_id;
> > > +
> > > +	uint8_t revision[3];
> > > +	uint32_t serialnr;
> > > +	uint8_t board_model;
> > > +	char hw_name[6];
> > > +
> > > +	uint8_t reserved;
> > > +
> > > +	__be16 prod_date;
> > > +	__be16 rep_date;
> > > +
> > > +	uint8_t reserved2[4];
> > > +};
> > 
> > And what if the compiler reorders?
> 
> It's not allowed to reorder, but it can add padding wherever it wants
> to, which if this is a on-device structure, can cause problems.  Use
> __packed to prevent that.

Yes, took the wrong word, sorry, I meant __packed.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

On Thu, Oct 16, 2014 at 10:58:35AM +0200, Wolfram Sang wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Oct 16, 2014 at 10:15:08AM +0200, Andreas Werner wrote:
> > Added driver to support the MEN Board Information EEPROM.
> > The driver exports the production information as read only sysfs
> > entries, as well as a user section which is read/write accessible.
> > 
> > Tested on PPC QorIQ and Intel Atom E680.
> > 
> > Tested-by: Johannes Thumshirn <johannes.thumshirn@men.de>
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> 
> I guess this is just a standard EEPROM with a well defined layout?
> Why don't you want to use the standard driver then and parse the thing
> in userspace?
>

Yes it is a standard 256byte eeprom.
I do not want to parse the things in userspace because this EEPROM data
are related to the hardware and i want to give our customer the easiest way
to access the data without installing any tool.

The current state to read the eeprom data is, that customer needs to install a big
environment where the tool is integrated to have access to those kind of simple
data or they have to write their own code.

If our support department get some questions from the customer they always want
to have exact information about the board. Therefore I want to use this driver
and sysfs to get the informations as fast as possible without installing anything.
 
> Consider how bloated the sysfs-ABI might get if every vendor who uses an
> eeprom wants to expose the data this way?
> 

Yes and no. The possible sysfs entries gets bloated if every vendor will do it
like this way, but normally there is just one Board EEPROM on the board, therefore
only one driver gets loaded.

I mean its the same for every i2c device like a temperature sensor, I can also
read it from userspace without any special hwmon driver.

> > +struct eeprom_data {
> > +	uint8_t eeprod_id;
> > +
> > +	uint8_t revision[3];
> > +	uint32_t serialnr;
> > +	uint8_t board_model;
> > +	char hw_name[6];
> > +
> > +	uint8_t reserved;
> > +
> > +	__be16 prod_date;
> > +	__be16 rep_date;
> > +
> > +	uint8_t reserved2[4];
> > +};
> 
> And what if the compiler reorders?

Shit you are right, I will pack it.#

> 
> 
> * Unknown Key
> * 0x14A029B6

Regards
Andy

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]


> I do not want to parse the things in userspace because this EEPROM data
> are related to the hardware and i want to give our customer the easiest way
> to access the data without installing any tool.

I understand that point of view. From an upstream point of view, things
may look different, though.

> The current state to read the eeprom data is, that customer needs to install a big
> environment where the tool is integrated to have access to those kind of simple
> data or they have to write their own code.

i2cget from i2c-tools? You could do a simple shell script to parse the
data. Or do a board specific hook which reads the data and prints it to
the logfiles...

> > Consider how bloated the sysfs-ABI might get if every vendor who uses an
> > eeprom wants to expose the data this way?
> > 
> 
> Yes and no. The possible sysfs entries gets bloated if every vendor will do it
> like this way, but normally there is just one Board EEPROM on the board, therefore
> only one driver gets loaded.

I am not talking about runtime here, I don't care about that. I am
talking about the ABI we create and we have to maintain basically
forever. And with vendor specific configuartion data I have doubts with
that being stable.

> I mean its the same for every i2c device like a temperature sensor, I can also
> read it from userspace without any special hwmon driver.

These is a HUGE difference. If I read tempX_input, I don't need to care
if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
The files you create are for your I2C EEPROM only. Data gets
"reformatted" and access gets hidden, but nothing is abstracted away.
It would be different if we had a generic convention for "serial_id" or
stuff like that. But as configuration data is highly specific I don't
see this coming.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote:
> * PGP Signed by an unknown key
> 
> 
> > I do not want to parse the things in userspace because this EEPROM data
> > are related to the hardware and i want to give our customer the easiest way
> > to access the data without installing any tool.
> 
> I understand that point of view. From an upstream point of view, things
> may look different, though.
> 

I also understand your point of view :-).
Most customers wants just to have a running system without installing anything.
And for me an EEPROM is so simple and should not need a complicated way
to access it.

> > The current state to read the eeprom data is, that customer needs to install a big
> > environment where the tool is integrated to have access to those kind of simple
> > data or they have to write their own code.
> 
> i2cget from i2c-tools? You could do a simple shell script to parse the
> data. Or do a board specific hook which reads the data and prints it to
> the logfiles...
> 

Yes of course there are a lot of possibilities. This was just an example
what we currently use and what was developed years ago.

With a driver like this you can also define read only attributes to prevent customer
to write or modify the data in the production section. With i2ctools you can just
write any data to it you want.

> > > Consider how bloated the sysfs-ABI might get if every vendor who uses an
> > > eeprom wants to expose the data this way?
> > > 
> > 
> > Yes and no. The possible sysfs entries gets bloated if every vendor will do it
> > like this way, but normally there is just one Board EEPROM on the board, therefore
> > only one driver gets loaded.
> 
> I am not talking about runtime here, I don't care about that. I am
> talking about the ABI we create and we have to maintain basically
> forever. And with vendor specific configuartion data I have doubts with
> that being stable.
> 

Ok, but i do not think that we can make a "general" ABI definition for those kind
of devices because every vendor will have its own data in the EEPROM which he want
to have.

> > I mean its the same for every i2c device like a temperature sensor, I can also
> > read it from userspace without any special hwmon driver.
> 
> These is a HUGE difference. If I read tempX_input, I don't need to care
> if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
> The files you create are for your I2C EEPROM only. Data gets
> "reformatted" and access gets hidden, but nothing is abstracted away.
> It would be different if we had a generic convention for "serial_id" or
> stuff like that. But as configuration data is highly specific I don't
> see this coming.
> 

For a standard sysfs interface it is a huge difference yes. At the point
of few from the EEPROM device it is a device like a temp sensor which
could be different from vendor to vendor.

Regards
Andy

> 
> * Unknown Key
> * 0x14A029B6

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]


> Most customers wants just to have a running system without installing anything.
> And for me an EEPROM is so simple and should not need a complicated way
> to access it.

As I pointed out, there are ways to do it other than a seperate driver.

> Yes of course there are a lot of possibilities. This was just an example
> what we currently use and what was developed years ago.

And please notice that this solution you chose is not acceptible for
upstream. We can't have n copies of the at24 driver with just some minor
differences. This is a maintenance nightmare.

Yes, I know it was easiest to start with and it works, but that does not
help here.

> With a driver like this you can also define read only attributes to prevent customer
> to write or modify the data in the production section. With i2ctools you can just
> write any data to it you want.

i2cget won't modify any data. i2cset does, if anyone wants to do that.
BTW it does that also after you removed your driver, so basically no big
difference here.

> > I am not talking about runtime here, I don't care about that. I am
> > talking about the ABI we create and we have to maintain basically
> > forever. And with vendor specific configuartion data I have doubts with
> > that being stable.
> > 
> 
> Ok, but i do not think that we can make a "general" ABI definition for those kind
> of devices because every vendor will have its own data in the EEPROM which he want
> to have.

Exactly, that was what I was saying.

What we probably should have in at24 is regions which should not be
exposed to userspace, because they contain config data. That would be
nice.

I'm not sure if we can add table based decoding to at24, that needs some
experiments. But it will really need such experiments and some more
effort.

> > These is a HUGE difference. If I read tempX_input, I don't need to care
> > if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
> > The files you create are for your I2C EEPROM only. Data gets
> > "reformatted" and access gets hidden, but nothing is abstracted away.
> > It would be different if we had a generic convention for "serial_id" or
> > stuff like that. But as configuration data is highly specific I don't
> > see this coming.
> > 
> 
> For a standard sysfs interface it is a huge difference yes. At the point
> of few from the EEPROM device it is a device like a temp sensor which
> could be different from vendor to vendor.

Here it gets frustrating. It seems you have no idea what an OS is for,
not even after I tried to describe it :(


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

> Here it gets frustrating. It seems you have no idea what an OS is for,
> not even after I tried to describe it :(

Sorry, that might have been too strong. Still, we can't map any hardware
which is out there 1:1 into userspace, we need abstraction.

If you want to help with this abstraction, this is more than
appreciated. If you want to keep your driver, this will have to stay
out-of-tree, I'm afraid.

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

On Mon, Oct 20, 2014 at 10:24:22AM +0200, Wolfram Sang wrote:
> * PGP Signed by an unknown key
> 
> > Here it gets frustrating. It seems you have no idea what an OS is for,
> > not even after I tried to describe it :(
> 

I am pretty sure that i know what an OS is for.

> Sorry, that might have been too strong. Still, we can't map any hardware
> which is out there 1:1 into userspace, we need abstraction.
> 
> If you want to help with this abstraction, this is more than
> appreciated. If you want to keep your driver, this will have to stay
> out-of-tree, I'm afraid.
> 

Yes, my goal is to find a good way to get hardware support upstream, and
it is also nice to discuss my point of view with your upstream point of few.

And yes, i want to help to find a way to develope an abstraction.

Regards
Andy


> Regards,
> 
>    Wolfram
> 
> 
> * Unknown Key
> * 0x14A029B6

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote:
> On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote:
> > * PGP Signed by an unknown key
> > 
> > 
> > > I do not want to parse the things in userspace because this EEPROM data
> > > are related to the hardware and i want to give our customer the easiest way
> > > to access the data without installing any tool.
> > 
> > I understand that point of view. From an upstream point of view, things
> > may look different, though.
> > 
> 
> I also understand your point of view :-).
> Most customers wants just to have a running system without installing anything.
> And for me an EEPROM is so simple and should not need a complicated way
> to access it.
> 
> > > The current state to read the eeprom data is, that customer needs to install a big
> > > environment where the tool is integrated to have access to those kind of simple
> > > data or they have to write their own code.
> > 
> > i2cget from i2c-tools? You could do a simple shell script to parse the
> > data. Or do a board specific hook which reads the data and prints it to
> > the logfiles...
> > 
> 
> Yes of course there are a lot of possibilities. This was just an example
> what we currently use and what was developed years ago.
> 
> With a driver like this you can also define read only attributes to prevent customer
> to write or modify the data in the production section. With i2ctools you can just
> write any data to it you want.
> 
> > > > Consider how bloated the sysfs-ABI might get if every vendor who uses an
> > > > eeprom wants to expose the data this way?
> > > > 
> > > 
> > > Yes and no. The possible sysfs entries gets bloated if every vendor will do it
> > > like this way, but normally there is just one Board EEPROM on the board, therefore
> > > only one driver gets loaded.
> > 
> > I am not talking about runtime here, I don't care about that. I am
> > talking about the ABI we create and we have to maintain basically
> > forever. And with vendor specific configuartion data I have doubts with
> > that being stable.
> > 
> 
> Ok, but i do not think that we can make a "general" ABI definition for those kind
> of devices because every vendor will have its own data in the EEPROM which he want
> to have.
> 
> > > I mean its the same for every i2c device like a temperature sensor, I can also
> > > read it from userspace without any special hwmon driver.
> > 
> > These is a HUGE difference. If I read tempX_input, I don't need to care
> > if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
> > The files you create are for your I2C EEPROM only. Data gets
> > "reformatted" and access gets hidden, but nothing is abstracted away.
> > It would be different if we had a generic convention for "serial_id" or
> > stuff like that. But as configuration data is highly specific I don't
> > see this coming.
> > 
> 
> For a standard sysfs interface it is a huge difference yes. At the point
> of few from the EEPROM device it is a device like a temp sensor which
> could be different from vendor to vendor.
> 
> Regards
> Andy
>

Greg what do you think about that driver as a Maintainer of the sysfs?
To we have other ways to get those kind of drivers in the mainline kernel?

Regards
Andy

 
> > 
> > * Unknown Key
> > * 0x14A029B6

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Greg KH]

On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote:
> On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote:
> > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote:
> > > * PGP Signed by an unknown key
> > > 
> > > 
> > > > I do not want to parse the things in userspace because this EEPROM data
> > > > are related to the hardware and i want to give our customer the easiest way
> > > > to access the data without installing any tool.
> > > 
> > > I understand that point of view. From an upstream point of view, things
> > > may look different, though.
> > > 
> > 
> > I also understand your point of view :-).
> > Most customers wants just to have a running system without installing anything.
> > And for me an EEPROM is so simple and should not need a complicated way
> > to access it.
> > 
> > > > The current state to read the eeprom data is, that customer needs to install a big
> > > > environment where the tool is integrated to have access to those kind of simple
> > > > data or they have to write their own code.
> > > 
> > > i2cget from i2c-tools? You could do a simple shell script to parse the
> > > data. Or do a board specific hook which reads the data and prints it to
> > > the logfiles...
> > > 
> > 
> > Yes of course there are a lot of possibilities. This was just an example
> > what we currently use and what was developed years ago.
> > 
> > With a driver like this you can also define read only attributes to prevent customer
> > to write or modify the data in the production section. With i2ctools you can just
> > write any data to it you want.
> > 
> > > > > Consider how bloated the sysfs-ABI might get if every vendor who uses an
> > > > > eeprom wants to expose the data this way?
> > > > > 
> > > > 
> > > > Yes and no. The possible sysfs entries gets bloated if every vendor will do it
> > > > like this way, but normally there is just one Board EEPROM on the board, therefore
> > > > only one driver gets loaded.
> > > 
> > > I am not talking about runtime here, I don't care about that. I am
> > > talking about the ABI we create and we have to maintain basically
> > > forever. And with vendor specific configuartion data I have doubts with
> > > that being stable.
> > > 
> > 
> > Ok, but i do not think that we can make a "general" ABI definition for those kind
> > of devices because every vendor will have its own data in the EEPROM which he want
> > to have.
> > 
> > > > I mean its the same for every i2c device like a temperature sensor, I can also
> > > > read it from userspace without any special hwmon driver.
> > > 
> > > These is a HUGE difference. If I read tempX_input, I don't need to care
> > > if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
> > > The files you create are for your I2C EEPROM only. Data gets
> > > "reformatted" and access gets hidden, but nothing is abstracted away.
> > > It would be different if we had a generic convention for "serial_id" or
> > > stuff like that. But as configuration data is highly specific I don't
> > > see this coming.
> > > 
> > 
> > For a standard sysfs interface it is a huge difference yes. At the point
> > of few from the EEPROM device it is a device like a temp sensor which
> > could be different from vendor to vendor.
> > 
> > Regards
> > Andy
> >
> 
> Greg what do you think about that driver as a Maintainer of the sysfs?

I maintain the sysfs core that drivers use, I don't dictate the policy
that those drivers create and send to userspace, that is up to the
maintainer of the subsystem.  There are some basic rules of sysfs (one
value per file), but other than that, please work with the maintainer to
come up with an interface that will work for all types of this device,
not just a one-off interface which does not scale at all, as Wolfram has
pointed out.

> To we have other ways to get those kind of drivers in the mainline kernel?

Yes, work on a common interface that your driver can use, and it can be
accepted.  Why do you not want to do that?

thanks,

greg k-h

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Andreas Werner]

On Mon, Oct 20, 2014 at 05:11:41PM +0800, Greg KH wrote:
> On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote:
> > On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote:
> > > On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote:
> > > > * PGP Signed by an unknown key
> > > > 
> > > > 
> > > > > I do not want to parse the things in userspace because this EEPROM data
> > > > > are related to the hardware and i want to give our customer the easiest way
> > > > > to access the data without installing any tool.
> > > > 
> > > > I understand that point of view. From an upstream point of view, things
> > > > may look different, though.
> > > > 
> > > 
> > > I also understand your point of view :-).
> > > Most customers wants just to have a running system without installing anything.
> > > And for me an EEPROM is so simple and should not need a complicated way
> > > to access it.
> > > 
> > > > > The current state to read the eeprom data is, that customer needs to install a big
> > > > > environment where the tool is integrated to have access to those kind of simple
> > > > > data or they have to write their own code.
> > > > 
> > > > i2cget from i2c-tools? You could do a simple shell script to parse the
> > > > data. Or do a board specific hook which reads the data and prints it to
> > > > the logfiles...
> > > > 
> > > 
> > > Yes of course there are a lot of possibilities. This was just an example
> > > what we currently use and what was developed years ago.
> > > 
> > > With a driver like this you can also define read only attributes to prevent customer
> > > to write or modify the data in the production section. With i2ctools you can just
> > > write any data to it you want.
> > > 
> > > > > > Consider how bloated the sysfs-ABI might get if every vendor who uses an
> > > > > > eeprom wants to expose the data this way?
> > > > > > 
> > > > > 
> > > > > Yes and no. The possible sysfs entries gets bloated if every vendor will do it
> > > > > like this way, but normally there is just one Board EEPROM on the board, therefore
> > > > > only one driver gets loaded.
> > > > 
> > > > I am not talking about runtime here, I don't care about that. I am
> > > > talking about the ABI we create and we have to maintain basically
> > > > forever. And with vendor specific configuartion data I have doubts with
> > > > that being stable.
> > > > 
> > > 
> > > Ok, but i do not think that we can make a "general" ABI definition for those kind
> > > of devices because every vendor will have its own data in the EEPROM which he want
> > > to have.
> > > 
> > > > > I mean its the same for every i2c device like a temperature sensor, I can also
> > > > > read it from userspace without any special hwmon driver.
> > > > 
> > > > These is a HUGE difference. If I read tempX_input, I don't need to care
> > > > if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
> > > > The files you create are for your I2C EEPROM only. Data gets
> > > > "reformatted" and access gets hidden, but nothing is abstracted away.
> > > > It would be different if we had a generic convention for "serial_id" or
> > > > stuff like that. But as configuration data is highly specific I don't
> > > > see this coming.
> > > > 
> > > 
> > > For a standard sysfs interface it is a huge difference yes. At the point
> > > of few from the EEPROM device it is a device like a temp sensor which
> > > could be different from vendor to vendor.
> > > 
> > > Regards
> > > Andy
> > >
> > 
> > Greg what do you think about that driver as a Maintainer of the sysfs?
> 
> I maintain the sysfs core that drivers use, I don't dictate the policy
> that those drivers create and send to userspace, that is up to the
> maintainer of the subsystem.  There are some basic rules of sysfs (one
> value per file), but other than that, please work with the maintainer to
> come up with an interface that will work for all types of this device,
> not just a one-off interface which does not scale at all, as Wolfram has
> pointed out.
> 

Ok.

> > To we have other ways to get those kind of drivers in the mainline kernel?
> 
> Yes, work on a common interface that your driver can use, and it can be
> accepted.  Why do you not want to do that?
> 
> thanks,
> 
> greg k-h

I have never talked about that I do not want to do it. I just want to discuss
it with you.

Right now we are at a point that i know that those kind of drivers can't be upstream
and i will try to find a way of abstraction to get it upstream.

Thanks
Andy

Re: [PATCH 1/2] drivers/misc/eeprom/men_eeprod: Introduce MEN Board Information EEPROM driver

[Igor Grinberg]

On 10/20/14 13:09, Andreas Werner wrote:
> On Mon, Oct 20, 2014 at 05:11:41PM +0800, Greg KH wrote:
>> On Mon, Oct 20, 2014 at 10:33:45AM +0200, Andreas Werner wrote:
>>> On Thu, Oct 16, 2014 at 01:44:02PM +0200, Andreas Werner wrote:
>>>> On Thu, Oct 16, 2014 at 11:59:10AM +0200, Wolfram Sang wrote:
>>>>> * PGP Signed by an unknown key
>>>>>
>>>>>
>>>>>> I do not want to parse the things in userspace because this EEPROM data
>>>>>> are related to the hardware and i want to give our customer the easiest way
>>>>>> to access the data without installing any tool.
>>>>>
>>>>> I understand that point of view. From an upstream point of view, things
>>>>> may look different, though.
>>>>>
>>>>
>>>> I also understand your point of view :-).
>>>> Most customers wants just to have a running system without installing anything.
>>>> And for me an EEPROM is so simple and should not need a complicated way
>>>> to access it.
>>>>
>>>>>> The current state to read the eeprom data is, that customer needs to install a big
>>>>>> environment where the tool is integrated to have access to those kind of simple
>>>>>> data or they have to write their own code.
>>>>>
>>>>> i2cget from i2c-tools? You could do a simple shell script to parse the
>>>>> data. Or do a board specific hook which reads the data and prints it to
>>>>> the logfiles...
>>>>>
>>>>
>>>> Yes of course there are a lot of possibilities. This was just an example
>>>> what we currently use and what was developed years ago.
>>>>
>>>> With a driver like this you can also define read only attributes to prevent customer
>>>> to write or modify the data in the production section. With i2ctools you can just
>>>> write any data to it you want.
>>>>
>>>>>>> Consider how bloated the sysfs-ABI might get if every vendor who uses an
>>>>>>> eeprom wants to expose the data this way?
>>>>>>>
>>>>>>
>>>>>> Yes and no. The possible sysfs entries gets bloated if every vendor will do it
>>>>>> like this way, but normally there is just one Board EEPROM on the board, therefore
>>>>>> only one driver gets loaded.
>>>>>
>>>>> I am not talking about runtime here, I don't care about that. I am
>>>>> talking about the ABI we create and we have to maintain basically
>>>>> forever. And with vendor specific configuartion data I have doubts with
>>>>> that being stable.
>>>>>
>>>>
>>>> Ok, but i do not think that we can make a "general" ABI definition for those kind
>>>> of devices because every vendor will have its own data in the EEPROM which he want
>>>> to have.
>>>>
>>>>>> I mean its the same for every i2c device like a temperature sensor, I can also
>>>>>> read it from userspace without any special hwmon driver.
>>>>>
>>>>> These is a HUGE difference. If I read tempX_input, I don't need to care
>>>>> if the sensor is I2C or SPI or whatever. The kernel abstracts that away.
>>>>> The files you create are for your I2C EEPROM only. Data gets
>>>>> "reformatted" and access gets hidden, but nothing is abstracted away.
>>>>> It would be different if we had a generic convention for "serial_id" or
>>>>> stuff like that. But as configuration data is highly specific I don't
>>>>> see this coming.
>>>>>
>>>>
>>>> For a standard sysfs interface it is a huge difference yes. At the point
>>>> of few from the EEPROM device it is a device like a temp sensor which
>>>> could be different from vendor to vendor.
>>>>
>>>> Regards
>>>> Andy
>>>>
>>>
>>> Greg what do you think about that driver as a Maintainer of the sysfs?
>>
>> I maintain the sysfs core that drivers use, I don't dictate the policy
>> that those drivers create and send to userspace, that is up to the
>> maintainer of the subsystem.  There are some basic rules of sysfs (one
>> value per file), but other than that, please work with the maintainer to
>> come up with an interface that will work for all types of this device,
>> not just a one-off interface which does not scale at all, as Wolfram has
>> pointed out.
>>
> 
> Ok.
> 
>>> To we have other ways to get those kind of drivers in the mainline kernel?
>>
>> Yes, work on a common interface that your driver can use, and it can be
>> accepted.  Why do you not want to do that?
>>
>> thanks,
>>
>> greg k-h
> 
> I have never talked about that I do not want to do it. I just want to discuss
> it with you.
> 
> Right now we are at a point that i know that those kind of drivers can't be upstream
> and i will try to find a way of abstraction to get it upstream.

IMO, at24 provides you a good enough abstraction which you parse
from user space with a help of a really small utility or a shell script...
That is a really small effort.

If you want to take it further, may it is worth looking at how the DMI
stuff is exported on x86 (except that you talk to eeprom directly)?

-- 
Regards,
Igor.

[PATCH 2/2] Documentation/ABI/testing/men_eeprod: Added sysfs description for men_eeprod

[Andreas Werner]

This patch adds the description for the men_eeprod.
men_eeprod is a driver for the MEN Board Information EEPROM which
exports the data using sysfs entries.

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 .../ABI/testing/sysfs-bus-i2c-devices-men_eeprod   | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod b/Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod
new file mode 100644
index 0000000..a488d0e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-men_eeprod
@@ -0,0 +1,69 @@
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/eeprod_id
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Show the identifier of the EEPROM structure.
+		There are different layouts of the EEPROM with more or less
+		information. The ID identifies each of the layouts.
+
+		Currently supported structure is EEPROD2 (0x0E)
+
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/revision
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Show the revision of the board.
+		Format of the revision field is XX.XX.XX
+
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/serialnr
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Show the serialnumber of the board. The serial number is
+		a decimal number.
+
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/hw_name
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Show the name of the board including the model.
+		There are boards which have different features such as
+		different size of RAM, the model identifies the different
+		types.
+
+		Format of the hw_name is: <name><model>.
+		Example: F075P00 where F75P is the hw_name and 00 the model.
+
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/prod_date
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Show the board production date.
+
+		Format of the date: YYYY-MM-DD
+
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/rep_date
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Show the last repair date.
+		If the board returns back to the repair department, this field
+		will be programmed with the current date.
+		If the board has never returned, the field will be empty and
+		the read access to this sysfs entry returns -EINVAL.
+
+		Format of the date: YYYY-MM-DD
+
+What:		/sys/bus/i2c/devices/i2c-<busnr>/<busnr>-<devaddr>/user_section
+Date:		October 2014
+Contact:	Andreas Werner <andreas.werner@men.de>
+Description:
+		Read or write access to the user section of the EEPROM
+		This sysfs entry is a binary file where customer can write
+		there own data to.
+		The size of this file depends on the EEPROM layout which is
+		identified by the eeprod_id.
+
+		eeprod_id	user_section size
+		---------------------------------
+		0x0E		232 byte
-- 
2.1.0