Discussion:
[PATCH 0/2] Introduce MEN Board Information EEPROM driver
Andreas Werner
2014-10-16 08:14:48 UTC
Permalink
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
Andreas Werner
2014-10-16 08:15:08 UTC
Permalink
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 <***@men.de>
Signed-off-by: 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 <***@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 <***@imgtec.com>
L: linux-***@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 <***@men.de>");
+MODULE_LICENSE("GPL v2");
--
2.1.0
Greg KH
2014-10-16 08:44:12 UTC
Permalink
Post by Andreas Werner
+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.)
Post by Andreas Werner
+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.
Andreas Werner
2014-10-16 10:11:03 UTC
Permalink
Post by Greg KH
Post by Andreas Werner
+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.
Post by Greg KH
Post by Andreas Werner
+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
Wolfram Sang
2014-10-16 08:58:35 UTC
Permalink
Post by 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.
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?
Post by Andreas Werner
+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?
Andreas Werner
2014-10-16 10:21:27 UTC
Permalink
* PGP Signed by an unknown key
Post by 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.
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.
Post by Andreas Werner
+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
Wolfram Sang
2014-10-16 09:59:10 UTC
Permalink
Post by Andreas Werner
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.
Post by Andreas Werner
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...
Post by Andreas Werner
Post by Wolfram Sang
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.
Post by Andreas Werner
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.
Andreas Werner
2014-10-16 11:44:02 UTC
Permalink
* PGP Signed by an unknown key
Post by Andreas Werner
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.
Post by Andreas Werner
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.
Post by Andreas Werner
Post by Wolfram Sang
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.
Post by Andreas Werner
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
Andreas Werner
2014-10-20 08:33:45 UTC
Permalink
Post by Andreas Werner
* PGP Signed by an unknown key
Post by Andreas Werner
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.
Post by Andreas Werner
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.
Post by Andreas Werner
Post by Wolfram Sang
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.
Post by Andreas Werner
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
Post by Andreas Werner
* Unknown Key
* 0x14A029B6
Greg KH
2014-10-20 09:11:41 UTC
Permalink
Post by Andreas Werner
Post by Andreas Werner
* PGP Signed by an unknown key
Post by Andreas Werner
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.
Post by Andreas Werner
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.
Post by Andreas Werner
Post by Wolfram Sang
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.
Post by Andreas Werner
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.
Post by Andreas Werner
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
Andreas Werner
2014-10-20 10:09:33 UTC
Permalink
Post by Greg KH
Post by Andreas Werner
Post by Andreas Werner
* PGP Signed by an unknown key
Post by Andreas Werner
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.
Post by Andreas Werner
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.
Post by Andreas Werner
Post by Wolfram Sang
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.
Post by Andreas Werner
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.
Post by Greg KH
Post by Andreas Werner
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

Wolfram Sang
2014-10-20 08:18:22 UTC
Permalink
Post by Andreas Werner
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.
Post by Andreas Werner
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.
Post by Andreas Werner
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.
Post by Andreas Werner
Post by Wolfram Sang
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.
Post by Andreas Werner
Post by Wolfram Sang
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 :(
Wolfram Sang
2014-10-20 08:24:22 UTC
Permalink
Post by Wolfram Sang
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
Andreas Werner
2014-10-20 10:04:37 UTC
Permalink
* PGP Signed by an unknown key
Post by Wolfram Sang
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
Greg KH
2014-10-16 09:34:14 UTC
Permalink
Post by Wolfram Sang
Post by Andreas Werner
+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
Wolfram Sang
2014-10-16 09:48:04 UTC
Permalink
Post by Greg KH
Post by Wolfram Sang
Post by Andreas Werner
+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.
Andreas Werner
2014-10-16 08:15:27 UTC
Permalink
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 <***@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 <***@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 <***@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 <***@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 <***@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 <***@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 <***@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 <***@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
Loading...