Re: [PATCHv2 1/7] pwm: Add pwm core driver
Arun MURTHY arun.mur...@stericsson.com writes: [...] I suggest that you work on Kevin's comments before making any code changes though. This pwm driver also supports the Davinci pwm driver as suggested by Kelvin. My concern isn't whether it supports davinci or not. Adapting existing drivers is the easy part. My concern is that there are now two proposals for a generic PWM framework, and I would prefer to see that those projects are aligned before anything merges. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 1/7] pwm: Add pwm core driver
The existing pwm based led and backlight driver makes use of the
pwm(include/linux/pwm.h). So all the board specific pwm drivers will
be exposing the same set of function name as in include/linux/pwm.h.
Consder a platform with multi Soc or having more than one pwm module, in
such a case, there exists more than one pwm driver for a platform. Each
of these pwm drivers export the same set of function and hence leads to
re-declaration build error.
In order to overcome this issue all the pwm drivers must register to
some core pwm driver with function pointers for pwm operations (i.e
pwm_config, pwm_enable, pwm_disable).
The clients of pwm device will have to call pwm_request, wherein
they will get the pointer to struct pwm_ops. This structure include
function pointers for pwm_config, pwm_enable and pwm_disable.
Signed-off-by: Arun Murthy arun.mur...@stericsson.com
Acked-by: Linus Walleij linus.wall...@stericsson.com
---
drivers/Kconfig|2 +
drivers/Makefile |1 +
drivers/pwm/Kconfig| 18 ++
drivers/pwm/Makefile |1 +
drivers/pwm/pwm-core.c | 135
include/linux/pwm.h| 12 -
6 files changed, 168 insertions(+), 1 deletions(-)
create mode 100644 drivers/pwm/Kconfig
create mode 100644 drivers/pwm/Makefile
create mode 100644 drivers/pwm/pwm-core.c
diff --git a/drivers/Kconfig b/drivers/Kconfig
index a2b902f..e042f27 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -111,4 +111,6 @@ source drivers/xen/Kconfig
source drivers/staging/Kconfig
source drivers/platform/Kconfig
+
+source drivers/pwm/Kconfig
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 4ca727d..0061ec4 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -116,3 +116,4 @@ obj-$(CONFIG_STAGING) += staging/
obj-y += platform/
obj-y += ieee802154/
obj-y += vbus/
+obj-$(CONFIG_PWM_DEVICES) += pwm/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 000..03a9813
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,18 @@
+#
+# PWM devices
+#
+
+menuconfig PWM_DEVICES
+ bool PWM devices
+ default y
+ ---help---
+ Say Y to enable pwm core driver and see options for various pwm
+ drivers. This option enables pwm drivers to register with the
+ pwm core driver and thereby provide a single interface to the
+ clients using PWM.
+
+ If you say N, all options in this submenu will be skipped and
disabled.
+
+if PWM_DEVICES
+
+endif # PWM_DEVICES
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 000..552f969
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PWM_DEVICES) += pwm-core.o
diff --git a/drivers/pwm/pwm-core.c b/drivers/pwm/pwm-core.c
new file mode 100644
index 000..6c6f6a6
--- /dev/null
+++ b/drivers/pwm/pwm-core.c
@@ -0,0 +1,135 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ * Author: Arun R Murthy arun.mur...@stericsson.com
+ */
+#include linux/init.h
+#include linux/device.h
+#include linux/slab.h
+#include linux/rwsem.h
+#include linux/err.h
+#include linux/pwm.h
+
+struct pwm_device {
+ struct pwm_ops *pops;
+ int pwm_id;
+};
+
+struct pwm_dev_info {
+ struct pwm_device *pwm_dev;
+ struct list_head list;
+};
+static struct pwm_dev_info *di;
+
+DECLARE_RWSEM(pwm_list_lock);
+
+void __deprecated pwm_free(struct pwm_device *pwm)
+{
+}
+
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+ if (!pwm-pops)
+ -EFAULT;
+ return pwm-pops-pwm_config(pwm, duty_ns, period_ns);
+}
+EXPORT_SYMBOL(pwm_config);
+
+int pwm_enable(struct pwm_device *pwm)
+{
+ if (!pwm-pops)
+ -EFAULT;
+ return pwm-pops-pwm_enable(pwm);
+}
+EXPORT_SYMBOL(pwm_enable);
+
+void pwm_disable(struct pwm_device *pwm)
+{
+ if (!pwm-pops)
+ -EFAULT;
+ pwm-pops-pwm_disable(pwm);
+}
+EXPORT_SYMBOL(pwm_disable);
+
+int pwm_device_register(struct pwm_device *pwm_dev)
+{
+ struct pwm_dev_info *pwm;
+
+ down_write(pwm_list_lock);
+ pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+ if (!pwm) {
+ up_write(pwm_list_lock);
+ return -ENOMEM;
+ }
+ pwm-pwm_dev = pwm_dev;
+ list_add_tail(pwm-list, di-list);
+ up_write(pwm_list_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(pwm_device_register);
+
+int pwm_device_unregister(struct pwm_device *pwm_dev)
+{
+ struct pwm_dev_info *tmp;
+ struct list_head *pos, *tmp_lst;
+
+ down_write(pwm_list_lock);
+ list_for_each_safe(pos, tmp_lst, di-list) {
+ tmp = list_entry(pos, struct pwm_dev_info, list);
+ if (tmp-pwm_dev == pwm_dev) {
+ list_del(pos);
+
Re: [PATCHv2 1/7] pwm: Add pwm core driver
On Tue, 5 Oct 2010 17:29:56 +0530
Arun Murthy arun.mur...@stericsson.com wrote:
The existing pwm based led and backlight driver makes use of the
pwm(include/linux/pwm.h). So all the board specific pwm drivers will
be exposing the same set of function name as in include/linux/pwm.h.
Consder a platform with multi Soc or having more than one pwm module, in
such a case, there exists more than one pwm driver for a platform. Each
of these pwm drivers export the same set of function and hence leads to
re-declaration build error.
In order to overcome this issue all the pwm drivers must register to
some core pwm driver with function pointers for pwm operations (i.e
pwm_config, pwm_enable, pwm_disable).
The clients of pwm device will have to call pwm_request, wherein
they will get the pointer to struct pwm_ops. This structure include
function pointers for pwm_config, pwm_enable and pwm_disable.
Have we worked out who will be merging this work, if it gets merged?
...
+struct pwm_dev_info {
+ struct pwm_device *pwm_dev;
+ struct list_head list;
+};
+static struct pwm_dev_info *di;
We could just do
static struct pwm_dev_info {
...
} *di;
+DECLARE_RWSEM(pwm_list_lock);
This can/should be static.
+void __deprecated pwm_free(struct pwm_device *pwm)
+{
+}
Why are we adding a new function and already deprecating it?
Probably this was already addressed in earlier review, but I'm asking
again, because there's no comment explaining the reasons. Lesson
learned, please add a comment.
Oh, I see that pwm_free() already exists. This patch adds a new copy
and doesn't remove the old function. Does this all actually work?
It still needs a comment explaining why it's deprecated.
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+ if (!pwm-pops)
+ -EFAULT;
+ return pwm-pops-pwm_config(pwm, duty_ns, period_ns);
+}
+EXPORT_SYMBOL(pwm_config);
+
+int pwm_enable(struct pwm_device *pwm)
+{
+ if (!pwm-pops)
+ -EFAULT;
+ return pwm-pops-pwm_enable(pwm);
+}
+EXPORT_SYMBOL(pwm_enable);
+
+void pwm_disable(struct pwm_device *pwm)
+{
+ if (!pwm-pops)
+ -EFAULT;
+ pwm-pops-pwm_disable(pwm);
+}
+EXPORT_SYMBOL(pwm_disable);
+
+int pwm_device_register(struct pwm_device *pwm_dev)
+{
+ struct pwm_dev_info *pwm;
+
+ down_write(pwm_list_lock);
+ pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+ if (!pwm) {
+ up_write(pwm_list_lock);
+ return -ENOMEM;
+ }
The allocation attempt can be moved outside the lock, making the code
faster, cleaner and shorter.
+ pwm-pwm_dev = pwm_dev;
+ list_add_tail(pwm-list, di-list);
+ up_write(pwm_list_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(pwm_device_register);
+
+int pwm_device_unregister(struct pwm_device *pwm_dev)
+{
+ struct pwm_dev_info *tmp;
+ struct list_head *pos, *tmp_lst;
+
+ down_write(pwm_list_lock);
+ list_for_each_safe(pos, tmp_lst, di-list) {
+ tmp = list_entry(pos, struct pwm_dev_info, list);
+ if (tmp-pwm_dev == pwm_dev) {
+ list_del(pos);
+ kfree(tmp);
+ up_write(pwm_list_lock);
+ return 0;
+ }
+ }
+ up_write(pwm_list_lock);
+ return -ENOENT;
+}
+EXPORT_SYMBOL(pwm_device_unregister);
+
+struct pwm_device *pwm_request(int pwm_id, const char *name)
+{
+ struct pwm_dev_info *pwm;
+ struct list_head *pos;
+
+ down_read(pwm_list_lock);
+ list_for_each(pos, di-list) {
+ pwm = list_entry(pos, struct pwm_dev_info, list);
+ if ((!strcmp(pwm-pwm_dev-pops-name, name))
+ (pwm-pwm_dev-pwm_id == pwm_id)) {
+ up_read(pwm_list_lock);
+ return pwm-pwm_dev;
+ }
+ }
+ up_read(pwm_list_lock);
+ return ERR_PTR(-ENOENT);
+}
+EXPORT_SYMBOL(pwm_request);
We have a new kernel-wide exported-to-modules formal API. We prefer
that such things be fully documented, please. kerneldoc is a suitable
way but please avoid falling into the kerneldoc trap of filling out
fields with obvious boilerplate and not actually telling people
anything interesting or useful.
+static int __init pwm_init(void)
+{
+ struct pwm_dev_info *pwm;
+
+ pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+ INIT_LIST_HEAD(pwm-list);
+ di = pwm;
+ return 0;
+}
OK, this looks wrong.
AFACIT you've created a dummy pwm_dev_info as a singleton, kernel-wide
anchor for a list of all pwm_dev_info's. So this anchor pwm_dev_info
never actually gets used for anything.
The way to do this is to remove `di' altogether and instead use a
singleton, kernel-wide list_head as the anchor for all the
RE: [PATCHv2 1/7] pwm: Add pwm core driver
On Tue, 5 Oct 2010 17:29:56 +0530
Arun Murthy arun.mur...@stericsson.com wrote:
The existing pwm based led and backlight driver makes use of the
pwm(include/linux/pwm.h). So all the board specific pwm drivers will
be exposing the same set of function name as in include/linux/pwm.h.
Consder a platform with multi Soc or having more than one pwm module,
in
such a case, there exists more than one pwm driver for a platform.
Each
of these pwm drivers export the same set of function and hence leads
to
re-declaration build error.
In order to overcome this issue all the pwm drivers must register to
some core pwm driver with function pointers for pwm operations (i.e
pwm_config, pwm_enable, pwm_disable).
The clients of pwm device will have to call pwm_request, wherein
they will get the pointer to struct pwm_ops. This structure include
function pointers for pwm_config, pwm_enable and pwm_disable.
Have we worked out who will be merging this work, if it gets merged?
I request Samuel to merge this through MFD tree.
...
+struct pwm_dev_info {
+ struct pwm_device *pwm_dev;
+ struct list_head list;
+};
+static struct pwm_dev_info *di;
We could just do
static struct pwm_dev_info {
...
} *di;
+DECLARE_RWSEM(pwm_list_lock);
This can/should be static.
+void __deprecated pwm_free(struct pwm_device *pwm)
+{
+}
Why are we adding a new function and already deprecating it?
Probably this was already addressed in earlier review, but I'm asking
again, because there's no comment explaining the reasons. Lesson
learned, please add a comment.
Oh, I see that pwm_free() already exists. This patch adds a new copy
and doesn't remove the old function. Does this all actually work?
It still needs a comment explaining why it's deprecated.
The existing pwm drivers make use of this function and now I am in the process
of developing a new pwm core driver and align the existing pwm drivers with
this core driver. I was able to align all the existing pwm drivers except the
jz4740 pwm driver in mips. So in order to retain the support for this mips, I
have deprecated this function. This will be removed once jz4740 pwm driver is
aligned with pwm core driver.
Will add the same comments in code.
+ struct pwm_dev_info *pwm;
+
+ down_write(pwm_list_lock);
+ pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+ if (!pwm) {
+ up_write(pwm_list_lock);
+ return -ENOMEM;
+ }
The allocation attempt can be moved outside the lock, making the code
faster, cleaner and shorter.
Will correct this in v3 patch.
+ up_write(pwm_list_lock);
+ return -ENOENT;
+}
+EXPORT_SYMBOL(pwm_device_unregister);
+
+struct pwm_device *pwm_request(int pwm_id, const char *name)
+{
+ struct pwm_dev_info *pwm;
+ struct list_head *pos;
+
+ down_read(pwm_list_lock);
+ list_for_each(pos, di-list) {
+ pwm = list_entry(pos, struct pwm_dev_info, list);
+ if ((!strcmp(pwm-pwm_dev-pops-name, name))
+ (pwm-pwm_dev-pwm_id == pwm_id)) {
+ up_read(pwm_list_lock);
+ return pwm-pwm_dev;
+ }
+ }
+ up_read(pwm_list_lock);
+ return ERR_PTR(-ENOENT);
+}
+EXPORT_SYMBOL(pwm_request);
We have a new kernel-wide exported-to-modules formal API. We prefer
that such things be fully documented, please. kerneldoc is a suitable
way but please avoid falling into the kerneldoc trap of filling out
fields with obvious boilerplate and not actually telling people
anything interesting or useful.
Sure, Will document this as part of v3 patch.
+static int __init pwm_init(void)
+{
+ struct pwm_dev_info *pwm;
+
+ pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+ INIT_LIST_HEAD(pwm-list);
+ di = pwm;
+ return 0;
+}
OK, this looks wrong.
AFACIT you've created a dummy pwm_dev_info as a singleton, kernel-wide
anchor for a list of all pwm_dev_info's. So this anchor pwm_dev_info
never actually gets used for anything.
The way to do this is to remove `di' altogether and instead use a
singleton, kernel-wide list_head as the anchor for all the
dynamically-allocated pwm_dev_info's.
OK, will implement this in v3 patch.
+subsys_initcall(pwm_init);
+
+static void __exit pwm_exit(void)
+{
+ kfree(di);
+}
+
+module_exit(pwm_exit);
+
+MODULE_LICENSE(GPL v2);
+MODULE_AUTHOR(Arun R Murthy);
+MODULE_ALIAS(core:pwm);
+MODULE_DESCRIPTION(Core pwm driver);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 7c77575..6e7da1f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,13 @@
struct pwm_device;
+struct pwm_ops {
+ int (*pwm_config)(struct pwm_device *pwm, int duty_ns, int
period_ns);
+ int (*pwm_enable)(struct pwm_device
