| Message ID | 1424365708-26681-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote: > From: Maxime Ripard <maxime.ripard@free-electrons.com> > > Up until now, EEPROM drivers were stored in drivers/misc, where they all had to > duplicate pretty much the same code to register a sysfs file, allow in-kernel > users to access the content of the devices they were driving, etc. > > This was also a problem as far as other in-kernel users were involved, since > the solutions used were pretty much different from on driver to another, there > was a rather big abstraction leak. > > This introduction of this framework aims at solving this. It also introduces DT > representation for consumer devices to go get the data they require (MAC > Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs. > > Having regmap interface to this framework would give much better > abstraction for eeproms on different buses. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > [srinivas.kandagatla: Moved to regmap based and cleanedup apis] > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/eeprom/Kconfig | 19 ++ > drivers/eeprom/Makefile | 9 + > drivers/eeprom/core.c | 290 +++++++++++++++++++++ > include/linux/eeprom-consumer.h | 73 ++++++ > include/linux/eeprom-provider.h | 51 ++++ > 8 files changed, 493 insertions(+) > create mode 100644 Documentation/devicetree/bindings/eeprom/eeprom.txt > create mode 100644 drivers/eeprom/Kconfig > create mode 100644 drivers/eeprom/Makefile > create mode 100644 drivers/eeprom/core.c > create mode 100644 include/linux/eeprom-consumer.h > create mode 100644 include/linux/eeprom-provider.h > > diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt > new file mode 100644 > index 0000000..9ec1ec2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt > @@ -0,0 +1,48 @@ > += EEPROM Data Device Tree Bindings = > + > +This binding is intended to represent the location of hardware > +configuration data stored in EEPROMs. > + > +On a significant proportion of boards, the manufacturer has stored > +some data on an EEPROM-like device, for the OS to be able to retrieve > +these information and act upon it. Obviously, the OS has to know > +about where to retrieve these data from, and where they are stored on > +the storage device. > + > +This document is here to document this. > + > += Data providers = > + > +Required properties: > +#eeprom-cells: Number of cells in an eeprom specifier; The common > + case is 2. > + > +For example: > + > + at24: eeprom@42 { > + #eeprom-cells = <2>; > + }; > + > += Data consumers = > + > +Required properties: > + > +eeproms: List of phandle and data cell specifier triplet, one triplet > + for each data cell the device might be interested in. The > + triplet consists of the phandle to the eeprom provider, then > + the offset in byte within that storage device, and the length bytes > + in byte of the data we care about. bytes > + > +Optional properties: > + > +eeprom-names: List of data cell name strings sorted in the same order > + as the resets property. Consumers drivers will use resets? I guess this text was cut/paste from the reset documentation?\ > + eeprom-names to differentiate between multiple cells, > + and hence being able to know what these cells are for. > + > +For example: > + > + device { > + eeproms = <&at24 14 42>; I like to use 42, but is it realistic to have a soc-rev-id which is 42 bytes long? How about using 42 as the offset and a sensible length of say 4? > + eeprom-names = "soc-rev-id"; > + }; > diff --git a/drivers/Kconfig b/drivers/Kconfig > index c70d6e4..d7afc82 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -184,4 +184,6 @@ source "drivers/thunderbolt/Kconfig" > > source "drivers/android/Kconfig" > > +source "drivers/eeprom/Kconfig" > + > endmenu > diff --git a/drivers/Makefile b/drivers/Makefile > index 527a6da..57eb5b0 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ > obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ > obj-$(CONFIG_CORESIGHT) += coresight/ > obj-$(CONFIG_ANDROID) += android/ > +obj-$(CONFIG_EEPROM) += eeprom/ > diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig > new file mode 100644 > index 0000000..2c5452a > --- /dev/null > +++ b/drivers/eeprom/Kconfig > @@ -0,0 +1,19 @@ > +menuconfig EEPROM > + bool "EEPROM Support" > + depends on OF > + help > + Support for EEPROM alike devices. like. > + > + This framework is designed to provide a generic interface to EEPROM EPROMs > + from both the Linux Kernel and the userspace. > + > + If unsure, say no. > + > +if EEPROM > + > +config EEPROM_DEBUG > + bool "EEPROM debug support" > + help > + Say yes here to enable debugging support. > + > +endif > diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile > new file mode 100644 > index 0000000..e130079 > --- /dev/null > +++ b/drivers/eeprom/Makefile > @@ -0,0 +1,9 @@ > +# > +# Makefile for eeprom drivers. > +# > + > +ccflags-$(CONFIG_EEPROM_DEBUG) += -DDEBUG > + > +obj-$(CONFIG_EEPROM) += core.o > + > +# Devices > diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c > new file mode 100644 > index 0000000..bc877a6 > --- /dev/null > +++ b/drivers/eeprom/core.c > @@ -0,0 +1,290 @@ > +/* > + * EEPROM framework core. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/device.h> > +#include <linux/eeprom-provider.h> > +#include <linux/eeprom-consumer.h> > +#include <linux/export.h> > +#include <linux/fs.h> > +#include <linux/idr.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/uaccess.h> > + > +struct eeprom_cell { > + struct eeprom_device *eeprom; > + loff_t offset; > + size_t count; > +}; > + > +static DEFINE_MUTEX(eeprom_list_mutex); > +static LIST_HEAD(eeprom_list); > +static DEFINE_IDA(eeprom_ida); > + > +static ssize_t bin_attr_eeprom_read_write(struct kobject *kobj, > + char *buf, loff_t offset, > + size_t count, bool read) > +{ > + struct device *dev = container_of(kobj, struct device, kobj); > + struct eeprom_device *eeprom = container_of(dev, struct eeprom_device, > + edev); > + int rc; > + > + if (offset > eeprom->size) > + return 0; > + > + if (offset + count > eeprom->size) > + count = eeprom->size - offset; > + > + if (read) > + rc = regmap_bulk_read(eeprom->regmap, offset, > + buf, count/eeprom->stride); This division will round down, so you could get one less byte than what you expected, and the value you actually return. It seems like there should be a check here, the count is a multiple of stride and return an error if it is not. > + else > + rc = regmap_bulk_write(eeprom->regmap, offset, > + buf, count/eeprom->stride); > + > + if (IS_ERR_VALUE(rc)) > + return 0; > + I don't think returning 0 here, and above is the best thing to do. Return the real error code from regmap, or EINVAL or some other error code for going off the end of the eerpom. > + return count; > +} > + > +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t offset, size_t count) > +{ > + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true); > +} > + > +static ssize_t bin_attr_eeprom_write(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t offset, size_t count) > +{ > + return bin_attr_eeprom_read_write(kobj, buf, offset, count, false); > +} > These two functions seem to be identical. So just have one of them? + > +static struct bin_attribute bin_attr_eeprom = { > + .attr = { > + .name = "eeprom", > + .mode = 0660, Symbolic values like S_IRUGO | S_IWUSR would be better. Are you also sure you want group write? > + }, > + .read = bin_attr_eeprom_read, > + .write = bin_attr_eeprom_write, > +}; > + > +static struct bin_attribute *eeprom_bin_attributes[] = { > + &bin_attr_eeprom, > + NULL, > +}; > + > +static const struct attribute_group eeprom_bin_group = { > + .bin_attrs = eeprom_bin_attributes, > +}; > + > +static const struct attribute_group *eeprom_dev_groups[] = { > + &eeprom_bin_group, > + NULL, > +}; > + > +static struct class eeprom_class = { > + .name = "eeprom", > + .dev_groups = eeprom_dev_groups, > +}; > + > +int eeprom_register(struct eeprom_device *eeprom) > +{ > + int rval; > + > + if (!eeprom->regmap || !eeprom->size) { > + dev_err(eeprom->dev, "Regmap not found\n"); > + return -EINVAL; > + } > + > + eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL); > + if (eeprom->id < 0) > + return eeprom->id; > + > + eeprom->edev.class = &eeprom_class; > + eeprom->edev.parent = eeprom->dev; > + eeprom->edev.of_node = eeprom->dev ? eeprom->dev->of_node : NULL; > + dev_set_name(&eeprom->edev, "eeprom%d", eeprom->id); > + > + device_initialize(&eeprom->edev); > + > + dev_dbg(&eeprom->edev, "Registering eeprom device %s\n", > + dev_name(&eeprom->edev)); > + > + rval = device_add(&eeprom->edev); > + if (rval) > + return rval; > + > + mutex_lock(&eeprom_list_mutex); > + list_add(&eeprom->list, &eeprom_list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(eeprom_register); > + > +int eeprom_unregister(struct eeprom_device *eeprom) > +{ > + device_del(&eeprom->edev); > + > + mutex_lock(&eeprom_list_mutex); > + list_del(&eeprom->list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(eeprom_unregister); > + > +static struct eeprom_cell *__eeprom_cell_get(struct device_node *node, > + int index) > +{ > + struct of_phandle_args args; > + struct eeprom_cell *cell; > + struct eeprom_device *e, *eeprom = NULL; > + int ret; > + > + ret = of_parse_phandle_with_args(node, "eeproms", > + "#eeprom-cells", index, &args); > + if (ret) > + return ERR_PTR(ret); > + > + if (args.args_count != 2) > + return ERR_PTR(-EINVAL); > + > + mutex_lock(&eeprom_list_mutex); > + > + list_for_each_entry(e, &eeprom_list, list) { > + if (args.np == e->edev.of_node) { > + eeprom = e; > + break; > + } > + } > + mutex_unlock(&eeprom_list_mutex); Shouldn't you increment a reference count to the eeprom here? You are going to have trouble if the eeprom is unregistered and there is a call still referring to it. > + > + if (!eeprom) > + return ERR_PTR(-EPROBE_DEFER); > + > + cell = kzalloc(sizeof(*cell), GFP_KERNEL); > + if (!cell) > + return ERR_PTR(-ENOMEM); > + > + cell->eeprom = eeprom; > + cell->offset = args.args[0]; > + cell->count = args.args[1]; > + > + return cell; > +} > + > +static struct eeprom_cell *__eeprom_cell_get_byname(struct device_node *node, > + const char *id) > +{ > + int index = 0; > + > + if (id) > + index = of_property_match_string(node, > + "eeprom-names", > + id); > + return __eeprom_cell_get(node, index); > + > +} > + > +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index) > +{ > + if (!dev) > + return ERR_PTR(-EINVAL); > + > + /* First, attempt to retrieve the cell through the DT */ > + if (dev->of_node) > + return __eeprom_cell_get(dev->of_node, index); > + > + /* We don't support anything else yet */ > + return ERR_PTR(-ENODEV); > +} > +EXPORT_SYMBOL(eeprom_cell_get); > + > +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, const char *id) > +{ > + if (!dev) > + return ERR_PTR(-EINVAL); > + > + if (id && dev->of_node) > + return __eeprom_cell_get_byname(dev->of_node, id); > + > + /* We don't support anything else yet */ > + return ERR_PTR(-ENODEV); > +} > +EXPORT_SYMBOL(eeprom_cell_get_byname); > + > +void eeprom_cell_put(struct eeprom_cell *cell) > +{ > + kfree(cell); > +} > +EXPORT_SYMBOL(eeprom_cell_put); > + > +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len) > +{ > + struct eeprom_device *eeprom = cell->eeprom; > + char *buf; > + int rc; > + > + if (!eeprom || !eeprom->regmap) > + return ERR_PTR(-EINVAL); > + > + buf = kzalloc(cell->count, GFP_KERNEL); > + if (!buf) > + return ERR_PTR(-ENOMEM); > + > + rc = regmap_bulk_read(eeprom->regmap, cell->offset, > + buf, cell->count/eeprom->stride); Same comment as above. > + if (IS_ERR_VALUE(rc)) { > + kfree(buf); > + return ERR_PTR(rc); > + } > + > + *len = cell->count; > + > + return buf; > +} > +EXPORT_SYMBOL(eeprom_cell_read); > + > +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len) > +{ > + struct eeprom_device *eeprom = cell->eeprom; > + > + if (!eeprom || !eeprom->regmap) > + return -EINVAL; > + > + return regmap_bulk_write(eeprom->regmap, cell->offset, > + buf, cell->count/eeprom->stride); > +} > +EXPORT_SYMBOL(eeprom_cell_write); > + > +static int eeprom_init(void) > +{ > + return class_register(&eeprom_class); > +} > + > +static void eeprom_exit(void) > +{ > + class_unregister(&eeprom_class); > +} > + > +subsys_initcall(eeprom_init); > +module_exit(eeprom_exit); > + > +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com"); > +MODULE_DESCRIPTION("EEPROM Driver Core"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/eeprom-consumer.h b/include/linux/eeprom-consumer.h > new file mode 100644 > index 0000000..706ae9d > --- /dev/null > +++ b/include/linux/eeprom-consumer.h > @@ -0,0 +1,73 @@ > +/* > + * EEPROM framework consumer. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#ifndef _LINUX_EEPROM_CONSUMER_H > +#define _LINUX_EEPROM_CONSUMER_H > + > +struct eeprom_cell; > + > +/** > + * eeprom_cell_get(): Get eeprom cell of device form a given index. of a device for a > + * > + * @dev: Device that will be interacted with > + * @index: Index of the eeprom cell. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a struct eeprom_cell. The eeprom_cell will be freed by the > + * eeprom_cell_put(). > + */ > +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index); > + > +/** > + * eeprom_cell_get(): Get eeprom cell of device form a given name. same again > + * > + * @dev: Device that will be interacted with > + * @name: Name of the eeprom cell. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a struct eeprom_cell. The eeprom_cell will be freed by the > + * eeprom_cell_put(). > + */ > +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, > + const char *name); > + > +/** > + * eeprom_cell_put(): Release previously allocated eeprom cell. > + * > + * @cell: Previously allocated eeprom cell by eeprom_cell_get() > + * or eeprom_cell_get_byname(). > + */ > +void eeprom_cell_put(struct eeprom_cell *cell); > + > +/** > + * eeprom_cell_read(): Read a given eeprom cell > + * > + * @cell: eeprom cell to be read. > + * @len: pointer to length of cell which will be populated on successful read. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a char * bufffer. The buffer should be freed by the consumer with a > + * kfree(). > + */ > +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len); Would void * be better than char *? I guess the contents is mostly data, not strings. Andrew > + > +/** > + * eeprom_cell_write(): Write to a given eeprom cell > + * > + * @cell: eeprom cell to be written. > + * @buf: Buffer to be written. > + * @len: length of buffer to be written to eeprom cell. > + * > + * The return value will be an non zero on error or a zero on successful write. > + */ > +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len); > + > +#endif /* ifndef _LINUX_EEPROM_CONSUMER_H */ > diff --git a/include/linux/eeprom-provider.h b/include/linux/eeprom-provider.h > new file mode 100644 > index 0000000..3943c2f > --- /dev/null > +++ b/include/linux/eeprom-provider.h > @@ -0,0 +1,51 @@ > +/* > + * EEPROM framework provider. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#ifndef _LINUX_EEPROM_PROVIDER_H > +#define _LINUX_EEPROM_PROVIDER_H > + > +#include <linux/device.h> > +#include <linux/regmap.h> > +#include <linux/list.h> > + > +struct eeprom_device { > + struct regmap *regmap; > + int stride; > + size_t size; > + struct device *dev; > + > + /* Internal to framework */ > + struct device edev; > + int id; > + struct list_head list; > +}; > + > +/** > + * eeprom_register(): Register a eeprom device for given eeprom. > + * Also creates an binary entry in /sys/class/eeprom/eeprom[id]/eeprom > + * > + * @eeprom: eeprom device that needs to be created > + * > + * The return value will be an error code on error or a zero on success. > + * The eeprom_device and sysfs entery will be freed by the eeprom_unregister(). > + */ > +int eeprom_register(struct eeprom_device *eeprom); > + > +/** > + * eeprom_unregister(): Unregister previously registered eeprom device > + * > + * @eeprom: Pointer to previously registered eeprom device. > + * > + * The return value will be an non zero on error or a zero on success. > + */ > +int eeprom_unregister(struct eeprom_device *eeprom); > + > +#endif /* ifndef _LINUX_EEPROM_PROVIDER_H */ > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/15 09:08, Srinivas Kandagatla wrote: > diff --git a/drivers/Kconfig b/drivers/Kconfig > index c70d6e4..d7afc82 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -184,4 +184,6 @@ source "drivers/thunderbolt/Kconfig" > > source "drivers/android/Kconfig" > > +source "drivers/eeprom/Kconfig" > + > endmenu > diff --git a/drivers/Makefile b/drivers/Makefile > index 527a6da..57eb5b0 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ > obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ > obj-$(CONFIG_CORESIGHT) += coresight/ > obj-$(CONFIG_ANDROID) += android/ > +obj-$(CONFIG_EEPROM) += eeprom/ > diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig > new file mode 100644 > index 0000000..2c5452a > --- /dev/null > +++ b/drivers/eeprom/Kconfig > @@ -0,0 +1,19 @@ > +menuconfig EEPROM > + bool "EEPROM Support" > + depends on OF Doesn't this need some sort of select REGMAP somewhere? Also, why do we need to use regmap for the eeprom framework read/write ops? I liked the simple eeprom::{read,write} API that Maxime had. The regmap part could be a regmap-eeprom driver that implements read/write ops like you've done in the core. > + help > + Support for EEPROM alike devices. > + > + This framework is designed to provide a generic interface to EEPROM > + from both the Linux Kernel and the userspace. > + > + If unsure, say no. > + > +if EEPROM > + > +config EEPROM_DEBUG > + bool "EEPROM debug support" > + help > + Say yes here to enable debugging support. > + > +endif > > diff --git a/include/linux/eeprom-provider.h b/include/linux/eeprom-provider.h > new file mode 100644 > index 0000000..3943c2f > --- /dev/null > +++ b/include/linux/eeprom-provider.h > @@ -0,0 +1,51 @@ > +/* > + * EEPROM framework provider. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#ifndef _LINUX_EEPROM_PROVIDER_H > +#define _LINUX_EEPROM_PROVIDER_H > + > +#include <linux/device.h> > +#include <linux/regmap.h> > +#include <linux/list.h> > + > +struct eeprom_device { > + struct regmap *regmap; > + int stride; > + size_t size; > + struct device *dev; > + > + /* Internal to framework */ > + struct device edev; > + int id; > + struct list_head list; Should there be a module owner here to handle module removal?
On 20/02/15 02:36, Stephen Boyd wrote: > On 02/19/15 09:08, Srinivas Kandagatla wrote: >> diff --git a/drivers/Kconfig b/drivers/Kconfig >> index c70d6e4..d7afc82 100644 >> --- a/drivers/Kconfig >> +++ b/drivers/Kconfig >> @@ -184,4 +184,6 @@ source "drivers/thunderbolt/Kconfig" >> >> source "drivers/android/Kconfig" >> >> +source "drivers/eeprom/Kconfig" >> + >> endmenu >> diff --git a/drivers/Makefile b/drivers/Makefile >> index 527a6da..57eb5b0 100644 >> --- a/drivers/Makefile >> +++ b/drivers/Makefile >> @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ >> obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ >> obj-$(CONFIG_CORESIGHT) += coresight/ >> obj-$(CONFIG_ANDROID) += android/ >> +obj-$(CONFIG_EEPROM) += eeprom/ >> diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig >> new file mode 100644 >> index 0000000..2c5452a >> --- /dev/null >> +++ b/drivers/eeprom/Kconfig >> @@ -0,0 +1,19 @@ >> +menuconfig EEPROM >> + bool "EEPROM Support" >> + depends on OF > > Doesn't this need some sort of select REGMAP somewhere? May be depends REGMAP would be good. > > Also, why do we need to use regmap for the eeprom framework read/write > ops? I liked the simple eeprom::{read,write} API that Maxime had. The > regmap part could be a regmap-eeprom driver that implements read/write > ops like you've done in the core. regmap bus does the same job. The only reason for using regmap here is to have more generic drivers for eeprom providers based on different buses like mmio, i2c, spi... As of today we could just make the qfprom eeprom provider as more generic eeprom-mmio provider. May be sunxi_sid could reuse it too with some effort. In future It may be possible to have eeprom-i2c or eeprom-spi providers. > >> + >> +struct eeprom_device { >> + struct regmap *regmap; >> + int stride; >> + size_t size; >> + struct device *dev; >> + >> + /* Internal to framework */ >> + struct device edev; >> + int id; >> + struct list_head list; > > Should there be a module owner here to handle module removal? > Good point, we should do some reference counting. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Andrew for your comments, On 19/02/15 18:12, Andrew Lunn wrote: >> + >> +Required properties: >> + >> +eeproms: List of phandle and data cell specifier triplet, one triplet >> + for each data cell the device might be interested in. The >> + triplet consists of the phandle to the eeprom provider, then >> + the offset in byte within that storage device, and the length > > bytes > >> + in byte of the data we care about. > > bytes Yep will fix it in next version. > >> + >> +Optional properties: >> + >> +eeprom-names: List of data cell name strings sorted in the same order >> + as the resets property. Consumers drivers will use > > resets? I guess this text was cut/paste from the reset documentation?\ > I think so. Will fix it. >> + eeprom-names to differentiate between multiple cells, >> + and hence being able to know what these cells are for. >> + >> +For example: >> + >> + device { >> + eeproms = <&at24 14 42>; > > I like to use 42, but is it realistic to have a soc-rev-id which is 42 > bytes long? How about using 42 as the offset and a sensible length of > say 4? Ok, will fix it.. > >> + eeprom-names = "soc-rev-id"; >> +menuconfig EEPROM >> + bool "EEPROM Support" >> + depends on OF >> + help >> + Support for EEPROM alike devices. > > like. Ok > >> + >> + This framework is designed to provide a generic interface to EEPROM > > EPROMs Ok. > >> + >> + >> +static ssize_t bin_attr_eeprom_read_write(struct kobject *kobj, >> + char *buf, loff_t offset, >> + size_t count, bool read) >> +{ >> + struct device *dev = container_of(kobj, struct device, kobj); >> + struct eeprom_device *eeprom = container_of(dev, struct eeprom_device, >> + edev); >> + int rc; >> + >> + if (offset > eeprom->size) >> + return 0; >> + >> + if (offset + count > eeprom->size) >> + count = eeprom->size - offset; >> + >> + if (read) >> + rc = regmap_bulk_read(eeprom->regmap, offset, >> + buf, count/eeprom->stride); > > This division will round down, so you could get one less byte than > what you expected, and the value you actually return. It seems like > there should be a check here, the count is a multiple of stride and > return an error if it is not. Thats a good catch, I will fix this for other such instances too. > >> + else >> + rc = regmap_bulk_write(eeprom->regmap, offset, >> + buf, count/eeprom->stride); >> + >> + if (IS_ERR_VALUE(rc)) >> + return 0; >> + > > I don't think returning 0 here, and above is the best thing to > do. Return the real error code from regmap, or EINVAL or some other > error code for going off the end of the eerpom. Ok, I will fix the return value here for both the cases. > >> + return count; >> +} >> + >> +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *attr, >> + char *buf, loff_t offset, size_t count) >> +{ >> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true); >> +} >> + >> +static ssize_t bin_attr_eeprom_write(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *attr, >> + char *buf, loff_t offset, size_t count) >> +{ >> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, false); >> +} >> > > These two functions seem to be identical. So just have one of them? One is read and other is write.. there is a true and false flag at the end of the call to bin_attr_eeprom_read_write(). > > + >> +static struct bin_attribute bin_attr_eeprom = { >> + .attr = { >> + .name = "eeprom", >> + .mode = 0660, > > Symbolic values like S_IRUGO | S_IWUSR would be better. Yep, thats correct, I will fix it. > > Are you also sure you want group write? > S_IWUSR should be enough. >> + }, >> + .read = bin_attr_eeprom_read, >> + .write = bin_attr_eeprom_write, >> +}; >> + >> +static struct eeprom_cell *__eeprom_cell_get(struct device_node *node, >> + int index) >> +{ >> + struct of_phandle_args args; >> + struct eeprom_cell *cell; >> + struct eeprom_device *e, *eeprom = NULL; >> + int ret; >> + >> + ret = of_parse_phandle_with_args(node, "eeproms", >> + "#eeprom-cells", index, &args); >> + if (ret) >> + return ERR_PTR(ret); >> + >> + if (args.args_count != 2) >> + return ERR_PTR(-EINVAL); >> + >> + mutex_lock(&eeprom_list_mutex); >> + >> + list_for_each_entry(e, &eeprom_list, list) { >> + if (args.np == e->edev.of_node) { >> + eeprom = e; >> + break; >> + } >> + } >> + mutex_unlock(&eeprom_list_mutex); > > Shouldn't you increment a reference count to the eeprom here? You are > going to have trouble if the eeprom is unregistered and there is a > call still referring to it. Yes, Stephen Byod also pointed the same, having owner in eeprom_device should fix this. I will fix it in next version. > >> + >> + if (!eeprom) >> + return ERR_PTR(-EPROBE_DEFER); >> + >> + cell = kzalloc(sizeof(*cell), GFP_KERNEL); >> + if (!cell) >> + return ERR_PTR(-ENOMEM); >> + >> + cell->eeprom = eeprom; >> + cell->offset = args.args[0]; >> + cell->count = args.args[1]; >> + >> + return cell; >> +} >> + >> + >> diff --git a/include/linux/eeprom-consumer.h b/include/linux/eeprom-consumer.h >> new file mode 100644 >> index 0000000..706ae9d >> --- /dev/null >> +++ b/include/linux/eeprom-consumer.h >> @@ -0,0 +1,73 @@ >> +/* >> + * EEPROM framework consumer. >> + * >> + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + */ >> + >> +#ifndef _LINUX_EEPROM_CONSUMER_H >> +#define _LINUX_EEPROM_CONSUMER_H >> + >> +struct eeprom_cell; >> + >> +/** >> + * eeprom_cell_get(): Get eeprom cell of device form a given index. > > of a device for a > Ok, will be fixed in next version. >> + * >> + * @dev: Device that will be interacted with >> + * @index: Index of the eeprom cell. >> + * >> + * The return value will be an ERR_PTR() on error or a valid pointer >> + * to a struct eeprom_cell. The eeprom_cell will be freed by the >> + * eeprom_cell_put(). >> + */ >> +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index); >> + >> +/** >> + * eeprom_cell_get(): Get eeprom cell of device form a given name. > > same again Ok, will be fixed in next version. > >> + * >> + * @dev: Device that will be interacted with >> + * @name: Name of the eeprom cell. >> + * >> + * The return value will be an ERR_PTR() on error or a valid pointer >> + * to a struct eeprom_cell. The eeprom_cell will be freed by the >> + * eeprom_cell_put(). >> + */ >> +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, >> + const char *name); >> + >> +/** >> + * eeprom_cell_put(): Release previously allocated eeprom cell. >> + * >> + * @cell: Previously allocated eeprom cell by eeprom_cell_get() >> + * or eeprom_cell_get_byname(). >> + */ >> +void eeprom_cell_put(struct eeprom_cell *cell); >> + >> +/** >> + * eeprom_cell_read(): Read a given eeprom cell >> + * >> + * @cell: eeprom cell to be read. >> + * @len: pointer to length of cell which will be populated on successful read. >> + * >> + * The return value will be an ERR_PTR() on error or a valid pointer >> + * to a char * bufffer. The buffer should be freed by the consumer with a >> + * kfree(). >> + */ >> +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len); > > Would void * be better than char *? I guess the contents is mostly > data, not strings. Yes, thats sounds sensible. > > Andrew > >> + >> +/** >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/02/15 08:14, Srinivas Kandagatla wrote: >> >> Doesn't this need some sort of select REGMAP somewhere? > May be depends REGMAP would be good. You are right, just realized that it should be "select REGMAP" and for QFPROM it should be "select REGMAP_MMIO" --srini -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 19, 2015 at 11:08 AM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > From: Maxime Ripard <maxime.ripard@free-electrons.com> > > Up until now, EEPROM drivers were stored in drivers/misc, where they all had to > duplicate pretty much the same code to register a sysfs file, allow in-kernel > users to access the content of the devices they were driving, etc. > > This was also a problem as far as other in-kernel users were involved, since > the solutions used were pretty much different from on driver to another, there > was a rather big abstraction leak. > > This introduction of this framework aims at solving this. It also introduces DT > representation for consumer devices to go get the data they require (MAC > Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs. > > Having regmap interface to this framework would give much better > abstraction for eeproms on different buses. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > [srinivas.kandagatla: Moved to regmap based and cleanedup apis] > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/eeprom/Kconfig | 19 ++ > drivers/eeprom/Makefile | 9 + > drivers/eeprom/core.c | 290 +++++++++++++++++++++ > include/linux/eeprom-consumer.h | 73 ++++++ > include/linux/eeprom-provider.h | 51 ++++ Who is going to be the maintainer for this? > 8 files changed, 493 insertions(+) > create mode 100644 Documentation/devicetree/bindings/eeprom/eeprom.txt > create mode 100644 drivers/eeprom/Kconfig > create mode 100644 drivers/eeprom/Makefile > create mode 100644 drivers/eeprom/core.c > create mode 100644 include/linux/eeprom-consumer.h > create mode 100644 include/linux/eeprom-provider.h > > diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt > new file mode 100644 > index 0000000..9ec1ec2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt Please make bindings a separate patch. > @@ -0,0 +1,48 @@ > += EEPROM Data Device Tree Bindings = > + > +This binding is intended to represent the location of hardware > +configuration data stored in EEPROMs. > + > +On a significant proportion of boards, the manufacturer has stored > +some data on an EEPROM-like device, for the OS to be able to retrieve > +these information and act upon it. Obviously, the OS has to know > +about where to retrieve these data from, and where they are stored on > +the storage device. > + > +This document is here to document this. > + > += Data providers = > + > +Required properties: > +#eeprom-cells: Number of cells in an eeprom specifier; The common > + case is 2. We already have eeproms in DTs, it would be nice to be able to support them with this framework as well. > + > +For example: > + > + at24: eeprom@42 { > + #eeprom-cells = <2>; > + }; > + > += Data consumers = > + > +Required properties: > + > +eeproms: List of phandle and data cell specifier triplet, one triplet > + for each data cell the device might be interested in. The > + triplet consists of the phandle to the eeprom provider, then > + the offset in byte within that storage device, and the length > + in byte of the data we care about. The problem with this is it assumes you know who the consumer is and that it is a DT node. For example, how would you describe a serial number? Rob > + > +Optional properties: > + > +eeprom-names: List of data cell name strings sorted in the same order > + as the resets property. Consumers drivers will use > + eeprom-names to differentiate between multiple cells, > + and hence being able to know what these cells are for. > + > +For example: > + > + device { > + eeproms = <&at24 14 42>; > + eeprom-names = "soc-rev-id"; > + }; > diff --git a/drivers/Kconfig b/drivers/Kconfig > index c70d6e4..d7afc82 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -184,4 +184,6 @@ source "drivers/thunderbolt/Kconfig" > > source "drivers/android/Kconfig" > > +source "drivers/eeprom/Kconfig" > + > endmenu > diff --git a/drivers/Makefile b/drivers/Makefile > index 527a6da..57eb5b0 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ > obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ > obj-$(CONFIG_CORESIGHT) += coresight/ > obj-$(CONFIG_ANDROID) += android/ > +obj-$(CONFIG_EEPROM) += eeprom/ > diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig > new file mode 100644 > index 0000000..2c5452a > --- /dev/null > +++ b/drivers/eeprom/Kconfig > @@ -0,0 +1,19 @@ > +menuconfig EEPROM > + bool "EEPROM Support" > + depends on OF > + help > + Support for EEPROM alike devices. > + > + This framework is designed to provide a generic interface to EEPROM > + from both the Linux Kernel and the userspace. > + > + If unsure, say no. > + > +if EEPROM > + > +config EEPROM_DEBUG > + bool "EEPROM debug support" > + help > + Say yes here to enable debugging support. > + > +endif > diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile > new file mode 100644 > index 0000000..e130079 > --- /dev/null > +++ b/drivers/eeprom/Makefile > @@ -0,0 +1,9 @@ > +# > +# Makefile for eeprom drivers. > +# > + > +ccflags-$(CONFIG_EEPROM_DEBUG) += -DDEBUG > + > +obj-$(CONFIG_EEPROM) += core.o > + > +# Devices > diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c > new file mode 100644 > index 0000000..bc877a6 > --- /dev/null > +++ b/drivers/eeprom/core.c > @@ -0,0 +1,290 @@ > +/* > + * EEPROM framework core. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/device.h> > +#include <linux/eeprom-provider.h> > +#include <linux/eeprom-consumer.h> > +#include <linux/export.h> > +#include <linux/fs.h> > +#include <linux/idr.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/slab.h> > +#include <linux/uaccess.h> > + > +struct eeprom_cell { > + struct eeprom_device *eeprom; > + loff_t offset; > + size_t count; > +}; > + > +static DEFINE_MUTEX(eeprom_list_mutex); > +static LIST_HEAD(eeprom_list); > +static DEFINE_IDA(eeprom_ida); > + > +static ssize_t bin_attr_eeprom_read_write(struct kobject *kobj, > + char *buf, loff_t offset, > + size_t count, bool read) > +{ > + struct device *dev = container_of(kobj, struct device, kobj); > + struct eeprom_device *eeprom = container_of(dev, struct eeprom_device, > + edev); > + int rc; > + > + if (offset > eeprom->size) > + return 0; > + > + if (offset + count > eeprom->size) > + count = eeprom->size - offset; > + > + if (read) > + rc = regmap_bulk_read(eeprom->regmap, offset, > + buf, count/eeprom->stride); > + else > + rc = regmap_bulk_write(eeprom->regmap, offset, > + buf, count/eeprom->stride); > + > + if (IS_ERR_VALUE(rc)) > + return 0; > + > + return count; > +} > + > +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t offset, size_t count) > +{ > + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true); > +} > + > +static ssize_t bin_attr_eeprom_write(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t offset, size_t count) > +{ > + return bin_attr_eeprom_read_write(kobj, buf, offset, count, false); > +} > + > +static struct bin_attribute bin_attr_eeprom = { > + .attr = { > + .name = "eeprom", > + .mode = 0660, > + }, > + .read = bin_attr_eeprom_read, > + .write = bin_attr_eeprom_write, > +}; > + > +static struct bin_attribute *eeprom_bin_attributes[] = { > + &bin_attr_eeprom, > + NULL, > +}; > + > +static const struct attribute_group eeprom_bin_group = { > + .bin_attrs = eeprom_bin_attributes, > +}; > + > +static const struct attribute_group *eeprom_dev_groups[] = { > + &eeprom_bin_group, > + NULL, > +}; > + > +static struct class eeprom_class = { > + .name = "eeprom", > + .dev_groups = eeprom_dev_groups, > +}; > + > +int eeprom_register(struct eeprom_device *eeprom) > +{ > + int rval; > + > + if (!eeprom->regmap || !eeprom->size) { > + dev_err(eeprom->dev, "Regmap not found\n"); > + return -EINVAL; > + } > + > + eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL); > + if (eeprom->id < 0) > + return eeprom->id; > + > + eeprom->edev.class = &eeprom_class; > + eeprom->edev.parent = eeprom->dev; > + eeprom->edev.of_node = eeprom->dev ? eeprom->dev->of_node : NULL; > + dev_set_name(&eeprom->edev, "eeprom%d", eeprom->id); > + > + device_initialize(&eeprom->edev); > + > + dev_dbg(&eeprom->edev, "Registering eeprom device %s\n", > + dev_name(&eeprom->edev)); > + > + rval = device_add(&eeprom->edev); > + if (rval) > + return rval; > + > + mutex_lock(&eeprom_list_mutex); > + list_add(&eeprom->list, &eeprom_list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(eeprom_register); > + > +int eeprom_unregister(struct eeprom_device *eeprom) > +{ > + device_del(&eeprom->edev); > + > + mutex_lock(&eeprom_list_mutex); > + list_del(&eeprom->list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(eeprom_unregister); > + > +static struct eeprom_cell *__eeprom_cell_get(struct device_node *node, > + int index) > +{ > + struct of_phandle_args args; > + struct eeprom_cell *cell; > + struct eeprom_device *e, *eeprom = NULL; > + int ret; > + > + ret = of_parse_phandle_with_args(node, "eeproms", > + "#eeprom-cells", index, &args); > + if (ret) > + return ERR_PTR(ret); > + > + if (args.args_count != 2) > + return ERR_PTR(-EINVAL); > + > + mutex_lock(&eeprom_list_mutex); > + > + list_for_each_entry(e, &eeprom_list, list) { > + if (args.np == e->edev.of_node) { > + eeprom = e; > + break; > + } > + } > + mutex_unlock(&eeprom_list_mutex); > + > + if (!eeprom) > + return ERR_PTR(-EPROBE_DEFER); > + > + cell = kzalloc(sizeof(*cell), GFP_KERNEL); > + if (!cell) > + return ERR_PTR(-ENOMEM); > + > + cell->eeprom = eeprom; > + cell->offset = args.args[0]; > + cell->count = args.args[1]; > + > + return cell; > +} > + > +static struct eeprom_cell *__eeprom_cell_get_byname(struct device_node *node, > + const char *id) > +{ > + int index = 0; > + > + if (id) > + index = of_property_match_string(node, > + "eeprom-names", > + id); > + return __eeprom_cell_get(node, index); > + > +} > + > +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index) > +{ > + if (!dev) > + return ERR_PTR(-EINVAL); > + > + /* First, attempt to retrieve the cell through the DT */ > + if (dev->of_node) > + return __eeprom_cell_get(dev->of_node, index); > + > + /* We don't support anything else yet */ > + return ERR_PTR(-ENODEV); > +} > +EXPORT_SYMBOL(eeprom_cell_get); > + > +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, const char *id) > +{ > + if (!dev) > + return ERR_PTR(-EINVAL); > + > + if (id && dev->of_node) > + return __eeprom_cell_get_byname(dev->of_node, id); > + > + /* We don't support anything else yet */ > + return ERR_PTR(-ENODEV); > +} > +EXPORT_SYMBOL(eeprom_cell_get_byname); > + > +void eeprom_cell_put(struct eeprom_cell *cell) > +{ > + kfree(cell); > +} > +EXPORT_SYMBOL(eeprom_cell_put); > + > +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len) > +{ > + struct eeprom_device *eeprom = cell->eeprom; > + char *buf; > + int rc; > + > + if (!eeprom || !eeprom->regmap) > + return ERR_PTR(-EINVAL); > + > + buf = kzalloc(cell->count, GFP_KERNEL); > + if (!buf) > + return ERR_PTR(-ENOMEM); > + > + rc = regmap_bulk_read(eeprom->regmap, cell->offset, > + buf, cell->count/eeprom->stride); > + if (IS_ERR_VALUE(rc)) { > + kfree(buf); > + return ERR_PTR(rc); > + } > + > + *len = cell->count; > + > + return buf; > +} > +EXPORT_SYMBOL(eeprom_cell_read); > + > +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len) > +{ > + struct eeprom_device *eeprom = cell->eeprom; > + > + if (!eeprom || !eeprom->regmap) > + return -EINVAL; > + > + return regmap_bulk_write(eeprom->regmap, cell->offset, > + buf, cell->count/eeprom->stride); > +} > +EXPORT_SYMBOL(eeprom_cell_write); > + > +static int eeprom_init(void) > +{ > + return class_register(&eeprom_class); > +} > + > +static void eeprom_exit(void) > +{ > + class_unregister(&eeprom_class); > +} > + > +subsys_initcall(eeprom_init); > +module_exit(eeprom_exit); > + > +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com"); > +MODULE_DESCRIPTION("EEPROM Driver Core"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/eeprom-consumer.h b/include/linux/eeprom-consumer.h > new file mode 100644 > index 0000000..706ae9d > --- /dev/null > +++ b/include/linux/eeprom-consumer.h > @@ -0,0 +1,73 @@ > +/* > + * EEPROM framework consumer. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#ifndef _LINUX_EEPROM_CONSUMER_H > +#define _LINUX_EEPROM_CONSUMER_H > + > +struct eeprom_cell; > + > +/** > + * eeprom_cell_get(): Get eeprom cell of device form a given index. > + * > + * @dev: Device that will be interacted with > + * @index: Index of the eeprom cell. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a struct eeprom_cell. The eeprom_cell will be freed by the > + * eeprom_cell_put(). > + */ > +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index); > + > +/** > + * eeprom_cell_get(): Get eeprom cell of device form a given name. > + * > + * @dev: Device that will be interacted with > + * @name: Name of the eeprom cell. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a struct eeprom_cell. The eeprom_cell will be freed by the > + * eeprom_cell_put(). > + */ > +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, > + const char *name); > + > +/** > + * eeprom_cell_put(): Release previously allocated eeprom cell. > + * > + * @cell: Previously allocated eeprom cell by eeprom_cell_get() > + * or eeprom_cell_get_byname(). > + */ > +void eeprom_cell_put(struct eeprom_cell *cell); > + > +/** > + * eeprom_cell_read(): Read a given eeprom cell > + * > + * @cell: eeprom cell to be read. > + * @len: pointer to length of cell which will be populated on successful read. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a char * bufffer. The buffer should be freed by the consumer with a > + * kfree(). > + */ > +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len); > + > +/** > + * eeprom_cell_write(): Write to a given eeprom cell > + * > + * @cell: eeprom cell to be written. > + * @buf: Buffer to be written. > + * @len: length of buffer to be written to eeprom cell. > + * > + * The return value will be an non zero on error or a zero on successful write. > + */ > +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len); > + > +#endif /* ifndef _LINUX_EEPROM_CONSUMER_H */ > diff --git a/include/linux/eeprom-provider.h b/include/linux/eeprom-provider.h > new file mode 100644 > index 0000000..3943c2f > --- /dev/null > +++ b/include/linux/eeprom-provider.h > @@ -0,0 +1,51 @@ > +/* > + * EEPROM framework provider. > + * > + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#ifndef _LINUX_EEPROM_PROVIDER_H > +#define _LINUX_EEPROM_PROVIDER_H > + > +#include <linux/device.h> > +#include <linux/regmap.h> > +#include <linux/list.h> > + > +struct eeprom_device { > + struct regmap *regmap; > + int stride; > + size_t size; > + struct device *dev; > + > + /* Internal to framework */ > + struct device edev; > + int id; > + struct list_head list; > +}; > + > +/** > + * eeprom_register(): Register a eeprom device for given eeprom. > + * Also creates an binary entry in /sys/class/eeprom/eeprom[id]/eeprom > + * > + * @eeprom: eeprom device that needs to be created > + * > + * The return value will be an error code on error or a zero on success. > + * The eeprom_device and sysfs entery will be freed by the eeprom_unregister(). > + */ > +int eeprom_register(struct eeprom_device *eeprom); > + > +/** > + * eeprom_unregister(): Unregister previously registered eeprom device > + * > + * @eeprom: Pointer to previously registered eeprom device. > + * > + * The return value will be an non zero on error or a zero on success. > + */ > +int eeprom_unregister(struct eeprom_device *eeprom); > + > +#endif /* ifndef _LINUX_EEPROM_PROVIDER_H */ > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote: > +static struct class eeprom_class = { > + .name = "eeprom", > + .dev_groups = eeprom_dev_groups, > +}; > + > +int eeprom_register(struct eeprom_device *eeprom) > +{ > + int rval; > + > + if (!eeprom->regmap || !eeprom->size) { > + dev_err(eeprom->dev, "Regmap not found\n"); > + return -EINVAL; > + } > + > + eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL); > + if (eeprom->id < 0) > + return eeprom->id; > + > + eeprom->edev.class = &eeprom_class; > + eeprom->edev.parent = eeprom->dev; > + eeprom->edev.of_node = eeprom->dev ? eeprom->dev->of_node : NULL; > + dev_set_name(&eeprom->edev, "eeprom%d", eeprom->id); > + > + device_initialize(&eeprom->edev); > + > + dev_dbg(&eeprom->edev, "Registering eeprom device %s\n", > + dev_name(&eeprom->edev)); > + > + rval = device_add(&eeprom->edev); > + if (rval) > + return rval; > + > + mutex_lock(&eeprom_list_mutex); > + list_add(&eeprom->list, &eeprom_list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(eeprom_register); > + > +int eeprom_unregister(struct eeprom_device *eeprom) > +{ > + device_del(&eeprom->edev); > + > + mutex_lock(&eeprom_list_mutex); > + list_del(&eeprom->list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; There's problems with this. "edev" is embedded into eeprom, and "edev" is a refcounted structure - it has a lifetime, and the lifetime of eeprom is thus determined by edev. What this means is that calling eeprom_unregister() and then freeing the eeprom structure is a potentially unsafe operation - the memory backing edev must only be freed when the release method for the struct device has been called. > +struct eeprom_device { > + struct regmap *regmap; > + int stride; > + size_t size; > + struct device *dev; Failing to understand and address the driver model life time issues is a major blocker for this patch. Sorry.
On 20/02/15 17:46, Russell King - ARM Linux wrote: > On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote: >> +static struct class eeprom_class = { >> + .name = "eeprom", >> + .dev_groups = eeprom_dev_groups, >> +}; >> + >> +int eeprom_register(struct eeprom_device *eeprom) >> +{ >> + int rval; >> + >> + if (!eeprom->regmap || !eeprom->size) { >> + dev_err(eeprom->dev, "Regmap not found\n"); >> + return -EINVAL; >> + } >> + >> + eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL); >> + if (eeprom->id < 0) >> + return eeprom->id; >> + >> + eeprom->edev.class = &eeprom_class; >> + eeprom->edev.parent = eeprom->dev; >> + eeprom->edev.of_node = eeprom->dev ? eeprom->dev->of_node : NULL; >> + dev_set_name(&eeprom->edev, "eeprom%d", eeprom->id); >> + >> + device_initialize(&eeprom->edev); >> + >> + dev_dbg(&eeprom->edev, "Registering eeprom device %s\n", >> + dev_name(&eeprom->edev)); >> + >> + rval = device_add(&eeprom->edev); >> + if (rval) >> + return rval; >> + >> + mutex_lock(&eeprom_list_mutex); >> + list_add(&eeprom->list, &eeprom_list); >> + mutex_unlock(&eeprom_list_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(eeprom_register); >> + >> +int eeprom_unregister(struct eeprom_device *eeprom) >> +{ >> + device_del(&eeprom->edev); >> + >> + mutex_lock(&eeprom_list_mutex); >> + list_del(&eeprom->list); >> + mutex_unlock(&eeprom_list_mutex); >> + >> + return 0; > > There's problems with this. "edev" is embedded into eeprom, and "edev" > is a refcounted structure - it has a lifetime, and the lifetime of > eeprom is thus determined by edev. > > What this means is that calling eeprom_unregister() and then freeing > the eeprom structure is a potentially unsafe operation - the memory > backing edev must only be freed when the release method for the > struct device has been called. Thats a good catch, Yes I see the issue. Moving the struct eeprom_device allocation to eeprom core.c should address this issue. This makes eeprom self contained and can safely free the eeprom_device memory in eeprom_class.eeprom_release(). Will fix this in next version. > >> +struct eeprom_device { >> + struct regmap *regmap; >> + int stride; >> + size_t size; >> + struct device *dev; > > Failing to understand and address the driver model life time issues is > a major blocker for this patch. Sorry. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/02/15 17:21, Rob Herring wrote: > On Thu, Feb 19, 2015 at 11:08 AM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> From: Maxime Ripard <maxime.ripard@free-electrons.com> >> >> Up until now, EEPROM drivers were stored in drivers/misc, where they all had to >> duplicate pretty much the same code to register a sysfs file, allow in-kernel >> users to access the content of the devices they were driving, etc. >> >> This was also a problem as far as other in-kernel users were involved, since >> the solutions used were pretty much different from on driver to another, there >> was a rather big abstraction leak. >> >> This introduction of this framework aims at solving this. It also introduces DT >> representation for consumer devices to go get the data they require (MAC >> Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs. >> >> Having regmap interface to this framework would give much better >> abstraction for eeproms on different buses. >> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> [srinivas.kandagatla: Moved to regmap based and cleanedup apis] >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ >> drivers/Kconfig | 2 + >> drivers/Makefile | 1 + >> drivers/eeprom/Kconfig | 19 ++ >> drivers/eeprom/Makefile | 9 + >> drivers/eeprom/core.c | 290 +++++++++++++++++++++ >> include/linux/eeprom-consumer.h | 73 ++++++ >> include/linux/eeprom-provider.h | 51 ++++ > > Who is going to be the maintainer for this? Am happy to be one. > >> 8 files changed, 493 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/eeprom/eeprom.txt >> create mode 100644 drivers/eeprom/Kconfig >> create mode 100644 drivers/eeprom/Makefile >> create mode 100644 drivers/eeprom/core.c >> create mode 100644 include/linux/eeprom-consumer.h >> create mode 100644 include/linux/eeprom-provider.h >> >> diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt >> new file mode 100644 >> index 0000000..9ec1ec2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt > > Please make bindings a separate patch. Sure, Will do it in next version. > >> @@ -0,0 +1,48 @@ >> += EEPROM Data Device Tree Bindings = >> + >> +This binding is intended to represent the location of hardware >> +configuration data stored in EEPROMs. >> + >> +On a significant proportion of boards, the manufacturer has stored >> +some data on an EEPROM-like device, for the OS to be able to retrieve >> +these information and act upon it. Obviously, the OS has to know >> +about where to retrieve these data from, and where they are stored on >> +the storage device. >> + >> +This document is here to document this. >> + >> += Data providers = >> + >> +Required properties: >> +#eeprom-cells: Number of cells in an eeprom specifier; The common >> + case is 2. > > We already have eeproms in DTs, it would be nice to be able to support > them with this framework as well. Yes, I can see more than 60% of them are atmel,at24* eeproms in DT. We have some plans to migrate at24 and at25 eeproms to this framework once the the framework itself is accepted. > >> + >> +For example: >> + >> + at24: eeprom@42 { >> + #eeprom-cells = <2>; >> + }; >> + >> += Data consumers = >> + >> +Required properties: >> + >> +eeproms: List of phandle and data cell specifier triplet, one triplet >> + for each data cell the device might be interested in. The >> + triplet consists of the phandle to the eeprom provider, then >> + the offset in byte within that storage device, and the length >> + in byte of the data we care about. > > The problem with this is it assumes you know who the consumer is and > that it is a DT node. For example, how would you describe a serial > number? Correct me if I miss understood. Is serial number any different? Am hoping that the eeprom consumer would be aware of offset and size of serial number in the eeprom Cant the consumer do: eeprom-consumer { eeproms = <&at24 0 4>; eeprom-names = "device-serial-number"; }; --srini > > Rob > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 20, 2015 at 1:25 PM, Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > > > On 20/02/15 17:21, Rob Herring wrote: >> >> On Thu, Feb 19, 2015 at 11:08 AM, Srinivas Kandagatla >> <srinivas.kandagatla@linaro.org> wrote: >>> >>> From: Maxime Ripard <maxime.ripard@free-electrons.com> >>> >>> Up until now, EEPROM drivers were stored in drivers/misc, where they all >>> had to >>> duplicate pretty much the same code to register a sysfs file, allow >>> in-kernel >>> users to access the content of the devices they were driving, etc. >>> >>> This was also a problem as far as other in-kernel users were involved, >>> since >>> the solutions used were pretty much different from on driver to another, >>> there >>> was a rather big abstraction leak. >>> >>> This introduction of this framework aims at solving this. It also >>> introduces DT >>> representation for consumer devices to go get the data they require (MAC >>> Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs. >>> >>> Having regmap interface to this framework would give much better >>> abstraction for eeproms on different buses. >>> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>> [srinivas.kandagatla: Moved to regmap based and cleanedup apis] >>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>> --- >>> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ >>> drivers/Kconfig | 2 + >>> drivers/Makefile | 1 + >>> drivers/eeprom/Kconfig | 19 ++ >>> drivers/eeprom/Makefile | 9 + >>> drivers/eeprom/core.c | 290 >>> +++++++++++++++++++++ >>> include/linux/eeprom-consumer.h | 73 ++++++ >>> include/linux/eeprom-provider.h | 51 ++++ >> >> >> Who is going to be the maintainer for this? > > > Am happy to be one. So please add a MAINTAINERS entry. [...] >>> += Data consumers = >>> + >>> +Required properties: >>> + >>> +eeproms: List of phandle and data cell specifier triplet, one triplet >>> + for each data cell the device might be interested in. The >>> + triplet consists of the phandle to the eeprom provider, then >>> + the offset in byte within that storage device, and the length >>> + in byte of the data we care about. >> >> >> The problem with this is it assumes you know who the consumer is and >> that it is a DT node. For example, how would you describe a serial >> number? > > Correct me if I miss understood. > Is serial number any different? > Am hoping that the eeprom consumer would be aware of offset and size of > serial number in the eeprom > > Cant the consumer do: > > eeprom-consumer { > eeproms = <&at24 0 4>; > eeprom-names = "device-serial-number"; Yes, but who is "eeprom-consumer"? DT nodes generally describe a h/w block, but it this case, the consumer depends on the OS, not the h/w. I'm not saying you can't describe where things are, but I don't think you should imply who is the consumer and doing so is unnecessarily complicated. Also, the layout of EEPROM is likely very much platform specific. Some could have a more complex structure perhaps with key ids and linked list structure. I would do something more simple that is just a list of keys and their location like this: device-serial-number = <start size>; key1 = <start size>; key2 = <start size>; Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/02/15 22:01, Rob Herring wrote: > On Fri, Feb 20, 2015 at 1:25 PM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: >> >> >> On 20/02/15 17:21, Rob Herring wrote: >>> >>> On Thu, Feb 19, 2015 at 11:08 AM, Srinivas Kandagatla >>> <srinivas.kandagatla@linaro.org> wrote: >>>> >>>> From: Maxime Ripard <maxime.ripard@free-electrons.com> >>>> >>>> Up until now, EEPROM drivers were stored in drivers/misc, where they all >>>> had to >>>> duplicate pretty much the same code to register a sysfs file, allow >>>> in-kernel >>>> users to access the content of the devices they were driving, etc. >>>> >>>> This was also a problem as far as other in-kernel users were involved, >>>> since >>>> the solutions used were pretty much different from on driver to another, >>>> there >>>> was a rather big abstraction leak. >>>> >>>> This introduction of this framework aims at solving this. It also >>>> introduces DT >>>> representation for consumer devices to go get the data they require (MAC >>>> Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs. >>>> >>>> Having regmap interface to this framework would give much better >>>> abstraction for eeproms on different buses. >>>> >>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>> [srinivas.kandagatla: Moved to regmap based and cleanedup apis] >>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>>> --- >>>> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ >>>> drivers/Kconfig | 2 + >>>> drivers/Makefile | 1 + >>>> drivers/eeprom/Kconfig | 19 ++ >>>> drivers/eeprom/Makefile | 9 + >>>> drivers/eeprom/core.c | 290 >>>> +++++++++++++++++++++ >>>> include/linux/eeprom-consumer.h | 73 ++++++ >>>> include/linux/eeprom-provider.h | 51 ++++ >>> >>> >>> Who is going to be the maintainer for this? >> >> >> Am happy to be one. > > So please add a MAINTAINERS entry. Yep, I will do that in next version. > > [...] > >>>> += Data consumers = >>>> + >>>> +Required properties: >>>> + >>>> +eeproms: List of phandle and data cell specifier triplet, one triplet >>>> + for each data cell the device might be interested in. The >>>> + triplet consists of the phandle to the eeprom provider, then >>>> + the offset in byte within that storage device, and the length >>>> + in byte of the data we care about. >>> >>> >>> The problem with this is it assumes you know who the consumer is and >>> that it is a DT node. For example, how would you describe a serial >>> number? >> >> Correct me if I miss understood. >> Is serial number any different? >> Am hoping that the eeprom consumer would be aware of offset and size of >> serial number in the eeprom >> >> Cant the consumer do: >> >> eeprom-consumer { >> eeproms = <&at24 0 4>; >> eeprom-names = "device-serial-number"; > > Yes, but who is "eeprom-consumer"? DT nodes generally describe a h/w > block, but it this case, the consumer depends on the OS, not the h/w. Consumer could be any driver for the IP on the SOC, for example an ethernet driver which needs Mac Address from eeprom or an thermal sensor which requires cablibration values or an cpufreq driver which requires OPP settings. Am not sure who could be the consumer for serial number, I guess it should some soc specific driver. > I'm not saying you can't describe where things are, but I don't think > you should imply who is the consumer and doing so is unnecessarily > complicated. > > Also, the layout of EEPROM is likely very much platform specific. Some > could have a more complex structure perhaps with key ids and linked > list structure. I agree, the data layout is very specific to platform and could vary in complexity. This simple framework is attempting to solve most common usecase where in the consumer drivers like thermal-sensor/network/cpufreq needs to read an location in the eeprom. Am sure we can find a way to accommodate the complex layout as well. > > I would do something more simple that is just a list of keys and their > location like this: > > device-serial-number = <start size>; > key1 = <start size>; > key2 = <start size>; > There are pros and cons doing it as list of keys. One reason for doing it as fixed properties("eeproms", "eemprom-names") is "consistency and familiarity" like interrupts, regs, dmas, clocks, pinctrl, reset, pwm have fixed property names, trying to get most subsystems to do it the same way makes it easier for people to write dts files. --srini > Rob > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote: > [...] > > >>> += Data consumers = > >>> + > >>> +Required properties: > >>> + > >>> +eeproms: List of phandle and data cell specifier triplet, one triplet > >>> + for each data cell the device might be interested in. The > >>> + triplet consists of the phandle to the eeprom provider, then > >>> + the offset in byte within that storage device, and the length > >>> + in byte of the data we care about. > >> > >> > >> The problem with this is it assumes you know who the consumer is and > >> that it is a DT node. For example, how would you describe a serial > >> number? > > > > Correct me if I miss understood. > > Is serial number any different? > > Am hoping that the eeprom consumer would be aware of offset and size of > > serial number in the eeprom > > > > Cant the consumer do: > > > > eeprom-consumer { > > eeproms = <&at24 0 4>; > > eeprom-names = "device-serial-number"; > > Yes, but who is "eeprom-consumer"? Any device that should lookup values in one of the EEPROM. > DT nodes generally describe a h/w block, but it this case, the > consumer depends on the OS, not the h/w. Not really, or at least, not more than any similar binding we currently have. The fact that a MAC-address for the first ethernet chip is stored at a given offset in a given eeprom has nothing to do with the OS. > I'm not saying you can't describe where things are, but I don't > think you should imply who is the consumer and doing so is > unnecessarily complicated. If you only take the case of a serial number, indeed. If you take other usage into account, you can't really do without it. > Also, the layout of EEPROM is likely very much platform specific. Indeed, which is why it should be in the DT. > Some could have a more complex structure perhaps with key ids and > linked list structure. Then just request the size of the whole list, and parse it afterwards in your driver? > I would do something more simple that is just a list of keys and their > location like this: > > device-serial-number = <start size>; > key1 = <start size>; > key2 = <start size>; I'm sorry, but what's the difference? Maxime
On Sat, Feb 21, 2015 at 11:31:49AM +0000, Srinivas Kandagatla wrote: > >I would do something more simple that is just a list of keys and their > >location like this: > > > >device-serial-number = <start size>; > >key1 = <start size>; > >key2 = <start size>; > > > There are pros and cons doing it as list of keys. > > One reason for doing it as fixed properties("eeproms", "eemprom-names") is > "consistency and familiarity" like interrupts, regs, dmas, clocks, pinctrl, > reset, pwm have fixed property names, trying to get most subsystems to do it > the same way makes it easier for people to write dts files. And eeprom-names was something that was asked for last time we discussed it (I don't really remember who though, maybe Arnd?) Maxime
On Sun, Feb 22, 2015 at 8:32 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote: >> [...] >> >> >>> += Data consumers = >> >>> + >> >>> +Required properties: >> >>> + >> >>> +eeproms: List of phandle and data cell specifier triplet, one triplet >> >>> + for each data cell the device might be interested in. The >> >>> + triplet consists of the phandle to the eeprom provider, then >> >>> + the offset in byte within that storage device, and the length >> >>> + in byte of the data we care about. >> >> >> >> >> >> The problem with this is it assumes you know who the consumer is and >> >> that it is a DT node. For example, how would you describe a serial >> >> number? >> > >> > Correct me if I miss understood. >> > Is serial number any different? >> > Am hoping that the eeprom consumer would be aware of offset and size of >> > serial number in the eeprom >> > >> > Cant the consumer do: >> > >> > eeprom-consumer { >> > eeproms = <&at24 0 4>; >> > eeprom-names = "device-serial-number"; >> >> Yes, but who is "eeprom-consumer"? > > Any device that should lookup values in one of the EEPROM. > >> DT nodes generally describe a h/w block, but it this case, the >> consumer depends on the OS, not the h/w. > > Not really, or at least, not more than any similar binding we > currently have. > > The fact that a MAC-address for the first ethernet chip is stored at a > given offset in a given eeprom has nothing to do with the OS. So MAC address would be a valid use to link to a h/w device, but there are certainly cases that don't correspond to a device. >> I'm not saying you can't describe where things are, but I don't >> think you should imply who is the consumer and doing so is >> unnecessarily complicated. > > If you only take the case of a serial number, indeed. If you take > other usage into account, you can't really do without it. > >> Also, the layout of EEPROM is likely very much platform specific. > > Indeed, which is why it should be in the DT. Agreed. I'm not saying the layout should not be. >> Some could have a more complex structure perhaps with key ids and >> linked list structure. > > Then just request the size of the whole list, and parse it afterwards > in your driver? > >> I would do something more simple that is just a list of keys and their >> location like this: >> >> device-serial-number = <start size>; >> key1 = <start size>; >> key2 = <start size>; > > I'm sorry, but what's the difference? It can describe the layout completely whether the fields are tied to a h/w device or not. What I would like to see here is the entire layout described covering both types of fields. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote: > On Fri, Feb 20, 2015 at 1:25 PM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: > > > > > > On 20/02/15 17:21, Rob Herring wrote: > >> > >> On Thu, Feb 19, 2015 at 11:08 AM, Srinivas Kandagatla > >> <srinivas.kandagatla@linaro.org> wrote: > >>> > >>> From: Maxime Ripard <maxime.ripard@free-electrons.com> > >>> > >>> Up until now, EEPROM drivers were stored in drivers/misc, where they all > >>> had to > >>> duplicate pretty much the same code to register a sysfs file, allow > >>> in-kernel > >>> users to access the content of the devices they were driving, etc. > >>> > >>> This was also a problem as far as other in-kernel users were involved, > >>> since > >>> the solutions used were pretty much different from on driver to another, > >>> there > >>> was a rather big abstraction leak. > >>> > >>> This introduction of this framework aims at solving this. It also > >>> introduces DT > >>> representation for consumer devices to go get the data they require (MAC > >>> Addresses, SoC/Revision ID, part numbers, and so on) from the EEPROMs. > >>> > >>> Having regmap interface to this framework would give much better > >>> abstraction for eeproms on different buses. > >>> > >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >>> [srinivas.kandagatla: Moved to regmap based and cleanedup apis] > >>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > >>> --- > >>> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ > >>> drivers/Kconfig | 2 + > >>> drivers/Makefile | 1 + > >>> drivers/eeprom/Kconfig | 19 ++ > >>> drivers/eeprom/Makefile | 9 + > >>> drivers/eeprom/core.c | 290 > >>> +++++++++++++++++++++ > >>> include/linux/eeprom-consumer.h | 73 ++++++ > >>> include/linux/eeprom-provider.h | 51 ++++ > >> > >> > >> Who is going to be the maintainer for this? > > > > > > Am happy to be one. > > So please add a MAINTAINERS entry. > > [...] > > >>> += Data consumers = > >>> + > >>> +Required properties: > >>> + > >>> +eeproms: List of phandle and data cell specifier triplet, one triplet > >>> + for each data cell the device might be interested in. The > >>> + triplet consists of the phandle to the eeprom provider, then > >>> + the offset in byte within that storage device, and the length > >>> + in byte of the data we care about. > >> > >> > >> The problem with this is it assumes you know who the consumer is and > >> that it is a DT node. For example, how would you describe a serial > >> number? > > > > Correct me if I miss understood. > > Is serial number any different? > > Am hoping that the eeprom consumer would be aware of offset and size of > > serial number in the eeprom > > > > Cant the consumer do: > > > > eeprom-consumer { > > eeproms = <&at24 0 4>; > > eeprom-names = "device-serial-number"; > > Yes, but who is "eeprom-consumer"? DT nodes generally describe a h/w > block, but it this case, the consumer depends on the OS, not the h/w. > I'm not saying you can't describe where things are, but I don't think > you should imply who is the consumer and doing so is unnecessarily > complicated. > > Also, the layout of EEPROM is likely very much platform specific. Some > could have a more complex structure perhaps with key ids and linked > list structure. > > I would do something more simple that is just a list of keys and their > location like this: > > device-serial-number = <start size>; > key1 = <start size>; > key2 = <start size>; Maybe better a node instead of a property. That would allow to use the standard reg property to describe the position in the eeprom. Also it would give consumers in dt a possibility to use a phandle to point to a variable: serial_number: var@c { reg = <0xc 0x8>; name = <?>; }; Sascha
On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote: > .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/eeprom/Kconfig | 19 ++ > drivers/eeprom/Makefile | 9 + > drivers/eeprom/core.c | 290 +++++++++++++++++++++ > include/linux/eeprom-consumer.h | 73 ++++++ > include/linux/eeprom-provider.h | 51 ++++ This seems to have a bunch of different things in it - there's some binding, there's some framework code, there's some user code for the framework... splitting it up more would probably help with review. I'd at least make sure the framework is split from the DT code, that will cut down on the bulk and help make sure the framework isn't too DT tied. > + if (read) > + rc = regmap_bulk_read(eeprom->regmap, offset, > + buf, count/eeprom->stride); > + else > + rc = regmap_bulk_write(eeprom->regmap, offset, > + buf, count/eeprom->stride); > + > + if (IS_ERR_VALUE(rc)) > + return 0; > + > + return count; > +} > +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t offset, size_t count) > +{ > + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true); > +} I'm not sure the factoring out is actually helping the clarity here TBH. > +int eeprom_unregister(struct eeprom_device *eeprom) > +{ > + device_del(&eeprom->edev); > + > + mutex_lock(&eeprom_list_mutex); > + list_del(&eeprom->list); > + mutex_unlock(&eeprom_list_mutex); > + > + return 0; > +} > +EXPORT_SYMBOL(eeprom_unregister); Here we return having dropped the lock without doing anything else with the eeprom, meaning the caller could delete it. > + mutex_lock(&eeprom_list_mutex); > + > + list_for_each_entry(e, &eeprom_list, list) { > + if (args.np == e->edev.of_node) { > + eeprom = e; > + break; > + } > + } > + mutex_unlock(&eeprom_list_mutex); Here we iterate the list, find the relevant eeprom and drop the lock while still holding a reference to the eeprom we just found. This means that a removal could race with us and free the eeprom underneath us. I'm also not seeing anything stopping or even noticing a device being removed with a cell active on it. > +/** > + * eeprom_cell_get(): Get eeprom cell of device form a given index. > + * > + * @dev: Device that will be interacted with > + * @index: Index of the eeprom cell. > + * > + * The return value will be an ERR_PTR() on error or a valid pointer > + * to a struct eeprom_cell. The eeprom_cell will be freed by the > + * eeprom_cell_put(). > + */ > +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index); Normally the kerneldoc goes with the function definition, not the prototype.
Thankyou for the comments. On 23/02/15 15:04, Mark Brown wrote: > On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote: > >> .../devicetree/bindings/eeprom/eeprom.txt | 48 ++++ >> drivers/Kconfig | 2 + >> drivers/Makefile | 1 + >> drivers/eeprom/Kconfig | 19 ++ >> drivers/eeprom/Makefile | 9 + >> drivers/eeprom/core.c | 290 +++++++++++++++++++++ >> include/linux/eeprom-consumer.h | 73 ++++++ >> include/linux/eeprom-provider.h | 51 ++++ > > This seems to have a bunch of different things in it - there's some > binding, there's some framework code, there's some user code for the > framework... splitting it up more would probably help with review. > I'd at least make sure the framework is split from the DT code, that > will cut down on the bulk and help make sure the framework isn't too DT > tied. Yes I agree, will make sure that I split it into different patches in next version. > >> + if (read) >> + rc = regmap_bulk_read(eeprom->regmap, offset, >> + buf, count/eeprom->stride); >> + else >> + rc = regmap_bulk_write(eeprom->regmap, offset, >> + buf, count/eeprom->stride); >> + >> + if (IS_ERR_VALUE(rc)) >> + return 0; >> + >> + return count; >> +} > >> +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *attr, >> + char *buf, loff_t offset, size_t count) >> +{ >> + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true); >> +} > > I'm not sure the factoring out is actually helping the clarity here TBH. > ok. >> +int eeprom_unregister(struct eeprom_device *eeprom) >> +{ >> + device_del(&eeprom->edev); >> + >> + mutex_lock(&eeprom_list_mutex); >> + list_del(&eeprom->list); >> + mutex_unlock(&eeprom_list_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(eeprom_unregister); > > Here we return having dropped the lock without doing anything else with > the eeprom, meaning the caller could delete it. > >> + mutex_lock(&eeprom_list_mutex); >> + >> + list_for_each_entry(e, &eeprom_list, list) { >> + if (args.np == e->edev.of_node) { >> + eeprom = e; >> + break; >> + } >> + } >> + mutex_unlock(&eeprom_list_mutex); > > Here we iterate the list, find the relevant eeprom and drop the lock > while still holding a reference to the eeprom we just found. This means > that a removal could race with us and free the eeprom underneath us. > I'm also not seeing anything stopping or even noticing a device being > removed with a cell active on it. > As suggested by Stephen Boyd reference counting on eeprom should ensure safe removal of eeprom. >> +/** >> + * eeprom_cell_get(): Get eeprom cell of device form a given index. >> + * >> + * @dev: Device that will be interacted with >> + * @index: Index of the eeprom cell. >> + * >> + * The return value will be an ERR_PTR() on error or a valid pointer >> + * to a struct eeprom_cell. The eeprom_cell will be freed by the >> + * eeprom_cell_put(). >> + */ >> +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index); > > Normally the kerneldoc goes with the function definition, not the > prototype. Thats true, will fix it in next version. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/22/15 16:57, Rob Herring wrote: > On Sun, Feb 22, 2015 at 8:32 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: >> On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote: >>> [...] >>> >>>>>> += Data consumers = >>>>>> + >>>>>> +Required properties: >>>>>> + >>>>>> +eeproms: List of phandle and data cell specifier triplet, one triplet >>>>>> + for each data cell the device might be interested in. The >>>>>> + triplet consists of the phandle to the eeprom provider, then >>>>>> + the offset in byte within that storage device, and the length >>>>>> + in byte of the data we care about. >>>>> >>>>> The problem with this is it assumes you know who the consumer is and >>>>> that it is a DT node. For example, how would you describe a serial >>>>> number? >>>> Correct me if I miss understood. >>>> Is serial number any different? >>>> Am hoping that the eeprom consumer would be aware of offset and size of >>>> serial number in the eeprom >>>> >>>> Cant the consumer do: >>>> >>>> eeprom-consumer { >>>> eeproms = <&at24 0 4>; >>>> eeprom-names = "device-serial-number"; >>> Yes, but who is "eeprom-consumer"? >> Any device that should lookup values in one of the EEPROM. >> >>> DT nodes generally describe a h/w block, but it this case, the >>> consumer depends on the OS, not the h/w. >> Not really, or at least, not more than any similar binding we >> currently have. >> >> The fact that a MAC-address for the first ethernet chip is stored at a >> given offset in a given eeprom has nothing to do with the OS. > So MAC address would be a valid use to link to a h/w device, but there > are certainly cases that don't correspond to a device. > >>> I'm not saying you can't describe where things are, but I don't >>> think you should imply who is the consumer and doing so is >>> unnecessarily complicated. >> If you only take the case of a serial number, indeed. If you take >> other usage into account, you can't really do without it. >> >>> Also, the layout of EEPROM is likely very much platform specific. >> Indeed, which is why it should be in the DT. > Agreed. I'm not saying the layout should not be. > >>> Some could have a more complex structure perhaps with key ids and >>> linked list structure. >> Then just request the size of the whole list, and parse it afterwards >> in your driver? >> >>> I would do something more simple that is just a list of keys and their >>> location like this: >>> >>> device-serial-number = <start size>; >>> key1 = <start size>; >>> key2 = <start size>; >> I'm sorry, but what's the difference? > It can describe the layout completely whether the fields are tied to a > h/w device or not. > > What I would like to see here is the entire layout described covering > both types of fields. > I was thinking the DT might be like this on the provider side: qfprom@1000000 { reg = <0x1000000 0x1000>; ranges = <0 0x1000000 0x1000>; compatible = "qcom,qfprom-msm8960" pvs-data: pvs-data@40 { compatible = "qcom,pvs-a"; reg = <0x40 0x20>, #eeprom-cells = <0>; }; tsens-data: tmdata@10 { compatible = "qcom,tsens-data-msm8960"; reg = <0x10 4>, <0x16 4>; #eeprom-cells = <0>; }; serial-number: serial@50 { compatible = "qcom,serial-msm8960"; reg = <0x50 4>, <0x60 4>; #eeprom-cells = <0>; }; }; and then on the consumer side: device { eeproms = <&serial-number>; eeprom-names = "soc-rev-id"; }; This would solve a problem where the consumer device is some standard off-the-shelf IP block that needs to get some SoC specific calibration data from the eeprom. I may want to interpret the bits differently depending on which eeprom is connected to my SoC. Sometimes that data format may be the same across many variations of the SoC (e.g. the qcom,pvs-a node) or it may be completely different for a given SoC (e.g. qcom,serial-msm8960 node). I imagine for other SoCs out there it could be different depending on which eeprom the board manufacturer decides to wire onto their board and how they choose to program the data. So this is where I think the eeprom-cells and offset + length starts to fall apart. It forces us to make up a bunch of different compatible strings for our consumer device just so that we can parse the eeprom that we decided to use for some SoC/board specific data. Instead I'd like to see some framework that expresses exactly which eeprom is on my board and how to interpret the bits in a way that doesn't require me to keep refining the compatible string for my generic IP block. I worry that if we put all those details in DT we'll end up having to describe individual bits like serial-number-bit-2, serial-number-bit-3, etc. because sometimes these pieces of data are scattered all around the eeprom and aren't contiguous or aligned on a byte boundary. It may be easier to just have a way to express that this is an eeprom with this specific layout and my device has data stored in there. Then the driver can be told what layout it is (via compatible or some other string based means if we're not using DT?) and match that up with some driver data if it needs to know how to understand the bits it can read with the eeprom_read() API.
Thanks Stephen for the comments. On 23/02/15 23:11, Stephen Boyd wrote: > On 02/22/15 16:57, Rob Herring wrote: >> On Sun, Feb 22, 2015 at 8:32 AM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >>> On Fri, Feb 20, 2015 at 04:01:55PM -0600, Rob Herring wrote: >>>> [...] >>>> >>>>>>> += Data consumers = >>>>>>> + >>>>>>> +Required properties: >>>>>>> + >>>>>>> +eeproms: List of phandle and data cell specifier triplet, one triplet >>>>>>> + for each data cell the device might be interested in. The >>>>>>> + triplet consists of the phandle to the eeprom provider, then >>>>>>> + the offset in byte within that storage device, and the length >>>>>>> + in byte of the data we care about. >>>>>> >>>>>> The problem with this is it assumes you know who the consumer is and >>>>>> that it is a DT node. For example, how would you describe a serial >>>>>> number? >>>>> Correct me if I miss understood. >>>>> Is serial number any different? >>>>> Am hoping that the eeprom consumer would be aware of offset and size of >>>>> serial number in the eeprom >>>>> >>>>> Cant the consumer do: >>>>> >>>>> eeprom-consumer { >>>>> eeproms = <&at24 0 4>; >>>>> eeprom-names = "device-serial-number"; >>>> Yes, but who is "eeprom-consumer"? >>> Any device that should lookup values in one of the EEPROM. >>> >>>> DT nodes generally describe a h/w block, but it this case, the >>>> consumer depends on the OS, not the h/w. >>> Not really, or at least, not more than any similar binding we >>> currently have. >>> >>> The fact that a MAC-address for the first ethernet chip is stored at a >>> given offset in a given eeprom has nothing to do with the OS. >> So MAC address would be a valid use to link to a h/w device, but there >> are certainly cases that don't correspond to a device. >> >>>> I'm not saying you can't describe where things are, but I don't >>>> think you should imply who is the consumer and doing so is >>>> unnecessarily complicated. >>> If you only take the case of a serial number, indeed. If you take >>> other usage into account, you can't really do without it. >>> >>>> Also, the layout of EEPROM is likely very much platform specific. >>> Indeed, which is why it should be in the DT. >> Agreed. I'm not saying the layout should not be. >> >>>> Some could have a more complex structure perhaps with key ids and >>>> linked list structure. >>> Then just request the size of the whole list, and parse it afterwards >>> in your driver? >>> >>>> I would do something more simple that is just a list of keys and their >>>> location like this: >>>> >>>> device-serial-number = <start size>; >>>> key1 = <start size>; >>>> key2 = <start size>; >>> I'm sorry, but what's the difference? >> It can describe the layout completely whether the fields are tied to a >> h/w device or not. >> >> What I would like to see here is the entire layout described covering >> both types of fields. >> > > I was thinking the DT might be like this on the provider side: > > qfprom@1000000 { > reg = <0x1000000 0x1000>; > ranges = <0 0x1000000 0x1000>; > compatible = "qcom,qfprom-msm8960" > > pvs-data: pvs-data@40 { > compatible = "qcom,pvs-a"; These compatibles could be optional. As it might not be required for devices which are simple and do not require any special interpretation of eeprom data. > reg = <0x40 0x20>, > #eeprom-cells = <0>; Do we still need eeprom-cells if we are moving to use reg property here? > }; > > tsens-data: tmdata@10 { > compatible = "qcom,tsens-data-msm8960"; > reg = <0x10 4>, <0x16 4>; > #eeprom-cells = <0>; > > }; > > serial-number: serial@50 { > compatible = "qcom,serial-msm8960"; > reg = <0x50 4>, <0x60 4>; > #eeprom-cells = <0>; > > }; > }; > > and then on the consumer side: > > device { > eeproms = <&serial-number>; > eeprom-names = "soc-rev-id"; > }; > Yes, this looks good, and Sasha was also recommending something on these lines too. Also this addresses the use cases like serial number too. > > This would solve a problem where the consumer device is some standard > off-the-shelf IP block that needs to get some SoC specific calibration > data from the eeprom. I may want to interpret the bits differently > depending on which eeprom is connected to my SoC. Sometimes that data > format may be the same across many variations of the SoC (e.g. the > qcom,pvs-a node) or it may be completely different for a given SoC (e.g. > qcom,serial-msm8960 node). I imagine for other SoCs out there it could > be different depending on which eeprom the board manufacturer decides to > wire onto their board and how they choose to program the data. > > So this is where I think the eeprom-cells and offset + length starts to > fall apart. It forces us to make up a bunch of different compatible > strings for our consumer device just so that we can parse the eeprom > that we decided to use for some SoC/board specific data. Instead I'd > like to see some framework that expresses exactly which eeprom is on my > board and how to interpret the bits in a way that doesn't require me to > keep refining the compatible string for my generic IP block. > > I worry that if we put all those details in DT we'll end up having to > describe individual bits like serial-number-bit-2, serial-number-bit-3, > etc. because sometimes these pieces of data are scattered all around the > eeprom and aren't contiguous or aligned on a byte boundary. It may be > easier to just have a way to express that this is an eeprom with this > specific layout and my device has data stored in there. Then the driver > can be told what layout it is (via compatible or some other string based > means if we're not using DT?) and match that up with some driver data if > it needs to know how to understand the bits it can read with the > eeprom_read() API. Yes this sounds more sensible to let the consumer driver interpret the eeprom data than the eeprom framework itself. --srini > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 23, 2015 at 03:11:40PM -0800, Stephen Boyd wrote: > >>> I would do something more simple that is just a list of keys and > >>> their location like this: > >>> > >>> device-serial-number = <start size>; > >>> key1 = <start size>; > >>> key2 = <start size>; > >> I'm sorry, but what's the difference? > > It can describe the layout completely whether the fields are tied to a > > h/w device or not. > > > > What I would like to see here is the entire layout described covering > > both types of fields. > > > > I was thinking the DT might be like this on the provider side: > > qfprom@1000000 { > reg = <0x1000000 0x1000>; > ranges = <0 0x1000000 0x1000>; > compatible = "qcom,qfprom-msm8960" > > pvs-data: pvs-data@40 { > compatible = "qcom,pvs-a"; > reg = <0x40 0x20>, > #eeprom-cells = <0>; > }; > > tsens-data: tmdata@10 { > compatible = "qcom,tsens-data-msm8960"; > reg = <0x10 4>, <0x16 4>; > #eeprom-cells = <0>; > > }; > > serial-number: serial@50 { > compatible = "qcom,serial-msm8960"; > reg = <0x50 4>, <0x60 4>; > #eeprom-cells = <0>; > > }; > }; I'm not sure the compatible is really needed. A label of some sort, just like the MTD partitions do would do just fine, and wouldn't have the implicit expectation that a driver will be probed from that node. > and then on the consumer side: > > device { > eeproms = <&serial-number>; > eeprom-names = "soc-rev-id"; > }; > > > This would solve a problem where the consumer device is some standard > off-the-shelf IP block that needs to get some SoC specific calibration > data from the eeprom. I may want to interpret the bits differently > depending on which eeprom is connected to my SoC. Sometimes that data > format may be the same across many variations of the SoC (e.g. the > qcom,pvs-a node) or it may be completely different for a given SoC (e.g. > qcom,serial-msm8960 node). I imagine for other SoCs out there it could > be different depending on which eeprom the board manufacturer decides to > wire onto their board and how they choose to program the data. Oh, so you'd like to infer the data format it's stored in from the compatible? AFAICT, this format will be highly depending on the board itself, rather than on the SoC, do you think it will scale enough? > So this is where I think the eeprom-cells and offset + length starts to > fall apart. It forces us to make up a bunch of different compatible > strings for our consumer device just so that we can parse the eeprom > that we decided to use for some SoC/board specific data. Instead I'd > like to see some framework that expresses exactly which eeprom is on my > board and how to interpret the bits in a way that doesn't require me to > keep refining the compatible string for my generic IP block. Hmmmm, apparently you don't :) > I worry that if we put all those details in DT we'll end up having to > describe individual bits like serial-number-bit-2, serial-number-bit-3, > etc. because sometimes these pieces of data are scattered all around the > eeprom and aren't contiguous or aligned on a byte boundary. It may be > easier to just have a way to express that this is an eeprom with this > specific layout and my device has data stored in there. Then the driver > can be told what layout it is (via compatible or some other string based > means if we're not using DT?) and match that up with some driver data if > it needs to know how to understand the bits it can read with the > eeprom_read() API. I'm half convinced that the layout information will actually work for more complex cases, like the linked list Rob described. If such a structure is ever to be found, it would feel wrong to have that in the EEPROM driver, but it would feel just as wrong to put that in the client driver, that would have to handle the parsing of raw data coming flashed by one single crazy board vendor. Maybe we can have each cell carry a property that defines the format it's stored in, and match that to some parsers plugins, starting with the generic and trivial cases but still allowing for custom parsers to be defined? Something like eeprom@42 { compatible = "atmel,at24something"; reg = <0x42>; serial@0 { label = "board serial"; reg = <0x0 0x10>; format = "packed"; }; opps@10 { label = "board serial"; reg = <0x10 0x10>, <0x40 0x10>, <0x80 0x10>; format = "random-vendor,opp-linked-list"; }; }; That would make eeprom_read always return the same format of data to the client drivers, without cripling the generic EEPROM drivers either. Maxime
On 02/24, Maxime Ripard wrote: > On Mon, Feb 23, 2015 at 03:11:40PM -0800, Stephen Boyd wrote: > > >>> I would do something more simple that is just a list of keys and > > >>> their location like this: > > >>> > > >>> device-serial-number = <start size>; > > >>> key1 = <start size>; > > >>> key2 = <start size>; > > >> I'm sorry, but what's the difference? > > > It can describe the layout completely whether the fields are tied to a > > > h/w device or not. > > > > > > What I would like to see here is the entire layout described covering > > > both types of fields. > > > > > > > I was thinking the DT might be like this on the provider side: > > > > qfprom@1000000 { > > reg = <0x1000000 0x1000>; > > ranges = <0 0x1000000 0x1000>; > > compatible = "qcom,qfprom-msm8960" > > > > pvs-data: pvs-data@40 { > > compatible = "qcom,pvs-a"; > > reg = <0x40 0x20>, > > #eeprom-cells = <0>; > > }; > > > > tsens-data: tmdata@10 { > > compatible = "qcom,tsens-data-msm8960"; > > reg = <0x10 4>, <0x16 4>; > > #eeprom-cells = <0>; > > > > }; > > > > serial-number: serial@50 { > > compatible = "qcom,serial-msm8960"; > > reg = <0x50 4>, <0x60 4>; > > #eeprom-cells = <0>; > > > > }; > > }; > > I'm not sure the compatible is really needed. > > A label of some sort, just like the MTD partitions do would do just > fine, and wouldn't have the implicit expectation that a driver will be > probed from that node. I wasn't aware that compatible meant driver probe. I thought compatible just meant some software entity can understand what I've described within this node. For example, compatible for reserved-memory nodes doesn't mean we're going to probe a device. > > > and then on the consumer side: > > > > device { > > eeproms = <&serial-number>; > > eeprom-names = "soc-rev-id"; > > }; > > > > > > This would solve a problem where the consumer device is some standard > > off-the-shelf IP block that needs to get some SoC specific calibration > > data from the eeprom. I may want to interpret the bits differently > > depending on which eeprom is connected to my SoC. Sometimes that data > > format may be the same across many variations of the SoC (e.g. the > > qcom,pvs-a node) or it may be completely different for a given SoC (e.g. > > qcom,serial-msm8960 node). I imagine for other SoCs out there it could > > be different depending on which eeprom the board manufacturer decides to > > wire onto their board and how they choose to program the data. > > Oh, so you'd like to infer the data format it's stored in from the > compatible? > > AFAICT, this format will be highly depending on the board itself, > rather than on the SoC, do you think it will scale enough? > > > So this is where I think the eeprom-cells and offset + length starts to > > fall apart. It forces us to make up a bunch of different compatible > > strings for our consumer device just so that we can parse the eeprom > > that we decided to use for some SoC/board specific data. Instead I'd > > like to see some framework that expresses exactly which eeprom is on my > > board and how to interpret the bits in a way that doesn't require me to > > keep refining the compatible string for my generic IP block. > > Hmmmm, apparently you don't :) > > > I worry that if we put all those details in DT we'll end up having to > > describe individual bits like serial-number-bit-2, serial-number-bit-3, > > etc. because sometimes these pieces of data are scattered all around the > > eeprom and aren't contiguous or aligned on a byte boundary. It may be > > easier to just have a way to express that this is an eeprom with this > > specific layout and my device has data stored in there. Then the driver > > can be told what layout it is (via compatible or some other string based > > means if we're not using DT?) and match that up with some driver data if > > it needs to know how to understand the bits it can read with the > > eeprom_read() API. > > I'm half convinced that the layout information will actually work for > more complex cases, like the linked list Rob described. > > If such a structure is ever to be found, it would feel wrong to have > that in the EEPROM driver, but it would feel just as wrong to put that > in the client driver, that would have to handle the parsing of raw > data coming flashed by one single crazy board vendor. > > Maybe we can have each cell carry a property that defines the format > it's stored in, and match that to some parsers plugins, starting with > the generic and trivial cases but still allowing for custom parsers to > be defined? > > Something like > > eeprom@42 { > compatible = "atmel,at24something"; > reg = <0x42>; > > serial@0 { > label = "board serial"; > reg = <0x0 0x10>; > format = "packed"; > }; > > opps@10 { > label = "board serial"; > reg = <0x10 0x10>, <0x40 0x10>, <0x80 0x10>; > format = "random-vendor,opp-linked-list"; > }; > }; > > That would make eeprom_read always return the same format of data to > the client drivers, without cripling the generic EEPROM drivers > either. > Is the goal here to make eeprom_read() figure out how to return the next byte of data and hide the parsing logic behind the eeprom APIs? I imagine "random-vendor,opp-linked-list" would be handled by the eeprom driver and that would return OPPs byte by byte across the different reg properties to the eeprom consumer? This approach concerns me because every eeprom_read() call needs to fit the format that the client driver is expecting. How do we validate that? What do we do if we have a random OPP client #1 that expects to get the data from eeprom_read() with OPPs in ascending order and random OPP client #2 that expects to get the data from eeprom_read() with OPPs in descending order? It feels like we're making the eeprom framework too smart without a well defined abstraction. If we were to make it so that eeprom_get_opps() knew what to do and parsed/populated the OPPs, it might work. But if we're just exporting raw data across a read/write API with some implementation specific mangling it sounds like it's going to get messy fast. And if the API is well defined, it would start to become rather large with many different types of data that need to be parsed and sometimes data that's only specific to a single SoC. I wonder how much we could get away with this approach though. If the eeprom driver probed and populated OPPs, made a serial number available via the soc device, and then we made up framework(s) for things like our thermal sensor calibration data and display panel calibration data, I would guess that covers most of my use-cases. The client drivers would need some sort of 'wait for eeprom to populate things' API or we'd need to work that into the new calibration framework.
On 25/02/15 01:30, Stephen Boyd wrote: > On 02/24, Maxime Ripard wrote: >> On Mon, Feb 23, 2015 at 03:11:40PM -0800, Stephen Boyd wrote: >>>>>> I would do something more simple that is just a list of keys and >>>>>> their location like this: >>>>>> >>>>>> device-serial-number = <start size>; >>>>>> key1 = <start size>; >>>>>> key2 = <start size>; >>>>> I'm sorry, but what's the difference? >>>> It can describe the layout completely whether the fields are tied to a >>>> h/w device or not. >>>> >>>> What I would like to see here is the entire layout described covering >>>> both types of fields. >>>> >>> >>> I was thinking the DT might be like this on the provider side: >>> >>> qfprom@1000000 { >>> reg = <0x1000000 0x1000>; >>> ranges = <0 0x1000000 0x1000>; >>> compatible = "qcom,qfprom-msm8960" >>> >>> pvs-data: pvs-data@40 { >>> compatible = "qcom,pvs-a"; >>> reg = <0x40 0x20>, >>> #eeprom-cells = <0>; >>> }; >>> >>> tsens-data: tmdata@10 { >>> compatible = "qcom,tsens-data-msm8960"; >>> reg = <0x10 4>, <0x16 4>; >>> #eeprom-cells = <0>; >>> >>> }; >>> >>> serial-number: serial@50 { >>> compatible = "qcom,serial-msm8960"; >>> reg = <0x50 4>, <0x60 4>; >>> #eeprom-cells = <0>; >>> >>> }; >>> }; >> >> I'm not sure the compatible is really needed. >> >> A label of some sort, just like the MTD partitions do would do just >> fine, and wouldn't have the implicit expectation that a driver will be >> probed from that node. > > I wasn't aware that compatible meant driver probe. I thought > compatible just meant some software entity can understand what > I've described within this node. For example, compatible for > reserved-memory nodes doesn't mean we're going to probe a device. > >> >>> and then on the consumer side: >>> >>> device { >>> eeproms = <&serial-number>; >>> eeprom-names = "soc-rev-id"; >>> }; >>> >>> >>> This would solve a problem where the consumer device is some standard >>> off-the-shelf IP block that needs to get some SoC specific calibration >>> data from the eeprom. I may want to interpret the bits differently >>> depending on which eeprom is connected to my SoC. Sometimes that data >>> format may be the same across many variations of the SoC (e.g. the >>> qcom,pvs-a node) or it may be completely different for a given SoC (e.g. >>> qcom,serial-msm8960 node). I imagine for other SoCs out there it could >>> be different depending on which eeprom the board manufacturer decides to >>> wire onto their board and how they choose to program the data. >> >> Oh, so you'd like to infer the data format it's stored in from the >> compatible? >> >> AFAICT, this format will be highly depending on the board itself, >> rather than on the SoC, do you think it will scale enough? >> >>> So this is where I think the eeprom-cells and offset + length starts to >>> fall apart. It forces us to make up a bunch of different compatible >>> strings for our consumer device just so that we can parse the eeprom >>> that we decided to use for some SoC/board specific data. Instead I'd >>> like to see some framework that expresses exactly which eeprom is on my >>> board and how to interpret the bits in a way that doesn't require me to >>> keep refining the compatible string for my generic IP block. >> >> Hmmmm, apparently you don't :) >> >>> I worry that if we put all those details in DT we'll end up having to >>> describe individual bits like serial-number-bit-2, serial-number-bit-3, >>> etc. because sometimes these pieces of data are scattered all around the >>> eeprom and aren't contiguous or aligned on a byte boundary. It may be >>> easier to just have a way to express that this is an eeprom with this >>> specific layout and my device has data stored in there. Then the driver >>> can be told what layout it is (via compatible or some other string based >>> means if we're not using DT?) and match that up with some driver data if >>> it needs to know how to understand the bits it can read with the >>> eeprom_read() API. >> >> I'm half convinced that the layout information will actually work for >> more complex cases, like the linked list Rob described. >> >> If such a structure is ever to be found, it would feel wrong to have >> that in the EEPROM driver, but it would feel just as wrong to put that >> in the client driver, that would have to handle the parsing of raw >> data coming flashed by one single crazy board vendor. >> >> Maybe we can have each cell carry a property that defines the format >> it's stored in, and match that to some parsers plugins, starting with >> the generic and trivial cases but still allowing for custom parsers to >> be defined? >> >> Something like >> >> eeprom@42 { >> compatible = "atmel,at24something"; >> reg = <0x42>; >> >> serial@0 { >> label = "board serial"; >> reg = <0x0 0x10>; >> format = "packed"; >> }; >> >> opps@10 { >> label = "board serial"; >> reg = <0x10 0x10>, <0x40 0x10>, <0x80 0x10>; >> format = "random-vendor,opp-linked-list"; >> }; >> }; >> >> That would make eeprom_read always return the same format of data to >> the client drivers, without cripling the generic EEPROM drivers >> either. >> > > Is the goal here to make eeprom_read() figure out how to return > the next byte of data and hide the parsing logic behind the > eeprom APIs? I imagine "random-vendor,opp-linked-list" would be > handled by the eeprom driver and that would return OPPs byte by > byte across the different reg properties to the eeprom consumer? > > This approach concerns me because every eeprom_read() call needs > to fit the format that the client driver is expecting. How do we > validate that? What do we do if we have a random OPP client #1 > that expects to get the data from eeprom_read() with OPPs in > ascending order and random OPP client #2 that expects to get the > data from eeprom_read() with OPPs in descending order? > > It feels like we're making the eeprom framework too smart without > a well defined abstraction. If we were to make it so that > eeprom_get_opps() knew what to do and parsed/populated the OPPs, > it might work. But if we're just exporting raw data across a > read/write API with some implementation specific mangling it > sounds like it's going to get messy fast. And if the API is well > defined, it would start to become rather large with many > different types of data that need to be parsed and sometimes data > that's only specific to a single SoC. > > I wonder how much we could get away with this approach though. If > the eeprom driver probed and populated OPPs, made a serial number > available via the soc device, and then we made up framework(s) > for things like our thermal sensor calibration data and display > panel calibration data, I would guess that covers most of my > use-cases. The client drivers would need some sort of 'wait for > eeprom to populate things' API or we'd need to work that into the > new calibration framework. > I think we are making simple eeprom framework too smart which will break in future. IMHO, Anything on top of eeprom interface that interprets the data should not go into the eeprom framework itself, it can either live some parsers/SOC specific drivers/interfaces. As Stephen pointed out earlier lets start with something like this, which would provide a better abstraction to the discussed use cases like serial-number and packed data in eeprom. qfprom@1000000 { reg = <0x1000000 0x1000>; ranges = <0 0x1000000 0x1000>; compatible = "qcom,qfprom-msm8960" pvs-data: pvs-data@40 { compatible = "qcom,pvs-a"; reg = <0x40 0x20>, }; tsens-data: tmdata@10 { reg = <0x10 40>; }; serial-number: serial@50 { compatible = "qcom,serial-msm8960"; reg = <0x50 4>, <0x60 4>; }; }; and then on the consumer side: device { eeproms = <&serial-number>; eeprom-names = "soc-rev-id"; }; driver side: eeprom_get_cell() eeprom_read(); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 24, 2015 at 05:30:49PM -0800, Stephen Boyd wrote: > On 02/24, Maxime Ripard wrote: > > On Mon, Feb 23, 2015 at 03:11:40PM -0800, Stephen Boyd wrote: > > > >>> I would do something more simple that is just a list of keys and > > > >>> their location like this: > > > >>> > > > >>> device-serial-number = <start size>; > > > >>> key1 = <start size>; > > > >>> key2 = <start size>; > > > >> I'm sorry, but what's the difference? > > > > It can describe the layout completely whether the fields are tied to a > > > > h/w device or not. > > > > > > > > What I would like to see here is the entire layout described covering > > > > both types of fields. > > > > > > > > > > I was thinking the DT might be like this on the provider side: > > > > > > qfprom@1000000 { > > > reg = <0x1000000 0x1000>; > > > ranges = <0 0x1000000 0x1000>; > > > compatible = "qcom,qfprom-msm8960" > > > > > > pvs-data: pvs-data@40 { > > > compatible = "qcom,pvs-a"; > > > reg = <0x40 0x20>, > > > #eeprom-cells = <0>; > > > }; > > > > > > tsens-data: tmdata@10 { > > > compatible = "qcom,tsens-data-msm8960"; > > > reg = <0x10 4>, <0x16 4>; > > > #eeprom-cells = <0>; > > > > > > }; > > > > > > serial-number: serial@50 { > > > compatible = "qcom,serial-msm8960"; > > > reg = <0x50 4>, <0x60 4>; > > > #eeprom-cells = <0>; > > > > > > }; > > > }; > > > > I'm not sure the compatible is really needed. > > > > A label of some sort, just like the MTD partitions do would do just > > fine, and wouldn't have the implicit expectation that a driver will be > > probed from that node. > > I wasn't aware that compatible meant driver probe. I thought > compatible just meant some software entity can understand what > I've described within this node. For example, compatible for > reserved-memory nodes doesn't mean we're going to probe a device. Maybe it's just me then :) > > > and then on the consumer side: > > > > > > device { > > > eeproms = <&serial-number>; > > > eeprom-names = "soc-rev-id"; > > > }; > > > > > > > > > This would solve a problem where the consumer device is some standard > > > off-the-shelf IP block that needs to get some SoC specific calibration > > > data from the eeprom. I may want to interpret the bits differently > > > depending on which eeprom is connected to my SoC. Sometimes that data > > > format may be the same across many variations of the SoC (e.g. the > > > qcom,pvs-a node) or it may be completely different for a given SoC (e.g. > > > qcom,serial-msm8960 node). I imagine for other SoCs out there it could > > > be different depending on which eeprom the board manufacturer decides to > > > wire onto their board and how they choose to program the data. > > > > Oh, so you'd like to infer the data format it's stored in from the > > compatible? > > > > AFAICT, this format will be highly depending on the board itself, > > rather than on the SoC, do you think it will scale enough? > > > > > So this is where I think the eeprom-cells and offset + length starts to > > > fall apart. It forces us to make up a bunch of different compatible > > > strings for our consumer device just so that we can parse the eeprom > > > that we decided to use for some SoC/board specific data. Instead I'd > > > like to see some framework that expresses exactly which eeprom is on my > > > board and how to interpret the bits in a way that doesn't require me to > > > keep refining the compatible string for my generic IP block. > > > > Hmmmm, apparently you don't :) > > > > > I worry that if we put all those details in DT we'll end up having to > > > describe individual bits like serial-number-bit-2, serial-number-bit-3, > > > etc. because sometimes these pieces of data are scattered all around the > > > eeprom and aren't contiguous or aligned on a byte boundary. It may be > > > easier to just have a way to express that this is an eeprom with this > > > specific layout and my device has data stored in there. Then the driver > > > can be told what layout it is (via compatible or some other string based > > > means if we're not using DT?) and match that up with some driver data if > > > it needs to know how to understand the bits it can read with the > > > eeprom_read() API. > > > > I'm half convinced that the layout information will actually work for > > more complex cases, like the linked list Rob described. > > > > If such a structure is ever to be found, it would feel wrong to have > > that in the EEPROM driver, but it would feel just as wrong to put that > > in the client driver, that would have to handle the parsing of raw > > data coming flashed by one single crazy board vendor. > > > > Maybe we can have each cell carry a property that defines the format > > it's stored in, and match that to some parsers plugins, starting with > > the generic and trivial cases but still allowing for custom parsers to > > be defined? > > > > Something like > > > > eeprom@42 { > > compatible = "atmel,at24something"; > > reg = <0x42>; > > > > serial@0 { > > label = "board serial"; > > reg = <0x0 0x10>; > > format = "packed"; > > }; > > > > opps@10 { > > label = "board serial"; > > reg = <0x10 0x10>, <0x40 0x10>, <0x80 0x10>; > > format = "random-vendor,opp-linked-list"; > > }; > > }; > > > > That would make eeprom_read always return the same format of data to > > the client drivers, without cripling the generic EEPROM drivers > > either. > > > > Is the goal here to make eeprom_read() figure out how to return > the next byte of data and hide the parsing logic behind the > eeprom APIs? I imagine "random-vendor,opp-linked-list" would be > handled by the eeprom driver and that would return OPPs byte by > byte across the different reg properties to the eeprom consumer? > > This approach concerns me because every eeprom_read() call needs > to fit the format that the client driver is expecting. How do we > validate that? What do we do if we have a random OPP client #1 > that expects to get the data from eeprom_read() with OPPs in > ascending order and random OPP client #2 that expects to get the > data from eeprom_read() with OPPs in descending order? Without going that far, we could have the little/big endian topic here as well. But I guess it all boils down to wether we should support only the trivial cases, or not. Generally speaking, and not just about the OPPs above, we could really well end up with a "generic" (not necessarily a really generic driver, but also IPs used across several SoCs, like the Mentor/Synopsis ones) driver, requiring to read some data from an EEPROM for some reason. Where would you fit the raw data parsing code? In that generic driver. It would end up being just as messy, if not more. So yeah, it really depends on wether we just want to support reading a contiguous block of data, or if we want to cover all cases. And in that case, we should indeed support the cases you mentioned above. > It feels like we're making the eeprom framework too smart without > a well defined abstraction. If we were to make it so that > eeprom_get_opps() knew what to do and parsed/populated the OPPs, > it might work. But if we're just exporting raw data across a > read/write API with some implementation specific mangling it > sounds like it's going to get messy fast. And if the API is well > defined, it would start to become rather large with many > different types of data that need to be parsed and sometimes data > that's only specific to a single SoC. Or even a single board. Most of the drivers are in that case. That doesn't mean that the frameworks should just ignore them entirely because of that fact. Maxime
On Thu, Feb 26, 2015 at 09:16:27AM +0000, Srinivas Kandagatla wrote: > I think we are making simple eeprom framework too smart which will > break in future. > > IMHO, Anything on top of eeprom interface that interprets the data should > not go into the eeprom framework itself, it can either live some parsers/SOC > specific drivers/interfaces. True, but that doesn't mean that this parser support can't be built within the framework itself. > As Stephen pointed out earlier lets start with something like this, which > would provide a better abstraction to the discussed use cases like > serial-number and packed data in eeprom. > > qfprom@1000000 { > reg = <0x1000000 0x1000>; > ranges = <0 0x1000000 0x1000>; > compatible = "qcom,qfprom-msm8960" > > pvs-data: pvs-data@40 { > compatible = "qcom,pvs-a"; > reg = <0x40 0x20>, > }; > > tsens-data: tmdata@10 { > reg = <0x10 40>; > }; > > serial-number: serial@50 { > compatible = "qcom,serial-msm8960"; > reg = <0x50 4>, <0x60 4>; > }; > > }; And I'm sorry, but I still don't get why the compatibles are needed here. > and then on the consumer side: > > device { > eeproms = <&serial-number>; > eeprom-names = "soc-rev-id"; > }; > > driver side: > > eeprom_get_cell() > eeprom_read(); Looks good otherwise. Maxime
On 26/02/15 13:21, Maxime Ripard wrote: > On Thu, Feb 26, 2015 at 09:16:27AM +0000, Srinivas Kandagatla wrote: >> I think we are making simple eeprom framework too smart which will >> break in future. >> >> IMHO, Anything on top of eeprom interface that interprets the data should >> not go into the eeprom framework itself, it can either live some parsers/SOC >> specific drivers/interfaces. > > True, but that doesn't mean that this parser support can't be built > within the framework itself. I was more of thinking parsers/interpreters as a layer on top of this framework. Allowing the simplest users direct access to framework. Also just eeprom_read() apis would not cater for all the parsers and I think it would be very difficult to come up with abstracted consumer apis for all the parsers which could be iterator or link-lists parsers or something very different. > >> As Stephen pointed out earlier lets start with something like this, which >> would provide a better abstraction to the discussed use cases like >> serial-number and packed data in eeprom. >> >> qfprom@1000000 { >> reg = <0x1000000 0x1000>; >> ranges = <0 0x1000000 0x1000>; >> compatible = "qcom,qfprom-msm8960" >> >> pvs-data: pvs-data@40 { >> compatible = "qcom,pvs-a"; >> reg = <0x40 0x20>, >> }; >> >> tsens-data: tmdata@10 { >> reg = <0x10 40>; >> }; >> >> serial-number: serial@50 { >> compatible = "qcom,serial-msm8960"; >> reg = <0x50 4>, <0x60 4>; >> }; >> >> }; > > And I'm sorry, but I still don't get why the compatibles are needed > here. This is an optional property, only purpose of this would be to serve as parser/soc-specific identifier. > >> and then on the consumer side: >> >> device { >> eeproms = <&serial-number>; >> eeprom-names = "soc-rev-id"; >> }; >> >> driver side: >> >> eeprom_get_cell() >> eeprom_read(); > > Looks good otherwise. thanks --srini > > Maxime > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt b/Documentation/devicetree/bindings/eeprom/eeprom.txt new file mode 100644 index 0000000..9ec1ec2 --- /dev/null +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt @@ -0,0 +1,48 @@ += EEPROM Data Device Tree Bindings = + +This binding is intended to represent the location of hardware +configuration data stored in EEPROMs. + +On a significant proportion of boards, the manufacturer has stored +some data on an EEPROM-like device, for the OS to be able to retrieve +these information and act upon it. Obviously, the OS has to know +about where to retrieve these data from, and where they are stored on +the storage device. + +This document is here to document this. + += Data providers = + +Required properties: +#eeprom-cells: Number of cells in an eeprom specifier; The common + case is 2. + +For example: + + at24: eeprom@42 { + #eeprom-cells = <2>; + }; + += Data consumers = + +Required properties: + +eeproms: List of phandle and data cell specifier triplet, one triplet + for each data cell the device might be interested in. The + triplet consists of the phandle to the eeprom provider, then + the offset in byte within that storage device, and the length + in byte of the data we care about. + +Optional properties: + +eeprom-names: List of data cell name strings sorted in the same order + as the resets property. Consumers drivers will use + eeprom-names to differentiate between multiple cells, + and hence being able to know what these cells are for. + +For example: + + device { + eeproms = <&at24 14 42>; + eeprom-names = "soc-rev-id"; + }; diff --git a/drivers/Kconfig b/drivers/Kconfig index c70d6e4..d7afc82 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -184,4 +184,6 @@ source "drivers/thunderbolt/Kconfig" source "drivers/android/Kconfig" +source "drivers/eeprom/Kconfig" + endmenu diff --git a/drivers/Makefile b/drivers/Makefile index 527a6da..57eb5b0 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -165,3 +165,4 @@ obj-$(CONFIG_RAS) += ras/ obj-$(CONFIG_THUNDERBOLT) += thunderbolt/ obj-$(CONFIG_CORESIGHT) += coresight/ obj-$(CONFIG_ANDROID) += android/ +obj-$(CONFIG_EEPROM) += eeprom/ diff --git a/drivers/eeprom/Kconfig b/drivers/eeprom/Kconfig new file mode 100644 index 0000000..2c5452a --- /dev/null +++ b/drivers/eeprom/Kconfig @@ -0,0 +1,19 @@ +menuconfig EEPROM + bool "EEPROM Support" + depends on OF + help + Support for EEPROM alike devices. + + This framework is designed to provide a generic interface to EEPROM + from both the Linux Kernel and the userspace. + + If unsure, say no. + +if EEPROM + +config EEPROM_DEBUG + bool "EEPROM debug support" + help + Say yes here to enable debugging support. + +endif diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile new file mode 100644 index 0000000..e130079 --- /dev/null +++ b/drivers/eeprom/Makefile @@ -0,0 +1,9 @@ +# +# Makefile for eeprom drivers. +# + +ccflags-$(CONFIG_EEPROM_DEBUG) += -DDEBUG + +obj-$(CONFIG_EEPROM) += core.o + +# Devices diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c new file mode 100644 index 0000000..bc877a6 --- /dev/null +++ b/drivers/eeprom/core.c @@ -0,0 +1,290 @@ +/* + * EEPROM framework core. + * + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/device.h> +#include <linux/eeprom-provider.h> +#include <linux/eeprom-consumer.h> +#include <linux/export.h> +#include <linux/fs.h> +#include <linux/idr.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/uaccess.h> + +struct eeprom_cell { + struct eeprom_device *eeprom; + loff_t offset; + size_t count; +}; + +static DEFINE_MUTEX(eeprom_list_mutex); +static LIST_HEAD(eeprom_list); +static DEFINE_IDA(eeprom_ida); + +static ssize_t bin_attr_eeprom_read_write(struct kobject *kobj, + char *buf, loff_t offset, + size_t count, bool read) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct eeprom_device *eeprom = container_of(dev, struct eeprom_device, + edev); + int rc; + + if (offset > eeprom->size) + return 0; + + if (offset + count > eeprom->size) + count = eeprom->size - offset; + + if (read) + rc = regmap_bulk_read(eeprom->regmap, offset, + buf, count/eeprom->stride); + else + rc = regmap_bulk_write(eeprom->regmap, offset, + buf, count/eeprom->stride); + + if (IS_ERR_VALUE(rc)) + return 0; + + return count; +} + +static ssize_t bin_attr_eeprom_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, + char *buf, loff_t offset, size_t count) +{ + return bin_attr_eeprom_read_write(kobj, buf, offset, count, true); +} + +static ssize_t bin_attr_eeprom_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *attr, + char *buf, loff_t offset, size_t count) +{ + return bin_attr_eeprom_read_write(kobj, buf, offset, count, false); +} + +static struct bin_attribute bin_attr_eeprom = { + .attr = { + .name = "eeprom", + .mode = 0660, + }, + .read = bin_attr_eeprom_read, + .write = bin_attr_eeprom_write, +}; + +static struct bin_attribute *eeprom_bin_attributes[] = { + &bin_attr_eeprom, + NULL, +}; + +static const struct attribute_group eeprom_bin_group = { + .bin_attrs = eeprom_bin_attributes, +}; + +static const struct attribute_group *eeprom_dev_groups[] = { + &eeprom_bin_group, + NULL, +}; + +static struct class eeprom_class = { + .name = "eeprom", + .dev_groups = eeprom_dev_groups, +}; + +int eeprom_register(struct eeprom_device *eeprom) +{ + int rval; + + if (!eeprom->regmap || !eeprom->size) { + dev_err(eeprom->dev, "Regmap not found\n"); + return -EINVAL; + } + + eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL); + if (eeprom->id < 0) + return eeprom->id; + + eeprom->edev.class = &eeprom_class; + eeprom->edev.parent = eeprom->dev; + eeprom->edev.of_node = eeprom->dev ? eeprom->dev->of_node : NULL; + dev_set_name(&eeprom->edev, "eeprom%d", eeprom->id); + + device_initialize(&eeprom->edev); + + dev_dbg(&eeprom->edev, "Registering eeprom device %s\n", + dev_name(&eeprom->edev)); + + rval = device_add(&eeprom->edev); + if (rval) + return rval; + + mutex_lock(&eeprom_list_mutex); + list_add(&eeprom->list, &eeprom_list); + mutex_unlock(&eeprom_list_mutex); + + return 0; +} +EXPORT_SYMBOL(eeprom_register); + +int eeprom_unregister(struct eeprom_device *eeprom) +{ + device_del(&eeprom->edev); + + mutex_lock(&eeprom_list_mutex); + list_del(&eeprom->list); + mutex_unlock(&eeprom_list_mutex); + + return 0; +} +EXPORT_SYMBOL(eeprom_unregister); + +static struct eeprom_cell *__eeprom_cell_get(struct device_node *node, + int index) +{ + struct of_phandle_args args; + struct eeprom_cell *cell; + struct eeprom_device *e, *eeprom = NULL; + int ret; + + ret = of_parse_phandle_with_args(node, "eeproms", + "#eeprom-cells", index, &args); + if (ret) + return ERR_PTR(ret); + + if (args.args_count != 2) + return ERR_PTR(-EINVAL); + + mutex_lock(&eeprom_list_mutex); + + list_for_each_entry(e, &eeprom_list, list) { + if (args.np == e->edev.of_node) { + eeprom = e; + break; + } + } + mutex_unlock(&eeprom_list_mutex); + + if (!eeprom) + return ERR_PTR(-EPROBE_DEFER); + + cell = kzalloc(sizeof(*cell), GFP_KERNEL); + if (!cell) + return ERR_PTR(-ENOMEM); + + cell->eeprom = eeprom; + cell->offset = args.args[0]; + cell->count = args.args[1]; + + return cell; +} + +static struct eeprom_cell *__eeprom_cell_get_byname(struct device_node *node, + const char *id) +{ + int index = 0; + + if (id) + index = of_property_match_string(node, + "eeprom-names", + id); + return __eeprom_cell_get(node, index); + +} + +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index) +{ + if (!dev) + return ERR_PTR(-EINVAL); + + /* First, attempt to retrieve the cell through the DT */ + if (dev->of_node) + return __eeprom_cell_get(dev->of_node, index); + + /* We don't support anything else yet */ + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL(eeprom_cell_get); + +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, const char *id) +{ + if (!dev) + return ERR_PTR(-EINVAL); + + if (id && dev->of_node) + return __eeprom_cell_get_byname(dev->of_node, id); + + /* We don't support anything else yet */ + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL(eeprom_cell_get_byname); + +void eeprom_cell_put(struct eeprom_cell *cell) +{ + kfree(cell); +} +EXPORT_SYMBOL(eeprom_cell_put); + +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len) +{ + struct eeprom_device *eeprom = cell->eeprom; + char *buf; + int rc; + + if (!eeprom || !eeprom->regmap) + return ERR_PTR(-EINVAL); + + buf = kzalloc(cell->count, GFP_KERNEL); + if (!buf) + return ERR_PTR(-ENOMEM); + + rc = regmap_bulk_read(eeprom->regmap, cell->offset, + buf, cell->count/eeprom->stride); + if (IS_ERR_VALUE(rc)) { + kfree(buf); + return ERR_PTR(rc); + } + + *len = cell->count; + + return buf; +} +EXPORT_SYMBOL(eeprom_cell_read); + +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len) +{ + struct eeprom_device *eeprom = cell->eeprom; + + if (!eeprom || !eeprom->regmap) + return -EINVAL; + + return regmap_bulk_write(eeprom->regmap, cell->offset, + buf, cell->count/eeprom->stride); +} +EXPORT_SYMBOL(eeprom_cell_write); + +static int eeprom_init(void) +{ + return class_register(&eeprom_class); +} + +static void eeprom_exit(void) +{ + class_unregister(&eeprom_class); +} + +subsys_initcall(eeprom_init); +module_exit(eeprom_exit); + +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com"); +MODULE_DESCRIPTION("EEPROM Driver Core"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/eeprom-consumer.h b/include/linux/eeprom-consumer.h new file mode 100644 index 0000000..706ae9d --- /dev/null +++ b/include/linux/eeprom-consumer.h @@ -0,0 +1,73 @@ +/* + * EEPROM framework consumer. + * + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#ifndef _LINUX_EEPROM_CONSUMER_H +#define _LINUX_EEPROM_CONSUMER_H + +struct eeprom_cell; + +/** + * eeprom_cell_get(): Get eeprom cell of device form a given index. + * + * @dev: Device that will be interacted with + * @index: Index of the eeprom cell. + * + * The return value will be an ERR_PTR() on error or a valid pointer + * to a struct eeprom_cell. The eeprom_cell will be freed by the + * eeprom_cell_put(). + */ +struct eeprom_cell *eeprom_cell_get(struct device *dev, int index); + +/** + * eeprom_cell_get(): Get eeprom cell of device form a given name. + * + * @dev: Device that will be interacted with + * @name: Name of the eeprom cell. + * + * The return value will be an ERR_PTR() on error or a valid pointer + * to a struct eeprom_cell. The eeprom_cell will be freed by the + * eeprom_cell_put(). + */ +struct eeprom_cell *eeprom_cell_get_byname(struct device *dev, + const char *name); + +/** + * eeprom_cell_put(): Release previously allocated eeprom cell. + * + * @cell: Previously allocated eeprom cell by eeprom_cell_get() + * or eeprom_cell_get_byname(). + */ +void eeprom_cell_put(struct eeprom_cell *cell); + +/** + * eeprom_cell_read(): Read a given eeprom cell + * + * @cell: eeprom cell to be read. + * @len: pointer to length of cell which will be populated on successful read. + * + * The return value will be an ERR_PTR() on error or a valid pointer + * to a char * bufffer. The buffer should be freed by the consumer with a + * kfree(). + */ +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len); + +/** + * eeprom_cell_write(): Write to a given eeprom cell + * + * @cell: eeprom cell to be written. + * @buf: Buffer to be written. + * @len: length of buffer to be written to eeprom cell. + * + * The return value will be an non zero on error or a zero on successful write. + */ +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len); + +#endif /* ifndef _LINUX_EEPROM_CONSUMER_H */ diff --git a/include/linux/eeprom-provider.h b/include/linux/eeprom-provider.h new file mode 100644 index 0000000..3943c2f --- /dev/null +++ b/include/linux/eeprom-provider.h @@ -0,0 +1,51 @@ +/* + * EEPROM framework provider. + * + * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#ifndef _LINUX_EEPROM_PROVIDER_H +#define _LINUX_EEPROM_PROVIDER_H + +#include <linux/device.h> +#include <linux/regmap.h> +#include <linux/list.h> + +struct eeprom_device { + struct regmap *regmap; + int stride; + size_t size; + struct device *dev; + + /* Internal to framework */ + struct device edev; + int id; + struct list_head list; +}; + +/** + * eeprom_register(): Register a eeprom device for given eeprom. + * Also creates an binary entry in /sys/class/eeprom/eeprom[id]/eeprom + * + * @eeprom: eeprom device that needs to be created + * + * The return value will be an error code on error or a zero on success. + * The eeprom_device and sysfs entery will be freed by the eeprom_unregister(). + */ +int eeprom_register(struct eeprom_device *eeprom); + +/** + * eeprom_unregister(): Unregister previously registered eeprom device + * + * @eeprom: Pointer to previously registered eeprom device. + * + * The return value will be an non zero on error or a zero on success. + */ +int eeprom_unregister(struct eeprom_device *eeprom); + +#endif /* ifndef _LINUX_EEPROM_PROVIDER_H */