diff mbox

[v2,1/7] eeprom: Add a simple EEPROM framework for eeprom providers

Message ID 1426240214-2434-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Srinivas Kandagatla March 13, 2015, 9:50 a.m. UTC
This patch adds just providers part of the framework just to enable easy
review.

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>
[Maxime Ripard: intial version of eeprom framework]
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/Kconfig                 |   2 +
 drivers/Makefile                |   1 +
 drivers/eeprom/Kconfig          |  11 +++
 drivers/eeprom/Makefile         |   5 +
 drivers/eeprom/core.c           | 213 ++++++++++++++++++++++++++++++++++++++++
 include/linux/eeprom-provider.h |  47 +++++++++
 6 files changed, 279 insertions(+)
 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-provider.h

Comments

Mark Brown March 23, 2015, 9:09 p.m. UTC | #1
On Fri, Mar 13, 2015 at 09:50:14AM +0000, Srinivas Kandagatla wrote:

A couple of *very* minor points below, otherwise this looks OK to me.

> +struct eeprom_device *eeprom_register(struct eeprom_config *config)
> +{
> +	struct eeprom_device *eeprom;
> +	int rval;
> +
> +	if (!config->regmap || !config->size) {
> +		dev_err(config->dev, "Regmap not found\n");
> +		return ERR_PTR(-EINVAL);
> +	}

You have a struct device in the config and the regmap API has
dev_get_regmap() which for most devices that don't have multiple regmaps
will give the right regmap.  It would be nice to support this as a
convenience for users.

> +	eeprom = kzalloc(sizeof(*eeprom), GFP_KERNEL);
> +	if (!eeprom)
> +		return ERR_PTR(-ENOMEM);

...

> +	rval = device_add(&eeprom->dev);
> +	if (rval)
> +		return ERR_PTR(rval);

Don't you need a kfree() if device_add() fails?
Srinivas Kandagatla March 23, 2015, 10:05 p.m. UTC | #2
On 23/03/15 21:09, Mark Brown wrote:
> On Fri, Mar 13, 2015 at 09:50:14AM +0000, Srinivas Kandagatla wrote:
>
> A couple of *very* minor points below, otherwise this looks OK to me.
>
Thankyou for the review.

>> +struct eeprom_device *eeprom_register(struct eeprom_config *config)
>> +{
>> +	struct eeprom_device *eeprom;
>> +	int rval;
>> +
>> +	if (!config->regmap || !config->size) {
>> +		dev_err(config->dev, "Regmap not found\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>
> You have a struct device in the config and the regmap API has
> dev_get_regmap() which for most devices that don't have multiple regmaps
> will give the right regmap.  It would be nice to support this as a
> convenience for users.
Yes, sure that makes sense, I will give it a try.

>
>> +	eeprom = kzalloc(sizeof(*eeprom), GFP_KERNEL);
>> +	if (!eeprom)
>> +		return ERR_PTR(-ENOMEM);
>
> ...
>
>> +	rval = device_add(&eeprom->dev);
>> +	if (rval)
>> +		return ERR_PTR(rval);
>
> Don't you need a kfree() if device_add() fails?
I will fix it in next version.

--srini
>
Srinivas Kandagatla March 24, 2015, 9:18 a.m. UTC | #3
On 23/03/15 22:05, Srinivas Kandagatla wrote:
>
>
> On 23/03/15 21:09, Mark Brown wrote:
>> On Fri, Mar 13, 2015 at 09:50:14AM +0000, Srinivas Kandagatla wrote:
>>
>> A couple of *very* minor points below, otherwise this looks OK to me.
>>
> Thankyou for the review.
>
>>> +struct eeprom_device *eeprom_register(struct eeprom_config *config)
>>> +{
>>> +    struct eeprom_device *eeprom;
>>> +    int rval;
>>> +
>>> +    if (!config->regmap || !config->size) {
>>> +        dev_err(config->dev, "Regmap not found\n");
>>> +        return ERR_PTR(-EINVAL);
>>> +    }
>>
>> You have a struct device in the config and the regmap API has
>> dev_get_regmap() which for most devices that don't have multiple regmaps
>> will give the right regmap.  It would be nice to support this as a
>> convenience for users.
> Yes, sure that makes sense, I will give it a try.
>
I did try your suggestion, by which I could remove the regmap from 
config. One thing I did not like was eeprom-core getting size/stride 
info directly from providers and regmap from regmap apis. I was 
wondering if we could take a step further and introduce new regmap 
helpers like

regmap_get_size(regmap)
regmap_get_stride(regmap)

Which would be give eeprom-core the size and stride info, doing this way 
would cut down regmap related things from eeprom_config structure to 
minimal and also the source of information would come from just regmap apis.


--srini

>>
>>> +    eeprom = kzalloc(sizeof(*eeprom), GFP_KERNEL);
>>> +    if (!eeprom)
>>> +        return ERR_PTR(-ENOMEM);
>>
>> ...
>>
>>> +    rval = device_add(&eeprom->dev);
>>> +    if (rval)
>>> +        return ERR_PTR(rval);
>>
>> Don't you need a kfree() if device_add() fails?
> I will fix it in next version.
>
> --srini
>>
Mark Brown March 24, 2015, 5:23 p.m. UTC | #4
On Tue, Mar 24, 2015 at 09:18:14AM +0000, Srinivas Kandagatla wrote:

> I did try your suggestion, by which I could remove the regmap from config.
> One thing I did not like was eeprom-core getting size/stride info directly
> from providers and regmap from regmap apis. I was wondering if we could take
> a step further and introduce new regmap helpers like

> regmap_get_size(regmap)

This is already there.

> regmap_get_stride(regmap)

> Which would be give eeprom-core the size and stride info, doing this way
> would cut down regmap related things from eeprom_config structure to minimal
> and also the source of information would come from just regmap apis.

Documentation/SubmittingPatches...
Srinivas Kandagatla March 24, 2015, 6:34 p.m. UTC | #5
On 24/03/15 17:23, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 09:18:14AM +0000, Srinivas Kandagatla wrote:
>
>> I did try your suggestion, by which I could remove the regmap from config.
>> One thing I did not like was eeprom-core getting size/stride info directly
>> from providers and regmap from regmap apis. I was wondering if we could take
>> a step further and introduce new regmap helpers like
>
>> regmap_get_size(regmap)
>
> This is already there.
Sorry, I can't see any such api atleast in v4.0-rc5 and linux-next? Are 
you referring to another api which does the same job?

>
>> regmap_get_stride(regmap)
>
>> Which would be give eeprom-core the size and stride info, doing this way
>> would cut down regmap related things from eeprom_config structure to minimal
>> and also the source of information would come from just regmap apis.
>
> Documentation/SubmittingPatches...
>
Am not sure what you meant here, Am guessing that you asked me to keep 
the respective maintainers in the loop and follow the guide, when I 
resend the patch?

--srini
Mark Brown March 24, 2015, 7:02 p.m. UTC | #6
On Tue, Mar 24, 2015 at 06:34:32PM +0000, Srinivas Kandagatla wrote:
> On 24/03/15 17:23, Mark Brown wrote:

> >>regmap_get_size(regmap)

> >This is already there.

> Sorry, I can't see any such api atleast in v4.0-rc5 and linux-next? Are you
> referring to another api which does the same job?

regmap_get_val_bytes()

> >>Which would be give eeprom-core the size and stride info, doing this way
> >>would cut down regmap related things from eeprom_config structure to minimal
> >>and also the source of information would come from just regmap apis.

> >Documentation/SubmittingPatches...

> Am not sure what you meant here, Am guessing that you asked me to keep the
> respective maintainers in the loop and follow the guide, when I resend the
> patch?

I'm saying that you should send patches if you want to add features.
Srinivas Kandagatla March 24, 2015, 7:26 p.m. UTC | #7
On 24/03/15 19:02, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 06:34:32PM +0000, Srinivas Kandagatla wrote:
>> >On 24/03/15 17:23, Mark Brown wrote:
>>>> > >>regmap_get_size(regmap)
>>> > >This is already there.
>> >Sorry, I can't see any such api atleast in v4.0-rc5 and linux-next? Are you
>> >referring to another api which does the same job?
> regmap_get_val_bytes()
>
This would return value bytes, but I wanted is the regmap->max_register 
value which would be used for sanity checks in eeprom-core.

--srini
Mark Brown March 24, 2015, 8:55 p.m. UTC | #8
On Tue, Mar 24, 2015 at 07:26:45PM +0000, Srinivas Kandagatla wrote:
> On 24/03/15 19:02, Mark Brown wrote:
> >On Tue, Mar 24, 2015 at 06:34:32PM +0000, Srinivas Kandagatla wrote:
> >>>On 24/03/15 17:23, Mark Brown wrote:

> >>>>> >>regmap_get_size(regmap)

> >>>> >This is already there.

> >>>Sorry, I can't see any such api atleast in v4.0-rc5 and linux-next? Are you
> >>>referring to another api which does the same job?

> >regmap_get_val_bytes()

> This would return value bytes, but I wanted is the regmap->max_register
> value which would be used for sanity checks in eeprom-core.

Then you *really* want to pick a better name then, I'd never have
inferred that meaning from "size" (consider sparse register maps for
example).  Like I said, send patches (preferrably showing the users as
well).
diff mbox

Patch

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..21e1847
--- /dev/null
+++ b/drivers/eeprom/Kconfig
@@ -0,0 +1,11 @@ 
+menuconfig EEPROM
+	tristate "EEPROM Support"
+	depends on OF
+	select REGMAP
+	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.
diff --git a/drivers/eeprom/Makefile b/drivers/eeprom/Makefile
new file mode 100644
index 0000000..250c95a
--- /dev/null
+++ b/drivers/eeprom/Makefile
@@ -0,0 +1,5 @@ 
+#
+# Makefile for eeprom drivers.
+#
+
+obj-$(CONFIG_EEPROM)		+= core.o
diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c
new file mode 100644
index 0000000..a9839de
--- /dev/null
+++ b/drivers/eeprom/core.c
@@ -0,0 +1,213 @@ 
+/*
+ * EEPROM framework core.
+ *
+ * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
+ * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/eeprom-provider.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_device {
+	struct regmap		*regmap;
+	int			stride;
+	size_t			size;
+
+	struct module		*owner;
+	struct device		dev;
+	int			id;
+	int			users;
+};
+
+static DEFINE_MUTEX(eeprom_mutex);
+static DEFINE_IDA(eeprom_ida);
+
+#define to_eeprom(d) container_of(d, struct eeprom_device, dev)
+
+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)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct eeprom_device *eeprom = to_eeprom(dev);
+	int rc;
+
+	if (offset > eeprom->size)
+		return -EINVAL;
+
+	if (offset + count > eeprom->size)
+		count = eeprom->size - offset;
+
+	rc = regmap_bulk_read(eeprom->regmap, offset,
+			      buf, count/eeprom->stride);
+
+	if (IS_ERR_VALUE(rc))
+		return rc;
+
+	return count - count % eeprom->stride;
+}
+
+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)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct eeprom_device *eeprom = to_eeprom(dev);
+	int rc;
+
+	if (offset > eeprom->size)
+		return -EINVAL;
+
+	if (offset + count > eeprom->size)
+		count = eeprom->size - offset;
+
+	rc = regmap_bulk_write(eeprom->regmap, offset,
+			       buf, count/eeprom->stride);
+
+	if (IS_ERR_VALUE(rc))
+		return rc;
+
+	return count - count % eeprom->stride;
+}
+
+static struct bin_attribute bin_attr_eeprom = {
+	.attr	= {
+		.name	= "eeprom",
+		.mode	= S_IWUSR | S_IRUGO,
+	},
+	.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 void eeprom_release(struct device *dev)
+{
+	kfree(to_eeprom(dev));
+}
+
+static struct class eeprom_class = {
+	.name		= "eeprom",
+	.dev_groups	= eeprom_dev_groups,
+	.dev_release	= eeprom_release,
+};
+
+/**
+ * eeprom_register(): Register a eeprom device for given eeprom.
+ * Also creates an binary entry in /sys/class/eeprom/name-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().
+ */
+
+struct eeprom_device *eeprom_register(struct eeprom_config *config)
+{
+	struct eeprom_device *eeprom;
+	int rval;
+
+	if (!config->regmap || !config->size) {
+		dev_err(config->dev, "Regmap not found\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	eeprom = kzalloc(sizeof(*eeprom), GFP_KERNEL);
+	if (!eeprom)
+		return ERR_PTR(-ENOMEM);
+
+	eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL);
+	if (eeprom->id < 0)
+		return ERR_PTR(eeprom->id);
+
+	eeprom->owner = config->owner;
+	eeprom->regmap = config->regmap;
+	eeprom->stride = config->stride;
+	eeprom->size = config->size;
+	eeprom->dev.class = &eeprom_class;
+	eeprom->dev.parent = config->dev;
+	eeprom->dev.of_node = config->dev ? config->dev->of_node : NULL;
+	dev_set_name(&eeprom->dev, "%s%d",
+		     config->name ? : "eeprom", config->id);
+
+	device_initialize(&eeprom->dev);
+
+	dev_dbg(&eeprom->dev, "Registering eeprom device %s\n",
+		dev_name(&eeprom->dev));
+
+	rval = device_add(&eeprom->dev);
+	if (rval)
+		return ERR_PTR(rval);
+
+	return eeprom;
+}
+EXPORT_SYMBOL_GPL(eeprom_register);
+
+/**
+ * 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)
+{
+	mutex_lock(&eeprom_mutex);
+	if (eeprom->users) {
+		mutex_unlock(&eeprom_mutex);
+		return -EBUSY;
+	}
+	mutex_unlock(&eeprom_mutex);
+
+	device_del(&eeprom->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(eeprom_unregister);
+
+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("Srinivas Kandagatla <srinivas.kandagatla@linaro.org");
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com");
+MODULE_DESCRIPTION("EEPROM Driver Core");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/eeprom-provider.h b/include/linux/eeprom-provider.h
new file mode 100644
index 0000000..21afdaa
--- /dev/null
+++ b/include/linux/eeprom-provider.h
@@ -0,0 +1,47 @@ 
+/*
+ * 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/regmap.h>
+
+struct eeprom_device;
+
+struct eeprom_config {
+	struct device		*dev;
+	struct regmap		*regmap;
+	const char		*name;
+	int			id;
+	int			stride;
+	size_t			size;
+	struct module		*owner;
+};
+
+#if IS_ENABLED(CONFIG_EEPROM)
+
+struct eeprom_device *eeprom_register(struct eeprom_config *cfg);
+int eeprom_unregister(struct eeprom_device *eeprom);
+
+#else
+
+static inline struct eeprom_device *eeprom_register(struct eeprom_config *cfg)
+{
+	return NULL;
+}
+static inline int eeprom_unregister(struct eeprom_device *eeprom)
+{
+	return -ENOSYS;
+}
+
+#endif /* CONFIG_EEPROM */
+
+#endif  /* ifndef _LINUX_EEPROM_PROVIDER_H */