Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On 07/03/15 15:00, Mark Brown wrote:
On Thu, Mar 05, 2015 at 09:45:41AM +, Srinivas Kandagatla wrote:
+
+ return eeprom;
+}
+EXPORT_SYMBOL(eeprom_register);
This framework uses regmap but regmap is EXPORT_SYMBOL_GPL() and this is
using EXPORT_SYMBOL().
Thanks for spotting this, I will fix this in next version.
+int eeprom_unregister(struct eeprom_device *eeprom)
+{
+ mutex_lock(_mutex);
+ if (atomic_read(>users)) {
+ mutex_unlock(_mutex);
Atomic reads and a mutex - isn't the mutex enough? Atomics are
generally a recipie for bugs due to the complexity in using them.
Yes, you are right as long as we protect users variable with mutex,
using atomic is really redundant, will fix it in next version.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On 07/03/15 15:00, Mark Brown wrote:
On Thu, Mar 05, 2015 at 09:45:41AM +, Srinivas Kandagatla wrote:
+
+ return eeprom;
+}
+EXPORT_SYMBOL(eeprom_register);
This framework uses regmap but regmap is EXPORT_SYMBOL_GPL() and this is
using EXPORT_SYMBOL().
Thanks for spotting this, I will fix this in next version.
+int eeprom_unregister(struct eeprom_device *eeprom)
+{
+ mutex_lock(eeprom_mutex);
+ if (atomic_read(eeprom-users)) {
+ mutex_unlock(eeprom_mutex);
Atomic reads and a mutex - isn't the mutex enough? Atomics are
generally a recipie for bugs due to the complexity in using them.
Yes, you are right as long as we protect users variable with mutex,
using atomic is really redundant, will fix it in next version.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On Thu, Mar 05, 2015 at 09:45:41AM +, Srinivas Kandagatla wrote:
> +
> + return eeprom;
> +}
> +EXPORT_SYMBOL(eeprom_register);
This framework uses regmap but regmap is EXPORT_SYMBOL_GPL() and this is
using EXPORT_SYMBOL().
> +int eeprom_unregister(struct eeprom_device *eeprom)
> +{
> + mutex_lock(_mutex);
> + if (atomic_read(>users)) {
> + mutex_unlock(_mutex);
Atomic reads and a mutex - isn't the mutex enough? Atomics are
generally a recipie for bugs due to the complexity in using them.
signature.asc
Description: Digital signature
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On Thu, Mar 05, 2015 at 09:45:41AM +, Srinivas Kandagatla wrote:
+
+ return eeprom;
+}
+EXPORT_SYMBOL(eeprom_register);
This framework uses regmap but regmap is EXPORT_SYMBOL_GPL() and this is
using EXPORT_SYMBOL().
+int eeprom_unregister(struct eeprom_device *eeprom)
+{
+ mutex_lock(eeprom_mutex);
+ if (atomic_read(eeprom-users)) {
+ mutex_unlock(eeprom_mutex);
Atomic reads and a mutex - isn't the mutex enough? Atomics are
generally a recipie for bugs due to the complexity in using them.
signature.asc
Description: Digital signature
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On 05/03/15 10:23, Paul Bolle wrote:
On Thu, 2015-03-05 at 09:45 +, Srinivas Kandagatla wrote:
--- /dev/null
+++ b/drivers/eeprom/Kconfig
@@ -0,0 +1,20 @@
+menuconfig EEPROM
+ bool "EEPROM Support"
EEPROM is a bool symbol.
+ 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.
+
+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 000..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
So core.o will be built-in (or not built at all, of course).
+# Devices
diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c
new file mode 100644
index 000..243e466
--- /dev/null
+++ b/drivers/eeprom/core.c
@@ -0,0 +1,208 @@
+/*
+ * EEPROM framework core.
+ *
+ * Copyright (C) 2015 Srinivas Kandagatla
+ * Copyright (C) 2013 Maxime Ripard
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
So I guess this header is not needed.
+#include
+#include
+#include
+module_exit(eeprom_exit);
And this will never be called.
+MODULE_AUTHOR("Srinivas Kandagatla
And those four macros will basically be preprocessed away. But if you
actually want this code to be built modular too, and change EEPROM to
tristate, you probably want to use
MODULE_LICENSE("GPL v2");
here.
Thanks, Thats a good catch,
There is no reason why this driver can't be a module, I will change the
Kconfig and License in next version.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On Thu, 2015-03-05 at 09:45 +, Srinivas Kandagatla wrote:
> --- /dev/null
> +++ b/drivers/eeprom/Kconfig
> @@ -0,0 +1,20 @@
> +menuconfig EEPROM
> + bool "EEPROM Support"
EEPROM is a bool symbol.
> + 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.
> +
> +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 000..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
So core.o will be built-in (or not built at all, of course).
> +# Devices
> diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c
> new file mode 100644
> index 000..243e466
> --- /dev/null
> +++ b/drivers/eeprom/core.c
> @@ -0,0 +1,208 @@
> +/*
> + * EEPROM framework core.
> + *
> + * Copyright (C) 2015 Srinivas Kandagatla
> + * Copyright (C) 2013 Maxime Ripard
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
So I guess this header is not needed.
> +#include
> +#include
> +#include
> +module_exit(eeprom_exit);
And this will never be called.
> +MODULE_AUTHOR("Srinivas Kandagatla +MODULE_AUTHOR("Maxime Ripard +MODULE_DESCRIPTION("EEPROM Driver Core");
> +MODULE_LICENSE("GPL");
And those four macros will basically be preprocessed away. But if you
actually want this code to be built modular too, and change EEPROM to
tristate, you probably want to use
MODULE_LICENSE("GPL v2");
here.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On Thu, 2015-03-05 at 09:45 +, Srinivas Kandagatla wrote: --- /dev/null +++ b/drivers/eeprom/Kconfig @@ -0,0 +1,20 @@ +menuconfig EEPROM + bool EEPROM Support EEPROM is a bool symbol. + 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. + +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 000..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 So core.o will be built-in (or not built at all, of course). +# Devices diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c new file mode 100644 index 000..243e466 --- /dev/null +++ b/drivers/eeprom/core.c @@ -0,0 +1,208 @@ +/* + * EEPROM framework core. + * + * Copyright (C) 2015 Srinivas Kandagatla srinivas.kandaga...@linaro.org + * Copyright (C) 2013 Maxime Ripard maxime.rip...@free-electrons.com + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed as is without any + * warranty of any kind, whether express or implied. + */ + +#include linux/device.h +#include linux/eeprom-provider.h +#include linux/export.h +#include linux/fs.h +#include linux/idr.h +#include linux/init.h +#include linux/module.h So I guess this header is not needed. +#include linux/of.h +#include linux/slab.h +#include linux/uaccess.h +module_exit(eeprom_exit); And this will never be called. +MODULE_AUTHOR(Srinivas Kandagatla srinivas.kandaga...@linaro.org); +MODULE_AUTHOR(Maxime Ripard maxime.rip...@free-electrons.com); +MODULE_DESCRIPTION(EEPROM Driver Core); +MODULE_LICENSE(GPL); And those four macros will basically be preprocessed away. But if you actually want this code to be built modular too, and change EEPROM to tristate, you probably want to use MODULE_LICENSE(GPL v2); here. Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 1/6] eeprom: Add a simple EEPROM framework for eeprom providers
On 05/03/15 10:23, Paul Bolle wrote: On Thu, 2015-03-05 at 09:45 +, Srinivas Kandagatla wrote: --- /dev/null +++ b/drivers/eeprom/Kconfig @@ -0,0 +1,20 @@ +menuconfig EEPROM + bool EEPROM Support EEPROM is a bool symbol. + 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. + +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 000..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 So core.o will be built-in (or not built at all, of course). +# Devices diff --git a/drivers/eeprom/core.c b/drivers/eeprom/core.c new file mode 100644 index 000..243e466 --- /dev/null +++ b/drivers/eeprom/core.c @@ -0,0 +1,208 @@ +/* + * EEPROM framework core. + * + * Copyright (C) 2015 Srinivas Kandagatla srinivas.kandaga...@linaro.org + * Copyright (C) 2013 Maxime Ripard maxime.rip...@free-electrons.com + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed as is without any + * warranty of any kind, whether express or implied. + */ + +#include linux/device.h +#include linux/eeprom-provider.h +#include linux/export.h +#include linux/fs.h +#include linux/idr.h +#include linux/init.h +#include linux/module.h So I guess this header is not needed. +#include linux/of.h +#include linux/slab.h +#include linux/uaccess.h +module_exit(eeprom_exit); And this will never be called. +MODULE_AUTHOR(Srinivas Kandagatla srinivas.kandaga...@linaro.org); +MODULE_AUTHOR(Maxime Ripard maxime.rip...@free-electrons.com); +MODULE_DESCRIPTION(EEPROM Driver Core); +MODULE_LICENSE(GPL); And those four macros will basically be preprocessed away. But if you actually want this code to be built modular too, and change EEPROM to tristate, you probably want to use MODULE_LICENSE(GPL v2); here. Thanks, Thats a good catch, There is no reason why this driver can't be a module, I will change the Kconfig and License in next version. Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
