[RFC,1/3] eeprom: Add a simple EEPROM framework

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

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/