Message-ID: <20150219181230.GC795@lunn.ch>
Date:	Thu, 19 Feb 2015 19:12:30 +0100
From:	Andrew Lunn <andrew@...n.ch>
To:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	Arnd Bergmann <arnd@...db.de>, Pawel Moll <pawel.moll@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-api@...r.kernel.org, broonie@...nel.org,
	Stephen Boyd <sboyd@...eaurora.org>,
	linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework

On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote:
> From: Maxime Ripard <maxime.ripard@...e-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@...e-electrons.com>
> [srinivas.kandagatla: Moved to regmap based and cleanedup apis]
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.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@...aro.org>
> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@...e-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@...e-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@...aro.org>
> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@...e-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@...aro.org>
> + * Copyright (C) 2013 Maxime Ripard <maxime.ripard@...e-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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists