From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Tue, 05 Oct 2010 21:28:17 +0200 (CEST) Received: from smtp1.linux-foundation.org ([140.211.169.13]:51902 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1491024Ab0JET2N (ORCPT ); Tue, 5 Oct 2010 21:28:13 +0200 Received: from imap1.linux-foundation.org (imap1.linux-foundation.org [140.211.169.55]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id o95JMTcX030337 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 5 Oct 2010 12:22:29 -0700 Received: from akpm.mtv.corp.google.com (localhost [127.0.0.1]) by imap1.linux-foundation.org (8.13.5.20060308/8.13.5/Debian-3ubuntu1.1) with SMTP id o95JMP1f014730; Tue, 5 Oct 2010 12:22:25 -0700 Date: Tue, 5 Oct 2010 12:22:25 -0700 From: Andrew Morton To: Arun Murthy Cc: , , , , , , , , , , , , , , Subject: Re: [PATCHv2 1/7] pwm: Add pwm core driver Message-Id: <20101005122225.6dda30ff.akpm@linux-foundation.org> In-Reply-To: <1286280002-1636-2-git-send-email-arun.murthy@stericsson.com> References: <1286280002-1636-1-git-send-email-arun.murthy@stericsson.com> <1286280002-1636-2-git-send-email-arun.murthy@stericsson.com> X-Mailer: Sylpheed 2.4.8 (GTK+ 2.12.9; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.13 Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 27952 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: akpm@linux-foundation.org Precedence: bulk X-list: linux-mips On Tue, 5 Oct 2010 17:29:56 +0530 Arun Murthy 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 dynamically-allocated pwm_dev_info's. > +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 *pwm); > + int (*pwm_disable)(struct pwm_device *pwm); > + char *name; > +}; This also should be documented. > > ... > I suggest that you work on Kevin's comments before making any code changes though. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp1.linux-foundation.org ([140.211.169.13]:51902 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1491024Ab0JET2N (ORCPT ); Tue, 5 Oct 2010 21:28:13 +0200 Date: Tue, 5 Oct 2010 12:22:25 -0700 From: Andrew Morton Subject: Re: [PATCHv2 1/7] pwm: Add pwm core driver Message-ID: <20101005122225.6dda30ff.akpm@linux-foundation.org> In-Reply-To: <1286280002-1636-2-git-send-email-arun.murthy@stericsson.com> References: <1286280002-1636-1-git-send-email-arun.murthy@stericsson.com> <1286280002-1636-2-git-send-email-arun.murthy@stericsson.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org To: Arun Murthy Cc: lars@metafoo.de, kernel@pengutronix.de, philipp.zabel@gmail.com, robert.jarzmik@free.fr, marek.vasut@gmail.com, eric.y.miao@gmail.com, rpurdie@rpsys.net, sameo@linux.intel.com, kgene.kim@samsung.com, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mips@linux-mips.org, STEricsson_nomadik_linux@list.st.com, bgat@billgatliff.com Message-ID: <20101005192225.odJgr-Q0RK_WV81fGvwDCijVGrZxGPWAzYbrBYIuNJo@z> On Tue, 5 Oct 2010 17:29:56 +0530 Arun Murthy 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 dynamically-allocated pwm_dev_info's. > +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 *pwm); > + int (*pwm_disable)(struct pwm_device *pwm); > + char *name; > +}; This also should be documented. > > ... > I suggest that you work on Kevin's comments before making any code changes though. From mboxrd@z Thu Jan 1 00:00:00 1970 From: akpm@linux-foundation.org (Andrew Morton) Date: Tue, 5 Oct 2010 12:22:25 -0700 Subject: [PATCHv2 1/7] pwm: Add pwm core driver In-Reply-To: <1286280002-1636-2-git-send-email-arun.murthy@stericsson.com> References: <1286280002-1636-1-git-send-email-arun.murthy@stericsson.com> <1286280002-1636-2-git-send-email-arun.murthy@stericsson.com> Message-ID: <20101005122225.6dda30ff.akpm@linux-foundation.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 5 Oct 2010 17:29:56 +0530 Arun Murthy 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 dynamically-allocated pwm_dev_info's. > +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 *pwm); > + int (*pwm_disable)(struct pwm_device *pwm); > + char *name; > +}; This also should be documented. > > ... > I suggest that you work on Kevin's comments before making any code changes though.