Message ID | 1426240214-2434-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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?
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 >
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 >>
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...
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
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.
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
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 --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 */